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

feat: enhance bootloader for multiple flash support #2540

Merged
merged 7 commits into from
Feb 9, 2024
Merged

feat: enhance bootloader for multiple flash support #2540

merged 7 commits into from
Feb 9, 2024

Conversation

badrbouslikhin
Copy link
Contributor

@badrbouslikhin badrbouslikhin commented Feb 6, 2024

Summary

This pull request introduces a enhancement to the BootLoaderConfig and FirmwareUpdaterConfig in the embassy-boot crate. The primary goal is to implement support for multiple flash partitions, enabling the bootloader and firmware updater to handle ActiveFlash, DFUFlash, and StateFlash independently. This change is particularly beneficial for systems with dual-bank or external flash setups.

Key Changes

  • Configurations Refactored: Both BootLoaderConfig and FirmwareUpdaterConfig have been updated to support different flash partition types.
  • Partition Handling: Adjustments made in various modules for alignment with the new multi-flash support, specifically in the instantiation of BlockingPartition and Partition.
  • bootloader/stm32 and bootloader/stm32wb-dfu have been updated and a new example bootloader/stm32-dual-bank has been introduced.

Testing

  • The changes have been tested in the blocking flavor on an STM32H747XI.

TODO

  • Update all examples
  • Test on other targets (is the CI capable of performing this?)
  • Improve from_linkerfile_blocking api?

Your review would be greatly appreciated!

Specifically, I am not fully satisfied with the from_linkerfile_blocking when the same flash is used:

let config = BootLoaderConfig::from_linkerfile_blocking(&flash, &flash, &flash);

I see two ways to handle this: a builder pattern or wrapping &flash with an Option.

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this. The previous incarnation of the config did support multiple flashes but somehow it lost this capability. I've usually resorted to do this outside embassy-boot (https://github.com/lulf/watchful/blob/main/firmware/boot/src/main.rs#L68), but having this supported 'natively' is good.

@badrbouslikhin
Copy link
Contributor Author

badrbouslikhin commented Feb 6, 2024

Ah, that explains why the Embassy bootloader doc. page says: the bootloader supports both internal and external flash!

Once I get your thoughts on the changes and from_linkerfile_blocking api, I'll proceed with the rest :).

>
{
/// Create a bootloader config from the flash and address symbols defined in the linkerfile
// #[cfg(target_os = "none")]
pub fn from_linkerfile_blocking(flash: &'a Mutex<NoopRawMutex, RefCell<FLASH>>) -> Self {
pub fn from_linkerfile_blocking(
Copy link
Member

Choose a reason for hiding this comment

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

Personally I don't think this API is too bad. Using a builder doesn't provide that much gain, but perhaps a better rustdoc order explaining the parameters would be appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 634c409.

@badrbouslikhin
Copy link
Contributor Author

badrbouslikhin commented Feb 7, 2024

I updated and compiled all examples.
Some of them required changes in the memory.x most likely due to recent code inflation (they were not compiling before this change). I adjusted linker files but I do not have the boards to validate the change.

Side note: is it useful to maintain that many boot/application STM32 examples? They look very similar.

I am not sure how to fix CI failures though. Does it test examples on hardware?

@lulf
Copy link
Member

lulf commented Feb 7, 2024

There is no HIL test for the bootloader, but any examples should be added to the ci.sh to ensure they always compile. It's unfortunate that the size increased previously though, and it goes undetected because in ci it compiles with --features skip-include to avoid the double pass.

The ci/test is failing on the unit tests in the bootloader though https://ci.embassy.dev/jobs/d45560625688

@badrbouslikhin
Copy link
Contributor Author

badrbouslikhin commented Feb 7, 2024

Alright, CI is happy now.
Thanks for the review! Let me know if there's anything else I need to do.

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@lulf lulf added this pull request to the merge queue Feb 9, 2024
Merged via the queue into embassy-rs:main with commit c4f3e0d Feb 9, 2024
6 checks passed
@badrbouslikhin badrbouslikhin deleted the bootloader-modular-flash-partitions branch February 9, 2024 22:12
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.

2 participants