976457 - B2G NFC: API for supporting NFC_A technology

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

This bug is for supporting NfcA (ISO 14443-3A) technology tags.

Comment 1

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Is it required by bug 916428? 
Not sure if KDDI would like to take this.

Comment 2

[Junichi Hashimoto]

Two bugs are independent from each other.
API should be designed for both but NfcA (ISO 14443-3A) implementation is not in our scope currently.

Comment 3

[Dimi Lee [:dimi]]

Yoshi raised a question about the tag technology relationship design, Ex, if ISO_DEP should inherit from NFC_A.
I will check this in this bug.

Comment 4

[Dimi Lee [:dimi]]

Since ISO_DEP may base on NFC_A or NFC_B, also NFC_A and NFC_B do not have directly inheritance relationship between each other. So for this case, i think it may not suitable use inheritance for ISO_DEP : NFC_A or ISO_DEP : NFC_B

Comment 5

[Dimi Lee [:dimi]]

Attachment: WebAPI for supporting NFC_A tags patch v1

This patch basically only support transceive function.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8551663 [details] [diff] [review]
WebAPI for supporting NFC_A tags patch v1

Review of attachment 8551663 [details] [diff] [review]:
-----------------------------------------------------------------

I guess you miss MozNFCTag.webidl.

::: dom/nfc/MozNfcATech.cpp
@@ +39,5 @@
> +/* static */
> +already_AddRefed<MozNfcATech>
> +MozNfcATech::Constructor(const GlobalObject& aGlobal,
> +                           MozNFCTag& aNFCTag,
> +                           ErrorResult& aRv)

Align

@@ +59,5 @@
> +  if (techList.IsNull() || !(techList.Value().Contains(mTechnology))) {
> +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return nullptr;
> +  }
> +

Maybe create a helper to do this.

@@ +88,5 @@
> +    return nullptr;
> +  }
> +
> +  return promise.forget();
> +}

perhaps this too.

::: dom/webidl/MozNfcATech.webidl
@@ +7,5 @@
> +interface MozNfcATech {
> +  /**
> +   * Send raw command to tag and receive the response.
> +   */
> +  [Throws]

You don't have to [Throws]
Bug 1113827.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8551663 [details] [diff] [review]
WebAPI for supporting NFC_A tags patch v1

Review of attachment 8551663 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/MozNfcATech.webidl
@@ +7,5 @@
> +interface MozNfcATech {
> +  /**
> +   * Send raw command to tag and receive the response.
> +   */
> +  [Throws]

Oh, I think I am wrong, ingore this.

Comment 8

[Dimi Lee [:dimi]]

Attachment: WebAPI for supporting NFC_A tags patch v2

Comment 9

[Dimi Lee [:dimi]]

Comment on attachment 8553591 [details] [diff] [review]
WebAPI for supporting NFC_A tags patch v2

Review of attachment 8553591 [details] [diff] [review]:
-----------------------------------------------------------------

I move |static const NFCTechType mTechnology = NFCTechType::ISO_DEP;| to cpp because of bug 1121900,
It seems in certain version of gcc may cause build error by using original coding style

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8553591 [details] [diff] [review]
WebAPI for supporting NFC_A tags patch v2

Review of attachment 8553591 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/MozIsoDepTech.h
@@ +44,5 @@
>  
>    nsRefPtr<nsPIDOMWindow> mWindow;
>    nsRefPtr<MozNFCTag> mTag;
>  
> +  static const NFCTechType mTechnology;

fix Bug 1121900 first.

::: dom/nfc/TagUtils.h
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +/*

Javadoc style.
/**
 * ...
 */

Comment 11

[Thomas Nguyen (:tnguyen)]

(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> Comment on attachment 8551663 [details] [diff] [review]
> WebAPI for supporting NFC_A tags patch v1
> 
> Review of attachment 8551663 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess you miss MozNFCTag.webidl.
> 
> ::: dom/nfc/MozNfcATech.cpp
> @@ +39,5 @@
> > +/* static */
> > +already_AddRefed<MozNfcATech>
> > +MozNfcATech::Constructor(const GlobalObject& aGlobal,
> > +                           MozNFCTag& aNFCTag,
> > +                           ErrorResult& aRv)
> 
> Align
> 
> @@ +59,5 @@
> > +  if (techList.IsNull() || !(techList.Value().Contains(mTechnology))) {
> > +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> > +    return nullptr;
> > +  }
> > +
> 
> Maybe create a helper to do this.
> 
> @@ +88,5 @@
> > +    return nullptr;
> > +  }
> > +
> > +  return promise.forget();
> > +}
> 
> perhaps this too.
> 
> ::: dom/webidl/MozNfcATech.webidl
> @@ +7,5 @@
> > +interface MozNfcATech {
> > +  /**
> > +   * Send raw command to tag and receive the response.
> > +   */
> > +  [Throws]
> 
> You don't have to [Throws]
> Bug 1113827.

We may have diversity tech types tag (NFCA in this bug, and Mifare Ultralight in bug 884478 and maybe other types later). We could refactor to create a based class ( MozBasicTag includes: transceive(...) and checkTechnologySupport(...)
Other tag type class (MozNfcATech for example) will be derived from the based class. It looks better in term of pattern design. Do you agree with that?

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #11)
 > We may have diversity tech types tag (NFCA in this bug, and Mifare
> Ultralight in bug 884478 and maybe other types later). We could refactor to
> create a based class ( MozBasicTag includes: transceive(...) and
> checkTechnologySupport(...)
See MozNFCTag.webidl

> Other tag type class (MozNfcATech for example) will be derived from the
> based class. It looks better in term of pattern design. Do you agree with
> that?

No, this is wrong.
NfcA is a radio *technology*, whereas a MozNFCTag abstracts a NFC Tag.

A tag is not neccessary to have a NFC_A technology, how do you inherit it?

Comment 13

[Thomas Nguyen (:tnguyen)]

(In reply to Yoshi Huang[:allstars.chh] from comment #12)
> (In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #11)
>  > We may have diversity tech types tag (NFCA in this bug, and Mifare
> > Ultralight in bug 884478 and maybe other types later). We could refactor to
> > create a based class ( MozBasicTag includes: transceive(...) and
> > checkTechnologySupport(...)
> See MozNFCTag.webidl
> 
> > Other tag type class (MozNfcATech for example) will be derived from the
> > based class. It looks better in term of pattern design. Do you agree with
> > that?
> 
> No, this is wrong.
> NfcA is a radio *technology*, whereas a MozNFCTag abstracts a NFC Tag.
> 
> A tag is not neccessary to have a NFC_A technology, how do you inherit it?

Thank you.
I mean create a NEW internal based class (for example MozBasicTech) to use later.
MozNfcATech will extend MozBasicTech

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #13)
> > > Other tag type class (MozNfcATech for example) will be derived from the
> > > based class. It looks better in term of pattern design. Do you agree with
> > > that?
> > 
> > No, this is wrong.
> > NfcA is a radio *technology*, whereas a MozNFCTag abstracts a NFC Tag.
> > 
> > A tag is not neccessary to have a NFC_A technology, how do you inherit it?
> 
> Thank you.
> I mean create a NEW internal based class (for example MozBasicTech) to use
> later.
> MozNfcATech will extend MozBasicTech

No, I didn't choose this design because not every technology could do 'transceive'.

Comment 15

[Thomas Nguyen (:tnguyen)]

Attachment: WebAPI for supporting NFC_A tags patch v3 Rebased

Changes compared to patch v2
- Rebased and fixed conflicts
- Fix mode line due to bug 1152551

Thanks

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8683466 [details] [diff] [review]
WebAPI for supporting NFC_A tags patch v3 Rebased

Review of attachment 8683466 [details] [diff] [review]:
-----------------------------------------------------------------

Please use 8 lines of context.

Mostly okay,
but it would be great if we can test this first,
for example, 
try to add somes tests for NFC_A in https://github.com/mozilla-b2g/gaia/blob/master/dev_apps/nfc-api-test/js/nfc_tech.js

::: dom/nfc/MozNfcATech.cpp
@@ +56,5 @@
> +    return nullptr;
> +  }
> +
> +  RefPtr<MozNfcATech> nfcA = new MozNfcATech(win, aNFCTag);
> +

remove extra line

::: dom/nfc/TagUtils.cpp
@@ +19,5 @@
> +  Nullable<nsTArray<NFCTechType>> techList;
> +  aTag.GetTechList(techList, rv);
> +  if (rv.Failed()) {
> +    return false;
> +  }

NS_ENSURE_SUCCESS(rv, false);

@@ +25,5 @@
> +  if (techList.IsNull() || !(techList.Value().Contains(aTechnology))) {
> +    return false;
> +  }
> +
> +  return true;

return !A && B;

::: dom/nfc/TagUtils.h
@@ +10,5 @@
> +#include "mozilla/dom/MozNFCTagBinding.h"
> +
> +namespace mozilla {
> +namespace dom {
> +

should have another namespace nfc

@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +/*

Use /**
same for others.

@@ +12,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +/*
> + * This a helper function for various nfc tag.

Please rephrase here.

@@ +22,5 @@
> +  /*
> +   * Check if specified technogy is supported in this tag.
> +   */
> +  static bool
> +  CheckTechnologySupport(const MozNFCTag& aNFCTag,

IsTechSupported

@@ +31,5 @@
> +   */
> +  static already_AddRefed<Promise>
> +  Transceive(MozNFCTag* aTag,
> +             const Uint8Array& aCommand,
> +             const NFCTechType& aTechnology,

aTechnology should be the 2nd argument.

Comment 17

[Thomas Nguyen (:tnguyen)]

Attachment: WebAPI for supporting NFC_A tags patch v4

Comment 18

[Thomas Nguyen (:tnguyen)]

Attachment: Add NFCA transceive test functions to gaia nfc-api-test app

Add some tests for NFCA transceive API in gaia/dev-app/nfc-nci-test including:
- Write data to 1 block of NFCA tag type 2
- Read data from 4 blocks of NFCA tag type 2
- Get UID of NFCA tag type 1

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Why do we need both MozNfcATech and MozIsoDepTech webidl interfaces? The APIs look exactly the same.
Why not just rename MozIsoDepTech to MozTagTech and remove the typedef?

And if the implementation needs tech type, add that as param to the ChromeConstructor?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8684833 [details] [diff] [review]
WebAPI for supporting NFC_A tags patch v4

So, don't understand the reason for functionally duplicate interfaces.
Either merge them and upload a new patch, or explain why two different webidl interfaces are needed and ask review again.

Comment 21

[Thomas Nguyen (:tnguyen)]

Thank you for your review.
There's diversity of nfc tech (ISO-DEP, NFC-A in this bug, Mifare-Ultralight in bug 884478, etc ..), and most of them have capability of "transceive command". Reference NFC digital Protocol Technical Specification http://www.cardsys.dk/download/NFC_Docs/NFC%20Digital%20Protocol%20Technical%20Specification.pdf
Apparently, the interfaces look exactly the same in term of coding design, however the transceive command protocols are totally different. 
For example, ISO-DEP interface transceive supports ADPU command sending to NFC tag, but NFC-A interface uses low level command to send to the tag.
It would be better if there're 2 separated interfaces
Thanks

Comment 22

[Thomas Nguyen (:tnguyen)]

Attachment: WebAPI for supporting NFC_A tags patch v5

Changes:
- Update comment in WebIDL transceive operations more clearly

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #19)
> Why do we need both MozNfcATech and MozIsoDepTech webidl interfaces? The
> APIs look exactly the same.
> Why not just rename MozIsoDepTech to MozTagTech and remove the typedef?
> 
> And if the implementation needs tech type, add that as param to the
> ChromeConstructor?

So far we only implemented transceive for these two technologies, but they still have other interfaces we didn't implement yet.

See IsoDep [1] and NfcA [2] on Android

[1] : http://developer.android.com/reference/android/nfc/tech/IsoDep.html
[2] : http://developer.android.com/reference/android/nfc/tech/NfcA.html

Also we didn't choose to add a base class like TagTechology [3] to be inherited by NFC techonogies on Android, because some techonogy like NFCBarcode doesn't have trasnceive.

So adding a base class like TagTechonology which has only 1~2 interfaces (trasnceive and getTransceiveTimeout), but could not be inherited by all NFC techonology classes, doesn't look like a cleaner solution to us with regarding to implementation. 

[3]: http://developer.android.com/reference/android/nfc/tech/TagTechnology.html

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8685227 [details] [diff] [review]
WebAPI for supporting NFC_A tags patch v5

>+
>+  static const NFCTechType mTechnology;

Nit, static member variables should have s-prefix, not m-prefix.
So, sTechnology




Ok, thanks for the explanation.

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8685227 [details] [diff] [review]
WebAPI for supporting NFC_A tags patch v5

Review of attachment 8685227 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/MozIsoDepTech.cpp
@@ +50,5 @@
>      aRv.Throw(NS_ERROR_FAILURE);
>      return nullptr;
>    }
>  
> +  using namespace mozilla::dom::nfc;

put 
using namespace mozilla::dom::nfc::TagUtils; 
at top,
same for MozNfcATech.cpp

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8684836 [details] [diff] [review]
Add NFCA transceive test functions to gaia nfc-api-test app

use PR

Comment 27

[GitHub Autolander Bot]

Attachment: [gaia] tungmangtdh3:bug976457 > mozilla-b2g:master

Comment 28

[Thomas Nguyen (:tnguyen)]

Attachment: Add NFCA transceive test functions to gaia nfc-api-test app V1

Comment 29

[GitHub Autolander Bot]

Attachment: (gaia)Add NFCA tech tests api PR v2

Comment 30

[Thomas Nguyen (:tnguyen)]

Attachment: 0001-Bug-976457-B2G-NFC-API-for-supporting-NFC_A-tags.-r-.patch

- Rename mTechnology
- Move using namespace to the top

Comment 31

[Thomas Nguyen (:tnguyen)]

Hi Tim,
Sorry to flag you as I have no idea who is available to review this gaia PR (nfc-test-api) at the moment. Could you please tell me I could tag whom to this bug?
Thanks for your help

Comment 32

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8687112 [details] [review]
(gaia)Add NFCA tech tests api PR v2

you can land anything for the eng build and for manual testing.

Comment 33

[Thomas Nguyen (:tnguyen)]

Try results:
- Gecko
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d74c44ca331e
- Gaia
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=5d5cd515339fa844af410938295a14442168a91b

Comment 34

[Carsten Book [:Tomcat]]

gaia: https://github.com/mozilla-b2g/gaia/commit/089eb0fe17c9809a5465f92ec82b63a990413698

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/44aec780d0d5

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/44aec780d0d5