Skip to content

nrf refactor#6815

Open
TychoVrahe wants to merge 3 commits intomainfrom
tychovrahe/pico/nrf_refactor
Open

nrf refactor#6815
TychoVrahe wants to merge 3 commits intomainfrom
tychovrahe/pico/nrf_refactor

Conversation

@TychoVrahe
Copy link
Copy Markdown
Contributor

this PR refactors nrf driver a bit, mainly focused on making the pinout configurable by board, and support ble usage without backup ram

@TychoVrahe TychoVrahe self-assigned this Apr 23, 2026
@github-project-automation github-project-automation Bot moved this to 🔎 Needs review in Firmware Apr 23, 2026
@TychoVrahe TychoVrahe requested a review from kopecdav April 23, 2026 07:01
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aee0313a-89d0-42a0-b43f-ddeb996ddf3c

📥 Commits

Reviewing files that changed from the base of the PR and between 27b2845 and 43b1e76.

📒 Files selected for processing (2)
  • core/embed/models/T3W1/boards/trezor_t3w1_revA.h
  • core/embed/models/T3W1/boards/trezor_t3w1_revB.h
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/embed/models/T3W1/boards/trezor_t3w1_revB.h

Walkthrough

BLE backup RAM usage for persisting the BLE enabled state is now optional via #ifdef USE_BACKUP_RAM; related header imports and read/write operations were guarded so ble_init no longer recovers and ble_set_enabled no longer writes that flag when the macro is unset. NRF drivers for STM32U5 were updated to replace hard-coded IRQ identifiers and peripheral pins with board-specific NRF_* macros (SPI and UART), switch EXTI pending-bit clearing from __HAL_GPIO_EXTI_CLEAR_FLAG to __HAL_GPIO_EXTI_CLEAR_IT, and rename SPI/UART IRQ handlers to NRF_SPI_INTERRUPT_HANDLER / NRF_UART_INTERRUPT_HANDLER. T3W1 board headers A/B/C were extended with NRF SPI1 and USART3 macro definitions and reordered GPIO macros.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'nrf refactor' is vague and generic, using a non-descriptive term that doesn't convey meaningful information about the specific changes in the changeset. Provide a more descriptive title that highlights the main changes, such as 'Make NRF pinout configurable by board and support BLE without backup RAM' or 'Refactor NRF driver to move pin configuration to board definitions'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The PR description provides a brief overview of the changes (NRF driver refactoring, configurable pinout, BLE without backup RAM) but lacks required sections from the template such as development status, PR setup information, and testing instructions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tychovrahe/pico/nrf_refactor

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 23, 2026

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: 24822270347

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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-247: NRF_UART_IRQN is incorrectly set to USART1_IRQn while
NRF_UART_INSTANCE and NRF_UART_INTERRUPT_HANDLER reference USART3; update the
IRQ define so NRF_UART_IRQN maps to USART3_IRQn to match NRF_UART_INSTANCE
(USART3) and NRF_UART_INTERRUPT_HANDLER (USART3_IRQHandler), ensuring nrf.c
enabling/disabling uses the correct interrupt line.
🪄 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: d63c177a-ea73-4c09-8c12-a3cb3b8342cd

📥 Commits

Reviewing files that changed from the base of the PR and between 39c051b and 27b2845.

📒 Files selected for processing (7)
  • core/embed/io/ble/stm32/ble.c
  • core/embed/io/nrf/stm32u5/nrf.c
  • core/embed/io/nrf/stm32u5/nrf_spi.c
  • core/embed/io/nrf/stm32u5/nrf_uart.c
  • core/embed/models/T3W1/boards/trezor_t3w1_revA.h
  • core/embed/models/T3W1/boards/trezor_t3w1_revB.h
  • core/embed/models/T3W1/boards/trezor_t3w1_revC.h

Comment thread core/embed/models/T3W1/boards/trezor_t3w1_revB.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🔎 Needs review

Development

Successfully merging this pull request may close these issues.

1 participant