1016777 - [NFC] Emulator support for reading NDEF data from type 1 tag

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Dimi Lee [ooo 16.04 - 21.04］]

Emulator should support processing type 1 tag
- reading function should be implemented in this bug

Comment 1

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part1. Add type 1 tag as a remote end point v1

Comment 2

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part2. Process type 1 tag read command v1

Comment 3

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Testcase for bug 1016777

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8429949 [details] [diff] [review]
Part1.  Add type 1 tag as a remote end point v1

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

Looks good.

::: hw/nfc-re.c
@@ +26,4 @@
>      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_T1T, NCI_RF_NFC_A_PASSIVE_LISTEN_MODE, nfc_tags+0, "deadbeaf2", nfc_res+2),
> +    INIT_NFC_RE([3], NCI_RF_PROTOCOL_T2T, NCI_RF_NFC_A_PASSIVE_LISTEN_MODE, nfc_tags+1, "deadbeaf3", nfc_res+3)

It looks like the attached test case handles the index change correctly.

::: hw/nfc-tag.c
@@ +60,5 @@
> +
> +            data = tag->t1.format.data;
> +
> +            /* [Type 1 Tag Operation Specificatio] 6.1 NDEF Management */
> +            memcpy(data + offset, t1t_cc, ARRAY_SIZE(t1t_cc));

Since |t1t_cc| has static const qualifiers, I wonder if you could just init data with it's value and only append the NDEF message here.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8429950 [details] [diff] [review]
Part2. Process type 1 tag read command v1

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

Looks good.

::: hw/nfc-re.c
@@ +703,5 @@
> + * But from libnfc-nci it seems proprietary parameters is required for
> + * t1t tag.
> + */
> +static size_t
> +nfc_re_create_activated_ntf_t1t(struct nfc_re* re, uint8_t* act)

Since this is static, you should drop the prefix 'nfc_re_', here and below.

@@ +721,1 @@
>  size_t

'static'

::: hw/nfc-tag.c
@@ +115,5 @@
>  }
>  
> +static size_t
> +process_t1t_rid(struct nfc_tag* tag, const struct t1t_rid_command* cmd,
> +            uint8_t* consumed, struct t1t_rid_response* rsp)

Weird indention. This line should be aligned with the first argument above. Here and below.

@@ +125,5 @@
> +
> +    rsp->hr[0] = T1T_HRO;
> +    rsp->hr[1] = T1T_HR1;
> +
> +    memcpy(rsp->uid, tag->t1.format.uid, ARRAY_SIZE(rsp->uid));

sizeof(rsp->uid). It shouldn't make a difference here, but sizeof is more correct than ARRAY_SIZE in this case.

::: hw/nfc-tag.h
@@ +34,5 @@
> +    RALL_COMMAND = 0x00,
> +    RID_COMMAND = 0x78
> +};
> +
> +struct t1t_rall_command {

Maybe add reference to RALL spec like you did below for RID.

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8429956 [details] [diff] [review]
Testcase for bug 1016777

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

This probably requires a small update.

::: dom/nfc/tests/marionette/test_nfc_tag.js
@@ +59,5 @@
>  
>      toggleNFC(false, runNextTest);
>    });
>  
>    toggleNFC(true, function() {

This function now returns a Promise.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks a lot, Dimi! There are just nits and that one interface change in the test case.

Comment 8

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8429949 [details] [diff] [review]
Part1.  Add type 1 tag as a remote end point v1

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

::: hw/nfc-tag.c
@@ +60,5 @@
> +
> +            data = tag->t1.format.data;
> +
> +            /* [Type 1 Tag Operation Specificatio] 6.1 NDEF Management */
> +            memcpy(data + offset, t1t_cc, ARRAY_SIZE(t1t_cc));

The value of CC field here only indicate that there is NDEF data. So i think configure it when set NDEF would be better.
I will change the naming of t1t_cc to t1t_cc_ndef for better naming.

Comment 9

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Testcase for bug 1016777 v2

Comment 10

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: pull request for mozilla-b2g/platform_external_qemu

Comment 11

[Dimi Lee [ooo 16.04 - 21.04］]

Remove checkin-need because the coding style here may cause build fail in MAC environment.
Reference bug :  bug 1017038

Comment 12

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: pull request for mozilla-b2g/platform_external_qemu v2

Comment 13

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Testcase for bug 1016777 v3

Comment 14

[Dimi Lee [ooo 16.04 - 21.04］]

Set checkin-needed because bug 1017038 landed

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/platform_external_qemu/commit/0a2b7e94dce4989a3740fea6f6e3152978216c88

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Test case (v4)

Changes since v3:

  - rebased onto latest Gecko master

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Landing missing test case

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

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=30e01a6763ad

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/30e01a6763ad