bluetooth: Add DeviceChanged logging in Device service.

device/bluetooth/public/interfaces/device.mojom

Index: device/bluetooth/public/interfaces/device.mojom
diff --git a/device/bluetooth/public/interfaces/device.mojom b/device/bluetooth/public/interfaces/device.mojom
index b629bab1703d8441c10271394e9345ce2845b566..860c3edfc91c6edd0517b29f1383cac22452f61a 100644
--- a/device/bluetooth/public/interfaces/device.mojom
+++ b/device/bluetooth/public/interfaces/device.mojom
@@ -4,6 +4,12 @@
 
 module bluetooth.mojom;
 
+struct AdvertisingPacket {
+  uint64 timestamp;
+  int8 rssi;
+  DeviceInfo device;
+};
+
 struct DeviceInfo {
   string? name;
   string name_for_display;
@@ -14,4 +20,18 @@ interface Device {
   // Gets basic information about the device. Returns null, if no device is
   // available.
   GetInfo() => (DeviceInfo? info);
+
+  // Sets the client that listens to events from this device.
+  SetClient(GattClient client);
+};
+
+interface GattClient {
+  // Called when one of the following properties of the device changes:
+  //   Address, appearance, Bluetooth class, Inquiry RSSI, Inquiry TX Power,
+  //   Service UUIDs, Connectionable state, Connection state, Pairing state,
+  //   Trustable state.
+  // Generally called for each advertisement packet recevied, but this is not
+  // guaranteed on ChromeOS or Linux. Because the RSSI is always changing,
+  // it's very likely this will be called for each advertising packet.
+  DeviceChanged(AdvertisingPacket packet);

[ortuno, 2016/10/13 22:22:49]

In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the adapter
client and it should match DeviceChanged more closely i.e. just call with the
address :(

Alternatively we could call it with a DeviceInfo struct since that would save
clients a round trip but that might be too much.

scheib: Do you have any thoughts?

[scheib, 2016/10/14 07:14:51]

We've deviated a bit already with DeviceInfo aggregating all the accessors for
e.g. GetName, GetAddress. If we continue that route and stick to the
DeviceChanged only replying with an address then the client will still need to
call GetInfo. At that point we'll need to include GetInquiryRSSI in the
DeviceInfo packet as well.

One could argue that populating those packet members will happen either in a
call to GetInfo or if we do it directly in a response to DeviceChanged. So, even
in our "just a thin wrapper" we'll have some untested accessors populating a
struct. If we know those only have meaning in a DeviceChanged event that I think
we may as well make the calls in that event and save the DeviceChanged ->
address -> GetInfo step and just return the AdvertisingPacket as written here.

In other words, I think it's OK to clean up naming, and move functionality into
better clusters of interfaces, when the work is doing the same thing regardless
(populating struct members with accessor calls).

What wouldn't be OK is when we start adding logic, caching state, or changing
the way algorithms work.

[mbrunson, 2016/10/14 18:25:25]

I remember ortuno recommending the Mojo clients to change, noting that the way
Mac or Android does it seems better. I was planning to follow the BluetoothGatt
pattern from Android where a single client attached to the device interface
handles the GATT-related callbacks for that device. Let me know if you think
this is a good idea because GattConnection and the Service interface will be my
next CLs.

[scheib, 2016/10/14 20:25:40]

I dislike the Android and Core Bluetooth way of getting results from e.g.
characteristic reads or notifications on a single client interface -- and then
needing to interpret / route the request. 

Mojo will make this easier for us, as we have an asynchronous response that can
provide a value. So, we should be able to characteristic.Read().then(status,
value); That's what is sketched out on the go/fizzbluetoothinternals doc.
Perhaps update that doc with GattClient (as is here) and other similar changes
and we can look again quickly to verify it makes sense.

Notifications look like the one thing that will need a client, but I'd prefer to
keep the current design doc style of a CharacteristicClient that can be set
per-characteristic and receive notifications just for that object. (versus
having the GattClient receive notifications)

I'm curious ortuno's thoughts.

[ortuno, 2016/10/17 05:03:31]

If we are going to go ahead with this pattern I have a couple of concerns:

 1. We are adding logic since now Device has to check if the event corresponds
to that specific Bluetooth Device.
 2. We will be adding a ton of observers. Say we have 100 devices each of those
device will receive an event and will have to check if the event corresponds
that device.
 3. Most importantly though, I don't think this event should go in Device. Say
you are interested in all Advertisement Packets. Where would you get them? Would
you have to get a Device interface for each device so you can get all
advertisements?

I'm not opposed to the event but I think it should live in AdapterClient.
(I remember a while ago, I was really excited about Mojo and how I was going to
be able to have an interface for services and another one for characteristics,
etc. But then scheib was like: "But why?"[1] so I started going in the other
direction of just having one interface haha)

My suggestion was more regarding how we perform read/writes rather than where
the events arrive. The asynchronous nature of Mojo means that we will need two
callbacks to get a characteristic or a service which I think makes the code much
harder to follow[2]. Anyway I think we should go for the interface that leads to
the easiest implementation on top of device/bluetooth.

I would personally go for the following due to the ease of implementation and
lack of extra interfaces:

interface Device { // Note that everything is in the device.
  GetServices() => (array<ServiceInfo> services);
  GetCharacteristics(string service_id) => (array<CharacteristicInfo>
characteristics);
  GetDescriptors(string service_id, string characteristic_id) =>
(array<DescriptorInfo> descriptors);

  ReadValueForCharacteristic(string service_id, string characteristic_id) =>
(array<uint8_t> value);
  WriteValueForCharacteristic(string service_id, string characteristic_id,
array<uint8_t>) => ();

  StartNotificationsForCharacteristic(string service_id, string
characteristic_id);
  SetNotificationsClient(string service_id, string characteristic_id, client);
  
}


// Device.cpp

ReadValueForCharacteristic(string service_id, string characteristic_id,
callback) {
  auto* device = adapter_->GetDevice(device_address_);
  if (device == nullptr)
    callback.Run(/* some error */);
  
  auto* service = device->GetService(service_id);
  if (service == nullptr)
    callback.Run(/* some error */);
  
  auto* characteristic = service->GetCharacteristic(characteristic_id);
  if (characteristic == nullptr)
    callback.Run(/* some error */);

  characteristic->ReadValue(base::Bind(..., callback));
}

I don't think we can get around the routing since we are not allowed to keep
pointers to any objects :(

[1] http://i.imgur.com/Br00TCn.gif?noredirect

[2]
Interface per object:

interface Service {
  GetCharacteristic(CharacteristicId id);
  GetCharacteristics() => (array<CharacteristicInfo> characteristics);
}

How Web Bluetooth would use it:

// BluetoothRemoteGATTService.cpp
GetCharacteristic(String uuid) {
  service_->GetCharacteristics(base::Bind(&OnGetCharacteristics, this, uuid,
...));
});

OnGetCharacteristics(uuid, characteristics, promise) {
  for (characteristic : characteristics) {
    if (characteristic->uuid == uuid) {
      service_->GetCharacteristic(base::Bind(&OnGetCharacteristic, this, uuid,
characteristic->id, promise));
      return;
    }
  }
}

OnGetCharacteristic(uuid, id, characteristic, promise) {
  promise.resolve(new BluetoothRemoteGATTCharacteristic(id, uuid));
}

As opposed to:

interface GattRemoteServer {
  GetServices() => (array<ServiceInfo> services);
  GetCharacteristicsForService(ServiceId id) => (array<CharacteristicInfo>
characteristics);
}

How Web Bluetooth would use it:

// BluetoothRemoteGATTService.cpp
GetCharacteristic(String uuid) {
  gatt_->GetCharacteristicsForService(base::Bind(&OnGetCharacteristics, this,
uuid, ...));
}

OnGetCharacteristics(uuid, promise, characteristics) {
  for (characteristic : characteristics) {
    if (characteristic->uuid == uuid) {
      promise.resolve(new BluetoothRemoteGATTCharacteristic(characteristic));
    }
  }
  promise.reject("Not Found Error");
}

Regarding notifications, I think we can still have a client just for
notifications:

interface RemoteGATTServer {
  SetCharacteristicNotificationsClient(CharacteristicId id,
CharacteristicNotificationClient client)
}

interface CharacteristicNotificationClient {
  NotificationReceived(type, value, ...);
}

or something like that.

[mbrunson, 2016/10/17 21:33:38]

I've moved PacketReceived to Adapter. 

Given your suggestion, where would the GATT connection logic go?  Instead of
having the functions in Device, putting them in RemoteGattServer and having the
Device call "ConnectGatt" that returns a RemoteGattServer seems like a good idea
(I've noted these changes in the go/fizzbluetoothinternals design doc). This
leaves room for other Bluetooth device logic (pairing, other types of
connections, etc.) and keeps the GATT connection's lifetime tied to the lifetime
of the RemoteGattServer instance. Thoughts?

 };