Closed Bug 993330 Opened 6 years ago Closed 6 years ago

[NFC] support notify tag discover by emulator console command

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
1.4 S6 (25apr)
tracking-b2g backlog

People

(Reporter: dimi, Assigned: dimi)

References

Details

(Whiteboard: [p=2])

Attachments

(3 files, 1 obsolete file)

Use emulator NFC console command "nfc ntf rf_intf_activated <i>" should be able to notify tag information. But not working currently.
Root cause:
Local RF technology activated and Remote End Point Tag protocol not matched

Remote End Point Protocol      RF technology & Mode                           
NCI_RF_PROTOCOL_T1T             NCI_RF_NFC_A_PASSIVE_POLL_MODE
NCI_RF_PROTOCOL_T2T             NCI_RF_NFC_A_PASSIVE_POLL_MODE
NCI_RF_PROTOCOL_T3T             NCI_RF_NFC_F_PASSIVE_POLL_MODE
NCI_RF_PROTOCOL_ISO_DEP         NCI_RF_NFC_A_PASSIVE_POLL_MODE or NCI_RF_NFC_B_PASSIVE_POLL_MODE
Blocks: 993836
Whiteboard: [p=2]
Target Milestone: --- → 1.4 S6 (25apr)
blocking-b2g: --- → backlog
Attached patch Patch v1 (obsolete) — Splinter Review
Comment on attachment 8408070 [details] [diff] [review]
Patch v1

Following is the modification in this patch:
1. Add a T2T remote end point.
2. Add FRAME_INTERFACE for detecting T2T
3. Support multiple RF interface for local device, active rf interface could be specified or auto detected
4. Update nfc emulator command 
 - nfc ntf rf_intf_activated          : auto detect active rf interface
 - nfc ntf rf_intf_activated <i>      : auto detect active rf interface
 - nfc ntf rf_intf_activated <i> <j>  : specify rf interface index <j>
Attachment #8408070 - Flags: review?(tzimmermann)
The easiest way to trigger a tag detection is use following command
>> nfc ntf rf_intf_activated 2

But now NDEF message detect and receive from tag is not yet support and should be implement in bug 993836
Comment on attachment 8408070 [details] [diff] [review]
Patch v1

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

Hi Dimi,

Thanks for the patch. Besides the listed comments, there are two things:

- I can't apply the patch for testing. Could you rebase it on the latest b2g/b2g-jellybean?
- Could you split of the move of rf_state into a separate patch?

Best regards
Thomas

::: android/console.c
@@ +3416,5 @@
> +            case NCI_RF_PROTOCOL_ISO_DEP:
> +                iface = NCI_RF_INTERFACE_ISO_DEP;
> +                break;
> +            default:
> +                control_write(param->client, "KO: invalid remote endpoint protocol\n");

Maybe output the value of |param->re->rfproto|. And there has to be a dash betweeen 'remote endpoint'.

@@ +3420,5 @@
> +                control_write(param->client, "KO: invalid remote endpoint protocol\n");
> +                return -1;
> +        }
> +
> +        for (; i < sizeof(nfc->rf)/sizeof(nfc->rf[0]); i++) {

Two things:

 - 'i = 0' seems missing.
 - IIRC qemu has ARRAY_SIZE for sizeof(x)/sizeof(x[0])

@@ +3425,5 @@
> +            if (iface == nfc->rf[i].iface) {
> +                nfc->active_rf =  &nfc->rf[i];
> +                break;
> +            }
> +        }

This whole for loop should be moved into a separate function in nfc.c

struct nfc_rf*
nfc_find_rf_by_rf_interface(struct nfc* nfc, enum nci_rf_interface iface)

@@ +3529,5 @@
> +            p = strsep(&args, " ");
> +            if (!p) {
> +                param.rf = -1;
> +            } else {
> +                param.rf = strtol(p, NULL, 0);

Clear |errno| first.

@@ +3536,5 @@
> +                                  "KO: invalid rf index '%s', error %d(%s)\r\n",
> +                                  p, errno, strerror(errno));
> +                    return -1;
> +                }
> +                if (param.rf < -1 || param.rf > NUMBER_OF_SUPPORT_NCI_RF_INTERFACE - 1) {

I think |param.rf| should depend on ARRAY_SIZE(nfc->rf).

And there are a lot of operators in this term and precedence is only implicitly given. '|| param.rf >= ARRAY_SIZE(nfc->rf)) {' seems easier to read IMHO.

::: hw/nfc-rf.c
@@ +50,2 @@
>  
>      NFC_D("rf->state from %d to %d\n", rf->state, state);

|rf| is gone. Turn on debugging to make this line fail. :)

::: hw/nfc.h
@@ +19,4 @@
>  
>  struct nfc_re;
>  
> +#define NUMBER_OF_SUPPORT_NCI_RF_INTERFACE 2

'NUMBER_OF_SUPPORTED_NCI_RF_INTERFACES' sounds better.

Limiting the use of CPP is considered good style. So this should be an enum if possible.

@@ +32,5 @@
>      enum nfc_fsm_state state;
> +    enum nfc_rfst rf_state;
> +
> +    /* Support Frame and NFC-DEP interface */
> +    struct nfc_rf rf[2];

Should this be 'struct nfc_rf[NUMBER_OF_SUPPORTED_NCI_RF_INTERFACES]' ?
Attachment #8408070 - Flags: review?(tzimmermann) → review-
Attachment #8408070 - Attachment is obsolete: true
Attachment #8408898 - Flags: review?(tzimmermann)
Attachment #8408899 - Flags: review?(tzimmermann)
Comment on attachment 8408898 [details] [diff] [review]
Part1. Move nci rf state from rf to nfc_device

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

Nice. :)
Attachment #8408898 - Flags: review?(tzimmermann) → review+
Comment on attachment 8408899 [details] [diff] [review]
Part2. Support notify tag

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

Just a few nits left to address. Thanks for your patches, Dimi!

::: android/console.c
@@ +3527,5 @@
> +            } else {
> +                errno = 0;
> +                param.rf = strtol(p, NULL, 0);
> +                if (errno) {
> +                    control_write(client, 

whitespace

@@ +3532,5 @@
> +                                  "KO: invalid rf index '%s', error %d(%s)\r\n",
> +                                  p, errno, strerror(errno));
> +                    return -1;
> +		        }
> +		        if (param.rf < -1 || 

whitespace

@@ +3562,4 @@
>      { "ntf", "send NCI notification",
>        "'nfc ntf rf_discover <i> <type>' send RC_DISCOVER_NTF for Remote Endpoint <i> with notification type <type>\r\n"
>        "'nfc ntf rf_intf_activated' send RC_DISCOVER_NTF for selected Remote Endpoint\r\n"
> +      "'nfc ntf rf_intf_activated <i>' send RC_DISCOVER_NTF for Remote Endpoint <i>\r\n"

I know that it's auto-detected, but could you add a line that mentions '-1' as well? I'd prefer test-case implementers to use '-1' explicitly.

::: hw/nfc-nci.c
@@ +373,4 @@
>                                  union nci_packet* rsp)
>  {
>      struct nci_core_init_rsp* payload;
> +    size_t i;

|i| should be of the same type as |payload->nrfs| (i.e., uint8_t).

@@ +585,4 @@
>      const struct nci_rf_discover_select_cmd *payload;
>      struct nfc_re* re;
>      enum nfc_rfst rfst;
> +    size_t i;

|i| is unused, I think.

::: hw/nfc.c
@@ +65,5 @@
> +struct nfc_rf*
> +nfc_find_rf_by_rf_interface(struct nfc_device* nfc, enum nci_rf_interface iface)
> +{
> +    size_t i;
> +    

whitespace

::: hw/nfc.h
@@ +19,4 @@
>  
>  struct nfc_re;
>  
> +enum nfc_interface_count {

'nfc_rf_interface_count'

@@ +19,5 @@
>  
>  struct nfc_re;
>  
> +enum nfc_interface_count {
> +    NUMBER_OF_SUPPORTED_NCI_RF_INTERFACE = 2

'NUMBER_OF_SUPPORTED_NCI_RF_INTERFACES'
Attachment #8408899 - Flags: review?(tzimmermann) → review+
Hi Vicamo,
  Could you help merge the pull request ? Thanks!
Flags: needinfo?(vyang)
Please fix indentation and re-ni me again.
Flags: needinfo?(vyang)
Nits fixed, thanks
Flags: needinfo?(vyang)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.