bluetooth: mac: add connected LE devices to chooser

device/bluetooth/bluetooth_adapter_mac.mm

Index: device/bluetooth/bluetooth_adapter_mac.mm
diff --git a/device/bluetooth/bluetooth_adapter_mac.mm b/device/bluetooth/bluetooth_adapter_mac.mm
index 61d169e2c21937557362c21eb9b3d2f77e15d2f5..79de647f192e750e2c0217b4cab00d8a61a233f4 100644
--- a/device/bluetooth/bluetooth_adapter_mac.mm
+++ b/device/bluetooth/bluetooth_adapter_mac.mm
@@ -170,6 +170,27 @@ bool BluetoothAdapterMac::IsDiscovering() const {
   return is_discovering;
 }
 
+void BluetoothAdapterMac::RetrievedConnectedPeripherals() {

[ortuno, 2016/10/06 02:22:07]

+scheib for thoughts. Some context: We are adding a new function to retrieve
connected devices. These devices are probably not advertising and platforms
require us to manually query for these.

I don't think this function need to do the lower half of LowEnergyDeviceUpdated:

const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()

1. Initialize connected_devices to be an empty std::vector<BluetoothDevice*>
2. Query the platform for GATT connected devices
3. For each device in Step 2.:
  1. If we don't know of the device already, initialize a new BluetoothDevice
and add it to the list a known devices. Otherwise, retrieve the BluetoothDevice
representing device.
  2. Add the BluetoothDevice from step 1 to connected_devices.
4. Return connected_devices.

Note that we won't be calling DeviceAdded for new devices. I think calling
DeviceAdded in the middle of a synchronous function will be confusing and cause
bugs for clients. Furthermore DeviceAdded is only useful for OS clients that
need to know everything about all devices. In the next iteration of the API we
will probably not have such an event.

[ortuno, 2016/10/06 02:22:52]

+scheib for realz

[scheib, 2016/10/06 21:01:34]

RetrievedConnectedPeripherals seems to be incorrectly named, should be
RetrieveConnectedPeripherals.

Even with a changing API, we need to be consistent with the current style and
API.

The best way to avoid bugs with re-entrant calls to ::Observers is to PostTask
instead of calling the observers directly. We could consider changing
LowEnergyDeviceUpdated now to use PostTask (but this will likely require test
changes as well - and should be split to another patch).

If a new device is identified, I think that DeviceAdded needs to be called.
Clients need to be able to have the full list of devices known trusting that
DeviceAdded will offer them, as specified by that method [1]

Ditto for DeviceChanged I think, but it would be best if we only sent those
updates for devices that actually did change. I could see that as not practical.
I'd be happy enough with RetrieveConnectedPeripherals calling
LowEnergyDeviceUpdated in a way that only generated DeviceAdded calls.


[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?typ...

[ortuno, 2016/10/06 23:43:12]

Right, but we still have the problem of providing the same device to clients in
two different ways (through the return value of this function and through
DeviceAdded) and of returning devices for which we haven't called DeviceAdded.
Both of these will lead to very subtle bugs since this will only happen on macOS
and Android. If we really wanted to keep the style of the API, we wouldn't have
this function but rather we would poll like we do for the adapter name and
paired devices. The introduction of this function is trying to avoid that.
I would like to push clients of the API to use DeviceAdded as a signal that the
device was discovered during a scanning procedure. Having this function not call
DeviceAdded would fit into that model.

[scheib, 2016/10/07 17:08:00]

This method doesn't return devices. It causes connected devices to be
discovered. Perhaps we can think about it as another kind of scan? DeviceAdded
is the only way clients find the new devices.

I think I'm OK with that.



As an alternative, should GetDevices implementation discover these devices? And
the Web Bluetooth for example would start by GetDevices, filter out only the
ones that are connected, and add those to the chooser, then start a scan and any
DeviceAdded devices get added as well.

[ortuno, 2016/10/10 02:57:22]

The current method as is does not return devices but when I proposed the method
I thought it should and I still think it should. Having a synchronous method is
the easiest to use and implement:

1. Android: We would use BluetoothManager.getConnectedDevices() and create the
new devices if we haven't seen them before.
2. macOS: We would use retrieveConnectedPeripheralsWithServices and create the
new devices if we haven't seen them before.
3. BlueZ: We would iterate over the currently known devices and return the ones
that are connected.
4. Windows: Enumerate all LE devices and return the connected ones. (In fact I
think the current implementation of tying this to a Discovery session is wrong.)

Clients will simply need to do:

for (BluetoothDevice* device : adapter->GetConnectedDevices()) {
  // Do something with the device.
}

Having methods that abstract bluetooth concepts or build on top of them is very
annoying e.g. all the Device/Service/Characteristic/Descriptor Added/Removed
events, GetUUIDs, DeviceChanged, etc. add more complexity to the implementation,
means clients need to understand yet another concept and all its subtleties, and
sometimes result in clients not being able to implement features e.g. we had
many problems with the chooser because of how DeviceChanged worked and Arc++ is
still not able to implement advertisements because of DeviceChanged.

Although I agree that we should keep the style of the API, I also think that
whatever work we do to move clients to use the API in a way that is compatible
with the eventual mojo API will save future refactoring work.

Overall I think exposing low level APIs reduces the API complexity, reduces the
burden on clients, allows clients to implement more powerful features and will
be easier to refactor.

[jlebel, 2016/10/13 15:29:02]

Acknowledged.

+  // It is not possible to ask for all connected peripherals with
+  // -[CBCentralManager retrieveConnectedPeripheralsWithServices:] by passing
+  // nil. To try to get most of the peripherals, the search is done with
+  // Generic Access service.
+  CBUUID* genericAccessServiceUUID = [CBUUID UUIDWithString:@"1800"];

[ortuno, 2016/10/12 06:42:46]

I've been thinking about this and I think we need to do more on this method. As
proposed, this function will be adding new BluetoothDevices for each new device
but those devices won't have any uuids so they will probably be filtered out.
Does [CBPeripheral services] return any services that we can use to populate the
uuids member? Otherwise developers might have to retrieve all connected devices
and force users to pick the right connected device on their own D:

[jlebel, 2016/10/13 15:29:01]

I've tried, and yes, the devices are filtered out because CBPeripheral are not
connected, and therefore, we have no informations on their services.

I guess to solve that problem we would need to give the service used to filter
to this method, and add the service to the device.

[ortuno, 2016/10/14 03:03:03]

To summarize the problem: Peripherals returned by retrieveConnectedPeripherals
do not include any information about their services. We need to find a way to
expose that information.

I think passing the filter to only get a set of devices that match the filters
makes sense but there are a couple of subtleties we need to consider:
  - How does retrieveConnectedPeripheralsWithServices's filters work? If a
device only has a Heart Rate Service and the filter has Glucose and Heart Rate
would the function find the device? (I might be missing something but I don't
think the documentation is clear here)
  - How do we integrate the services with the current GetUUIDs model?
  - How long do we persist these devices and their UUIDs?

I thought of the following approaches:

  1. We would call DeviceAdded for each new device, set the device's UUIDs to
the filter's UUIDs if IsGattDiscoveryComplete is false, call DeviceChanged() and
clear the UUIDs.

Pros:
 - Ensures the UUIDs are always up to date.
 - Doesn't affect old connected devices.
 - DeviceAdded and DeviceChanged allow other clients to stay up to date
regarding peripherals.
Cons:
 - Yet another concept that we pile up on GetUUIDs() D:
 - The fact that the uuids are cleared after DeviceChanged runs is very subtle.

2. Another approach would be to call DeviceAdded for each new device and if the
filter has UUIDs not in the device add them and call DeviceChanged.

Pros:
  - GetUUIDs is more stable
  - DeviceAdded and DeviceChanged allow other clients to stay up to date
regarding peripherals.
Cons:
  - Yet another concept that we pile up on GetUUIDs() D:
  - GetUUIDs could return uuids that are no longer present in the device or we
would have to implement some sort of time out for these uuids.

3. A third option would be to call DeviceAdded for each new device and then just
return all the connected devices that match the filters.

Pros:
  - Easy to implement: No need to mess with GetUUIDs() or DeviceChanged.
Cons: 
  - For some devices, we would be calling DeviceAdded and then returning the
device.
  - We won't be exposing known UUIDs through GetUUIDs() or DeviceChanged meaning
bluetooth-internals won't get notified of the device's UUIDs.

I don't think any option is ideal D: but I'm leaning towards the last approach.
Adding more things to GetUUIDs() would be very unfortunate.

jlebel, scheib: wdyt?

[scheib, 2016/10/14 17:16:14]

:( 

I'm a tad confused on if we have the UUIDs to filter for. If we have multiple
UUIDs in a call to retrieveConnectedPeripheralsWithServices, does it return all
devices with any UUID that matches, or only those with every UUID? Either way I
don't think it helps -- we need to find devices that match any of multiple UUID
filters, but then when Web Bluetooth filters them for the chooser we need to
know more specifically which UUIDs are associated with each device.

If we can call retrieveConnectedPeripheralsWithServices with a useful set of
UUIDs, I'd suggest:

call retrieveConnectedPeripheralsWithServices
for each peripheral
 call LowEnergyDeviceUpdated with synthetic advertisementData that we construct
including the UUIDs associated with that device.
A DeviceAdded should result that includes the correct UUIDs

But, as mentioned above, perhaps that's not possible. In that case:

I wonder if it is possible to cause the services to be discovered for these
connected devices? Calling discoverServices on each one. Then that process
should complete and we can get a DeviceUpdated that has known UUIDs?

+  NSArray* connectedServices = @[ genericAccessServiceUUID ];
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wpartial-availability"
+  // Can remove ignore -Wpartial-availability when 10.8 will not be supported
+  // anymore.
+  // TODO(crbug.com/653056)
+  NSArray* peripherals = [low_energy_central_manager_
+      retrieveConnectedPeripheralsWithServices:connectedServices];
+#pragma clang diagnostic pop
+  for (CBPeripheral* peripheral in peripherals) {
+    LowEnergyDeviceUpdated(peripheral, nil /* advertisementData */,
+                           0 /* rssi */);
+  }
+}
+
 BluetoothAdapter::UUIDList BluetoothAdapterMac::GetUUIDs() const {
   NOTIMPLEMENTED();
   return UUIDList();
@@ -513,40 +534,42 @@ void BluetoothAdapterMac::LowEnergyDeviceUpdated(
 
   DCHECK(device_mac);
 
-  // Get Advertised UUIDs
-  BluetoothDevice::UUIDList advertised_uuids;
-  NSArray* service_uuids =
-      [advertisement_data objectForKey:CBAdvertisementDataServiceUUIDsKey];
-  for (CBUUID* uuid in service_uuids) {
-    advertised_uuids.push_back(BluetoothUUID([[uuid UUIDString] UTF8String]));
-  }
-  NSArray* overflow_service_uuids = [advertisement_data
-      objectForKey:CBAdvertisementDataOverflowServiceUUIDsKey];
-  for (CBUUID* uuid in overflow_service_uuids) {
-    advertised_uuids.push_back(BluetoothUUID([[uuid UUIDString] UTF8String]));
-  }
+  if (advertisement_data) {
+    // Get Advertised UUIDs
+    BluetoothDevice::UUIDList advertised_uuids;
+    NSArray* service_uuids =
+        [advertisement_data objectForKey:CBAdvertisementDataServiceUUIDsKey];
+    for (CBUUID* uuid in service_uuids) {
+      advertised_uuids.push_back(BluetoothUUID([[uuid UUIDString] UTF8String]));
+    }
+    NSArray* overflow_service_uuids = [advertisement_data
+        objectForKey:CBAdvertisementDataOverflowServiceUUIDsKey];
+    for (CBUUID* uuid in overflow_service_uuids) {
+      advertised_uuids.push_back(BluetoothUUID([[uuid UUIDString] UTF8String]));
+    }
 
-  // Get Service Data.
-  BluetoothDevice::ServiceDataMap service_data_map;
-  NSDictionary* service_data =
-      [advertisement_data objectForKey:CBAdvertisementDataServiceDataKey];
-  for (CBUUID* uuid in service_data) {
-    NSData* data = [service_data objectForKey:uuid];
-    const uint8_t* bytes = static_cast<const uint8_t*>([data bytes]);
-    size_t length = [data length];
-    service_data_map.emplace(BluetoothUUID([[uuid UUIDString] UTF8String]),
-                             std::vector<uint8_t>(bytes, bytes + length));
-  }
+    // Get Service Data.
+    BluetoothDevice::ServiceDataMap service_data_map;
+    NSDictionary* service_data =
+        [advertisement_data objectForKey:CBAdvertisementDataServiceDataKey];
+    for (CBUUID* uuid in service_data) {
+      NSData* data = [service_data objectForKey:uuid];
+      const uint8_t* bytes = static_cast<const uint8_t*>([data bytes]);
+      size_t length = [data length];
+      service_data_map.emplace(BluetoothUUID([[uuid UUIDString] UTF8String]),
+                               std::vector<uint8_t>(bytes, bytes + length));
+    }
 
-  // Get Tx Power.
-  NSNumber* tx_power =
-      [advertisement_data objectForKey:CBAdvertisementDataTxPowerLevelKey];
-  int8_t clamped_tx_power = BluetoothDevice::ClampPower([tx_power intValue]);
+    // Get Tx Power.
+    NSNumber* tx_power =
+        [advertisement_data objectForKey:CBAdvertisementDataTxPowerLevelKey];
+    int8_t clamped_tx_power = BluetoothDevice::ClampPower([tx_power intValue]);
 
-  device_mac->UpdateAdvertisementData(
-      BluetoothDevice::ClampPower(rssi), std::move(advertised_uuids),
-      std::move(service_data_map),
-      tx_power == nil ? nullptr : &clamped_tx_power);
+    device_mac->UpdateAdvertisementData(
+        BluetoothDevice::ClampPower(rssi), std::move(advertised_uuids),
+        std::move(service_data_map),
+        tx_power == nil ? nullptr : &clamped_tx_power);
+  }
 
   if (is_new_device) {
     std::string device_address =