Closed Bug 1029389 Opened 10 years ago Closed 10 years ago

[Bluetooth] Convert Bluedroid HFP API to asynchronous design

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(9 files, 5 obsolete files)

[02] Bug 1029389: Add result-handler class for Bluetooth Handsfree profile
3.03 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[06] Bug 1029389: Asynchronous |BluetoothHandsfreeInterface::VolumeControl|
4.02 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[07] Bug 1029389: Asynchronous |BluetoothHandsfreeInterface::DeviceStatusNotification|
4.03 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[08] Bug 1029389: Asynchronous Bluetooth Handsfree response methods
12.83 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[09] Bug 1029389: Asynchronous |BluetoothHandsfreeInterface::PhoneStateChange|
4.58 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[01] Bug 1029389: Asynchronous starting and stopping of profile managers (v2)
11.36 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[03] Bug 1029389: Asynchronous Bluetooth Handsfree init and cleanup methods (v2)
7.53 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[04] Bug 1029389: Asynchronous Bluetooth Handsfree connection handling (v2)
10.32 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[05] Bug 1029389: Asynchronous Bluetooth Handsfree voice-recognition functions (v2)
2.55 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review 
      No description provided.
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Next patch set! :)

This should be a lot simpler than the socket patches. The asynchronous profile init/cleanup will also be use by the A2DP manager in a later patch set.
Comment on attachment 8453792 [details] [diff] [review]
[01] Bug 1029389: Asynchronous starting and stopping of profile managers

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

Overall, LGTM. I only have a few suggestion.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +517,5 @@
>    sBtAvrcpInterface = btInf->GetBluetoothAvrcpInterface();
>    NS_ENSURE_TRUE_VOID(sBtAvrcpInterface);
>  
>    ret = sBtAvrcpInterface->Init(&sBtAvrcpCallbacks);
>    if (ret != BT_STATUS_SUCCESS) {

if (aRes) {
  aRes->OnError(NS_ERROR_FAILURE);
}
Just like BluetoothHfpManager.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +162,5 @@
> +class ProfileDeinitResultHandler MOZ_FINAL
> +: public BluetoothProfileResultHandler
> +{
> +public:
> +  ProfileDeinitResultHandler(unsigned long aNumProfiles)

How about "unsigned char aNumProfiles"? That consumes less memory.

@@ +863,5 @@
> +class ProfileInitResultHandler MOZ_FINAL
> +: public BluetoothProfileResultHandler
> +{
> +public:
> +  ProfileInitResultHandler(unsigned long aNumProfiles)

Ditto.

@@ +889,5 @@
> +  {
> +    sBtInterface->Enable(new EnableResultHandler());
> +  }
> +
> +  unsigned long mNumProfiles;

Ditto.

@@ +902,5 @@
>  
>      // Register all the bluedroid callbacks before enable() get called
>      // It is required to register a2dp callbacks before a2dp media task starts up.
>      // If any interface cannot be initialized, turn on bluetooth core anyway.
> +    nsRefPtr<ProfileInitResultHandler> res = new ProfileInitResultHandler(2);

Can we define the number of supported profiles as constant? That will be much clearer.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +450,5 @@
>      btInf->GetBluetoothHandsfreeInterface();
>    NS_ENSURE_TRUE_VOID(interface);
>  
> +  if (interface->Init(&sBluetoothHfpCallbacks) != BT_STATUS_SUCCESS) {
> +    aRes->OnError(NS_ERROR_FAILURE);

if (aRes) {
 aRes->OnError(NS_ERROR_FAILURE);
}
Attachment #8453792 - Flags: review?(shuang)
Comment on attachment 8453795 [details] [diff] [review]
[03] Bug 1029389: Asynchronous Bluetooth Handsfree init and cleanup methods

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

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +463,5 @@
> +
> +  void Cleanup() MOZ_OVERRIDE
> +  {
> +    sBluetoothHfpInterface = nullptr;
> +    RunInit();

When we call |Cleanup|, why do we need to do |RunInit| to register callbacks? I'm a bit confused.

@@ +468,5 @@
> +  }
> +
> +  void RunInit()
> +  {
> +    mInterface->Init(&sBluetoothHfpCallbacks, this);

Could we check bt_status_t return value for |Init|?
Attachment #8453795 - Flags: review?(shuang)
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #12)
> Comment on attachment 8453795 [details] [diff] [review]
> [03] Bug 1029389: Asynchronous Bluetooth Handsfree init and cleanup methods
> 
> Review of attachment 8453795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> @@ +463,5 @@
> > +
> > +  void Cleanup() MOZ_OVERRIDE
> > +  {
> > +    sBluetoothHfpInterface = nullptr;
> > +    RunInit();
> 
> When we call |Cleanup|, why do we need to do |RunInit| to register
> callbacks? I'm a bit confused.

In the original code, if the HFP manager has been initialized, it will be cleaned up in |BluetoothHfpManager::InitHfpInterface| and then initialized again. The new code duplicates this behavior. If the HFP manager has already been initialized, we first call | Cleanup| to clean up and then call |Init| from the cleanup handler.
> > +
> > +  void RunInit()
> > +  {
> > +    mInterface->Init(&sBluetoothHfpCallbacks, this);
> 
> Could we check bt_status_t return value for |Init|?

This is still asynchronous code. Please see the comment above why this is here.
> In the original code, if the HFP manager has been initialized, it will be
> cleaned up in |BluetoothHfpManager::InitHfpInterface| and then initialized
> again. The new code duplicates this behavior. If the HFP manager has already
> been initialized, we first call | Cleanup| to clean up and then call |Init|
> from the cleanup handler.
Ok. The only reason I raised this question is because |Cleanup| function did initialization (call |RunInit|). I expect |Cleanup| only simply cleanup resources. Thanks for your confirmation.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> This is still asynchronous code. Please see the comment above why this is
> here.
Sigh! Sorry I forgot I lived in a pure async world.
Comment on attachment 8453792 [details] [diff] [review]
[01] Bug 1029389: Asynchronous starting and stopping of profile managers

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

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +517,5 @@
>    sBtAvrcpInterface = btInf->GetBluetoothAvrcpInterface();
>    NS_ENSURE_TRUE_VOID(sBtAvrcpInterface);
>  
>    ret = sBtAvrcpInterface->Init(&sBtAvrcpCallbacks);
>    if (ret != BT_STATUS_SUCCESS) {

Since later patch change this part, please ignore this comment.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +450,5 @@
>      btInf->GetBluetoothHandsfreeInterface();
>    NS_ENSURE_TRUE_VOID(interface);
>  
> +  if (interface->Init(&sBluetoothHfpCallbacks) != BT_STATUS_SUCCESS) {
> +    aRes->OnError(NS_ERROR_FAILURE);

Since later patch change this part, please ignore this comment.
Attachment #8453792 - Flags: review+
Comment on attachment 8453795 [details] [diff] [review]
[03] Bug 1029389: Asynchronous Bluetooth Handsfree init and cleanup methods

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

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +463,5 @@
> +
> +  void Cleanup() MOZ_OVERRIDE
> +  {
> +    sBluetoothHfpInterface = nullptr;
> +    RunInit();

|Cleanup| usually cleanup resources, but |Cleanup| function did initialization (call |RunInit|) here. Can you add comments to explain the reason why we init inside |Cleanup|? That can make things clearer and easy to read.

In the original code, if the HFP manager has been initialized, it will be cleaned up and been initialized again in |BluetoothHfpManager::InitHfpInterface|, and |InitHfpInterface| function name matches the behavior. However, function name |Cleanup| doesn't have strong relationship with |Init|.
Attachment #8453795 - Flags: review+
Comment on attachment 8453796 [details] [diff] [review]
[04] Bug 1029389: Asynchronous Bluetooth Handsfree connection handling

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

Hi Thomas,
Would you please check DispatchBluetoothHandsfreeResult parts?

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +661,5 @@
> +  bt_status_t status = mInterface->disconnect(aBdAddr);
> +
> +  if (aRes) {
> +    DispatchBluetoothHandsfreeResult(
> +      aRes, &BluetoothHandsfreeResultHandler::Connect, status);

&BluetoothHandsfreeResultHandler::Connect ? Is it correct?

@@ +673,5 @@
> +  bt_status_t status = mInterface->connect_audio(aBdAddr);
> +
> +  if (aRes) {
> +    DispatchBluetoothHandsfreeResult(
> +      aRes, &BluetoothHandsfreeResultHandler::Connect, status);

Should be &BluetoothHandsfreeResultHandler::ConnectAudio?

@@ +685,5 @@
> +  bt_status_t status = mInterface->disconnect_audio(aBdAddr);
> +
> +  if (aRes) {
> +    DispatchBluetoothHandsfreeResult(
> +      aRes, &BluetoothHandsfreeResultHandler::Connect, status);

Should be &BluetoothHandsfreeResultHandler::ConnectAudio?
Attachment #8453796 - Flags: review?(shuang)
Comment on attachment 8453799 [details] [diff] [review]
[05] Bug 1029389: Asynchronous Bluetooth Handsfree voice-recognition functions

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

LGTM. Thanks.
Attachment #8453799 - Flags: review?(shuang) → review+
> @@ +685,5 @@
> > +  bt_status_t status = mInterface->disconnect_audio(aBdAddr);
> > +
> > +  if (aRes) {
> > +    DispatchBluetoothHandsfreeResult(
> > +      aRes, &BluetoothHandsfreeResultHandler::Connect, status);
> 
typo: Should be &BluetoothHandsfreeResultHandler::DisconnectAudio?
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #11)
> Comment on attachment 8453792 [details] [diff] [review]
> [01] Bug 1029389: Asynchronous starting and stopping of profile managers
> 
> Review of attachment 8453792 [details] [diff] [review]:
> -----------------------------------------------------------------

> 
> How about "unsigned char aNumProfiles"? That consumes less memory.

Done.

> @@ +902,5 @@
> >  
> >      // Register all the bluedroid callbacks before enable() get called
> >      // It is required to register a2dp callbacks before a2dp media task starts up.
> >      // If any interface cannot be initialized, turn on bluetooth core anyway.
> > +    nsRefPtr<ProfileInitResultHandler> res = new ProfileInitResultHandler(2);
> 
> Can we define the number of supported profiles as constant? That will be
> much clearer.

I added these methods to an array of callback functions and code uses the array length as parameter. For new profiles, just add new functions to the arrays and everything will work correctly.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #18)
> Comment on attachment 8453795 [details] [diff] [review]
> [03] Bug 1029389: Asynchronous Bluetooth Handsfree init and cleanup methods

> 
> |Cleanup| usually cleanup resources, but |Cleanup| function did
> initialization (call |RunInit|) here. Can you add comments to explain the
> reason why we init inside |Cleanup|? That can make things clearer and easy
> to read.

I left a comment. But see below for a more general point.

> In the original code, if the HFP manager has been initialized, it will be
> cleaned up and been initialized again in
> |BluetoothHfpManager::InitHfpInterface|, and |InitHfpInterface| function
> name matches the behavior. However, function name |Cleanup| doesn't have
> strong relationship with |Init|.

|Cleanup| the name of the BluetoothHandsfreeInterface, which come from Android's API and the BlueZ protocol. I'd prefer to stick with these names and have a little inconvenience here.

But in general, I think the code should simply fail hard if we're trying to initialize an already initialized interface. Just cleaning up is dangerous, because the current instance might still be in use; and that would be an error in the overall logic of the module. But this refactoring needs to be done in an independent patch set.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #19)
> Comment on attachment 8453796 [details] [diff] [review]
> [04] Bug 1029389: Asynchronous Bluetooth Handsfree connection handling
> 
> Review of attachment 8453796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Thomas,
> Would you please check DispatchBluetoothHandsfreeResult parts?
> 
> ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> @@ +661,5 @@
> > +  bt_status_t status = mInterface->disconnect(aBdAddr);
> > +
> > +  if (aRes) {
> > +    DispatchBluetoothHandsfreeResult(
> > +      aRes, &BluetoothHandsfreeResultHandler::Connect, status);
> 
> &BluetoothHandsfreeResultHandler::Connect ? Is it correct?
> 
> @@ +673,5 @@
> > +  bt_status_t status = mInterface->connect_audio(aBdAddr);
> > +
> > +  if (aRes) {
> > +    DispatchBluetoothHandsfreeResult(
> > +      aRes, &BluetoothHandsfreeResultHandler::Connect, status);
> 
> Should be &BluetoothHandsfreeResultHandler::ConnectAudio?
> 
> @@ +685,5 @@
> > +  bt_status_t status = mInterface->disconnect_audio(aBdAddr);
> > +
> > +  if (aRes) {
> > +    DispatchBluetoothHandsfreeResult(
> > +      aRes, &BluetoothHandsfreeResultHandler::Connect, status);
> 
> Should be &BluetoothHandsfreeResultHandler::ConnectAudio?

This is embarrassing. :( I successfully tested this with a handset, but the result handlers in |BluetoothHfpManager| only need to implement error handlers, so I didn't notice. Fixed now.
Changes since v1:

  - compute number of managers from data structures
  - store number of managers in unsigned char
Attachment #8453792 - Attachment is obsolete: true
Attachment #8455242 - Flags: review+
Changes since v1:

  - commented on cleanup during initialization
Attachment #8453793 - Attachment is obsolete: true
Attachment #8455243 - Flags: review+
Attachment #8455243 - Attachment is obsolete: true
Comment on attachment 8453793 [details] [diff] [review]
[02] Bug 1029389: Add result-handler class for Bluetooth Handsfree profile

Reverted incorrect upload
Attachment #8453793 - Attachment is obsolete: false
Changes since v1:

  - commented on cleanup during initialization
Attachment #8453795 - Attachment is obsolete: true
Attachment #8455244 - Flags: review+
Changes since v1:

  - call correct result handler methods from |BluetoothHandsfreeInterface|
Attachment #8453796 - Attachment is obsolete: true
Attachment #8455246 - Flags: review?(shuang)
Changes since v1:

  - rebased
Attachment #8453799 - Attachment is obsolete: true
Attachment #8455249 - Flags: review+
Blocks: 1038645
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: