993330 - [NFC] support notify tag discover by emulator console command

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Dimi Lee [:dimi]]

Use emulator NFC console command "nfc ntf rf_intf_activated <i>" should be able to notify tag information. But not working currently.

Comment 1

[Dimi Lee [:dimi]]

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

Comment 2

[Dimi Lee [:dimi]]

Attachment: Patch v1

Comment 3

[Dimi Lee [:dimi]]

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>

Comment 4

[Dimi Lee [:dimi]]

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 5

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

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]' ?

Comment 6

[Dimi Lee [:dimi]]

Attachment: Part1. Move nci rf state from rf to nfc_device

Comment 7

[Dimi Lee [:dimi]]

Attachment: Part2. Support notify tag

Comment 8

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

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. :)

Comment 9

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

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'

Comment 10

[Dimi Lee [:dimi]]

Attachment: pull request for mozilla-b2g/platform_external_qemu

Hi Vicamo,
  Could you help merge the pull request ? Thanks!

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Please fix indentation and re-ni me again.

Comment 12

[Dimi Lee [:dimi]]

Nits fixed, thanks

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

https://github.com/mozilla-b2g/platform_external_qemu/commit/bb11a0417efa7e6a08ed1cb2d5d6a13a3100abd3
https://hg.mozilla.org/integration/b2g-inbound/rev/b7fcecdd8de2