Add initial NFC support to Mbed OS #7822

donatieng · 2018-08-17T19:03:08Z

Description

This PR adds initial NFC support to mbed OS:

NDEF encoding/decoding
Support for NFC EEPROMs including HAL API
Initial support for NFC controllers in card emulation mode (NXP PN512 support)

Design document and discussions are held in PR #7426.
This PR depends on ~~#7815~~ (merged) and ~~#7828~~ (merged).

Please do not merge for now, we still need to perform more testing but the PR is feature-complete so ready for code review.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Breaking change

cmonr · 2018-08-17T21:09:38Z

Wow, you weren't kidding about "by end of week".

paul-szczepanek-arm · 2018-08-20T08:11:32Z

features/nfc/stack/nfc_errors.h

+#ifndef NFC_ERRORS_H_
+#define NFC_ERRORS_H_
+
+#define NFC_OK                    0   ///< No error


cool, I should use these, let's make them into a SafeEnum so it's nice and type safe

paul-szczepanek-arm · 2018-08-20T10:38:05Z

features/nfc/nfc/NFCEEPROMDriver.h

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


all these headers are redundant (last one tries to include self?)

paul-szczepanek-arm · 2018-08-20T10:45:43Z

features/nfc/nfc/NFCEEPROMDriver.h

+             * 
+             * @note this function can be called in interrupt context
+             */
+            virtual void on_event() = 0;


We can make this simpler by handing over the event queue which you already have in the nfc target

See:
ARMmbed/mbed-nfc-m24sr@ec0d191#diff-3856d36332fc549ca6b08cafbfcac790

ithinuel · 2018-08-20T13:18:53Z

features/nfc/nfc/NFCControllerDriver.h

+        /**
+         * The NFCControllerDriver delegate
+         */ 
+        struct Delegate {


Is this struct meant to be private ?

ithinuel · 2018-08-20T13:30:14Z

features/nfc/nfc/ISO7816App.h

+    /**
+     * This base class represents an ISO7816-4 application.
+     */ 
+    class ISO7816App {


This class is completely private, it only has a friend Type4RemoteInitiator. Is that intended ?

ithinuel · 2018-08-20T14:12:37Z

features/nfc/controllers/PN512Driver.h

+namespace nfc {
+
+    struct PN512TransportDriver;
+    class PN512Driver : public NFCControllerDriver, private PN512TransportDriver::Delegate {


Is PN512TransportDriver referring to the struct PN512TransportDriver or to the included class PN512TransportDriver ?
From the ::Delegate it looks like it is the included class but then what is the use of the struct PN512TransportDriver line 28 ?

ithinuel · 2018-08-20T14:30:40Z

features/nfc/nfc/NFCDefinitions.h

+namespace nfc {
+    struct nfc_rf_protocols_bitmask_t
+    {
+        uint8_t initiator_t1t : 1;


Those elements are 1 bit wide, wouldn't that make sense to make them boolean ?

ithinuel · 2018-08-20T15:53:24Z

features/nfc/nfc/NFCController.h

+        nfc_err_t cancel_discovery();
+
+    private:
+        friend class NFCRemoteEndpoint;


NFCController does not need to have NFCRemoteEndpoint as a friend because it's not using any of NFCController's private member.

ithinuel · 2018-08-20T16:29:51Z

features/nfc/nfc/NFCController.h

+        friend class NFCRemoteEndpoint;
+        friend class NFCRemoteInitiator;
+        friend class Type4RemoteInitiator;
+        nfc_transceiver_t* transceiver() const;


This method is :

private

non-virtual

only returning the private member _transceiver.

Type4RemoteInitiator is a friend so it could also directly access the member.
Or making this method public would allow to remove the need for to be a friend.

ithinuel · 2018-08-20T16:30:55Z

features/nfc/nfc/NFCController.h

+
+    private:
+        friend class NFCRemoteEndpoint;
+        friend class NFCRemoteInitiator;


NFCController does not need to have NFCRemoteInitiator as a friend because it's not using any of NFCController's private member.

ithinuel · 2018-08-20T16:37:30Z

features/nfc/source/nfc/NFCController.cpp

+                Type4RemoteInitiator type4_remote_initiator_ptr = new (std::nothrow) Type4RemoteInitiator(_transceiver, _ndef_buffer, _ndef_buffer_sz);
+                if( type4_remote_initiator_ptr == NULL ) {
+                    // No memory :(
+                    SharedPtr<Type4RemoteInitiator> type4_remote_initiator( type4_remote_initiator_ptr );


Is calling _delegate->on_nfc_initiator_discovered with a NULL shared pointer intended ?

ithinuel · 2018-08-20T16:46:06Z

features/nfc/stack/transceiver/pn512/pn512_hw.h

+ */
+static inline void pn512_hw_write(pn512_t* pPN512, uint8_t addr, uint8_t* buf, size_t len)
+{
+  nfc_transport_write(((nfc_transceiver_t*)pPN512)->pTransport, addr, buf, len);


This type of cast are unnecessary and can lead to unexpected error if the structure gets reordered to reduce padding or a new member gets inserted there.

ithinuel · 2018-08-20T16:46:51Z

features/nfc/stack/transceiver/pn512/pn512_irq.c

@@ -0,0 +1,34 @@
+/**


This file does not contain any code.

cmonr · 2018-08-25T00:25:39Z

@donatieng Please take a look at the Travis doc test failures.

mbed-ci · 2018-08-29T22:31:34Z

Test : FAILURE

Build number : 2701
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7822/2701

cmonr · 2018-08-29T22:33:45Z

@donatieng Ignore the test result for now. We're looking into an issue that's blocking #7844. Will restart when able to.

mbed-ci · 2018-08-29T23:51:15Z

Exporter Build : FAILURE

Build number : 2573
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7822/

cmonr · 2018-08-29T23:58:43Z

Cool. Looks like this only failed because the license server is down.

mbed-ci · 2018-08-30T00:27:36Z

Test : SUCCESS

Build number : 2706
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7822/2706

0xc0170 · 2018-08-30T07:47:56Z

/morph build

mbed-ci · 2018-08-30T09:05:59Z

Build : SUCCESS

Build number : 2962
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7822/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-08-30T11:05:18Z

Exporter Build : SUCCESS

Build number : 2577
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7822/

0xc0170 · 2018-08-30T11:36:03Z

/morph test

mbed-ci · 2018-08-30T16:40:18Z

Test : FAILURE

Build number : 2713
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7822/2713

cmonr · 2018-08-30T16:42:03Z

Stopping and restarting. NucleoF401 test failure, but doesn't seem to be related to PR.

/morph test

mbed-ci · 2018-08-30T22:36:11Z

Test : FAILURE

Build number : 2716
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7822/2716

cmonr · 2018-08-31T04:16:29Z

/morph test

mbed-ci · 2018-08-31T07:29:29Z

Test : FAILURE

Build number : 2721
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7822/2721

0xc0170 · 2018-08-31T07:30:44Z

@ARMmbed/mbed-os-ipcore the latest failure (SYNCHRONOUS_DNS_CACHE
) for nucleo f7, can you reproduce ?

mikaleppanen · 2018-08-31T07:39:34Z

@0xc0170 Some revisions of Nucleo F7 boards have knows issues with ethernet hardware. I'll test this locally to see how often problem occurs.

0xc0170 · 2018-08-31T08:05:11Z

@0xc0170 Some revisions of Nucleo F7 boards have knows issues with ethernet hardware. I'll test this locally to see how often problem occurs.

You can fetch the binary used for testing/build your own on master to compare results.Thanks for the help, we need to minimize the failures like this these days.

mikaleppanen · 2018-08-31T08:17:07Z

Cannot repeat locally here with Nucleo F746ZG. Since test is measuring time for DNS requests could be that time values configured to test should be tuned. This error needs to monitored if it happens regularly. Asynchronous DNS tests have had similar test for some time, and I have not heard about failures on it.

Nucleo F7 boards with "A" revision of the chip have problems with Ethernet (#6262). Could you check that boards used for Ethernet testing are revision "Z". Using "A" revision can result random failures on Ethernet.

0xc0170 · 2018-08-31T08:49:37Z

Nucleo F7 boards with "A" revision of the chip have problems with Ethernet (#6262). Could you check that boards used for Ethernet testing are revision "Z". Using "A" revision can result random failures on Ethernet.

@studavekar @cmonr Can you verify please

cmonr · 2018-08-31T16:21:24Z

@mikaleppanen @0xc0170
It looks like we have neither. The version label on the bottom indicates they're a revision 'B'.

donatieng · 2018-08-31T17:23:50Z

If it becomes too critical, we can revert to a239c5e which is just missing exporter build

cmonr · 2018-08-31T18:37:40Z

/morph test

mbed-ci · 2018-08-31T23:58:32Z

Test : SUCCESS

Build number : 2729
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7822/2729

donatieng added the do not merge label Aug 17, 2018

donatieng requested review from kjbracey and a team August 17, 2018 19:03

cmonr requested a review from a team August 17, 2018 21:09

cmonr added needs: preceding PR needs: review labels Aug 17, 2018

paul-szczepanek-arm reviewed Aug 20, 2018

View reviewed changes

ithinuel suggested changes Aug 20, 2018

View reviewed changes

donatieng force-pushed the nfc-impl branch 2 times, most recently from bc2c8e7 to 71f7156 Compare August 21, 2018 17:26

donatieng changed the title ~~Add initial NFC support to mbed OS~~ Add initial NFC support to Mbed OS Aug 21, 2018

cmonr added needs: work risk: G and removed needs: review labels Aug 23, 2018

cmonr mentioned this pull request Aug 24, 2018

Platform: Add C++ Span class. #7828

Merged

cmonr added risk: A and removed risk: G labels Aug 24, 2018

donatieng removed the do not merge label Aug 24, 2018

Donatien Garnier added 8 commits August 28, 2018 09:58

Adding initial API

8d03c55

Added nfc_tag_type() and fixed some const qualifiers

04304e0

Move nfc_tag_type_t to NFCDefinitions.h

18b2bb3

Remove redundant boolean parameters in NDEF Capable

da4842d

NFCTarget should not inherit from NFCEndpoint

3321a59

Missing end of comment

1d61ab6

Add has_started_session() in NFC EEPROM driver delegate

0e9e2fd

Simplify NFCNDEFCapable.h

5ae65aa

paul-szczepanek-arm added 2 commits August 30, 2018 08:37

fix gt test

be5527b

removed unnecessary headers

f3c3abd

paul-szczepanek-arm approved these changes Aug 30, 2018

View reviewed changes

pan- approved these changes Aug 30, 2018

View reviewed changes

cmonr merged commit f82feec into ARMmbed:master Sep 1, 2018

0xc0170 removed the needs: CI label Sep 1, 2018

cmonr removed the risk: A label Sep 2, 2018


		#include <stdint.h>

		#include "NFCDefinitions.h"

Add initial NFC support to Mbed OS #7822

Add initial NFC support to Mbed OS #7822

Uh oh!

Conversation

donatieng commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

cmonr commented Aug 17, 2018

Uh oh!

paul-szczepanek-arm Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Aug 25, 2018

Uh oh!

mbed-ci commented Aug 29, 2018

Uh oh!

cmonr commented Aug 29, 2018

Uh oh!

mbed-ci commented Aug 29, 2018

Uh oh!

cmonr commented Aug 29, 2018

Uh oh!

mbed-ci commented Aug 30, 2018

Uh oh!

0xc0170 commented Aug 30, 2018

Uh oh!

mbed-ci commented Aug 30, 2018

Triggering tests

Uh oh!

mbed-ci commented Aug 30, 2018

Uh oh!

0xc0170 commented Aug 30, 2018

Uh oh!

mbed-ci commented Aug 30, 2018

Uh oh!

cmonr commented Aug 30, 2018

Uh oh!

mbed-ci commented Aug 30, 2018

Uh oh!

cmonr commented Aug 31, 2018

Uh oh!

mbed-ci commented Aug 31, 2018

Uh oh!

0xc0170 commented Aug 31, 2018

Uh oh!

mikaleppanen commented Aug 31, 2018

Uh oh!

0xc0170 commented Aug 31, 2018

Uh oh!

mikaleppanen commented Aug 31, 2018

Uh oh!

0xc0170 commented Aug 31, 2018

Uh oh!

donatieng commented Aug 17, 2018 •

edited

Loading

paul-szczepanek-arm Aug 20, 2018 •

edited

Loading