Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1739383002:
Implement read & write remote GATT characteristic value for Windows (Closed)
DescriptionImplement read & write remote GATT characteristic value for Windows
This CL implements read & write remote GATT characteristic value for Windows and related unit tests.
BUG=579202
Committed: https://crrev.com/50818ce5049e84526cdadfe11a0ea08405b5c036
Cr-Commit-Position: refs/heads/master@{#379103}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : rebase #Patch Set 5 : rename #Patch Set 6 : #Patch Set 7 : rename #Patch Set 8 : fix conversion from 'size_t' to 'ULONG' error on trybot #
Total comments: 12
Patch Set 9 : address comments #Patch Set 10 : format #
Total comments: 4
Patch Set 11 : address comments #
Total comments: 6
Patch Set 12 : Fix unit test error for Android #Patch Set 13 : address comments #
Total comments: 1
Messages
Total messages: 64 (30 generated)
Description was changed from ========== pass all tests draft one BUG= ========== to ========== Implement read/write remote value in BluetoothRemoteGattCharacteristicWin This CL implements read/write remote value in BluetoothRemoteGattCharacteristicWin and related unit tests. BUG=579202 ==========
Description was changed from ========== Implement read/write remote value in BluetoothRemoteGattCharacteristicWin This CL implements read/write remote value in BluetoothRemoteGattCharacteristicWin and related unit tests. BUG=579202 ========== to ========== Implement read/write remote GATT characteristic value for Windows This CL implements read/write remote GATT characteristic value for Windows and related unit tests. BUG=579202 ==========
Description was changed from ========== Implement read/write remote GATT characteristic value for Windows This CL implements read/write remote GATT characteristic value for Windows and related unit tests. BUG=579202 ========== to ========== Implement read & write remote GATT characteristic value for Windows This CL implements read & write remote GATT characteristic value for Windows and related unit tests. BUG=579202 ==========
Patchset #2 (id:20001) has been deleted
Hi Vincent, PTAL. This is the next CL of https://codereview.chromium.org/1728163006/. I've tried to keep it small and done self review. Hope it could make the review easier.
Hi Vincent, PTAL. This is the next CL of https://codereview.chromium.org/1728163006/. I've tried to keep it small and done self review. Hope it could make the review easier.
gogerald@chromium.org changed reviewers: + scheib@chromium.org
Hi Vincent, PTAL. This is the next CL of https://codereview.chromium.org/1728163006/. I've tried to keep it small and done self review. Hope it could make the review easier.
gogerald@chromium.org changed reviewers: + rogerta@chromium.org
Hi Roger, thanks for taking a look.
Looks good, one question below. https://codereview.chromium.org/1739383002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_win_fake.h (right): https://codereview.chromium.org/1739383002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_win_fake.h:76: PBTH_LE_GATT_CHARACTERISTIC_VALUE value) = 0; Should this be const?
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/120001
https://codereview.chromium.org/1739383002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_win_fake.h (right): https://codereview.chromium.org/1739383002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_win_fake.h:76: PBTH_LE_GATT_CHARACTERISTIC_VALUE value) = 0; On 2016/03/01 15:22:54, Roger Tawa wrote: > Should this be const? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:473: characteristic2_->ReadRemoteCharacteristic( This changes the test meaning, in that two read characteristics are not pending at the same time before they start to have responses. Is this change necessary to make windows work? What happens with the previous test logic? If this change is to make sure an edge condition is tested consider duplicating the testing into two related tests. https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:507: SimulateGattCharacteristicWrite(characteristic1_); Similar to above, this changes the test logic -- explain why the change, what the behavior before change is. If this change is to make sure an edge condition is tested consider duplicating the testing into two related tests. https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:167: virtual HRESULT ReadTheValueOfACharacteristic( ReadCharacteristic or ReadCharacteristicValue https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:174: virtual HRESULT WriteTheValueOfACharacteristic( WriteCharacteristic or WriteCharacteristicValue https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:156: read_remote_characteristic_value_callbacks_.push_back( Android is supporting only one outstanding read operation at a time. What if the device returns a counter value, [1,2,3,4,5,...] for each read. If code calls 3 reads and then the first one is returned, then 3 more, it would receive [1,1,1,4,4,4,...]. What do you think of only one callback being stored per characteristic and returning GATT_ERROR_IN_PROGRESS otherwise? https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:292: for (const auto& callback : read_remote_characteristic_value_callbacks_) When running callbacks always clear the member variable of the callback in case the callback ends up being reentrant: OnReadRemoteCharacteristicValueCallback calls CallbackFoo calls ReadRemoteCharacteristic changes read_remote_characteristic_value_callbacks_
Patchset #9 (id:200001) has been deleted
Patchset #9 (id:220001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/240001
gogerald@chromium.org changed reviewers: - rogerta@chromium.org
https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:473: characteristic2_->ReadRemoteCharacteristic( On 2016/03/02 05:59:05, scheib wrote: > This changes the test meaning, in that two read characteristics are not pending > at the same time before they start to have responses. Is this change necessary > to make windows work? What happens with the previous test logic? If this change > is to make sure an edge condition is tested consider duplicating the testing > into two related tests. Done. https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:507: SimulateGattCharacteristicWrite(characteristic1_); On 2016/03/02 05:59:05, scheib wrote: > Similar to above, this changes the test logic -- explain why the change, what > the behavior before change is. If this change is to make sure an edge condition > is tested consider duplicating the testing into two related tests. Done. https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:167: virtual HRESULT ReadTheValueOfACharacteristic( On 2016/03/02 05:59:05, scheib wrote: > ReadCharacteristic or > ReadCharacteristicValue Done. https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:174: virtual HRESULT WriteTheValueOfACharacteristic( On 2016/03/02 05:59:05, scheib wrote: > WriteCharacteristic or > WriteCharacteristicValue Done. https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:156: read_remote_characteristic_value_callbacks_.push_back( On 2016/03/02 05:59:05, scheib wrote: > Android is supporting only one outstanding read operation at a time. What if the > device returns a counter value, [1,2,3,4,5,...] for each read. If code calls 3 > reads and then the first one is returned, then 3 more, it would receive > [1,1,1,4,4,4,...]. What do you think of only one callback being stored per > characteristic and returning GATT_ERROR_IN_PROGRESS otherwise? Done. https://codereview.chromium.org/1739383002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:292: for (const auto& callback : read_remote_characteristic_value_callbacks_) On 2016/03/02 05:59:05, scheib wrote: > When running callbacks always clear the member variable of the callback in case > the callback ends up being reentrant: > > OnReadRemoteCharacteristicValueCallback > calls CallbackFoo > calls ReadRemoteCharacteristic > changes read_remote_characteristic_value_callbacks_ Done.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/260001
https://codereview.chromium.org/1739383002/diff/260001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.h (right): https://codereview.chromium.org/1739383002/diff/260001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.h:107: read_remote_characteristic_value_callbacks_; optional, but those names seem long vs: read_callbacks_ write_callbacks_ https://codereview.chromium.org/1739383002/diff/260001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_win.cc (right): https://codereview.chromium.org/1739383002/diff/260001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.cc:435: if (task.ToString().find(function, 0) != std::string::npos) This seems to be too brittle and indirect. The task string is coming from a the task posting function, which is too indirect an implementation detail I think. We have concrete feedback mechanisms in this test fixture: callback_count_/error_callback_count_ or gatt_read_characteristic_attempts_ gatt_write_characteristic_attempts_ I think run tasks until one of those changes value, then stop.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/280001
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/300001
https://codereview.chromium.org/1739383002/diff/260001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.h (right): https://codereview.chromium.org/1739383002/diff/260001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.h:107: read_remote_characteristic_value_callbacks_; On 2016/03/03 00:39:42, scheib wrote: > optional, but those names seem long vs: > read_callbacks_ > write_callbacks_ Done. https://codereview.chromium.org/1739383002/diff/260001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_win.cc (right): https://codereview.chromium.org/1739383002/diff/260001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.cc:435: if (task.ToString().find(function, 0) != std::string::npos) On 2016/03/03 00:39:42, scheib wrote: > This seems to be too brittle and indirect. The task string is coming from a the > task posting function, which is too indirect an implementation detail I think. > > We have concrete feedback mechanisms in this test fixture: > callback_count_/error_callback_count_ or gatt_read_characteristic_attempts_ > gatt_write_characteristic_attempts_ > > I think run tasks until one of those changes value, then stop. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/320001
https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_win.cc (right): https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.cc:422: // Run pending Bluetooth tasks until one monitored callback get called. "gets called" or "is called" https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.cc:426: int original_callback_count = callback_count_; You'd want to also stop on an error callback. https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_win.h (right): https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.h:88: void RunPendingBluetoothTasksUntilGetCallback(); grammar of 'get' is incorrect here. Try something like: RunPendingTasksUntilCallback RunPendingTasksUntilFirstCallback RunPendingTasksUntilSuccessCallback RunPendingTasksUntilErrorCallback And document in this header that it runs tasks until the first callback that the test fixture tracks.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/340001
https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_win.cc (right): https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.cc:422: // Run pending Bluetooth tasks until one monitored callback get called. On 2016/03/03 19:03:28, scheib wrote: > "gets called" or "is called" Done. https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.cc:426: int original_callback_count = callback_count_; On 2016/03/03 19:03:27, scheib wrote: > You'd want to also stop on an error callback. Done. https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_win.h (right): https://codereview.chromium.org/1739383002/diff/300001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.h:88: void RunPendingBluetoothTasksUntilGetCallback(); On 2016/03/03 19:03:28, scheib wrote: > grammar of 'get' is incorrect here. Try something like: > RunPendingTasksUntilCallback > RunPendingTasksUntilFirstCallback > RunPendingTasksUntilSuccessCallback > RunPendingTasksUntilErrorCallback > > And document in this header that it runs tasks until the first callback that the > test fixture tracks. 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 scheib@chromium.org
lgtm LGTM
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739383002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739383002/340001
Message was sent while issue was closed.
Description was changed from ========== Implement read & write remote GATT characteristic value for Windows This CL implements read & write remote GATT characteristic value for Windows and related unit tests. BUG=579202 ========== to ========== Implement read & write remote GATT characteristic value for Windows This CL implements read & write remote GATT characteristic value for Windows and related unit tests. BUG=579202 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Implement read & write remote GATT characteristic value for Windows This CL implements read & write remote GATT characteristic value for Windows and related unit tests. BUG=579202 ========== to ========== Implement read & write remote GATT characteristic value for Windows This CL implements read & write remote GATT characteristic value for Windows and related unit tests. BUG=579202 Committed: https://crrev.com/50818ce5049e84526cdadfe11a0ea08405b5c036 Cr-Commit-Position: refs/heads/master@{#379103} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/50818ce5049e84526cdadfe11a0ea08405b5c036 Cr-Commit-Position: refs/heads/master@{#379103}
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
Found an apparent bug when looking at this CL (pointed to me by my /analyze bog). See referenced bug for details. https://codereview.chromium.org/1739383002/diff/340001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1739383002/diff/340001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:333: case ERROR_INVALID_USER_BUFFER: This is a semantically invalid case statement. I opened crbug.com/592843 for this issue. |
