Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2244703005:
arc: bluetooth: Fix advertised uuid (Closed)
|
Created:
4 years, 4 months ago by puthik_chromium Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: bluetooth: Fix advertised uuid
device->GetUUIDs() is the correct way to get the advertised uuid
for recreate advertise data.
BUG=None
TEST=Build
Committed: https://crrev.com/6de80cb2538a41d1144cae3622011ee2e17c1119
Cr-Commit-Position: refs/heads/master@{#412065}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix service dat, fix test, add test #
Total comments: 5
Dependent Patchsets: Messages
Total messages: 23 (10 generated)
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
puthik@chromium.org changed reviewers: + lhchavez@chromium.org, ortuno@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
This seems to be causing crashes in tests.
https://codereview.chromium.org/2244703005/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (left): https://codereview.chromium.org/2244703005/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1545: BluetoothDevice::UUIDList uuid_list = device->GetServiceDataUUIDs(); There is more to fix in this function. I would recommend reading on Advertisement Data a bit[1]. Let me know if you have any questions [1] https://www.bluetooth.org/en-us/specification/assigned-numbers/generic-access...
https://codereview.chromium.org/2244703005/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (left): https://codereview.chromium.org/2244703005/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1545: BluetoothDevice::UUIDList uuid_list = device->GetServiceDataUUIDs(); On 2016/08/15 15:46:57, ortuno wrote: > There is more to fix in this function. I would recommend reading on > Advertisement Data a bit[1]. Let me know if you have any questions > > [1] > https://www.bluetooth.org/en-us/specification/assigned-numbers/generic-access... Sorry. I was careless. Fixed.
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
defer to ortuno@ https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1545: BluetoothDevice::UUIDList uuid_list = device->GetUUIDs(); nit: const BluetoothDevice::UUIDList&
https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1545: BluetoothDevice::UUIDList uuid_list = device->GetUUIDs(); On 2016/08/15 21:23:12, Luis Héctor Chávez wrote: > nit: const BluetoothDevice::UUIDList& ignore this, I double checked and the code is correct as-is.
https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1569: // Convert xxxxyyyy-xxxx-xxxx-xxxx-xxxxxxxxxxxx to int16 yyyy Out of curiosity: Does android really expect only an int16? This could cause unexpected collisions if true e.g.: e5b0180d-d3a3-45c1-a019-ea3c00d8d739 <- Custom UUID 0000180d-0000-1000-8000-00805F9B34FB <- Standard Heart Rate Service Both of these would be seen as a Heart Rate Service...
https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1569: // Convert xxxxyyyy-xxxx-xxxx-xxxx-xxxxxxxxxxxx to int16 yyyy On 2016/08/15 21:27:09, ortuno wrote: > Out of curiosity: Does android really expect only an int16? This could cause > unexpected collisions if true e.g.: > > > e5b0180d-d3a3-45c1-a019-ea3c00d8d739 <- Custom UUID > 0000180d-0000-1000-8000-00805F9B34FB <- Standard Heart Rate Service > > Both of these would be seen as a Heart Rate Service... Unfortunately yes[1]. The advertised service uuid is also need to be in uuid16[2]. (This will be in another patch) [1] https://android.googlesource.com/platform/frameworks/base/+/android-n-preview... [2] https://android.googlesource.com/platform/packages/apps/Bluetooth/+/android-n...
lgtm https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2244703005/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1569: // Convert xxxxyyyy-xxxx-xxxx-xxxx-xxxxxxxxxxxx to int16 yyyy On 2016/08/15 at 21:32:15, puthik_chromium wrote: > On 2016/08/15 21:27:09, ortuno wrote: > > Out of curiosity: Does android really expect only an int16? This could cause > > unexpected collisions if true e.g.: > > > > > > e5b0180d-d3a3-45c1-a019-ea3c00d8d739 <- Custom UUID > > 0000180d-0000-1000-8000-00805F9B34FB <- Standard Heart Rate Service > > > > Both of these would be seen as a Heart Rate Service... > > Unfortunately yes[1]. The advertised service uuid is also need to be in uuid16[2]. (This will be in another patch) > > [1] https://android.googlesource.com/platform/frameworks/base/+/android-n-preview... > [2] https://android.googlesource.com/platform/packages/apps/Bluetooth/+/android-n... *sigh* ok
lgtm
The CQ bit was unchecked by puthik@chromium.org
The CQ bit was checked by puthik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Fix advertised uuid device->GetUUIDs() is the correct way to get the advertised uuid for recreate advertise data. BUG=None TEST=Build ========== to ========== arc: bluetooth: Fix advertised uuid device->GetUUIDs() is the correct way to get the advertised uuid for recreate advertise data. BUG=None TEST=Build Committed: https://crrev.com/6de80cb2538a41d1144cae3622011ee2e17c1119 Cr-Commit-Position: refs/heads/master@{#412065} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6de80cb2538a41d1144cae3622011ee2e17c1119 Cr-Commit-Position: refs/heads/master@{#412065} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
