967345 - Do a BT connect() for a static handover

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[arno]

So far only a pair() is done with the remote device during a static NFC handover. Also do a connect() after pairing.

Comment 1

[arno]

Attachment: do-connect-after-pairing.patch

This patch adds a call to connect() after pair() that is done in response to a static handover via NFC. I am using the Motorola UE Mini Boom speakers for testing. Connecting is not reliable and unfortunately I cannot work out a pattern. I have noticed that sometimes I have also troubles connecting via the Settings app, so perhaps the problem is unrelated to the NFC static handover. Either way, please take a look at this patch if you can spot an obvious problem with it.

Comment 2

[Eric Chou [:ericchou] [:echou]]

The patch looks good to me (I mean the logic related to Bluetooth, not javascript syntax or coding style). Arno, you said that "Connecting is not reliable" even via Settings app, could you attach the logcat to the bug? It would be great if you could record a short video and paste the link. Build info is also necessary for debugging. Thanks.

Comment 3

[arno]

Attachment: BT shows device as connected although it is not

Eric: doing a connect() after pair() is still not reliable. I have uploaded the logcat to pastebin. Here is the scenario: external device is not paired. BT is turned on. Hold phone next to external device that triggers the static handover. Pairing succeeds (line 898 in logcat). Then I enumerate all BluetoothDevices in order to do a connect(). However, once I find the right BluetoothDevice, it claims to be already connected (line 903 in logcat). Because of this, I don't call connect(). However, if you then go into BT Settings, it shows the external device as paired but not connected. I also removed the check from my code that tests if the device is already connected (since it shouldn't be anyways right after pairing). But that also does not work reliably (by that I mean that sometimes it does work but most of the time it does not).

Comment 4

[Eric Chou [:ericchou] [:echou]]

Hi Arno,

I've tested your patch with the latest codebase, and I could reproduce the problem you met. I have several suggestions and hope the successful rate can raise after taking these:

1. Delay 2 seconds between pairing succeeds and calling doConnect()

     req.onsuccess = function() {
       self.debug('Pairing succeeded');
-      self.doConnect(mac);
+      setTimeout(function test() {
+        self.doConnect(mac);
+      }, 2000);
     };

  This is a workaround for a Gecko bug(I haven't filed yet).

2. Do not check device.connected before doConnect()

  device.connected represents if the device has an ACL connection
  with local BluetoothAdapter. ACL is a low-level connection of
  the whole Bluetooth protocol stack. An ACL has to be established
  to make pairing work. The 'connected' you would like to know should
  be the one representing a profile-layer(HFP) connection.

3. Apply my patch to your Gecko codebase

  Will attach later.

I've tried with these revisions on my Nexus 4 and it can work well.

Comment 5

[Eric Chou [:ericchou] [:echou]]

Attachment: Workaround-for-NFC

* Gecko patch for static handover

Comment 6

[arno]

(In reply to Eric Chou [:ericchou] [:echou] from comment #4)
> I've tested your patch with the latest codebase, and I could reproduce the
> problem you met. I have several suggestions and hope the successful rate can
> raise after taking these: [...]

thanks, Eric!

Comment 7

[arno]

Attachment: Bug 967345 - Do a connect() after pair() #16643

Evelyn: the pull request does *not* contain the 2 second delay that Eric suggested in comment #4. I assume he will create a permanent fix for the gecko problem he mentioned.

Comment 8

[Eric Chou [:ericchou] [:echou]]

The point (1) mentioned in comment 4 may be related to bug 915602. Add it to the see-also list.

Comment 9

[Evelyn Hung]

Comment on attachment 8381875 [details] [review]
Bug 967345 - Do a connect() after pair() #16643

The patch is okay but I feel sad when I'm tracing the code to see who will call doPairing(). I found there are two files which are almost the same: nfc_handover_manager.js and nfc_handover.js. Digging more from commit logs, it shows nfc_handover.js is dead code - nobody uses it now. So I think it's better to do some clean up as you've committed in bug 933093 comment 60. I file a bug 977025 for you. I hope you can fix the problem there first and rebase your patch here. If you think it's not a good idea, let me know. Thanks.

Comment 10

[arno]

(In reply to Evelyn Hung [:evelyn] from comment #9)
> [...] I found there are two files which are almost the same:
> nfc_handover_manager.js and nfc_handover.js. Digging more from commit logs,
> it shows nfc_handover.js is dead code - nobody uses it now.

You are right, nfc_handover.js is dead code; it is not referenced from anywhere. There is Bug 963556 that does some (much needed) cleanup of the NFC subsystem which also includes removal of nfc_handover.js. Do you prefer to have that unneeded file removed separately in bug 977025? Thanks.

Comment 11

[Evelyn Hung]

(In reply to arno from comment #10)
> You are right, nfc_handover.js is dead code; it is not referenced from
> anywhere. There is Bug 963556 that does some (much needed) cleanup of the
> NFC subsystem which also includes removal of nfc_handover.js. Do you prefer
> to have that unneeded file removed separately in bug 977025? Thanks.

Bug 963556 is good, I just close bug 977025 as a dup. Thanks!

Comment 12

[arno]

Attachment: Bug 967345 - Do a connect() after pair() #16643

Rebased to latest b2g/master.

Comment 13

[Preeti Raghunath(:Preeti)]

Wesley,

Please verify if this is needed in 1.4. Is this feature needed for QC? NFC will be disabled in 1.4 so wondering if this can also move to 1.5.

Comment 14

[Evelyn Hung]

Comment on attachment 8382811 [details] [review]
Bug 967345 - Do a connect() after pair() #16643

r+ with comment addressed. Since we can't land this patch to gaia master due to some project decision, would you like to land it to a temporary gaia branch "bubble-tea" for system and settings apps?

[1] https://groups.google.com/forum/#!search/Temporary$20bubble-tea$20branch/mozilla.dev.gaia/t-yUkIMcYsA/kFgIAEO8XKkJ
[2] https://wiki.mozilla.org/Gaia/Team/Taipei/BubbleTea

Comment 15

[arno]

Attachment: Bug 967345 - Do a connect() after pair() r=evelyn

I fixed the two little nits. Please land this on bubble-tea.

Comment 16

[Evelyn Hung]

(In reply to arno from comment #15)
> Created attachment 8385617 [details] [review]
> Bug 967345 - Do a connect() after pair() r=evelyn
> 
> I fixed the two little nits. Please land this on bubble-tea.

So please redirect the PR to https://github.com/mozilla-b2g/gaia/tree/bubble-tea, so I can land it to bubble-tea. Thanks!

Comment 17

[arno]

Attachment: Bug 967345 - Do a connect() after pair() r=evelyn

The bubble-tea branch didn't work well for me. I've opted to wait until the 3/17 deadline for this patch. I rebased to current master.

Comment 18

[Evelyn Hung]

merged into gaia master.
https://github.com/mozilla-b2g/gaia/commit/fb8d0f5c1a1d299e76bc160bf8ce6e1d4c79516a