Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1583333003:
bluetooth: Invalidate connection objects if a connection fails and add histograms (Closed)
Descriptionbluetooth: Invalidate connection objects if a connection fails and add histograms
DidFailToConnectGatt is sometimes called when there are valid Connection
objects so we need to invalidate them.
Also adds histograms to understand what types of error can we expect in android.
BUG=577741
Committed: https://crrev.com/41ec82434c76605df09935ebb1ed97f551d764c4
Cr-Commit-Position: refs/heads/master@{#370322}
Patch Set 1 #Patch Set 2 : Clean up #
Total comments: 20
Patch Set 3 : Address scheib's comments #
Total comments: 11
Patch Set 4 : Address scheib's comments #
Total comments: 2
Messages
Total messages: 22 (7 generated)
ortuno@chromium.org changed reviewers: + scheib@chromium.org
scheib: PTAL
https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.cc (right): https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.cc:305: for (BluetoothGattConnection* connection : gatt_connections_) { DCHECK this instead. https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.cc:314: CHECK(!create_gatt_connection_error_callbacks_.size()); If you use CHECK, please TODO and file a ship blocking issue to return to DCHECK. I think DCHECK is sufficient though. https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:11: #include "base/metrics/histogram_macros.h" Why is histogram_macros required? Seems to be a bug if it's just for UMA_HISTOGRAM_SPARSE_SLOWLY. https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:266: return DidFailToConnectGatt(ERROR_UNKNOWN); Call DidFail or DidDisconnect based on the pending callbacks. https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3149: + Records the outcome of failed GATT connections. Used to better understand How about "Failed GATT connection error codes." The change is to focus the description to it's meaning and remove unnecessary or redundant text. https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3150: + error seen in Android. understand errors ^ https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3160: + Records the outcome of successfull GATT connections. Used to better "Successful GATT connection result codes." https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3165: +<histogram name="Bluetooth.Android.GATTConnection.Terminated.Result" Terminated vs Disconnected? Unsure, but rest of our terminology seems to be disconnected - and that's Android's term: http://developer.android.com/reference/android/bluetooth/BluetoothGattCallbac... https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3171: + Records the outcome when a connection is terminated. Used to better Disconnected GATT connection status codes. https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57165: +<enum name="BluetoothConnectionErrorCodes" type="int"> This list should should be the merge of Bluetooth codes, Android codes, and BlueDroid, right? It may be suitable for any other histogram of status codes we receive. Name: AndroidGATTErrorCodes? Include the values adjacent to http://developer.android.com/reference/android/bluetooth/BluetoothGatt.html#G... e.g. 0x8f, 0x101.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.cc (right): https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.cc:305: for (BluetoothGattConnection* connection : gatt_connections_) { On 2016/01/15 at 01:39:45, scheib wrote: > DCHECK this instead. Done. https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.cc:314: CHECK(!create_gatt_connection_error_callbacks_.size()); On 2016/01/15 at 01:39:44, scheib wrote: > If you use CHECK, please TODO and file a ship blocking issue to return to DCHECK. > > I think DCHECK is sufficient though. Done. https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:11: #include "base/metrics/histogram_macros.h" On 2016/01/15 at 01:39:45, scheib wrote: > Why is histogram_macros required? Seems to be a bug if it's just for UMA_HISTOGRAM_SPARSE_SLOWLY. Done. https://codereview.chromium.org/1583333003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:266: return DidFailToConnectGatt(ERROR_UNKNOWN); On 2016/01/15 at 01:39:45, scheib wrote: > Call DidFail or DidDisconnect based on the pending callbacks. Done. https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3149: + Records the outcome of failed GATT connections. Used to better understand On 2016/01/15 at 01:39:45, scheib wrote: > How about "Failed GATT connection error codes." The change is to focus the description to it's meaning and remove unnecessary or redundant text. Done. https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3150: + error seen in Android. On 2016/01/15 at 01:39:45, scheib wrote: > understand errors > ^ Done. https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3160: + Records the outcome of successfull GATT connections. Used to better On 2016/01/15 at 01:39:45, scheib wrote: > "Successful GATT connection result codes." Done. https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3165: +<histogram name="Bluetooth.Android.GATTConnection.Terminated.Result" On 2016/01/15 at 01:39:45, scheib wrote: > Terminated vs Disconnected? Unsure, but rest of our terminology seems to be disconnected - and that's Android's term: http://developer.android.com/reference/android/bluetooth/BluetoothGattCallbac... Done. https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3171: + Records the outcome when a connection is terminated. Used to better On 2016/01/15 at 01:39:45, scheib wrote: > Disconnected GATT connection status codes. Done. https://codereview.chromium.org/1583333003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57165: +<enum name="BluetoothConnectionErrorCodes" type="int"> On 2016/01/15 at 01:39:45, scheib wrote: > This list should should be the merge of Bluetooth codes, Android codes, and BlueDroid, right? It may be suitable for any other histogram of status codes we receive. > Name: AndroidGATTErrorCodes? They are not GATT errors but rather connection errors. > Include the values adjacent to http://developer.android.com/reference/android/bluetooth/BluetoothGatt.html#G... e.g. 0x8f, 0x101. The Bluedroid errors match the spec. I added 0x101 which is an android error.
https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:234: DidFailToConnectGatt(ERROR_FAILED); Add a LOG for now to help us understand codes in bugreports. TODO it to be removed. https://code.google.com/p/chromium/issues/detail?id=529510 https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:458: SimulateGattConnectionError(device, BluetoothDevice::ERROR_FAILED); This test is intentionally sending 2 errors and expecting only the first to be reported. Please report 2. I'd prefer them to have different values as well, so the test can verify that it is the first value that is passed - but if the enums are cleared out that won't happen. Perhaps add a TODO for restoring that behavior when the enums are fixed. https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/test/b... File device/bluetooth/test/bluetooth_test_android.cc (right): https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/test/b... device/bluetooth/test/bluetooth_test_android.cc:93: // TODO(ortuno): Add all types of errors Android can produce. For now we I agree that the above switch is wrong, but I don't see value in removing it until we fix issue 578191. Not worth debating though, so make the decision. https://codereview.chromium.org/1583333003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1583333003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56224: + <int value="0" label="0x00 Success"/> Comment the sources of the error codes. Are the GATT errors from Core spec Vol 2, Part D? Are they all possible GATT connection errors? https://codereview.chromium.org/1583333003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56297: + Using Clock Dragging"/> Should this one from bluedroid be included? GATT_CONN_CANCEL /* 0x0100 L2CAP connection cancelled */
https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:234: DidFailToConnectGatt(ERROR_FAILED); On 2016/01/16 at 23:59:52, scheib wrote: > Add a LOG for now to help us understand codes in bugreports. TODO it to be removed. https://code.google.com/p/chromium/issues/detail?id=529510 I didn't add log because Android already logs it. Do you think I should still log? https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:458: SimulateGattConnectionError(device, BluetoothDevice::ERROR_FAILED); On 2016/01/16 at 23:59:52, scheib wrote: > This test is intentionally sending 2 errors and expecting only the first to be reported. Please report 2. > > I'd prefer them to have different values as well, so the test can verify that it is the first value that is passed - but if the enums are cleared out that won't happen. Perhaps add a TODO for restoring that behavior when the enums are fixed. Done. https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/test/b... File device/bluetooth/test/bluetooth_test_android.cc (right): https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/test/b... device/bluetooth/test/bluetooth_test_android.cc:93: // TODO(ortuno): Add all types of errors Android can produce. For now we On 2016/01/16 at 23:59:52, scheib wrote: > I agree that the above switch is wrong, but I don't see value in removing it until we fix issue 578191. Not worth debating though, so make the decision. Removed. https://codereview.chromium.org/1583333003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1583333003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56224: + <int value="0" label="0x00 Success"/> On 2016/01/16 at 23:59:52, scheib wrote: > Comment the sources of the error codes. Are the GATT errors from Core spec Vol 2, Part D? Done. > Are they all possible GATT connection errors? Those are all I could find. https://codereview.chromium.org/1583333003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56297: + Using Clock Dragging"/> On 2016/01/16 at 23:59:52, scheib wrote: > Should this one from bluedroid be included? > GATT_CONN_CANCEL /* 0x0100 L2CAP connection cancelled */ Done.
LGTM https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1583333003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:234: DidFailToConnectGatt(ERROR_FAILED); On 2016/01/19 20:40:08, ortuno wrote: > On 2016/01/16 at 23:59:52, scheib wrote: > > Add a LOG for now to help us understand codes in bugreports. TODO it to be > removed. https://code.google.com/p/chromium/issues/detail?id=529510 > > I didn't add log because Android already logs it. Do you think I should still > log? No, the BluetoothGattCallbackImpl log is sufficient.
ortuno@chromium.org changed reviewers: + isherman@chromium.org
isherman: PTAL at histograms.xml
https://codereview.chromium.org/1583333003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1583333003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3175: + enum="AndroidGATTConnectionErrorCodes"> What codes might be logged in this histogram, other than "0x00 Success"?
https://codereview.chromium.org/1583333003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1583333003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3175: + enum="AndroidGATTConnectionErrorCodes"> On 2016/01/20 00:35:43, Ilya Sherman wrote: > What codes might be logged in this histogram, other than "0x00 Success"? We discovered that Android doesn't document what error codes may be returned and that we were receiving unexpected codes. These UMA Histograms are intended to discover what codes are actually being returned so that we can handle them in a more structured way in chromium and make explicit enumerations for them which we report eventually to javascript code using Web Bluetooth. By examining the Android source and Bluedroid (the Bluetooth library used) we have found several of these GATT errors to be returned, hence the enum list below which merges the Bluetooth Specification of error codes, and a few Android/Bluedroid specific ones.
Okay, in that case, LGTM.
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1583333003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583333003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by scheib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1583333003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583333003/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Invalidate connection objects if a connection fails and add histograms DidFailToConnectGatt is sometimes called when there are valid Connection objects so we need to invalidate them. Also adds histograms to understand what types of error can we expect in android. BUG=577741 ========== to ========== bluetooth: Invalidate connection objects if a connection fails and add histograms DidFailToConnectGatt is sometimes called when there are valid Connection objects so we need to invalidate them. Also adds histograms to understand what types of error can we expect in android. BUG=577741 Committed: https://crrev.com/41ec82434c76605df09935ebb1ed97f551d764c4 Cr-Commit-Position: refs/heads/master@{#370322} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/41ec82434c76605df09935ebb1ed97f551d764c4 Cr-Commit-Position: refs/heads/master@{#370322} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
