1050186 - [NFC] Emulator support tag format command

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Dimi Lee [:dimi]]

For bug 1049429, testcase will test tag support NDEF and NON-NDEF format.

So emulator will have to support function to set tag data back to original format.

Comment 1

[Dimi Lee [:dimi]]

Attachment: Bug 1050186 patch v1

Comment 2

[Dimi Lee [:dimi]]

Comment on attachment 8469183 [details] [diff] [review]
Bug 1050186 patch v1

This version may have bug when running get ndef detail for type 4 tag

Comment 3

[Dimi Lee [:dimi]]

Attachment: Part1. Use proprietarty cc as the initial value of t4t

Before ndef data is set to type 4 tag, use proprietary cc instead of ndef cc. Otherwise we will detect type 4 tag as NDEF format but there is no ndef data actually.

Comment 4

[Dimi Lee [:dimi]]

Attachment: Part2. Emulator support tag format command

Support format command to rollback tag data to the state without setting NDEF data

Comment 5

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

Comment on attachment 8469774 [details] [diff] [review]
Part1. Use proprietarty cc as the initial value of t4t

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

Just a quick question. I thought the tags wouldn't be detected before we set a value. So I don't understand where this problem shows up.

::: hw/nfc-tag.c
@@ +46,5 @@
>  
>  /* [Type 4 Tag Operation Specification] */
>  static enum t4t_file_select t4t_file_sel = NONE;
>  
>  /* [T4TOP] Table5 */

Maybe also refer to table 7, to signal difference between NDEF and proprietary values.

@@ +157,5 @@
>      assert(tag);
>      assert(ndef_msg || !len);
>      assert(len + 2 < sizeof(tag->t.t4.format.data));
>  
> +    uint8_t cc[] = T4T_NDEF_CC;

Please move this line to the op of the function and mark it as 'static const'.

Comment 6

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

Comment on attachment 8469776 [details] [diff] [review]
Part2. Emulator support tag format command

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

r- for now because there is a bug.

::: hw/nfc-tag.c
@@ +194,5 @@
> +int
> +nfc_tag_format(struct nfc_tag* tag)
> +{
> +    switch (tag->type) {
> +        case T1T: {

Please remove the braces here and put them into the format macros.

@@ +210,5 @@
> +        case T4T: {
> +            FORMAT_NFC_T4T(tag, T4T_PROPRIETARY_CC)
> +            break;
> +        }
> +        default: {

No braces at all in the default branch.

::: hw/nfc-tag.h
@@ +278,5 @@
>      uint8_t mem[T3T_MEMORY_SIZE];
>  };
>  
>  struct nfc_t3t_format {
> +    uint8_t ver[1];

I don't understand this. Why do you use |ver[]|, instead of |ver|? The memcpy in FORMAT_NFC_... should be able to use  |&ver|.

@@ +354,5 @@
>          .t.t4.format.cc = cc_, \
>      }
>  
> +#define FORMAT_NFC_T1T(tag_, uid_, res_) \
> +    memset(tag, 0, sizeof(struct nfc_tag)); \

I'd prefer to not have these memsets in the macros. We're writing most fields anyway afterwards. If you need specific fields to be cleared, please do that instead.

@@ +356,5 @@
>  
> +#define FORMAT_NFC_T1T(tag_, uid_, res_) \
> +    memset(tag, 0, sizeof(struct nfc_tag)); \
> +    tag_->type = T1T; \
> +    uint8_t uid[] = uid_; memcpy(tag_->t.t1.format.uid, uid, sizeof(uid)); \

Again, please move these arrays to the top of the macro and mark them as 'static const'. Here and below.

@@ +370,5 @@
> +#define FORMAT_NFC_T3T(tag_, v_, r_, w_, nb_, u_, wf_, rw_, ln_, cs_) \
> +    memset(tag, 0, sizeof(struct nfc_tag)); \
> +    tag_->type = T3T; \
> +    uint8_t v[] = v_;   memcpy(tag_->t.t3.format.ver, v, sizeof(v));   \
> +    uint8_t r[] = r_;   memcpy(tag_->t.t3.format.ver, r, sizeof(r));   \

You are writing to |ver| over and over. ;)

Comment 7

[Dimi Lee [:dimi]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> Comment on attachment 8469774 [details] [diff] [review]
> Part1. Use proprietarty cc as the initial value of t4t
> 
> Review of attachment 8469774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a quick question. I thought the tags wouldn't be detected before we set
> a value. So I don't understand where this problem shows up.
> 

The problem is that I want to write a testcase trying to get information from a NON-NDEF formatted tag.

In testcase test_nfc_read_tag.js it will set tag with NDEF data, so i will need a way to rollback the tag to original state before doing my new testcase.

Comment 8

[Dimi Lee [:dimi]]

Comment on attachment 8469776 [details] [diff] [review]
Part2. Emulator support tag format command

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

::: hw/nfc-tag.h
@@ +278,5 @@
>      uint8_t mem[T3T_MEMORY_SIZE];
>  };
>  
>  struct nfc_t3t_format {
> +    uint8_t ver[1];

just want to make the FORMAT... MACRO looks uniform. I will remove this change in next patch.

@@ +370,5 @@
> +#define FORMAT_NFC_T3T(tag_, v_, r_, w_, nb_, u_, wf_, rw_, ln_, cs_) \
> +    memset(tag, 0, sizeof(struct nfc_tag)); \
> +    tag_->type = T3T; \
> +    uint8_t v[] = v_;   memcpy(tag_->t.t3.format.ver, v, sizeof(v));   \
> +    uint8_t r[] = r_;   memcpy(tag_->t.t3.format.ver, r, sizeof(r));   \

Ah...thanks for finding this. I am really too uncareful :(

Comment 9

[Dimi Lee [:dimi]]

Attachment: Part2. Emulator support tag format command v2

Comment 10

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

Comment on attachment 8471307 [details] [diff] [review]
Part2. Emulator support tag format command v2

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

Thanks!

Comment 11

[Dimi Lee [:dimi]]

Attachment: pull request for mozilla-b2g/platform_external_qemu

Comment 12

[Ryan VanderMeulen [:RyanVM]]

b2g-jellybean: https://github.com/mozilla-b2g/platform_external_qemu/commit/30d0dfa566651fea8031551e86cec6018b7bbb12

Oh my, we're running off the same rev from this repo for 1.4+? That doesn't necessarily seem....good...