Add application description for ESP-IDF image format #3124
Conversation
| @@ -180,6 +180,26 @@ fn main() -> Result<(), Box<dyn Error>> { | |||
| copy_dir_all(&config_symbols, "ld/sections", &out)?; | |||
| copy_dir_all(&config_symbols, format!("ld/{device_name}"), &out)?; | |||
|
|
|||
| // supply build time and date | |||
| // see https://reproducible-builds.org/docs/source-date-epoch/ | |||
| let ts = match std::env::var("SOURCE_DATE_EPOCH") { | |||
There was a problem hiding this comment.
this is obviously not the time when some source files got touched for the last time but when the build script was run
I guess that is fine since production builds are usually clean builds
The app name/version doesn't have to be the real value. We could use esp-config to let the user configure these values independent of their Cargo.toml (which isn't ideal, but they would have the option to set it without the macro), or we could provide an esp-config switch to allow the user to call the macro themselves with data from their Cargo.toml. My point is, we could default to a "something exists that we partially fake". |
|
I'm not even sure this should be a macro (it's more or less just the macro from esp-idf-hal) We could do it all in the build.rs - which would work if we only honor config-values We can also consider to get all the fields (including build time) via the config. (And we could change the esp-generate generated build.rs to set the fields automatically by default - in a follow up step) 🤔 |
|
Lets discuss how to proceed here - as said I'm not in favor of a macro at all |
| @@ -202,6 +202,14 @@ pub use procmacros::load_lp_code; | |||
| #[cfg_attr(not(feature = "unstable"), allow(unused))] | |||
| pub use procmacros::{handler, ram}; | |||
|
|
|||
| // used by a macro | |||
| #[doc(hidden)] | |||
| pub const BUILD_TIME: &str = env!("BUILD_TIME"); | |||
There was a problem hiding this comment.
Can you put these inside the __macro_implementation module?
|
|
||
| /// Macro to define the application descriptor. | ||
| #[macro_export] | ||
| macro_rules! esp_app_desc { |
There was a problem hiding this comment.
This does feel a bit odd living here in the HAL, but I'm not sure where else it should go either 🤔. I suppose we should also mark it as unstable for now?
There was a problem hiding this comment.
t.b.h. I am not sure I like to have this as a macro at all
My other idea would be to generate the app-desc in build.rs and let the linker include it - but then we would need to get the app-name and app-version from a config option instead (or play some dirty tricks to get the binary's manifest)
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 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
This adds the
esp_app_descmacro which defines the app descriptor from information which either gathered from Cargo or is hardcoded for now.Since we get e.g. the package name and version we cannot just put this macro in
esp_hal::initbut the user needs to use it in the binary crate.While the macro is quite straight forward placing the app-descriptor where the bootloader expects it required some changed to the linker scripts. I tested a couple of scenarios and hopefully nothing broke
Closes #3058
Testing
I tested with a custom made esp-idf bootloader. In theory you could just enable some security features which require the app-desc but for me that always resulted in a bootloader which is too big. In the end I just changed the bootloader code to load and parse the app-descriptor.
Alternatively, bootloaders from later esp-idf versions always get the app-descriptor (so just update esp-idf and try a recent bootloader)