Closed
Bug 993836
Opened 10 years ago
Closed 10 years ago
[NFC] Emulator support for reading NDEF data from type 2 tag
Categories
(Firefox OS Graveyard :: NFC, defect)
Tracking
(tracking-b2g:backlog, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)
People
(Reporter: dimi, Assigned: dimi)
References
Details
(Whiteboard: [p=5])
Attachments
(2 files, 11 obsolete files)
No description provided.
Assignee | ||
Updated•10 years ago
|
Whiteboard: [p=3]
Target Milestone: --- → 1.4 S6 (25apr)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [p=3] → [p=5]
Updated•10 years ago
|
blocking-b2g: --- → backlog
Assignee | ||
Updated•10 years ago
|
Summary: [NFC] Emulator support for reading NDEF data from type 1 tag → [NFC] Emulator support for reading NDEF data from type 2 tag
Assignee | ||
Comment 1•10 years ago
|
||
Testcase will add in bug 1001315
Attachment #8412411 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 2•10 years ago
|
||
Hi Thomas, In this bug, i use hard code value for NDEF message because i think we will not change it very often and we do not require create NDEF message dynamically. But it is also ok for me if you think it is more flexible to generate it dynamically. Use following command should pop up browser in emulator "nfc ntf rf_intf_activated 2"
Assignee | ||
Updated•10 years ago
|
Attachment #8412411 -
Attachment is obsolete: true
Attachment #8412411 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8412417 -
Flags: review?(tzimmermann)
Comment 4•10 years ago
|
||
Hi Dimi (In reply to Dimi Lee[:dimi][:dlee] from comment #2) > Hi Thomas, > In this bug, i use hard code value for NDEF message because i think we will > not change it very often and we do not require create NDEF message > dynamically. But it is also ok for me if you think it is more flexible to > generate it dynamically. Have a look at bug 1000935 for dynamic NDEF messages. The patches contains support for reading and writing via console. NDEF messages are given to the emulator in a simple textual format. And the emulator can output received NDEF messages in JSON format for parsing by a JS test case. > Use following command should pop up browser in emulator > "nfc ntf rf_intf_activated 2" I strongly oppose mixing up functionality. This command is for sending a specific NCI notification. There should be a different command for transferring data.
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4) > Hi Dimi > > (In reply to Dimi Lee[:dimi][:dlee] from comment #2) > > Hi Thomas, > > In this bug, i use hard code value for NDEF message because i think we will > > not change it very often and we do not require create NDEF message > > dynamically. But it is also ok for me if you think it is more flexible to > > generate it dynamically. > > Have a look at bug 1000935 for dynamic NDEF messages. The patches contains > support for reading and writing via console. NDEF messages are given to the > emulator in a simple textual format. And the emulator can output received > NDEF messages in JSON format for parsing by a JS test case. > I will rebase after bug 1000935 landed and use dynamic NDEF. > > Use following command should pop up browser in emulator > > "nfc ntf rf_intf_activated 2" > > I strongly oppose mixing up functionality. This command is for sending a > specific NCI notification. There should be a different command for > transferring data. This command is not for sending data, it just notify t1t tag discover. The reason browser will pop up is because after detecting tag, nfcd will fetch ndef data from tag immediately and notify GAIA. As discussed on IRC, I will cancel the review for now and re-base the code in bug 1000935.
Assignee | ||
Updated•10 years ago
|
Attachment #8412417 -
Flags: review?(tzimmermann)
Updated•10 years ago
|
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Assignee | ||
Updated•10 years ago
|
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8412417 -
Attachment is obsolete: true
Attachment #8425330 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8425331 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8425333 -
Flags: review?(tzimmermann)
Comment 9•10 years ago
|
||
Comment on attachment 8425330 [details] [diff] [review] Part1. Add RF technology specific parameters v1 Review of attachment 8425330 [details] [diff] [review]: ----------------------------------------------------------------- This looks quite reasonable to me, except for the nits I noticed. Is there a test case? I won't r+ these patches until we have a test for them. So only f+ for now. ::: hw/nfc-re.c @@ +636,5 @@ > + *p = 0; > + } else if (cid1_len == 7) { > + *p = 1 << 6; > + } else if (cid1_len == 10) { > + *p = 2 << 6; Instead of shifting, you could also define some constants and set the bitmask directly here. To me, this seems easier to understand. @@ +643,5 @@ > + } > + > + if (protocol == NCI_RF_PROTOCOL_T1T) { > + p++; /* Byte1 of SENS_RES */ > + *p++ = 3 << 2; /* Byte2 of SENS_RES */ I'd prefer to have constant bitmasks everywhere instead of shifting numbers around. I know that I'm sloppily doing this as well, but don't copy my bad habits. ;) @@ +646,5 @@ > + p++; /* Byte1 of SENS_RES */ > + *p++ = 3 << 2; /* Byte2 of SENS_RES */ > + *p++ = 0; /* NFCID1 Length */ > + *p++ = 0; > + } else { Alignment.
Attachment #8425330 -
Flags: review?(tzimmermann) → feedback+
Comment 10•10 years ago
|
||
Comment on attachment 8425331 [details] [diff] [review] Part2. Add type 2 tag as a remote end point v1 Review of attachment 8425331 [details] [diff] [review]: ----------------------------------------------------------------- r- because the tag-handling needs to be reworked. ::: hw/goldfish_nfc.c @@ +293,4 @@ > goldfish_device_add(&s->dev, goldfish_nfc_readfn, > goldfish_nfc_writefn, s); > > + nfc_re_init(); Should be gone after the changes to nfc-tag.{c,h} ::: hw/nfc-re.c @@ +24,5 @@ > > struct nfc_re nfc_res[3] = { > + 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_T2T, NCI_RF_NFC_A_PASSIVE_LISTEN_MODE, nfc_tags, "deadbeaf2", nfc_res+2) Are you referring to a single element or the complete array? My feeling says that the RE is the tag, so it's just one element, but the code says otherwise. ::: hw/nfc-tag.c @@ +30,5 @@ > + 0x0c, 0x04, 0x55, 0x6e, > + 0x75, 0x6c, 0x6c, 0x01, > + 0x6d, 0x6f, 0x7a, 0x69, > + 0x6c, 0x6c, 0x61, 0x2e, > + 0x6f, 0x72, 0x67, 0xfe }; I have no idea what these numbers mean and it's almost impossible to change them. So no NDEF messages here please. NDEF messages should be constructed in the test cases and passed to the emulator. There is helper code for the tests and the console now contains an NDEF parser that should be good enough. @@ +37,5 @@ > + > +void > +nfc_tag_init() > +{ > + INIT_NFC_T2T(nfc_tags[0], t2t_internal, t2t_lock, t2t_cc, tlv_url_ndef); As mentioned in nfc-tag.h, just pass the values so that you don't need to memcpy. Than you should remove nfc_tag_init and make it a global static initializer. ::: hw/nfc-tag.h @@ +45,5 @@ > +#define INIT_NFC_T2T(tag_, internal_, lock_, cc_, data_) \ > + memcpy(tag_.t2t.format.internal, internal_, ARRAY_SIZE(internal_)); \ > + memcpy(tag_.t2t.format.lock, lock_, ARRAY_SIZE(lock_)); \ > + memcpy(tag_.t2t.format.cc, cc_, ARRAY_SIZE(cc_)); \ > + memcpy(tag_.t2t.format.data, data_, ARRAY_SIZE(data_)); This is a static initializer, no memcpys here. And since everything you pass in is static itself, you don't have to memcpy. Simply do an assignment from the values.
Attachment #8425331 -
Flags: review?(tzimmermann) → review-
Updated•10 years ago
|
Attachment #8425333 -
Flags: review?(tzimmermann) → feedback+
Comment 11•10 years ago
|
||
Hi Dimi, Thanks for the patch set. I think what's still missing is a console interface for setting the tag's NDEF message and a test case.
Assignee | ||
Comment 12•10 years ago
|
||
create bug 1013746 to support console interface for setting tag's NDEF message
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Dimi Lee[:dimi][:dlee] from comment #12) > create bug 1013746 to support console interface for setting tag's NDEF > message Ah, sorry, it would be better and easier to provide patch in this bug. Set bug 1013746 to duplicate
Comment 15•10 years ago
|
||
Hi, I'm not 100% sure about the requirements of the console interface, but I guess the easiest variant would be a command like nfc tag set <tag-index> <ndef-message> for filling the tag at the beginning of the test case. What about the other attributes besides the NDEF message?
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #15) > Hi, > > I'm not 100% sure about the requirements of the console interface, but I > guess the easiest variant would be a command like > > nfc tag set <tag-index> <ndef-message> > Agree, i am implementing in this way. The only difference is that i think it would be better to set <remote-endpoint-index> instead of <tag-index>. Otherwise in our scenario will look like: 1. nfc tag set 0 <ndef-message> // set data of first tag 2. nfc nci rf_intf_activated_ntf 2 // activate second remote end point which is actual the first tag I am thinking we could provide another console interface that returns remote endpoints information, for example, endpoint type(device or tag), type of tag ...etc . Then testcases could utilize this information to write more flexible code. How do you think ? > for filling the tag at the beginning of the test case. What about the other > attributes besides the NDEF message? Do you mean tag specific attributes like uid, lock bit ...etc ?
Comment 17•10 years ago
|
||
(In reply to Dimi Lee[:dimi][:dlee] from comment #16) > > nfc tag set <tag-index> <ndef-message> > > > Agree, i am implementing in this way. The only difference is that i think it > would be better to set <remote-endpoint-index> instead of <tag-index>. Sure, if that works better in practice. > > I am thinking we could provide another console interface that returns remote > endpoints information, for example, endpoint type(device or tag), type of > tag ...etc . Then testcases could utilize this information to write more > flexible code. How do you think ? I don't know how helpful this is to test-case writers, but it could be a good thing. I can imaging having a helper function in the test cases that returns the index of an RE with a set of properties. > > > for filling the tag at the beginning of the test case. What about the other > > attributes besides the NDEF message? > > Do you mean tag specific attributes like uid, lock bit ...etc ? Yes.
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17) > (In reply to Dimi Lee[:dimi][:dlee] from comment #16) > > > for filling the tag at the beginning of the test case. What about the other > > > attributes besides the NDEF message? > > > > Do you mean tag specific attributes like uid, lock bit ...etc ? > > Yes. For each type of tags it may has different attributes, also most of those attributes are not related to upper layer. I think it will too complicate for testcase writers to fill all those attributes. I would suggest keeping those will not affect NFC behavior hard-coded in Emulator (Manufacture ID, UID ...etc), for those may affect NFC behavior we could set a default value in Emulator and provide another tag command to config, for example, readable/writable bit. Is it reasonable for you ?
Comment 19•10 years ago
|
||
Sure, makes sense.
Assignee | ||
Comment 20•10 years ago
|
||
Change: - Add nfc-def.h to include some pre-defined values in NFC Digital Spec
Attachment #8425330 -
Attachment is obsolete: true
Attachment #8428600 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8425331 -
Attachment is obsolete: true
Attachment #8428601 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8425333 -
Attachment is obsolete: true
Attachment #8428603 -
Flags: review?(tzimmermann)
Assignee | ||
Comment 23•10 years ago
|
||
Nothing changed
Attachment #8428604 -
Flags: review?(tzimmermann)
Assignee | ||
Updated•10 years ago
|
Attachment #8428603 -
Attachment description: Part3. Support console interface to set NFC tag data → Part3. Support console interface to set NFC tag data v1
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8428607 -
Flags: review?(tzimmermann)
Comment 25•10 years ago
|
||
Comment on attachment 8428600 [details] [diff] [review] Part1. Add RF technology specific parameters v2 Review of attachment 8428600 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nit ::: hw/nfc-def.h @@ +11,5 @@ > +*/ > + > +#ifndef nfc_def_h > +#define nfc_def_h > + Please move everything from this file into nfc-re.h
Attachment #8428600 -
Flags: review?(tzimmermann) → review+
Comment 26•10 years ago
|
||
Comment on attachment 8428601 [details] [diff] [review] Part2. Add type 2 tag as a remote end point v2 Review of attachment 8428601 [details] [diff] [review]: ----------------------------------------------------------------- ::: hw/nfc-tag.h @@ +14,5 @@ > +#define nfc_tag_h > + > +#include "qemu-common.h" > + > +enum tag_type { Maybe should be 'enum nfc_tag_type'?
Attachment #8428601 -
Flags: review?(tzimmermann) → review+
Comment 27•10 years ago
|
||
Comment on attachment 8428603 [details] [diff] [review] Part3. Support console interface to set NFC tag data v1 Review of attachment 8428603 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits. ::: android/console.c @@ +4210,5 @@ > + > + if (nfc_tag_set_data(re->tag, buf, res) < 0) { > + return -1; > + } > + } Ideally we'd have a nice interface for decoding the NDEF input directly into the tag's data field. But for now, this interface will do fine. ::: hw/nfc-tag.c @@ +32,5 @@ > INIT_NFC_T2T([0], T2T_INTERNAL, T2T_LOCK, T2T_CC) > }; > > +int > +nfc_tag_set_data(const struct nfc_tag* tag, uint8_t* ndef_msg, ssize_t len) The second argument should have the 'const' qualifier. @@ +59,5 @@ > + data[offset++] = NDEF_TERMINATOR_TLV; > + > + return 1; > +} > + Empty line.
Attachment #8428603 -
Flags: review?(tzimmermann) → review+
Comment 28•10 years ago
|
||
Comment on attachment 8428604 [details] [diff] [review] Part4. Support process type 2 tag read command v2 Review of attachment 8428604 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits. ::: hw/nfc-tag.c @@ +78,5 @@ > + max_read = ARRAY_SIZE(rsp->payload); > + > + for (i = 0; i < max_read && offset < T2T_STATIC_MEMORY_SIZE; i++, offset++) { > + rsp->payload[i] = mem[offset]; > + } Could this return undefined bytes? If so, the tail of mem should be cleared when setting it's value. @@ +83,5 @@ > + rsp->status = 0; > + > + *consumed = sizeof(struct t2t_read_response); > + > + return *consumed; The value |*consumed| is the amount of consumed bytes from the input. The return value is the length of the response in bytes. Even if they are the same value, it's not actually correct to compute *consumed from the response and return *consumed here. So if I'm not totally mistaken, *consumed should be sizeof(t2t_read_command) and the return value should be sizeof(t2t_read_response). ::: hw/nfc-tag.h @@ +21,5 @@ > T3T, > T4T, > }; > > +struct common_command_hdr { Should be prefixed by something, maybe 't2t_'. Here and below. @@ +40,5 @@ > + > +/* [Digital], Table52 */ > +struct t2t_read_response { > + uint8_t payload[16]; > + uint8_t status; I can't |status| in the referenced document. There are only the 16 bytes for payload listed.
Attachment #8428604 -
Flags: review?(tzimmermann) → review+
Comment 29•10 years ago
|
||
Comment on attachment 8428603 [details] [diff] [review] Part3. Support console interface to set NFC tag data v1 Review of attachment 8428603 [details] [diff] [review]: ----------------------------------------------------------------- Some more comments on patch #3. ::: hw/nfc-tag.c @@ +42,5 @@ > + assert(ndef_msg); > + > + switch (tag->type) { > + case T2T: > + data = tag->t2.format.data; Make sure that len <= length of data. @@ +57,5 @@ > + offset += len; > + > + data[offset++] = NDEF_TERMINATOR_TLV; > + > + return 1; Return 0 on success and -1 on error. ::: hw/nfc-tag.h @@ +21,5 @@ > T3T, > T4T, > }; > > + Extra white line.
Comment 30•10 years ago
|
||
Comment on attachment 8428607 [details] [diff] [review] Testcase for reading NFC URL tag Review of attachment 8428607 [details] [diff] [review]: ----------------------------------------------------------------- The test case passed.
Attachment #8428607 -
Flags: review?(tzimmermann) → review+
Comment 31•10 years ago
|
||
Hi Dimi, There are only some minor nits, but looks fine in general. Thanks for this patch set!
Blocks: b2g-nfc
Assignee | ||
Comment 32•10 years ago
|
||
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #28) > Comment on attachment 8428604 [details] [diff] [review] > Part4. Support process type 2 tag read command v2 > > Review of attachment 8428604 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: hw/nfc-tag.h > @@ +21,5 @@ > > T3T, > > T4T, > > }; > > > > +struct common_command_hdr { > > Should be prefixed by something, maybe 't2t_'. Here and below. > Those structures without t2t header are shared by other type of tags. > @@ +40,5 @@ > > + > > +/* [Digital], Table52 */ > > +struct t2t_read_response { > > + uint8_t payload[16]; > > + uint8_t status; > > I can't |status| in the referenced document. There are only the 16 bytes for > payload listed. Yes, you are right. It doesn't list in the document. But i found libnfc-nci will read an extra byte as status code and also test with real tag it did return an extra status byte. I will add a comment here to explain why i add a status byte.
No longer blocks: b2g-nfc
Assignee | ||
Comment 33•10 years ago
|
||
Fix nits addressed by Thomas
Attachment #8428600 -
Attachment is obsolete: true
Attachment #8428601 -
Attachment is obsolete: true
Attachment #8428603 -
Attachment is obsolete: true
Attachment #8428604 -
Attachment is obsolete: true
Attachment #8428607 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8429009 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #8429009 -
Attachment is obsolete: true
Attachment #8429062 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8429066 -
Flags: review+
Comment 36•10 years ago
|
||
platform_external_qemu/b2g-jellybean: https://github.com/mozilla-b2g/platform_external_qemu/commit/02f0cc39345c36689ea4bca436b789b38f5751e0
Comment 38•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/700deec4f31d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
status-b2g-v1.2:
--- → fixed
status-b2g-v1.3:
--- → fixed
status-b2g-v1.3T:
--- → fixed
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•