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

boot_serial: Add optional img mgmt slot info feature #2024

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

nordicjm
Copy link
Collaborator

@nordicjm nordicjm commented Jul 26, 2024

Adds a minimal version of the slot info feature to serial recovery,
and enables it by default. This feature adds an additional 688 bytes
of flash usage and 16 bytes of RAM usage on a cortex M33 device

boot/boot_serial/src/boot_serial.c Show resolved Hide resolved
boot/boot_serial/src/boot_serial.c Show resolved Hide resolved
boot/boot_serial/src/boot_serial.c Show resolved Hide resolved
boot/boot_serial/src/boot_serial.c Outdated Show resolved Hide resolved
boot/boot_serial/src/boot_serial.c Outdated Show resolved Hide resolved
@@ -202,4 +202,11 @@ config BOOT_SERIAL_IMG_GRP_IMAGE_STATE
If y, image states will be included with image lists and the set
state command can be used to mark an image as test/confirmed.

config BOOT_SERIAL_IMG_GRP_SLOT_INFO
bool "Slot info"
default y
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure we should bring it by default from the beginning; this increases flash and and surprised developers will be forced to check what happened and disable the feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about default if updateable images > 1? The problem that always comes up is (with multi image devices) knowing what "slot" to use to upload to, on single image devices yes this feature is mostly useless (maybe max image size is useful) but for multi image devices especially those that support uploading via MCUboot and application, it gives clarity over where to upload

Copy link
Collaborator Author

@nordicjm nordicjm Aug 1, 2024

Choose a reason for hiding this comment

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

Have moved it to be > 1 image. Though had to add a fix for the app size not being populated (it is populated if you enter serial recovery by way of there being no application which I was testing before, but not through button press). Unfortunately because you now need to duplicate the sector data, it has significantly bumped RAM.
Without:
With:
+724 bytes flash, +4112 bytes RAM. Will ask David if we can just make it static in the file or add a define for it because that is pretty ridiculous
Have found a workaround, back to normal:
Without:

Memory region         Used Size  Region Size  %age Used
           FLASH:       43188 B        64 KB     65.90%
             RAM:       28448 B       448 KB      6.20%

With:

Memory region         Used Size  Region Size  %age Used
           FLASH:       43912 B        64 KB     67.00%
             RAM:       28464 B       448 KB      6.20%

+724 byte flash, +16 bytes RAM

boot/boot_serial/src/boot_serial.c Show resolved Hide resolved
boot/boot_serial/src/boot_serial.c Outdated Show resolved Hide resolved
@nordicjm nordicjm force-pushed the mcumgrimgslots branch 6 times, most recently from b4f205a to 18b5624 Compare August 1, 2024 08:33
@nordicjm nordicjm requested a review from de-nordic August 1, 2024 08:35
Adds a minimal version of the slot info feature to serial recovery,
and enables it by default.

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
Adds release notes on the addition of this feature

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm nordicjm merged commit 148c2c1 into mcu-tools:main Aug 13, 2024
58 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