Closed Bug 967345 Opened 11 years ago Closed 11 years ago

Do a BT connect() for a static handover

Categories

(Firefox OS Graveyard :: NFC, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: arno, Assigned: arno)

References

Details

Attachments

(2 files, 5 obsolete files)

So far only a pair() is done with the remote device during a static NFC handover. Also do a connect() after pairing.
Blocks: NFC-Gaia
Attached patch do-connect-after-pairing.patch (obsolete) (deleted) — Splinter Review
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)
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: nobody → arno
Attached file BT shows device as connected although it is not (obsolete) (deleted) —
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)
Attached patch Workaround-for-NFC (deleted) — Splinter Review
* Gecko patch for static handover
(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!
Attached file Bug 967345 - Do a connect() after pair() #16643 (obsolete) (deleted) —
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.
Comment on attachment 8381875 [details] 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)
(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)
(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)
Attached file Bug 967345 - Do a connect() after pair() #16643 (obsolete) (deleted) —
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 on attachment 8382811 [details] 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+
Attached file Bug 967345 - Do a connect() after pair() r=evelyn (obsolete) (deleted) —
I fixed the two little nits. Please land this on bubble-tea.
Attachment #8382811 - Attachment is obsolete: true
Flags: needinfo?(ehung)
(In reply to arno from comment #15) > Created attachment 8385617 [details] > 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)
Flags: needinfo?(arno)
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)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
blocking-b2g: 2.0? → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: