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

NFC APIs Design Specification #7426

Merged
merged 28 commits into from Sep 2, 2018

Conversation

Projects
None yet
7 participants
@donatieng
Member

donatieng commented Jul 5, 2018

Description

This pull request includes the design specification for adding NFC APIs in mbed OS, with a support for card emulation and NFC EEPROMs and NDEF encoding/decoding.

The design document for review can be found here: Design Document

It follows guidelines defined in this PR: #7561.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[x] Feature
[ ] Breaking change

@donatieng donatieng requested a review from ARMmbed/mbed-os-pan Jul 5, 2018

@cmonr cmonr requested a review from kjbracey-arm Jul 5, 2018

@cmonr cmonr added the needs: review label Jul 5, 2018

@donatieng donatieng changed the title from NFC Design Specification to NFC APIs Design Specification Jul 5, 2018

```
These methods called when a remote initiator (the local controller is acting as a target) or a remote target (the local controller is acting as an initiator) is detected.
Shared pointers are used so that the user does not have to maintain the lifetime of these objects. The `NFCController` instance will release its reference when the endpoint is lost (see below).

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

If these objects are copyable then it might be better to pass them by value or const/reference and let the user code handle the copy if it is required.

This comment has been minimized.

@donatieng

donatieng Aug 3, 2018

Member

Thanks, I need to check if this is achievable but if so it makes sense.

This comment has been minimized.

@pan-

pan- Aug 14, 2018

Member

Any update on that one ?

```cpp
bool is_ndef_supported();
size_t nfc_tag_type();

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

Why use a size_t here ?

This comment has been minimized.

@donatieng

donatieng Aug 3, 2018

Member

Good point will switch to an enum

bool is_ndef_supported();
size_t nfc_tag_type();
void set_ndef_message(const NDEFMessage& message);

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

There is mismatch with the diagram for set_ndef_message, clear_ndef_message, get_ndef_message.
Does it returns nfc_err_t ?

virtual void on_deselected();
```
Some phones/readers 'park' a target and re-select it later - these events let the user know if the what state the local target is being put in.

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

typo: if the what

Some phones/readers 'park' a target and re-select it later - these events let the user know if the what state the local target is being put in.
```cpp
virtual void on_before_ndef_read();

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

Maybe the naming on_ndef_read_request and on_ndef_written would be more appropriate. I also wonder if the word message shouldn't be added to be more consistent with other delegates (see NFCTargetDelegate) What do you think ?

A NDEF Message is made of multiple NDEF Records which is reflected by the API:
```cpp
bool parse(const uint8_t* buffer, size_t sz)

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

What is the purpose of the parse function ? Does it parse or does it validate then copy an NDEFMessage from a byte stream ?

All arrays are passed by reference (no copy made).
```cpp
static bool parse(const uint8_t* buffer, size_t max_sz)

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

Is it parse or validate ?

static bool parse(const uint8_t* buffer, size_t max_sz)
ssize_t build(const uint8_t* buffer, size_t max_sz)
uint8_t tnf()

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

What is tnf ?

The following events must be called to signal completion of long operations:
```cpp
void on_backend_has_read_bytes(bool success, const uint8_t* bytes)

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

I think we can do better naming wise. I'm not sure that the on_backend prefix is required there.
Also I think it can be valuable to get a more descriptive error code.

Note: Shouldn't we get the size of the bytes read in the callback ?

# Dependencies
* Event Queue
There are currently at least four event queues (Plaftorm, BLE, USB, IP) in mbed OS and NFC will also require an event queing mechanism. We should aim at reusing one of these existing queues with the long term goal of unifying these code bases.

This comment has been minimized.

@pan-

pan- Jul 19, 2018

Member

This can be solved by not instantiating the event queue inside the NFC stack but rather let the application provide it.
Dependency injection delegate the layout of the program to the application writer. Ble already does that for this reason.

@pan-

This comment has been minimized.

Member

pan- commented Jul 19, 2018

One think I forgot to mention is naming consistency towards event handler classes. Should it be ClassNameDelegate, ClassNameEventHandler, ClassName::EventHandler ?

It would be nice to pick one and stick with it at the OS scale. So it become a recognizable pattern.

pan- and others added some commits Jul 30, 2018

NFC: Amend ndef parsing design.
* Decouple Message parsing from Record parsing
* Decouple record parsing from Common objects
* Provide an easy to use parser that parse the most common objects.
Merge pull request #1 from pan-/nfc-spec
NFC: Amend ndef parsing design.
@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

@donatieng Making sure, is the idea that once this is done with review, that it will be merged into Mbed OS' master branch, closed, with code changes coming, or should this go into its own feature branch?

@cmonr cmonr referenced this pull request Aug 14, 2018

Merged

[RFC] SPI HAL Specification #7671

@donatieng donatieng force-pushed the donatieng:nfc-spec branch from f7ff432 to f8b42be Aug 14, 2018

@donatieng

This comment has been minimized.

Member

donatieng commented Aug 14, 2018

@cmonr This one should land on master and will be followed by a subsequent PR for the implementation (targeting master as well). Aiming at having this one finalized by end of week.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Aug 14, 2018

@pan- Please confirm if you are happy with this now ?

@pan-

pan- approved these changes Aug 14, 2018

@adbridge adbridge added needs: CI and removed needs: review labels Aug 15, 2018

@adbridge adbridge requested a review from AnotherButler Aug 15, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

@donatieng Please take a look and address @AnotherButler's comments.

@donatieng

This comment has been minimized.

Member

donatieng commented Aug 31, 2018

@AnotherButler thanks for the review, comments addressed.
This PR is up to date and ready to go from our standpoint.

@cmonr cmonr added needs: CI and removed needs: work labels Aug 31, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 1, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 1, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 1, 2018

Known failure, will be restarted once Connectivity drivers are in

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 1, 2018

It's in!
/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 2, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 2, 2018

@0xc0170

0xc0170 approved these changes Sep 2, 2018

@0xc0170 0xc0170 merged commit c6e18f9 into ARMmbed:master Sep 2, 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 , RTOS ROM(+0.0%) RAM(+0.09%)
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, 563 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9853 cycles (+588 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 (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Sep 2, 2018

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

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