Skip to content

Improve app-descriptor detection #975

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.

Sign up for GitHub

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

Merged
SergioGasquez merged 5 commits into from
Nov 27, 2025
Merged

Improve app-descriptor detection #975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bf77b30
Improve app-descriptor detection
bjoernQ Nov 26, 2025
7fa5fb4
CHANGELOG.md
bjoernQ Nov 26, 2025
28340be
Support esp-hal app with `strip=true`
bjoernQ Nov 26, 2025
7414168
Clarify
bjoernQ Nov 26, 2025
1fd5b59
Fix rebase
bjoernQ Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Corrected eFuse BLOCK0 definitions for ESP32-C2, ESP32-C3, and ESP32-S3 (#961)
- Fixed Secure Download Mode detection on ESP32-P4 (#972)
- Several fixes in `read_efuse` (#969)
- Fixed a problem in detecting the app-descriptor for a project if `strip = true` is used (#975)

### Removed

Expand Down
11 changes: 9 additions & 2 deletions espflash/src/image_format/idf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,10 +809,17 @@ where
pub fn check_idf_bootloader(elf_data: &Vec<u8>) -> Result<()> {
Copy link
Contributor

@bugadani bugadani Nov 28, 2025

Choose a reason for hiding this comment

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

I've just tried using this function in probe-rs to fix probe-rs/probe-rs#3564 and come on why are we returning a miette diagnostic from this...?

Copy link
Contributor Author

@bjoernQ bjoernQ Nov 28, 2025

Choose a reason for hiding this comment

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

we should change that! maybe there are more places like this?

Copy link
Contributor Author

@bjoernQ bjoernQ Nov 28, 2025

Choose a reason for hiding this comment

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

but ... probably for 5.x since this is already published public API :(

Copy link
Contributor

@bugadani bugadani Nov 28, 2025

Choose a reason for hiding this comment

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

OK I've managed to work around it by this line - turning the diagnostic back into a string, and wrapping it into the espflash::Error::AppDescriptorNotPresent error, which it was supposed to be in the first place...

Copy link
Contributor

@bugadani bugadani Nov 28, 2025

Choose a reason for hiding this comment

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

We either shouldn't use miette in the library, or embrace it and don't expose espflash's error type, but this hybrid mess is just sad.

but ... probably for 5.x since this is already published public API :(

We can extract the implementation into another function that returns the Result<(), Error> type, and just call that().into_diagnostic() in the current function.

Copy link
Contributor Author

@bjoernQ bjoernQ Nov 28, 2025

Choose a reason for hiding this comment

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

We either shouldn't use miette in the library, or embrace it and don't expose espflash's error type, but this hybrid mess is just sad.

+1 ... we probably shouldn't embrace it but scrape it from the API surface

We can extract the implementation into another function that returns the Result<(), Error> type, and just call that().into_diagnostic() in the current function.

sure - and deprecate the current function for now

Might be worth a separate issue ... pretty sure comments on a merged PR will be lost in an instant

let object = File::parse(elf_data.as_slice()).into_diagnostic()?;

let has_app_desc = object.symbols().any(|sym| sym.name() == Ok("esp_app_desc"));
let is_esp_hal = object.section_by_name(".espressif.metadata").is_some();
// A project with `strip = true` will discard the
// symbol we are looking for but the section is kept
let has_app_desc = object.symbols().any(|sym| sym.name() == Ok("esp_app_desc"))
|| object.section_by_name(".flash.appdesc").is_some()
|| object.section_by_name(".rodata_desc").is_some();

if !has_app_desc {
// when using `strip = true` we will (maybe wrongly) assume ESP-IDF
// but at least it should still guide the user into the right direction
let is_esp_hal = object.section_by_name(".espressif.metadata").is_some();

if is_esp_hal {
return Err(Error::AppDescriptorNotPresent(
"ESP-IDF App Descriptor (https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/system/app_image_format.html#application-description) missing in your`esp-hal` application.\n
Expand Down