bluetooth: Add Device connection logic and accompanying user interface.

device/bluetooth/device.cc

Index: device/bluetooth/device.cc
diff --git a/device/bluetooth/device.cc b/device/bluetooth/device.cc
index 7e662802eabb5aed91c9859f782903049c866170..b6b493587713cd1852d211c6c7f5f0b64555ac91 100644
--- a/device/bluetooth/device.cc
+++ b/device/bluetooth/device.cc
@@ -10,9 +10,9 @@
 
 namespace bluetooth {
 
-Device::Device(const std::string& address,
-               scoped_refptr<device::BluetoothAdapter> adapter)
-    : address_(address), adapter_(std::move(adapter)) {}
+Device::Device(scoped_refptr<device::BluetoothAdapter> adapter,
+               std::unique_ptr<device::BluetoothGattConnection> connection)
+    : adapter_(std::move(adapter)), connection_(std::move(connection)) {}

[ortuno, 2016/10/25 10:42:09]

You mention that the pipe is tied to the lifetime of the connection but we never
close the pipe if the connection to the device dies. We should close the pipe
when the connection dies.

I think that would also require some extra logic to route the DeviceChanged
event. scheib: I think it's getting harder to keep this as a thin layer. Do you
think we should add some tests?

[mbrunson, 2016/10/25 20:03:04]

Hmm, ok. More logic would be needed in Adapter to handle this case.
Specifically, a collection (probably an unordered_set) of WeakPtrs to all of the
connected Device bindings. In DeviceChanged, we would forcibly close the
connection to a Device if the connection is dropped.

Alternatives?

[ortuno, 2016/10/26 02:52:21]

I don't think you need to add the logic in Adapter.
  1. Rather than creating a StrongBinding you could hold a regular Binding in
this class. If there is a connection error (e.g. the other side dropped the
pipe) or if the device disconnects for some reason then you can just close the
binding manually and delete Device.
  2. You would make Device a BluetoothAdapter::Observer that overrides
DeviceChanged. If BluetoothDevice in DeviceChanged has the same address as this
device and is no longer GattConnected then you can close the pipe and delete the
Device instance.

[mbrunson, 2016/10/28 21:06:47]

Oh ok. That makes sense. Done.

 
 Device::~Device() {}
 
@@ -25,6 +25,7 @@ mojom::DeviceInfoPtr Device::ConstructDeviceInfoStruct(
   device_info->name_for_display =
       base::UTF16ToUTF8(device->GetNameForDisplay());
   device_info->address = device->GetAddress();
+  device_info->connected = device->IsGattConnected();
 
   if (device->GetInquiryRSSI()) {
     device_info->rssi = mojom::RSSIWrapper::New();
@@ -35,7 +36,8 @@ mojom::DeviceInfoPtr Device::ConstructDeviceInfoStruct(
 }
 
 void Device::GetInfo(const GetInfoCallback& callback) {
-  device::BluetoothDevice* device = adapter_->GetDevice(address_);
+  device::BluetoothDevice* device =
+      adapter_->GetDevice(connection_->GetDeviceAddress());
   if (device) {
     mojom::DeviceInfoPtr device_info = ConstructDeviceInfoStruct(device);
     callback.Run(std::move(device_info));
@@ -44,4 +46,30 @@ void Device::GetInfo(const GetInfoCallback& callback) {
   }
 }
 
+void Device::GetServices(const GetServicesCallback& callback) {
+  device::BluetoothDevice* device =
+      adapter_->GetDevice(connection_->GetDeviceAddress());
+  std::vector<mojom::ServiceInfoPtr> services;
+
+  if (device) {
+    for (const device::BluetoothRemoteGattService* service :
+         device->GetGattServices()) {

[ortuno, 2016/10/25 10:42:09]

To avoid returning services that no longer exist or not returning any services
we shouldn't call GetGattServices if IsGattDiscoveryComplete is false.

Ideally we would have something like Web Bluetooth's getPrimaryServices()[1]
which waits for IsGattDiscoveryComplete() but that might be too much logic i.e.
you would have to queue the calls and this would have to become an observer of
the adapter and filter out events for other devices.

Alternatives that come to mind:

1. Add the event to the AdapterClient and keep track of which devices have
completed GATT Discovery. Though, we might introduce some races since
AdapterClient and Device use two different pipes. We would still need to add
this information to DeviceInfo.

2. Add a DiscoverServices() method that runs its callback when the device
finishes the GATT discovery procedure. This will probably result in the same
amount of logic as the Web Bluetooth method but at least is closer to the
current interface.

3. Add a IsGattDiscoveryComplete() method that returns whether or not discovery
is complete. We would poll until this becomes true. This would keep the
implementation simple but polling is really ugly.

scheib, mbrunson: Can you think of any other alternatives?

My vote would be for following Web Bluetooth's pattern i.e. GetServices runs its
callback after IsGattDiscoveryComplete becomes true. It is more logic but I
don't think the other alternatives are any better :/

[1]
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

[mbrunson, 2016/10/25 20:03:04]

The original pattern I was thinking of would be having the client wait for the
ServicesDiscovered callback before calling GetServices. Is this not recommended?
It does put more burden on the client.

Alternatively, we could wait for service discovery to complete when a device
connection is requested, but this would require some kind of queue to run the
ConnectToDevice callback when discovery is complete. I think ortuno brought this
idea up one time before.

[ortuno, 2016/10/26 02:52:21]

Do you mean AdapterClient::ServicesDiscovered?

That event is only dispatched when the services are discovered so there is a
chance we might miss it e.g. User connects to a device and discovers its
services through Web Bluetooth. Then user opens the internals page and connects
to the device. Since the services have been discovered beforehand the
ServicesDiscovered event will never be dispatched.

Furthermore GATT discoveries can happen multiple times during a connection. So
we always need to check if the discovery is complete before returning the
services.
Good suggestion but abstracting two procedures into one could lead to loss of
functionality for clients. In the past we've had users that needed to measure
the time between a GATT connection and GATT Discovery, if we were to abstract
them under the same function this use case would no longer be possible.

[mbrunson, 2016/10/28 21:06:47]

Queue implemented with tests. Done.

+      mojom::ServiceInfoPtr service_info = ConstructServiceInfoStruct(service);
+      services.push_back(std::move(service_info));
+    }
+  }
+
+  callback.Run(std::move(services));
+}
+
+mojom::ServiceInfoPtr Device::ConstructServiceInfoStruct(

[ortuno, 2016/10/25 10:42:09]

nit: No need for this helper function. The code is only being used once and
there won't be a Service service.

[mbrunson, 2016/10/25 20:03:04]

The ServiceChanged callback in AdapterClient will require a ServiceInfo struct,
so eventually this will have to change to a public static function when that is
added.

+    const device::BluetoothRemoteGattService* service) {
+  mojom::ServiceInfoPtr service_info = mojom::ServiceInfo::New();
+
+  service_info->uuid = service->GetUUID();
+  service_info->primary = service->IsPrimary();
+
+  return service_info;
+}
+
 }  // namespace bluetooth