Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1606523002:
Tidy up service discovery state for bluez on disconnect (Closed)
DescriptionTidy up service discovery state for bluez on disconnect
This should fix cases where you connect, disconnect and then reconnect
to a BLE device. I've extended the ServicesDiscovered unit test to test
this.
BUG=577641
Committed: https://crrev.com/9e68515b6ef4fe5a87db25e6ae16014091f3dfa5
Cr-Commit-Position: refs/heads/master@{#370628}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address issues highlighted in code review #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Tidy up state and services for bluez on disconnect This should fix cases where you connect, disconnect and then reconnect to a BLE device. I've extended the ServicesDiscovered unit test to test this. BUG=577641 ========== to ========== Tidy up state and services for bluez on disconnect This should fix cases where you connect, disconnect and then reconnect to a BLE device. I've extended the ServicesDiscovered unit test to test this. BUG=577641 ==========
tommyt@opera.com changed reviewers: + ortuno@chromium.org, scheib@chromium.org
Here's the change that cleans up services upon disconnect for the bluez implementation. It's pretty much what ortuno suggested I do in my previous codereview. The change passes the blink layout tests and the unit tests, but I have not actually been able to test the code for real, yet. I'm struggling a little with getting either of the linux versions use my USB BLE dongle to talk to my bluetooth devices. If you think the code looks ok, and you want the change in ASAP, feel free to LGTM. If you feel that it should be tested better before committing, then you can wait until I've figured out how (or test it yourself, if you have a setup that works).
I'll review after Giovanni On Jan 18, 2016 7:27 AM, <tommyt@opera.com> wrote: > Reviewers: scheib, ortuno > CL: https://codereview.chromium.org/1606523002/ > > Message: > Here's the change that cleans up services upon disconnect for the bluez > implementation. It's pretty much what ortuno suggested I do in my previous > codereview. > > The change passes the blink layout tests and the unit tests, but I have not > actually been able to test the code for real, yet. I'm struggling a little > with > getting either of the linux versions use my USB BLE dongle to talk to my > bluetooth devices. > > If you think the code looks ok, and you want the change in ASAP, feel free > to > LGTM. If you feel that it should be tested better before committing, then > you > can wait until I've figured out how (or test it yourself, if you have a > setup > that works). > > Description: > Tidy up state and services for bluez on disconnect > > This should fix cases where you connect, disconnect and then reconnect > to a BLE device. I've extended the ServicesDiscovered unit test to test > this. > > BUG=577641 > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+39, -4 lines): > M device/bluetooth/bluetooth_adapter_bluez.cc > M device/bluetooth/bluetooth_device.cc > M device/bluetooth/bluetooth_device_android.cc > M device/bluetooth/bluetooth_gatt_bluez_unittest.cc > > > Index: device/bluetooth/bluetooth_adapter_bluez.cc > diff --git a/device/bluetooth/bluetooth_adapter_bluez.cc > b/device/bluetooth/bluetooth_adapter_bluez.cc > index > 4b50cd72f86bcea6ba5dcb6339bb74417af5840b..2f55fa2d9f17d7bfd2973d9a7a8e09ed4c1d9c46 > 100644 > --- a/device/bluetooth/bluetooth_adapter_bluez.cc > +++ b/device/bluetooth/bluetooth_adapter_bluez.cc > @@ -493,9 +493,12 @@ void BluetoothAdapterBlueZ::DevicePropertyChanged( > // PlayStation joystick tries to reconnect after disconnection from > USB. > // If it is still not trusted, set it, so it becomes available on the > // list of known devices. > - if (properties->connected.value() && device_bluez->IsTrustable() && > - !properties->trusted.value()) > - device_bluez->SetTrusted(); > + if (properties->connected.value()) { > + if (device_bluez->IsTrustable() && !properties->trusted.value()) > + device_bluez->SetTrusted(); > + } else { > + device_bluez->SetGattServicesDiscoveryComplete(false); > + } > > int count = 0; > > Index: device/bluetooth/bluetooth_device.cc > diff --git a/device/bluetooth/bluetooth_device.cc > b/device/bluetooth/bluetooth_device.cc > index > 05532cc2b071b831cbc744c2d107cf6c07da519f..e3dc817f2548eaa6207ed7a9dfed7a54e2ebe8c8 > 100644 > --- a/device/bluetooth/bluetooth_device.cc > +++ b/device/bluetooth/bluetooth_device.cc > @@ -218,6 +218,8 @@ void BluetoothDevice::CreateGattConnection( > > void BluetoothDevice::SetGattServicesDiscoveryComplete(bool complete) { > gatt_services_discovery_complete_ = complete; > + if (!complete) > + gatt_services_.clear(); > } > > bool BluetoothDevice::IsGattServicesDiscoveryComplete() const { > Index: device/bluetooth/bluetooth_device_android.cc > diff --git a/device/bluetooth/bluetooth_device_android.cc > b/device/bluetooth/bluetooth_device_android.cc > index > 71ceb666998d4d66ba56ba4138cb005e4fe6c78e..cf585b17d82520fc29cbb5b3095be78b3f393677 > 100644 > --- a/device/bluetooth/bluetooth_device_android.cc > +++ b/device/bluetooth/bluetooth_device_android.cc > @@ -210,7 +210,6 @@ void BluetoothDeviceAndroid::OnConnectionStateChange( > if (gatt_connected_) { > DidConnectGatt(); > } else { > - gatt_services_.clear(); > SetGattServicesDiscoveryComplete(false); > > switch (status) { // Constants are from > android.bluetooth.BluetoothGatt. > Index: device/bluetooth/bluetooth_gatt_bluez_unittest.cc > diff --git a/device/bluetooth/bluetooth_gatt_bluez_unittest.cc > b/device/bluetooth/bluetooth_gatt_bluez_unittest.cc > index > 575c867b2f56ebccf49aba049ce136f1f0aacba6..cc7e6cfa1f12cad81f92a2b3b880430419a77064 > 100644 > --- a/device/bluetooth/bluetooth_gatt_bluez_unittest.cc > +++ b/device/bluetooth/bluetooth_gatt_bluez_unittest.cc > @@ -377,10 +377,41 @@ TEST_F(BluetoothGattBlueZTest, ServicesDiscovered) { > properties->gatt_services.ReplaceValue( > fake_bluetooth_gatt_service_client_->GetServices()); > > + EXPECT_TRUE(device->IsGattServicesDiscoveryComplete()); > + EXPECT_EQ(1u, device->GetGattServices().size()); > EXPECT_EQ(1, observer.gatt_services_discovered_count()); > EXPECT_EQ(device, observer.last_device()); > EXPECT_EQ(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress, > observer.last_device_address()); > + > + // Disconnect from the device > + device->Disconnect(base::Bind(&BluetoothGattBlueZTest::SuccessCallback, > + base::Unretained(this)), > + base::Bind(&BluetoothGattBlueZTest::ErrorCallback, > + base::Unretained(this))); > + fake_bluetooth_gatt_service_client_->HideHeartRateService(); > + properties->connected.ReplaceValue(false); > + > + EXPECT_FALSE(device->IsConnected()); > + EXPECT_FALSE(device->IsGattServicesDiscoveryComplete()); > + EXPECT_EQ(0u, device->GetGattServices().size()); > + > + // Verify that the device can be connected to again > + device->CreateGattConnection( > + base::Bind(&BluetoothGattBlueZTest::GattConnectionCallback, > + base::Unretained(this)), > + base::Bind(&BluetoothGattBlueZTest::ConnectErrorCallback, > + base::Unretained(this))); > + properties->connected.ReplaceValue(true); > + EXPECT_TRUE(device->IsConnected()); > + > + // Verify that service discovery can be done again > + fake_bluetooth_gatt_service_client_->ExposeHeartRateService( > + dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath)); > + properties->gatt_services.ReplaceValue( > + fake_bluetooth_gatt_service_client_->GetServices()); > + EXPECT_TRUE(device->IsGattServicesDiscoveryComplete()); > + EXPECT_EQ(1u, device->GetGattServices().size()); > } > > TEST_F(BluetoothGattBlueZTest, GattCharacteristicAddedAndRemoved) { > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
ortuno@chromium.org changed reviewers: + armansito@chromium.org
armansito: Can you confirm that we shouldn't be manually deleting GATT Services upon disconnection for Bluez? Bluez should handle this, correct? https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device.cc (right): https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.cc:222: gatt_services_.clear(); We shouldn't be deleting the services on all platforms. armansito can confirm but I believe bluez should be deleting the services upon disconnection. If it's not then it's a bug in bluez. https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_gatt_bluez_unittest.cc (right): https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_gatt_bluez_unittest.cc:387: // Disconnect from the device Colon at the end of a sentence. https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_gatt_bluez_unittest.cc:399: // Verify that the device can be connected to again Colon at the end of a sentence. https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_gatt_bluez_unittest.cc:408: // Verify that service discovery can be done again Colon at the end of a sentence.
On 2016/01/19 17:55:46, ortuno wrote: > armansito: Can you confirm that we shouldn't be manually deleting GATT Services > upon disconnection for Bluez? Bluez should handle this, correct? > Yes, IIRC when a device disconnects BlueZ will take down the GATT service objects IF there is no bond between the devices. Otherwise it should persist the cache and keep the services around.
Description was changed from ========== Tidy up state and services for bluez on disconnect This should fix cases where you connect, disconnect and then reconnect to a BLE device. I've extended the ServicesDiscovered unit test to test this. BUG=577641 ========== to ========== Tidy up service discovery state for bluez on disconnect This should fix cases where you connect, disconnect and then reconnect to a BLE device. I've extended the ServicesDiscovered unit test to test this. BUG=577641 ==========
I've uploaded a patchset that should address the issues from code review (i.e. removing the code I added for clearing the services and fixing comments). https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device.cc (right): https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.cc:222: gatt_services_.clear(); On 2016/01/19 17:55:45, ortuno wrote: > We shouldn't be deleting the services on all platforms. armansito can confirm > but I believe bluez should be deleting the services upon disconnection. If it's > not then it's a bug in bluez. Done. https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_gatt_bluez_unittest.cc (right): https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_gatt_bluez_unittest.cc:387: // Disconnect from the device On 2016/01/19 17:55:45, ortuno wrote: > Colon at the end of a sentence. Done. https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_gatt_bluez_unittest.cc:399: // Verify that the device can be connected to again On 2016/01/19 17:55:45, ortuno wrote: > Colon at the end of a sentence. Done. https://codereview.chromium.org/1606523002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_gatt_bluez_unittest.cc:408: // Verify that service discovery can be done again On 2016/01/19 17:55:46, ortuno wrote: > Colon at the end of a sentence. Done.
lgtm
The CQ bit was checked by scheib@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/1606523002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1606523002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
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/1606523002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1606523002/20001
Message was sent while issue was closed.
Description was changed from ========== Tidy up service discovery state for bluez on disconnect This should fix cases where you connect, disconnect and then reconnect to a BLE device. I've extended the ServicesDiscovered unit test to test this. BUG=577641 ========== to ========== Tidy up service discovery state for bluez on disconnect This should fix cases where you connect, disconnect and then reconnect to a BLE device. I've extended the ServicesDiscovered unit test to test this. BUG=577641 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Tidy up service discovery state for bluez on disconnect This should fix cases where you connect, disconnect and then reconnect to a BLE device. I've extended the ServicesDiscovered unit test to test this. BUG=577641 ========== to ========== Tidy up service discovery state for bluez on disconnect This should fix cases where you connect, disconnect and then reconnect to a BLE device. I've extended the ServicesDiscovered unit test to test this. BUG=577641 Committed: https://crrev.com/9e68515b6ef4fe5a87db25e6ae16014091f3dfa5 Cr-Commit-Position: refs/heads/master@{#370628} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9e68515b6ef4fe5a87db25e6ae16014091f3dfa5 Cr-Commit-Position: refs/heads/master@{#370628} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
