Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2101283003:
arc: bluetooth: Implement Gatt server request to read/write (Closed)
|
Created:
4 years, 5 months ago by puthik_chromium Modified:
4 years, 5 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@gs2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: bluetooth: Implement Gatt server request to read/write
Implement the following functionality
- Request to read/write Gatt characteristic/descriptor.
- Send response message back for the request read/write.
- Send reply back that response message is sent.
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.
4. Ryu can correctly read / write Gatt server object.
Committed: https://crrev.com/8dcc59fa56cb65f1f4cd48bb0e11eb1633c9ccf8
Cr-Commit-Position: refs/heads/master@{#407587}
Patch Set 1 #
Total comments: 10
Patch Set 2 : rebase #Patch Set 3 : Add GattServerCallbacks type to store the callbacks #
Total comments: 2
Patch Set 4 : Fix bug / More descriptive type name for template #
Total comments: 5
Patch Set 5 : Fix code comment #Patch Set 6 : GattObjectT -> GattAttribute #Patch Set 7 : run error_callback if offset < 0 #
Total comments: 11
Patch Set 8 : Rebase / Address lhchavez comment #Patch Set 9 : fix trybot error #Patch Set 10 : remove sendResponse #Patch Set 11 : fix comment in #10 #Patch Set 12 : Remove unused methods #Patch Set 13 : Remove unneeded Callback struct #
Total comments: 2
Patch Set 14 : More thread check, auto deduce template #Patch Set 15 : rebase #
Total comments: 9
Patch Set 16 : rebase #Patch Set 17 : Check maximum read/write offset #
Total comments: 2
Patch Set 18 : Change variable name #Patch Set 19 : Rebase to head #
Messages
Total messages: 91 (59 generated)
puthik@chromium.org changed reviewers: + rkc@chromium.org
WIP cl. Head up early to make sure that I use Gatt Server API correctly.
ejcaruso@chromium.org changed reviewers: + ejcaruso@chromium.org
https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:309: std::pair<ValueCallback, ErrorCallback>(successCallback, errorCallback)); Instead of using std::pair here and pair::first and pair::second below, would it be easier to read if we made a struct for this and called the members ::success and ::error? I was pretty confused by SendResponse for a bit.
https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:301: // We will use odd transaction_id for read and even id for write. Why? https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:307: gatt_read_callbacks_.emplace( Can't use map::emplace. https://chromium-cpp.appspot.com - notes section for emplace. https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:386: std::map<int32_t, std::pair<ValueCallback, ErrorCallback>> Can we have this aliased with using? It would also help understand what each of the fields are.
https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:301: // We will use odd transaction_id for read and even id for write. On 2016/07/01 22:51:32, rkc wrote: > Why? 1. To make sure that it does not repeat same number. 2. To make it easy to check that the transaction is read or write Feel free to suggest the alternative.
https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:301: // We will use odd transaction_id for read and even id for write. On 2016/07/01 22:56:09, puthik_chromium wrote: > On 2016/07/01 22:51:32, rkc wrote: > > Why? > > 1. To make sure that it does not repeat same number. > 2. To make it easy to check that the transaction is read or write > > Feel free to suggest the alternative. You got me wrong :) I was saying that the comment says what we're doing, but not why. i.e., just add a line also explaining why we're doing it.
https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:301: // We will use odd transaction_id for read and even id for write. On 2016/07/01 23:04:02, rkc wrote: > On 2016/07/01 22:56:09, puthik_chromium wrote: > > On 2016/07/01 22:51:32, rkc wrote: > > > Why? > > > > 1. To make sure that it does not repeat same number. > > 2. To make it easy to check that the transaction is read or write > > > > Feel free to suggest the alternative. > > You got me wrong :) I was saying that the comment says what we're doing, but not > why. i.e., just add a line also explaining why we're doing it. Done. https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:307: gatt_read_callbacks_.emplace( On 2016/07/01 22:51:32, rkc wrote: > Can't use map::emplace. > https://chromium-cpp.appspot.com - notes section for emplace. Done. https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:309: std::pair<ValueCallback, ErrorCallback>(successCallback, errorCallback)); On 2016/06/30 23:46:20, Eric Caruso wrote: > Instead of using std::pair here and pair::first and pair::second below, would it > be easier to read if we made a struct for this and called the members ::success > and ::error? I was pretty confused by SendResponse for a bit. Done. https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2101283003/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:386: std::map<int32_t, std::pair<ValueCallback, ErrorCallback>> On 2016/07/01 22:51:32, rkc wrote: > Can we have this aliased with using? It would also help understand what each of > the fields are. Done.
lgtm % comment https://codereview.chromium.org/2101283003/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2101283003/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:44: template <class T> Use more descriptive template arguments. Here and other places.
puthik@chromium.org changed reviewers: + lhchavez@chromium.org
https://codereview.chromium.org/2101283003/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2101283003/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:44: template <class T> On 2016/07/14 23:28:12, rkc wrote: > Use more descriptive template arguments. Here and other places. Done.
palmer@chromium.org changed reviewers: + palmer@chromium.org
(drive-by review) https://codereview.chromium.org/2101283003/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:330: gatt_handle_[gatt_obj->GetIdentifier()], offset, false /*is_long */); Is there code somewhere (in the recipient?) that validates that |offset| is within a reasonable range? https://codereview.chromium.org/2101283003/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1250: gatt_server_read_callbacks_[trans_id].success_callback.Run( When the body if an if/else block is more than 1 line, use curly braces.
https://codereview.chromium.org/2101283003/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:330: gatt_handle_[gatt_obj->GetIdentifier()], offset, false /*is_long */); On 2016/07/16 00:48:26, palmer wrote: > Is there code somewhere (in the recipient?) that validates that |offset| is > within a reasonable range? Look like Android API expected app to override this method and do the method check there. I can't find any range check in the Android HAL/Framework code[1]. Android Example code:[2] [1] https://developer.android.com/reference/android/bluetooth/BluetoothGattServer..., int, int, android.bluetooth.BluetoothGattCharacteristic) [2] https://android.googlesource.com/platform/system/bt/+/a4bd0d2/service/example... We can't do the range check by here because we don't know the length of characteristic/descriptor to read/write. https://codereview.chromium.org/2101283003/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1250: gatt_server_read_callbacks_[trans_id].success_callback.Run( On 2016/07/16 00:48:26, palmer wrote: > When the body if an if/else block is more than 1 line, use curly braces. Done.
Description was changed from ========== arc: bluetooth: Implement Gatt server request to read/write BUG=b:29612626 TEST=Build ========== to ========== arc: bluetooth: Implement Gatt server request to read/write Implement the following functionality - Request to read/write Gatt characteristic/descriptor. - Send response message back for the request read/write. - Send reply back that response message is sent. 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. 4. Ryu can correctly read / write Gatt server object. ==========
Rebase to latest version of previous patch
https://codereview.chromium.org/2101283003/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:330: gatt_handle_[gatt_obj->GetIdentifier()], offset, false /*is_long */); On 2016/07/18 21:30:32, puthik_chromium wrote: > On 2016/07/16 00:48:26, palmer wrote: > > Is there code somewhere (in the recipient?) that validates that |offset| is > > within a reasonable range? > > Look like Android API expected app to override this method and do the method > check there. I can't find any range check in the Android HAL/Framework code[1]. > Android Example code:[2] > [1] > https://developer.android.com/reference/android/bluetooth/BluetoothGattServer..., > int, int, android.bluetooth.BluetoothGattCharacteristic) > > [2] > https://android.googlesource.com/platform/system/bt/+/a4bd0d2/service/example... > > We can't do the range check by here because we don't know the length of > characteristic/descriptor to read/write. I also add sanity check to just call error_callback when offset < 0
I found at least one compile error (probably not on the compiler you're using, but definitely in at least one bot). Can you run `git cl format`, `git cl lint`, and trybots on all your changes? https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:330: gatt_handle_[gatt_obj->GetIdentifier()], offset, false /*is_long */); What happens if |gatt_obj->GetIdentifier()| is invalid? https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:348: arc_bridge_service()->bluetooth()->instance()->RequestGattWrite( Instead of doing this transaction magic, is it possible to make RequestGatt{Read,Write} have a callback? That should simplify stuff significantly. https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:141: const GattServerReadCallbacks& callbacks); override? https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:408: std::map<std::string, int32_t> gatt_handle_; std::unordered_map?
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: 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_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/2101283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:330: gatt_handle_[gatt_obj->GetIdentifier()], offset, false /*is_long */); On 2016/07/19 01:49:17, Luis Héctor Chávez wrote: > What happens if |gatt_obj->GetIdentifier()| is invalid? It should valid all the time because we are delegate only for the LocalGattServer that we create. Added DCHECK. https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:348: arc_bridge_service()->bluetooth()->instance()->RequestGattWrite( On 2016/07/19 01:49:16, Luis Héctor Chávez wrote: > Instead of doing this transaction magic, is it possible to make > RequestGatt{Read,Write} have a callback? That should simplify stuff > significantly. This require 3 calls. Chrome to Android: RequestGatt{Read,Write} Android to Chrome: SendResponse Chrome to Android: callback of SendResponse If we make RequestGatt{Read,Write} have a callback, it will be more complicate because 1) we still need to remember trannsaction_id in Android side, 2) SendResponse will be the callback of mojo function and we still need to do something for callback of that. https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:141: const GattServerReadCallbacks& callbacks); On 2016/07/19 01:49:17, Luis Héctor Chávez wrote: > override? Function defined in wrong place. Fixed. On{Characteristic,Descriptor}ReadRequest is the overridden function. This is the templace function to avoid duplicate code. https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:408: std::map<std::string, int32_t> gatt_handle_; On 2016/07/19 01:49:17, Luis Héctor Chávez wrote: > std::unordered_map? Done.
https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:348: arc_bridge_service()->bluetooth()->instance()->RequestGattWrite( On 2016/07/19 22:21:47, puthik_chromium wrote: > On 2016/07/19 01:49:16, Luis Héctor Chávez wrote: > > Instead of doing this transaction magic, is it possible to make > > RequestGatt{Read,Write} have a callback? That should simplify stuff > > significantly. > > This require 3 calls. > Chrome to Android: RequestGatt{Read,Write} > Android to Chrome: SendResponse > Chrome to Android: callback of SendResponse > > If we make RequestGatt{Read,Write} have a callback, it will be more complicate > because 1) we still need to remember trannsaction_id in Android side, 2) > SendResponse will be the callback of mojo function and we still need to do > something for callback of that. What do you need the callback of SendResponse for? You're unconditionally returning success, so it's probably easier if you remove SendResponse altogether.
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_...)
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/2101283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:348: arc_bridge_service()->bluetooth()->instance()->RequestGattWrite( On 2016/07/19 22:38:00, Luis Héctor Chávez wrote: > On 2016/07/19 22:21:47, puthik_chromium wrote: > > On 2016/07/19 01:49:16, Luis Héctor Chávez wrote: > > > Instead of doing this transaction magic, is it possible to make > > > RequestGatt{Read,Write} have a callback? That should simplify stuff > > > significantly. > > > > This require 3 calls. > > Chrome to Android: RequestGatt{Read,Write} > > Android to Chrome: SendResponse > > Chrome to Android: callback of SendResponse > > > > If we make RequestGatt{Read,Write} have a callback, it will be more complicate > > because 1) we still need to remember trannsaction_id in Android side, 2) > > SendResponse will be the callback of mojo function and we still need to do > > something for callback of that. > > What do you need the callback of SendResponse for? You're unconditionally > returning success, so it's probably easier if you remove SendResponse > altogether. Look like the response of SendResponse is optional in Android[1]. I thought it is mandatory. Will remove the SendResponse in the next patch after make sure that it work on real machine. [1] Android just debug print this. https://android.googlesource.com/platform/packages/apps/Bluetooth/+/master/sr...
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 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 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/2101283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:348: arc_bridge_service()->bluetooth()->instance()->RequestGattWrite( On 2016/07/19 23:05:43, puthik_chromium wrote: > On 2016/07/19 22:38:00, Luis Héctor Chávez wrote: > > On 2016/07/19 22:21:47, puthik_chromium wrote: > > > On 2016/07/19 01:49:16, Luis Héctor Chávez wrote: > > > > Instead of doing this transaction magic, is it possible to make > > > > RequestGatt{Read,Write} have a callback? That should simplify stuff > > > > significantly. > > > > > > This require 3 calls. > > > Chrome to Android: RequestGatt{Read,Write} > > > Android to Chrome: SendResponse > > > Chrome to Android: callback of SendResponse > > > > > > If we make RequestGatt{Read,Write} have a callback, it will be more > complicate > > > because 1) we still need to remember trannsaction_id in Android side, 2) > > > SendResponse will be the callback of mojo function and we still need to do > > > something for callback of that. > > > > What do you need the callback of SendResponse for? You're unconditionally > > returning success, so it's probably easier if you remove SendResponse > > altogether. > > Look like the response of SendResponse is optional in Android[1]. > I thought it is mandatory. Will remove the SendResponse in the next patch after > make sure that it work on real machine. > > [1] Android just debug print this. > https://android.googlesource.com/platform/packages/apps/Bluetooth/+/master/sr... Done. SendResponse removed
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.
friendly ping.
this looks much better :) components/arc/ lgtm (but please wait for security review) https://codereview.chromium.org/2101283003/diff/240001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/240001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:418: gatt_handle_[gatt_obj->GetIdentifier()], offset, false /*is_long */, nit: /* is_long */
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/2101283003/diff/240001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/240001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:418: gatt_handle_[gatt_obj->GetIdentifier()], offset, false /*is_long */, On 2016/07/20 23:09:31, Luis Héctor Chávez wrote: > nit: /* is_long */ 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/...) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:410: !CheckBluetoothInstanceVersion(kMinGattServerVersion) || offset < 0) { Is there an upper bound for offset? Or, if sanity is checked inside |arc_bridge_service()->bluetooth()->instance()->RequestGattRead|, is it redundant to also check here? https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:415: DCHECK(gatt_handle_.find(gatt_obj->GetIdentifier()) != gatt_handle_.end()); This could only happen by programmer error? I ask because I'm not sure what all callers this function has. https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:424: void ArcBluetoothBridge::OnGattAttributeWriteRequest( Same questions as above for this function.
https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:410: !CheckBluetoothInstanceVersion(kMinGattServerVersion) || offset < 0) { On 2016/07/22 01:46:10, palmer (OOO through 21 July) wrote: > Is there an upper bound for offset? Or, if sanity is checked inside > |arc_bridge_service()->bluetooth()->instance()->RequestGattRead|, is it > redundant to also check here? The upper bound is the size of the Gatt Attribute which we don't know at this time. |arc_bridge_service()->bluetooth()->instance()->RequestGattRead| will propagate to Android app that request GattServer. And it's up to app developer to do the boundary check there. Thay why I think it is better to also do the sanity check here. https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:415: DCHECK(gatt_handle_.find(gatt_obj->GetIdentifier()) != gatt_handle_.end()); On 2016/07/22 01:46:10, palmer (OOO through 21 July) wrote: > This could only happen by programmer error? I ask because I'm not sure what all > callers this function has. The caller of these function is On{Characteristic,Descriptor}ReadRequest which is override from device::BluetoothLocalGattService::Delegate. We are delegate to only GattAtrribute that we create. So this DCHECK will fail only by programmer error. https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:424: void ArcBluetoothBridge::OnGattAttributeWriteRequest( On 2016/07/22 01:46:10, palmer (OOO through 21 July) wrote: > Same questions as above for this function. Same answer as above
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/2101283003/diff/280001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:410: !CheckBluetoothInstanceVersion(kMinGattServerVersion) || offset < 0) { > The upper bound is the size of the Gatt Attribute which we don't know at this > time. I'm not sure I'm looking at the right thing, but this: https://community.nxp.com/thread/332030 seems to say that the maximum size for a GATT attribute is 512 bytes. https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:415: DCHECK(gatt_handle_.find(gatt_obj->GetIdentifier()) != gatt_handle_.end()); > The caller of these function is On{Characteristic,Descriptor}ReadRequest > which is override from device::BluetoothLocalGattService::Delegate. > We are delegate to only GattAtrribute that we create. So this DCHECK will fail > only by programmer error. Great, thanks.
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
https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/280001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:410: !CheckBluetoothInstanceVersion(kMinGattServerVersion) || offset < 0) { On 2016/07/22 21:49:11, palmer (OOO through 21 July) wrote: > > The upper bound is the size of the Gatt Attribute which we don't know at this > > time. > > I'm not sure I'm looking at the right thing, but this: > > https://community.nxp.com/thread/332030 > > seems to say that the maximum size for a GATT attribute is 512 bytes. Done. Found it in the spec.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM % nits. https://codereview.chromium.org/2101283003/diff/320001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/320001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:416: const LocalGattAttribute* gatt_obj, Nit: Might be better to name this |attribute|. (And in the next function.)
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
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/2101283003/diff/320001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2101283003/diff/320001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:416: const LocalGattAttribute* gatt_obj, On 2016/07/22 23:17:01, palmer (OOO through 21 July) wrote: > Nit: Might be better to name this |attribute|. (And in the next function.) Done.
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
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/2101283003/#ps340001 (title: "Change variable name")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2104043002 Patch 200001). 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...
The CQ bit was unchecked by commit-bot@chromium.org
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/...) 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
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/2101283003/#ps360001 (title: "Rebase to head")
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 request to read/write Implement the following functionality - Request to read/write Gatt characteristic/descriptor. - Send response message back for the request read/write. - Send reply back that response message is sent. 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. 4. Ryu can correctly read / write Gatt server object. ========== to ========== arc: bluetooth: Implement Gatt server request to read/write Implement the following functionality - Request to read/write Gatt characteristic/descriptor. - Send response message back for the request read/write. - Send reply back that response message is sent. 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. 4. Ryu can correctly read / write Gatt server object. ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Implement Gatt server request to read/write Implement the following functionality - Request to read/write Gatt characteristic/descriptor. - Send response message back for the request read/write. - Send reply back that response message is sent. 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. 4. Ryu can correctly read / write Gatt server object. ========== to ========== arc: bluetooth: Implement Gatt server request to read/write Implement the following functionality - Request to read/write Gatt characteristic/descriptor. - Send response message back for the request read/write. - Send reply back that response message is sent. 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. 4. Ryu can correctly read / write Gatt server object. Committed: https://crrev.com/8dcc59fa56cb65f1f4cd48bb0e11eb1633c9ccf8 Cr-Commit-Position: refs/heads/master@{#407587} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/8dcc59fa56cb65f1f4cd48bb0e11eb1633c9ccf8 Cr-Commit-Position: refs/heads/master@{#407587} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
