[Bluetooth] Convert Bluedroid Core API to asynchronous design

RESOLVED FIXED in 2.0 S5 (4july)

Status

Firefox OS
Bluetooth
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
2.0 S5 (4july)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 9 obsolete attachments)

2.90 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
4.85 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
3.44 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
8.27 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
6.53 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
9.96 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
5.12 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
6.22 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
6.68 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
3.52 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 8447230 [details] [diff] [review]
[01] Bug 1029386: Add result handler for Bluetooth Core profile
Attachment #8447230 - Flags: review?(shuang)
Created attachment 8447237 [details] [diff] [review]
[02] Bug 1029386: Split Bluedroid start/stop code
Attachment #8447237 - Flags: review?(shuang)
Created attachment 8447238 [details] [diff] [review]
[03] Bug 1029386: Asynchronous Bluedroid starting and stopping
Attachment #8447238 - Flags: review?(shuang)
Created attachment 8447240 [details] [diff] [review]
[04] Bug 1029386: Asynchronous Bluedroid adapter methods
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Attachment #8447240 - Flags: review?(shuang)
Created attachment 8447242 [details] [diff] [review]
[05] Bug 1029386: Asynchronous Bluedroid device-property methods
Attachment #8447242 - Flags: review?(shuang)
Created attachment 8447243 [details] [diff] [review]
[06] Bug 1029386: Asynchronous remote-service methods in Bluedroid
Attachment #8447243 - Flags: review?(shuang)
Created attachment 8447245 [details] [diff] [review]
[07] Bug 1029386: Asynchronous discovery methods in Bluedroid
Attachment #8447245 - Flags: review?(shuang)
Created attachment 8447247 [details] [diff] [review]
[08] Bug 1029386: Asynchronous Bluedroid device bonding
Attachment #8447247 - Flags: review?(shuang)
Created attachment 8447248 [details] [diff] [review]
[09] Bug 1029386: Asynchronous authentification in Bluedroid
Attachment #8447248 - Flags: review?(shuang)
Created attachment 8447250 [details] [diff] [review]
[10] Bug 1029386: Asynchronous Bluedroid DUT and LE interfaces
Attachment #8447250 - Flags: review?(shuang)
Hi Shawn,

While there are quite a lot of patches, most of them are very similar.

The return values of Bluedroid Core interfaces are now handled in these BluetoothResultHandler classes, which are passed as final arguments to the interface calls. BluetoothResultHandler contains a result method for each interface and a method for handling errors.

These BluetoothInterfaceRunnables in the first patch look a bit like black magic, but all they do is to execute the supplied method in the context of the supplied BluetoothResultHandler.
Hi Thomas,
Thanks, i will review today.
Comment on attachment 8447230 [details] [diff] [review]
[01] Bug 1029386: Add result handler for Bluetooth Core profile

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

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +5,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "BluetoothInterface.h"
> +#include "nsThreadUtils.h"
> +#include "nsAutoPtr.h"

nit: Could we keep alphabetical order?

::: dom/bluetooth/bluedroid/BluetoothInterface.h
@@ +189,5 @@
> +  virtual ~BluetoothResultHandler() { }
> +
> +  virtual void OnError(int aStatus)
> +  {
> +    BT_WARNING("received error code %d", aStatus);

nit: BT_WARNING message can only be seen in debug build, can we use BT_LOGR or BT_LOGD here? It will be hard to debug if we got feedback from the users.
Attachment #8447230 - Flags: review?(shuang) → review+
Comment on attachment 8447237 [details] [diff] [review]
[02] Bug 1029386: Split Bluedroid start/stop code

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

Please check |aShouldEnable| is still required. Happy to review again.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +763,5 @@
>    return sBtInterface->Enable();
>  }
>  
>  static nsresult
> +StartGonkBluetooth()

Well, I don't like |StartStopGonkBluetooth| either. :)

@@ +790,5 @@
> +}
> +
> +static nsresult
> +StopGonkBluetooth(bool aShouldEnable)
> +{

I don't see |aShouldEnable| is required here.

@@ +881,5 @@
>  BluetoothServiceBluedroid::StopInternal()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  nsresult ret = StopGonkBluetooth(false);

ditto. false is not required.
Attachment #8447237 - Flags: review?(shuang)
Comment on attachment 8447240 [details] [diff] [review]
[04] Bug 1029386: Asynchronous Bluedroid adapter methods

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

Hi Thomas,
One question is addressed, and hope you can answer it first. Thanks.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +105,5 @@
> +    {
> +      MOZ_ASSERT(NS_IsMainThread());
> +
> +      BT_LOGR("Fail to set: BT_SCAN_MODE_CONNECTABLE");
> +      SetAdapterProperty();

This is a bit strange logic here. The original code only tries to SetAdapterProperty to 'Connectable'. No matter SetAdapterProperty is succeed or failed, |AdapterAddedReceived| and |TryFriingAdapterAdded| shall be executed anyway. Now, it seems only reaching 'OnError', we will fire up AdapterAdded.
The reason why we have set adapter to 'Connectable' here is because by default local Bluetooth adapter remains invisible and non-connectable.
Attachment #8447240 - Flags: review?(shuang)
Comment on attachment 8447238 [details] [diff] [review]
[03] Bug 1029386: Asynchronous Bluedroid starting and stopping

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

Hi Thomas,
Only one question needs you to feedback. Happy to review again. :)

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +545,4 @@
>  {
>    mInterface->cleanup();
> +
> +  if (aRes) {

It seems in |BluetoothServiceBluedroid|, you pass nullptr to |BluetoothInterface::Cleanup|, then DispatchBluetoothResult won't be executed is it preferable?

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +774,5 @@
> +    BluetoothHfpManager::InitHfpInterface();
> +    BluetoothA2dpManager::InitA2dpInterface();
> +    sBtInterface->Enable(new EnableResultHandler());
> +  }
> +  void OnError(int aStatus) MOZ_OVERRIDE

nit: add one extra line before |onError|
Attachment #8447238 - Flags: review?(shuang)
Hi Shawn

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #15)
> Comment on attachment 8447240 [details] [diff] [review]
> [04] Bug 1029386: Asynchronous Bluedroid adapter methods
> 
> Review of attachment 8447240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Thomas,
> One question is addressed, and hope you can answer it first. Thanks.
> 
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +105,5 @@
> > +    {
> > +      MOZ_ASSERT(NS_IsMainThread());
> > +
> > +      BT_LOGR("Fail to set: BT_SCAN_MODE_CONNECTABLE");
> > +      SetAdapterProperty();
> 
> This is a bit strange logic here. The original code only tries to
> SetAdapterProperty to 'Connectable'. No matter SetAdapterProperty is succeed
> or failed, |AdapterAddedReceived| and |TryFriingAdapterAdded| shall be
> executed anyway. Now, it seems only reaching 'OnError', we will fire up
> AdapterAdded.
> The reason why we have set adapter to 'Connectable' here is because by
> default local Bluetooth adapter remains invisible and non-connectable.

AFAICS in the original code, the only difference is the extra error message that is written on error.

Now, on Error, the method |OnError| will be called, print an error message and call |SetAdapterProperty|. On success, |SetAdapterProperty| will be called directly by the Bluetooth wrapper. (Interface method and result callback always share the same name.) So the code should do the same as the original. I will change the implementation of the result handler to make the code more clearer.

And what is actually missing is 'MOZ_OVERRIDE' after |SetAdapterProperty|.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #14)
> Comment on attachment 8447237 [details] [diff] [review]
> [02] Bug 1029386: Split Bluedroid start/stop code
> 
> Review of attachment 8447237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please check |aShouldEnable| is still required. Happy to review again.

Oopsi, I guess I just forgot about this.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #16)
> Comment on attachment 8447238 [details] [diff] [review]
> [03] Bug 1029386: Asynchronous Bluedroid starting and stopping
> 
> Review of attachment 8447238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Thomas,
> Only one question needs you to feedback. Happy to review again. :)
> 
> ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> @@ +545,4 @@
> >  {
> >    mInterface->cleanup();
> > +
> > +  if (aRes) {
> 
> It seems in |BluetoothServiceBluedroid|, you pass nullptr to
> |BluetoothInterface::Cleanup|, then DispatchBluetoothResult won't be
> executed is it preferable?

I didn't want to allocate a pointless result handler for the call. There is no further code after the original call to |Cleanup|, so there's nothing else depending on the cleanup call running first. So the callback method for |Cleanup|would be empty. And |Cleanup| can't fail either with an error.

> 
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +774,5 @@
> > +    BluetoothHfpManager::InitHfpInterface();
> > +    BluetoothA2dpManager::InitA2dpInterface();
> > +    sBtInterface->Enable(new EnableResultHandler());
> > +  }
> > +  void OnError(int aStatus) MOZ_OVERRIDE
> 
> nit: add one extra line before |onError|

This would be inconsistent with the other result handlers. Shall I than add an empty line to all results handlers?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17)
> Hi Shawn
> AFAICS in the original code, the only difference is the extra error message
> that is written on error.
> 
> Now, on Error, the method |OnError| will be called, print an error message
> and call |SetAdapterProperty|. On success, |SetAdapterProperty| will be
> called directly by the Bluetooth wrapper. (Interface method and result
> callback always share the same name.) So the code should do the same as the
> original. I will change the implementation of the result handler to make the
> code more clearer.
> 
> And what is actually missing is 'MOZ_OVERRIDE' after |SetAdapterProperty|.
Oops, you're right. Indeed, I misunderstood the code :-|, no matter OnSuccess/OnError |SetAdapterProperty| will be executed.
So, as you explained in the comment, does that mean whenever |SetAdapterProperty| get called, bs->AdapterAddedReceived();
bs->TryFiringAdapterAdded(); 
// Trigger BluetoothOppManager to listen
BluetoothOppManager* opp = BluetoothOppManager::Get();

Do we need to run all these whenever do |SetAdapterProperty| even if SetAdapterProperty with name/discoverable property?
Comment on attachment 8447240 [details] [diff] [review]
[04] Bug 1029386: Asynchronous Bluedroid adapter methods

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +107,5 @@
> +
> +      BT_LOGR("Fail to set: BT_SCAN_MODE_CONNECTABLE");
> +      SetAdapterProperty();
> +    }
> +    void SetAdapterProperty()

Please add MOZ_OVERRIDE.

@@ +110,5 @@
> +    }
> +    void SetAdapterProperty()
> +    {
> +      MOZ_ASSERT(NS_IsMainThread());
> +

I think line 115 to 127 shall move back to the original place. It shall only be executed after Bluetooth enabled. Does that make sense?
Comment on attachment 8447242 [details] [diff] [review]
[05] Bug 1029386: Asynchronous Bluedroid device-property methods

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +993,5 @@
> +
> +    BT_WARNING("GetRemoteDeviceProperties(%s) failed: %d",
> +               mDeviceAddress.get(), aStatus);
> +
> +    /* dispatch result after final pending operation */

Good catch!
Attachment #8447242 - Flags: review?(shuang) → review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #19)
> This would be inconsistent with the other result handlers. Shall I than add
> an empty line to all results handlers?
Ya. Because i saw you add one empty line per each member function in BluetoothInterface. That will be fine, if you want to handle them in other patch to fix all coding style nits.
Hi Shawn

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #21)
> Comment on attachment 8447240 [details] [diff] [review]
> [04] Bug 1029386: Asynchronous Bluedroid adapter methods
> 
> Review of attachment 8447240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +110,5 @@
> > +    }
> > +    void SetAdapterProperty()
> > +    {
> > +      MOZ_ASSERT(NS_IsMainThread());
> > +
> 
> I think line 115 to 127 shall move back to the original place. It shall only
> be executed after Bluetooth enabled. Does that make sense?

We cannot move these lines back to the original place, because their execution depends on the success of |SetAdapterProperty|.

But there are _two_ result handlers for |SetAdapterProperty|. The one we're looking at right now is member of |SetupAfterEnableTask| and is only used when Bluetooth gets enabled. There is another |SetAdapterPropertyResultHandler| near line 1100. The one is in the bluetooth namespace and is used for regular calls to |BluetoothServiceBluedroid::SetProperty|. It doesn't do anything besides error handling. Using different classes in different situations prevents the problem you mention in comment 20.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> Hi Shawn
> But there are _two_ result handlers for |SetAdapterProperty|. The one we're
> looking at right now is member of |SetupAfterEnableTask| and is only used
> when Bluetooth gets enabled. There is another
> |SetAdapterPropertyResultHandler| near line 1100. The one is in the
> bluetooth namespace and is used for regular calls to
> |BluetoothServiceBluedroid::SetProperty|. It doesn't do anything besides
> error handling. Using different classes in different situations prevents the
> problem you mention in comment 20.
Oh, ok, I just think those lines don't depend on the results of SetProperty. Thanks for explanation, please ignore Comment 20.
Comment on attachment 8447243 [details] [diff] [review]
[06] Bug 1029386: Asynchronous remote-service methods in Bluedroid

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

It looks good to me. Thanks.
Attachment #8447243 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #25)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> > Hi Shawn
> > But there are _two_ result handlers for |SetAdapterProperty|. The one we're
> > looking at right now is member of |SetupAfterEnableTask| and is only used
> > when Bluetooth gets enabled. There is another
> > |SetAdapterPropertyResultHandler| near line 1100. The one is in the
> > bluetooth namespace and is used for regular calls to
> > |BluetoothServiceBluedroid::SetProperty|. It doesn't do anything besides
> > error handling. Using different classes in different situations prevents the
> > problem you mention in comment 20.
> Oh, ok, I just think those lines don't depend on the results of SetProperty.
> Thanks for explanation, please ignore Comment 20.

Oh, I see. Look at it once more, I guess you're right about the dependencies. I'll move these lines back to their original position.
Attachment #8447245 - Flags: review?(shuang) → review+
Attachment #8447250 - Flags: review?(shuang) → review+
Comment on attachment 8447247 [details] [diff] [review]
[08] Bug 1029386: Asynchronous Bluedroid device bonding

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1248,5 @@
> +    sBondingRunnableArray[mRunnableIndex] = nullptr;
> +    ReplyStatusError(runnable, aStatus, NS_LITERAL_STRING("CreatedPairedDevice"));
> +  }
> +private:
> +  size_t mRunnableIndex;

Why do we need to use |size_t| type instead of |PRUint32|?
Attachment #8447247 - Flags: review?(shuang)
Attachment #8447248 - Flags: review?(shuang) → review+
Created attachment 8448798 [details] [diff] [review]
[01] Bug 1029386: Add result handler for Bluetooth Core profile (v2)

Changes since v1:

  - sort include statements alphabetically
  - use BT_LOGR instead of BT_WARNING
Attachment #8447230 - Attachment is obsolete: true
Attachment #8448798 - Flags: review+
Created attachment 8448800 [details] [diff] [review]
[02] Bug 1029386: Split Bluedroid start/stop code (v2)

Changes since v1:

  - remove aShouldEnable
Attachment #8447237 - Attachment is obsolete: true
Attachment #8448800 - Flags: review?(shuang)
Created attachment 8448802 [details] [diff] [review]
[03] Bug 1029386: Asynchronous Bluedroid starting and stopping (v2)

Changes since v1:

  - add empty lines between result handler methods.
Attachment #8447238 - Attachment is obsolete: true
Attachment #8448802 - Flags: review+
Created attachment 8448807 [details] [diff] [review]
[04] Bug 1029386: Asynchronous Bluedroid adapter methods (v2)

Changes since v1:

  - moved setup code back to its original location; simplifies result handler
Attachment #8447240 - Attachment is obsolete: true
Attachment #8448807 - Flags: review?(shuang)
Created attachment 8448814 [details] [diff] [review]
[05] Bug 1029386: Asynchronous Bluedroid device-property methods (v2)

Changes since v1:

  - added empty lines to result handlers
Attachment #8447242 - Attachment is obsolete: true
Attachment #8448814 - Flags: review+
Created attachment 8448817 [details] [diff] [review]
[07] Bug 1029386: Asynchronous discovery methods in Bluedroid (v2)

Changes since v1:

  - added empty lines to result handlers
Attachment #8447245 - Attachment is obsolete: true
Attachment #8448817 - Flags: review+
Created attachment 8448818 [details] [diff] [review]
[08] Bug 1029386: Asynchronous Bluedroid device bonding

Changes since v1:

  - replace size_t by PRUint32
  - added empty lines to result handlers
Attachment #8447247 - Attachment is obsolete: true
Attachment #8448818 - Flags: review?(shuang)
Created attachment 8448819 [details] [diff] [review]
[09] Bug 1029386: Asynchronous authentification in Bluedroid (v2)

Changes since v1:

  - added empty lines to result handlers
Attachment #8447248 - Attachment is obsolete: true
Attachment #8448819 - Flags: review+
Attachment #8448818 - Flags: review?(shuang) → review+
Attachment #8448807 - Flags: review?(shuang) → review+
Attachment #8448800 - Flags: review?(shuang) → review+
Created attachment 8449332 [details] [diff] [review]
[10] Bug 1029386: Asynchronous Bluedroid DUT and LE interfaces (v2)

Changes since v1:

  - don't call le_test_mode on Android 4.2
Attachment #8447250 - Attachment is obsolete: true
Attachment #8449332 - Flags: review?(shuang)
Comment on attachment 8449332 [details] [diff] [review]
[10] Bug 1029386: Asynchronous Bluedroid DUT and LE interfaces (v2)

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

It looks good to me, thanks.
Attachment #8449332 - Flags: review?(shuang) → review+
Blocks: 1033961
You need to log in before you can comment on or make changes to this bug.