Closed Bug 1199110 Opened 4 years ago Closed 4 years ago

Separate AVRCP and A2DP managers

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
FxOS-S6 (04Sep)
Tracking Status
firefox43 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(3 files, 4 obsolete files)

I'd like to simplify the Bluetooth backend interfaces a bit.

The first change should be the introduction of a separate manager class for AVRCP. It's a different profile and can be used independently from A2DP.

This change will allow for further cleanups and simplifications in the backend interfaces, and the removal of dependencies between backend modules.
Comment on attachment 8653321 [details] [diff] [review]
[02] Bug 1199110: Add |BluetoothAvrcpManager| to Bluedroid and BlueZ backends

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

Please clean up '#if 0' section.

::: dom/bluetooth/bluez/BluetoothAvrcpManager.cpp
@@ +54,2 @@
>  {
> +#if 0

Please clean up '#if 0'
Comment on attachment 8653322 [details] [diff] [review]
[03] Bug 1199110: Remove AVRCP support from |BluetoothA2dpManager| and convert callers

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

Please clean up '#if 0' sections.
Comment on attachment 8653321 [details] [diff] [review]
[02] Bug 1199110: Add |BluetoothAvrcpManager| to Bluedroid and BlueZ backends

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

::: dom/bluetooth/bluez/BluetoothAvrcpManager.cpp
@@ +415,2 @@
>  {
> +  return !mDeviceAddress.IsEmpty();

What's the difference between |IsConnected| and |IsAvrcpConnected|? It seems to be duplicated function.
(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> Comment on attachment 8653322 [details] [diff] [review]
> [03] Bug 1199110: Remove AVRCP support from |BluetoothA2dpManager| and
> convert callers
> 
> Review of attachment 8653322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please clean up '#if 0' sections.

Oops, sorry about that. :(
(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> > +  return !mDeviceAddress.IsEmpty();
> 
> What's the difference between |IsConnected| and |IsAvrcpConnected|? It seems
> to be duplicated function.

I was asking myself the same question. |IsConnected| is required by the base class. |IsAvrcpConnected| seems to be only used in the BlueZ. Somehow that method made it into Bluedroid as well; for symmetry I guess. I left it in because I didn't know if it was important.
Sorry about the #if 0 blocks. I should have checked the patches once more before uploading them.
Changes since v1:

  - cleaned up '#if 0' statements
  - Merged |ResetAvrcp| into |Reset|
Attachment #8653321 - Attachment is obsolete: true
Attachment #8653483 - Flags: review?(shuang)
Changes since v1:

  - cleaned up '#if 0' statements
  - merged |ResetA2dp| into |Reset|
Attachment #8653322 - Attachment is obsolete: true
Attachment #8653484 - Flags: review?(shuang)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> > > +  return !mDeviceAddress.IsEmpty();
> > 
> > What's the difference between |IsConnected| and |IsAvrcpConnected|? It seems
> > to be duplicated function.
> 
> I was asking myself the same question. |IsConnected| is required by the base
> class. |IsAvrcpConnected| seems to be only used in the BlueZ. Somehow that
> method made it into Bluedroid as well; for symmetry I guess. I left it in
> because I didn't know if it was important.
The original A2dpManager function managed two states. |IsConnected| returns a2dp sink state and |IsAvrcpConnected| return avrcp state, this is why we have another |IsAvrcpConnected|.

I think we can remove |IsAvrcpConnected| inside BluetoothAvrcpManager and handle actual connection state inside |IsConnected|, since we now separate A2DP/AVRCP logic. The function |IsAvrcpConnected| is actually reported AVRCP state, the reason for that is because BlueZ avrcp (control module) can report avrcp connection state, however, the later bluedroid interface does not expose any HAL API to get AVRCP state, somehow AVRCP depends on A2DP connection, as for bluedroid BluetoothAvrcpManager, I guess we can only depends on a2dp sink state.
Comment on attachment 8653483 [details] [diff] [review]
[02] Bug 1199110: Add |BluetoothAvrcpManager| to Bluedroid and BlueZ backends (v2)

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

::: dom/bluetooth/bluedroid/BluetoothAvrcpManager.cpp
@@ +347,5 @@
>    MOZ_ASSERT(!aDeviceAddress.IsEmpty());
>    MOZ_ASSERT(aController);
>  
> +  // AVRCP doesn't require connecting. We just set the remote address here.
> +  mDeviceAddress = aDeviceAddress;

I'm afraid this function won't be called. We need to modify |BluetoothProfileController::AddProfileWithServiceClass|.

@@ +416,2 @@
>  {
> +  return !mDeviceAddress.IsEmpty();

Ok. It should be fine to use |mDeviceAddress| to tell the state, but need to modify |BluetoothProfileController|, or only use mAvrcpConnected, will be better?
Attachment #8653483 - Flags: review?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #15)
> Comment on attachment 8653483 [details] [diff] [review]
> [02] Bug 1199110: Add |BluetoothAvrcpManager| to Bluedroid and BlueZ
> backends (v2)
> 
> Review of attachment 8653483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothAvrcpManager.cpp
> @@ +347,5 @@
> >    MOZ_ASSERT(!aDeviceAddress.IsEmpty());
> >    MOZ_ASSERT(aController);
> >  
> > +  // AVRCP doesn't require connecting. We just set the remote address here.
> > +  mDeviceAddress = aDeviceAddress;
> 
> I'm afraid this function won't be called. We need to modify
> |BluetoothProfileController::AddProfileWithServiceClass|.
> 
> @@ +416,2 @@
> >  {
> > +  return !mDeviceAddress.IsEmpty();
> 
> Ok. It should be fine to use |mDeviceAddress| to tell the state, but need to
> modify |BluetoothProfileController|, or only use mAvrcpConnected, will be
> better?

Oh sorry, it's in Patch [3]. Please ignore the comment.
Comment on attachment 8653483 [details] [diff] [review]
[02] Bug 1199110: Add |BluetoothAvrcpManager| to Bluedroid and BlueZ backends (v2)

r=me, but i think to use |mAvrcpConnected| is better than checking mDeviceAddress.
Changes since v2:

  - removed |IsAvrcpConnected| in favor of |IsConnected|
Attachment #8653483 - Attachment is obsolete: true
Attachment #8654069 - Flags: review+
Changes since v2:

  - rebased onto [02](v3)
Attachment #8653484 - Attachment is obsolete: true
Attachment #8654071 - Flags: review+
Attachment #8654069 - Attachment description: [02] Bug 1199110: Add |BluetoothAvrcpManager| to Bluedroid and BlueZ backends (v2) → [02] Bug 1199110: Add |BluetoothAvrcpManager| to Bluedroid and BlueZ backends (v3)
Let's wait for bug 1193379 to land.
Depends on: 1193379
Some BlueZ versions seem to support connect and disconnect for AVRCP [1]. In preparation of bug 1088574, I'll certainly add these methods to |BluetoothAvrcpInterface|. In the case of Bluedroid, they will simply be dummies that dispatch the result handler.

[1] https://www.codeaurora.org/cgit/quic/le/platform/external/bluetooth/bluez/tree/doc/control-api.txt#n15
Depends on: 1202060
You need to log in before you can comment on or make changes to this bug.