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

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gasolin, Assigned: gasolin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
Therefore we could align v1/v2 adapter retrieval usage, and can test on v1 to make sure transfer works well.
Created attachment 8576427 [details] [review]
[gaia] gasolin:issue-1142371 > mozilla-b2g:master
(Assignee)

Comment 2

4 years ago
Comment on attachment 8576427 [details] [review]
[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] [review]
[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

4 years ago
Comment on attachment 8576427 [details] [review]
[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] [review]
[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] [review]
[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

4 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.
(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] [review]
[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

4 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)

Updated

4 years ago
Blocks: 1090799
(Assignee)

Comment 11

4 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

4 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

4 years ago
@alive with above concerns, do you think its an acceptable approach?
Flags: needinfo?(alive)
(Assignee)

Comment 14

4 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)
Created attachment 8580476 [details] [review]
[gaia] gasolin:issue-1142371-2 > mozilla-b2g:master
(Assignee)

Comment 16

4 years ago
Comment on attachment 8580476 [details] [review]
[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] [review]
[gaia] gasolin:issue-1142371-2 > mozilla-b2g:master

LGTM
Attachment #8580476 - Flags: feedback?(alive) → feedback+
(Assignee)

Updated

4 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

4 years ago
Comment on attachment 8580476 [details] [review]
[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+
(Assignee)

Comment 19

4 years ago
Respond to Comment 8 from @ian, Test NFC image transfer with nexus 4 <-> nexu 5 ok (with BTv1)
Comment on attachment 8580476 [details] [review]
[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?
(Assignee)

Comment 22

4 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

4 years ago
Comment on attachment 8580476 [details] [review]
[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] [review]
[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

4 years ago
Comment on attachment 8580476 [details] [review]
[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] [review]
[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

4 years ago
Thanks!

Open bug 1146818 to track above issue happened in APIv2.
Keywords: checkin-needed

Updated

4 years ago
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.
(Assignee)

Comment 29

4 years ago
merged https://github.com/mozilla-b2g/gaia/commit/56758ff00e418aba404fee3cf961586b1241bc74
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.