Closed Bug 1142371 Opened 10 years ago Closed 10 years ago

[Bluetooth] Replace getAdapter with service query to make bluetooth transfer work

Categories

(Firefox OS Graveyard :: Gaia::Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

Attachments

(2 files)

Therefore we could align v1/v2 adapter retrieval usage, and can test on v1 to make sure transfer works well.
Comment on attachment 8576427 [details] [gaia] gasolin:issue-1142371 > mozilla-b2g:master The patch make BT transfer v1 also works with event listener. So we can reuse bluetooth_transfer for both v1/v2. Test android -> v1 device transfer works. If feedback is positive I'll fix the unit tests
Attachment #8576427 - Flags: feedback?(iliu)
Comment on attachment 8576427 [details] [gaia] gasolin:issue-1142371 > mozilla-b2g:master Good work in this patch, r+ with me. BTW, please have a manual test for these relative user story before we land the patch. I will also help you to do that for v1/v2. Thanks.
Attachment #8576427 - Flags: feedback?(iliu) → feedback+
Comment on attachment 8576427 [details] [gaia] gasolin:issue-1142371 > mozilla-b2g:master Test android -> v1, android -> v2, v1 -> v2 works. The v2 -> v1 does not work yet due to the bluetooth app adapter is not ready yet. The v1 gecko interface does not define adapter removed case, therefore the bluetooth adapter unavailable state is not dispatched in bluetooth v1. https://dxr.mozilla.org/mozilla-central/source/dom/webidl/BluetoothManager.webidl
Attachment #8576427 - Flags: review?(iliu)
Comment on attachment 8576427 [details] [gaia] gasolin:issue-1142371 > mozilla-b2g:master Basically, the patch is fine to me. Let's wait Gecko devs to response that we can use 'ondisabled' event to fire 'bluetooth-unavailable'. And do you have a manual test for NFC file transfer? I think that is also required some manual test. Please check my comment on GitHub. Thanks.
Attachment #8576427 - Flags: review?(iliu)
Comment on attachment 8576427 [details] [gaia] gasolin:issue-1142371 > mozilla-b2g:master Alive, could you please help to review 'bluetooth_core'? Thanks.
Attachment #8576427 - Flags: review?(alive)
For use 'ondisabled' event to fire 'bluetooth-unavailable', I've append a new commit and testing it now. NFC file transfer part is currently disabled in v2. Would be solved by a followup patch in bug 1090799.
(In reply to Fred Lin [:gasolin] from comment #7) > For use 'ondisabled' event to fire 'bluetooth-unavailable', I've append a > new commit and testing it now. > > NFC file transfer part is currently disabled in v2. Would be solved by a > followup patch in bug 1090799. Okay, does the patch work for NFC file transfer in v1?
Comment on attachment 8576427 [details] [gaia] gasolin:issue-1142371 > mozilla-b2g:master The problem here is you need a guaranteed launching order between Bluetooth and BluetoothTransfer. If BluetoothTransfer is inited after Bluetooth, you will never get the event. IMO BluetoothTransfer should act like the submodule of Bluetooth; it could just access this.parent.adapter when it is started. One another problem is NfcHandoverManager is fetching bluetooth adapter too, what is the plan to merge it in Bluetooth?
Attachment #8576427 - Flags: review?(alive)
@alive I understand the concern, but it will be hard to go with BaseModule and keep BTv1/BTv2 share the same BluetoothTransfer file without modify too much of BTv1 structure. The current plan for handling NfcHandoverManager part is listen adapter-avaialbe event in it, and change the BluetoothCore caller after NfcCore https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/core.js#L39 to make sure Bluetooth is called after NfcHandoverManager.
Blocks: 1090799
@Ian since NfcHandoverManager now only use service query to access BluetoothTransfer property, the change should not affect NFC transfer for v1. Currently NFC function is down due to bug 1143528 / bug 1120849 , I'll add this manual test case once NFC is ready.
@alive FYR WIP NfcHandoverManager with adapter https://github.com/gasolin/gaia/commit/b9fb8396d22033540b812a0039d4c21ccc2ce5c4 , it takes same approach to listen adapter-avaialbe/unavailable event in it, would go for bug 1090799
@alive with above concerns, do you think its an acceptable approach?
Flags: needinfo?(alive)
As offline discussion with alive, I'll load BluetoothTransfer.start inside of Bluetooth, and use service query to get adapter instead.
Flags: needinfo?(alive)
Comment on attachment 8580476 [details] [gaia] gasolin:issue-1142371-2 > mozilla-b2g:master the patch load BluetoothTransfer.start inside of Bluetooth, and use service query to get adapter instead. Test ok for v1/v2 receiving, v1 transferring. To expose bluetooth state to system, I still keep 'bluetooth-available/bluetooth-unavailable' event on this patch.
Attachment #8580476 - Flags: feedback?(iliu)
Attachment #8580476 - Flags: feedback?(alive)
Comment on attachment 8580476 [details] [gaia] gasolin:issue-1142371-2 > mozilla-b2g:master LGTM
Attachment #8580476 - Flags: feedback?(alive) → feedback+
Summary: [Bluetooth] Replace getAdapter with bluetooth-available event listener → [Bluetooth] Replace getAdapter with service query to make bluetooth transfer work
Comment on attachment 8580476 [details] [gaia] gasolin:issue-1142371-2 > mozilla-b2g:master test fixed, set for review
Attachment #8580476 - Flags: review?(iliu)
Attachment #8580476 - Flags: review?(alive)
Attachment #8580476 - Flags: feedback?(iliu)
Attachment #8580476 - Flags: review?(alive) → review+
Respond to Comment 8 from @ian, Test NFC image transfer with nexus 4 <-> nexu 5 ok (with BTv1)
Comment on attachment 8580476 [details] [gaia] gasolin:issue-1142371-2 > mozilla-b2g:master Small nits in this path addressed on GitHub. And shall we still dispatch 'bluetooth-available' event since we move 'getAdapter' to service query?
Attachment #8580476 - Flags: review?(iliu)
(In reply to Fred Lin [:gasolin] from comment #19) > Respond to Comment 8 from @ian, Test NFC image transfer with nexus 4 <-> > nexu 5 ok (with BTv1) Dose it mean that Flame is not working now for NFC files transfer via Bluetooth? Is there any other blocking issue?
It means I just not have spare flame for this test :p I'll find another flame for that test. And it seems fine to remove the 'bluetooth-available' event.
Comment on attachment 8580476 [details] [gaia] gasolin:issue-1142371-2 > mozilla-b2g:master remove bluetooth-available/unavailable event for BTv1/BTv2 test transfer ok between btv1/btv2, BTv1 nfc transfer ok (still test on nexus 4/5), as mentioned the BTv2 nfc transfer will be a followup work on bug 1090799
Attachment #8580476 - Flags: review?(iliu)
Comment on attachment 8580476 [details] [gaia] gasolin:issue-1142371-2 > mozilla-b2g:master The patch looks good. Only one nit addressed on GitHub.
Attachment #8580476 - Flags: review?(iliu)
Comment on attachment 8580476 [details] [gaia] gasolin:issue-1142371-2 > mozilla-b2g:master replace bind(this) with arrow functions, treeherder green, v1/v2 test ok with no error log, please kindly review it again :)
Attachment #8580476 - Flags: review?(iliu)
Comment on attachment 8580476 [details] [gaia] gasolin:issue-1142371-2 > mozilla-b2g:master The patch is also working for me. And I also pull patch of Bug 1121909(https://github.com/mozilla-b2g/gaia/pull/28971). Bluetooth send file(s) flow is working good as before. Thanks for Fred's effort here. BTW, I find out an unexpected behavior in the new code base. While turning the connected headset on/off, then it will re-connect with 'hfp' profile only. We have to file a bug for tracking why the 'a2dp' profile won't be re-connected as v1 Gaia/Gecko code base.
Attachment #8580476 - Flags: review?(iliu) → review+
Thanks! Open bug 1146818 to track above issue happened in APIv2.
Keywords: checkin-needed
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: