All users were logged out of Bugzilla on October 13th, 2018

Do a BT connect() for a static handover

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: arno, Assigned: arno)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Blocks: 933640
(Assignee)

Comment 1

5 years ago
Created attachment 8369829 [details] [diff] [review]
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.
Flags: needinfo?(echou)
(Assignee)

Updated

5 years ago
blocking-b2g: --- → 1.4?
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.
Flags: needinfo?(echou)
(Assignee)

Updated

5 years ago
Assignee: nobody → arno
(Assignee)

Comment 3

5 years ago
Created attachment 8379939 [details]
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).
Attachment #8369829 - Attachment is obsolete: true
Flags: needinfo?(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.
Flags: needinfo?(echou)
Created attachment 8381333 [details] [diff] [review]
Workaround-for-NFC

* Gecko patch for static handover
(Assignee)

Comment 6

5 years ago
(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!
(Assignee)

Comment 7

5 years ago
Created attachment 8381875 [details] [review]
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.
Attachment #8379939 - Attachment is obsolete: true
Attachment #8381875 - Flags: review?(ehung)
The point (1) mentioned in comment 4 may be related to bug 915602. Add it to the see-also list.
See Also: → bug 915602

Comment 9

5 years ago
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.
Attachment #8381875 - Flags: review?(ehung)
(Assignee)

Comment 10

5 years ago
(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.
Flags: needinfo?(ehung)

Comment 11

5 years ago
(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!
Flags: needinfo?(ehung)
(Assignee)

Comment 12

5 years ago
Created attachment 8382811 [details] [review]
Bug 967345 - Do a connect() after pair() #16643

Rebased to latest b2g/master.
Attachment #8381875 - Attachment is obsolete: true
Attachment #8382811 - Flags: review?(ehung)
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.
Flags: needinfo?(whuang)
blocking-b2g: 1.4? → 1.5?
Flags: needinfo?(whuang)

Comment 14

5 years ago
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
Attachment #8382811 - Flags: review?(ehung) → review+
(Assignee)

Comment 15

5 years ago
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.
Attachment #8382811 - Attachment is obsolete: true
Flags: needinfo?(ehung)

Comment 16

5 years ago
(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!
Flags: needinfo?(ehung)

Updated

5 years ago
Flags: needinfo?(arno)
(Assignee)

Comment 17

5 years ago
Created attachment 8392367 [details] [review]
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.
Attachment #8385617 - Attachment is obsolete: true
Flags: needinfo?(arno) → needinfo?(ehung)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
blocking-b2g: 2.0? → ---
You need to log in before you can comment on or make changes to this bug.