[NFC] Emulator support tag format command

RESOLVED FIXED in Firefox OS v1.4

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dimi, Assigned: dimi)

Tracking

unspecified
2.1 S2 (15aug)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [p=2])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 8469183 [details] [diff] [review]
Bug 1050186 patch v1
Attachment #8469183 - Flags: review?(tzimmermann)
(Assignee)

Comment 2

4 years ago
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
Attachment #8469183 - Attachment is obsolete: true
Attachment #8469183 - Flags: review?(tzimmermann)
(Assignee)

Comment 3

4 years ago
Created attachment 8469774 [details] [diff] [review]
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.
Attachment #8469774 - Flags: review?(tzimmermann)
(Assignee)

Comment 4

4 years ago
Created attachment 8469776 [details] [diff] [review]
Part2. Emulator support tag format command

Support format command to rollback tag data to the state without setting NDEF data
Attachment #8469776 - Flags: review?(tzimmermann)
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 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. ;)
Attachment #8469776 - Flags: review?(tzimmermann) → review-
Attachment #8469774 - Flags: review?(tzimmermann) → review+
(Assignee)

Comment 7

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

Comment 8

4 years ago
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 :(
(Assignee)

Comment 9

4 years ago
Created attachment 8471307 [details] [diff] [review]
Part2. Emulator support tag format command v2
Attachment #8469776 - Attachment is obsolete: true
Attachment #8471307 - Flags: review?(tzimmermann)
Comment on attachment 8471307 [details] [diff] [review]
Part2. Emulator support tag format command v2

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

Thanks!
Attachment #8471307 - Flags: review?(tzimmermann) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 8472234 [details] [review]
pull request for mozilla-b2g/platform_external_qemu
Attachment #8469774 - Attachment is obsolete: true
Attachment #8471307 - Attachment is obsolete: true
Attachment #8472234 - Flags: review+
(Assignee)

Updated

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

Updated

4 years ago
Keywords: checkin-needed
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...
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g-v1.4: --- → fixed
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.