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]

TychoVrahe · 2025-02-04T08:15:00Z

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

+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef TREZORHAL_NFC_H


please use pragma once, we are sort of in transitory period but we tend to use this in new&modified files (this applies to the other headers as well, but not the 3rd pary ones)

fixed in d0ecd9f

TychoVrahe · 2025-02-04T08:17:32Z

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

+#ifndef TREZORHAL_NFC_H
+#define TREZORHAL_NFC_H
+
+#include <trezor_bsp.h>


Neither trezor_bsp nor trezor_rtl should be included in header files. use trezor_types instead. See the respective headers for instructions

fixed in d0ecd9f

TychoVrahe · 2025-02-04T08:27:13Z

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

+
+#include <trezor_bsp.h>
+#include <trezor_rtl.h>
+#include "ndef.h"


ndef seems not needed now, lets remove it and add it when necessary

fixed in d0ecd9f

TychoVrahe · 2025-02-04T08:28:02Z

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

+  uint8_t uid_len;
+} nfc_dev_info_t;
+
+// Initialize NFC driver including supportive RFAL middlewere


type middleware, also few other typos in comments below (drive, activly,workerwith, NDFE)

fixed in d0ecd9f

TychoVrahe · 2025-02-04T08:31:36Z

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

+nfc_status_t nfc_register_event_callback(nfc_event_t event_type,
+                                         void (*cb_fn)(void));
+
+// Activate the NFC RFAL state machine which will explore the registered


this comment needs some polishing

fixed in d0ecd9f

TychoVrahe · 2025-02-04T09:17:13Z

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

+
+  // Enable clock of relevant peripherals
+  // SPI + GPIO ports
+  SPI_INSTANCE_3_CLK_EN();


lets also add FORCE_RESET & RELEASE RESET of the SPI peripheral

make sense, fixed in d0ecd9f

TychoVrahe · 2025-02-04T09:17:24Z

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

+  GPIO_InitStruct.Pin = SPI_INSTANCE_3_SCK_PIN;
+  HAL_GPIO_Init(SPI_INSTANCE_3_SCK_PORT, &GPIO_InitStruct);
+
+  // NSS pin controled by software, set as classical GPIO


typo controled

fixed in d0ecd9f

TychoVrahe · 2025-02-04T09:18:31Z

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

+      /*******************************************************************************/
+
+      // Card emulators need to promptly respond to the reader commands, so when
+      // activated rfal worker is called seveal times untill back to back


some typos here

fixed in d0ecd9f

TychoVrahe · 2025-02-04T09:19:57Z

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

+
+
+#include "ndef.h"
+#include <stdint.h>


use trezor_types, add license

fixed in d0ecd9f

TychoVrahe · 2025-02-04T09:26:39Z

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

+
+// clang-format off
+
+PRODTEST_CLI_CMD(


add readme entries for nrf commands in prodtest/README.md

fixed in d0ecd9f

cepetr · 2025-02-06T13:00:11Z

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

+} nfc_dev_info_t;
+
+// Initialize NFC driver including supportive RFAL middlewere
+nfc_status_t nfc_init();


In general it's better to explicitly specify that functions have no arguments, so in this case:

nfc_status_t nfc_init(void);

fixed in d0ecd9f

cepetr · 2025-02-06T13:27:16Z

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

+#include "../rfal/include/rfal_t2t.h"
+#include "../rfal/include/rfal_utils.h"
+
+#include "stm32u5xx_hal.h"


This header is already included in <trezor_bsp.h>

fixed in d0ecd9f

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

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.

cepetr · 2025-02-06T13:40:17Z

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

+  ndef_record_t records[NDEF_MAX_RECORDS];
+} ndef_message_t;
+
+ndef_status_t parse_ndef_message(uint8_t *buffer, uint16_t buffer_len,


In these functions it would be better to define input arguments as const pointers.

Agree, fixed in d0ecd9f

cepetr · 2025-02-06T13:51:18Z

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

+
+  uint32_t tick = systick_ms();
+
+  while (!nfc_dev_activated) {  // Todo, while timeout or device found


Consider this patter (that also some the problem with uint32_t wrapping):

uint32_t expire_time = ticks_timeout(timeout); if (tick_expired(expire_time)) { cli_error(...); return; }

good idea, fixed in d0ecd9f

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

cepetr · 2025-02-06T14:05:41Z

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

+  return NFC_OK;
+}
+
+// static void parse_tag_header(uint8_t *data, uint16_t dataLen) {


We usually remove any commented-out code before merging a pull request.

I wanted to keep the parser for the future, but i agree. removed in d0ecd9f

cepetr · 2025-02-06T14:06:40Z

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

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


see ticks_timeout() in sys/systick.h

fixed in d0ecd9f

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

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


		uint32_t tick = systick_ms();

		while (!nfc_dev_activated) { // Todo, while timeout or device found


		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

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

kopecdav commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025 •

edited

Loading

kopecdav Feb 6, 2025 •

edited

Loading