prepare NCS update#6817
Open
TychoVrahe wants to merge 10 commits intomainfrom
Open
Conversation
WalkthroughThis PR updates the t3w1_revA_nrf52832 board configuration and trezor-ble application. Changes include: correcting board metadata (vendor, full_name, and display name); restructuring device tree GPIO bindings by consolidating leds/buttons into outputs/inputs blocks and renaming SPI node labels; removing static partition files and migrating to DTS fixed-partitions via sysbuild configuration; updating build signing logic to derive parameters from Zephyr artifacts instead of hardcoded values; replacing printk logging with Zephyr LOG API throughout application code; converting DK-specific button/LED handling to device-tree GPIO specifications; and adjusting MCUBoot configuration toward single-slot bootloader mode. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| 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: 24880013503
TychoVrahe
force-pushed
the
tychovrahe/pico/upg_prep2
branch
from
c52f0a9 to
8ea1326
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/embed/models/T3W1/boards/trezor_t3w1_revB.h`:
- Around line 240-246: The IRQ macro for the NRF UART is wrong: update the
NRF_UART_IRQN macro in trezor_t3w1_revB.h to use the USART3 IRQ (matching
NRF_UART_INSTANCE and NRF_UART_INTERRUPT_HANDLER) so the NVIC enables the
correct interrupt under USE_SMP; locate the NRF_UART_IRQN definition and replace
the USART1_IRQn reference with the corresponding USART3_IRQn symbol so
USART3_IRQHandler is invoked for UART RX/TX events.
In `@nordic/trezor/scripts/build_sign_flash.sh`:
- Line 156: The flash command hard-codes the sysbuild domain "trezor-ble" which
breaks when the script is invoked with a different application directory via -a;
update the command in build_sign_flash.sh that currently runs 'west flash
--domain trezor-ble --hex-file ./build/zephyr.merged.signed_trz.hex' to use the
APP_DIR variable instead (e.g., --domain "$APP_DIR") and ensure proper quoting;
if the intent was to disallow different apps with -f, add an explicit check that
rejects -a when -f is passed or introduce a separate DOMAIN variable and use
that in the flash command.
- Around line 18-19: The script currently hard-codes partition constants
(HEADER_SIZE, SLOT_ADDR and the -S 0x6c000 size) which can drift from the
authoritative DTS; change build_sign_flash.sh to parse the generated Device Tree
(build/$APP_DIR/zephyr/zephyr.dts) at runtime to extract the fixed-partitions
properties and derive HEADER_SIZE, SLOT_ADDR and the slot size instead of using
literals; update the code paths that reference HEADER_SIZE, SLOT_ADDR and the -S
value so they use the parsed values and add a clear error if the expected
partition node or properties cannot be found.
- Around line 136-137: The dd invocation that creates zephyr_nohdr.bin should be
made robust: quote the variable usages ($APP_DIR and $HEADER_SIZE) in the dd
command, stop discarding stderr (remove 2>/dev/null), and immediately check dd's
exit status so the script fails fast if header stripping fails; on failure
delete any partially-created build/$APP_DIR/zephyr/zephyr_nohdr.bin and exit
with a non-zero status so subsequent signing doesn't operate on a stale file.
Target the dd call that produces build/$APP_DIR/zephyr/zephyr_nohdr.bin and add
the quoted variables, error checking, and cleanup logic around it.
In `@nordic/trezor/trezor-ble/prj.conf`:
- Around line 101-112: The production config currently disables logging via
CONFIG_LOG=n, CONFIG_UART_CONSOLE=n and CONFIG_LOG_BACKEND_UART=n which silences
all LOG_ERR/LOG_WRN calls; either enable the logger and an appropriate backend
in prj.conf (set CONFIG_LOG=y and enable a backend such as
CONFIG_LOG_BACKEND_UART or another transport) or guard the diagnostics so they
only compile when logging is enabled (use Kconfig checks or
IS_ENABLED(CONFIG_LOG) around the LOG_ERR/LOG_WRN sites) — update prj.conf or
the modules emitting LOG_ERR/LOG_WRN (UART/SPI/BLE/GPIO/prodtest) accordingly so
diagnostic messages are actually delivered in production.
In `@nordic/trezor/trezor-ble/src/management/management.c`:
- Around line 158-160: The read_image_sha256 call's return value is not checked
so failures produce an all-zero hash and the code still calls trz_comm_send_msg
with that bogus INFO payload; update the logic around
read_image_sha256(FIXED_PARTITION_ID(slot0_partition), &data[9]) to capture its
return code, and on error avoid sending the success INFO: either log the error
via the existing logging mechanism and return/fail early, or populate data with
an explicit error TLV/status and call trz_comm_send_msg with that error payload
instead of the zero hash; ensure trz_comm_send_msg is only called with a valid
hash when read_image_sha256 succeeded.
In `@nordic/trezor/trezor-ble/src/signals/signals.c`:
- Around line 58-64: signals_set_reserved currently sets the cached out_reserved
before calling gpio_pin_set_dt, so failures are silently ignored; change
signals_set_reserved(bool set) to call gpio_pin_set_dt(&reserved_output, set),
check its return (0 success, negative errno on failure), and only update
out_reserved when gpio_pin_set_dt succeeds; on failure do not update
out_reserved and emit an error log (or otherwise surface the errno) so
signals_out_get_reserved() cannot report a state that wasn't applied to
hardware.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d233b68-9fd0-4a2b-82cc-abed1234d1cc
📒 Files selected for processing (28)
core/embed/io/ble/stm32/ble.ccore/embed/io/nrf/stm32u5/nrf.ccore/embed/io/nrf/stm32u5/nrf_spi.ccore/embed/io/nrf/stm32u5/nrf_uart.ccore/embed/models/T3W1/boards/trezor_t3w1_revA.hcore/embed/models/T3W1/boards/trezor_t3w1_revB.hcore/embed/models/T3W1/boards/trezor_t3w1_revC.hnordic/trezor/boards/arm/t3w1_revA_nrf52832/Kconfig.defconfignordic/trezor/boards/arm/t3w1_revA_nrf52832/board.ymlnordic/trezor/boards/arm/t3w1_revA_nrf52832/t3w1_revA_nrf52832.dtsnordic/trezor/boards/arm/t3w1_revA_nrf52832/t3w1_revA_nrf52832.yamlnordic/trezor/direct_test_mode/pm_static.ymlnordic/trezor/direct_test_mode/sysbuild.confnordic/trezor/radio_test/pm_static.ymlnordic/trezor/radio_test/sysbuild.confnordic/trezor/scripts/build_sign_flash.shnordic/trezor/trezor-ble/boards/t3w1_revA_nrf52832.overlaynordic/trezor/trezor-ble/pm_static.ymlnordic/trezor/trezor-ble/prj.confnordic/trezor/trezor-ble/src/ble/pairing.cnordic/trezor/trezor-ble/src/main.cnordic/trezor/trezor-ble/src/management/management.cnordic/trezor/trezor-ble/src/signals/signals.cnordic/trezor/trezor-ble/src/trz_comm/spi.cnordic/trezor/trezor-ble/src/trz_comm/uart.cnordic/trezor/trezor-ble/sysbuild.confnordic/trezor/trezor-ble/sysbuild/mcuboot.confvendor/libtropic
💤 Files with no reviewable changes (5)
- nordic/trezor/trezor-ble/sysbuild/mcuboot.conf
- nordic/trezor/trezor-ble/src/main.c
- nordic/trezor/radio_test/pm_static.yml
- nordic/trezor/direct_test_mode/pm_static.yml
- nordic/trezor/trezor-ble/pm_static.yml
…tions [no changelog]
[no changelog]
[no changelog]
[no changelog]
[no changelog]
… DTS and config files [no changelog]
TychoVrahe
force-pushed
the
tychovrahe/pico/upg_prep2
branch
from
8ea1326 to
d305b97
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nordic/trezor/trezor-ble/src/management/management.c (1)
158-162: LGTM — addresses prior feedback on unchecked hash read.
rcis now captured fromread_image_sha256, logged, and the INFO response is suppressed on failure, so a partition/read/TLV error no longer produces a zero-hash success response. Partition ID migration toFIXED_PARTITION_ID(slot0_partition)is consistent with the PR's move frompm_staticto DTS fixed-partitions.Minor (optional): the docstring at lines 86–92 still refers to
FLASH_AREA_ID(image_0)/image_1as the expectedarea_idargument — worth refreshing toFIXED_PARTITION_ID(slot0_partition)/slot1_partitionfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nordic/trezor/trezor-ble/src/management/management.c` around lines 158 - 162, Update the docstring that still references FLASH_AREA_ID(image_0)/image_1 to reflect the move to DTS fixed-partitions: change the expected area_id examples to use FIXED_PARTITION_ID(slot0_partition) and FIXED_PARTITION_ID(slot1_partition) (the docstring near the management code handling read_image_sha256 in management.c); keep wording consistent with the partition ID migration from pm_static to FIXED_PARTITION_ID(slotX) and ensure the argument description matches the actual calls to read_image_sha256 and other partition helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nordic/trezor/trezor-ble/src/management/management.c`:
- Around line 158-162: Update the docstring that still references
FLASH_AREA_ID(image_0)/image_1 to reflect the move to DTS fixed-partitions:
change the expected area_id examples to use FIXED_PARTITION_ID(slot0_partition)
and FIXED_PARTITION_ID(slot1_partition) (the docstring near the management code
handling read_image_sha256 in management.c); keep wording consistent with the
partition ID migration from pm_static to FIXED_PARTITION_ID(slotX) and ensure
the argument description matches the actual calls to read_image_sha256 and other
partition helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03728008-eb98-4cc6-a147-faefc0645479
📒 Files selected for processing (20)
nordic/trezor/boards/arm/t3w1_revA_nrf52832/Kconfig.defconfignordic/trezor/boards/arm/t3w1_revA_nrf52832/board.ymlnordic/trezor/boards/arm/t3w1_revA_nrf52832/t3w1_revA_nrf52832.dtsnordic/trezor/boards/arm/t3w1_revA_nrf52832/t3w1_revA_nrf52832.yamlnordic/trezor/direct_test_mode/pm_static.ymlnordic/trezor/direct_test_mode/sysbuild.confnordic/trezor/radio_test/pm_static.ymlnordic/trezor/radio_test/sysbuild.confnordic/trezor/scripts/build_sign_flash.shnordic/trezor/trezor-ble/boards/t3w1_revA_nrf52832.overlaynordic/trezor/trezor-ble/pm_static.ymlnordic/trezor/trezor-ble/prj.confnordic/trezor/trezor-ble/src/ble/pairing.cnordic/trezor/trezor-ble/src/main.cnordic/trezor/trezor-ble/src/management/management.cnordic/trezor/trezor-ble/src/signals/signals.cnordic/trezor/trezor-ble/src/trz_comm/spi.cnordic/trezor/trezor-ble/src/trz_comm/uart.cnordic/trezor/trezor-ble/sysbuild.confnordic/trezor/trezor-ble/sysbuild/mcuboot.conf
💤 Files with no reviewable changes (5)
- nordic/trezor/trezor-ble/src/main.c
- nordic/trezor/trezor-ble/sysbuild/mcuboot.conf
- nordic/trezor/trezor-ble/pm_static.yml
- nordic/trezor/radio_test/pm_static.yml
- nordic/trezor/direct_test_mode/pm_static.yml
✅ Files skipped from review due to trivial changes (7)
- nordic/trezor/boards/arm/t3w1_revA_nrf52832/Kconfig.defconfig
- nordic/trezor/direct_test_mode/sysbuild.conf
- nordic/trezor/trezor-ble/sysbuild.conf
- nordic/trezor/trezor-ble/src/trz_comm/uart.c
- nordic/trezor/trezor-ble/src/ble/pairing.c
- nordic/trezor/boards/arm/t3w1_revA_nrf52832/t3w1_revA_nrf52832.yaml
- nordic/trezor/radio_test/sysbuild.conf
🚧 Files skipped from review as they are similar to previous changes (4)
- nordic/trezor/trezor-ble/boards/t3w1_revA_nrf52832.overlay
- nordic/trezor/boards/arm/t3w1_revA_nrf52832/board.yml
- nordic/trezor/scripts/build_sign_flash.sh
- nordic/trezor/trezor-ble/prj.conf
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.