1023683 - [NFC] Emulator support for reading NDEF data from type 3 tag

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Dimi Lee [:dimi]]

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

Comment 1

[Dimi Lee [:dimi]]

Attachment: Part1. Set rf mode according to protocol v1

Comment 2

[Dimi Lee [:dimi]]

Attachment: Part2 Support t3t polling command v1

Comment 3

[Dimi Lee [:dimi]]

Attachment: Part3 add type 3 tag as a remote end point v1

Comment 4

[Dimi Lee [:dimi]]

Attachment: Part4. Process type 3 tag check command v1

Comment 5

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

Currently blocked by bug 1021180.

Comment 6

[Dimi Lee [:dimi]]

Attachment: Bug 1023683 Testcase

Comment 7

[Dimi Lee [:dimi]]

Attachment: Part4. Process type 3 tag check command v2

Comment 8

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

Comment on attachment 8439094 [details] [diff] [review]
Part1. Set rf mode according to protocol v1

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

I'm r-'ing this patch, because I'm not convinced by the dynamic approach.

The current RF mode is quick hack I put in when I needed to get something working. Let's first discuss on IRC how to clean this up.

::: android/console.c
@@ +4001,5 @@
>      } else {
>          nfc->active_rf = nfc->rf + param->rf;
>      }
> +
> +    nfc_set_rf_mode_by_protocol(nfc->active_rf, param->re->rfproto);

This value should rather be set as part of the initialization of nfc_rf. Or is there a reason why this _must_ be set dynamically? I'd rather prefer to add new entries to nfc->rf.

::: hw/nfc-rf.c
@@ -22,4 @@
>      rf->iface = iface;
>      switch (iface) {
>          case NCI_RF_INTERFACE_FRAME:
> -            // TODO: this is also depend on protocol

You should pass in the protocol as well and do a more complex setup here.

::: hw/nfc.c
@@ +78,5 @@
>      return NULL;
>  }
>  
> +int
> +nfc_set_rf_mode_by_protocol(struct nfc_rf* rf, enum nci_rf_protocol proto)

This function should be located in nfc_rf.c

Comment 9

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

Comment on attachment 8439097 [details] [diff] [review]
Part2 Support t3t polling command v1

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

Nits only. Looks good!

::: hw/nfc-nci.c
@@ +781,5 @@
> +{
> +    /* We do nothing here except send notification to HOST */
> +    struct nfc_t3t_polling_ntf_param* ntf;
> +
> +    ntf = qemu_malloc(sizeof(struct nfc_t3t_polling_ntf_param));

Use 'sizeof(*ntf)'. It's less likely to break if the data structures get renamed or refactored.

::: hw/nfc-nci.h
@@ +488,5 @@
> +struct nci_t3t_polling_cmd {
> +    uint16_t sc;
> +    uint8_t rc;
> +    uint8_t tsn;
> +};

Please add ' __attribute__((packed))' right before the semicolon. It's not strictly necessary here, but certainly safer in the long run.

@@ +494,5 @@
> +struct nci_t3t_polling_ntf {
> +    uint8_t status;
> +    uint8_t nres;
> +    uint8_t res[];
> +};

Again, packed.

Comment 10

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

Comment on attachment 8439098 [details] [diff] [review]
Part3 add type 3 tag as a remote end point v1

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

Looks good.

::: hw/nfc-re.c
@@ +22,4 @@
>  #include "llcp-snep.h"
>  #include "nfc-re.h"
>  
> +/* NFCID2 is deifned in [Digital] Table44 */

'defined'

@@ +24,5 @@
>  
> +/* NFCID2 is deifned in [Digital] Table44 */
> +struct nfc_re nfc_res[5] = {
> +    INIT_NFC_RE([0], NCI_RF_PROTOCOL_NFC_DEP, NCI_RF_NFC_F_PASSIVE_LISTEN_MODE,
> +                NULL, "deadbeaf0", "\x01\xfe\x0\x0\x0\x0\x1", nfc_res+0),

Start counting at 0, instead of 1, so that indices in the names are equal.

::: hw/nfc-tag.h
@@ +142,5 @@
>      struct nfc_t2t_format format;
>  };
>  
> +/**
> + * There is no specefic size defined in T3T sepc.

'specific'

Comment 11

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

Comment on attachment 8439764 [details] [diff] [review]
Part4. Process type 3 tag check command v2

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

This looks good, except for some the missing enum values. If we define constants from a standard, we should define all that belong together. Will make it easier to later see what's missing.

As a side note, nfc-re.c keeps growing and we should think about splitting it up.

::: hw/nfc-tag.h
@@ +90,5 @@
> +
> +enum t3t_command_set {
> +    CHECK_COMMAND = 0x06,
> +    UPDATE_COMMAND = 0x08
> +};

[T3TOP] also lists the command 'POLLING'.

Comment 12

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

Comment on attachment 8439094 [details] [diff] [review]
Part1. Set rf mode according to protocol v1

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

We land this now and improve the RF and RE handling in bug 1026422.

Comment 13

[Dimi Lee [:dimi]]

Attachment: Part1. Set rf mode according to protocol v2

Comment 14

[Dimi Lee [:dimi]]

Attachment: Part2. Support t3t polling command v2

Comment 15

[Dimi Lee [:dimi]]

Attachment: Part3. Add type 3 tag as a remote end point v2

Comment 16

[Dimi Lee [:dimi]]

Attachment: Part4. Process type 3 tag check command v3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please run this through Try (and post a link here) before requesting checkin. Thanks! :)

Comment 18

[Dimi Lee [:dimi]]

Attachment: pull request for mozilla-b2g/platform_external_qemu

sorry, i forget this should be a pull request

Comment 19

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/platform_external_qemu/commit/973367035a1f2545f3dad6e40e354463dc56a7f4

Comment 20

[Dimi Lee [:dimi]]

Test case try server result :  https://tbpl.mozilla.org/?tree=Try&rev=5521312d4b27
Please help merge test case for this bug, thanks

Comment 21

[Dimi Lee [:dimi]]

remove checkin-need base testcase need to update according to latest head.js change

Comment 22

[Dimi Lee [:dimi]]

Attachment: Bug 1023683 Testcase v2

Comment 23

[Dimi Lee [:dimi]]

checkin-needed for rebased testcase

Comment 24

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

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

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/a8408f4e3642