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)
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)
12.66 KB,
patch
|
brsun
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8605747 -
Flags: review?(joliu)
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8605747 -
Attachment is obsolete: true
Attachment #8614597 -
Flags: review?(joliu)
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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 6•9 years ago
|
||
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•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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)
Updated•9 years ago
|
Assignee: nobody → brsun
Comment 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8628199 -
Attachment is obsolete: true
Attachment #8628607 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9398d5778834
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/3770ec3a97fa
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3770ec3a97fa
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S2 (10Jul)
You need to log in
before you can comment on or make changes to this bug.
Description
•