Closed Bug 1141616 Opened 9 years ago Closed 9 years ago

Lookup service channels in Bluedroid OPP manager

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox43 fixed)

Status:
RESOLVED FIXED
Milestone:
FxOS-S7 (18Sep)
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(3 files, 8 obsolete files)

[01] Bug 1141616: Implement |BluetoothService::GetServiceChannel| (v2)
6.89 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
[02] Bug 1141616: Support SDP lookups in |BluetoothServiceBluedroid| (v3)
5.02 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[03] Bug 1141616: Lookup service channel in Bluedroid's OPP manager (v3)
4.24 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review 
Right now, Bluedroid's OPP manager passes '-1' as channel to |BluetoothSocket| and let's Bluedroid handle the channel internally. To be compatible with other Bluetooth drivers, the OPP manager should lookup the channel and pass it to the socket.
|GetServiceChannel| currently fails. The returned UUID is 0 and the logcat contains

> E/        (  207): ### ASSERT : external/bluetooth/bluedroid/main/../btif/src/btif_dm.c line 1621 unhandled remote service record event (4) ###
See Also: → 1142007
I moved the service-lookup issue to bug 1142007. I have been able to get the lookup working if I unconditionally return -1 for any services' channel.
I have been working on patches for this bug, but I'm waiting for more feedback on bug 1142007.
Depends on: 1142007
Depends on: 1170509
No longer depends on: 1142007
Shawn, I used to run these patches with Bluetooth v1. I need some more time to make them work with v2. Sorry for the delay.
I cannot get this to work on v2. v2 contains |FetchUuidsInternal|, which somehow breaks Bluedroid's SDP. The STR for the bug is

 (1) use the bluetoothd branch from bug 1142007
 (2) pair with remote device
 (3) open Gallery app and send a picture to remote device
 (4) a notification says that the transfer has started, but nothing actually happens

I can see that bluetoothd calls Bluedroid's function, but no results are ever returned to the callback function.
Changes since v1

  - rebased onto latest master
Attachment #8614003 - Attachment is obsolete: true
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> I cannot get this to work on v2. v2 contains |FetchUuidsInternal|, which
> somehow breaks Bluedroid's SDP. The STR for the bug is
> 
>  (1) use the bluetoothd branch from bug 1142007
>  (2) pair with remote device
>  (3) open Gallery app and send a picture to remote device
>  (4) a notification says that the transfer has started, but nothing actually
> happens
> 
> I can see that bluetoothd calls Bluedroid's function, but no results are
> ever returned to the callback function.

ni myself for tacking this bug.
Flags: needinfo?(shuang)
Attachment #8614004 - Attachment is obsolete: true
Attachment #8634075 - Attachment is obsolete: true
I made progress wit this bug report and got it working by switching of discovery mode before doing SDP operations. I saw that |FetchUuidsInternal| does this as well.
Flags: needinfo?(shuang)
Comment on attachment 8651678 [details] [diff] [review]
[01] Bug 1141616: Implement |BluetoothService::GetServiceChannel|

Asking OPP maintainer
Attachment #8651678 - Flags: review?(shuang) → review?(btian)
Attachment #8651679 - Flags: review?(shuang) → review?(btian)
Attachment #8651680 - Flags: review?(shuang) → review?(btian)
Comment on attachment 8651678 [details] [diff] [review]
[01] Bug 1141616: Implement |BluetoothService::GetServiceChannel|

Review of attachment 8651678 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my comment below.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +887,5 @@
> +      }
> +      if (mGetRemoteServiceRecordArray[i].mUuid != mUuid) {
> +        continue;
> +      }
> +      break;

Simplify to

  if (mGetRemoteServiceRecordArray[i].mDeviceAddress == mDeviceAddress &&
      mGetRemoteServiceRecordArray[i].mUuid == mUuid) {
    break;
  }

@@ +893,5 @@
> +    if (NS_WARN_IF(i == mGetRemoteServiceRecordArray.Length())) {
> +      return;
> +    }
> +
> +    // Signal error to profile manager

Can we signal error immediately once we find call in array?

  for (...) {
    if (address and uuis matches) {
      // signal error to profile manager
      return;
    }
  }

  NS_WARN_IF(i == mGetRemoteServiceRecordArray.Length());

@@ +1661,5 @@
> +      if (NS_WARN_IF(i == mGetRemoteServiceRecordArray.Length())) {
> +        continue; // continue outer loop
> +      }
> +
> +      // Signal channel to profile manager

Ditto. Can we do searching and signaling together?

@@ +1670,5 @@
> +      mGetRemoteServiceRecordArray[i].mManager->OnGetServiceChannel(
> +        aBdAddr, uuidStr, p.mServiceRecord.mChannel);
> +
> +      mGetRemoteServiceRecordArray.RemoveElementAt(i);
> +      return;

Why return here instead of continuing outer loop?
Attachment #8651678 - Flags: review?(btian)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #0)
> Right now, Bluedroid's OPP manager passes '-1' as channel to
> |BluetoothSocket| and let's Bluedroid handle the channel internally. To be
> compatible with other Bluetooth drivers, the OPP manager should lookup the
> channel and pass it to the socket.

Thomas, I don't get why bluedroid OPP manager has to be compatible with other Bluetooth drivers. I think it's bluedroid specific and the change makes the flow more complicated. Are you going to make the OPP manager backend neutral?
Flags: needinfo?(tzimmermann)
Hi

(In reply to Ben Tian [:btian] from comment #16)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #0)
> > Right now, Bluedroid's OPP manager passes '-1' as channel to
> > |BluetoothSocket| and let's Bluedroid handle the channel internally. To be
> > compatible with other Bluetooth drivers, the OPP manager should lookup the
> > channel and pass it to the socket.
> 
> Thomas, I don't get why bluedroid OPP manager has to be compatible with
> other Bluetooth drivers. I think it's bluedroid specific and the change
> makes the flow more complicated. Are you going to make the OPP manager
> backend neutral?

In the long term, I want to write a BlueZ backend for the interfaces in BluetoothInterface.h. I already had a prototype in bug 1088574. With this change, we could remove our current BlueZ code and merge all the duplicated code for managers and profiles. That would reduce our code base significantly.

While testing bug 1088574, I found that the OPP manager for Bluedroid must look up service channels explicitly to work with BlueZ.
Flags: needinfo?(tzimmermann)
Comment on attachment 8651680 [details] [diff] [review]
[03] Bug 1141616: Lookup service channel in Bluedroid's OPP manager

Review of attachment 8651680 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my question below.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +293,5 @@
>      OnSocketConnectError(mSocket);
>      return;
>    }
>  
> +  if (!bs->UpdateSdpRecords(aDeviceAddress, this)) {

Why not follow bluez/BluetoothOppManager [1] to get service channel first and updates sdp record only if it fails to get service channel?
[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluez/BluetoothOppManager.cpp?offset=600#1653

Also create |mSocket| here to block later connect intention when one is already ongoing (getting service channel/updating sdp record).
Attachment #8651680 - Flags: review?(btian)
> @@ +1670,5 @@
> > +      mGetRemoteServiceRecordArray[i].mManager->OnGetServiceChannel(
> > +        aBdAddr, uuidStr, p.mServiceRecord.mChannel);
> > +
> > +      mGetRemoteServiceRecordArray.RemoveElementAt(i);
> > +      return;
> 
> Why return here instead of continuing outer loop?

I was afraid of sending a signal without any content after leaving the loop. But apparently this seems to work for other cases (UNKNOWN). Will be changed in the updated patch.
> 
> Why not follow bluez/BluetoothOppManager [1] to get service channel first
> and updates sdp record only if it fails to get service channel?
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluez/
> BluetoothOppManager.cpp?offset=600#1653

I'll change this in the next iteration.
Hi Ben,

Sorry that the update took so long. I had some problems with my computer that prevented my from getting work done for two days.

Chances since v1:

  - update loop logic
  - process all remote attributes after service record
Attachment #8651678 - Attachment is obsolete: true
Attachment #8657057 - Flags: review?(btian)
Changes since v1:

  - updated loop logic
  - process all remote attributes after UUIDs
Attachment #8657058 - Flags: review?(btian)
Attachment #8651679 - Attachment is obsolete: true
Attachment #8651679 - Flags: review?(btian)
Changes since v1:

  - create socket early in connect process
  - only lookup UUIDs if no service channel was found (aka BlueZ logic)
Attachment #8651680 - Attachment is obsolete: true
Attachment #8657059 - Flags: review?(btian)
Comment on attachment 8657057 [details] [diff] [review]
[01] Bug 1141616: Implement |BluetoothService::GetServiceChannel| (v2)

Review of attachment 8657057 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8657057 - Flags: review?(btian) → review+
Comment on attachment 8657058 [details] [diff] [review]
[02] Bug 1141616: Support SDP lookups in |BluetoothServiceBluedroid| (v2)

Review of attachment 8657058 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1834,5 @@
> +      }
> +
> +      if (index < mGetRemoteServicesArray.Length()) {
> +        mGetRemoteServicesArray.RemoveElementAt(index);
> +        mGetRemoteServicesArray[index].mManager->OnUpdateSdpRecords(aBdAddr);

Remove element AFTER calling |OnUpdateSdpRecords| otherwise incorrect profile manager is informed.
Attachment #8657058 - Flags: review?(btian) → review+
Comment on attachment 8657059 [details] [diff] [review]
[03] Bug 1141616: Lookup service channel in Bluedroid's OPP manager (v2)

Review of attachment 8657059 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +1629,5 @@
> +      OnSocketConnectError(mSocket);
> +      return;
> +    }
> +
> +    if (mNeedsUpdatingSdpRecords) {

Revise with guardian clause:

  if (mNeedsUpdatingSdpRecords) {
    mNeedsUpdatingSdpRecords = false;
    mLastServiceChannelCheck = TimeStamp::Now();
  } else {
    TimeDuration duration = TimeStamp::Now() - mLastServiceChannelCheck;
    if (duration.ToMilliseconds() >= kSdpUpdatingTimeoutMs) {
      OnSocketConnectError(mSocket);
      return;
    } 
  }

  if (!bs->UpdateSdpRecords(aDeviceAddress, this)) {
    OnSocketConnectError(mSocket);
    return;
  }
Attachment #8657059 - Flags: review?(btian) → review+
Changes since v2:

  - fix order of operations in |RemoteDevicePropertiesNotification|
Attachment #8657058 - Attachment is obsolete: true
Attachment #8658648 - Flags: review+
Changes since v2:

  - simplify SDP update logic in OPP manager
Attachment #8657059 - Attachment is obsolete: true
Attachment #8658650 - Flags: review+
Thanks a lot, Ben. I'm happy to make progress with this long-standing issue.

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=ee015390f701
See Also: → 1204617
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: