[Bluetooth] Wrap Bluedroid interface

RESOLVED FIXED in 2.0 S5 (4july)

Status

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

(2 attachments, 1 obsolete attachment)

We need to wrap the Bluedroid interface for converting it to an asynchronous implementation.
Created attachment 8442065 [details] [diff] [review]
[01] Bug 1027030: Wrap Bluedroid interfaces in classes

Hi Eric,

These patches are the first steps away from direct calls to Bluedroid. How shall I handle bluetooth2? I'd supply ported patches if required.
Attachment #8442065 - Flags: review?(echou)
Created attachment 8442067 [details] [diff] [review]
[02] Bug 1027030: Convert Bluetooth to use Bluedroid wrappers
Attachment #8442067 - Flags: review?(echou)
Blocks: 1029386
Blocks: 1029387
Blocks: 1029389
Blocks: 1029390
Attachment #8442065 - Flags: review?(echou) → review?(shuang)
Attachment #8442067 - Flags: review?(echou) → review?(shuang)
Hi Shawn,

These are the patches for wrapping BT-HAL calls. The plan is to

 - wrap the calls;
 - convert the interfaces into an asynchronous style, where the result values 
   are handled by a call-back class; and
 - make the interface independent from Bluedroid data types, so that we can 
   implement the BlueZ5 protocol or BlueZ support behind them.

These patches provide step 1 and I'm currently in the middle of state 2 of the process.
s/state 2/step 2
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
>  - make the interface independent from Bluedroid data types, so that we can 
>    implement the BlueZ5 protocol or BlueZ support behind them.
> 
> These patches provide step 1 and I'm currently in the middle of state 2 of
> the process.
Thomas, thanks for explanation. This looks cool, will get you further update before Friday.
Comment on attachment 8442067 [details] [diff] [review]
[02] Bug 1027030: Convert Bluetooth to use Bluedroid wrappers

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

Overall, it looks good to me.
Attachment #8442067 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #6)
> Comment on attachment 8442067 [details] [diff] [review]
> [02] Bug 1027030: Convert Bluetooth to use Bluedroid wrappers
> 
> Review of attachment 8442067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, it looks good to me.

We probably need to also change BluetoothHfpManager for hfp-fallback?
Comment on attachment 8442065 [details] [diff] [review]
[01] Bug 1027030: Wrap Bluedroid interfaces in classes

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

Only a few nits addressed here. I'm sorry for the late reply.

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +12,5 @@
> +struct interface_traits
> +{ };
> +
> +//
> +// Socket Profile

nit: Socket is not profile, can we change to 'Socket Interface'?

@@ +162,5 @@
> +}
> +
> +bt_status_t
> +BluetoothHandsfreeInterface::CindResponse(int aSvc, int aNumActive, int aNumHeld,
> +                           bthf_call_state_t aCallSetupState, int aSignal,

nits: weired indentation

@@ +368,5 @@
> +
> +//
> +// Bluetooth Core Interface
> +//
> +

Good catch! Can you add one line comment for better understanding this marco?

@@ +410,5 @@
> +    goto err_get_bluetooth_interface;
> +  }
> +
> +  if (bt_interface->size != sizeof(*bt_interface)) {
> +    BT_WARNING("interface of incorrect size");

We usually checked sizeof(bt_callback_t) to avoid missing registration, do we need to check |bt_interfce|? Or you intend to check size of bt_callback_t?

@@ +417,5 @@
> +
> +  sBluetoothInterface = new BluetoothInterface(bt_interface);
> +
> +  return sBluetoothInterface;
> +err_bt_interface_size:

nit: Add new line before line 'err_bt_interface_size'

@@ +421,5 @@
> +err_bt_interface_size:
> +err_get_bluetooth_interface:
> +  err = device->close(device);
> +  if (err)
> +    BT_WARNING("close failed: %s", strerror(err));

super nit: Based on Gecko coding style, "Always brace controlled statements, even a single-line consequent of an if else else".

@@ +495,5 @@
> +}
> +
> +int
> +BluetoothInterface::SetRemoteDeviceProperty(bt_bdaddr_t* aRemoteAddr,
> +                                      const bt_property_t* aProperty)

nit: bad indentation

@@ +560,5 @@
> +}
> +
> +int
> +BluetoothInterface::SspReply(const bt_bdaddr_t* aBdAddr, bt_ssp_variant_t aVariant,
> +                       uint8_t aAccept, uint32_t aPasskey)

nit: bad indentation

@@ +595,5 @@
> +BluetoothInterface::GetProfileInterface()
> +{
> +  static T* sBluetoothProfileInterface;
> +
> +  if (sBluetoothProfileInterface)

super nit: Based on Gecko coding style, "Always brace controlled statements, even a single-line consequent of an if else else".

::: dom/bluetooth/bluedroid/BluetoothInterface.h
@@ +203,5 @@
> +  int GetRemoteDeviceProperties(bt_bdaddr_t *aRemoteAddr);
> +  int GetRemoteDeviceProperty(bt_bdaddr_t* aRemoteAddr,
> +                              bt_property_type_t aType);
> +  int SetRemoteDeviceProperty(bt_bdaddr_t* aRemoteAddr,
> +                        const bt_property_t* aProperty);

nit:bad indentation
Attachment #8442065 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #7)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #6)
> > Comment on attachment 8442067 [details] [diff] [review]
> > [02] Bug 1027030: Convert Bluetooth to use Bluedroid wrappers
> > 
> > Review of attachment 8442067 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Overall, it looks good to me.
> 
> We probably need to also change BluetoothHfpManager for hfp-fallback?

Hmm, it doesn't look like hfp-fallback has any dependencies in bluedroid.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #7)
> > (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #6)
> > > Comment on attachment 8442067 [details] [diff] [review]
> > > [02] Bug 1027030: Convert Bluetooth to use Bluedroid wrappers
> > > 
> > > Review of attachment 8442067 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Overall, it looks good to me.
> > 
> > We probably need to also change BluetoothHfpManager for hfp-fallback?
> 
> Hmm, it doesn't look like hfp-fallback has any dependencies in bluedroid.
Sorry, please ignore my comment.
Hi Shawn,

Thanks for your review. I fixed all issues, except for the bt_interface_t below. Could you further comment on that?

> 
> Only a few nits addressed here. I'm sorry for the late reply.
> 
> ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> @@ +12,5 @@
> > +struct interface_traits
> > +{ };
> > +
> > +//
> > +// Socket Profile
> 
> nit: Socket is not profile, can we change to 'Socket Interface'?

I fixed the misch-masch of names and used 'Interface' in all comments

> 
> @@ +368,5 @@
> > +
> > +//
> > +// Bluetooth Core Interface
> > +//
> > +
> 
> Good catch! Can you add one line comment for better understanding this marco?

Yeah, it's safer than the original code. I added a comment before the macro.

> @@ +410,5 @@
> > +    goto err_get_bluetooth_interface;
> > +  }
> > +
> > +  if (bt_interface->size != sizeof(*bt_interface)) {
> > +    BT_WARNING("interface of incorrect size");
> 
> We usually checked sizeof(bt_callback_t) to avoid missing registration, do
> we need to check |bt_interfce|? Or you intend to check size of bt_callback_t?

Not sure if I understand this comment. I think 'bt_interface->size' is the compiled size of Bluedroid's bt_interface_t structure. Comparing against 'sizeof(*bt_interface)' (mind the asterisk) increases the chance that our compiled code and the Bluedroid binary blob are compatible.

For callbacks, 'bt_callback_t' provides a similar mechanism.

Could you further comment on that?

> 
> @@ +421,5 @@
> > +err_bt_interface_size:
> > +err_get_bluetooth_interface:
> > +  err = device->close(device);
> > +  if (err)
> > +    BT_WARNING("close failed: %s", strerror(err));
> 
> super nit: Based on Gecko coding style, "Always brace controlled statements,
> even a single-line consequent of an if else else".

Fixed here, and everywhere else in the file.
Flags: needinfo?(shuang)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)

> Not sure if I understand this comment. I think 'bt_interface->size' is the
> compiled size of Bluedroid's bt_interface_t structure. Comparing against
> 'sizeof(*bt_interface)' (mind the asterisk) increases the chance that our
> compiled code and the Bluedroid binary blob are compatible.
> 
> For callbacks, 'bt_callback_t' provides a similar mechanism.
> 
OK, now I got your point. Please ignore my question.
Flags: needinfo?(shuang)
Created attachment 8447010 [details] [diff] [review]
[01] Bug 1027030: Wrap Bluedroid interfaces in classes (v2)

Changes since v1:

  - updated patch according to review comments
Attachment #8442065 - Attachment is obsolete: true
Attachment #8447010 - Flags: review+
Blocks: 1031185
https://hg.mozilla.org/mozilla-central/rev/f8f38cdc0b40
https://hg.mozilla.org/mozilla-central/rev/18dc239fe9de
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S5 (4july)
You need to log in before you can comment on or make changes to this bug.