Closed Bug 976457 Opened 6 years ago Closed 4 years ago

B2G NFC: API for supporting NFC_A technology

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog, firefox45 fixed)

RESOLVED FIXED
2.6 S1 - 11/20
tracking-b2g backlog
Tracking Status
firefox45 --- fixed

People

(Reporter: allstars.chh, Assigned: tnguyen)

References

Details

Attachments

(2 files, 8 obsolete files)

46 bytes, text/x-github-pull-request
timdream
: review+
Details | Review
15.43 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
This bug is for supporting NfcA (ISO 14443-3A) technology tags.
Blocks: b2g-NFC-2.0
No longer blocks: b2g-NFC-2.0
Is it required by bug 916428? 
Not sure if KDDI would like to take this.
Flags: needinfo?(xju-hashimoto)
See Also: → 916428
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.
Flags: needinfo?(xju-hashimoto)
blocking-b2g: --- → backlog
No longer blocks: 963541
Depends on: 963541
Assignee: nobody → dlee
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.
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
This patch basically only support transceive function.
Attachment #8551663 - Flags: review?(allstars.chh)
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.
Attachment #8551663 - Flags: review?(allstars.chh)
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.
Depends on: 1125041
Attachment #8551663 - Attachment is obsolete: true
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
Attachment #8553591 - Flags: review?(allstars.chh)
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.
/**
 * ...
 */
Attachment #8553591 - Flags: review?(allstars.chh) → feedback+
blocking-b2g: backlog → ---
Blocks: 884478
Assignee: dlee → tnguyen
Status: NEW → ASSIGNED
(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?
Flags: needinfo?(allstars.chh)
(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?
Flags: needinfo?(allstars.chh)
(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
(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'.
Changes compared to patch v2
- Rebased and fixed conflicts
- Fix mode line due to bug 1152551

Thanks
Attachment #8553591 - Attachment is obsolete: true
Attachment #8683466 - Flags: review?(allstars.chh)
Summary: B2G NFC: API for supporting NFC_A tags → B2G NFC: API for supporting NFC_A technology
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.
Attachment #8683466 - Flags: review?(allstars.chh) → feedback+
Attachment #8683466 - Attachment is obsolete: true
Attachment #8684833 - Flags: review?(allstars.chh)
Attachment #8684833 - Attachment description: 0001-Bug-976457-B2G-NFC-API-for-supporting-NFC_A-tags.-r-.patch → WebAPI for supporting NFC_A tags patch v4
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
Attachment #8684836 - Flags: feedback?(allstars.chh)
Attachment #8684833 - Flags: review?(bugs)
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 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.
Attachment #8684833 - Flags: review?(bugs) → review-
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
Changes:
- Update comment in WebIDL transceive operations more clearly
Attachment #8684833 - Attachment is obsolete: true
Attachment #8684833 - Flags: review?(allstars.chh)
Attachment #8685227 - Flags: review?(bugs)
Attachment #8685227 - Flags: review?(allstars.chh)
(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 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.
Attachment #8685227 - Flags: review?(bugs) → review+
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
Attachment #8685227 - Flags: review?(allstars.chh) → review+
Comment on attachment 8684836 [details] [diff] [review]
Add NFCA transceive test functions to gaia nfc-api-test app

use PR
Attachment #8684836 - Flags: feedback?(allstars.chh)
Attachment #8684836 - Attachment is obsolete: true
Attachment #8687078 - Flags: feedback?(allstars.chh)
Attachment #8687075 - Attachment is obsolete: true
Attachment #8687078 - Attachment is obsolete: true
Attachment #8687078 - Flags: feedback?(allstars.chh)
Attachment #8687112 - Attachment description: [gaia] tungmangtdh3:bug976457 > mozilla-b2g:master → Add NFCA tech tests api PR v2
- Rename mTechnology
- Move using namespace to the top
Attachment #8685227 - Attachment is obsolete: true
Attachment #8687115 - Flags: review+
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
Flags: needinfo?(timdream)
Attachment #8687112 - Attachment description: Add NFCA tech tests api PR v2 → (gaia)Add NFCA tech tests api PR v2
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.
Flags: needinfo?(timdream)
Attachment #8687112 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/44aec780d0d5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.6 S1 - 11/20
You need to log in before you can comment on or make changes to this bug.