Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104043002:
arc: bluetooth: Implement Gatt Server add/delete/start/stop service (Closed)
|
Created:
4 years, 5 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@gs1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: bluetooth: Implement Gatt Server Add/Delete/Start/Stop service
Implement the following functionality
- Add Gatt service / characteristic / descriptor to Gatt Server
"Gatt included service" is currently not support in Chrome side.
- Start / Stop Gatt service
- Delete Gatt Service
BUG=629210
TEST=manual test using step below
1. Create custom Gatt server in nrF connect app.
2. Connect minnie to ryu via chrome interface.
3. Ryu can correctly see minnie's Gatt server object.
Committed: https://crrev.com/a8e840b9957e040e0e283cce8a9bbf27802d70b9
Cr-Commit-Position: refs/heads/master@{#407566}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Use running id / implement add descriptor #
Total comments: 2
Patch Set 3 : More descriptive type name for template #
Total comments: 13
Patch Set 4 : Fix code comment #Patch Set 5 : GattObjectT -> GattAttribute #
Total comments: 25
Patch Set 6 : Rebase / Address lhchavez comment #Patch Set 7 : fix trybot error #Patch Set 8 : rebase #
Total comments: 10
Patch Set 9 : More thread check, auto deduce template #Patch Set 10 : rebase #
Total comments: 23
Patch Set 11 : Address Palmer comment. Add max handle. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 64 (39 generated)
puthik@chromium.org changed reviewers: + rkc@chromium.org
WIP cl. Head up early to make sure that I use Gatt Server API correctly.
https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1068: int32_t id = ConvertGattIdentifierToId(identifier); This function won't work with IDs returned by gatt server functions. We use a guid as the last part of the ID. You can always just have an incremental handle count instead of trying to use the internal ID format (even for remote gatt objects). https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1104: // TODO(next patch version) Android send service handle not characteristic. TODO(puthik)? So this is because on Android, you first create a descriptor, then add it to a characteristic. This is the opposite from how our API was designed. This will require a bit of a hack to solve. Let's discuss this offline. https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:306: using GattStatusCallback = base::Callback<void(mojom::BluetoothGattStatus)>; Move alias definition to the top of the class. https://google.github.io/styleguide/cppguide.html#Declaration_Order
https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1068: int32_t id = ConvertGattIdentifierToId(identifier); On 2016/07/01 22:21:46, rkc wrote: > This function won't work with IDs returned by gatt server functions. We use a > guid as the last part of the ID. You can always just have an incremental handle > count instead of trying to use the internal ID format (even for remote gatt > objects). I added the incremental handle count. https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1104: // TODO(next patch version) Android send service handle not characteristic. On 2016/07/01 22:21:45, rkc wrote: > TODO(puthik)? > > So this is because on Android, you first create a descriptor, then add it to a > characteristic. This is the opposite from how our API was designed. This will > require a bit of a hack to solve. Let's discuss this offline. > I add the descriptor to the last added characteristic and that seems to be working correctly https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:306: using GattStatusCallback = base::Callback<void(mojom::BluetoothGattStatus)>; On 2016/07/01 22:21:46, rkc wrote: > Move alias definition to the top of the class. > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done.
https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1104: // TODO(next patch version) Android send service handle not characteristic. On 2016/07/14 19:01:47, puthik_chromium wrote: > On 2016/07/01 22:21:45, rkc wrote: > > TODO(puthik)? > > > > So this is because on Android, you first create a descriptor, then add it to a > > characteristic. This is the opposite from how our API was designed. This will > > require a bit of a hack to solve. Let's discuss this offline. > > > > I add the descriptor to the last added characteristic and that seems to be > working correctly Have you read through the Android code to verify this will always be the case? If so, please add a detailed comment here explaining what the Android code does and why we know that descriptors for characteristic will only be added after it. Links to Android code would be super useful. As far as I can tell, the Android code constructs a service Usually relying on current behavior is not a great way to write code but in this case, since we don't actually have a characteristic ID at all, at any point, we may not have an option. I'd like to us to make sure that we have done the due diligence to verify that this is always true :) https://codereview.chromium.org/2104043002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1057: template <class T> template <class Attribute>
On 2016/07/14 23:15:25, rkc wrote: > https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > https://codereview.chromium.org/2104043002/diff/1/components/arc/bluetooth/ar... > components/arc/bluetooth/arc_bluetooth_bridge.cc:1104: // TODO(next patch > version) Android send service handle not characteristic. > On 2016/07/14 19:01:47, puthik_chromium wrote: > > On 2016/07/01 22:21:45, rkc wrote: > > > TODO(puthik)? > > > > > > So this is because on Android, you first create a descriptor, then add it to > a > > > characteristic. This is the opposite from how our API was designed. This > will > > > require a bit of a hack to solve. Let's discuss this offline. > > > > > > > I add the descriptor to the last added characteristic and that seems to be > > working correctly > > Have you read through the Android code to verify this will always be the case? > If so, please add a detailed comment here explaining what the Android code does > and why we know that descriptors for characteristic will only be added after it. > Links to Android code would be super useful. As far as I can tell, the Android > code constructs a service > > Usually relying on current behavior is not a great way to write code but in this > case, since we don't actually have a characteristic ID at all, at any point, we > may not have an option. > > I'd like to us to make sure that we have done the due diligence to verify that > this is always true :) From Android code below, upper layer gives Android framework complete service structure with all Characteristic / Descriptor in it. And the framework add it one-by-one. I confirmed that the descriptor is belong to the last added characteristic. https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > https://codereview.chromium.org/2104043002/diff/20001/components/arc/bluetoot... > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > https://codereview.chromium.org/2104043002/diff/20001/components/arc/bluetoot... > components/arc/bluetooth/arc_bluetooth_bridge.cc:1057: template <class T> > template <class Attribute>
puthik@chromium.org changed reviewers: + lhchavez@chromium.org
https://codereview.chromium.org/2104043002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1057: template <class T> On 2016/07/14 23:15:25, rkc wrote: > template <class Attribute> Done. I think LocalGattObjectT sound better as we use it for LocalGatt[service/characteristic/descriptor]
puthik@chromium.org changed reviewers: + ejcaruso@chromium.org
lgtm % comments https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1116: // last characteristic that was added to the given service. This matches the Incomplete comment? Please add the Android source link here to show that this is how Android will always call AddDescriptor. https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:198: template <class RemoteGattObjectT> Gatt Objects are collectively called Attributes. Why do we want to create a term when one already exists? Also, the T is redundant. We typically use T in a template typename to denote an object of any arbitrary type (which is not the case here).
palmer@chromium.org changed reviewers: + palmer@chromium.org
(drive-by review) https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:868: // Common callback function for Gatt operation that only need to report Comment style: Common callback for GATT operations that only need to report GATT status back to Android. https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1066: gatt_identifier_[gatt_server_obj_handle] = identifier; Nit: You could save a line and write: gatt_identifier_[gatt_server_obj_handle] = gatt_obj->GetIdentifier(); https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1103: // Chrome automatically add CCC Descriptor to a characteristic when needed. Nit: Check your grammar and punctuation in comments generally. E.g.: Chrome automatically adds a CCC Descriptor to a characteristic when needed. We will generate a bogus handle for Android. https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1114: // Since Android API does not give information which characteristic is the Comment style: Since the Android API does not give information about which characteristic is the parent of the new descriptor, we assume that it would be the last characteristic that was added to the given service. This matches the ... [fill in the rest]
https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:868: // Common callback function for Gatt operation that only need to report On 2016/07/16 00:45:13, palmer wrote: > Comment style: > > Common callback for GATT operations that only need to report GATT status back to > Android. Done. https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1066: gatt_identifier_[gatt_server_obj_handle] = identifier; On 2016/07/16 00:45:13, palmer wrote: > Nit: You could save a line and write: > > gatt_identifier_[gatt_server_obj_handle] = gatt_obj->GetIdentifier(); We will reuse |identifier| in the next patch when introducing the map in the reverse direction for RequestRead / RequestWrite. https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1103: // Chrome automatically add CCC Descriptor to a characteristic when needed. On 2016/07/16 00:45:13, palmer wrote: > Nit: Check your grammar and punctuation in comments generally. E.g.: > > Chrome automatically adds a CCC Descriptor to a characteristic when needed. We > will generate a bogus handle for Android. Done. https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1114: // Since Android API does not give information which characteristic is the On 2016/07/16 00:45:13, palmer wrote: > Comment style: > > Since the Android API does not give information about which characteristic is > the parent of the new descriptor, we assume that it would be the last > characteristic that was added to the given service. This matches the ... [fill > in the rest] I accidentally deleted the last line of comment. Done. https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1116: // last characteristic that was added to the given service. This matches the On 2016/07/16 00:37:29, rkc wrote: > Incomplete comment? > Please add the Android source link here to show that this is how Android will > always call AddDescriptor. I accidentally deleted the last line of comment. Done.
Description was changed from ========== arc: bluetooth: Implement Gatt Server add/delete/start/stop service BUG=b:29612626 TEST=Build ========== to ========== arc: bluetooth: Implement Gatt Server Add/Delete/Start/Stop service Implement the following functionality - Add Gatt service / characteristic / descriptor to Gatt Server "Gatt included service" is currently not support in Chrome side. - Start / Stop Gatt service - Delete Gatt Service BUG=629210 TEST=manual test using step below 1. Create custom Gatt server in nrF connect app. 2. Connect minnie to ryu via chrome interface. 3. Ryu can correctly see minnie's Gatt server object. ==========
https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:198: template <class RemoteGattObjectT> On 2016/07/16 00:37:29, rkc wrote: > Gatt Objects are collectively called Attributes. Why do we want to create a term > when one already exists? > Also, the T is redundant. We typically use T in a template typename to denote an > object of any arbitrary type (which is not the case here). Unaddressed? Please rename "GattObject" and similar names to "Attribute" or similar.
https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104043002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:198: template <class RemoteGattObjectT> On 2016/07/18 21:48:16, rkc wrote: > On 2016/07/16 00:37:29, rkc wrote: > > Gatt Objects are collectively called Attributes. Why do we want to create a > term > > when one already exists? > > Also, the T is redundant. We typically use T in a template typename to denote > an > > object of any arbitrary type (which is not the case here). > > Unaddressed? > > Please rename "GattObject" and similar names to "Attribute" or similar. Missed this comment. Done
https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:758: mojom::BluetoothGattDBElementPtr ArcBluetoothBridge::CreateGattDBElement( anonymous namespace? https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:820: RemoteGattAttribute* ArcBluetoothBridge::FindGattAttributeFromUuid( Anonymous namespace? https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:825: [&](RemoteGattAttribute* attr) { return attr->GetUUID() == uuid; }); default captures are disallowed: https://chromium-cpp.appspot.com/#core-whitelist use [uuid](RemoteGattAttribute* attr) { ... } instead https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:870: void ArcBluetoothBridge::OnGattOperationDone( Definitely anonymous namespace. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:875: void ArcBluetoothBridge::OnGattOperationError( Anonymous namespace https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1061: LocalGattAttribute* gatt_attr) { This needs a DCHECK to validate it's running on the correct thread. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1064: gatt_server_attr_handle++; int32_t handle = ++gatt_server_attribute_next_handle_; https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1065: std::string identifier = gatt_attr->GetIdentifier(); can this be const std::string&? https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1107: gatt_server_attr_handle++; You should ideally modify this value in a single function. Please create an inline int32_t next_handle() { return ++gatt_server_attribute_next_handle_; } in the .h https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1113: bluetooth_adapter_->GetGattService(gatt_identifier_[service_handle]); What happens when |service_handle| is invalid? Same in all other places you do |mapping[service_handle]| https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:377: std::map<int32_t, std::string> gatt_identifier_; can these be std::unordered_map? https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:380: // Running number for handle to give to Android side. How about "Monotonically increasing value to use as handle"? https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:381: int32_t gatt_server_attr_handle = 0; gatt_server_attribute_next_handle_?
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:758: mojom::BluetoothGattDBElementPtr ArcBluetoothBridge::CreateGattDBElement( On 2016/07/19 01:42:17, Luis Héctor Chávez wrote: > anonymous namespace? Done. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:820: RemoteGattAttribute* ArcBluetoothBridge::FindGattAttributeFromUuid( On 2016/07/19 01:42:17, Luis Héctor Chávez wrote: > Anonymous namespace? Done. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:825: [&](RemoteGattAttribute* attr) { return attr->GetUUID() == uuid; }); On 2016/07/19 01:42:17, Luis Héctor Chávez wrote: > default captures are disallowed: > https://chromium-cpp.appspot.com/#core-whitelist > > use > > [uuid](RemoteGattAttribute* attr) { ... } > > instead Done. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:875: void ArcBluetoothBridge::OnGattOperationError( On 2016/07/19 01:42:17, Luis Héctor Chávez wrote: > Anonymous namespace Done. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1061: LocalGattAttribute* gatt_attr) { On 2016/07/19 01:42:17, Luis Héctor Chávez wrote: > This needs a DCHECK to validate it's running on the correct thread. Done. Also added thread check when dealing with observer. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1064: gatt_server_attr_handle++; On 2016/07/19 01:42:17, Luis Héctor Chávez wrote: > int32_t handle = ++gatt_server_attribute_next_handle_; Done. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1065: std::string identifier = gatt_attr->GetIdentifier(); On 2016/07/19 01:42:17, Luis Héctor Chávez wrote: > can this be const std::string&? Done. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1107: gatt_server_attr_handle++; On 2016/07/19 01:42:17, Luis Héctor Chávez wrote: > You should ideally modify this value in a single function. Please create an > inline > > int32_t next_handle() { return ++gatt_server_attribute_next_handle_; } > > in the .h Done. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1113: bluetooth_adapter_->GetGattService(gatt_identifier_[service_handle]); It shouldn't invalid according to upper layer android code. I think just add DCHECK is enough. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:377: std::map<int32_t, std::string> gatt_identifier_; On 2016/07/19 01:42:18, Luis Héctor Chávez wrote: > can these be std::unordered_map? Done. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:380: // Running number for handle to give to Android side. On 2016/07/19 01:42:18, Luis Héctor Chávez wrote: > How about "Monotonically increasing value to use as handle"? Done. https://codereview.chromium.org/2104043002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:381: int32_t gatt_server_attr_handle = 0; On 2016/07/19 01:42:18, Luis Héctor Chávez wrote: > gatt_server_attribute_next_handle_? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:96: const std::vector<RemoteGattAttribute*> gatt_attrs, should this be a const-reference? https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:889: FindGattAttributeFromUuid<BluetoothRemoteGattService>( nit: C++ might be able to autodeduce the template type based on the parameters, which should net you slightly more concise code. https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1077: service_id->is_primary, nullptr /*included_service */, nit: /* included service */ https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1080: CreateGattAttributeHandle<BluetoothLocalGattService>(service.get())); This (and all other uses of CreateGattAttributeHandle) should also be autodeduced their template types. https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1162: DCHECK(gatt_identifier_.find(service_handle) != gatt_identifier_.end()); Just for my peace of mind, can you add thread-checks in all the places you are using |gatt_identifier_| and the other maps?
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...
https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:96: const std::vector<RemoteGattAttribute*> gatt_attrs, On 2016/07/20 23:11:04, Luis Héctor Chávez wrote: > should this be a const-reference? Done. https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:889: FindGattAttributeFromUuid<BluetoothRemoteGattService>( On 2016/07/20 23:11:04, Luis Héctor Chávez wrote: > nit: C++ might be able to autodeduce the template type based on the parameters, > which should net you slightly more concise code. Done. https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1077: service_id->is_primary, nullptr /*included_service */, On 2016/07/20 23:11:04, Luis Héctor Chávez wrote: > nit: /* included service */ Done. https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1080: CreateGattAttributeHandle<BluetoothLocalGattService>(service.get())); On 2016/07/20 23:11:04, Luis Héctor Chávez wrote: > This (and all other uses of CreateGattAttributeHandle) should also be > autodeduced their template types. Done. https://codereview.chromium.org/2104043002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1162: DCHECK(gatt_identifier_.find(service_handle) != gatt_identifier_.end()); On 2016/07/20 23:11:04, Luis Héctor Chávez wrote: > Just for my peace of mind, can you add thread-checks in all the places you are > using |gatt_identifier_| and the other maps? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:71: // We want to convert last digit of the identifier to int in base 16 Nit: Correct the comment: "Convert the last 4 characters of |identifier| to an int, by interpreting them as hexadecimal digits." https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:73: return std::stoi(identifier.substr(identifier.size() - 4), nullptr, 16); If stoi fails (e.g. because the characters were not really hex digits), it will throw an exception. Is that expected/OK? (Maybe it is, I don't know.) https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:81: const RemoteGattAttribute* gatt_attr) { Style nit: Similar comment as before: My preference is to name this |attribute|. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:93: // Find Gatt Service/Characteristic/Descriptor from std::vector using UUID. I might rewrite this comment to use the argument names rather than their types. But, the names of the arguments and of the function can be made clear enough such that this comment might not be necessary. (See below.) https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:95: RemoteGattAttribute* FindGattAttributeFromUuid( I'd rename this to |FindGattAttributeByUuid|. (We select an element *from* a collection *by* its key.) https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:96: const std::vector<RemoteGattAttribute*>& gatt_attrs, Style nit: |attributes|. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:101: if (it == gatt_attrs.end()) Style nit: You could shorten this to: return it != attributes.end() ? *it : nullptr; but of course it's up to you. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1073: gatt_identifier_[handle] = identifier; Style nit: You could just do gatt_identifier_[handle] = attribute->GetIdentifier(); https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:342: int32_t CreateGattAttributeHandle(LocalGattAttribute* gatt_attr); Nit: I might name it |attribute| instead of |gatt_attr|. (I tend to disprefer terse abbreviations.) https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:355: std::unordered_map<std::string, Just out of curiosity: What's the reason for changing from map to unordered map? https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:358: // Map from android int handle to Chrome (BlueZ) string identifier. Nit: Capitalize Android. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:363: int32_t gatt_server_attribute_next_handle = 0; Nit: Should this identifier end with an _ ? Potential, speculative concern: Could a malicious caller cause this value to wrap into (presumably meaningless) negative values? Or even back to 0 and up, causing a mis-mapping between Android int handles and our string identifiers?
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...
https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:71: // We want to convert last digit of the identifier to int in base 16 On 2016/07/22 02:56:09, palmer (OOO through 21 July) wrote: > Nit: Correct the comment: "Convert the last 4 characters of |identifier| to an > int, by interpreting them as hexadecimal digits." Done. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:73: return std::stoi(identifier.substr(identifier.size() - 4), nullptr, 16); On 2016/07/22 02:56:10, palmer (OOO through 21 July) wrote: > If stoi fails (e.g. because the characters were not really hex digits), it will > throw an exception. Is that expected/OK? (Maybe it is, I don't know.) We rely on BlueZ implementation that the characters are not hex digits. If it's not true anymore, we need to change this code anyway. So it is better for this to be hard fail. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:81: const RemoteGattAttribute* gatt_attr) { On 2016/07/22 02:56:09, palmer (OOO through 21 July) wrote: > Style nit: Similar comment as before: My preference is to name this |attribute|. Done. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:93: // Find Gatt Service/Characteristic/Descriptor from std::vector using UUID. On 2016/07/22 02:56:10, palmer (OOO through 21 July) wrote: > I might rewrite this comment to use the argument names rather than their types. > But, the names of the arguments and of the function can be made clear enough > such that this comment might not be necessary. (See below.) Removed https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:95: RemoteGattAttribute* FindGattAttributeFromUuid( On 2016/07/22 02:56:09, palmer (OOO through 21 July) wrote: > I'd rename this to |FindGattAttributeByUuid|. (We select an element *from* a > collection *by* its key.) Done. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:96: const std::vector<RemoteGattAttribute*>& gatt_attrs, On 2016/07/22 02:56:10, palmer (OOO through 21 July) wrote: > Style nit: |attributes|. Done. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:101: if (it == gatt_attrs.end()) On 2016/07/22 02:56:09, palmer (OOO through 21 July) wrote: > Style nit: You could shorten this to: > > return it != attributes.end() ? *it : nullptr; > > but of course it's up to you. Done. This is more concise. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1073: gatt_identifier_[handle] = identifier; On 2016/07/22 02:56:10, palmer (OOO through 21 July) wrote: > Style nit: You could just do > > gatt_identifier_[handle] = attribute->GetIdentifier(); Won't fix. identifier will be used in next CL. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:355: std::unordered_map<std::string, On 2016/07/22 02:56:10, palmer (OOO through 21 July) wrote: > Just out of curiosity: What's the reason for changing from map to unordered map? lhchavez@ remind me this. We don't need the order of the item in map so unordered_map is better for performance. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:358: // Map from android int handle to Chrome (BlueZ) string identifier. On 2016/07/22 02:56:10, palmer (OOO through 21 July) wrote: > Nit: Capitalize Android. Done. https://codereview.chromium.org/2104043002/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:363: int32_t gatt_server_attribute_next_handle = 0; On 2016/07/22 02:56:11, palmer (OOO through 21 July) wrote: > Nit: Should this identifier end with an _ ? Done. > Potential, speculative concern: Could a malicious caller cause this value to > wrap into (presumably meaningless) negative values? Or even back to 0 and up, > causing a mis-mapping between Android int handles and our string identifiers? Good point. Add the maximum value check and return error if we go over that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
ping Need lhchavez@ owner review.
lgtm
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 Link to the patchset: https://codereview.chromium.org/2104043002/#ps200001 (title: "Address Palmer comment. Add max handle.")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2104023002 Patch 140001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
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.
Description was changed from ========== arc: bluetooth: Implement Gatt Server Add/Delete/Start/Stop service Implement the following functionality - Add Gatt service / characteristic / descriptor to Gatt Server "Gatt included service" is currently not support in Chrome side. - Start / Stop Gatt service - Delete Gatt Service BUG=629210 TEST=manual test using step below 1. Create custom Gatt server in nrF connect app. 2. Connect minnie to ryu via chrome interface. 3. Ryu can correctly see minnie's Gatt server object. ========== to ========== arc: bluetooth: Implement Gatt Server Add/Delete/Start/Stop service Implement the following functionality - Add Gatt service / characteristic / descriptor to Gatt Server "Gatt included service" is currently not support in Chrome side. - Start / Stop Gatt service - Delete Gatt Service BUG=629210 TEST=manual test using step below 1. Create custom Gatt server in nrF connect app. 2. Connect minnie to ryu via chrome interface. 3. Ryu can correctly see minnie's Gatt server object. ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Implement Gatt Server Add/Delete/Start/Stop service Implement the following functionality - Add Gatt service / characteristic / descriptor to Gatt Server "Gatt included service" is currently not support in Chrome side. - Start / Stop Gatt service - Delete Gatt Service BUG=629210 TEST=manual test using step below 1. Create custom Gatt server in nrF connect app. 2. Connect minnie to ryu via chrome interface. 3. Ryu can correctly see minnie's Gatt server object. ========== to ========== arc: bluetooth: Implement Gatt Server Add/Delete/Start/Stop service Implement the following functionality - Add Gatt service / characteristic / descriptor to Gatt Server "Gatt included service" is currently not support in Chrome side. - Start / Stop Gatt service - Delete Gatt Service BUG=629210 TEST=manual test using step below 1. Create custom Gatt server in nrF connect app. 2. Connect minnie to ryu via chrome interface. 3. Ryu can correctly see minnie's Gatt server object. Committed: https://crrev.com/a8e840b9957e040e0e283cce8a9bbf27802d70b9 Cr-Commit-Position: refs/heads/master@{#407566} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/a8e840b9957e040e0e283cce8a9bbf27802d70b9 Cr-Commit-Position: refs/heads/master@{#407566} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
