Handle registeration of multiple services.
When making any changes to the GATT attributes, we need to unregister the
full GATT application, and register it with the changed attributes. Currently
when removing services, we do this, but not when adding more services. This
CL fixes that.
[email protected],[email protected]
BUG=596742
Review-Url: https://codereview.chromium.org/1955813003
Cr-Commit-Position: refs/heads/master@{#392485}
diff --git a/device/BUILD.gn b/device/BUILD.gn
index 7427527..a463d99 100644
--- a/device/BUILD.gn
+++ b/device/BUILD.gn
@@ -29,6 +29,7 @@
"bluetooth/bluetooth_discovery_filter_unittest.cc",
"bluetooth/bluetooth_local_gatt_characteristic_unittest.cc",
"bluetooth/bluetooth_local_gatt_descriptor_unittest.cc",
+ "bluetooth/bluetooth_local_gatt_service_unittest.cc",
"bluetooth/bluetooth_low_energy_win_unittest.cc",
"bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc",
"bluetooth/bluetooth_remote_gatt_descriptor_unittest.cc",
diff --git a/device/bluetooth/bluetooth_local_gatt_service_unittest.cc b/device/bluetooth/bluetooth_local_gatt_service_unittest.cc
new file mode 100644
index 0000000..725bdcf8
--- /dev/null
+++ b/device/bluetooth/bluetooth_local_gatt_service_unittest.cc
@@ -0,0 +1,64 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/memory/weak_ptr.h"
+#include "device/bluetooth/bluetooth_local_gatt_characteristic.h"
+#include "device/bluetooth/test/bluetooth_gatt_server_test.h"
+#include "device/bluetooth/test/bluetooth_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace device {
+
+class BluetoothLocalGattServiceTest : public BluetoothGattServerTest {
+ public:
+ void SetUp() override {
+ BluetoothGattServerTest::SetUp();
+
+ StartGattSetup();
+ CompleteGattSetup();
+ }
+
+ protected:
+ base::WeakPtr<BluetoothLocalGattCharacteristic> read_characteristic_;
+ base::WeakPtr<BluetoothLocalGattCharacteristic> write_characteristic_;
+};
+
+#if defined(OS_CHROMEOS) || defined(OS_LINUX)
+TEST_F(BluetoothLocalGattServiceTest, RegisterMultipleServices) {
+ base::WeakPtr<BluetoothLocalGattService> service2 =
+ BluetoothLocalGattService::Create(
+ adapter_.get(), BluetoothUUID(kTestUUIDGenericAttribute), true,
+ nullptr, delegate_.get());
+ base::WeakPtr<BluetoothLocalGattService> service3 =
+ BluetoothLocalGattService::Create(
+ adapter_.get(), BluetoothUUID(kTestUUIDGenericAttribute), true,
+ nullptr, delegate_.get());
+ base::WeakPtr<BluetoothLocalGattService> service4 =
+ BluetoothLocalGattService::Create(
+ adapter_.get(), BluetoothUUID(kTestUUIDGenericAttribute), true,
+ nullptr, delegate_.get());
+ service2->Register(GetCallback(Call::EXPECTED),
+ GetGattErrorCallback(Call::NOT_EXPECTED));
+ service3->Register(GetCallback(Call::EXPECTED),
+ GetGattErrorCallback(Call::NOT_EXPECTED));
+
+ service2->Unregister(GetCallback(Call::EXPECTED),
+ GetGattErrorCallback(Call::NOT_EXPECTED));
+
+ service4->Register(GetCallback(Call::EXPECTED),
+ GetGattErrorCallback(Call::NOT_EXPECTED));
+
+ service3->Register(GetCallback(Call::NOT_EXPECTED),
+ GetGattErrorCallback(Call::EXPECTED));
+ service3->Unregister(GetCallback(Call::EXPECTED),
+ GetGattErrorCallback(Call::NOT_EXPECTED));
+
+ service4->Unregister(GetCallback(Call::EXPECTED),
+ GetGattErrorCallback(Call::NOT_EXPECTED));
+ // TODO(rkc): Once we add a IsRegistered method to service, use it to add
+ // more verification to this test.
+}
+#endif // defined(OS_CHROMEOS) || defined(OS_LINUX)
+
+} // namespace device
diff --git a/device/bluetooth/bluez/bluetooth_adapter_bluez.cc b/device/bluetooth/bluez/bluetooth_adapter_bluez.cc
index c6a886ea..ef9f81cf 100644
--- a/device/bluetooth/bluez/bluetooth_adapter_bluez.cc
+++ b/device/bluetooth/bluez/bluetooth_adapter_bluez.cc
@@ -107,10 +107,16 @@
void OnRegisterationErrorCallback(
const device::BluetoothGattService::ErrorCallback& error_callback,
+ bool is_register_callback,
const std::string& error_name,
const std::string& error_message) {
- VLOG(1) << "Failed to [Un]register service: " << error_name << ", "
- << error_message;
+ if (is_register_callback) {
+ VLOG(1) << "Failed to Register service: " << error_name << ", "
+ << error_message;
+ } else {
+ VLOG(1) << "Failed to Unregister service: " << error_name << ", "
+ << error_message;
+ }
error_callback.Run(
BluetoothGattServiceBlueZ::DBusErrorToServiceError(error_name));
}
@@ -1080,11 +1086,20 @@
}
registered_gatt_services_[service->object_path()] = service;
- gatt_application_provider_ = BluetoothGattApplicationServiceProvider::Create(
- bluez::BluezDBusManager::Get()->GetSystemBus(),
- GetApplicationObjectPath(), registered_gatt_services_);
- RegisterApplication(callback, error_callback);
+ // Always assume that we were already registered. If we weren't registered
+ // we'll just get an error back which we can ignore. Any other approach will
+ // introduce a race since we will always have a period when we may have been
+ // registered with BlueZ, but not know that the registration succeeded
+ // because the callback hasn't come back yet.
+ bluez::BluezDBusManager::Get()
+ ->GetBluetoothGattManagerClient()
+ ->UnregisterApplication(
+ object_path_, GetApplicationObjectPath(),
+ base::Bind(&BluetoothAdapterBlueZ::RegisterApplication,
+ weak_ptr_factory_.GetWeakPtr(), callback, error_callback),
+ base::Bind(&BluetoothAdapterBlueZ::RegisterApplicationOnError,
+ weak_ptr_factory_.GetWeakPtr(), callback, error_callback));
}
void BluetoothAdapterBlueZ::UnregisterGattService(
@@ -1101,34 +1116,24 @@
}
registered_gatt_services_.erase(service->object_path());
-
// If we have no GATT services left, unregister our application.
if (registered_gatt_services_.size() == 0) {
bluez::BluezDBusManager::Get()
->GetBluetoothGattManagerClient()
->UnregisterApplication(
object_path_, GetApplicationObjectPath(), callback,
- base::Bind(&OnRegisterationErrorCallback, error_callback));
+ base::Bind(&OnRegisterationErrorCallback, error_callback, false));
return;
}
- // Otherwise, this is tricky (since at the moment, BlueZ does not support
- // adding/removing services individually). We need to update our list of
- // services, then unregister our application, then re-register it with the
- // updated services. TODO(rkc): Fix this once BlueZ is fixed.
- gatt_application_provider_ = BluetoothGattApplicationServiceProvider::Create(
- bluez::BluezDBusManager::Get()->GetSystemBus(), object_path_,
- registered_gatt_services_);
-
- // Unregister our current application. If we are successful, make a call to
- // register the application again with the current set of services.
+ // Re-register our current GATT application with the new set of services.
bluez::BluezDBusManager::Get()
->GetBluetoothGattManagerClient()
->UnregisterApplication(
object_path_, GetApplicationObjectPath(),
base::Bind(&BluetoothAdapterBlueZ::RegisterApplication,
weak_ptr_factory_.GetWeakPtr(), callback, error_callback),
- base::Bind(&OnRegisterationErrorCallback, error_callback));
+ base::Bind(&OnRegisterationErrorCallback, error_callback, false));
}
// Returns the object path of the adapter.
@@ -1573,13 +1578,28 @@
void BluetoothAdapterBlueZ::RegisterApplication(
const base::Closure& callback,
const device::BluetoothGattService::ErrorCallback& error_callback) {
+ // Recreate our application service provider with the currently registered
+ // GATT services before we register it.
+ gatt_application_provider_.reset();
+ gatt_application_provider_ = BluetoothGattApplicationServiceProvider::Create(
+ bluez::BluezDBusManager::Get()->GetSystemBus(),
+ GetApplicationObjectPath(), registered_gatt_services_);
+
DCHECK(bluez::BluezDBusManager::Get());
bluez::BluezDBusManager::Get()
->GetBluetoothGattManagerClient()
->RegisterApplication(
object_path_, GetApplicationObjectPath(),
BluetoothGattManagerClient::Options(), callback,
- base::Bind(&OnRegisterationErrorCallback, error_callback));
+ base::Bind(&OnRegisterationErrorCallback, error_callback, true));
+}
+
+void BluetoothAdapterBlueZ::RegisterApplicationOnError(
+ const base::Closure& callback,
+ const device::BluetoothGattService::ErrorCallback& error_callback,
+ const std::string& /* error_name */,
+ const std::string& /* error_message */) {
+ RegisterApplication(callback, error_callback);
}
} // namespace bluez
diff --git a/device/bluetooth/bluez/bluetooth_adapter_bluez.h b/device/bluetooth/bluez/bluetooth_adapter_bluez.h
index c989d7d6..4a387429 100644
--- a/device/bluetooth/bluez/bluetooth_adapter_bluez.h
+++ b/device/bluetooth/bluez/bluetooth_adapter_bluez.h
@@ -375,6 +375,14 @@
const base::Closure& callback,
const device::BluetoothGattService::ErrorCallback& error_callback);
+ // Register application, ignoring the given errors. Used to register a GATT
+ // application even if a previous unregister application call fails.
+ void RegisterApplicationOnError(
+ const base::Closure& callback,
+ const device::BluetoothGattService::ErrorCallback& error_callback,
+ const std::string& error_name,
+ const std::string& error_message);
+
InitCallback init_callback_;
bool initialized_;
diff --git a/device/bluetooth/dbus/fake_bluetooth_gatt_application_service_provider.cc b/device/bluetooth/dbus/fake_bluetooth_gatt_application_service_provider.cc
index a52491d..ea65bdb 100644
--- a/device/bluetooth/dbus/fake_bluetooth_gatt_application_service_provider.cc
+++ b/device/bluetooth/dbus/fake_bluetooth_gatt_application_service_provider.cc
@@ -18,7 +18,7 @@
const std::map<dbus::ObjectPath, BluetoothLocalGattServiceBlueZ*>&
services)
: object_path_(object_path) {
- VLOG(1) << "Creating Bluetooth GATT service: " << object_path_.value();
+ VLOG(1) << "Creating Bluetooth GATT application: " << object_path_.value();
FakeBluetoothGattManagerClient* fake_bluetooth_gatt_manager_client =
static_cast<FakeBluetoothGattManagerClient*>(
diff --git a/device/bluetooth/dbus/fake_bluetooth_gatt_manager_client.cc b/device/bluetooth/dbus/fake_bluetooth_gatt_manager_client.cc
index 34777b8b..1d06f3f 100644
--- a/device/bluetooth/dbus/fake_bluetooth_gatt_manager_client.cc
+++ b/device/bluetooth/dbus/fake_bluetooth_gatt_manager_client.cc
@@ -105,10 +105,14 @@
VLOG(1) << "Register GATT application: " << application_path.value();
ApplicationProvider* provider =
GetApplicationServiceProvider(application_path);
- if (!provider || provider->second)
+ if (!provider || provider->second) {
error_callback.Run(bluetooth_gatt_service::kErrorFailed, "");
- if (!VerifyProviderHierarchy(provider->first))
+ return;
+ }
+ if (!VerifyProviderHierarchy(provider->first)) {
error_callback.Run(bluetooth_gatt_service::kErrorFailed, "");
+ return;
+ }
provider->second = true;
callback.Run();
}
@@ -122,8 +126,10 @@
VLOG(1) << "Unregister GATT application: " << application_path.value();
ApplicationProvider* provider =
GetApplicationServiceProvider(application_path);
- if (!provider || !provider->second)
+ if (!provider || !provider->second) {
error_callback.Run(bluetooth_gatt_service::kErrorFailed, "");
+ return;
+ }
provider->second = false;
callback.Run();
}
diff --git a/device/bluetooth/test/bluetooth_gatt_server_test.cc b/device/bluetooth/test/bluetooth_gatt_server_test.cc
index 430f649..232b39c 100644
--- a/device/bluetooth/test/bluetooth_gatt_server_test.cc
+++ b/device/bluetooth/test/bluetooth_gatt_server_test.cc
@@ -28,26 +28,17 @@
void BluetoothGattServerTest::CompleteGattSetup() {
service_->Register(GetCallback(Call::EXPECTED),
GetGattErrorCallback(Call::NOT_EXPECTED));
- EXPECT_EQ(1, callback_count_);
- EXPECT_EQ(0, error_callback_count_);
}
void BluetoothGattServerTest::SetUp() {
BluetoothTest::SetUp();
-
last_read_value_ = std::vector<uint8_t>();
}
void BluetoothGattServerTest::TearDown() {
- int callback_count = callback_count_;
- int error_callback_count = error_callback_count_;
service_->Unregister(GetCallback(Call::EXPECTED),
GetGattErrorCallback(Call::NOT_EXPECTED));
- EXPECT_EQ(callback_count + 1, callback_count_);
- EXPECT_EQ(error_callback_count, error_callback_count_);
-
- delegate_ = nullptr;
-
+ delegate_.reset();
BluetoothTest::TearDown();
}
diff --git a/device/device_tests.gyp b/device/device_tests.gyp
index 4f40cb2..9903675d 100644
--- a/device/device_tests.gyp
+++ b/device/device_tests.gyp
@@ -46,6 +46,7 @@
'bluetooth/bluetooth_discovery_filter_unittest.cc',
'bluetooth/bluetooth_local_gatt_characteristic_unittest.cc',
'bluetooth/bluetooth_local_gatt_descriptor_unittest.cc',
+ 'bluetooth/bluetooth_local_gatt_service_unittest.cc',
'bluetooth/bluetooth_low_energy_win_unittest.cc',
'bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc',
'bluetooth/bluetooth_remote_gatt_descriptor_unittest.cc',
