Closed Bug 993836 Opened 6 years ago Closed 6 years ago

[NFC] Emulator support for reading NDEF data from type 2 tag

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
tracking-b2g backlog
Tracking Status
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: dimi, Assigned: dimi)

References

Details

(Whiteboard: [p=5])

Attachments

(2 files, 11 obsolete files)

61 bytes, text/x-github-pull-request
dimi
: review+
Details | Review
3.61 KB, patch
dimi
: review+
Details | Diff | Splinter Review
No description provided.
Whiteboard: [p=3]
Target Milestone: --- → 1.4 S6 (25apr)
Whiteboard: [p=3] → [p=5]
blocking-b2g: --- → backlog
Summary: [NFC] Emulator support for reading NDEF data from type 1 tag → [NFC] Emulator support for reading NDEF data from type 2 tag
Blocks: 997576
Depends on: 998274
Blocks: 1001315
Attached patch Patch support read t2t NDEF v1 (obsolete) — Splinter Review
Testcase will add in bug 1001315
Attachment #8412411 - Flags: review?(tzimmermann)
Hi Thomas,
In this bug, i use hard code value for NDEF message because i think we will not change it very often and we do not require create NDEF message dynamically. But it is also ok for me if you think it is more flexible to generate it dynamically.

Use following command should pop up browser in emulator
"nfc ntf rf_intf_activated 2"
Attachment #8412411 - Attachment is obsolete: true
Attachment #8412411 - Flags: review?(tzimmermann)
Attached patch Patch support read t2t NDEF v2 (obsolete) — Splinter Review
Attachment #8412417 - Flags: review?(tzimmermann)
Hi Dimi

(In reply to Dimi Lee[:dimi][:dlee] from comment #2)
> Hi Thomas,
> In this bug, i use hard code value for NDEF message because i think we will
> not change it very often and we do not require create NDEF message
> dynamically. But it is also ok for me if you think it is more flexible to
> generate it dynamically.

Have a look at bug 1000935 for dynamic NDEF messages. The patches contains support for reading and writing via console. NDEF messages are given to the emulator in a simple textual format. And the emulator can output received NDEF messages in JSON format for parsing by a JS test case.

> Use following command should pop up browser in emulator
> "nfc ntf rf_intf_activated 2"

I strongly oppose mixing up functionality. This command is for sending a specific NCI notification. There should be a different command for transferring data.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
> Hi Dimi
> 
> (In reply to Dimi Lee[:dimi][:dlee] from comment #2)
> > Hi Thomas,
> > In this bug, i use hard code value for NDEF message because i think we will
> > not change it very often and we do not require create NDEF message
> > dynamically. But it is also ok for me if you think it is more flexible to
> > generate it dynamically.
> 
> Have a look at bug 1000935 for dynamic NDEF messages. The patches contains
> support for reading and writing via console. NDEF messages are given to the
> emulator in a simple textual format. And the emulator can output received
> NDEF messages in JSON format for parsing by a JS test case.
> 
I will rebase after bug 1000935 landed and use dynamic NDEF.

> > Use following command should pop up browser in emulator
> > "nfc ntf rf_intf_activated 2"
> 
> I strongly oppose mixing up functionality. This command is for sending a
> specific NCI notification. There should be a different command for
> transferring data.

This command is not for sending data, it just notify t1t tag discover. The reason browser
will pop up is because after detecting tag, nfcd will fetch ndef data from tag immediately 
and notify GAIA.

As discussed on IRC, I will cancel the review for now and re-base the code in bug 1000935.
Attachment #8412417 - Flags: review?(tzimmermann)
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Attachment #8412417 - Attachment is obsolete: true
Attachment #8425330 - Flags: review?(tzimmermann)
Attachment #8425331 - Flags: review?(tzimmermann)
Attachment #8425333 - Flags: review?(tzimmermann)
Comment on attachment 8425330 [details] [diff] [review]
Part1. Add RF technology specific parameters v1

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

This looks quite reasonable to me, except for the nits I noticed.

Is there a test case? I won't r+ these patches until we have a test for them. So only f+ for now.

::: hw/nfc-re.c
@@ +636,5 @@
> +        *p = 0;
> +    } else if (cid1_len == 7) {
> +        *p = 1 << 6;
> +    } else if (cid1_len == 10) {
> +        *p = 2 << 6;

Instead of shifting, you could also define some constants and set the bitmask directly here. To me, this seems easier to understand.

@@ +643,5 @@
> +    }
> +
> +    if (protocol == NCI_RF_PROTOCOL_T1T) {
> +        p++;               /* Byte1 of SENS_RES */
> +        *p++ = 3 << 2;     /* Byte2 of SENS_RES */

I'd prefer to have constant bitmasks everywhere instead of shifting numbers around. I know that I'm sloppily doing this as well, but don't copy my bad habits. ;)

@@ +646,5 @@
> +        p++;               /* Byte1 of SENS_RES */
> +        *p++ = 3 << 2;     /* Byte2 of SENS_RES */
> +        *p++ = 0;          /* NFCID1 Length */
> +        *p++ = 0;
> +	} else {

Alignment.
Attachment #8425330 - Flags: review?(tzimmermann) → feedback+
Comment on attachment 8425331 [details] [diff] [review]
Part2. Add type 2 tag as a remote end point v1

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

r- because the tag-handling needs to be reworked.

::: hw/goldfish_nfc.c
@@ +293,4 @@
>      goldfish_device_add(&s->dev, goldfish_nfc_readfn,
>                          goldfish_nfc_writefn, s);
>  
> +    nfc_re_init();

Should be gone after the changes to nfc-tag.{c,h}

::: hw/nfc-re.c
@@ +24,5 @@
>  
>  struct nfc_re nfc_res[3] = {
> +    INIT_NFC_RE([0], NCI_RF_PROTOCOL_NFC_DEP, NCI_RF_NFC_F_PASSIVE_LISTEN_MODE, NULL, "deadbeaf0", nfc_res+0),
> +    INIT_NFC_RE([1], NCI_RF_PROTOCOL_NFC_DEP, NCI_RF_NFC_F_PASSIVE_LISTEN_MODE, NULL, "deadbeaf1", nfc_res+1),
> +    INIT_NFC_RE([2], NCI_RF_PROTOCOL_T2T, NCI_RF_NFC_A_PASSIVE_LISTEN_MODE, nfc_tags, "deadbeaf2", nfc_res+2)

Are you referring to a single element or the complete array? My feeling says that the RE is the tag, so it's just one element, but the code says otherwise.

::: hw/nfc-tag.c
@@ +30,5 @@
> +                                    0x0c, 0x04, 0x55, 0x6e,
> +                                    0x75, 0x6c, 0x6c, 0x01,
> +                                    0x6d, 0x6f, 0x7a, 0x69,
> +                                    0x6c, 0x6c, 0x61, 0x2e,
> +                                    0x6f, 0x72, 0x67, 0xfe };

I have no idea what these numbers mean and it's almost impossible to change them. So no NDEF messages here please.  NDEF messages should be constructed in the test cases and passed to the emulator. There is helper code for the tests and the console now contains an NDEF parser that should be good enough.

@@ +37,5 @@
> +
> +void
> +nfc_tag_init()
> +{
> +    INIT_NFC_T2T(nfc_tags[0], t2t_internal, t2t_lock, t2t_cc, tlv_url_ndef);

As mentioned in nfc-tag.h, just pass the values so that you don't need to memcpy. Than you should remove nfc_tag_init and make it a global static initializer.

::: hw/nfc-tag.h
@@ +45,5 @@
> +#define INIT_NFC_T2T(tag_, internal_, lock_, cc_, data_) \
> +    memcpy(tag_.t2t.format.internal, internal_, ARRAY_SIZE(internal_)); \
> +    memcpy(tag_.t2t.format.lock, lock_, ARRAY_SIZE(lock_)); \
> +    memcpy(tag_.t2t.format.cc, cc_, ARRAY_SIZE(cc_)); \
> +    memcpy(tag_.t2t.format.data, data_, ARRAY_SIZE(data_));

This is a static initializer, no memcpys here. And since everything you pass in is static itself, you don't have to memcpy. Simply do an assignment from the values.
Attachment #8425331 - Flags: review?(tzimmermann) → review-
Attachment #8425333 - Flags: review?(tzimmermann) → feedback+
Hi Dimi,

Thanks for the patch set. I think what's still missing is a console interface for setting the tag's NDEF message and a test case.
Depends on: 1013746
create bug 1013746 to support console interface for setting tag's NDEF message
Duplicate of this bug: 1013746
(In reply to Dimi Lee[:dimi][:dlee] from comment #12)
> create bug 1013746 to support console interface for setting tag's NDEF
> message
Ah, sorry, it would be better and easier to provide patch in this bug.
Set bug 1013746 to duplicate
Hi,

I'm not 100% sure about the requirements of the console interface, but I guess the easiest variant would be a command like

  nfc tag set <tag-index> <ndef-message>

for filling the tag at the beginning of the test case. What about the other attributes besides the NDEF message?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #15)
> Hi,
> 
> I'm not 100% sure about the requirements of the console interface, but I
> guess the easiest variant would be a command like
> 
>   nfc tag set <tag-index> <ndef-message>
> 
Agree, i am implementing in this way. The only difference is that i think it would be better to set <remote-endpoint-index> instead of <tag-index>. Otherwise in our scenario will look like:
1. nfc tag set 0 <ndef-message>       // set data of first tag
2. nfc nci rf_intf_activated_ntf 2    // activate second remote end point which is actual the first tag

I am thinking we could provide another console interface that returns remote endpoints information, for example, endpoint type(device or tag), type of tag ...etc . Then testcases could utilize this information to write more flexible code. How do you think ?

> for filling the tag at the beginning of the test case. What about the other
> attributes besides the NDEF message?

Do you mean tag specific attributes like uid, lock bit ...etc ?
(In reply to Dimi Lee[:dimi][:dlee] from comment #16)
> >   nfc tag set <tag-index> <ndef-message>
> > 
> Agree, i am implementing in this way. The only difference is that i think it
> would be better to set <remote-endpoint-index> instead of <tag-index>.

Sure, if that works better in practice.

> 
> I am thinking we could provide another console interface that returns remote
> endpoints information, for example, endpoint type(device or tag), type of
> tag ...etc . Then testcases could utilize this information to write more
> flexible code. How do you think ?

I don't know how helpful this is to test-case writers, but it could be a good thing. I can imaging having a helper function in the test cases that returns the index of an RE with a set of properties.

> 
> > for filling the tag at the beginning of the test case. What about the other
> > attributes besides the NDEF message?
> 
> Do you mean tag specific attributes like uid, lock bit ...etc ?

Yes.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #16)
> > > for filling the tag at the beginning of the test case. What about the other
> > > attributes besides the NDEF message?
> > 
> > Do you mean tag specific attributes like uid, lock bit ...etc ?
> 
> Yes.

For each type of tags it may has different attributes, also most of those attributes are not related to upper layer. I think it will too complicate for testcase writers to fill all those attributes. I would suggest keeping those will not affect NFC behavior hard-coded in Emulator (Manufacture ID, UID ...etc), for those may affect NFC behavior we could set a default value in Emulator and provide another tag command to config, for example, readable/writable bit. 
Is it reasonable for you ?
Sure, makes sense.
Change:
- Add nfc-def.h to include some pre-defined values in NFC Digital Spec
Attachment #8425330 - Attachment is obsolete: true
Attachment #8428600 - Flags: review?(tzimmermann)
Attachment #8425331 - Attachment is obsolete: true
Attachment #8428601 - Flags: review?(tzimmermann)
Attachment #8425333 - Attachment is obsolete: true
Attachment #8428603 - Flags: review?(tzimmermann)
Nothing changed
Attachment #8428604 - Flags: review?(tzimmermann)
Attachment #8428603 - Attachment description: Part3. Support console interface to set NFC tag data → Part3. Support console interface to set NFC tag data v1
Attached patch Testcase for reading NFC URL tag (obsolete) — Splinter Review
Attachment #8428607 - Flags: review?(tzimmermann)
Comment on attachment 8428600 [details] [diff] [review]
Part1. Add RF technology specific parameters v2

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

r+ with nit

::: hw/nfc-def.h
@@ +11,5 @@
> +*/
> +
> +#ifndef nfc_def_h
> +#define nfc_def_h
> +

Please move everything from this file into nfc-re.h
Attachment #8428600 - Flags: review?(tzimmermann) → review+
Comment on attachment 8428601 [details] [diff] [review]
Part2. Add type 2 tag as a remote end point v2

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

::: hw/nfc-tag.h
@@ +14,5 @@
> +#define nfc_tag_h
> +
> +#include "qemu-common.h"
> +
> +enum tag_type {

Maybe should be 'enum nfc_tag_type'?
Attachment #8428601 - Flags: review?(tzimmermann) → review+
Comment on attachment 8428603 [details] [diff] [review]
Part3.  Support console interface to set NFC tag data v1

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

r+ with nits.

::: android/console.c
@@ +4210,5 @@
> +
> +        if (nfc_tag_set_data(re->tag, buf, res) < 0) {
> +            return -1;
> +        }
> +    }

Ideally we'd have a nice interface for decoding the NDEF input directly into the tag's data field. But for now, this interface will do fine.

::: hw/nfc-tag.c
@@ +32,5 @@
>     INIT_NFC_T2T([0], T2T_INTERNAL, T2T_LOCK, T2T_CC)
>  };
>  
> +int
> +nfc_tag_set_data(const struct nfc_tag* tag, uint8_t* ndef_msg, ssize_t len)

The second argument should have the 'const' qualifier.

@@ +59,5 @@
> +    data[offset++] = NDEF_TERMINATOR_TLV;
> +
> +    return 1;
> +}
> +

Empty line.
Attachment #8428603 - Flags: review?(tzimmermann) → review+
Comment on attachment 8428604 [details] [diff] [review]
Part4. Support process type 2 tag read command v2

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

r+ with nits.

::: hw/nfc-tag.c
@@ +78,5 @@
> +    max_read = ARRAY_SIZE(rsp->payload);
> +
> +    for (i = 0; i < max_read && offset < T2T_STATIC_MEMORY_SIZE; i++, offset++) {
> +        rsp->payload[i] = mem[offset];
> +    }

Could this return undefined bytes? If so, the tail of mem should be cleared when setting it's value.

@@ +83,5 @@
> +    rsp->status = 0;
> +
> +    *consumed = sizeof(struct t2t_read_response);
> +
> +    return *consumed;

The value |*consumed| is the amount of consumed bytes from the input. The return value is the length of the response in bytes. Even if they are the same value, it's not actually correct to compute *consumed from the response and return *consumed here.

So if I'm not totally mistaken, *consumed should be sizeof(t2t_read_command) and the return value should be sizeof(t2t_read_response).

::: hw/nfc-tag.h
@@ +21,5 @@
>      T3T,
>      T4T,
>  };
>  
> +struct common_command_hdr {

Should be prefixed by something, maybe 't2t_'. Here and below.

@@ +40,5 @@
> +
> +/* [Digital], Table52 */
> +struct t2t_read_response {
> +    uint8_t payload[16];
> +    uint8_t status;

I can't |status| in the referenced document. There are only the 16 bytes for payload listed.
Attachment #8428604 - Flags: review?(tzimmermann) → review+
Comment on attachment 8428603 [details] [diff] [review]
Part3.  Support console interface to set NFC tag data v1

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

Some more comments on patch #3.

::: hw/nfc-tag.c
@@ +42,5 @@
> +    assert(ndef_msg);
> +
> +    switch (tag->type) {
> +        case T2T:
> +            data = tag->t2.format.data;

Make sure that len <= length of data.

@@ +57,5 @@
> +    offset += len;
> +
> +    data[offset++] = NDEF_TERMINATOR_TLV;
> +
> +    return 1;

Return 0 on success and -1 on error.

::: hw/nfc-tag.h
@@ +21,5 @@
>      T3T,
>      T4T,
>  };
>  
> +

Extra white line.
Comment on attachment 8428607 [details] [diff] [review]
Testcase for reading NFC URL tag

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

The test case passed.
Attachment #8428607 - Flags: review?(tzimmermann) → review+
Hi Dimi,

There are only some minor nits, but looks fine in general. Thanks for this patch set!
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #28)
> Comment on attachment 8428604 [details] [diff] [review]
> Part4. Support process type 2 tag read command v2
> 
> Review of attachment 8428604 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: hw/nfc-tag.h
> @@ +21,5 @@
> >      T3T,
> >      T4T,
> >  };
> >  
> > +struct common_command_hdr {
> 
> Should be prefixed by something, maybe 't2t_'. Here and below.
> 
Those structures without t2t header are shared by other type of tags.

> @@ +40,5 @@
> > +
> > +/* [Digital], Table52 */
> > +struct t2t_read_response {
> > +    uint8_t payload[16];
> > +    uint8_t status;
> 
> I can't |status| in the referenced document. There are only the 16 bytes for
> payload listed.

Yes, you are right. It doesn't list in the document. But i found libnfc-nci will read an extra byte as status code and also test with real tag it did return an extra status byte.

I will add a comment here to explain why i add a status byte.
No longer blocks: b2g-nfc
Blocks: b2g-nfc
Fix nits addressed by Thomas
Attachment #8428600 - Attachment is obsolete: true
Attachment #8428601 - Attachment is obsolete: true
Attachment #8428603 - Attachment is obsolete: true
Attachment #8428604 - Attachment is obsolete: true
Attachment #8428607 - Attachment is obsolete: true
Attachment #8429009 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Attachment #8429009 - Attachment is obsolete: true
Attachment #8429062 - Flags: review+
Keywords: checkin-needed
Attachment #8429066 - Flags: review+
https://hg.mozilla.org/integration/b2g-inbound/rev/700deec4f31d
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
https://hg.mozilla.org/mozilla-central/rev/700deec4f31d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.