Closed Bug 1164387 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
FxOS-S2 (10Jul)
Tracking Status
firefox42 --- fixed

People

(Reporter: brsun, Assigned: brsun)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attachment #8605747 - Flags: review?(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
Attachment #8605747 - Flags: review?(joliu)
Blocks: 1165848
Attachment #8605747 - Attachment is obsolete: true
Attachment #8614597 - Flags: review?(joliu)
Update daemon part to do unpack conversion from int8_t to BluetoothTypeOfDevice.
Attachment #8614597 - Attachment is obsolete: true
Attachment #8614597 - Flags: review?(joliu)
Attachment #8615114 - Flags: review?(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?
Attachment #8615114 - Flags: review?(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 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 ...
Attachment #8615114 - Flags: review+
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?
Attachment #8615114 - Attachment is obsolete: true
Attachment #8628199 - Flags: review?(joliu)
Assignee: nobody → brsun
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!
Attachment #8628199 - Flags: review?(joliu) → review+
Attachment #8628199 - Attachment is obsolete: true
Attachment #8628607 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3770ec3a97fa
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S2 (10Jul)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: