Lfarsky/nfc/convert to polling#7081
Open
FarskyL wants to merge 21 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe NFC public header is refactored to remove technology registration and RFAL STM functions, replacing them with Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 28371043993
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/embed/io/nfc/st25/nfc.c (1)
642-679:⚠️ Potential issue | 🟠 Major | ⚡ Quick winType mismatch:
nfc_transcieve_blockingreturnsnfc_status_t, notReturnCode.The variable
erris declared asReturnCode(line 643), butnfc_transcieve_blockingreturnsnfc_status_t. The loop condition compares againstRFAL_ERR_NONEandRFAL_ERR_SLEEP_REQ:
NFC_OK == 0andRFAL_ERR_NONE == 0— this works by coincidenceRFAL_ERR_SLEEP_REQwill never match anynfc_status_treturn value, so sleep-request handling is brokenThis could cause the card emulator loop to exit prematurely or behave incorrectly when a sleep request occurs.
Proposed fix: Use correct type and status values
static void nfc_card_emulator_loop(rfalNfcDevice *nfc_dev) { - ReturnCode err = RFAL_ERR_INTERNAL; + nfc_status_t status = NFC_ERROR; uint8_t *rx_buf; uint16_t *rcv_len; uint8_t tx_buf[150]; uint16_t tx_len; do { rfalNfcWorker(); switch (rfalNfcGetState()) { case RFAL_NFC_STATE_ACTIVATED: - err = nfc_transcieve_blocking(NULL, 0, &rx_buf, &rcv_len, 0); + status = nfc_transcieve_blocking(NULL, 0, &rx_buf, &rcv_len, 0); break; case RFAL_NFC_STATE_DATAEXCHANGE: case RFAL_NFC_STATE_DATAEXCHANGE_DONE: tx_len = ((nfc_dev->type == RFAL_NFC_POLL_TYPE_NFCA) ? card_emulation_t4t(rx_buf, *rcv_len, tx_buf, sizeof(tx_buf)) : rfalConvBytesToBits( card_emulation_t3t(rx_buf, rfalConvBitsToBytes(*rcv_len), tx_buf, sizeof(rx_buf)))); - err = nfc_transcieve_blocking(tx_buf, tx_len, &rx_buf, &rcv_len, + status = nfc_transcieve_blocking(tx_buf, tx_len, &rx_buf, &rcv_len, RFAL_FWT_NONE); break; case RFAL_NFC_STATE_START_DISCOVERY: return; case RFAL_NFC_STATE_LISTEN_SLEEP: default: break; } - } while ((err == RFAL_ERR_NONE) || (err == RFAL_ERR_SLEEP_REQ)); + } while (status == NFC_OK); }Note: If sleep-request handling is needed,
nfc_transcieve_blockingshould be extended to return a distinct status for that case, or the loop should check RFAL state directly for sleep conditions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/nfc/st25/nfc.c` around lines 642 - 679, The loop uses ReturnCode err but nfc_transcieve_blocking returns nfc_status_t, so replace the err declaration/type in nfc_card_emulator_loop with nfc_status_t and update the loop condition to compare against the correct nfc_status_t constants (e.g., NFC_OK and the nfc sleep-request constant, e.g., NFC_SLEEP_REQ) instead of RFAL_ERR_NONE/RFAL_ERR_SLEEP_REQ; keep references to nfc_transcieve_blocking, nfc_card_emulator_loop, and rx_buf/tx_buf logic intact (alternatively, if sleep handling should be driven by RFAL state, check rfalNfcGetState() for the sleep state rather than using RFAL_* error codes).
🧹 Nitpick comments (6)
core/embed/io/nfc/st25/nfc.c (1)
471-481: ⚡ Quick winVariable declarations inside switch case should use braces.
Declaring variables directly inside a
caselabel without braces can lead to initialization issues if the code is modified later (e.g., adding fallthrough cases). Wrap the case body in braces for safety.Proposed fix
switch (dev_info.type) { - case NFC_DEV_TYPE_A: - uint8_t rxBuf[20]; - uint16_t rxLen = sizeof(rxBuf); - ReturnCode err = rfalT2TPollerRead(0x00, rxBuf, sizeof(rxBuf), &rxLen); - return err == RFAL_ERR_NONE; + case NFC_DEV_TYPE_A: { + uint8_t rxBuf[20]; + uint16_t rxLen = sizeof(rxBuf); + ReturnCode err = rfalT2TPollerRead(0x00, rxBuf, sizeof(rxBuf), &rxLen); + return err == RFAL_ERR_NONE; + } case NFC_DEV_TYPE_B: case NFC_DEV_TYPE_V: default: return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/nfc/st25/nfc.c` around lines 471 - 481, The switch case for NFC_DEV_TYPE_A declares local variables (rxBuf, rxLen, err) without braces; wrap the NFC_DEV_TYPE_A case body in braces so the declarations are scoped correctly, e.g., put a { ... } around the code that calls rfalT2TPollerRead and returns its result, ensuring rxBuf, rxLen and err are declared inside that block and the return err == RFAL_ERR_NONE; remains unchanged.core/embed/io/nfc/inc/io/nfc.h (1)
82-87: ⚡ Quick winConsider using
nfc_dev_type_tinstead ofuint8_tfor thetypefield.The
typefield is declared asuint8_tbut stores values fromnfc_dev_type_tenum. Using the enum type directly would improve type safety and make the API self-documenting.Proposed fix
typedef struct { - uint8_t type; + nfc_dev_type_t type; nfc_dev_interface_t interface; char uid[NFC_MAX_UID_BUF_SIZE]; // Plus one for string termination uint8_t uid_len; } nfc_dev_info_t;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/nfc/inc/io/nfc.h` around lines 82 - 87, The struct nfc_dev_info_t uses a plain uint8_t for the type field even though values come from the nfc_dev_type_t enum; change the field declaration from uint8_t type to nfc_dev_type_t type to improve type safety and self-documentation, and update any code that constructs or assigns to nfc_dev_info_t.type to use the enum values (or cast where unavoidable); ensure the header that defines nfc_dev_type_t is visible (include or forward-declare) so the compilation remains correct.core/embed/io/nfc/st25/nfc_poll.c (2)
76-79: ⚡ Quick winAdd bounds check for
task_id.The
task_idparameter is used as an array index intog_nfc_tlswithout validation. While the system event framework should only call this with valid task IDs, adding a defensive bounds check would prevent potential out-of-bounds access if the contract changes.🛡️ Proposed fix
static void on_task_created(void* context, systask_id_t task_id) { + if (task_id >= SYSTASK_MAX_TASKS) { + return; + } nfc_fsm_t* fsm = &g_nfc_tls[task_id]; memset(fsm, 0, sizeof(nfc_fsm_t)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/nfc/st25/nfc_poll.c` around lines 76 - 79, The on_task_created callback uses task_id directly to index g_nfc_tls which can cause out-of-bounds access; add a defensive bounds check at the start of on_task_created to verify task_id is within the valid range for g_nfc_tls (e.g., compare against the array length/maximum task count constant), and only then obtain nfc_fsm_t* fsm = &g_nfc_tls[task_id] and call memset; if task_id is invalid, return early (optionally log or assert) to avoid touching g_nfc_tls.
116-123: ⚡ Quick winAdd parameter validation.
The function directly casts
paramtonfc_state_t*(Line 120) and usestask_idas an array index (Line 118) without validation. While the system event framework contract should guarantee valid values, adding defensive checks would improve robustness.🛡️ Proposed fix
static bool on_check_read_ready(void* context, systask_id_t task_id, void* param) { + if (task_id >= SYSTASK_MAX_TASKS || param == NULL) { + return false; + } nfc_fsm_t* fsm = &g_nfc_tls[task_id];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/nfc/st25/nfc_poll.c` around lines 116 - 123, The on_check_read_ready function lacks defensive checks: validate task_id before indexing g_nfc_tls (ensure task_id < ARRAY_SIZE(g_nfc_tls) or other valid range) and validate param is non-NULL (and optionally that it points to a valid nfc_state_t) before casting; if checks fail, return false (or an appropriate error) instead of performing the cast/index and call nfc_fsm_update only after validations pass.core/embed/io/nfc/st25/nfc_poll.h (1)
20-26: ⚡ Quick winConsider adding detailed module documentation.
The current comment at Line 22 provides a brief description but lacks important details that would help consumers use this module correctly:
- Initialization order requirements (must call
nfc_poll_init()afternfc_init())- Threading model (which functions are safe to call from which contexts)
- Relationship to system event framework (requires
SYSHANDLE_NFCregistration)- Usage pattern (call
nfc_get_event()aftersysevents_poll()signalsSYSHANDLE_NFCread-ready)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/nfc/st25/nfc_poll.h` around lines 20 - 26, Add comprehensive module-level documentation above the declarations in nfc_poll.h: state that nfc_poll_init() must be called only after nfc_init(), describe the threading/ISR/context model and which functions are safe to call from task vs interrupt contexts (including nfc_poll_deinit()), document the dependency on the system event framework and that SYSHANDLE_NFC must be registered, and show the typical usage pattern (call sysevents_poll(), wait for SYSHANDLE_NFC read-ready, then call nfc_get_event() / related APIs). Mention return/errno behavior for nfc_poll_init() and nfc_poll_deinit() and any lifetime/ordering constraints between nfc_poll_init(), nfc_poll_deinit(), and nfc_init()/nfc_shutdown.core/embed/io/nfc/st25/nfc_poll_internal.h (1)
27-33: ⚡ Quick winConsider adding function documentation.
The internal API functions lack documentation comments explaining their purpose, parameters, return values, and preconditions. This would help maintainers understand the contract between the polling module and the NFC driver implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/io/nfc/st25/nfc_poll_internal.h` around lines 27 - 33, Add Doxygen-style documentation comments above each internal API declaration (nfc_is_connected, nfc_identify, nfc_check_connection, nfc_restart_discovery) that state the function purpose, parameters (e.g., device_type for nfc_identify), return values and meaning, preconditions (required state before calling), side effects, and any thread-safety/ownership guarantees; place the comments in the header near the declarations so callers and implementers can read the contract for the polling module and NFC driver implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/embed/io/nfc/st25/nfc_poll.c`:
- Around line 51-57: nfc_get_event dereferences the pointer parameter `event`
without validation; add a NULL check at the start of nfc_get_event and return
false if event is NULL to avoid a null-pointer dereference. Locate the function
nfc_get_event and before accessing *event, verify `if (event == NULL) return
false;` then proceed to use fsm (g_nfc_tls and systask_id(systask_active())) and
set *event and clear fsm->events as existing code does.
In `@core/embed/io/nfc/st25/nfc.c`:
- Around line 431-446: The nfc_identify(rfalNfcDevType device_type) function
currently ignores its device_type parameter and instead uses
nfc_dev_read_info(&dev_info). Either remove the unused parameter from the
function signature and update its declaration in the header and every call site
(e.g., the call from nfc_poll.c) to match the new signature, or modify
nfc_identify to use the passed device_type by comparing device_type against
NFC_DEV_TYPE_A/B/V (or combining both checks: verify device_type matches
dev_info.type) and keep the signature. Update the prototype in the header and
all callers to ensure consistency.
In `@core/embed/projects/prodtest/cmd/prodtest_nfc.c`:
- Around line 124-138: The switch on dev_info.interface currently treats unknown
interfaces as an error; add an explicit case for NFC_DEV_INTERFACE_UNKNOWN in
the same switch (the block switching on dev_info.interface with cases
NFC_DEV_INTERFACE_RF, NFC_DEV_INTERFACE_ISODEP, NFC_DEV_INTERFACE_NFCDEP) and
handle it gracefully (e.g., call cli_trace(cli, "NFC Tag Type: UNKNOWN (%d)",
dev_info.interface)) instead of letting it fall through to the default that
calls cli_error and goto cleanup; leave the default case to report truly
unexpected values as before.
---
Outside diff comments:
In `@core/embed/io/nfc/st25/nfc.c`:
- Around line 642-679: The loop uses ReturnCode err but nfc_transcieve_blocking
returns nfc_status_t, so replace the err declaration/type in
nfc_card_emulator_loop with nfc_status_t and update the loop condition to
compare against the correct nfc_status_t constants (e.g., NFC_OK and the nfc
sleep-request constant, e.g., NFC_SLEEP_REQ) instead of
RFAL_ERR_NONE/RFAL_ERR_SLEEP_REQ; keep references to nfc_transcieve_blocking,
nfc_card_emulator_loop, and rx_buf/tx_buf logic intact (alternatively, if sleep
handling should be driven by RFAL state, check rfalNfcGetState() for the sleep
state rather than using RFAL_* error codes).
---
Nitpick comments:
In `@core/embed/io/nfc/inc/io/nfc.h`:
- Around line 82-87: The struct nfc_dev_info_t uses a plain uint8_t for the type
field even though values come from the nfc_dev_type_t enum; change the field
declaration from uint8_t type to nfc_dev_type_t type to improve type safety and
self-documentation, and update any code that constructs or assigns to
nfc_dev_info_t.type to use the enum values (or cast where unavoidable); ensure
the header that defines nfc_dev_type_t is visible (include or forward-declare)
so the compilation remains correct.
In `@core/embed/io/nfc/st25/nfc_poll_internal.h`:
- Around line 27-33: Add Doxygen-style documentation comments above each
internal API declaration (nfc_is_connected, nfc_identify, nfc_check_connection,
nfc_restart_discovery) that state the function purpose, parameters (e.g.,
device_type for nfc_identify), return values and meaning, preconditions
(required state before calling), side effects, and any thread-safety/ownership
guarantees; place the comments in the header near the declarations so callers
and implementers can read the contract for the polling module and NFC driver
implementation.
In `@core/embed/io/nfc/st25/nfc_poll.c`:
- Around line 76-79: The on_task_created callback uses task_id directly to index
g_nfc_tls which can cause out-of-bounds access; add a defensive bounds check at
the start of on_task_created to verify task_id is within the valid range for
g_nfc_tls (e.g., compare against the array length/maximum task count constant),
and only then obtain nfc_fsm_t* fsm = &g_nfc_tls[task_id] and call memset; if
task_id is invalid, return early (optionally log or assert) to avoid touching
g_nfc_tls.
- Around line 116-123: The on_check_read_ready function lacks defensive checks:
validate task_id before indexing g_nfc_tls (ensure task_id <
ARRAY_SIZE(g_nfc_tls) or other valid range) and validate param is non-NULL (and
optionally that it points to a valid nfc_state_t) before casting; if checks
fail, return false (or an appropriate error) instead of performing the
cast/index and call nfc_fsm_update only after validations pass.
In `@core/embed/io/nfc/st25/nfc_poll.h`:
- Around line 20-26: Add comprehensive module-level documentation above the
declarations in nfc_poll.h: state that nfc_poll_init() must be called only after
nfc_init(), describe the threading/ISR/context model and which functions are
safe to call from task vs interrupt contexts (including nfc_poll_deinit()),
document the dependency on the system event framework and that SYSHANDLE_NFC
must be registered, and show the typical usage pattern (call sysevents_poll(),
wait for SYSHANDLE_NFC read-ready, then call nfc_get_event() / related APIs).
Mention return/errno behavior for nfc_poll_init() and nfc_poll_deinit() and any
lifetime/ordering constraints between nfc_poll_init(), nfc_poll_deinit(), and
nfc_init()/nfc_shutdown.
In `@core/embed/io/nfc/st25/nfc.c`:
- Around line 471-481: The switch case for NFC_DEV_TYPE_A declares local
variables (rxBuf, rxLen, err) without braces; wrap the NFC_DEV_TYPE_A case body
in braces so the declarations are scoped correctly, e.g., put a { ... } around
the code that calls rfalT2TPollerRead and returns its result, ensuring rxBuf,
rxLen and err are declared inside that block and the return err ==
RFAL_ERR_NONE; remains unchanged.
🪄 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: 0a8537d2-a64b-433f-94a3-dbff15c88901
📒 Files selected for processing (9)
core/embed/io/nfc/inc/io/nfc.hcore/embed/io/nfc/st25/nfc.ccore/embed/io/nfc/st25/nfc_poll.ccore/embed/io/nfc/st25/nfc_poll.hcore/embed/io/nfc/st25/nfc_poll_internal.hcore/embed/projects/prodtest/cmd/prodtest_nfc.ccore/embed/projects/prodtest/main.ccore/embed/sys/task/inc/sys/sysevent.hcore/site_scons/models/T3W1/trezor_t3w1_revC.py
Comment on lines
+51
to
+57
| bool nfc_get_event(nfc_event_t* event) { | ||
| nfc_fsm_t* fsm = &g_nfc_tls[systask_id(systask_active())]; | ||
|
|
||
| *event = fsm->events; | ||
| fsm->events = NFC_NO_EVENT; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Add NULL check for the event parameter.
The function dereferences event at Line 54 without validating it. If a caller passes NULL, this will cause a null-pointer dereference.
🛡️ Proposed fix
bool nfc_get_event(nfc_event_t* event) {
+ if (event == NULL) {
+ return false;
+ }
nfc_fsm_t* fsm = &g_nfc_tls[systask_id(systask_active())];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/embed/io/nfc/st25/nfc_poll.c` around lines 51 - 57, nfc_get_event
dereferences the pointer parameter `event` without validation; add a NULL check
at the start of nfc_get_event and return false if event is NULL to avoid a
null-pointer dereference. Locate the function nfc_get_event and before accessing
*event, verify `if (event == NULL) return false;` then proceed to use fsm
(g_nfc_tls and systask_id(systask_active())) and set *event and clear
fsm->events as existing code does.
FarskyL
force-pushed
the
lfarsky/nfc/convert_to_polling
branch
from
5b01d49 to
61ba401
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/embed/io/nfc/st25/nfc_poll_internal.h`:
- Around line 27-31: Update the zero-argument function declarations and
implementations to use C's explicit void parameter list: change the prototypes
for nfc_is_connected, nfc_identify, and nfc_check_connection to use (void)
instead of empty () in the header, and modify the corresponding implementations
of nfc_is_connected and nfc_check_connection in the source (which currently use
empty ()) to also use (void) so signatures match nfc_identify(void) and enable
proper call-arity checking.
In `@core/embed/io/nfc/st25/nfc.c`:
- Around line 274-275: The waiting loop inside nfc_stop_discovery() that spins
until RFAL_NFC_STATE_IDLE must be bounded: replace the infinite wait with a
timed/iteration-bounded wait (e.g., check a monotonic timeout or max-iteration
counter) and if the timeout expires, break out, log an error, and perform a safe
fallback (abort the RFAL transition or force poll deinit) and return a failure
code so callers (e.g., nfc_deinit()) can handle it; apply the same bounded-wait
pattern to the other similar loop referenced in the review (the other stop/wait
loop around RFAL_NFC_STATE_IDLE) so both nfc_stop_discovery() and the deinit
path no longer hang on stuck RFAL transitions.
- Around line 96-98: The current poll configuration (.techs2Find includes
RFAL_NFC_POLL_TECH_B and _V) advertises Type B/V support but
nfc_check_connection() only implements a Type A liveness check, causing B/V
cards to immediately disconnect via nfc_restart_discovery(); either implement
proper B and V presence checks inside nfc_check_connection() (handle
RFAL_NFC_POLL_TECH_B and RFAL_NFC_POLL_TECH_V paths and use the appropriate RFAL
presence APIs) or remove RFAL_NFC_POLL_TECH_B/_V from the .techs2Find mask so
they are not advertised; ensure you update or validate related logic in
nfc_identify() and the poll configuration (techs2Find/techs2Bail) so the
cross-file contract between the poll configuration and nfc_check_connection() is
consistent.
- Around line 312-316: The code sets drv->disc_params.GBLen to the backing array
size (sizeof(default_disc_params.GB)) which can advertise trailing zeros; change
it to use the configured payload length (e.g. default_disc_params.GBLen or the
actual populated length) instead. Locate the assignment to
drv->disc_params.GBLen and replace the sizeof(...) usage with the configured GB
length constant/field from default_disc_params so the driver advertises only the
valid General Bytes. Ensure any memcpy or transmit logic that relies on GBLen
uses this length variable.
In `@core/embed/projects/prodtest/main.c`:
- Around line 207-209: The nfc_init() call in the driver startup ignores its
return value; update the startup code that calls nfc_init() (the nfc_init
invocation in main) to check its return status, and on failure log an explicit
error (including any error code/message), abort startup or return a non-zero
error from main (or set a clear "NFC unavailable" state) so the CLI cannot
proceed with a broken NFC subsystem; ensure the change covers both the `#ifdef`
USE_NFC path and any cleanup path (e.g., skip poll registration) so failures are
diagnosed immediately.
🪄 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: eedef743-2717-48e5-bcb2-1fc8929b5b46
📒 Files selected for processing (9)
core/embed/io/nfc/inc/io/nfc.hcore/embed/io/nfc/st25/nfc.ccore/embed/io/nfc/st25/nfc_poll.ccore/embed/io/nfc/st25/nfc_poll.hcore/embed/io/nfc/st25/nfc_poll_internal.hcore/embed/projects/prodtest/cmd/prodtest_nfc.ccore/embed/projects/prodtest/main.ccore/embed/sys/task/inc/sys/sysevent.hcore/site_scons/models/T3W1/trezor_t3w1_revC.py
✅ Files skipped from review due to trivial changes (1)
- core/embed/io/nfc/st25/nfc_poll.h
🚧 Files skipped from review as they are similar to previous changes (5)
- core/embed/sys/task/inc/sys/sysevent.h
- core/site_scons/models/T3W1/trezor_t3w1_revC.py
- core/embed/projects/prodtest/cmd/prodtest_nfc.c
- core/embed/io/nfc/st25/nfc_poll.c
- core/embed/io/nfc/inc/io/nfc.h
Comment on lines
+207
to
+209
| #ifdef USE_NFC | ||
| nfc_init(); | ||
| #endif |
There was a problem hiding this comment.
Handle nfc_init() failure explicitly in driver startup.
At Line 208, the return value is ignored. If NFC init/poll registration fails, startup proceeds silently and failures appear later in CLI flow with poor diagnosability.
Proposed fix
`#ifdef` USE_NFC
- nfc_init();
+ nfc_status_t nfc_status = nfc_init();
+ if (nfc_status != NFC_OK) {
+ // keep booting prodtest, but surface root-cause early
+ cli_trace(&g_cli, "NFC init failed: %d", nfc_status);
+ }
`#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/embed/projects/prodtest/main.c` around lines 207 - 209, The nfc_init()
call in the driver startup ignores its return value; update the startup code
that calls nfc_init() (the nfc_init invocation in main) to check its return
status, and on failure log an explicit error (including any error code/message),
abort startup or return a non-zero error from main (or set a clear "NFC
unavailable" state) so the CLI cannot proceed with a broken NFC subsystem;
ensure the change covers both the `#ifdef` USE_NFC path and any cleanup path
(e.g., skip poll registration) so failures are diagnosed immediately.
61ba401 to
8695ae0
Compare
kopecdav
reviewed
Jun 15, 2026
| } nfc_dev_info_t; | ||
|
|
||
| // Initialize NFC driver including supportive RFAL middleware | ||
| // Initialize NFC driver including supportive RFAL middleware and polling |
Contributor
There was a problem hiding this comment.
We should take opportunity to update the public api docu format to recent "doxygen like" structure. You can see example in 'power_manager.h' for instance.
| typedef struct { | ||
| bool initialized; | ||
| bool rfal_initialized; | ||
| bool card_connected; |
Contributor
There was a problem hiding this comment.
This flag seems to be duplicate to the connected state defined in nfc_state_t used in nfc_poll, the state should has only one single source of truth. nfc_get_state function and nfc_get_event should be using it.
| NFC_DEV_TYPE_ST25TB, | ||
| NFC_DEV_TYPE_AP2P, | ||
| NFC_DEV_TYPE_POLL_TYPE_A, | ||
| NFC_DEV_TYPE_POLL_TYPE_F, |
Contributor
There was a problem hiding this comment.
Lets keep NFC_DEV_TYPE_A and B only, please cleanup all other variables and configuration options in other places and disable unused technologies in rfal_platform.h
| NFC_OK = 0, | ||
| NFC_ERROR, | ||
| NFC_WRONG_STATE, | ||
| NFC_PARAM, |
Contributor
There was a problem hiding this comment.
whole nfc_status_t is somehow obsolete now. We should refactor whole driver with ts_t status codes defined in error_handling.h. You can look into haptic driver for reference.
Contributor
There was a problem hiding this comment.
Thanks, looks good, just please also replace classical status checks with TSH_CHECK macros.
kopecdav
requested changes
Jun 15, 2026
github-project-automation
Bot
moved this from 🔎 Needs review
to 🏃♀️ In progress
in Firmware
FarskyL
force-pushed
the
lfarsky/nfc/convert_to_polling
branch
from
f1a4a23 to
5be328f
Compare
[no changelog]
FarskyL
force-pushed
the
lfarsky/nfc/convert_to_polling
branch
from
5be328f to
304f365
Compare
FarskyL
force-pushed
the
lfarsky/nfc/convert_to_polling
branch
from
304f365 to
bff6414
Compare
kopecdav
requested changes
Jun 25, 2026
| default: | ||
| *event = NFC_NO_EVENT; | ||
| } | ||
| bool nfc_is_connected() { |
Contributor
There was a problem hiding this comment.
All those NFC-F defines and LM_SEL_RES is not used anymore, could be removed
| if (err != RFAL_ERR_NONE) { | ||
| return NFC_ERROR; | ||
| } | ||
| nfc_status_t err = nfc_transcieve_blocking( |
| void nfc_get_state(nfc_state_t *state); | ||
|
|
||
| // Read the general device information of the activated NFC device. | ||
| nfc_status_t nfc_dev_read_info(nfc_dev_info_t *dev_info); |
Contributor
There was a problem hiding this comment.
I think we should remove nfc_dev_write_ndef_uri functions from this layer and use nfc_transceive function instead in upper layer to read/write ndef .. Right now this may limit us to T4T devices, but for now should be fine.
Author
There was a problem hiding this comment.
fixed f318efa76ed15bf76f41ab246f56e7e2cd28fdc2
|
|
||
| // Read the general device information of the activated NFC device. | ||
| nfc_status_t nfc_dev_read_info(nfc_dev_info_t *dev_info); | ||
| /** @brief Read the general device information of the activated NFC device. */ |
Contributor
There was a problem hiding this comment.
We should also add parameters and return value context into the doc
| bool nfc_get_event(nfc_event_t *event); | ||
|
|
||
| /** @brief Get current state of NFC device. */ | ||
| void nfc_get_state(nfc_state_t *state); |
Contributor
There was a problem hiding this comment.
We should keep nfc_get_state function, this is essential since it should come in a pair with nfc_get_event function . It may be placed in the nfc_poll.c to conveniently connect it to state evaluated there. Also we should keep to define the state in the header file to be publicly available.
I would rather remove the connected flag in the internal nfc handler.
FarskyL
force-pushed
the
lfarsky/nfc/convert_to_polling
branch
from
e1aa816 to
2dab336
Compare
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
3 participants
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.
This PR converts NFC RFAL library to be called by sysevent loop.
In prodtest functions the developer must handles only start and stop of NFC discovery + handling of generated NFC events.
For NFC card reader, the RFAL configuration was extracted to const structure.