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)
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 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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.
Comment 8•10 years ago
|
||
(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 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
@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.
Assignee | ||
Comment 11•10 years ago
|
||
@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.
Assignee | ||
Comment 12•10 years ago
|
||
@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
Assignee | ||
Comment 13•10 years ago
|
||
@alive with above concerns, do you think its an acceptable approach?
Flags: needinfo?(alive)
Assignee | ||
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
Comment on attachment 8580476 [details]
[gaia] gasolin:issue-1142371-2 > mozilla-b2g:master
LGTM
Attachment #8580476 -
Flags: feedback?(alive) → feedback+
Assignee | ||
Updated•10 years ago
|
Summary: [Bluetooth] Replace getAdapter with bluetooth-available event listener → [Bluetooth] Replace getAdapter with service query to make bluetooth transfer work
Assignee | ||
Comment 18•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8580476 -
Flags: review?(alive) → review+
Assignee | ||
Comment 19•10 years ago
|
||
Respond to Comment 8 from @ian, Test NFC image transfer with nexus 4 <-> nexu 5 ok (with BTv1)
Comment 20•10 years ago
|
||
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)
Comment 21•10 years ago
|
||
(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?
Assignee | ||
Comment 22•10 years ago
|
||
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.
Assignee | ||
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
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)
Assignee | ||
Comment 25•10 years ago
|
||
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 26•10 years ago
|
||
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+
Assignee | ||
Comment 27•10 years ago
|
||
Thanks!
Open bug 1146818 to track above issue happened in APIv2.
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
Comment 28•10 years ago
|
||
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.
Assignee | ||
Comment 29•10 years ago
|
||
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.
Description
•