Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2016023002:
bluetooth: Add option dict to ReadValue/WriteValue dbus call (Closed)
|
Created:
4 years, 6 months ago by puthik_chromium Modified:
4 years, 6 months ago CC:
chromium-reviews, Miao, ortuno+watch_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Add option dict to ReadValue/WriteValue dbus call
The BlueZ CL below added option dict to ReadValue and WriteValue.
http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=93b64d9
As we intend to use BlueZ API change for Gatt server funtionality,
this CL updates Chrome DBus call to match the new BlueZ API by
just append an empty option dict for Gatt client call.
BUG=chromium:614903
TEST=Can read characteristic value from Pixel C keyboard
Committed: https://crrev.com/672ef3f9e8849b57eb03ef8752b861f410542ac7
Cr-Commit-Position: refs/heads/master@{#396595}
Patch Set 1 #Patch Set 2 : Add descriptor #
Total comments: 1
Patch Set 3 : Add empty dict instead #
Total comments: 1
Patch Set 4 : Use AppendValueData #
Messages
Total messages: 28 (8 generated)
puthik@google.com changed reviewers: + puthik@google.com, rkc@chromium.org
puthik@google.com changed reviewers: + ortuno@chromium.org
ortuno@ Can you review this? rkc@ is on vacation now. ChromeOS side patch is at http://crosreview.com/347459
On 2016/05/26 at 20:14:41, puthik wrote: > ortuno@ Can you review this? rkc@ is on vacation now. > > ChromeOS side patch is at http://crosreview.com/347459 What's the testing plan for this?
On 2016/05/26 at 20:21:41, ortuno wrote: > On 2016/05/26 at 20:14:41, puthik wrote: > > ortuno@ Can you review this? rkc@ is on vacation now. > > > > ChromeOS side patch is at http://crosreview.com/347459 > > What's the testing plan for this? I already verified with bluetoothctl command that Gatt Characteristic read is fixed. With this patch and ChromeOS one. I can read data from Pixel C keyboard using command described in http://crbug.com/614903
On 2016/05/26 at 21:04:43, puthik wrote: > On 2016/05/26 at 20:21:41, ortuno wrote: > > On 2016/05/26 at 20:14:41, puthik wrote: > > > ortuno@ Can you review this? rkc@ is on vacation now. > > > > > > ChromeOS side patch is at http://crosreview.com/347459 > > > > What's the testing plan for this? > > I already verified with bluetoothctl command that Gatt Characteristic read is fixed. > With this patch and ChromeOS one. I can read data from Pixel C keyboard using command > described in http://crbug.com/614903 Is there anyway we can write tests that exercises this code? I've personally have had to fix many bugs in this code because there were no tests and I would like to avoid that in the future.
The CQ bit was checked by puthik@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016023002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016023002/20001
On 2016/05/26 at 23:26:33, ortuno wrote: > On 2016/05/26 at 21:04:43, puthik wrote: > > On 2016/05/26 at 20:21:41, ortuno wrote: > > > On 2016/05/26 at 20:14:41, puthik wrote: > > > > ortuno@ Can you review this? rkc@ is on vacation now. > > > > > > > > ChromeOS side patch is at http://crosreview.com/347459 > > > > > > What's the testing plan for this? > > > > I already verified with bluetoothctl command that Gatt Characteristic read is fixed. > > With this patch and ChromeOS one. I can read data from Pixel C keyboard using command > > described in http://crbug.com/614903 > > Is there anyway we can write tests that exercises this code? I've personally have had to fix many bugs in this code because there were no tests and I would like to avoid that in the future. I'm not sure about how to write unit test for this but josephsih@ is looking at integration test for bluetooth in ChromeOS. That should include a test that exercises this code path.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ortuno@, I share your concerns about the testing of this code, but at this point, we don't have any good options. We can add a test like I did for the GATT application provider and test against a fixed string output - but that is hacky and very susceptible to bias, since I am constructing the oracle by reading the code that is expecting the output. While useful when testing an object as complex as the application provider exports, is overkill with questionable value in cases where we're testing smaller DBus objects such as this. We need an Oracle that is similar to BlueZ, constructing which is hard without re-writing BlueZ's parsing code. The correct solution would be to have a shim in BlueZ that does the input parsing, which could be imported from Chrome and then used to test the DBus bridge. Unfortunately considering our very minimal influence on BlueZ code/architecture, this isn't really a possibility at the moment. If you have any suggestions or ideas that you think would work, please do start a discussion offline. I would love for us to implement any testing strategy for this code that is viable and adds value. https://codereview.chromium.org/2016023002/diff/20001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_gatt_characteristic_client.cc (right): https://codereview.chromium.org/2016023002/diff/20001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_gatt_characteristic_client.cc:106: bluetooth_gatt_characteristic::kOptionOffset); The whole purpose of having an options/flags dictionary sent as a parameter is that we can add optional parameters without breaking existing code. This parameter is optional - if BlueZ expects this to be there, then BlueZ needs to be fixed. On the Chrome side, we just need to send an empty options dictionary (like in all the other places where we have an options dictionary).
> > Is there anyway we can write tests that exercises this code? I've personally > have had to fix many bugs in this code because there were no tests and I would > like to avoid that in the future. > > I'm not sure about how to write unit test See device/bluetooth/dbus/bluetooth_gatt_application_service_provider_unittest.cc
On 2016/05/27 04:01:24, scheib wrote: > > > Is there anyway we can write tests that exercises this code? I've personally > > have had to fix many bugs in this code because there were no tests and I would > > like to avoid that in the future. > > > > I'm not sure about how to write unit test > > See > device/bluetooth/dbus/bluetooth_gatt_application_service_provider_unittest.cc We should add infrastructure to easily add small unit tests that can be self-contained, "run this provider/client, test it against this output". Though I question the value of a test that checks code that creates an empty dictionary versus the output text of an empty dictionary; it would still have value if writing it was really easy to do and writing the infrastructure would allow us to do it for more components. That being said, I believe this CL will need to be merged back to M52. If that is the case, we should create that infrastructure and write this test in a separate CL.
Description was changed from ========== bluetooth: Add option dict to readValue()/WrieValue dbus call The BlueZ CL below added option dict to ReadValue and WriteValue. This CL updates Chrome dbus call to match that. http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=93b64d9 As we intend to use BlueZ API change for Gatt Server functionality. Hardcoded offset to 0 for now. BUG=chromium:614903 TEST=Can read characteristic value from Pixel C keyboard ========== to ========== bluetooth: Add option dict to ReadValue/WriteValue dbus call The BlueZ CL below added option dict to ReadValue and WriteValue. http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=93b64d9 As we intend to use BlueZ API change for Gatt server funtionality, this CL updates Chrome DBus call to match the new BlueZ API by just append an empty option dict for Gatt client call. BUG=chromium:614903 TEST=Can read characteristic value from Pixel C keyboard ==========
On 2016/05/27 at 04:01:12, rkc wrote: > ortuno@, I share your concerns about the testing of this code, but at this point, we don't have any good options. > > We can add a test like I did for the GATT application provider and test against a fixed string output - but that is hacky and very susceptible to bias, since I am constructing the oracle by reading the code that is expecting the output. While useful when testing an object as complex as the application provider exports, is overkill with questionable value in cases where we're testing smaller DBus objects such as this. > > We need an Oracle that is similar to BlueZ, constructing which is hard without re-writing BlueZ's parsing code. > > The correct solution would be to have a shim in BlueZ that does the input parsing, which could be imported from Chrome and then used to test the DBus bridge. Unfortunately considering our very minimal influence on BlueZ code/architecture, this isn't really a possibility at the moment. > > If you have any suggestions or ideas that you think would work, please do start a discussion offline. I would love for us to implement any testing strategy for this code that is viable and adds value. > > https://codereview.chromium.org/2016023002/diff/20001/device/bluetooth/dbus/b... > File device/bluetooth/dbus/bluetooth_gatt_characteristic_client.cc (right): > > https://codereview.chromium.org/2016023002/diff/20001/device/bluetooth/dbus/b... > device/bluetooth/dbus/bluetooth_gatt_characteristic_client.cc:106: bluetooth_gatt_characteristic::kOptionOffset); > The whole purpose of having an options/flags dictionary sent as a parameter is that we can add optional parameters without breaking existing code. > > This parameter is optional - if BlueZ expects this to be there, then BlueZ needs to be fixed. On the Chrome side, we just need to send an empty options dictionary (like in all the other places where we have an options dictionary). Done. I changed it to just append an empty option dict instead.
https://codereview.chromium.org/2016023002/diff/40001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_gatt_characteristic_client.cc (right): https://codereview.chromium.org/2016023002/diff/40001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_gatt_characteristic_client.cc:98: // Append empty option dict Any reason why you do this manually instead of using the AppendValueData[1] helper function? If not I suggest using the helper function. [1] https://code.google.com/p/chromium/codesearch#chromium/src/dbus/values_util.h...
lgtm
Parch #4 Use AppendValueData
thanks! 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/2016023002/#ps60001 (title: "Use AppendValueData")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016023002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2016023002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add option dict to ReadValue/WriteValue dbus call The BlueZ CL below added option dict to ReadValue and WriteValue. http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=93b64d9 As we intend to use BlueZ API change for Gatt server funtionality, this CL updates Chrome DBus call to match the new BlueZ API by just append an empty option dict for Gatt client call. BUG=chromium:614903 TEST=Can read characteristic value from Pixel C keyboard ========== to ========== bluetooth: Add option dict to ReadValue/WriteValue dbus call The BlueZ CL below added option dict to ReadValue and WriteValue. http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=93b64d9 As we intend to use BlueZ API change for Gatt server funtionality, this CL updates Chrome DBus call to match the new BlueZ API by just append an empty option dict for Gatt client call. BUG=chromium:614903 TEST=Can read characteristic value from Pixel C keyboard Committed: https://crrev.com/672ef3f9e8849b57eb03ef8752b861f410542ac7 Cr-Commit-Position: refs/heads/master@{#396595} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/672ef3f9e8849b57eb03ef8752b861f410542ac7 Cr-Commit-Position: refs/heads/master@{#396595} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
