1164387 - [bluetooth2][gatt] Use |btgatt_client_interface_t.get_device_type| to get a proper device type.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

This is a follow up for bug 1163912.

Thanks for Jocelyn's reminder, bluedroid has one function to query the device type directly from Gatt client API. So that we can use it instead of hard-coding the value by ourselves.

Comment 1

[Bruce Sun [:brsun]]

Attachment: bug1164387_gatt_client_get_device_type.patch

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8605747 [details] [diff] [review]
bug1164387_gatt_client_get_device_type.patch

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

Hi Bruce,

My main concern here is will this synchronize call still work if we adopt daemon?

Thanks,
Jocelyn

Comment 3

[Bruce Sun [:brsun]]

Attachment: bug1164387_gatt_client_get_device_type.v2.patch

Comment 4

[Bruce Sun [:brsun]]

Attachment: bug1164387_gatt_client_get_device_type.v3.patch

Update daemon part to do unpack conversion from int8_t to BluetoothTypeOfDevice.

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8615114 [details] [diff] [review]
bug1164387_gatt_client_get_device_type.v3.patch

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

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +884,5 @@
> +
> +  nsresult
> +  operator () (BluetoothTypeOfDevice& aArg1) const
> +  {
> +    /* Read address */

Read device type

@@ +886,5 @@
> +  operator () (BluetoothTypeOfDevice& aArg1) const
> +  {
> +    /* Read address */
> +    nsresult rv = UnpackPDU(
> +      GetPDU(), UnpackConversion<int8_t, BluetoothTypeOfDevice>(aArg1));

Shouldn't it be uint8_t here?
https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-msg.h#n917

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +1407,5 @@
>        new ConnectResultHandler(client));
>    }
>  }
>  
> +class BluetoothGattManager::ScanDeviceTypeResultHandler final

I would prefer to have a GetDeviceType function and GetDeviceTypeResultHandler for both general GetDeviceType operations and the scan case instead of only handles the scan case here.
What do you think?

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8615114 [details] [diff] [review]
bug1164387_gatt_client_get_device_type.v3.patch

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

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +1407,5 @@
>        new ConnectResultHandler(client));
>    }
>  }
>  
> +class BluetoothGattManager::ScanDeviceTypeResultHandler final

After having an offline discussion, I revisit this part and think it is indeed better to have a special result handler since this case is not quite trivial.
Though it's a bit of weird that we don't have a general case here, it makes sense since we didn't expose/use this operation at all.

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8615114 [details] [diff] [review]
bug1164387_gatt_client_get_device_type.v3.patch

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

Sorry for the delay here.
r=me after comments addressed.

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothGattHALInterface.cpp
@@ +69,5 @@
> +  Arg1 arg1;
> +
> +  nsresult rv = Convert(aArg1, arg1);
> +
> +  if (NS_FAILED(rv)) {

Suggest to revise similar as

if (NS_FAILED(rv) || aStatus != STATUS_SCCESS) {
  // dispatch error runnable
} else {
  // dispatch ResultRunnable
}

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +1466,2 @@
>  
> +  // Distribute signal for "LeDeviceFound" after we know the corresponding

nit: Distribute "LeDeviceFound" signal ...

Comment 8

[Bruce Sun [:brsun]]

Attachment: bug1164387_gatt_client_get_device_type.v4.patch

Address most of the suggestions on comment 5, comment 6, comment 7.

> Suggest to revise similar as
> 
> if (NS_FAILED(rv) || aStatus != STATUS_SCCESS) {
>   // dispatch error runnable
> } else {
>   // dispatch ResultRunnable
> }
The order of if-conditions are changed a little bit in this patch. Error cases are not merged because the error status are different for |aStatus != STATUS_SCCESS| case and |NS_FAILED(rv)| case.

Hi Jocelyn,
Would you mind having a glance on it to see if those if-conditions still needs to be improved?

Comment 9

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8628199 [details] [diff] [review]
bug1164387_gatt_client_get_device_type.v4.patch

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

r=me, thanks!

Comment 10

[Bruce Sun [:brsun]]

Attachment: bug1164387_gatt_client_get_device_type.commit.patch

Comment 11

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9398d5778834

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/3770ec3a97fa

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3770ec3a97fa