MCUBoot firmware loader update for NCS#27518
Conversation
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 7b34a697cfd07b9286ba8e1f6c4f084d0f6ddd78 more detailssdk-nrf:
Github labels
List of changed files detected by CI (16)Outputs:ToolchainVersion: 911f4c5c26 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
3bba72b to
295728e
Compare
295728e to
08e1250
Compare
| int rc = 0; | ||
|
|
||
| #if CONFIG_UNBOXER_FW_LOADER_ENTRANCE_SELF_INVALIDATE_BY_ERASE | ||
| rc = self_invalidate_by_erase(source_fa); |
There was a problem hiding this comment.
this rc value assignment requires separate check/log on error.
There was a problem hiding this comment.
I actually think that it is not strictly necessary - there is already a log in self_invalidate_by_erase and we do treat this as a method of "requesting FW loader entrance", so the log below, although not perfect is not really wrong. I'd prefer to keep this as it is so that the code does not get more complicated with more ifdefs
| } | ||
|
|
||
| off += fw_loader_tlv_info.it_tlv_tot; | ||
| *fw_loader_total_size = fw_loader_header.ih_hdr_size |
There was a problem hiding this comment.
calculating of that and the off is quite redundant. See not issue, just notice.
There was a problem hiding this comment.
It is in fact a bit redundant, but I thought that explicitily writing what values sum to *fw_loader_total_size is more informative than off - *fw_loader_data_start_offset
| uint32_t source_offset = fw_loader_data_start_offset; | ||
| uint32_t target_offset = 0; | ||
|
|
||
| rc = flash_area_flatten(target_fa, 0, flash_area_get_size(target_fa)); |
There was a problem hiding this comment.
Actually it should avoid flatten on RRAM memory (this will reduces total lifetime endurance).
code can check flash_area device capability and perform erase if needed.
There was a problem hiding this comment.
I've changed to flash_area_erase (also in self_invalidate_by_erase) - I did not find the capability you mentioned, but I assume all of the nordic devices support flash_area_erase so this does not really seem to be an issue
| LOG_ERR("Failed to clear flash area"); | ||
| return rc; | ||
| } | ||
| while (bytes_left > BUFFER_SIZE) { |
There was a problem hiding this comment.
you can heck whether flash_area_copy() will fit in instead of this code
There was a problem hiding this comment.
Indeed, a much better solution, changed
There was a problem hiding this comment.
Update: simply flash area copy was not enough, as the write alignment issue needed to be addressed. Please take a look now
|
|
||
| set(output ${CMAKE_BINARY_DIR}/fw_loader_unboxer.signed) | ||
|
|
||
| add_custom_command( |
There was a problem hiding this comment.
just for consideration: produce also signed .hex (on Kconfig demand) for verification?
08e1250 to
58a5899
Compare
| CONFIG_LOG_MODE_IMMEDIATE=y | ||
| CONFIG_LOG_MODE_MINIMAL=y |
There was a problem hiding this comment.
these 2 things clash, only one can be selected
There was a problem hiding this comment.
Ok, left only CONFIG_LOG_MODE_IMMEDIATE
| sysbuild_get(CONFIG_INSTALLER_FW_LOADER_ENTRANCE_BOOT_REQ IMAGE ${installer_image} | ||
| VAR CONFIG_INSTALLER_FW_LOADER_ENTRANCE_BOOT_REQ KCONFIG) |
There was a problem hiding this comment.
) goes on newline e.g.:
sysbuild_get(CONFIG_INSTALLER_FW_LOADER_ENTRANCE_BOOT_REQ IMAGE ${installer_image}
VAR CONFIG_INSTALLER_FW_LOADER_ENTRANCE_BOOT_REQ KCONFIG
)
There was a problem hiding this comment.
Fixed in all instances in this file
| sysbuild_get(mcuboot_CONFIG_BOOT_FIRMWARE_LOADER_BOOT_MODE IMAGE mcuboot | ||
| VAR CONFIG_BOOT_FIRMWARE_LOADER_BOOT_MODE KCONFIG) | ||
| if(NOT mcuboot_CONFIG_BOOT_FIRMWARE_LOADER_BOOT_MODE) | ||
| message(WARNING |
There was a problem hiding this comment.
rather than allowing these to mismatch, maybe a Kconfig should be added to sysbuild which sets it in the images?
There was a problem hiding this comment.
Actually looking at this currently these warnings should not be in this file at all, as Kconfigs like CONFIG_INSTALLER_FW_LOADER_ENTRANCE_BOOT_MODE are local to the application.
Removing this whole "configuration checks" part - the proper way to set the Kconfig-s is documented and errors are immediately detectable in runtime.
| dt_partition_addr(code_addr PATH "${code_flash}" TARGET ${installer_image} REQUIRED) | ||
|
|
||
| math(EXPR start_offset_dts "${code_addr} - ${slot0_addr}") | ||
| if(start_offset_dts GREATER 0) | ||
| math(EXPR header_size "${header_size} + ${start_offset_dts}") | ||
| set(pad_header "--pad-header") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
not following this, if pad header is needed then it should always be used, there shouldn't be a case of some things need it and some do not, that is the case with PM vs dts which is caused by PM but there is no PM here
There was a problem hiding this comment.
The code can be a bit simplified here (removed the start_offset_dts GREATER 0) as the "header by DTS offset" should be always used with SB_CONFIG_MCUBOOT_SIGN_MERGED_BINARY - so if you meant the if(start_offset_dts GREATER 0) - removed the if.
The code is copied from sign_nrf54h20.cmake - this case is specifically for the nRF54H20 merged image firmware loader. In this case we do not use CONFIG_ROM_START_OFFSET for the header, instead we offset the cpuapp_slot0_partition from slot0_partition and use this space for the MCUBoot header, see for example here:
Changing this approach would require the rework of the firmware loader merged image functionalities, which would be way out of scope of this PR.
| include(${ZEPHYR_NRF_MODULE_DIR}/cmake/sysbuild/mcuboot_nrf54h20.cmake) | ||
| endif() | ||
|
|
||
| if(SB_CONFIG_FIRMWARE_LOADER_UPDATE_ENABLE) |
There was a problem hiding this comment.
-enable from the option name, that's what enabling a Kconfig implies
There was a problem hiding this comment.
refactored (changed name to SB_CONFIG_FIRMWARE_LOADER_UPDATE)
6ea15ae to
35dda0e
Compare
tejlmand
left a comment
There was a problem hiding this comment.
Much better.
Small prompt proposals.
Only blocking prompt comments are the word Use, and missing help text.
Rest is just additional suggestions, but those are not blocking.
| bool "Use boot mode" | ||
| depends on RETAINED_MEM | ||
| depends on RETENTION | ||
| depends on RETENTION_BOOT_MODE |
There was a problem hiding this comment.
why is there no help text ?
| depends on RETAINED_MEM || SOC_NRF54H20 | ||
| depends on RETENTION || SOC_NRF54H20 | ||
| depends on NRF_MCUBOOT_BOOT_REQUEST | ||
| depends on IMG_MANAGER |
There was a problem hiding this comment.
why is there no help text ?
There was a problem hiding this comment.
Indeed would be helpful, added
bb7ff4b to
914c2f7
Compare
914c2f7 to
5b4f8ac
Compare
| * :kconfig:option:`SB_CONFIG_BOOTLOADER_MCUBOOT` | ||
| * :kconfig:option:`SB_CONFIG_MCUBOOT_MODE_FIRMWARE_UPDATER` | ||
| * :kconfig:option:`SB_CONFIG_FIRMWARE_LOADER_UPDATE` | ||
| This option selects :kconfig:option:`SB_CONFIG_FIRMWARE_LOADER_INSTALLER_BASIC` by default, which causes this application to be built. |
There was a problem hiding this comment.
| This option selects :kconfig:option:`SB_CONFIG_FIRMWARE_LOADER_INSTALLER_BASIC` by default, which causes this application to be built. | |
| This option selects :kconfig:option:`SB_CONFIG_FIRMWARE_LOADER_INSTALLER_BASIC` by default, which causes this application to be built. |
| depends on RETAINED_MEM | ||
| depends on RETENTION | ||
| depends on RETENTION_BOOT_MODE |
There was a problem hiding this comment.
surely it should just be RETENTION_BOOT_MODE since RETENTION_BOOT_MODE depends on RETENTION and RETENTION depends on RETAINED_MEM?
There was a problem hiding this comment.
Leftover from a previous iteration when select was used, left only RETENTION_BOOT_MODE
| depends on RETAINED_MEM || SOC_NRF54H20 | ||
| depends on RETENTION || SOC_NRF54H20 | ||
| depends on NRF_MCUBOOT_BOOT_REQUEST | ||
| depends on IMG_MANAGER |
There was a problem hiding this comment.
| depends on RETAINED_MEM || SOC_NRF54H20 | |
| depends on RETENTION || SOC_NRF54H20 | |
| depends on NRF_MCUBOOT_BOOT_REQUEST | |
| depends on IMG_MANAGER | |
| depends on (RETENTION || SOC_NRF54H20) && NRF_MCUBOOT_BOOT_REQUEST && IMG_MANAGER |
if I read it rightly?
There was a problem hiding this comment.
Changed as suggested
| generate_dfu_zip( | ||
| OUTPUT ${CMAKE_BINARY_DIR}/dfu_fw_loader_installer.zip | ||
| BIN_FILES ${output}.bin | ||
| TYPE application |
There was a problem hiding this comment.
type is application, trying to see what this means, seemingly in cmake/fw_zip.cmake it just passes it on to the script but scripts/bootloader/generate_zip.py doesn't even read it - what consumes this value and what does it do with it?
There was a problem hiding this comment.
The python script for zip generation puts an additional field "type" in the manifest - the mobile app then uses it for differentiating it from the mcuboot update. As the installer upload currently behave exactly like a normal application upload from the mobile app-s perspective - the application type is used here.
|
|
||
| The project must also configure MCUboot to operate in firmware loader mode and specify a firmware loader image in the :file:`sysbuild.conf` file. | ||
| For example to select ``smp_svr``, set the following options: | ||
| For example, to select the ``ble_mcumgr`` firmware loader image, set the following options: |
There was a problem hiding this comment.
Is there documentation for this sample? If so, might want to link to that here
| config FIRMWARE_LOADER_UPDATE | ||
| bool "Firmware loader update" | ||
| depends on MCUBOOT_MODE_FIRMWARE_UPDATER && !FIRMWARE_LOADER_IMAGE_NONE | ||
| help | ||
| Firmware loader update. | ||
| This will allow to update the firmware loader image with the help of the installer image. | ||
|
|
||
| if FIRMWARE_LOADER_UPDATE | ||
|
|
||
| choice FIRMWARE_LOADER_INSTALLER | ||
| prompt "Firmware loader installer image" | ||
| default FIRMWARE_LOADER_INSTALLER_BASIC | ||
|
|
||
| config FIRMWARE_LOADER_INSTALLER_BASIC | ||
| bool "Basic fw loader installer" | ||
|
|
||
| endchoice | ||
|
|
||
| config FIRMWARE_LOADER_INSTALLER_IMAGE_NAME | ||
| string | ||
| default "installer" if FIRMWARE_LOADER_INSTALLER_BASIC | ||
|
|
||
| config FIRMWARE_LOADER_INSTALLER_IMAGE_PATH | ||
| string | ||
| default "$(ZEPHYR_NRF_MODULE_DIR)/applications/installer" if FIRMWARE_LOADER_INSTALLER_BASIC |
There was a problem hiding this comment.
these things should all match the image name, so FIRMWARE_LOADER_INSTALLER_BASIC would mean installer_basic etc. easier to just rename to FIRMWARE_LOADER_INSTALLER
There was a problem hiding this comment.
Ok, changed - I had to change the name of the choice itself (it used to be FIRMWARE_LOADER_INSTALLER, now it is FIRMWARE_LOADER_INSTALLER_APPLICATION) and changed the option name to FIRMWARE_LOADER_INSTALLER as suggested
5b4f8ac to
bdb78fb
Compare
b3de8bf to
c27391c
Compare
FrancescoSer
left a comment
There was a problem hiding this comment.
Approving but please implement the previous suggestions
| The following are the DFU steps for updating the firmware loader image. | ||
|
|
||
| .. figure:: ../images/dfu_steps_firmware_loader_update.svg | ||
| :alt: DFU operations for a firmware loader image update in MCUboot firmware-loader mode |
There was a problem hiding this comment.
This should probably be added as a separate task, as it needs new pictures to be created from scratch.
3da9898 to
debce80
Compare
| /applications/matter_weather_station/*.rst @nrfconnect/ncs-matter-doc | ||
| /applications/nrf5340_audio/**/*.rst @nrfconnect/ncs-audio-doc | ||
| /applications/nrf_desktop/**/*.rst @nrfconnect/ncs-si-xcake-doc | ||
| /applications/installer/ @nrfconnect/ncs-eris |
There was a problem hiding this comment.
Perhaps this could be moved up to line 39
|
|
||
| * Added support for the nRF54H20 SoC. | ||
|
|
||
| * Added support for firmware loader updates using the :ref:`installer` application. |
There was a problem hiding this comment.
Shouldn't we have this as a new application under Applications?
There was a problem hiding this comment.
Added, I assume that if IPC radio is there this should be also here, although this is a helper application used specifically from the MCUBoot firmware loader update, so this is not really a separate entity - take a look if the added changelog entry is OK.
| @@ -0,0 +1,50 @@ | |||
| .. _installer: | |||
|
|
|||
| Installer | |||
There was a problem hiding this comment.
Should the name be more specific?
There was a problem hiding this comment.
The "installer" name was used in the NCS baremetal and we were asked by PMT to used the same name - however, I've added "(MCUboot Firmware Loader installer)" in parentheses - this should make the purpose clearer
524ea74 to
cda75c5
Compare
This commit adds support for FW loader updates, using a dedicated installer application. Signed-off-by: Artur Hadasz <artur.hadasz@nordicsemi.no>
cda75c5 to
7b34a69
Compare
Memory footprint analysis revealed the following potential issuesapplications.hpf.gpio.icmsg[nrf54l15dk/nrf54l15/cpuflpr]: High RAM usage: 9014[B] - link (cc: @nrfconnect/ncs-ll-ursus) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-27518/42) |
This PR adds support for FW loader updates, using a dedicated installer application. The idea of the installer application is similar to the installer application from nrf-bm (copy the attached FW loader to the target location) but the differences are significant (see attached table):