Closed Bug 1090799 Opened 7 years ago Closed 7 years ago

make nfc_handover v2 and reuse system bluetooth adapter

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

(4 files)

Since we are transition to bluetooth v2 API, we'd better make all API call through system/js/bluetooth so it could be replaced easier.
Hi Ian, the goal of this patch is to make nfc_handover reuse system bluetooth adapter. 

Since in v2 API the bluetooth.enabled is removed, and BT does not depends on settings('bluetooth.enabled') key, we'd better try to decouple them in system modules.

To achieve that, I use CustomEvent 'bluetooth-adapter-added' to denote enable state, 'bluetooth-disabled' to denote disable state to replace mozBluetooth.enable stat, then wrap settings.set to system/bluetooth's enable/disable method, so the other system module could wrap settings.set call via system/bluetooth module in the future patches.
Attachment #8514048 - Flags: review?(iliu)
Comment on attachment 8514048 [details] [review]
pull request redirect to github

Looks good for me with some comment addressed on GitHub. Before we land the patch, I think both of us have to do some manual test here.:)
Also cc Arno for the Bluetooth refactor work.
Depends on: 1090761
rename to implement BTv2 version in this bug. The previous patch is obsoleted.

according to the plan described in bug 1089511 Comment 8
Status: ASSIGNED → NEW
Summary: make nfc_handover reuse system bluetooth adapter → make nfc_handover v2 and reuse system bluetooth adapter
Blocks: 1093084
No longer blocks: 1093084
Depends on: 1128812, 1122327, 1093084
Depends on: 1143528, 1120849, 1142371
found nfc_handover_manager test use the wrong Bluetooth mock. mockBluetooth is used for mock bluetooth.js, but not mozbluetooth
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

The goal is make nfc handover manager reuse existing Bluetooth adapter via service.query. Currently test on BTv1 device and works with BT default on/off. (BTv2 has issue with nfc from gecko side)

We met a problem that BTv1 trigger bluetooth enabled event before bluetooth adapter-added event, therefore there will be a time gap between the BT enabled state and the timing that actually can get the BT adapter. I try to wrap the difference via promise interface.

If the current implementation looks fine, I'll keep adding the rest of unit tests before review.
Attachment #8582925 - Flags: feedback?(iliu)
Attachment #8582925 - Flags: feedback?(alive)
In part of BTv2, I leave some comment on GitHub. Since the return type of API pair(), getPairedDevices() is changed in BTv2, the patch won't be worked as before. Please check the feasibility with Gecko Bluetooth devs.

* If these API is not compatible for DOM request, please use version detector for separating them.
* If there are many version detector in 'nfc_handover_manager.js', we might to considerate have another script to support BTv2.

In part of BTv1, there is a setTimeout in _retrieveAdapter(). I don't think that is a good way to reach adapter. I considerate to have promise interface in Service.query module. But it will need Alive's suggestion here.
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

As above comment.
Attachment #8582925 - Flags: feedback?(iliu)
Thanks for feedback, I've wrap getPairedDevices() to promise interface to abstract API difference. So v2 supposedly work now (as commet 9, BTv2 has issue with nfc from gecko side, so related function is not testable)

Currently there are 3 places (_retrieveAdapter() for v1, _doPairing(), _getPairedDevices() for v1/v2) need version detection. Consider to 800+ lines of code, I feel it still worth to keep in the same script.
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

Need to use async service request to avoid race condition
Attachment #8582925 - Flags: feedback?(alive)
Let's not forget to ask :tauzen to feedback in next review turn.
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

I add the waitForAdapter method in Bluetooth.js V1, which use object.watch to check if the defaultAdapter is available. So we don't have to count on setTimeout and avoid racing issue.

test fine on v1 device.
Attachment #8582925 - Flags: feedback?(kmioduszewski)
Attachment #8582925 - Flags: feedback?(iliu)
Attachment #8582925 - Flags: feedback?(alive)
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

This looks good to me. Great that we're finally switching to arrow functions here. 

|this._debug| and |this._logVisibly| are actually not needed and debug from BaseModule could be used here (same changes suggested by Alive were made in NfcManager). This patch is quite big so if you don't want to deal with this here we could rise a follow up bug for that. 

Since Arno is the original author and was recently working on NfcHandoverManager I think it's valuable to ask for his opinion.
Attachment #8582925 - Flags: feedback?(kmioduszewski)
Attachment #8582925 - Flags: feedback?(arno)
Attachment #8582925 - Flags: feedback+
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

Fred, Per comment 13, we have to use |Service.register('getAdapter', this);| for async query. We cannot setTimeout in a promise method to decide when we resolve/reject it. 

In part of nfc_handover_manager, we can use |this.service.request('getAdapter', 'arg').then(function() {// TODO something.. });|. I think it could be a solution between NFC and Bluetooth modules.

Be careful the interface of Service module as following:

* register: function(service, server) {
* registerState: function(state, provider) { (Should be given with a state, not a async service or something..)


Alive, Please correct me, If I misunderstand. Thanks.
Attachment #8582925 - Flags: feedback?(iliu)
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

See Ian's comment
Attachment #8582925 - Flags: feedback?(alive)
Status: NEW → ASSIGNED
Depends on: 1153721
Since bug 1153721 is resolved. Have quick test on BTv2 devices. 
The transfer between v1/v2 works with this patch.
Hi Fred,
Thank you for providing patches to support BT API v2 for NFC handover manager.

I'd like to remind you that gaia patch of Bug 1146744 has been landed to m-c.
The patch is based on BT API v1 and it uses 'BluetoothDevice.connected' to determine whether to re-connect device again. 
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_handover_manager.js#L643

You may have noticed the attribute 'BluetoothDevice.connected' isn't supported by BT API v2 any more. However, you can use 'BluetoothAdapter.getConnectedDevices()' to achieve similar functionality.
See Also: → 1146744
@jamin thanks for notice, I'll fix that in PR as well.
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

ian & alive,

Current patch use service.request to get bluetooth adapter asynchronously, local test works on v1/v2 devices.

The main differences are:

* cache adapter when bluetooth enabled and disabled
* abstract API difference between BTv1/v2 API
  * wrap the difference interface of adapter.getPairedDevices API
  * wrap the difference callback of adapter.pair API
  * use adapter.getConnectedDevices API instead of device.connected

The patch also fix several issues in nfcHandoverManager:

* fix test missusing mock_bluetooth.js instead of mock_navigator_moz_bluetooth
* use BaseModule's debug, rename `_debug` to `debug`
* use arrow function instead of bind

The existing test cases are more like integration test and tightly coupled with the whole flow, so it needs more time to rewrite large parts to get per function test.

Please kindly feedback again, thanks.
Attachment #8582925 - Flags: feedback?(iliu)
Attachment #8582925 - Flags: feedback?(alive)
I remember the conclusion is use Service.request('Bluetooth.getAdapter') what's wrong with it?
The patch did use Service.request('Bluetooth:adapter') to get promised adapter. In offline discussion, Ian prefer name the method as `adapter` than `getAdapter`.
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

Good work, Fred! Regarding the v1/v2 BT API: might it make sense to have an abstraction layer that hides the differences and that could also be used in other modules (e.g., BluetoothTransfer)?
Attachment #8582925 - Flags: feedback?(arno) → feedback+
Thanks for feedback! 
Though those v1/v2 API now only used in nfc_transfer_manager, I'll abstract them into bluetooth.js v1/v2 to make it testable for different API version.
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

Feedback+ for the patch. And I leave some comment on GitHub. We can start to revise them for reviewing process. 

Thanks for the effort with |service.request| version. It makes the code base more decoupled than before. As Arno's suggestion in comment 25, we also have discussion and plan to move file transfer relative work to another module. And the module is able to handle file transfer via Bluetooth/Wifi(if needed) in long tern. We won't do so much Bluetooth file transfer pre-process(get adapter/pair/getPairedDevices/connect) in |nfc_handover_manager| module.
Attachment #8582925 - Flags: feedback?(iliu) → feedback+
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

I am going to deliver the review right to Ian here;
Ian, if you have uncertain issues during review please don't hesitate to flag me or find me directly.
Attachment #8582925 - Flags: feedback?(alive)
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

Thanks for all your feedback.

I've addressed the comment and make sure it work with BTv1/v2 devices. I've tested with/without BT enabled cases as well.
Attachment #8582925 - Flags: review?(kmioduszewski)
Attachment #8582925 - Flags: review?(iliu)
Component: Bluetooth → Gaia::Bluetooth File Transfer
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

I leave two major comments on GitHub.

1). Please don't use Object.watch() since it might make performance issue. And it's designed for debugging only[1]. I think we have to find out the other way to do the same thing.

2). I don't know why you have to do getConnectedDevices() here. I don't see the process in the old code base. And the usage is in-corrected. The parameter is serviceUuid(profile ID).

[1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/watch
Attachment #8582925 - Flags: review?(iliu)
(In reply to Ian Liu [:ianliu] from comment #30)
> Comment on attachment 8582925 [details] [review]
> [gaia] gasolin:issue-1090799-5 > mozilla-b2g:master
> 
> I leave two major comments on GitHub.
> 
> 1). Please don't use Object.watch() since it might make performance issue.
> And it's designed for debugging only[1]. I think we have to find out the
> other way to do the same thing.
Especially the patch is in System app, We cannot jump into the risk in potential. It's not worth to use Object.watch().
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

Left small comments on github.
Attachment #8582925 - Flags: review?(kmioduszewski)
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

Thanks for review. The device.connected API is deprecated in BTv2 so we have to use `getConnectedDevices()` to check if device is connected. It's my fault to miss-use the API.

All issue addressed and test work on v1/v2 device, please kindly review it again :)
Attachment #8582925 - Flags: review?(kmioduszewski)
Attachment #8582925 - Flags: review?(iliu)
Blocks: 1121359
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

Fred,

Thanks for reminder for API changed in Bluetooth v2. Do getConnectedDevices() is needed since we have to let NFC module to support in Bluetooth v1/v2.

Looks you have revised some of my comment. Then, I leave comment for other nits. Please take a look for them on GitHub. The patch is almost there. And is there any unit test for bluetooth.js/bluetooth_v2.js? We have to cover these changed in this patch also. Thanks.

Krzysztof, could you please help to review the unit test of nfc_handover_manager.js? I'm not familiar with that part. Thanks.
Attachment #8582925 - Flags: review?(iliu)
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

@Ian really thanks for review, issue addressed and unit tests added for bluetooth.js/bluetooth_v2.js to cover these changed in this patch. Please kindly review it again.
Attachment #8582925 - Flags: review?(iliu)
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

I've focused on NfcHandoverManager tests as suggested by Ian. I've left some comments on github. 

Great job with the unit tests Fred, this was really needed.
Attachment #8582925 - Flags: review?(kmioduszewski)
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

Thanks for checking unit test part and provide better naming for test cases. I've update test case issues, please kindly review it again.
Attachment #8582925 - Flags: review?(kmioduszewski)
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

I have done some manual tests successfully. The patch is working on NFC send/receive file via Bluetooth. 

My test version are as following.

send/receive:
v1/v1
v2/v2
v1/v2
v2/v1

And I see Fred has updated the patch with my finally comment. Thanks for your effort here. Good job, r+ with me.
Attachment #8582925 - Flags: review?(iliu) → review+
Comment on attachment 8582925 [details] [review]
[gaia] gasolin:issue-1090799-5 > mozilla-b2g:master

I can see that all the comments are addressed. I've also done manual testing with Nexus 5 (running your gaia branch) and regular Android Lollipop device. I can send/receive files as expected, failure prompt is also present in proper scenarios.

Looks good, r+ from me. As I already, wrote this is really great work. Thanks Fred!
Attachment #8582925 - Flags: review?(kmioduszewski) → review+
(In reply to Ian Liu [:ianliu] from comment #38)
> My test version are as following.
> 
> send/receive:
> v1/v1
> v2/v2
> v1/v2
> v2/v1
Note for my test device. flame <--> flame. 
And also thanks for Krzysztof's reviewing effort.:)
treeherder all green,

merged to master https://github.com/mozilla-b2g/gaia/commit/526e8c2ff20a222ec68f7e820a979cb24fe549d4

thanks!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.