Thomas Zimmermann [:tzimmermann] [:tdz] Reporter
	Description • 11 years ago

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.

	Thomas Zimmermann [:tzimmermann] [:tdz] Reporter
	Comment 1 • 11 years ago

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.

	Dimi Lee [:dimi] Assignee
	Comment 2 • 11 years ago

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 ?

	Thomas Zimmermann [:tzimmermann] [:tdz] Reporter
	Comment 3 • 11 years ago

(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.

	Dimi Lee [:dimi] Assignee
	Comment 5 • 11 years ago

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 ?

	Thomas Zimmermann [:tzimmermann] [:tdz] Reporter
	Comment 6 • 11 years ago

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.

	Dimi Lee [:dimi] Assignee
	Comment 8 • 11 years ago

I will also add test case for this

	Thomas Zimmermann [:tzimmermann] [:tdz] Reporter
	Comment 9 • 11 years ago

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