Skip to content

Conversation

@bleska
Copy link
Contributor

@bleska bleska commented Nov 17, 2025

This PR resolves #6043 issue.

The nRF FW is divided into 2 parts: MCUboot and application. Both parts are flashed as a merged binary file at the production line where the MCUboot is immutable and application ca be updated throughout the time. The update in the field is performed by MCU FW which embeds the nRF application inside. The decision whether an update is needed is based on MCU to nRF application SPI communication and hash comparison. In case it fails, there is no information available for decision => it was assumed that nRF app image is corrupted and update is needed.

The PR leaves the MCU to nRF SPI communication and has comparison intact but offers another option of nRF app version readout via nRF MCUboot by using UART interface with MCUboot's SMP serial recovery feature see https://docs.nordicsemi.com/bundle/ncs-latest/page/zephyr/services/device_mgmt/smp_groups/smp_group_1.html . Specifically "Get state of images request/response".

@bleska bleska self-assigned this Nov 17, 2025
@bleska bleska added this to Firmware Nov 17, 2025
@bleska bleska added the T3W1 Trezor Safe 7 label Nov 17, 2025
@github-project-automation github-project-automation bot moved this to 🔎 Needs review in Firmware Nov 17, 2025
@bleska bleska moved this from 🔎 Needs review to 🏃‍♀️ In progress in Firmware Nov 17, 2025
@github-actions
Copy link

github-actions bot commented Nov 17, 2025

en main(all)

model device_test click_test persistence_test
T2T1 test(all) main(all) test(all) main(all) test(all) main(all)
T3B1 test(all) main(all) test(all) main(all) test(all) main(all)
T3T1 test(all) main(all) test(all) main(all) test(all) main(all)
T3W1 test(all) main(all) test(all) main(all) test(all) main(all)

Latest CI run: 19671362919

@github-actions
Copy link

Looks like you changed Cargo.lock. Please make sure to review the dependencies and update internal version list.

@bleska bleska force-pushed the bleska/nRF_FW_version_readout branch 2 times, most recently from bbe2996 to f11aa8b Compare November 19, 2025 16:11
… for realiability increase e.g. for cases when the nRF FW application is broken from whatever reason and can't reply to Trezor's FW version check for possible nRF FW update.

[no changelog]
… SMP serial recovery. The RUST decoding has been added. The C-RUST API has been modified. Currently, the version is returned as a string (as received from nRF IC).

[no changelog]
… SMP serial recovery. The RUST decoding has been added. The C-RUST API has been modified. Currently, the version is returned as a structure of 4 uint fields representing major, minor, revision and build_num respectively (string received from nRF IC is decoded).

To be built with: "./scripts/build_sign_flash.sh -b t3w1_revA_nrf52832/nrf52832 -c -d -s -f" command. '-d' is essential, with '-p' the build script generates and signs the image in a way that MCUboot can't verify that (it uses debug keys see hash_signer.py but the MCUboot expects the production key which has embedded inside "image_validate.c" file).

[no changelog]
@bleska bleska force-pushed the bleska/nRF_FW_version_readout branch from f11aa8b to 774e809 Compare November 19, 2025 23:49
@bleska bleska marked this pull request as ready for review November 20, 2025 10:02
@bleska bleska requested review from Copilot and removed request for matejcik and obrusvit November 20, 2025 10:02
@bleska bleska moved this from 🏃‍♀️ In progress to 🔎 Needs review in Firmware Nov 20, 2025
Copilot finished reviewing on behalf of bleska November 20, 2025 10:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds functionality to read the nRF app version via MCUboot SMP (Simple Management Protocol) serial recovery when normal SPI communication fails. This provides a fallback mechanism to determine whether an nRF firmware update is required.

Key changes:

  • Implements SMP Image State command support to query version information from nRF MCUboot
  • Adds version string parsing with overflow-safe integer conversion
  • Integrates the SMP version query as a fallback in the update detection logic when SPI communication fails

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
core/embed/rust/src/smp/mod.rs Adds IMAGE_STATE command constant and ImageStateResponse message type handling
core/embed/rust/src/smp/image_info.rs New module implementing SMP image state query, CBOR parsing, and version string parsing
core/embed/rust/src/smp/api.rs Adds C FFI wrapper smp_image_version_get() with NrfAppVersion struct
core/embed/rust/rust_smp.h Defines C-compatible nrf_app_version_t struct and function declaration
core/embed/io/nrf/stm32u5/nrf_update.c Integrates SMP version fallback into update detection logic, adds version comparison and image header reading functions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

#include <trezor_types.h>

/// Represents a parsed version string in the format:
/// "major.minor.patch[.build_num]" where build_num is optional and defaults to
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The comment refers to the third component as "patch" but the struct field is named revision. For consistency, either update the comment to say "major.minor.revision[.build_num]" or explain why the terminology differs from the struct field names.

Suggested change
/// "major.minor.patch[.build_num]" where build_num is optional and defaults to
/// "major.minor.revision[.build_num]" where build_num is optional and defaults to

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here b12efb3 .

nrf_reboot();
systick_delay_ms(500);
try_cntr++;
if (try_cntr > 3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets try smp after the first SPI failure, the repeat both 3 times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 8319d25 .

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 subject to further changes because of #6188 issue.


#include <trezor_types.h>

/// Represents a parsed version string in the format:
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding comments here, please add it for the rest of functions here too, make it doxygen style and add license header while at it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here b12efb3 .

* @return 0 on success, or a negative errno on failure
*/
static int nrf_smp_version_get(nrf_app_version_t *out_version) {
int rc = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

not much use for the errno here, maybe bool would be enough? applies to image_version_read too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 8319d25 . Also applied the same approach to "read_image_sha256()" which was the origin for the other 2 functions' refactored.

let revision = parse_u16(parts.next()?)?;

// Build number is optional (defaults to 0 if absent)
let build_num = match parts.next() {
Copy link
Contributor

Choose a reason for hiding this comment

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

somewhat more rustier version of writing this could be:

let build_num = parts
.next()
.filter(|b| !b.is_empty())
.map_or(Some(0), parse_u32)?;

but both versions should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 355574e .

}

/// Macro to generate overflow-safe decimal parsers for different integer types
macro_rules! make_parser {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use let major = parts.next()?.parse::<u8>().ok()?; instead of these custom macro-generated functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 355574e .

…MCUboot SMP serial recovery. The RUST decoding has been added. The C-RUST API has been modified. Currently, the version is returned as a structure of 4 uint fields representing major, minor, revision and build_num respectively (string received from nRF IC is decoded).
…MCUboot SMP serial recovery. The RUST decoding has been added. The C-RUST API has been modified. Currently, the version is returned as a structure of 4 uint fields representing major, minor, revision and build_num respectively (string received from nRF IC is decoded).
…MCUboot SMP serial recovery. The RUST decoding has been added. The C-RUST API has been modified. Currently, the version is returned as a structure of 4 uint fields representing major, minor, revision and build_num respectively (string received from nRF IC is decoded).
…MCUboot SMP serial recovery. The RUST decoding has been added. The C-RUST API has been modified. Currently, the version is returned as a structure of 4 uint fields representing major, minor, revision and build_num respectively (string received from nRF IC is decoded).
@bleska bleska requested a review from TychoVrahe November 27, 2025 11:32
@bleska
Copy link
Contributor Author

bleska commented Nov 27, 2025

Can we perform another review round and then focus on what remains left, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T3W1 Trezor Safe 7

Projects

Status: 🔎 Needs review

Development

Successfully merging this pull request may close these issues.

improve reliability of nRF update by getting image list from nRF bootloader

3 participants