Introduce NFC driver with RFAL middleware

[kopecdav]

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.

[github-actions]

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)

[cepetr]

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

[TychoVrahe]

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

[cepetr]

yes, I flipped it:-)

[kopecdav]

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.

[cepetr]

There are still some places that use camelCase (e.g., txBuf, txBufSize, rcvLen). Of course, we should not change the rfal source code.

[TychoVrahe]

sure static const is even better

[TychoVrahe]

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

[cepetr]

The constant 7 corresponds to NDEF_MESSAGE_URI_OVERHEAD.

The code looks fine, but I would personally simplify it a bit - it’s up to you. If we have more code like this - and it looks like we have, it might be preferable to create a small helper function for buffer reading/writing. That would simplify the code and make it safer.

size_t ndef_create_uri(const char *uri, uint8_t *buffer, size_t buffer_size) {
  size_t uri_len = strlen(uri);

  if (buffer_size < (uri_len + NDEF_MESSAGE_URI_OVERHEAD)) {
    return 0;  // Not enough room to create URI
  }

  uint8_t* p = buffer;
  *p++ = 0x3;  // TLV header
  *p++ = uri_len + 5;  // uri + record header
  *p++ = 0xD1;  // NDEF record Header
  *p++ = 0x1;  // Type length
  *p++ = uri_len;  // Payload length
  *p++ = 0x55;  // URI type
  *p = 0x1;  // URI abreviation
  memcpy(p, uri, uri_len);
  p += uri_len;
  *p++ = 0xFE;  // TLV termination

  return dst - buffer;
}

[cepetr]

You are mixing size_t and uint16_t. The strlen() function returns a size_t. Perhaps we should consistently use size_t for buffer sizes and possibly for buffer indices as well. This also applies to ndef_parse_message(), where there is a mix of uint16_t and int16_t.

[cepetr]

Here, false is returned, and a few lines later, NFC_ERROR is returned.

[cepetr]

xxx_deinit() functions should not return error codes. They are called in situations where errors are not expected or cannot be handled. Therefore, the xxx_deinit() function should be designed to succeed in all cases. Ideally, even if the driver is only partially initialized, xxx_deinit() can still be called from the xxx_init() routine.

[cepetr]

If we return from the _init() function in this way, we leave the driver partially initialized. I know we don’t do it consistently in our code base, but a good approach is to use a goto/cleanup pattern and call the _deinit() routine in the cleanup section.

[cepetr]

st25r3916b_driver_t *drv = ...

[cepetr]

It would be better to maintain consistency with variable names (e.g., buffer_len,len, buffer_size).

[cepetr]

ndef_parse_record() returns an error code, but it's not checked here.