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_recovery: Add image hash support #1644

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

nordicjm
Copy link
Collaborator

Adds support for outputting the image hash TLV in serial recovery mode, which is needed to comply with the img_mgmt MCUmgr group requirements.

Fixes #1581

Test file upload and listing from mcuboot:

Images:
 image=0 slot=0
    version: 0.0.0.0
    bootable: false
    flags: 
    hash: 73fc86f2aee87d13620ca718b851e524a01cd6d20aede1672ed3e31457b35d24
Split status: N/A (0)

And checking from actual booted image:

Images:
 image=0 slot=0
    version: 0.0.0
    bootable: true
    flags: active confirmed
    hash: 73fc86f2aee87d13620ca718b851e524a01cd6d20aede1672ed3e31457b35d24
Split status: N/A (0)

@nordicjm
Copy link
Collaborator Author

@de-nordic

@nordicjm nordicjm changed the title boot: serial_recovery: Add image hash support [DNM[ boot: serial_recovery: Add image hash support Mar 13, 2023
@de-nordic de-nordic requested a review from utzig March 16, 2023 09:43
@de-nordic
Copy link
Collaborator

Adds support for outputting the image hash TLV in serial recovery mode, which is needed to comply with the img_mgmt MCUmgr group requirements.

We should start relaxing SMP requirements for MCUboot or have some alternative simplified version, because although this completes SMP protocol requirement for image status in reality it does not add that much of a value, as this is recovery protocol and should be rarely used, at the cost of flash.

@nordicjm
Copy link
Collaborator Author

We should start relaxing SMP requirements for MCUboot or have some alternative simplified version, because although this completes SMP protocol requirement for image status in reality it does not add that much of a value, as this is recovery protocol and should be rarely used, at the cost of flash.

I did check the flash before and after and the size increase was minimal (will recheck and get concrete numbers). This is needed where updatable images numbers is > 1 but seems like a useful enough feature even when it is 1 to see if the image was uploaded properly

@nordicjm
Copy link
Collaborator Author

nrf52840dongle_nrf52840 with:

Memory region         Used Size  Region Size  %age Used
           FLASH:       57188 B        60 KB     93.08%
             RAM:       36928 B       256 KB     14.09%
        IDT_LIST:          0 GB         2 KB      0.00%

without:

Memory region         Used Size  Region Size  %age Used
           FLASH:       57072 B        60 KB     92.89%
             RAM:         36 KB       256 KB     14.06%
        IDT_LIST:          0 GB         2 KB      0.00%

116 byte flash increase
64 byte RAM increase

@utzig
Copy link
Member

utzig commented Mar 16, 2023

I don't think it's a problem adding the hash, I just think it's useless in this scenario, which is probably why it was not added originally. I think it's even debatable if list should be implemented at all, In my opinion, and I might be missing something here, only upload with a pre-erase would be required for recovery. That said I am fine with the change.

@nordicjm
Copy link
Collaborator Author

It's probably not useful for serial recovery for the main core, but serial recovery can be used for more than that, e.g. storing images for other cores or other CPUs, in those instances it's useful as you can then query which images are presently loaded on a device, especially if there is a requirement for e.g. 2 cores to have a set of link images because of incompatibilities with different versions and there is a possibility that power can be lost whilst one of the updates is being transferred.

@de-nordic
Copy link
Collaborator

@nordicjm Can we have some MCUBOOT_CONFIG option turning this off? 100 bytes in flash may not be much, but we struggle to fit other stuff and things keep to add up.
I do not think RAM in MCUboot is that important, as we have the whole device until app boots.

@nordicjm
Copy link
Collaborator Author

@nordicjm Can we have some MCUBOOT_CONFIG option turning this off? 100 bytes in flash may not be much, but we struggle to fit other stuff and things keep to add up.

Added

@nordicjm nordicjm changed the title [DNM[ boot: serial_recovery: Add image hash support boot: serial_recovery: Add image hash support Mar 21, 2023
Adds support for outputting the image hash TLV in serial recovery
mode, which is needed to comply with the img_mgmt MCUmgr group
requirements.

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@de-nordic de-nordic requested a review from pepe2k March 22, 2023 08:54
@de-nordic de-nordic merged commit 827118f into mcu-tools:main Mar 23, 2023
@nordicjm nordicjm deleted the addhash branch December 12, 2023 15:01
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.

zephyr: serial recovery: Missing required hash field in state of images response
4 participants