Closed Bug 1027030 Opened 7 years ago Closed 7 years ago

[Bluetooth] Wrap Bluedroid interface

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S5 (4july)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(2 files, 1 obsolete file)

We need to wrap the Bluedroid interface for converting it to an asynchronous implementation.
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)
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)
Changes since v1:

  - updated patch according to review comments
Attachment #8442065 - Attachment is obsolete: true
Attachment #8447010 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f8f38cdc0b40
https://hg.mozilla.org/mozilla-central/rev/18dc239fe9de
Status: ASSIGNED → RESOLVED
Closed: 7 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.