Closed Bug 1146744 Opened 9 years ago Closed 9 years ago

[NFC][Bluetooth] NFC headset auto connect always failed when user manually disconnect already connected NFC headset

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S10 (17apr)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: ashiue, Assigned: arno)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached file flame_22.log
Gaia-Rev        e54c4ed1cc188f70ddf1156534d364005dc45490
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7ba1778d237b
Build-ID        20150323162503
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150323.200543
FW-Date         Mon Mar 23 20:05:54 EDT 2015
Bootloader      L1TC100118D0

STR:
1. Enable NFC on test device
2. Tap NFC headset on device
3. Show connect confirmed dialog and device click yes to connect
4. After NFC headset paired and connected, user manually disconnect NFC headset 
5. Tap NFC headset on device again 
6. Show connect confirmed dialog and device click yes to connect

Expected result:
NFC headset auto connect again.

Actual result:
Could not auto connect to NFC headset.
Shawn has some finding on bug 1141899 Comment 16.
blocking-b2g: --- → 2.2?
QA Whiteboard: [COM=NFC]
On flame-kk, can be paired and connect with the remote device at the first
time. If the device get paired previously but disconnected, re-connection
trigger by nfc never be initialized. I notice that NfcHandoverManager.js is
trying to do pairing again even device is in the paired list and I don't see
NfcHandoverManager creates connection.
ni? for Arno for NfcHandoverManager
Blocks: NFC-Gaia
Flags: needinfo?(arno)
triage: function not working
blocking-b2g: 2.2? → 2.2+
(In reply to Shawn Huang [:shawnjohnjr] from comment #1)
> On flame-kk, can be paired and connect with the remote device at the first
> time. If the device get paired previously but disconnected, re-connection
> trigger by nfc never be initialized. I notice that NfcHandoverManager.js is
> trying to do pairing again even device is in the paired list and I don't see
> NfcHandoverManager creates connection.

NfcHandoverManager will not check if the device is already paired. This has been the behavior since the first version and the pre-KK BT stack seemed to be lenient about this. However, NfcHandoverManager will always connect after pairing:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_handover_manager.js#L337

If you don't see the connect(), onsuccess() of the pair() request was not called. Will onerror() be called if the device is already paired? That would explain the missing connect().

Since this worked pre-KK, how do you want to handle this? Do you want to guard against multiple pairing on the BT layer or do you want me to check for this in NfcHandoverManager? If the latter, I guess I still need to do a connect() for already-paired devices, right?
Flags: needinfo?(arno) → needinfo?(shuang)
(In reply to arno from comment #4)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #1)
> > On flame-kk, can be paired and connect with the remote device at the first
> > time. If the device get paired previously but disconnected, re-connection
> > trigger by nfc never be initialized. I notice that NfcHandoverManager.js is
> > trying to do pairing again even device is in the paired list and I don't see
> > NfcHandoverManager creates connection.
> 
> NfcHandoverManager will not check if the device is already paired. This has
> been the behavior since the first version and the pre-KK BT stack seemed to
> be lenient about this. However, NfcHandoverManager will always connect after
> pairing:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> nfc_handover_manager.js#L337
> 
> If you don't see the connect(), onsuccess() of the pair() request was not
> called. Will onerror() be called if the device is already paired? That would
> explain the missing connect().
> 
> Since this worked pre-KK, how do you want to handle this? Do you want to
> guard against multiple pairing on the BT layer or do you want me to check
> for this in NfcHandoverManager? If the latter, I guess I still need to do a
> connect() for already-paired devices, right?

I think you can check paired devices before you re-pair again. Similar code: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_handover_manager.js#L532

I have not got much time to dig into the real problem yet. I expect i will do it on Friday.
Flags: needinfo?(shuang)
A soft reminder that feature complete day will be April6. 
Let's try to fix before that.
Flags: needinfo?(shuang)
Flags: needinfo?(arno)
Flags: needinfo?(arno)
Attachment #8585818 - Flags: review?(alive)
Hi arno, can you take the bug since you have submitted a patch for review?
Flags: needinfo?(arno)
Assignee: nobody → arno
Flags: needinfo?(arno)
Attachment #8585818 - Attachment is patch: true
Attachment #8585818 - Attachment mime type: text/x-github-pull-request → text/plain
Flags: needinfo?(shuang)
See Also: → 1146741
See Also: 11467411141899
Comment on attachment 8585818 [details] [diff] [review]
Don't call pair() on an already paired BT device (v2.2)

See nits before you land.
Attachment #8585818 - Flags: review?(alive) → review+
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> Comment on attachment 8585818 [details] [diff] [review]
> Don't call pair() on an already paired BT device (v2.2)
> 
> See nits before you land.

Thanks, Alive. I'll use promises for the master-branch patch.
[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: there is a hard JS error if the user tries to pair an already paired device.
[Testing completed]: Tests on Flame device and added new unit tests
[Risk to taking this patch] (and alternatives if risky): Minimal
[String changes made]: none
Attachment #8585817 - Attachment is obsolete: true
Attachment #8585818 - Attachment is obsolete: true
Attachment #8586116 - Flags: approval-gaia-v2.2?
waiting for mater landing before considering the 2.2 approval request.
Alive: here is the patch for master. As you will see, I didn't use Promise. I do have a version with Promise that works fine on a device, but I was not able to get the unit tests to work. Unfortunately I am running out of time. The sheriff will only accept the critical v2.2 patch when the patch for master has landed. If you agree, I will open a new bug to use Promises in NfcHandoverManager that I will work on when I have more time.
Attachment #8586545 - Flags: review?(alive)
Comment on attachment 8586545 [details] [review]
Don't call pair() on an already paired BT device (master)

Ya, you will have unit test problem :)

The keypoint is you will need to test the result in the returning another promise:
test('find paired device', function(done) {
  subject.findPairedDevice(address).then(function() {
    // test success stuff
  });
});

And test the one who call it by mock promise:
test('something called paired device', function() {
  this.sinon.stub(subject, 'findPairedDevice').returns(MockPromise);
  subject.someMethodWouldCallGetPairedDevice();
  MockPromise.mResolve();
  // Test the content in success callback is done
  MockPromise.mReject();
  // Test the content in fail callback is done
});

If this does not work, maybe we should return another problem in the method who calls findPairedDevice and test it using done().
subject.someMethodWouldCallGetPairedDevice().then(function() {
  // assert
  done();
});
Attachment #8586545 - Flags: review?(alive) → review+
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.
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #16)
>
> Ya, you will have unit test problem :)

Thanks for the suggestions. Introducing that one Promise broke over half of the unit tests for NfcHandoverManager because findPairedDevice() was on the execution path. I was trying to find a way without MockPromise but ultimately failed. I guess your suggestion of stubbing findPairedDevice() is the only way, although this will get quickly very painful if more promises are used. It is too bad that internal implementation details under test need to be exposed in the test case itself. Might it be possible to do something similar to sinon.seFakeTimers() that makes handling of Promises more deterministic?
alive/arno can you help per : https://bugzilla.mozilla.org/show_bug.cgi?id=1146220#c13
Flags: needinfo?(arno)
Flags: needinfo?(alive)
Vincent, Wesley: what needs to be done here? The autolander rejects the CI flag.
Flags: needinfo?(whuang)
Flags: needinfo?(vchang)
Flags: needinfo?(arno)
I think the problem in bug 1146220 had already being resolved. Clear ni.
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21)
> I think the problem in bug 1146220 had already being resolved. Clear ni.

we still need to land this in master.
similar case as https://bugzilla.mozilla.org/show_bug.cgi?id=1146220#c19
I'm changing the component to gaia:system
Component: NFC → Gaia::System
Flags: needinfo?(whuang)
Flags: needinfo?(vchang)
(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #23)
> similar case as https://bugzilla.mozilla.org/show_bug.cgi?id=1146220#c19
> I'm changing the component to gaia:system

Yes, this should work with the new component set, but unfortunately the linters are failing.

I've submitted a new PR to rollup a linter fix with the original patch. Assuming the linter and tests pass, I think we can land this. Thanks!
Comment on attachment 8586545 [details] [review]
Don't call pair() on an already paired BT device (master)

Obsoleting in favor of: https://github.com/mozilla-b2g/gaia/pull/29401
Attachment #8586545 - Attachment is obsolete: true
Comment on attachment 8586536 [details] [review]
[gaia] svic:Bug_1146744 > mozilla-b2g:master

Obsoleting in favor of: https://github.com/mozilla-b2g/gaia/pull/29401
Attachment #8586536 - Attachment is obsolete: true
Comment on attachment 8589853 [details] [review]
[gaia] KevinGrandon:svic-Bug_1146744 > mozilla-b2g:master

Carrying review from Alive earlier.
Attachment #8589853 - Flags: review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #8586116 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Blocks: 1154136
QA Whiteboard: [COM=NFC]
See Also: → 1154136
See Also: → 1090799
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: