Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add example of OTA update using http client #559

Merged
merged 7 commits into from
Feb 10, 2025

Conversation

EmrysMyrddin
Copy link
Contributor

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo fmt command to ensure that all changed code is formatted correctly.
  • I have used cargo clippy command to ensure that all changed code passes latest Clippy nightly lints.
  • My changes were added to the CHANGELOG.md in the proper section.

Pull Request Details 📖

Description

The documentation around OTA is very limited, and you have to make a lot of research on your own.

This PR introduces the example I would have been happy to find when implementing OTA updates on my project :-)

Testing

I've copy pasted the code from my working project. Not sure if there are unit tests for examples in this repo ?

Comments

I haven't added a changelog entry, not sure the addition of an example is actually a change :-P

@EmrysMyrddin EmrysMyrddin marked this pull request as ready for review February 2, 2025 21:54
@Vollbrecht
Copy link
Collaborator

Thanks for the effort. A little more documentation can always help!

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the example looks quite good already! I have two suggestions - one to more heavily document the bootloader-based rollback mechanism and the other - to demo EspFirmweareInfoLoad whereas the second thing I believe is important.

examples/ota_http_client.rs Outdated Show resolved Hide resolved
examples/ota_http_client.rs Outdated Show resolved Hide resolved
examples/ota_http_client.rs Outdated Show resolved Hide resolved
@EmrysMyrddin
Copy link
Contributor Author

By browsing the ESP-IDF documentation, it appears the even have a native function that is handling the update process over HTTP automatically. You just provide an URI and it does all the process for you.

It has been not been exposed on purpose ? If not, I would be happy to make a PR for it :-)

@ivmarkov
Copy link
Collaborator

ivmarkov commented Feb 3, 2025

By browsing the ESP-IDF documentation, it appears the even have a native function that is handling the update process over HTTP automatically. You just provide an URI and it does all the process for you.

It has been not been exposed on purpose ? If not, I would be happy to make a PR for it :-)

Sure, but this function is binding the EspOta api to the ESP IDF HTTP client. Not all folks might be using the ESP IDF HTTP client in the first place (I'm myself using edge-http instead). Also, not all users might choose HTTP as the channel for delivering the OTA image.

In other words, the API you want to expose is too opinionated. I don't mind you working on exposing it, but I don't think the existing EspOta API can be superseded in any way by it, as the exposed API is generic and agnostic as to from where and how you pull the new OTA image (HTTP or something else), while the function you have in mind is not.

@EmrysMyrddin
Copy link
Contributor Author

In other words, the API you want to expose is too opinionated. I don't mind you working on exposing it, but I don't think the existing EspOta API can be superseded in any way by it, as the exposed API is generic and agnostic as to from where and how you pull the new OTA image (HTTP or something else), while the function you have in mind is not.

Yes I totally agree ! It's more a util function that is ready to use for those how don't want manually implement it :-)
I will see if it is easy to expose, otherwise the code for making it manually is not that complex anyway.

@EmrysMyrddin
Copy link
Contributor Author

I need some help for using esp_app_desc.version :-) From what I understood, it a CStr, but I'm a bit lost about how to obtain a &str from it. Is there a way to do it without relying on unsafe keyword ?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Feb 3, 2025

I need some help for using esp_app_desc.version :-) From what I understood, it a CStr, but I'm a bit lost about how to obtain a &str from it. Is there a way to do it without relying on unsafe keyword ?

What are you trying to do? The esp_app_desc! macro does not take any parameters and sets your version to the one you have for your package in Cargo.toml. Furthermore, the EspFirmwareInfoLoad::fetch method populates directly a FirmwareInfo struct where everything is with &str or heapless::String already. With that said, just browse the CStr API.

@EmrysMyrddin
Copy link
Contributor Author

I must misunderstand what esp_app_desc! is doing ^^'
It's not supposed to allow me to get the firmware version ? as pointed out by @Vollbrecht ?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Feb 3, 2025

I must misunderstand what esp_app_desc! is doing ^^' It's not supposed to allow me to get the firmware version ? as pointed out by @Vollbrecht ?

It is defining the firmware version.
It is just putting a pre-populated esp_app_desc_t structure in a well-defined place into your firmware app image, which you can then read using EspFirmwareInfoLoad; and which is also read by the EspOta methods, which are returning it to you in the Slot structure. Read this https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/system/app_image_format.html#application-description and overall the whole page.

@EmrysMyrddin
Copy link
Contributor Author

Hooo that's makes sense ! That's why the version was not correct in the running slot when I checked ! Thank you for you help on all of this ^^'

@EmrysMyrddin
Copy link
Contributor Author

BTW, when i add esp_app_desc!, it generate tons of warning about unexpected cfg condition name. Is it expected ? There isn't much documentation around this macro, so I'm not sure i miss some setup.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Feb 3, 2025

BTW, when i add esp_app_desc!, it generate tons of warning about unexpected cfg condition name. Is it expected ? There isn't much documentation around this macro, so I'm not sure i miss some setup.

Yes, this is a long standing issue we have to fix at some point - nothing to do with esp_app_desc! per se. Just add #![cfg(allow(unexpected_cfgs))] in your lib.rs or main.rs.

@EmrysMyrddin
Copy link
Contributor Author

Ok, I think I've resolved all your comments ?

BTW, I'm not sure why the --bootloader option is not set by default in the .cargo/config.yml by default in the template ? I struggled to find out why my bootloader was not updated while changing settings in sdkconfig.defaults ^^'

@EmrysMyrddin EmrysMyrddin requested a review from ivmarkov February 3, 2025 14:41
@ivmarkov
Copy link
Collaborator

ivmarkov commented Feb 4, 2025

Sorry for the delay, could not get to it today. Will take a look tmr.

@EmrysMyrddin
Copy link
Contributor Author

No problem, there is no rush :-) It's just documentation !

@ivmarkov
Copy link
Collaborator

Thanks again!

@ivmarkov ivmarkov merged commit 15febb1 into esp-rs:master Feb 10, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants