Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1927803002:
arc: bluetooth: Add gatt client functionality (Closed)
|
Created:
4 years, 7 months ago by puthik_chromium Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), dmitrygr_chromium.org, Miao, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+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: Add gatt client functionality
Implement the Bluetooth Low Energy GATT client functionality.
- BLE Scan
- GATT connect/disconnect
- service/characteristic/descriptor discovery
- characteristic/descriptor read/write
- read remote rssi
- start/stop listen
- [Un]register for notification
BUG=b:28250518
TEST=Tested patches stack at http://ag/1092360
Committed: https://crrev.com/33ad83efd47da931442305068405891241df7f73
Cr-Commit-Position: refs/heads/master@{#400062}
Patch Set 1 #Patch Set 2 : Add gatt connect implementation #Patch Set 3 : Change API to Match N / Implement service discovery #Patch Set 4 : Add LE scan #Patch Set 5 : Implement listen / register for notification #Patch Set 6 : Add more code comment #
Total comments: 3
Patch Set 7 : Unblob the advertising data #Patch Set 8 : Change uuid to uint16 in BluetoothServiceData #
Total comments: 50
Patch Set 9 : Address rkc comment #Patch Set 10 : #
Total comments: 2
Patch Set 11 : manufacture -> manufacturer #
Total comments: 10
Patch Set 12 : #Patch Set 13 : Remove std:move for temporary #
Total comments: 22
Patch Set 14 : Address palmer / dcheng comment #
Total comments: 17
Patch Set 15 : Address palmer@ comment: add helper function to avoid dup code. #Patch Set 16 : Fix compile warning #
Total comments: 6
Patch Set 17 : refactor gatt permission to a const #
Messages
Total messages: 54 (15 generated)
puthik@chromium.org changed reviewers: + lhchavez@chromium.org
This CL will just add stub function. Implementation will be done in later CL. Will add ortuno@ for BT usage review and rickyz@ for mojo security review after get L-G-T-M for mojo
It's difficult for me to make a decision if the API is ok if there is no implementation :( What if we discover that there was a function that could be converted into the callback form? I'm also pretty sure that the security reviewer will want to see the implementation before blindly signing off.
Description was changed from ========== arc: bluetooth: Add mojom for gatt client Add mojo definition for GATT client functionality. Also add stub function to make it compile. BUG=b:28250518 TEST=build ========== to ========== arc: bluetooth: Add gatt client functionality Implement the Bluetooth Low Energy GATT client functionality. - BLE Scan - GATT connect/disconnect - service/characteristic/descriptor discovery - characteristic/descriptor read/write - read remote rssi - start/stop listen - [Un]register for notification BUG=b:28250518 TEST=Tested patches stack at http://ag/1092360 ==========
Will add ortuno@ for BT usage and rickyz for mojo security later
First pass. https://codereview.chromium.org/1927803002/diff/100001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/1927803002/diff/100001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:124: arc_bridge_service()->bluetooth_instance()->OnLEDeviceFound( Version check? Same with all other calls you introduce in this change. https://codereview.chromium.org/1927803002/diff/100001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1194: mojo::Array<uint8_t> ArcBluetoothBridge::GetAdvData( eek, sending a blob. Is there absolutely no way to make this a legit structure? Also, this doesn't seem to use any members of the class, can you move this to bluetooth_type_converters.h? https://codereview.chromium.org/1927803002/diff/100001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/1927803002/diff/100001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:252: mojo::Array<uint8_t> GetAdvData(device::BluetoothDevice* device) const; GetAdvertisingData.
puthik@google.com changed reviewers: + puthik@google.com, rkc@chromium.org
Add rkc@ for Bluetooth usage. (Will submit another one to address lhchavez comment soon)
Advertising data now send to android using an array of union instead of a blob. Also add version check.
Change the BluetoothServiceData to uint16 as lhchavez@ comment in http://ag/1025321
puthik@chromium.org changed reviewers: + rickyz@chromium.org
+rickyz for mojo security.
Mostly looks fine, I just have a lot of little nits :) Please do add tests. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:119: if (arc_bridge_service()->bluetooth_version() < 1) { s/1/kMinBtleVersion Here and other places where you do the version check. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:502: LOG(ERROR) << "Discovery session already running; leaving alone"; s/ERROR/WARNING https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:510: std::move(discovery_filter), s/discovery_filter/base::WrapUniqe(new BluetoothDiscoveryFilter(BluetoothDiscoveryFilter::Transport::TRANSPORT_LE)) Avoid the 2 temporaries. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:576: BluetoothDevice* device = Check if device is nullptr? Here and a few places below. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:587: mojom::BluetoothAddressPtr remote_addr1 = remote_addr.Clone(); remote_addr_clone would be cleaner than just adding a 1. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:614: weak_factory_.GetWeakPtr(), base::Passed(&remote_addr1))); as above. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:646: callback.Run(mojom::BluetoothGattStatus::GATT_FAILURE); Are there any better errors we can send up to Android other than a generic GATT_FAILURE? https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:653: adv_data->set_service_uuids( You should be able to create an advertisement with no data other than the type (the only required field). If for some reason this is broken, please file a bug assigned to me and mention it in a TODO(rkc) here. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:701: for (auto& service : device->GetGattServices()) { auto service https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:721: std::vector<BluetoothRemoteGattCharacteristic*> characteristics = const auto& characteristics = service->GetCharacteristics(); https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:735: for (auto& characteristic : characteristics) { auto characteristic https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:748: for (auto& descriptor : characteristic->GetDescriptors()) { auto descriptor https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:749: mojom::BluetoothGattDBElementPtr element = Naming all of these element is a scoping error waiting to happen. Rename these to 'service_element', 'characteristic_element' and 'descriptor_element' to avoid any scope conflicts. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:783: for (auto& s : device->GetGattServices()) { use std::find_if to find the service? See https://cs.chromium.org/chromium/src/device/bluetooth/dbus/bluetooth_gatt_app... Same with all the other places where you're finding an element in a vector. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:822: // same callback for both ReadGattCharacteristic and ReadGattDescriptor s/same/Same https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:869: BluetoothGattCharacteristic::Permission::PERMISSION_READ); There are several types of read permissions. Are we guaranteed to have a PERMISSION_READ every time we have, let's say PROPERTY_READ_ENCRYPTED? If so, leave the check, otherwise either remove the check or extend it to cover other type of read permissions. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:890: BluetoothGattCharacteristic::Permission::PERMISSION_WRITE); Same with write permissions. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:892: std::vector<uint8_t> new_value = value->value.To<std::vector<uint8_t>>(); don't need |new_value|. Here and below. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:973: DCHECK(characteristic); Why not log an error here and just return? Same with the Deregister function. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1201: for (auto& device : devices) { auto device https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1224: // However Chrome didn't expose AdvertiseFlag and ManufactureData. Please file a bug and add a TODO pointing to the bug to fix this. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1279: for (auto& device : devices) { auto device https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1316: for (auto& device : devices) { auto device https://codereview.chromium.org/1927803002/diff/140001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/140001/components/arc/common/... components/arc/common/bluetooth.mojom:133: enum BluetoothGattStatus { Document where the values that we're assigning to these enums are coming from. https://codereview.chromium.org/1927803002/diff/140001/components/arc/common/... components/arc/common/bluetooth.mojom:211: * handles. Here you're saying that they contain the starting and ending attribute handles, but later you're populating these with the first and last 'characteristic' handles. If this were the first and last attribute handles, then the last handle would be either a characteristic or descriptor handle. Either fix the implementation to match the comment, or change the comment to match the implementation. Whichever is the correct thing to do :)
Will add test in next patch. https://codereview.chromium.org/1927803002/diff/140001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/140001/components/arc/common/... components/arc/common/bluetooth.mojom:211: * handles. On 2016/06/07 at 02:34:31, rkc on vacation - slow reviews wrote: > Here you're saying that they contain the starting and ending attribute handles, but later you're populating these with the first and last 'characteristic' handles. If this were the first and last attribute handles, then the last handle would be either a characteristic or descriptor handle. > > Either fix the implementation to match the comment, or change the comment to match the implementation. Whichever is the correct thing to do :) The comment is correct because I copy from Android N.[1] So I changed the implementation to match the comment. Note that Android didn't use these 2 variables anywhere.[2] [1] http://cs/android/hardware/libhardware/include/hardware/bt_common_types.h?l=60 [2] http://cs/android/packages/apps/Bluetooth/src/com/android/bluetooth/gatt/Gatt...
Opal, please do mark all the addressed comments with Done, so it is easy to see that they have been addressed. I do see there are several comments that aren't addressed yet.
As this block bluetooth CTS for arc++. The unit test will be in separate CL at http://crrev.com/2046283003 (WIP) https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:119: if (arc_bridge_service()->bluetooth_version() < 1) { On 2016/06/07 02:34:31, rkc wrote: > s/1/kMinBtleVersion > Here and other places where you do the version check. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:502: LOG(ERROR) << "Discovery session already running; leaving alone"; On 2016/06/07 02:34:31, rkc wrote: > s/ERROR/WARNING Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:510: std::move(discovery_filter), On 2016/06/07 02:34:30, rkc wrote: > s/discovery_filter/base::WrapUniqe(new > BluetoothDiscoveryFilter(BluetoothDiscoveryFilter::Transport::TRANSPORT_LE)) > > Avoid the 2 temporaries. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:576: BluetoothDevice* device = On 2016/06/07 02:34:30, rkc wrote: > Check if device is nullptr? Here and a few places below. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:587: mojom::BluetoothAddressPtr remote_addr1 = remote_addr.Clone(); On 2016/06/07 02:34:29, rkc wrote: > remote_addr_clone would be cleaner than just adding a 1. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:614: weak_factory_.GetWeakPtr(), base::Passed(&remote_addr1))); On 2016/06/07 02:34:29, rkc wrote: > as above. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:646: callback.Run(mojom::BluetoothGattStatus::GATT_FAILURE); On 2016/06/07 02:34:31, rkc wrote: > Are there any better errors we can send up to Android other than a generic > GATT_FAILURE? No https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:653: adv_data->set_service_uuids( On 2016/06/07 02:34:31, rkc wrote: > You should be able to create an advertisement with no data other than the type > (the only required field). If for some reason this is broken, please file a bug > assigned to me and mention it in a TODO(rkc) here. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:701: for (auto& service : device->GetGattServices()) { On 2016/06/07 02:34:30, rkc wrote: > auto service Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:721: std::vector<BluetoothRemoteGattCharacteristic*> characteristics = On 2016/06/07 02:34:30, rkc wrote: > const auto& characteristics = service->GetCharacteristics(); Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:735: for (auto& characteristic : characteristics) { On 2016/06/07 02:34:31, rkc wrote: > auto characteristic Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:748: for (auto& descriptor : characteristic->GetDescriptors()) { On 2016/06/07 02:34:30, rkc wrote: > auto descriptor Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:749: mojom::BluetoothGattDBElementPtr element = On 2016/06/07 02:34:30, rkc wrote: > Naming all of these element is a scoping error waiting to happen. Rename these > to 'service_element', 'characteristic_element' and 'descriptor_element' to avoid > any scope conflicts. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:783: for (auto& s : device->GetGattServices()) { On 2016/06/07 02:34:30, rkc wrote: > use std::find_if to find the service? > See > https://cs.chromium.org/chromium/src/device/bluetooth/dbus/bluetooth_gatt_app... > > Same with all the other places where you're finding an element in a vector. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:822: // same callback for both ReadGattCharacteristic and ReadGattDescriptor On 2016/06/07 02:34:29, rkc wrote: > s/same/Same Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:869: BluetoothGattCharacteristic::Permission::PERMISSION_READ); On 2016/06/07 02:34:30, rkc wrote: > There are several types of read permissions. Are we guaranteed to have a > PERMISSION_READ every time we have, let's say PROPERTY_READ_ENCRYPTED? > If so, leave the check, otherwise either remove the check or extend it to cover > other type of read permissions. Reading BlueZ code, look like it is guaranteed. But can't find the text in Bluetooth Spec that said so. I think extending the permission check is right thing to do in this case. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:890: BluetoothGattCharacteristic::Permission::PERMISSION_WRITE); On 2016/06/07 02:34:29, rkc wrote: > Same with write permissions. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:892: std::vector<uint8_t> new_value = value->value.To<std::vector<uint8_t>>(); On 2016/06/07 02:34:31, rkc wrote: > don't need |new_value|. > Here and below. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:973: DCHECK(characteristic); On 2016/06/07 02:34:30, rkc wrote: > Why not log an error here and just return? Same with the Deregister function. Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1201: for (auto& device : devices) { On 2016/06/07 02:34:30, rkc wrote: > auto device Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1224: // However Chrome didn't expose AdvertiseFlag and ManufactureData. On 2016/06/07 02:34:30, rkc wrote: > Please file a bug and add a TODO pointing to the bug to fix this. > Done. http://crbug.com/618442 https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1279: for (auto& device : devices) { On 2016/06/07 02:34:29, rkc wrote: > auto device Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1316: for (auto& device : devices) { On 2016/06/07 02:34:31, rkc wrote: > auto device Done. https://codereview.chromium.org/1927803002/diff/140001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/140001/components/arc/common/... components/arc/common/bluetooth.mojom:133: enum BluetoothGattStatus { On 2016/06/07 02:34:31, rkc wrote: > Document where the values that we're assigning to these enums are coming from. Done.
lgtm
Friendly ping. Got Bluetooth l-g-t-m from rkc@ lhchavez@ Please review the arc usage. rickyz@ Please review mojo security.
Just an overall question about all the uint8_t blob arrays. Are all of those needed? Meaning that they are either arbitrary, unstructured buffers that are exposed that way by Chrome / consumed that way by Android. https://codereview.chromium.org/1927803002/diff/180001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/180001/components/arc/common/... components/arc/common/bluetooth.mojom:186: array<uint8> manufacture_data; Shouldn't this be |manufacturer_data|?
On 2016/06/09 20:59:13, Luis Héctor Chávez wrote: > Just an overall question about all the uint8_t blob arrays. Are all of those > needed? Meaning that they are either arbitrary, unstructured buffers that are > exposed that way by Chrome / consumed that way by Android. > > https://codereview.chromium.org/1927803002/diff/180001/components/arc/common/... > File components/arc/common/bluetooth.mojom (right): > > https://codereview.chromium.org/1927803002/diff/180001/components/arc/common/... > components/arc/common/bluetooth.mojom:186: array<uint8> manufacture_data; > Shouldn't this be |manufacturer_data|? The format is defined in Bluetooth spec[1] and Android is expected that data in the Bluetooth HAL callback. [1] http://screen/1OEkhkRLJ8a http://screen/1OEkhkRLJ8a
https://codereview.chromium.org/1927803002/diff/180001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/180001/components/arc/common/... components/arc/common/bluetooth.mojom:186: array<uint8> manufacture_data; On 2016/06/09 20:59:13, Luis Héctor Chávez wrote: > Shouldn't this be |manufacturer_data|? Done
Probably last questions from me. https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:155: uint8 is_primary; This looks like this is a boolean. https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:161: uint32 type; Can this be an enum? https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:312: [MinVersion=1] OnGetGattDB(BluetoothAddress remote_addr, Can this be called unrequested? Or can it be callback of GetGattDB?
https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:155: uint8 is_primary; On 2016/06/09 21:45:49, Luis Héctor Chávez wrote: > This looks like this is a boolean. This is a boolean but Android want it as uint8. So I will keep it as a uint8. Is that make sense? http://cs/android/hardware/libhardware/include/hardware/bt_gatt_types.h?l=43 https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:161: uint32 type; On 2016/06/09 21:45:49, Luis Héctor Chávez wrote: > Can this be an enum? It's defined in Android callback but Android does not use this at all. It is currently always set to 0. So it can't be an enum. I add this in mojo just in case for the future be cause I don't think we have [Extensible] equivalence for struct. https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:312: [MinVersion=1] OnGetGattDB(BluetoothAddress remote_addr, On 2016/06/09 21:45:49, Luis Héctor Chávez wrote: > Can this be called unrequested? Or can it be callback of GetGattDB? GetGattDB is the Android N API. It can be called unrequested when we upgrade to Android N. Currently in M, the Android HAL side will call GettGattDB when we get related API called and translate this to M API in the callback.
https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:155: uint8 is_primary; On 2016/06/09 22:12:54, puthik_chromium wrote: > On 2016/06/09 21:45:49, Luis Héctor Chávez wrote: > > This looks like this is a boolean. > > This is a boolean but Android want it as uint8. So I will keep it as a uint8. Is > that make sense? > http://cs/android/hardware/libhardware/include/hardware/bt_gatt_types.h?l=43 Sure, just add a comment explaining this. https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:161: uint32 type; On 2016/06/09 22:12:54, puthik_chromium wrote: > On 2016/06/09 21:45:49, Luis Héctor Chávez wrote: > > Can this be an enum? > > It's defined in Android callback but Android does not use this at all. It is > currently always set to 0. So it can't be an enum. I add this in mojo just in > case for the future be cause I don't think we have [Extensible] equivalence for > struct. structs are extensible by default as opposed to enums which have very aggressive bounds-checking. this bounds-checking causes disconnects, and [Extensible] removes the bounds-checkings. So i say you drop this for now and introduce it when we need it.
https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:155: uint8 is_primary; On 2016/06/09 22:17:49, Luis Héctor Chávez wrote: > On 2016/06/09 22:12:54, puthik_chromium wrote: > > On 2016/06/09 21:45:49, Luis Héctor Chávez wrote: > > > This looks like this is a boolean. > > > > This is a boolean but Android want it as uint8. So I will keep it as a uint8. > Is > > that make sense? > > http://cs/android/hardware/libhardware/include/hardware/bt_gatt_types.h?l=43 > > Sure, just add a comment explaining this. Done. https://codereview.chromium.org/1927803002/diff/200001/components/arc/common/... components/arc/common/bluetooth.mojom:161: uint32 type; On 2016/06/09 22:17:49, Luis Héctor Chávez wrote: > On 2016/06/09 22:12:54, puthik_chromium wrote: > > On 2016/06/09 21:45:49, Luis Héctor Chávez wrote: > > > Can this be an enum? > > > > It's defined in Android callback but Android does not use this at all. It is > > currently always set to 0. So it can't be an enum. I add this in mojo just in > > case for the future be cause I don't think we have [Extensible] equivalence > for > > struct. > > structs are extensible by default as opposed to enums which have very aggressive > bounds-checking. this bounds-checking causes disconnects, and [Extensible] > removes the bounds-checkings. > > So i say you drop this for now and introduce it when we need it. Done.
lgtm
puthik@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng From git log, look like dcheng is the person who review mojo security lately.
palmer@chromium.org changed reviewers: + palmer@chromium.org
I'll look at this more in depth later, but I wanted to get you my first-pass comments sooner rather than later. https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:93: // BluetoothUUID expects the format below with dash inserted. Typo: "...with the dashes inserted." https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:97: const int uuid_dash_pos[] = {8, 13, 18, 23}; Nit: size_t, not int. (Note that string::insert takes a size_t position: http://www.cplusplus.com/reference/string/string/insert/) https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:98: for (auto& pos : uuid_dash_pos) Nit: Just auto, not auto&. Don't need to take a reference to these size_ts. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:132: // Copy from Android API So how do we ensure that we correctly track the changes to the Android API? https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:156: // is_primary is not a boolean because Android define it as uint8_t. Typo: "defines" https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:165: uint32 len; Use the full word here: |length|. Is it really possible for the value to be 4 billion bytes long? https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:190: array<uint8> manufacturer_data; Why does |BluetoothGattValue| have a |length| field but these don't? https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:195: uint16 uuid_16bit; 16 bits doesn't really seem like a universally-unique ID. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:209: uint8 id; Nit: Indented too far.
+palmer, who has done a bunch of bluetooth reviews lately https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:96: base::HexEncode(address_bytes.data(), address_bytes.size()); This can be sent from ARC right? We can't assume anything about size here. This needs to be a StructTraits. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:317: array<BluetoothGattDBElement> db); Out of curiosity, why aren't we taking advantage of Mojo to just asynchronously return values in a callback? It's hard to see which methods are connected by looking at the interface definitions.
https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:93: // BluetoothUUID expects the format below with dash inserted. On 2016/06/13 19:07:18, palmer wrote: > Typo: "...with the dashes inserted." Done. https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:96: base::HexEncode(address_bytes.data(), address_bytes.size()); On 2016/06/13 20:19:27, dcheng wrote: > This can be sent from ARC right? We can't assume anything about size here. This > needs to be a StructTraits. The size is constant. Here is excerpt from mojom file. Or this is not good enough? struct BluetoothUUID { array<uint8, 16> uuid; }; https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:97: const int uuid_dash_pos[] = {8, 13, 18, 23}; On 2016/06/13 19:07:18, palmer wrote: > Nit: size_t, not int. (Note that string::insert takes a size_t position: > http://www.cplusplus.com/reference/string/string/insert/) Done. https://codereview.chromium.org/1927803002/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:98: for (auto& pos : uuid_dash_pos) On 2016/06/13 19:07:18, palmer wrote: > Nit: Just auto, not auto&. Don't need to take a reference to these size_ts. Done. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:132: // Copy from Android API On 2016/06/13 19:07:18, palmer wrote: > So how do we ensure that we correctly track the changes to the Android API? Android API will backward compatible. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:156: // is_primary is not a boolean because Android define it as uint8_t. On 2016/06/13 19:07:18, palmer wrote: > Typo: "defines" Done. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:165: uint32 len; On 2016/06/13 19:07:18, palmer wrote: > Use the full word here: |length|. > > Is it really possible for the value to be 4 billion bytes long? Removed. See below. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:190: array<uint8> manufacturer_data; On 2016/06/13 19:07:18, palmer wrote: > Why does |BluetoothGattValue| have a |length| field but these don't? I don't actually need the |length| field in BluetoothGattValue. Removed. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:195: uint16 uuid_16bit; On 2016/06/13 19:07:19, palmer wrote: > 16 bits doesn't really seem like a universally-unique ID. The BT spec and Android side want 16 bits data for this. It's not universally unique actually. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:209: uint8 id; On 2016/06/13 19:07:18, palmer wrote: > Nit: Indented too far. Done. https://codereview.chromium.org/1927803002/diff/240001/components/arc/common/... components/arc/common/bluetooth.mojom:317: array<BluetoothGattDBElement> db); On 2016/06/13 20:19:27, dcheng wrote: > Out of curiosity, why aren't we taking advantage of Mojo to just asynchronously > return values in a callback? It's hard to see which methods are connected by > looking at the interface definitions. On these functions, Chrome can call Android callback unprompted. Example use case: User use ChromeOS side UI to scan for LE device.
https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:534: DCHECK(addr); Nit/Bikeshed: I'd rather use complete words, e.g. |address|, than old-style terse abbreviations (|addr|). (You don't have to act on this comment if you don't want to.) https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:540: void ArcBluetoothBridge::OnGattConnectError( The only difference between this function and |OnGattConnected| is the true vs. false in the call to |OnLEConnectionStateChange|. It'd be good to find some way to factor out the common code between these functions. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:557: void ArcBluetoothBridge::OnGattDisconnected( This function is identical to |OnGattConnectError|. It'd be good to avoid this duplication (e.g. by changing |OnGattConnectError| to just call this function). https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:622: if (!HasBluetoothInstance()) Again, 622 if (!HasBluetoothInstance()) 623 return; 624 625 BluetoothDevice* device = 626 bluetooth_adapter_->GetDevice(remote_addr->To<std::string>()); 627 DCHECK(device); is repeated code. It'd be cool to factor that out into a common helper routine. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:642: scoped_refptr<BluetoothAdvertisement> adv) { Same nit/bikeshed as before: Disprefer terse names (especially in interfaces, like function signatures). https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:725: std::string last_id_str; Nit: You could reduce these lines: int last_id = GetIdFromServiceIdentifier(descriptors.size() > 0 ? descriptors.back()->GetIdentifier() : characteristics.back()->GetIdentifier()); (correctly "git cl format"'ed though of course). (See below for |GetIdFromServiceIdentifier|.) https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:732: std::stoi(last_id_str.substr(last_id_str.size() - 4), nullptr, 16); This is the 3rd instance of this not-quite-obvious line. (I say "not obvious" because it needed a comment on line 702.) Instead, I'd factor it out into a helper function with a self-explanatory name, like int GetIdFromServiceIdentifier(const std::string& identifier) or something like that. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:738: for (auto characteristic : characteristics) { Lines 738 – 766 are dense and repetetive. I suggest you look for ways to reduce repeated code and clarify, so that future readers can get up to speed more quickly.
https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:534: DCHECK(addr); On 2016/06/14 23:08:25, palmer wrote: > Nit/Bikeshed: I'd rather use complete words, e.g. |address|, than old-style > terse abbreviations (|addr|). > > (You don't have to act on this comment if you don't want to.) The |addr| name is used in Android side API. So I want to keep consistency with that. I will keep this in mind in the future. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:540: void ArcBluetoothBridge::OnGattConnectError( On 2016/06/14 23:08:25, palmer wrote: > The only difference between this function and |OnGattConnected| is the true vs. > false in the call to |OnLEConnectionStateChange|. It'd be good to find some way > to factor out the common code between these functions. Done. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:557: void ArcBluetoothBridge::OnGattDisconnected( On 2016/06/14 23:08:25, palmer wrote: > This function is identical to |OnGattConnectError|. It'd be good to avoid this > duplication (e.g. by changing |OnGattConnectError| to just call this function). Done. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:622: if (!HasBluetoothInstance()) On 2016/06/14 23:08:25, palmer wrote: > Again, > > 622 if (!HasBluetoothInstance()) > 623 return; > 624 > 625 BluetoothDevice* device = > 626 bluetooth_adapter_->GetDevice(remote_addr->To<std::string>()); > 627 DCHECK(device); > > is repeated code. It'd be cool to factor that out into a common helper routine. I don't agree with this. The code has very different purpose. And I think it does not make sense to cluster it together to a helper function. 622 if (!HasBluetoothInstance()) 623 return; This is used to check whether we have ARC++ or not. 625 BluetoothDevice* device = 626 bluetooth_adapter_->GetDevice(remote_addr->To<std::string>()); 627 DCHECK(device); This is used to check whether RemoteBluetooth device is valid or not. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:642: scoped_refptr<BluetoothAdvertisement> adv) { On 2016/06/14 23:08:26, palmer wrote: > Same nit/bikeshed as before: Disprefer terse names (especially in interfaces, > like function signatures). Done. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:725: std::string last_id_str; On 2016/06/14 23:08:25, palmer wrote: > Nit: You could reduce these lines: > > int last_id = GetIdFromServiceIdentifier(descriptors.size() > 0 ? > descriptors.back()->GetIdentifier() : characteristics.back()->GetIdentifier()); > > (correctly "git cl format"'ed though of course). (See below for > |GetIdFromServiceIdentifier|.) Done. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:732: std::stoi(last_id_str.substr(last_id_str.size() - 4), nullptr, 16); On 2016/06/14 23:08:25, palmer wrote: > This is the 3rd instance of this not-quite-obvious line. (I say "not obvious" > because it needed a comment on line 702.) > > Instead, I'd factor it out into a helper function with a self-explanatory name, > like > > int GetIdFromServiceIdentifier(const std::string& identifier) > > or something like that. Done. https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:738: for (auto characteristic : characteristics) { On 2016/06/14 23:08:25, palmer wrote: > Lines 738 – 766 are dense and repetetive. I suggest you look for ways to reduce > repeated code and clarify, so that future readers can get up to speed more > quickly. I added a helper function to allocate the element and fill in the common data.
Description was changed from ========== arc: bluetooth: Add gatt client functionality Implement the Bluetooth Low Energy GATT client functionality. - BLE Scan - GATT connect/disconnect - service/characteristic/descriptor discovery - characteristic/descriptor read/write - read remote rssi - start/stop listen - [Un]register for notification BUG=b:28250518 TEST=Tested patches stack at http://ag/1092360 ========== to ========== arc: bluetooth: Add gatt client functionality Implement the Bluetooth Low Energy GATT client functionality. - BLE Scan - GATT connect/disconnect - service/characteristic/descriptor discovery - characteristic/descriptor read/write - read remote rssi - start/stop listen - [Un]register for notification BUG=b:28250518 TEST=Tested patches stack at http://ag/1092360 ==========
https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/1927803002/diff/260001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:622: if (!HasBluetoothInstance()) On 2016/06/15 18:06:33, puthik_chromium wrote: > On 2016/06/14 23:08:25, palmer wrote: > > Again, > > > > 622 if (!HasBluetoothInstance()) > > 623 return; > > 624 > > 625 BluetoothDevice* device = > > 626 bluetooth_adapter_->GetDevice(remote_addr->To<std::string>()); > > 627 DCHECK(device); > > > > is repeated code. It'd be cool to factor that out into a common helper > routine. > > I don't agree with this. The code has very different purpose. And I think it > does not make sense to cluster it together to a helper function. > > 622 if (!HasBluetoothInstance()) > 623 return; > This is used to check whether we have ARC++ or not. > > 625 BluetoothDevice* device = > 626 bluetooth_adapter_->GetDevice(remote_addr->To<std::string>()); > 627 DCHECK(device); > This is used to check whether RemoteBluetooth device is valid or not. I agree with Opal. Additionally this code also serves to self-document what is expected for these functions. Pulling them into a common function will also reduce that clarity. Unless we want a name like CheckIfHasInstanceAndValidDevice, which seems like overkill :)
LGTM because I don't see any security concerns. But I strongly suggest you consider well-named functions instead of copy and pasting code chunks. They will very likely diverge over time, potentially creating bugs, and it's hard to read. https://codereview.chromium.org/1927803002/diff/300001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/1927803002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:725: for (auto characteristic : characteristics) { Nit: Could this (And line 733) be const auto&? https://codereview.chromium.org/1927803002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:836: DCHECK(characteristic->GetPermissions() & This is another repeated chunk of code could would be easier to read and to keep in sync (if there are changes in the future) if you factored out a utility function for it. (And with a self-documenting name.) https://codereview.chromium.org/1927803002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:916: descriptor->WriteRemoteDescriptor( So, a total of 4 functions that are all identical except for 1 different method being called on descriptor. Overall this code is hard to read due to its repetition.
https://codereview.chromium.org/1927803002/diff/300001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/1927803002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:725: for (auto characteristic : characteristics) { On 2016/06/15 23:10:16, palmer wrote: > Nit: Could this (And line 733) be const auto&? There is no need. |characteristics| is already a vector of pointer https://codereview.chromium.org/1927803002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:836: DCHECK(characteristic->GetPermissions() & On 2016/06/15 23:10:16, palmer wrote: > This is another repeated chunk of code could would be easier to read and to keep > in sync (if there are changes in the future) if you factored out a utility > function for it. (And with a self-documenting name.) I refactor the permission to be a const. This should be also easier to read and keep it in sync. https://codereview.chromium.org/1927803002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:916: descriptor->WriteRemoteDescriptor( On 2016/06/15 23:10:16, palmer wrote: > So, a total of 4 functions that are all identical except for 1 different method > being called on descriptor. Overall this code is hard to read due to its > repetition. What this 4 functions do is just passing the request to {read/write} {characteristic/descriptor} from android to BlueZ. I don't think we can avoid the repetition entirely. And with only 4 statement in a function, I think this is simple enough.
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, lhchavez@chromium.org, palmer@chromium.org Link to the patchset: https://codereview.chromium.org/1927803002/#ps320001 (title: "refactor gatt permission to a const")
The CQ bit was unchecked by puthik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927803002/320001
The CQ bit was checked by puthik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927803002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM stamp based on palmer's review
The CQ bit was checked by puthik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927803002/320001
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Add gatt client functionality Implement the Bluetooth Low Energy GATT client functionality. - BLE Scan - GATT connect/disconnect - service/characteristic/descriptor discovery - characteristic/descriptor read/write - read remote rssi - start/stop listen - [Un]register for notification BUG=b:28250518 TEST=Tested patches stack at http://ag/1092360 ========== to ========== arc: bluetooth: Add gatt client functionality Implement the Bluetooth Low Energy GATT client functionality. - BLE Scan - GATT connect/disconnect - service/characteristic/descriptor discovery - characteristic/descriptor read/write - read remote rssi - start/stop listen - [Un]register for notification BUG=b:28250518 TEST=Tested patches stack at http://ag/1092360 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Add gatt client functionality Implement the Bluetooth Low Energy GATT client functionality. - BLE Scan - GATT connect/disconnect - service/characteristic/descriptor discovery - characteristic/descriptor read/write - read remote rssi - start/stop listen - [Un]register for notification BUG=b:28250518 TEST=Tested patches stack at http://ag/1092360 ========== to ========== arc: bluetooth: Add gatt client functionality Implement the Bluetooth Low Energy GATT client functionality. - BLE Scan - GATT connect/disconnect - service/characteristic/descriptor discovery - characteristic/descriptor read/write - read remote rssi - start/stop listen - [Un]register for notification BUG=b:28250518 TEST=Tested patches stack at http://ag/1092360 Committed: https://crrev.com/33ad83efd47da931442305068405891241df7f73 Cr-Commit-Position: refs/heads/master@{#400062} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/33ad83efd47da931442305068405891241df7f73 Cr-Commit-Position: refs/heads/master@{#400062} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
