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

RESOLVED FIXED in Firefox OS v1.3

Status

Firefox OS
NFC
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dimi, Assigned: dimi)

Tracking

(Blocks: 1 bug)

unspecified
2.0 S3 (6june)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [p=2])

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

3 years ago
Emulator should support processing type 1 tag
- reading function should be implemented in this bug
(Assignee)

Updated

3 years ago
Whiteboard: [p=2]
(Assignee)

Updated

3 years ago
Summary: Bug 993836 - [NFC] Emulator support for reading NDEF data from type 1 tag → [NFC] Emulator support for reading NDEF data from type 1 tag
(Assignee)

Comment 1

3 years ago
Created attachment 8429949 [details] [diff] [review]
Part1.  Add type 1 tag as a remote end point v1
Attachment #8429949 - Flags: review?(tzimmermann)
(Assignee)

Comment 2

3 years ago
Created attachment 8429950 [details] [diff] [review]
Part2. Process type 1 tag read command v1
Attachment #8429950 - Flags: review?(tzimmermann)
(Assignee)

Comment 3

3 years ago
Created attachment 8429956 [details] [diff] [review]
Testcase for bug 1016777
Attachment #8429956 - Flags: review?(tzimmermann)
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.
Attachment #8429949 - Flags: review?(tzimmermann) → review+
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.
Attachment #8429950 - Flags: review?(tzimmermann) → review+
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.
Attachment #8429956 - Flags: review?(tzimmermann) → review+
Thanks a lot, Dimi! There are just nits and that one interface change in the test case.
Blocks: 980851
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
Created attachment 8430505 [details] [diff] [review]
Testcase for bug 1016777 v2
Attachment #8429956 - Attachment is obsolete: true
Attachment #8430505 - Flags: review+
(Assignee)

Comment 10

3 years ago
Created attachment 8430513 [details] [review]
pull request for mozilla-b2g/platform_external_qemu
Attachment #8429949 - Attachment is obsolete: true
Attachment #8429950 - Attachment is obsolete: true
Attachment #8430513 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 11

3 years ago
Remove checkin-need because the coding style here may cause build fail in MAC environment.
Reference bug :  bug 1017038
(Assignee)

Comment 12

3 years ago
Created attachment 8431300 [details] [review]
pull request for mozilla-b2g/platform_external_qemu v2
Attachment #8430513 - Attachment is obsolete: true
Attachment #8431300 - Flags: review+
(Assignee)

Comment 13

3 years ago
Created attachment 8431303 [details]
Testcase for bug 1016777 v3
Attachment #8430505 - Attachment is obsolete: true
Attachment #8431303 - Flags: review+
(Assignee)

Comment 14

3 years ago
Set checkin-needed because bug 1017038 landed
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/platform_external_qemu/commit/0a2b7e94dce4989a3740fea6f6e3152978216c88
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-v1.4: --- → fixed
status-b2g-v2.0: --- → fixed
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
status-b2g-v1.3: --- → fixed
status-b2g-v1.3T: --- → fixed
(Assignee)

Updated

3 years ago
Blocks: 1020057
Created attachment 8437532 [details] [diff] [review]
Test case (v4)

Changes since v3:

  - rebased onto latest Gecko master
Attachment #8431303 - Attachment is obsolete: true
Attachment #8437532 - Flags: review+
Landing missing test case

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

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=30e01a6763ad
https://hg.mozilla.org/mozilla-central/rev/30e01a6763ad
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1020057
Blocks: 860906
Blocks: 1030628
No longer blocks: 860906
You need to log in before you can comment on or make changes to this bug.