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.

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.

(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

(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.

(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.

Changes since v1: - updated patch according to review comments

Thanks, Shawn! https://hg.mozilla.org/integration/b2g-inbound/rev/f8f38cdc0b40 https://hg.mozilla.org/integration/b2g-inbound/rev/18dc239fe9de https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=18dc239fe9de

https://hg.mozilla.org/mozilla-central/rev/f8f38cdc0b40 https://hg.mozilla.org/mozilla-central/rev/18dc239fe9de