916428 - [NFC] APIs for ISO 14443-4 tags (IsoDep Support)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Junichi Hashimoto]

Attachment: API Proposal for accessing various tags

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.65 Safari/537.36

Steps to reproduce:

Touching to a ISO14443-4 tag causes nothing.


Actual results:

Touching to a ISO14443-4 tag causes nothing.


Expected results:

FirefoxOS should support various NFC tags including IS014443-4 and provide a developer friendly interface for this purpose.

Comment 1

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

Hi Hashimoto-san
We use Bug 860906 to track all B2G-NFC bugs, so I will add the dependency again here

Comment 2

[Junichi Hashimoto]

Attachment: adding transceive() to tagobject

Comment 3

[Junichi Hashimoto]

Attachment: passing APDU commands to NFC library

Comment 4

[Junichi Hashimoto]

Attachment: nfctag-idl.patch

A diff of MozNFCTag.webidl from gecko.patch
It simply add transceive() method to MozNFCTag.

Sid, could you give me any feedback?

Comment 5

[psiddh]

Attachment: IsoDEP.patch

In order to add IsoDep (or any other future technology classes)to the master and ensuring that current NDEF functionality also stays in tact, I have attached a rough patch to convey my thoughts (This patch is not compiled / tested against). I will do it shortly and submit another cleaner patch. Following things were done
1) First rename current MozNFCTag to 'MozNDEFTag' (Since the current MozNFCTag is pretty much NDEF functionality only) and commented out 'connect' & 'close'.
2) Then I copied your base interface 'nsIDOMNfcTag' from NfcTag.idl and named it MozNFCTag (with no interface . yes this can be a base class)
3) I derived 'MozNDEFTag' from 'MozNFCTag'.
4) Extended getNFCTag function in nsNfc.js to accept an optional parameter 'techType'. (Default is 'NDEF'.)
i;e; getNFCTag(sessionToken) --> will return NDEF object,
but say getNFCTag(sessionToken, 'ISO_DEP') should return IsoDep object 
This way we can extend into supporting new technologies and at the same time we can maintain the existing nomenclature.
What do you think? Also I will test out the patches (fix bugs if there are any) and submit a cleaner patch for yours & Mozilla's feedback as soon as I can.,

Comment 6

[Junichi Hashimoto]

Attachment: NFCTag class diagram

The attach is a diagram based on your suggestion (with moving transceive to IsoDepTag)
About getDetailsNDEF():
	(1) Can we use NFCTag.getDetails(tech) instead?
	(2) Does NFCTag.getDetails(tech) meet a use case where developer wants to access details *before* deciding which technology to connect?
About connect(), getNFCTag():
	(3) What if they are called more than twice? Should all previous tag-connections are canceled? (rel. bug963541)

Takagi-san, could you show your view especially on (2).

Comment 7

[Yoshihiko TAKAGI]

About (1), 
    We can use NFCTag.getDetails() instead of getDetailsNDEF(), but a parameter 'tech' isn't necessary since you have already specified a tech-type by 'getNFCTag(sessionId, tech).

About (2),
    A developer can access details *after" getNFCTag() and *before* connect(). I think it is reasonable because a developer should determine a tech-type before accessing details. 

    Before that, developer wants to filter types, and know which types are supported by a discovered tag.

    Here is a rough idea of manifest.webapp for filtering nfc-tech-type.
      "activities": {
          "nfc-technology": {
              "filters": {
                  "type": [ "NDEF", "IsoDep", ... ]
              },
          "disposition": "window"
          }
      }

    In addition to that, an app needs to receive a nfc-tech-list by an activity handler.

About (3),
    When nfc-tech-discovered is occured, Gaia has already know all tech-types supported by a discovered tag (otherwize filtering is impossible).
    If Gaia can keeps tech-list and details of a tag, getNFCTag() (and getDetails()) for different types can be called in parallel. But 'connect()' shall be called exclusively.

Comment 8

[psiddh]

Attachment: IsoDep_v01.patch

Attaching the patch that refactors existing MozNFCTag interface to support addition of new technology types such as 'ISO_DEP' or 'NFC_A' etc

Comment 9

[psiddh]

I concur with Yoshihiko TAKAGI-san mostly.

Yes after the call, 
ndef = getNFCTag(session, 'NDEF');
.
.
ndef.getDetails();   // No need of 'getDetailsNdef()'
.
ndef.connect();      // connect doesn't require 'tech' field


nfc-tech-list is already received at System app level.

Yes agreed that we should now have a way to allow Gaia apps to filter on tech types, now that we are supporting multiple techs. Maybe we should stage this changes in two phases, one for basic refactoring of MozNFCTag ( on the lines of attached patch) and other patch to support providing filters.

Yoshi, what is your view on the reafactoring part ?

Comment 10

[psiddh]

Requesting Yoshi's inputs on the refactoring of webIDLs to support new tech types.

Comment 11

[psiddh]

Attachment: IsoDep_v01.patch

Comment 12

[Junichi Hashimoto]

Attachment: NFCTag class diagram r1

Summary
- Common function
-- getNFCTag() modification (let's use this bug)
-- tech filtering (no bug?)
- various tags
-- MiFare https://bugzilla.mozilla.org/show_bug.cgi?id=884478
-- NFC_A https://bugzilla.mozilla.org/show_bug.cgi?id=976457
-- IsoDEP(this) https://bugzilla.mozilla.org/show_bug.cgi?id=916428

Comment 13

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

I'll intend to make things simpler, and I suggest we can split into some new bugs.

1. Keep MozNFCTag, please file a new bug for discussing whether we should have NDEFTag or not.
   Also given that NDEF is the format should be supported bt NFC-Forum. So I think it's okay for now we handle NDEF in MozNFCTag.

2. File a new bug for discussing rename getDetailsNDEF to getDetails. Given that right now it returns some attribute from NDEF, and not from other technology.

At this bug, we should discuss when should we get the IsoDep object first?
in mozNfc.getNFCTag()? or in MozNFCTag.connect() ?

If we plan to put 'connect' in MozNFCTag (the base class), I think we need to have the techType or tech as the parameter, i.e. connect("IsoDep");

In Bug 963541 it's already being discussed for the 'connect',
Only IsoDep tags need to switch RF interface.
Should we use FRAME as default so 'connect' can be moved to IsoDep class?

Also if we plan to get the IsoDep object in getNFCTag, are connect/close still neccesary?

Comment 14

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

I think Junichi-san is taking this bug, assign assignee to him.

Comment 15

[Junichi Hashimoto]

A metabug such as 'Support various NFC tag technologies" would be required for 1 and 2. Does it exists?
---

The idea of connect()/close() interfaces come from android[1].
If we plan to provide unique API for firefox OS, connect()/close() can be omitted.
Then pseudocode will be:

ndef = nfc.getNFCTag(session);
isodep.getDetails();
ndef.readNdef();
isodep = nfc.getNFCTag(session, "IsoDep"); // reconnect
isodep.getDetails();
isodep.transcieve(apdu);

[1] http://developer.android.com/reference/android/nfc/tech/TagTechnology.html
---
 
> In Bug 963541 it's already being discussed for the 'connect',
> Only IsoDep tags need to switch RF interface.
> Should we use FRAME as default so 'connect' can be moved to IsoDep class?

What do you mean by RF and FRAME?

Comment 16

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

(In reply to Junichi Hashimoto from comment #15)
> A metabug such as 'Support various NFC tag technologies" would be required
> for 1 and 2. Does it exists?
> ---
> 
No, but if we only see 2 bugs for now, it's okay to not to have a meta bug for it.
But it's up to you.
 
> What do you mean by RF and FRAME?
NCI spec, table 99 (p 140).

IsoDep tags will need to switch RF interface, that's what connet() is doing.

Comment 17

[Junichi Hashimoto]

I think I understand the needs of connect(). In order to make changes small it is better to keep connect() as is; with no parameters.

ndef = nfc.getNfcTag(session);           // returns MozNFCTag object (will be renamed as MozNDEFTag)
ndef.connect(/* nothing */);

iso = nfc.getNfcTag(session, "IsoDep");  // returns MozIsoDep object
iso.connect(/* nothing */);

Comment 20

[Dimi Lee [:dimi]]

Hi Junichi-san
Since MozNFCTag.webidl has been changed, i will update new proposal.

Set assignee to me.

Comment 21

[Dimi Lee [:dimi]]

Attachment: WebIDL for ISO 14443_4 tags patch v1

This patch only supports transceive function, and this patch depends on bug 1109456

Comment 22

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

Comment on attachment 8541631 [details] [diff] [review]
WebIDL for ISO 14443_4 tags patch v1

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

::: dom/webidl/MozNFCTag.webidl
@@ +33,5 @@
>  };
>  
> +[Func="Navigator::HasNFCSupport", AvailableIn="PrivilegedApps",
> + Constructor(MozNFCTag tag)]
> +interface MozNFCIsoDep {

Rename to MozIsoDepTech

@@ +37,5 @@
> +interface MozNFCIsoDep {
> +  /**
> +   * Send raw command to tag and receive the response.
> +   */
> +  Promise<sequence<byte>> transceive(sequence<byte> command);

Use Uint8Array.

Comment 23

[Dimi Lee [:dimi]]

Attachment: WIP Patch

Comment 24

[Dimi Lee [:dimi]]

Attachment: Transceive API for ISO DEP patch v1

Comment 25

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

Comment on attachment 8546450 [details] [diff] [review]
Transceive API for ISO DEP patch v1

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

r+ on JS impl.
WebIDL/C++ code needs to get review from smaug.

::: dom/nfc/MozIsoDepTech.cpp
@@ +1,1 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */

tag-width

@@ +37,5 @@
> +/* static */
> +already_AddRefed<MozIsoDepTech>
> +MozIsoDepTech::Constructor(const GlobalObject& aGlobal,
> +                          MozNFCTag& aNFCTag,
> +                          ErrorResult& aRv)

align

@@ +58,5 @@
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<MozIsoDepTech> isodep = new MozIsoDepTech(win, aNFCTag);

nit, isoDep

@@ +92,5 @@
> +  return promise.forget();
> +}
> +
> +JSObject*
> +MozIsoDepTech::WrapObject(JSContext* aCx)

inline

::: dom/nfc/MozIsoDepTech.h
@@ +1,1 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */

tag-width should be 80.

@@ +19,5 @@
> +
> +class Promise;
> +
> +class MozIsoDepTech : public nsISupports,
> +                     public nsWrapperCache

nit, align.

@@ +32,5 @@
> +
> +  virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;
> +
> +  static
> +  already_AddRefed<MozIsoDepTech> Constructor(const GlobalObject& aGlobal,

usually 
static already_AddRefed<MozIsoDepTech>
Constructor(..)

@@ +34,5 @@
> +
> +  static
> +  already_AddRefed<MozIsoDepTech> Constructor(const GlobalObject& aGlobal,
> +                                             MozNFCTag& aNFCTag,
> +                                             ErrorResult& aRv);

align

@@ +41,5 @@
> +  MozIsoDepTech(nsPIDOMWindow* aWindow, MozNFCTag& aNFCTag);
> +  ~MozIsoDepTech();
> +
> +  nsRefPtr<nsPIDOMWindow> mWindow;
> +  nsRefPtr<MozNFCTag> mNFCTag;

should be nsCOMPtr?
call it 'mTag' should be enough

::: dom/webidl/MozIsoDepTech.webidl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * Part of this idl is from:
> + * http://w3c.github.io/nfc/proposals/common/nfc.html#nfctag-interface

remove

@@ +4,5 @@
> + *
> + * Part of this idl is from:
> + * http://w3c.github.io/nfc/proposals/common/nfc.html#nfctag-interface
> + *
> + * Copyright © 2013 Deutsche Telekom, Inc.

You changed career?

@@ +12,5 @@
> + ChromeConstructor(MozNFCTag tag)]
> +interface MozIsoDepTech {
> +  /**
> +   * Send raw command to tag and receive the response.
> +   */

Throws

perhaps MozNFCTag.transceive doesn't have to do [Throws].

Comment 26

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

Comment on attachment 8546450 [details] [diff] [review]
Transceive API for ISO DEP patch v1

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

::: dom/nfc/MozIsoDepTech.cpp
@@ +92,5 @@
> +  return promise.forget();
> +}
> +
> +JSObject*
> +MozIsoDepTech::WrapObject(JSContext* aCx)

ingore this. I found many code also put it in cpp.

Comment 27

[Dimi Lee [:dimi]]

Comment on attachment 8546450 [details] [diff] [review]
Transceive API for ISO DEP patch v1

r? to smaug because of add MozIsoDepTech.webIdl and modify MozNFCTag.webidl

I will address yoshi's comment in next patch.

Comment 28

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

At some point we need to get rid of Moz prefixes. Do we know if W3C spec will be close to our implementation any time soon?

Comment 29

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

Comment on attachment 8546450 [details] [diff] [review]
Transceive API for ISO DEP patch v1

>+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(MozIsoDepTech)
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
>+NS_IMPL_CYCLE_COLLECTION_TRACE_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(MozIsoDepTech)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mNFCTag)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
Missing NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

>+MozIsoDepTech::Constructor(const GlobalObject& aGlobal,
>+                          MozNFCTag& aNFCTag,
>+                          ErrorResult& aRv)
align

>+{
>+  ErrorResult rv;
>+  nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aGlobal.GetAsSupports());
>+  if (!win) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
>+
>+  Nullable<nsTArray<NFCTechType>> techList;
>+  aNFCTag.GetTechList(techList, rv);
>+  if (rv.Failed()) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
>+
>+  if (techList.IsNull() || !(techList.Value().Contains(mTechnology))) {
>+    aRv.Throw(NS_ERROR_FAILURE);
NS_ERROR_FAILURE is Gecko specific error, and looks like a web page could trigger this code path easily.
Perhaps NS_ERROR_DOM_NOT_SUPPORTED_ERR or some such would be better


>+  nsRefPtr<MozIsoDepTech> isodep = new MozIsoDepTech(win, aNFCTag);
>+  if (!isodep) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
Useless null check. 'new' is infallible by default

>+class MozIsoDepTech : public nsISupports,
>+                     public nsWrapperCache
align

>+ * Part of this idl is from:
>+ * http://w3c.github.io/nfc/proposals/common/nfc.html#nfctag-interface
>+ *
>+ * Copyright © 2013 Deutsche Telekom, Inc.
Is this Copyright correct?

>+typedef MozIsoDepTech MozTagTech;
Why do we need this typedef? If not needed, remove.

Comment 30

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

(In reply to Olli Pettay [:smaug] from comment #28)
> At some point we need to get rid of Moz prefixes. Do we know if W3C spec
> will be close to our implementation any time soon?

Hi, Smaug
Actually W3C has changed their API dramatically recently (Maybe because of different editor) 
http://w3c.github.io/nfc/

Can you suggest a timing to remove Moz prefix?
Given that we're going to make our NFC API as privileged.

Thanks

Comment 31

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

(In reply to Olli Pettay [:smaug] from comment #29)
> >+typedef MozIsoDepTech MozTagTech;
> Why do we need this typedef? If not needed, remove.
Hi smaug
Sorry for not explaining this.

Next we're going to implement some other technologies, like MifareClassicTech.
So the method 'selectTech(NFCTechType tech)' will create the according technology and return it.

For example:

var isoDep = tag.selectTech("ISO-DEP"); // isoDep should be instance of MozIsoDepTech

var mifareClassic = tag.selectTech("MIFARE-Classic"); // mifareClassic will be instance of MozMifareClassicTech.

So next time the typedef will become

typedef (MozIsoDepTech or MozMifareClassicTech) TagTechnology;

interface MozNFCTag {
...
TagTechnology selectTech(NFCTechType tech);
};

Can we do like this?
Or we should create an empty base interface TagTechnology, and it will be inherited by those technologies?

Comment 32

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

(In reply to Yoshi Huang[:allstars.chh] from comment #30) 
> Hi, Smaug
> Actually W3C has changed their API dramatically recently (Maybe because of
> different editor) 
> http://w3c.github.io/nfc/
> 
> Can you suggest a timing to remove Moz prefix?
> Given that we're going to make our NFC API as privileged.
Oh, maybe then never, but it also probably means that we aren't going to expose the current NFC API to the web pages ever,
but have another API which also other browsers may support.
Are we actively participating the W3C work here? Giving comments and so.

Comment 33

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

(In reply to Yoshi Huang[:allstars.chh] from comment #31) 
> Can we do like this?
Sounds good. thanks for the explanation.

Comment 34

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

(In reply to Olli Pettay [:smaug] from comment #32)
> (In reply to Yoshi Huang[:allstars.chh] from comment #30) 
> > Hi, Smaug
> > Actually W3C has changed their API dramatically recently (Maybe because of
> > different editor) 
> > http://w3c.github.io/nfc/
> > 
> > Can you suggest a timing to remove Moz prefix?
> > Given that we're going to make our NFC API as privileged.
> Oh, maybe then never, but it also probably means that we aren't going to
> expose the current NFC API to the web pages ever,
> but have another API which also other browsers may support.
> Are we actively participating the W3C work here? Giving comments and so.

No, we don't have any interactions with W3C NFC.

Comment 35

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #25)
> Comment on attachment 8546450 [details] [diff] [review]
> Transceive API for ISO DEP patch v1
> 
> Review of attachment 8546450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +41,5 @@
> > +  MozIsoDepTech(nsPIDOMWindow* aWindow, MozNFCTag& aNFCTag);
> > +  ~MozIsoDepTech();
> > +
> > +  nsRefPtr<nsPIDOMWindow> mWindow;
> > +  nsRefPtr<MozNFCTag> mNFCTag;
> 
> should be nsCOMPtr?
> call it 'mTag' should be enough
should use nsRefPtr because MozNFCTag is a concrete class.

Comment 36

[Dimi Lee [:dimi]]

Attachment: Transceive API for ISO DEP patch v2

- Fix Nits
- Rebase to latest code

Comment 37

[Dimi Lee [:dimi]]

try result : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d46422b20f3e

Comment 38

[Dimi Lee [:dimi]]

Clearcheckin-need before i would like do another test before checkin

Comment 39

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

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

Comment 40

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

backout ...
https://hg.mozilla.org/integration/b2g-inbound/rev/0797aa48a780

Comment 41

[Dimi Lee [:dimi]]

Hi Yoshi,
Sorry, the test is passed

Comment 42

[Dimi Lee [:dimi]]

After checking with yoshi, i will update comment and update patch again.

Comment 43

[Dimi Lee [:dimi]]

Attachment: Transceive API for ISO DEP patch v3

Modify comment

Comment 44

[Dimi Lee [:dimi]]

Comment on attachment 8547995 [details] [diff] [review]
Transceive API for ISO DEP patch v3

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7d7a31553aa1

Comment 46

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

https://hg.mozilla.org/mozilla-central/rev/7d7a31553aa1

Comment 47

[Dimi Lee [:dimi]]

Comment on attachment 8547995 [details] [diff] [review]
Transceive API for ISO DEP patch v3

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
NFC feature

User impact if declined: 
Third party Apps can not use NFC transceive function to get information
from tag

Testing completed: 
Manually.

Risk to taking this patch (and alternatives if risky): 
No.

String or UUID changes made by this patch:
No.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8a003bc7632e