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

mgmt: mcumgr: grp: os: Add active b0 slot bootloader info query #19714

Merged
merged 4 commits into from
Jan 24, 2025

Conversation

nordicjm
Copy link
Contributor

@nordicjm nordicjm commented Dec 30, 2024

Allows querying the active b0 slot via MCUmgr so that the correct
update image can be loaded to a device

However, FW_INFO is flawed in that it does no image verification at all and just checks the struct, the whole image could have been marked as invalid by b0 and so the application can return the wrong result

@nordicjm nordicjm requested review from a team as code owners December 30, 2024 09:16
@github-actions github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Dec 30, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Dec 30, 2024

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
zephyr nrfconnect/sdk-zephyr@76a46cb nrfconnect/sdk-zephyr@763e6a8 (main) nrfconnect/[email protected]

All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Dec 30, 2024

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 10

Inputs:

Sources:

sdk-nrf: PR head: 16ea1c38b318987a4821c42467b6a3a5788e1ca7
zephyr: PR head: 763e6a8ab82bd1a346a9b14a4b8fc7509c8441b6

more details

sdk-nrf:

PR head: 16ea1c38b318987a4821c42467b6a3a5788e1ca7
merge base: 01b2deb38dd845f10c4b9c3e4bd94030dd9a93db
target head (main): 01b2deb38dd845f10c4b9c3e4bd94030dd9a93db
Diff

zephyr:

PR head: 763e6a8ab82bd1a346a9b14a4b8fc7509c8441b6
merge base: 76a46cb2baf310a06bbdb0b1d7d99394ec04a671
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (14)
cmake
│  ├── sysbuild
│  │  │ b0_mcuboot_signing.cmake
subsys
│  ├── CMakeLists.txt
│  ├── Kconfig
│  ├── mgmt
│  │  ├── mcumgr
│  │  │  ├── CMakeLists.txt
│  │  │  ├── Kconfig
│  │  │  ├── grp
│  │  │  │  ├── CMakeLists.txt
│  │  │  │  ├── Kconfig
│  │  │  │  ├── os_mgmt
│  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  ├── Kconfig
│  │  │  │  │  ├── src
│  │  │  │  │  │  │ os_mgmt_b0_active_slot.c
sysbuild
│  │ CMakeLists.txt
west.yml
zephyr
│  ├── include
│  │  ├── zephyr
│  │  │  ├── mgmt
│  │  │  │  ├── mcumgr
│  │  │  │  │  ├── grp
│  │  │  │  │  │  ├── os_mgmt
│  │  │  │  │  │  │  │ os_mgmt.h
│  ├── subsys
│  │  ├── mgmt
│  │  │  ├── mcumgr
│  │  │  │  ├── grp
│  │  │  │  │  ├── os_mgmt
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  │ os_mgmt.c

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
  • ❌ Integration tests
    • ✅ test-sdk-audio
    • ✅ desktop52_verification
    • ✅ test-fw-nrfconnect-boot
    • ✅ test-fw-nrfconnect-apps
    • ✅ test_ble_nrf_config
    • ✅ test-fw-nrfconnect-ble_mesh
    • ✅ test-fw-nrfconnect-ble_samples
    • ✅ test-fw-nrfconnect-chip
    • ✅ test-fw-nrfconnect-nfc
    • ✅ test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • ✅ test-fw-nrfconnect-nrf-iot_samples
    • ✅ test-fw-nrfconnect-nrf-iot_lwm2m
    • ✅ doc-internal
    • ✅ test-fw-nrfconnect-nrf-iot_thingy91
    • ✅ test-fw-nrfconnect-nrf_crypto
    • ✅ test-fw-nrfconnect-rpc
    • ✅ test-fw-nrfconnect-rs
    • ✅ test-fw-nrfconnect-fem
    • ✅ test-fw-nrfconnect-tfm
    • ✅ test-fw-nrfconnect-thread
    • ✅ test-fw-nrfconnect-zigbee
    • ✅ test-sdk-find-my
    • ✅ test-fw-nrfconnect-nrf-iot_mosh
    • ❌ test-fw-nrfconnect-nrf-iot_positioning
    • ✅ test-sdk-sidewalk
    • ✅ test-sdk-wifi
    • ✅ test-low-level
    • ✅ test-sdk-pmic-samples
    • ✅ test-sdk-mcuboot
    • ✅ test-sdk-dfu
    • ✅ test-fw-nrfconnect-ps
    • ✅ test-secdom-samples-public
    • ⚠️ test-fw-nrfconnect-fw-update

Note: This message is automatically posted and updated by the CI

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

return MGMT_CB_ERROR_RC;
}

/* If no parameter is recognized then just introduce the bootloader. */
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems copy-pasted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment removed

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 9, 2025

Memory footprint analysis revealed the following potential issues

sample.matter.template.debug[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 912194[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic)
sample.matter.template.release[nrf7002dk/nrf5340/cpuapp]: High ROM usage: 821134[B] - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic)

Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-19714/10)

@@ -138,7 +138,7 @@ if(SB_CONFIG_BOOTLOADER_MCUBOOT)

if(SB_CONFIG_SECURE_BOOT_BUILD_S1_VARIANT_IMAGE)
ncs_secure_boot_mcuboot_sign(s1_image "${bin_files}" "${signed_targets}" "")
set(extra_bin_data "signed_by_mcuboot_and_b0_s1_image.binload_address=$<TARGET_PROPERTY:partition_manager,PM_S1_ADDRESS>")
set(extra_bin_data "signed_by_mcuboot_and_b0_s1_image.binload_address=$<TARGET_PROPERTY:partition_manager,PM_S1_ADDRESS>;signed_by_mcuboot_and_b0_s1_image.binslot=1")
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit.

Very long line before, and even much longer now 😞

uint32_t active_slot;
bool ok;

if (!s1_info || (s0_info && s0_info->version >= s1_info->version)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is the !s1_info needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's possible that there is no s1 image loaded in which case s1 would be a null pointer so the check is needed.

active_slot = 0;
} else if (!s0_info || (s1_info && s1_info->version > s0_info->version)) {
active_slot = 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming that at this point we can have both s0_info and s1_info and the above else and else if have not been triggered, with the !s0_info and !s1_info, we have no processing of situation where both s0_info && s1_info, in which case active_slot is left uninitialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there must be an s0_info or s1_info for b0 to be able to boot the image so one of the conditions must run, if there is no s0_info and no s1_info and b0 has booted the image then something is very wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, my bad.

This can be used by the application to know if MCUboot and b0 are
enabled, though the slot itself cannot be used from the application

Signed-off-by: Jamie McCrae <[email protected]>
Includes an update to add a new MCUmgr OS group error code

Signed-off-by: Jamie McCrae <[email protected]>
Allows querying the active b0 slot via MCUmgr so that the correct
update image can be loaded to a device

Signed-off-by: Jamie McCrae <[email protected]>
Adds slot numbers to the manifest used when building MCUboot
updates for b0

Signed-off-by: Jamie McCrae <[email protected]>
@NordicBuilder NordicBuilder removed the DNM label Jan 23, 2025
@carlescufi carlescufi merged commit c73baaf into nrfconnect:main Jan 24, 2025
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. manifest manifest-zephyr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants