Closed Bug 984397 Opened 6 years ago Closed 6 years ago

[NFC] Fix rf_discover in NFC emulator

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: tzimmermann, Assigned: dimi)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file, 4 obsolete files)

The emulator's console command for RF_DISCOVER_NTF was implemented initially, but broke since. Currently you'll get something like

> emulator64-arm: external/qemu/hw/nfc-nci.c:975: nfc_create_rf_discovery_ntf: Assertion `rfst != NUMBER_OF_NFC_RFSTS' failed.
> ./run-emulator.sh: Zeile 52: 24459 Abgebrochen             ${DBG_CMD} $EMULATOR -kernel $KERNEL -sysdir $B2G_HOME/out/target/product/$DEVICE/ -data $B2G_HOME/out/target/product/$DEVICE/userdata.img -sdcard ${SDCARD_IMG} -memory 512 -partition-size 512 -skindir $B2G_HOME/development/tools/emulator/skins -skin HVGA -verbose -gpu on -camera-back webcam0 -qemu $TAIL_ARGS

This should be fixed and the emulator should be able to transition from 'idle' over multiple rf_discover and an rf_intf_activated notification into 'poll_active' RF state. A test case can be build on top of test_nfc_manager_tech_discovered.js.
Blocks: b2g-NFC-2.0
Assignee: nobody → dlee
Blocks: 986527
No longer blocks: b2g-NFC-2.0
Just a few notes.

If you look into the NCI spec, sec 5.2, there is a diagram of possible states of the NCI's RF device. When NFC gets switched on the state moves from RFST_IDLE to RFST_DISCOVERY. At this point we should be able to send RF_DISCOVERY_NTF via the emulator's console to transition into RFST_W4_ALL_DISCOVERIES, and from there to RFST_W4_HOST_SELECT and RFST_POLL_ACTIVE.

The emulator maintains these states and checks for invalid transitions between them. The error message in the bug report indicates that the transition is incorrect or that the test itself is broken. It used to work, but probably broke at some point.
Hi Thomas,
  I was trying to enter RFST_POLL_ACTIVE state by following console commands

nfc ntf rf_discover 1
OK
nfc ntf rf_discover 1
OK
nfc ntf rf_intf_activated 1

and then i will get 
emulator64-arm: external/qemu/hw/nfc-nci.c:1056: nfc_create_rf_intf_activated_ntf: Assertion `rfst != NUMBER_OF_NFC_RFSTS' failed.

Is this the case you mentioned in the bug ?
Flags: needinfo?(tzimmermann)
(In reply to Dimi Lee[:dimi][:dlee] from comment #2)
> Hi Thomas,
>   I was trying to enter RFST_POLL_ACTIVE state by following console commands
> 
> nfc ntf rf_discover 1
> OK
> nfc ntf rf_discover 1
> OK

You don't have to discover the same RE twice.

> nfc ntf rf_intf_activated 1
> 
> and then i will get 
> emulator64-arm: external/qemu/hw/nfc-nci.c:1056:
> nfc_create_rf_intf_activated_ntf: Assertion `rfst != NUMBER_OF_NFC_RFSTS'
> failed.
> 
> Is this the case you mentioned in the bug ?

Yes. This is a check to verify that you're following one of the state transitions that are given in the NCI spec. If it fails, either the transition or the state is wrong.
Flags: needinfo?(tzimmermann)
Attached patch WIP Patch v1 (obsolete) — Splinter Review
Comment on attachment 8403153 [details] [diff] [review]
WIP Patch v1

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

Hi Thomas,
  I add an additional parameter for rf_discover in this proposed patch, the usage will like 
nfc ntf rf_discover <i> <notification type>, please help take a look at it.

  And also i would like to discuss about function nfc_create_rf_discovery_ntf in nfc-nci.c
currently I use following check so state will stay in W4_ALL_DISCOVERIES whenever notification type is NCI_MORE_NOTIFICATIONS
if (re->id && param->ntype == NCI_MORE_NOTIFICATIONS) {
  return 0; /* RE already discovered */
}
  But according to NCI SPEC page79 "NFCC SHALL send RF_DISCOVER_NTF to the DH for each combination of Remote NFC Endpoint 
and RF Protocol detected during the RF Discovery Process."
  I think maybe we should change the check here in the future so we can send multiple notification if Remote End Point support
multiple RF protocol, how do you think ?
Attachment #8403153 - Flags: feedback?(tzimmermann)
Comment on attachment 8403153 [details] [diff] [review]
WIP Patch v1

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

Hi Dimi,

Thanks for taking care of this. You need to rebase your patch onto the latest version. I can't actually test it right now. For testing, I'd suggest you add an new RE to |nfc_res| in hw/nfc-re.c. Then you can try DISCOVER->W4_ALL->W4_ALL->W4_HOST_SELECT. And it would be cool if you could write a Marionette test.

::: android/console.c
@@ +3208,4 @@
>  
>  static size_t
>  nfc_rf_discovery_ntf_cb(void* data,
> +                        void* param,

Once you rebased onto the lastest ref, |data| will be a structure of type |struct nfc_ntf_param|, which you can extend by the type value. No need for an extra parameter here and everywhere below.

@@ +3257,5 @@
>  
> +        char* q = strsep(&args, " ");
> +
> +        if (!q) {
> +            return -1;

This is a general bug in the old ref you have. Returning '-1' will not signal failure to the emulator. You also need to print a message that starts with "KO: ".

@@ +3260,5 @@
> +        if (!q) {
> +            return -1;
> +        }
> +
> +        size_t j = strtoul(q, NULL, 0);

|j| should be of type 'unsigned long'.

@@ +3270,5 @@
> +
> +        struct nci_rf_discover_ntf_param param;
> +        param.ntype = j;
> +
> +        goldfish_nfc_send_ntf(nfc_rf_discovery_ntf_cb, nfc_res+i, &param);

Way too much empty lines! :)

::: hw/nfc-nci.c
@@ +929,4 @@
>  
>  size_t
>  nfc_create_rf_discovery_ntf(struct nfc_re* re,
> +                            struct nci_rf_discover_ntf_param* param,

Once you have the type, you'll also need to modify the switch statement near line 957. In RFST_DISCOVERY, we should assert that we don't have type #2. In RFST_W4_ALL_DISCOVERIES, the next state of our state machine depends on the notification type. It's easier for me to just write down the code :)

    switch (nfc->rf[0].state) {
        case NFC_RFST_DISCOVERY:
            assert(ntype == NCI_MORE_NOTIFICATIONS);
            payload->end[payload->nparams] = ntype;
            bits = NFC_RFST_DISCOVERY_BIT;
            rfst = NFC_RFST_W4_ALL_DISCOVERIES;
            break;
        case NFC_RFST_W4_ALL_DISCOVERIES:
            payload->end[payload->nparams] = ntype;
            bits = NFC_RFST_W4_ALL_DISCOVERIES_BIT;
            if (ntype == NCI_MORE_NOTIFICATIONS) {
              rfst = NFC_RFST_W4_ALL_DISCOVERIES;
            } else {
              rfst = NFC_RFST_W4_HOST_SELECT;
            }
            break;
        default:
            bits = 0;
            rfst = 0;
            break;
    }

@@ +938,4 @@
>      enum nfc_rfst rfst;
>  
>      assert(re);
> +    assert(param);

I suggest to add NUMBER_OF_NCI_NOTIFICATION_TYPES at the end of 'enum nci_notification_type' and assert that the supplied type is smaller.

@@ +942,2 @@
>  
> +    if (re->id && param->ntype == NCI_MORE_NOTIFICATIONS) {

Not necessary AFAICT.

::: hw/nfc-nci.h
@@ +530,3 @@
>  
>  size_t
>  nfc_create_rf_discovery_ntf(struct nfc_re* re,

These parameter types are a detail of the console's implementation and should be located in android/console.c only. Just pass an  'enum nci_notification_type' here.
Attachment #8403153 - Flags: feedback?(tzimmermann) → feedback-
Target Milestone: --- → 1.4 S6 (25apr)
Whiteboard: [p=1]
Attachment #8403153 - Attachment is obsolete: true
Attachment #8405229 - Flags: review?(tzimmermann)
I will also add test case for this
Comment on attachment 8405229 [details] [diff] [review]
Patch v1 - Add notification type parameter

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

Hi Dimi,,

r+ with the nits below. Thanks a lot.

::: android/console.c
@@ +3356,4 @@
>  struct nfc_ntf_param {
>      ControlClient client;
>      struct nfc_re* re;
> +    uint8_t discover_ntf_type;

Personally I prefer shorted names for fields, say |ntype|.

@@ +3443,5 @@
>              return -1;
>          }
> +
> +        /* read discover notification type */
> +        q = strsep(&args, " ");

No need for a new variable. (?) Please reuse |p| here.

@@ +3449,5 @@
> +            control_write(client, "KO: no discover notification type given\r\n");
> +            return -1;
> +        }
> +        errno = 0;
> +        j = strtoul(q, NULL, 0);

I think it's ok to write this value directly into param's field. The type of the value should be unsigned long.

struct nfc_ntf_param{
  unsigned long ntype;
}
param.ntype = strtoul(...)

::: hw/nfc-nci.c
@@ +937,4 @@
>      enum nfc_rfst rfst;
>  
>      assert(re);
> +    assert(type != NUMBER_OF_NCI_NOTIFICATION_TYPES);

type < NUMBER_OF_NCI_NOTIFICATION_TYPES
Attachment #8405229 - Flags: review?(tzimmermann) → review+
Attached patch Fix patch for rf_discover (obsolete) — Splinter Review
Attachment #8405229 - Attachment is obsolete: true
blocking-b2g: --- → backlog
Attachment #8405291 - Attachment is obsolete: true
Attachment #8406656 - Attachment is obsolete: true
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.