Introduce NFC driver with RFAL middleware #4566

kopecdav · 2025-01-31T12:42:19Z

This PR Introduce NFC driver with RFAL middleware to control ST25R3916.

RFAL is a ST middleware which provides a low level control of the ST25R3916 + an extra layer to support several different NFC standards.

PR also introduce prodtests to READ, EMULATE and WRITE to NFC card.

… serial interface [no changelog]

github-actions · 2025-01-31T15:49:07Z

core UI changes	device test	click test	persistence test
T2T1 Model T	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T3B1 Safe 3	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T3T1 Safe 5	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
All	main(screens)

…rev.B [no changelog]

… board rev.B [no changelog]

core/embed/io/nfc/inc/io/nfc.h

core/embed/io/nfc/st25r3916b/nfc.c

core/embed/io/nfc/st25r3916b/ndef.c

core/embed/projects/prodtest/cmd/prodtest_nfc.c

core/embed/io/nfc/inc/io/nfc.h

core/embed/io/nfc/st25r3916b/nfc.c

cepetr · 2025-02-06T13:31:52Z

core/embed/io/nfc/st25r3916b/nfc.c

+
+#include <sys/irq.h>
+#include <sys/systick.h>
+#include <trezor_bsp.h>


There is no strict rule on how to order #include statements, but in new code, we follow this convention:

#include <trezor_xxx.h>. <-- comes first // empty line #include <module/header.h> <-- comes next // empty line #include "local_header.h" <--- comes last

There is no strict rule on how to order #include statements, but in new code, we follow this convention:

Hope to fix correctly in d0ecd9f

Maybe #include <io/nfc.h> belongs to the second category. Otherwise it looks good.

fixed in bd5de3a

cepetr · 2025-02-06T13:33:14Z

core/embed/io/nfc/st25r3916b/nfc.c

+                             */
+    0x00,    0x00};         /* RD                                       */
+
+static ReturnCode nfc_transcieve_blocking(uint8_t *txBuf, uint16_t txBufSize,


In new code, we prefer camelCase over snake_case. camelCase is also used in other parts of the file.

wait do we? i am confused now, i believe snake_case is almost exclusively used in our C code

yes, I flipped it:-)

I changed the return type to the nfc_status_t but in rest of the code I have everything in snake_case i think. Unfortunatelly the RFAL is written in camelCase so the code looks bit wierd :/. But I dont think we can do anything about it.

core/embed/io/nfc/st25r3916b/ndef.h

core/embed/projects/prodtest/cmd/prodtest_nfc.c

cepetr · 2025-02-06T14:00:56Z

core/embed/io/nfc/st25r3916b/nfc.c

+      return NFC_ERROR;
+    }
+
+    char *uid_str = hex2Str(nfcDevice->nfcid, nfcDevice->nfcidLen);


Consider replacing hex2Str with cstr_encode_hex from <rtl/strutils.>

Didnt know we already have it 😮 , fixed d0ecd9f but not tested yet, please leave open

Yes, you couldn't have known. It's only been there for a few days.

core/embed/io/nfc/st25r3916b/nfc.c

cepetr · 2025-02-06T14:07:16Z

core/embed/io/nfc/st25r3916b/nfc.c

+
+uint32_t nfc_create_timer(uint16_t time) { return (systick_ms() + time); }
+
+bool nfc_timer_is_expired(uint32_t timer) {


see ticks_expired() in sys/systick.h

fixed in d0ecd9f

Maybe you don't need these functions at all. Why not use systick_xxxz in rfal_platform.h` directly?

right, fixed in bd5de3a

cepetr · 2025-02-07T08:29:05Z

core/embed/io/nfc/inc/io/nfc.h

+
+#pragma once
+
+#include "trezor_types.h"


We should include this header using <...> instead.

fixed in bd5de3a

cepetr · 2025-02-07T08:35:14Z

core/embed/io/nfc/st25r3916b/nfc.c

+#include "rfal_platform.h"
+
+#define LM_SEL_RES \
+  0x20U /*!<NFC-A SEL_RES configured for Type 4A Tag Platform    */


Our clang-format doesn’t handle this type of comment well. How about placing it on its own line before the definition, like this:

// NFC-A SEL_RES configured for Type 4A Tag Platform #define LM_SEL_RES 0x20

fixed in bd5de3a

cepetr · 2025-02-07T08:38:29Z

core/embed/io/nfc/st25r3916b/nfc.c

+#define LM_SC_BYTE2 \
+  0xFCU /*!<NFC-F System Code byte 2                             */
+#define LM_PAD0 \
+  0x00U /*!<NFC-F PAD0                                           */


Even though there's nothing inherently wrong with multiline comments, we don’t use them often and usually prefer single-line comments (except for the license header). These comments also contain Doxygen inline annotation syntax /*!<...

fixed in bd5de3a

cepetr · 2025-02-07T08:42:20Z

core/embed/projects/prodtest/cmd/prodtest_nfc.c

+      cli_trace(cli, "NFC Type AP2P: UID: %s", dev_info.uid);
+      break;
+    case NFC_DEV_TYPE_UNKNOWN:
+      cli_error(cli, CLI_ERROR, "NFC Type UNKNOWN");


In the case of this error, both cli_error() and cli_error() are called.

you mean cli_error() and cli cli_ok() right? changed break -> cleanup
fixed in bd5de3a

cepetr · 2025-02-07T08:46:43Z

core/embed/projects/prodtest/cmd/prodtest_nfc.c

+
+  nfc_deinit();
+
+  cli_ok(cli, "");


nfc_deinit() is not always called. How about this:

... cli_ok(cli, ""); cleanup: nfc_deinit(); }

then, instead of return, you can use goto cleanup

The similar issue exists in prodtest_nfc_emulate_card and prodtest_nfc_write_card

fixed in bd5de3a

cepetr · 2025-02-07T08:59:09Z

core/embed/io/nfc/st25r3916b/ndef.c

+  if (*buffer == 0xFF) {
+    // TLV 3 byte length format
+    buffer++;
+    message->message_total_len = (int16_t)(buffer[0] << 8 | buffer[1]);


If the message is corrupted, message_total_len might be initialized to a nonsense value. I think we should check whether message_total_len fits within the buffer passed to the function.

fixed in bd5de3a

cepetr · 2025-02-07T09:05:21Z

core/embed/io/nfc/st25r3916b/ndef.c

+  }
+
+  if (rec->type_length > 0) {
+    memcpy(&(rec->type), buffer + bp, rec->type_length);


hese parentheses are not necessary (personally, I find them a bit unusual and slightly confusing):

&(rec->type) ->&rec->type

A similar pattern appears in the code above: &(message->records[message->records_cnt]).

fixed in bd5de3a

cepetr · 2025-02-07T09:17:03Z

core/embed/io/nfc/st25r3916b/ndef.c

+    buffer++;
+  }
+
+  remaining_len = message->message_total_len;


The general issue with these parsing functions is that they do not properly validate the input data. If the data is corrupted (for any reason), the function may access memory beyond the buffer. But I understand that the code is still somewhat experimental, so this is not a critical issue for now.

See my comment above.

cepetr · 2025-02-07T09:20:32Z

core/embed/io/nfc/st25r3916b/ndef.c

+}
+
+uint16_t ndef_create_uri(const char *uri, uint8_t *buffer) {
+  *buffer = 0x3;  // TLV header


I would be much better not to rely on the caller it provide a sufficient buffer. Instead, we should take a more defensive approach and modify the function to have buffer_size argument:

size_t ndef_create_uri(const char* uri, uint8_t* buffer, size_t buffer_size);

fixed in bd5de3a

cepetr · 2025-02-07T09:28:22Z

core/embed/io/nfc/st25r3916b/ndef.c

+  }
+
+  // Look at first byte, parse header
+  memcpy(&(rec->header), buffer, 1);


I'm not sure we always need memcpy here. In some cases, a simple assignment would be enough. However, if you insist on using memcpy, it would be safer to use sizeof instead of a constant, like this:

memcpy(&rec->header, &buffer[bp], sizeof(rec->header); bp += sizeof(rec->header);

Personally, I prefer just:

rec->header = buffer[bp++];

header was a non-scalar type variable since i split the struct into bitfields so direct assignment was not possible. I changed the header into union so i can access it as a single byte.

TychoVrahe · 2025-02-07T09:08:48Z

core/embed/io/nfc/st25r3916b/card_emulation.h

+#endif
+
+/* Includes ------------------------------------------------------------------*/
+#include <trezor_bsp.h>


include trezor_types instead

fixed in bd5de3a

TychoVrahe · 2025-02-07T09:11:21Z

core/embed/io/nfc/st25r3916b/ndef.h

@@ -0,0 +1,44 @@
+#pragma once


include licence

fixed in bd5de3a

TychoVrahe · 2025-02-07T09:26:00Z

core/embed/io/nfc/st25r3916b/nfc.c

+  0x00U /*!<NFC-F PAD0                                           */
+
+/* P2P communication data */
+static uint8_t NFCID3[] = {0x01, 0xFE, 0x03, 0x04, 0x05,


sure static const is even better

TychoVrahe · 2025-02-07T09:30:13Z

core/embed/io/nfc/st25r3916b/nfc.c

+  st25r3916b_driver_t *drv = &g_st25r3916b_driver;
+
+  if(!drv->initialized) {
+    return NFC_NOT_INITIALIZED;


should return nfc_event_t, but this is different type. Either return NFC_NO_EVENT, or maybe better function signature would be bool nfc_get_event(nfc_event_t *event), which also allows adding data to events if needed.

Right, I changed the function structure to nfc_status_t nfc_get_event(nfc_event_t *event) so it returns error if the driver was not initialized as well.

TychoVrahe · 2025-02-07T09:41:26Z

core/embed/io/nfc/st25r3916b/nfc.c

+                             */
+    0x00,    0x00};         /* RD                                       */
+
+static ReturnCode nfc_transcieve_blocking(uint8_t *txBuf, uint16_t txBufSize,


wait do we? i am confused now, i believe snake_case is almost exclusively used in our C code

TychoVrahe · 2025-02-07T10:03:28Z

core/embed/io/nfc/inc/io/nfc.h

+
+typedef enum {
+  NFC_NO_EVENT,
+  NFC_STATE_ACTIVATED,


the polling mechanism is meant to get events, so changes of states, not the state itself. So in this case, we should return NFC_EVENT_ACTIVATED and NFC_EVENT_DEACTIVATED pair. We could have a separate nfc_get_state function that would return current state, but mainly we need the event polling.

right, understood. I renamed the event enum to NFC_EVENT_ACTIVATED, but for now that is the only event provided by nfc_get_event() function. The nfc_get_event() function might be further extended in the future to monitor more events when we have a proper usecase.

TychoVrahe · 2025-02-07T10:05:00Z

core/embed/io/nfc/st25r3916b/nfc_internal.h

@@ -0,0 +1,14 @@
+


include license

fixed in bd5de3a

TychoVrahe · 2025-02-07T10:05:16Z

core/embed/io/nfc/st25r3916b/nfc_internal.h

+
+#pragma once
+
+#include <trezor_bsp.h>


trezor_types shoudl be enough

fixed in bd5de3a

kopecdav added the T3W1 label Jan 31, 2025

kopecdav requested review from TychoVrahe and cepetr January 31, 2025 12:42

kopecdav self-assigned this Jan 31, 2025

kopecdav requested a review from prusnak as a code owner January 31, 2025 12:42

kopecdav added 5 commits January 31, 2025 15:33

feat(core): Introduce nfc module with rfal library [no changelog]

28b4f01

refactor(core): refactor rfal DSI to avoid conflicts with STM display…

c0a5522

… serial interface [no changelog]

feat(core): Redo nfc library API, add NDEF parser

1af9771

feat(core): Integrate NFC READ_TAG test to prodtest

f75fe14

feat(core): Introduce NFC tests into prodtest [no changelog]

3488569

TychoVrahe removed the request for review from prusnak January 31, 2025 15:06

kopecdav force-pushed the kopecdav/T3W1/NFC_driver branch from 2e6bf04 to 4c189eb Compare January 31, 2025 15:47

feat(core): Update to new prodtest structure + add support for board …

606d8c9

…rev.B [no changelog]

kopecdav force-pushed the kopecdav/T3W1/NFC_driver branch from 4c189eb to 606d8c9 Compare February 3, 2025 08:56

fixup! feat(core): Update to new prodtest structure + add support for…

b241955

… board rev.B [no changelog]

TychoVrahe requested changes Feb 4, 2025

View reviewed changes

kopecdav added 2 commits February 6, 2025 09:38

refactor(core): revert style checks in RFAL drivers

3a147fa

fix(core): refactor DSI variable in RFAL to not colide with STM HAL

a3136e2

cepetr requested changes Feb 6, 2025

View reviewed changes

refactor(core): resolve requested changes

d0ecd9f

kopecdav force-pushed the kopecdav/T3W1/NFC_driver branch from 09828c9 to d0ecd9f Compare February 6, 2025 19:17

feat(core): Exlude RFAL middleware from cstyle check

840d7d7

cepetr requested changes Feb 7, 2025

View reviewed changes

TychoVrahe reviewed Feb 7, 2025

View reviewed changes

kopecdav added 2 commits February 7, 2025 13:38

refactor(core): Refactor requested changes from review

bd5de3a

fix(core): Fix ndef uri buffer handling

9ca4e03


		uint32_t nfc_create_timer(uint16_t time) { return (systick_ms() + time); }

		bool nfc_timer_is_expired(uint32_t timer) {

Introduce NFC driver with RFAL middleware #4566

Are you sure you want to change the base?

Introduce NFC driver with RFAL middleware #4566

Conversation

kopecdav commented Jan 31, 2025 • edited Loading

github-actions bot commented Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kopecdav Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kopecdav Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kopecdav commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025 •

edited

Loading

kopecdav Feb 6, 2025 •

edited

Loading

kopecdav Feb 7, 2025 •

edited

Loading