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

Add initial NFC support to Mbed OS #7822

Merged
merged 92 commits into from Sep 1, 2018

Conversation

Projects
None yet
8 participants
@donatieng
Member

donatieng commented Aug 17, 2018

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

@donatieng donatieng requested review from kjbracey-arm and ARMmbed/mbed-os-pan Aug 17, 2018

@cmonr cmonr requested a review from ARMmbed/mbed-os-hal Aug 17, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 17, 2018

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

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

This comment has been minimized.

@paul-szczepanek-arm

paul-szczepanek-arm Aug 20, 2018

Member

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

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

This comment has been minimized.

@paul-szczepanek-arm

paul-szczepanek-arm Aug 20, 2018

Member

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

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

This comment has been minimized.

@paul-szczepanek-arm

paul-szczepanek-arm Aug 20, 2018

Member

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

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

This comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

Is this struct meant to be private ?

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

This comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

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

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

This comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

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 ?

namespace nfc {
struct nfc_rf_protocols_bitmask_t
{
uint8_t initiator_t1t : 1;

This comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

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

nfc_err_t cancel_discovery();
private:
friend class NFCRemoteEndpoint;

This comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

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

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

This comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

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.

private:
friend class NFCRemoteEndpoint;
friend class NFCRemoteInitiator;

This comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

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

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

This comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

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

*/
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 comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

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.

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

This comment has been minimized.

@ithinuel

ithinuel Aug 20, 2018

Member

This file does not contain any code.

@donatieng donatieng force-pushed the donatieng:nfc-impl branch 2 times, most recently from bc2c8e7 to 71f7156 Aug 21, 2018

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

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

@donatieng donatieng removed the do not merge label Aug 24, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 25, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

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

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

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

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 30, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 30, 2018

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

This comment has been minimized.

@pan-

pan- approved these changes Aug 30, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 30, 2018

/morph test

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 30, 2018

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

/morph test

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 31, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 31, 2018

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

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 31, 2018

@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

This comment has been minimized.

Member

0xc0170 commented Aug 31, 2018

@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

This comment has been minimized.

Contributor

mikaleppanen commented Aug 31, 2018

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

This comment has been minimized.

Member

0xc0170 commented Aug 31, 2018

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

This comment has been minimized.

Contributor

cmonr commented Aug 31, 2018

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

@donatieng

This comment has been minimized.

Member

donatieng commented Aug 31, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 31, 2018

/morph test

@mbed-ci

This comment has been minimized.

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

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 558 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10209 cycles (+762 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

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

@cmonr cmonr removed the risk: A label Sep 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment