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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: arno, Assigned: arno)
References
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-github-pull-request
|
Details |
So far only a pair() is done with the remote device during a static NFC handover. Also do a connect() after pairing.
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)
Comment 2•11 years ago
|
||
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)
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)
Comment 4•11 years ago
|
||
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)
Comment 5•11 years ago
|
||
* 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!
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)
Comment 8•11 years ago
|
||
The point (1) mentioned in comment 4 may be related to bug 915602. Add it to the see-also list.
Comment 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 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•11 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•11 years ago
|
||
Rebased to latest b2g/master.
Attachment #8381875 -
Attachment is obsolete: true
Attachment #8382811 -
Flags: review?(ehung)
Comment 13•11 years ago
|
||
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)
Updated•11 years ago
|
blocking-b2g: 1.4? → 1.5?
Flags: needinfo?(whuang)
Comment 14•11 years ago
|
||
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+
Assignee | ||
Comment 15•11 years ago
|
||
I fixed the two little nits. Please land this on bubble-tea.
Attachment #8382811 -
Attachment is obsolete: true
Flags: needinfo?(ehung)
Comment 16•11 years ago
|
||
(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)
Updated•11 years ago
|
Flags: needinfo?(arno)
Updated•11 years ago
|
Blocks: b2g-NFC-2.0
Assignee | ||
Comment 17•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
merged into gaia master.
https://github.com/mozilla-b2g/gaia/commit/fb8d0f5c1a1d299e76bc160bf8ce6e1d4c79516a
Flags: needinfo?(ehung)
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
blocking-b2g: 2.0? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•