Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2085083002:
arc: bluetooth: Implement Gatt notify (Closed)
|
Created:
4 years, 6 months ago by puthik_chromium Modified:
4 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: bluetooth: Implement Gatt notify
This patch does the following
- Implement the notification to arc bridge when remote
Gatt characteristic value changed.
- Make Android request for writing to CCC Descriptor does
nothing because Chrome bundle that to register/deregister
for notification.
BUG=b:28250518
TEST=Test battery notification with Pixel C Keyboard
Signed-off-by: Puthikorn Voravootivat <puthik@google.com>;
Committed: https://crrev.com/18fd8bf060e661af267764b4dc3465338675e5ae
Cr-Commit-Position: refs/heads/master@{#402331}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Workaround CCC descriptor write issue. #
Total comments: 4
Patch Set 3 : Use CCCUuid() instead of string #
Total comments: 1
Patch Set 4 : Add bug for CCC descriptor #
Total comments: 2
Patch Set 5 : Add more comment about CCC #
Total comments: 7
Patch Set 6 : Add CheckBluetoothInstanceVersion function #
Total comments: 2
Patch Set 7 : fix version check #
Messages
Total messages: 39 (9 generated)
puthik@chromium.org changed reviewers: + ortuno@chromium.org
Will add lhchavez & security team after get LGTM from bluetooth ppl
https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:257: void ArcBluetoothBridge::GattCharacteristicValueChanged( FYI: this function gets called when reading values as well. Is the android side expecting OnGattNotify to be called when reading values?
On 2016/06/21 20:21:29, ortuno wrote: > https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... > components/arc/bluetooth/arc_bluetooth_bridge.cc:257: void > ArcBluetoothBridge::GattCharacteristicValueChanged( > FYI: this function gets called when reading values as well. Is the android side > expecting OnGattNotify to be called when reading values? Android side does not expect this but it should be fine to call that anyway. Also, OnGattNotify will call to Android side arc bridge first, we can filter out there if needed.
rkc@chromium.org changed reviewers: + rkc@chromium.org
Any tests we can add for this?
On 2016/06/21 21:33:32, rkc wrote: > Any tests we can add for this Not in this CL. Maybe add it to http://crrev.com/2046283003 or after that CL landed
https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:49: const int kMinBtleNotifyVersion = 2; enum { kMinBtleNotifyVersion = 2 }; Context: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/hDqJ4KBVqog/ https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:957: void ArcBluetoothBridge::RegisterForGattNotification( Does calling setCharacteristicNotification()[1] result in this function getting called? If so there are a couple of problems with this implementation: On a real android device calling setCharacteristicNotification() does *not* write to the CCC descriptor. If you call StartNotifySession then you will write to the descriptor. Which means the behavior on Arc++ will be different than the behavior in Android. All BLE Android code that wants to use notifications writes to the CCC Descriptor[2]. Bluez blocks access to this descriptor so calls to write to the descriptor will fail. This could result in Apps not being able to use notifications in Arc++. Have you thought about these problems? [1] https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html [2] https://developer.android.com/guide/topics/connectivity/bluetooth-le.html#not... https://codereview.chromium.org/2085083002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:327: bool is_notify, What purpose does is_notify serve? When is it true and when is it false? It makes me really sad that there is not a single comment in all these functions. This code is going to be really hard to maintain...
https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:49: const int kMinBtleNotifyVersion = 2; On 2016/06/21 23:23:06, ortuno wrote: > enum { kMinBtleNotifyVersion = 2 }; > > Context: > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/hDqJ4KBVqog/ Done. https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:257: void ArcBluetoothBridge::GattCharacteristicValueChanged( On 2016/06/21 20:21:29, ortuno wrote: > FYI: this function gets called when reading values as well. Is the android side > expecting OnGattNotify to be called when reading values? Android side is not expect that. But it should be fine anyway. https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:957: void ArcBluetoothBridge::RegisterForGattNotification( On 2016/06/21 23:23:06, ortuno wrote: > Does calling setCharacteristicNotification()[1] result in this function getting > called? If so there are a couple of problems with this implementation: > > On a real android device calling setCharacteristicNotification() does *not* > write to the CCC descriptor. If you call StartNotifySession then you will write > to the descriptor. Which means the behavior on Arc++ will be different than the > behavior in Android. > > All BLE Android code that wants to use notifications writes to the CCC > Descriptor[2]. Bluez blocks access to this descriptor so calls to write to the > descriptor will fail. This could result in Apps not being able to use > notifications in Arc++. > > Have you thought about these problems? > > [1] https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html > [2] > https://developer.android.com/guide/topics/connectivity/bluetooth-le.html#not... Thank you for pointing this out. Since the CCC is only used for start notification, I think the best work-around is just always return write success when trying to write to CCC without doing anything. https://codereview.chromium.org/2085083002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:327: bool is_notify, On 2016/06/21 23:23:06, ortuno wrote: > What purpose does is_notify serve? When is it true and when is it false? > > It makes me really sad that there is not a single comment in all these > functions. This code is going to be really hard to maintain... It's map to is_notify in here. It should be true for notify and false for indicate. Now it is always true. [1] https://source.android.com/devices/halref/structbtgatt__notify__params__t.html
https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:957: void ArcBluetoothBridge::RegisterForGattNotification( On 2016/06/23 at 02:53:34, puthik_chromium wrote: > On 2016/06/21 23:23:06, ortuno wrote: > > Does calling setCharacteristicNotification()[1] result in this function getting > > called? If so there are a couple of problems with this implementation: > > > > On a real android device calling setCharacteristicNotification() does *not* > > write to the CCC descriptor. If you call StartNotifySession then you will write > > to the descriptor. Which means the behavior on Arc++ will be different than the > > behavior in Android. > > > > All BLE Android code that wants to use notifications writes to the CCC > > Descriptor[2]. Bluez blocks access to this descriptor so calls to write to the > > descriptor will fail. This could result in Apps not being able to use > > notifications in Arc++. > > > > Have you thought about these problems? > > > > [1] https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html > > [2] > > https://developer.android.com/guide/topics/connectivity/bluetooth-le.html#not... > > Thank you for pointing this out. > Since the CCC is only used for start notification, I think the best work-around is just always return write success when trying to write to CCC without doing anything. Developers will write to the CCC to stop notifications and if the implementation does nothing, notifications will continue to arrive. This seems very unexpected. Also a question: If setCharacteristicNotification, which is a synchronous function, ends up calling this function which is an async function, how do you notify of success/failure if for example the characteristic doesn't support notifications or there was an error when writing to the descriptor? https://codereview.chromium.org/2085083002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:327: bool is_notify, On 2016/06/23 at 02:53:34, puthik_chromium wrote: > On 2016/06/21 23:23:06, ortuno wrote: > > What purpose does is_notify serve? When is it true and when is it false? > > > > It makes me really sad that there is not a single comment in all these > > functions. This code is going to be really hard to maintain... > > It's map to is_notify in here. It should be true for notify and false for indicate. Now it is always true. > [1] https://source.android.com/devices/halref/structbtgatt__notify__params__t.html I see. Does this have to match the android implementation? Otherwise I would use an enum. We usually only use bools if the variable actually means false. https://codereview.chromium.org/2085083002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:935: if (descriptor->GetUUID().canonical_value().substr(4, 4) == nit: Also add this change to the description. https://codereview.chromium.org/2085083002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:936: kDescriptorCCCUUid) { Use BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()[1] [1] https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_gatt_descript...
Description was changed from ========== arc: bluetooth: Implement Gatt notify Implement the notification to arc bridge when remote Gatt characteristic value changed. BUG=b:28250518 TEST=Build Signed-off-by: Puthikorn Voravootivat <puthik@google.com> ========== to ========== arc: bluetooth: Implement Gatt notify This patch does the following - Implement the notification to arc bridge when remote Gatt characteristic value changed. - Make Android request for writing to CCC Descriptor does nothing because Chrome bundle that to register/deregister for notification. BUG=b:28250518 TEST=Test battery notification with Pixel C Keyboard Signed-off-by: Puthikorn Voravootivat <puthik@google.com> ==========
https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:957: void ArcBluetoothBridge::RegisterForGattNotification( On 2016/06/23 14:29:38, ortuno wrote: > On 2016/06/23 at 02:53:34, puthik_chromium wrote: > > On 2016/06/21 23:23:06, ortuno wrote: > > > Does calling setCharacteristicNotification()[1] result in this function > getting > > > called? If so there are a couple of problems with this implementation: > > > > > > On a real android device calling setCharacteristicNotification() does *not* > > > write to the CCC descriptor. If you call StartNotifySession then you will > write > > > to the descriptor. Which means the behavior on Arc++ will be different than > the > > > behavior in Android. > > > > > > All BLE Android code that wants to use notifications writes to the CCC > > > Descriptor[2]. Bluez blocks access to this descriptor so calls to write to > the > > > descriptor will fail. This could result in Apps not being able to use > > > notifications in Arc++. > > > > > > Have you thought about these problems? > > > > > > [1] > https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html > > > [2] > > > > https://developer.android.com/guide/topics/connectivity/bluetooth-le.html#not... > > > > Thank you for pointing this out. > > Since the CCC is only used for start notification, I think the best > work-around is just always return write success when trying to write to CCC > without doing anything. > > Developers will write to the CCC to stop notifications and if the implementation > does nothing, notifications will continue to arrive. This seems very unexpected. > > Also a question: > > If setCharacteristicNotification, which is a synchronous function, ends up > calling this function which is an async function, how do you notify of > success/failure if for example the characteristic doesn't support notifications > or there was an error when writing to the descriptor? Q1 Developers will write to the CCC to stop notifications... From the app I tested[1], they will write CCC to disable and then call the deregister_for_notification. And it worked as expected. Q2 how do you notify of success/failure for notify Chrome side has OnGattNotifyStartDone/OnGattNotifyStartError callback to Android side. Android side use Future class to make it synchronous [2]. [1] https://play.google.com/store/apps/details?id=no.nordicsemi.android.mcp&hl=en This app also provide library for other developers. [2] https://googleplex-android.googlesource.com/device/google/cheets2/+/591fc94de... Future<BluetoothGattStatus> status; mBluetoothHost->RegisterForGattNotification(std::move(remote_addr), std::move(service_id), std::move(char_id), FutureCallback<BluetoothGattStatus>(&status)); return status.get(); https://codereview.chromium.org/2085083002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:327: bool is_notify, On 2016/06/23 14:29:38, ortuno wrote: > On 2016/06/23 at 02:53:34, puthik_chromium wrote: > > On 2016/06/21 23:23:06, ortuno wrote: > > > What purpose does is_notify serve? When is it true and when is it false? > > > > > > It makes me really sad that there is not a single comment in all these > > > functions. This code is going to be really hard to maintain... > > > > It's map to is_notify in here. It should be true for notify and false for > indicate. Now it is always true. > > [1] > https://source.android.com/devices/halref/structbtgatt__notify__params__t.html > > I see. Does this have to match the android implementation? Otherwise I would use > an enum. We usually only use bools if the variable actually means false. I would prefer to match it to android. It will make life easier when we need to upgrade to Android N https://codereview.chromium.org/2085083002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:935: if (descriptor->GetUUID().canonical_value().substr(4, 4) == On 2016/06/23 14:29:38, ortuno wrote: > nit: Also add this change to the description. Done. https://codereview.chromium.org/2085083002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:936: kDescriptorCCCUUid) { On 2016/06/23 14:29:38, ortuno wrote: > Use BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid()[1] > > [1] > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_gatt_descript... Done.
usage of the api lgtm FWIW I think the current implementation will confuse developers a lot. Android developers expect to have fine grained control over the CCC descriptor. Returning success for all calls to write to the descriptor is bound to confuse developers and make their apps fail in very subtle ways. I understand speed is the goal here but if you have some time I would talk to rkc to see if there are ways the bluez API can be changed to better accommodate Arc++. https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:957: void ArcBluetoothBridge::RegisterForGattNotification( On 2016/06/23 at 18:36:30, puthik_chromium wrote: > On 2016/06/23 14:29:38, ortuno wrote: > > On 2016/06/23 at 02:53:34, puthik_chromium wrote: > > > On 2016/06/21 23:23:06, ortuno wrote: > > > > Does calling setCharacteristicNotification()[1] result in this function > > getting > > > > called? If so there are a couple of problems with this implementation: > > > > > > > > On a real android device calling setCharacteristicNotification() does *not* > > > > write to the CCC descriptor. If you call StartNotifySession then you will > > write > > > > to the descriptor. Which means the behavior on Arc++ will be different than > > the > > > > behavior in Android. > > > > > > > > All BLE Android code that wants to use notifications writes to the CCC > > > > Descriptor[2]. Bluez blocks access to this descriptor so calls to write to > > the > > > > descriptor will fail. This could result in Apps not being able to use > > > > notifications in Arc++. > > > > > > > > Have you thought about these problems? > > > > > > > > [1] > > https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html > > > > [2] > > > > > > https://developer.android.com/guide/topics/connectivity/bluetooth-le.html#not... > > > > > > Thank you for pointing this out. > > > Since the CCC is only used for start notification, I think the best > > work-around is just always return write success when trying to write to CCC > > without doing anything. > > > > Developers will write to the CCC to stop notifications and if the implementation > > does nothing, notifications will continue to arrive. This seems very unexpected. > > > > Also a question: > > > > If setCharacteristicNotification, which is a synchronous function, ends up > > calling this function which is an async function, how do you notify of > > success/failure if for example the characteristic doesn't support notifications > > or there was an error when writing to the descriptor? > > Q1 Developers will write to the CCC to stop notifications... > From the app I tested[1], they will write CCC to disable and then call the deregister_for_notification. And it worked as expected. > > Q2 how do you notify of success/failure for notify > Chrome side has OnGattNotifyStartDone/OnGattNotifyStartError callback to Android side. > Android side use Future class to make it synchronous [2]. > > [1] https://play.google.com/store/apps/details?id=no.nordicsemi.android.mcp&hl=en > This app also provide library for other developers. > [2] https://googleplex-android.googlesource.com/device/google/cheets2/+/591fc94de... > Future<BluetoothGattStatus> status; > mBluetoothHost->RegisterForGattNotification(std::move(remote_addr), std::move(service_id), > std::move(char_id), > FutureCallback<BluetoothGattStatus>(&status)); > return status.get(); Does this mean a call to setCharacteristicNotification will block until we've written the descriptor?
lgtm with the comment addressed. I agree with ortuno@; this will lead to issues with apps which are used to manipulating the CCC directly so this is not a tenable long term solution. However, for now, if this works with the apps you have tested, lets go with this partial solution for now with the aim to get to a complete solution in M54. If on more testing we find out that this is breaking other apps that users care about, we can look at trying to get a fix in to M53 itself. https://codereview.chromium.org/2085083002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:932: if (descriptor->GetUUID() == This is definitely going to break behavior in some apps. I am fine with this going in for M53, but please create a P1 bug for this and add it here in a TODO. We should discuss potential solutions and see if there is a simple one we can get in for 53 later. We could either simulate the write with calling StartNotify or we could just remove this check: http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/gatt-client.c#n588 but that can break notification behavior on BlueZ. More investigation is needed.
puthik@chromium.org changed reviewers: + dcheng@chromium.org, lhchavez@chromium.org, palmer@chromium.org
+lhchavez (arc bridge owner) +palmer / dcheng (mojo security)
On 2016/06/23 20:03:22, puthik_chromium wrote: > +lhchavez (arc bridge owner) > +palmer / dcheng (mojo security) -dcheng Please just pick one IPC reviewer, otherwise, everyone assumes that someone else will review it. And since I'm responding before palmer, palmer gets to be the lucky winner of this ipc review lottery ;-)
dcheng@chromium.org changed reviewers: - dcheng@chromium.org
https://codereview.chromium.org/2085083002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:931: // Always return success when encounter this. These 2 comment lines don't seem to agree — it's unsupported, so report success. Can you please explain in the comment why we want to do that?
https://codereview.chromium.org/2085083002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:931: // Always return success when encounter this. On 2016/06/27 19:25:42, palmer wrote: > These 2 comment lines don't seem to agree — it's unsupported, so report success. > Can you please explain in the comment why we want to do that? Done.
lgtm https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:933: // Chrome API group both steps to one API and does not support writing Nit: English grammar. "The Chrome API groups both steps into one API, and does not support writing directly to the CCC Descriptor. Therefore, until we fix https://crbug.com/622832, we return successfully when we encounter this."
https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:48: enum { kMinBtleVersion = 1 }; can we have constexpr int32_t instead of an enum? https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:265: LOG(WARNING) << "Bluetooth instance is too old and does not support notify"; nit: mention the version in the log.
https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:48: enum { kMinBtleVersion = 1 }; On 2016/06/27 at 20:16:20, Luis Héctor Chávez wrote: > can we have constexpr int32_t instead of an enum? I specifically asked for an enum based on the conclusion of https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/hDqJ4KBVqog
On 2016/06/27 20:27:20, ortuno wrote: > https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... > components/arc/bluetooth/arc_bluetooth_bridge.cc:48: enum { kMinBtleVersion = 1 > }; > On 2016/06/27 at 20:16:20, Luis Héctor Chávez wrote: > > can we have constexpr int32_t instead of an enum? > > I specifically asked for an enum based on the conclusion of > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/hDqJ4KBVqog Guidelines have been changed since then: http://chromium-cpp.appspot.com/ . constexpr was banned at that time (see danakj's last reply), and it's now both allowed and has been encouraged in other code reviews. Plus, the issue in that discussion was declaring it on an .h, whereas here it's declaring it on a .cc, where constexpr is non-controversial.
On 2016/06/27 20:36:01, Luis Héctor Chávez wrote: > On 2016/06/27 20:27:20, ortuno wrote: > > > https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... > > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > > > > https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... > > components/arc/bluetooth/arc_bluetooth_bridge.cc:48: enum { kMinBtleVersion = > 1 > > }; > > On 2016/06/27 at 20:16:20, Luis Héctor Chávez wrote: > > > can we have constexpr int32_t instead of an enum? > > > > I specifically asked for an enum based on the conclusion of > > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/hDqJ4KBVqog > > Guidelines have been changed since then: http://chromium-cpp.appspot.com/ . > constexpr was banned at that time (see danakj's last reply), and it's now both > allowed and has been encouraged in other code reviews. Plus, the issue in that > discussion was declaring it on an .h, whereas here it's declaring it on a .cc, > where constexpr is non-controversial. I'd still use enum { kXXXX = val} instead of static class variables, but in this case, this is not a class variable. constexpr is fine to use here.
On 2016/06/27 at 20:36:01, lhchavez wrote: > On 2016/06/27 20:27:20, ortuno wrote: > > https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... > > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > > > https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... > > components/arc/bluetooth/arc_bluetooth_bridge.cc:48: enum { kMinBtleVersion = 1 > > }; > > On 2016/06/27 at 20:16:20, Luis Héctor Chávez wrote: > > > can we have constexpr int32_t instead of an enum? > > > > I specifically asked for an enum based on the conclusion of > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/hDqJ4KBVqog > > Guidelines have been changed since then: http://chromium-cpp.appspot.com/ . constexpr was banned at that time (see danakj's last reply), and it's now both allowed and has been encouraged in other code reviews. Plus, the issue in that discussion was declaring it on an .h, whereas here it's declaring it on a .cc, where constexpr is non-controversial. Ah right. Given that it's in a .cc file it's ok. But fyi I believe constexpr doesn't always work; see last two responses. I've personally run into ODR violations with constexpr in the past, that's how I found out about using enum :)
https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:48: enum { kMinBtleVersion = 1 }; On 2016/06/27 20:16:20, Luis Héctor Chávez wrote: > can we have constexpr int32_t instead of an enum? Done. https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:265: LOG(WARNING) << "Bluetooth instance is too old and does not support notify"; On 2016/06/27 20:16:20, Luis Héctor Chávez wrote: > nit: mention the version in the log. Done. I make it to the function to remove duplicate code. https://codereview.chromium.org/2085083002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:933: // Chrome API group both steps to one API and does not support writing On 2016/06/27 20:11:56, palmer wrote: > Nit: English grammar. > > "The Chrome API groups both steps into one API, and does not support writing > directly to the CCC Descriptor. Therefore, until we fix > https://crbug.com/622832, we return successfully when we encounter this." Done.
https://codereview.chromium.org/2085083002/diff/100001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/100001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1354: if (version < version_need) shouldn't this be >=?
https://codereview.chromium.org/2085083002/diff/100001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2085083002/diff/100001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1354: if (version < version_need) On 2016/06/27 21:54:40, Luis Héctor Chávez wrote: > shouldn't this be >=? Yes. My bad.
Now that https://codereview.chromium.org/2046283003 has landed, please add a test for this. A test would have caught https://codereview.chromium.org/2085083002/diff/100001/components/arc/bluetoo...
lgtm
On 2016/06/27 21:59:37, rkc wrote: > Now that https://codereview.chromium.org/2046283003 has landed, please add a > test for this. > > A test would have caught > https://codereview.chromium.org/2085083002/diff/100001/components/arc/bluetoo... Sure, test in another CL. I need to write Gatt Server code first.
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, ortuno@chromium.org, palmer@chromium.org Link to the patchset: https://codereview.chromium.org/2085083002/#ps120001 (title: "fix version check")
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 notify This patch does the following - Implement the notification to arc bridge when remote Gatt characteristic value changed. - Make Android request for writing to CCC Descriptor does nothing because Chrome bundle that to register/deregister for notification. BUG=b:28250518 TEST=Test battery notification with Pixel C Keyboard Signed-off-by: Puthikorn Voravootivat <puthik@google.com> ========== to ========== arc: bluetooth: Implement Gatt notify This patch does the following - Implement the notification to arc bridge when remote Gatt characteristic value changed. - Make Android request for writing to CCC Descriptor does nothing because Chrome bundle that to register/deregister for notification. BUG=b:28250518 TEST=Test battery notification with Pixel C Keyboard Signed-off-by: Puthikorn Voravootivat <puthik@google.com> ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Implement Gatt notify This patch does the following - Implement the notification to arc bridge when remote Gatt characteristic value changed. - Make Android request for writing to CCC Descriptor does nothing because Chrome bundle that to register/deregister for notification. BUG=b:28250518 TEST=Test battery notification with Pixel C Keyboard Signed-off-by: Puthikorn Voravootivat <puthik@google.com> ========== to ========== arc: bluetooth: Implement Gatt notify This patch does the following - Implement the notification to arc bridge when remote Gatt characteristic value changed. - Make Android request for writing to CCC Descriptor does nothing because Chrome bundle that to register/deregister for notification. BUG=b:28250518 TEST=Test battery notification with Pixel C Keyboard Signed-off-by: Puthikorn Voravootivat <puthik@google.com> Committed: https://crrev.com/18fd8bf060e661af267764b4dc3465338675e5ae Cr-Commit-Position: refs/heads/master@{#402331} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/18fd8bf060e661af267764b4dc3465338675e5ae Cr-Commit-Position: refs/heads/master@{#402331} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
