Closed Bug 1023683 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S4 (20june)

People

(Reporter: dimi, Assigned: dimi)

References

Details

(Whiteboard: [p=5])

Attachments

(2 files, 10 obsolete files)

62 bytes, text/x-github-pull-request
dimi
: review+
Details | Review
1.38 KB, patch
dimi
: review+
Details | Diff | Splinter Review
Emulator should support processing type 3 tag
- reading function should be implemented in this bug
Attachment #8439094 - Flags: review?(tzimmermann)
Attachment #8439097 - Flags: review?(tzimmermann)
Attachment #8439098 - Flags: review?(tzimmermann)
Attachment #8439099 - Flags: review?(tzimmermann)
Currently blocked by bug 1021180.
Depends on: 1021180
Attached patch Bug 1023683 Testcase (obsolete) — Splinter Review
Attachment #8439729 - Flags: review?(tzimmermann)
Attachment #8439099 - Attachment is obsolete: true
Attachment #8439099 - Flags: review?(tzimmermann)
Attachment #8439764 - Flags: review?(tzimmermann)
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
Attachment #8439094 - Flags: review?(tzimmermann) → review-
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.
Attachment #8439097 - Flags: review?(tzimmermann) → review+
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'
Attachment #8439098 - Flags: review?(tzimmermann) → review+
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'.
Attachment #8439764 - Flags: review?(tzimmermann) → review+
Attachment #8439729 - Flags: review?(tzimmermann) → review+
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.
Attachment #8439094 - Flags: review- → review+
Attachment #8439094 - Attachment is obsolete: true
Attachment #8441283 - Flags: review+
Attachment #8439097 - Attachment is obsolete: true
Attachment #8441285 - Flags: review+
Attachment #8439098 - Attachment is obsolete: true
Attachment #8441287 - Flags: review+
Attachment #8439764 - Attachment is obsolete: true
Attachment #8441288 - Flags: review+
Keywords: checkin-needed
Please run this through Try (and post a link here) before requesting checkin. Thanks! :)
Keywords: checkin-needed
sorry, i forget this should be a pull request
Attachment #8441283 - Attachment is obsolete: true
Attachment #8441285 - Attachment is obsolete: true
Attachment #8441287 - Attachment is obsolete: true
Attachment #8441288 - Attachment is obsolete: true
Attachment #8441805 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Test case try server result :  https://tbpl.mozilla.org/?tree=Try&rev=5521312d4b27
Please help merge test case for this bug, thanks
Keywords: checkin-needed
remove checkin-need base testcase need to update according to latest head.js change
Keywords: checkin-needed
Attachment #8439729 - Attachment is obsolete: true
Attachment #8448546 - Flags: review+
checkin-needed for rebased testcase
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.