-
Notifications
You must be signed in to change notification settings - Fork 33
nfc: add tnep handling #606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The NFC TNEP signalling must be provided by sdk-nrf-bm. Signed-off-by: Andrzej Kuros <[email protected]>
The Kconfig NFC_TNEP_TAG_SIGNALLING choice is given the NFC_TNEP_TAG_SIGNALLING_BM option for Bare Metal platform. This option enables the NFC TNEP tag signalling appropriate for Bare Metal platform, where Zephyr primitives are not available. Signed-off-by: Andrzej Kuros <[email protected]>
|
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
|
You can find the documentation preview for this PR here. |
c06721c to
284f36a
Compare
The sample is based on sdk-nrf samples/nfc/tnep_tag sample and it's behavior is the same. Signed-off-by: Andrzej Kuros <[email protected]>
284f36a to
a61cb3c
Compare
|
|
||
| #include <stdint.h> | ||
|
|
||
| int nfc_tnep_tag_signalling_init(void); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add description to the API
| int err; | ||
| static const char svc_one_msg[] = "Service pi = 3.14159265358979323846"; | ||
|
|
||
| printk("Service one selected\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you decide which log backend is used, here and below are printk and in main() function also LOG_ are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix, thanks.
| err = nfc_ndef_msg_record_add(&NFC_NDEF_MSG(app_msg), | ||
| &NFC_NDEF_TEXT_RECORD_DESC(svc_one_rec)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for nfc_ndef_msg_record_add()
| static void tnep_svc_one_msg_received(const uint8_t *data, size_t len) | ||
| { | ||
| int err; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| static void tnep_svc_one_msg_received(const uint8_t *data, size_t len) | |
| { | |
| int err; | |
| static void tnep_svc_one_msg_received(const uint8_t *data, size_t len) | |
| { | |
| ARG_UNUSED(data); | |
| ARG_UNUSED(len); | |
| int err; | |
Do the same in the functions below where arguments are not used
| err = bm_buttons_init(configs, ARRAY_SIZE(configs), BM_BUTTONS_DETECTION_DELAY_MIN_US); | ||
| if (err) { | ||
| return err; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| err = bm_buttons_init(configs, ARRAY_SIZE(configs), BM_BUTTONS_DETECTION_DELAY_MIN_US); | |
| if (err) { | |
| return err; | |
| } | |
| int err = bm_buttons_init(configs, ARRAY_SIZE(configs), BM_BUTTONS_DETECTION_DELAY_MIN_US); | |
| if (err) { | |
| return err; | |
| } |
then you can remove line 240
| static const uint8_t svc_two_uri[] = "svc:e"; | ||
|
|
||
| static uint8_t tag_buffer[NDEF_TNEP_MSG_SIZE]; | ||
| static uint8_t tag_buffer2[NDEF_TNEP_MSG_SIZE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| static uint8_t tag_buffer2[NDEF_TNEP_MSG_SIZE]; | |
| static uint8_t swap_buffer[NDEF_TNEP_MSG_SIZE]; |
?
| enum tnep_event e = atomic_get(msg_event); | ||
|
|
||
| if (e != TNEP_EVENT_DUMMY) { | ||
| (void)atomic_cas(msg_event, e, TNEP_EVENT_DUMMY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When atomic_cas fails then the event may be duplicated. The atomic_cas() result should be handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree, if the atomic_cas fails it simply means that between atomic_get and atomic_cas (the write inside) the value of event has changed. The return value is intentionally ignored in this case to not loose the new updated value. There is no duplication of event.
| if (data_length > 0) { | ||
| nfc_tnep_tag_rx_msg_indicate(nfc_t4t_ndef_file_msg_get(data), | ||
| data_length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about:
| if (data_length > 0) { | |
| nfc_tnep_tag_rx_msg_indicate(nfc_t4t_ndef_file_msg_get(data), | |
| data_length); | |
| if (nfc_t4t_ndef_file_msg_get(data_length) > 0) { | |
| nfc_tnep_tag_rx_msg_indicate(nfc_t4t_ndef_file_msg_get(data), | |
| nfc_t4t_ndef_file_msg_get(data_length)); |
Due to fact that that nfc_t4t_ndef_file_msg_get() gets data with offset this should be safer.
https://github.com/nrfconnect/sdk-nrf/blob/cd414710184846fd1a3bd7213db5ae8199a801b9/include/nfc/t4t/ndef_file.h#L43
| err = nfc_ndef_msg_record_add(&NFC_NDEF_MSG(app_msg), | ||
| &NFC_NDEF_TEXT_RECORD_DESC(svc_two_rec)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling.
| err = nfc_tnep_tag_initial_msg_create(TNEP_INITIAL_MSG_RECORD_COUNT, | ||
| tnep_initial_msg_encode); | ||
| if (err) { | ||
| printk("Cannot create initial TNEP message, err: %d\n", err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing goto fail;?
Background:
The NFC TNEP handling is encapsulated by sdk-nrf file
subsys/nfc/tnep/tag.c.This file relied on Zephyr-provided primitives like
k_poll_event, which are not available in Bare Metal option.Changes:
This PR adds NFC TNEP handling to the Bare Metal by:
subsys/nfc/tnep/tag.cimplementation from signalling implementation. The implementationsubsys/nfc/tnep/tag.cwith these modifications is re-used in Bare Metal.subsys/nfc/tnep/tag_signalling_bm.c) using only atomic operations.