bluetooth: Add basic scanning to chrome://bluetooth-internals.

device/bluetooth/public/interfaces/adapter.mojom

 // 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.
 
 module bluetooth.mojom;
 
 import "device/bluetooth/public/interfaces/device.mojom";
 
 // Possible errors sent as a response by Adapter.ConnectToDevice on a Device
 // connection request.
@@ skipping 21 matching lines @@
 struct AdapterInfo {
   string address;
   string name;
   bool initialized;
   bool present;
   bool powered;
   bool discoverable;
   bool discovering;
 };
 
+interface DiscoverySession {
+  // Returns true if the session is active, false otherwise. If false, the
+  // adapter might still be discovering as there might still be other active
+  // sessions; this just means that this instance no longer has a say in
+  // whether or not discovery should continue. In this case, a new
+  // DiscoverySession should be started to make sure that device discovery
+  // continues.
+  IsActive() => (bool active);
+
+  // Requests this discovery session instance to stop. If this instance is
+  // active, the session will stop. After a successful invocation, the
+  // adapter may or may not stop device discovery, depending on whether or not
+  // other active discovery sessions are present. Users are highly encouraged
+  // to call this method to end a discovery session, instead of relying on
+  // disconnecting the message pipe, so that they can respond to the result.
+  // Returns true on success. Returns false if this session is inactive or an
+  // error occurs while stopping the session.
+  Stop() => (bool success);
+};
+
 interface Adapter {
-  // Gets basic information about the adapter.
-  GetInfo() => (AdapterInfo info);
-
   // Creates a GATT connection to the device with |address| and returns a
   // Device if the connection was succesful. The GATT connection is tied to the
   // the lifetime of the Device message pipe.
   ConnectToDevice(string address) => (ConnectResult result, Device? device);
 
   // Retrieves the list of the devices known by the adapter including Connected
   // Devices, GATT Connected Devices, Paired Devices and Devices discovered
   // during a classic or low-energy scan.
   GetDevices() => (array<DeviceInfo> devices);
 
+  // Gets basic information about the adapter.
+  GetInfo() => (AdapterInfo info);
+
   // Sets the client that listens for the adapter's events.
   SetClient(AdapterClient client);

[dcheng, 2017/01/05 22:28:23]

Btw, is there ever a time when we'd want to create an Adapter interface with no
AdapterClient? It seems like these should be 1:1 right?

[mbrunson, 2017/01/06 01:18:56]

The bluetooth-internals is currently the only user of this service. In the
future, other clients may not care to receive events from AdapterClient, so I
thought it would be better to keep it optional in this case.

This mimics the current observer model of the BluetoothAdapter and its observer
interface where subscribing to these events are optional. Keeping it similar
will minimize the headache of clients who decide to switch to the Mojo-based
service.

[dcheng, 2017/01/06 02:45:50]

Would we create multiple BluetoothAdapter mojo impls? Or would all the things
connect to the same BluetoothAdapter mojo impl?

I'm just trying to understand the long-term plan here: in general, the
SetClient() pattern is one I try to discourage in IPC reviews if there is a
strong 1:1 relationship, because it requires a lot more checking (all the newly
added code needs to check for a non-null client), and it can make the control
flow harder to follow.

[mbrunson, 2017/01/06 03:09:41]

Every Adapter that is requested is a new strongly bound mojo impl[1]. They will
share the same underlying device::BluetoothAdapter instance though since the
device/bluetooth system uses the default bluetooth adapter hardware. So, there
could potentially be multiple impls with different configurations or no client
at all depending on the needs of the user.

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/adapter_factory.cc?rcl=...

[dcheng, 2017/01/06 22:45:31]

Hmm. I'm not really sure what the best long-term approach here. Let's keep an
eye on this, and if it looks like it's going to be mostly used in a 1:1 context,
can we revisit the decision to have SetClient()? It makes all the methods more
complicated due to the null checking.

[mbrunson, 2017/01/06 23:14:37]

SGTM

+
+  // Requests the adapter to start a new discovery session. Returns null if
+  // session not created successfully.
+  StartDiscoverySession() => (DiscoverySession? session);
 };
 
 interface AdapterClient {
+  // Called when the discovering state of the adapter changes.
+  DiscoveringChanged(bool discovering);
+
   // Called the first time a device is discovered.
   DeviceAdded(DeviceInfo device);
 
-  // Called after the device hasn't been seen for 3 minutes.
-  DeviceRemoved(DeviceInfo device);
-
   // Called when one of the following properties of a device changes:
   //   Address, appearance, Bluetooth class, Inquiry RSSI, Inquiry TX Power,
   //   Service UUIDs, Connectionable state, Connection state, Pairing state,
   //   Trustable state.

[dcheng, 2017/01/05 22:28:23]

What properties of DeviceInfo *can't* change? IOW, how does the AdapterClient
know which device's info changed?

[mbrunson, 2017/01/06 01:18:56]

Almost all properties of a device could change except for its id. The
recommended way to identify a device is through its address[1] and this is how
the bluetooth-internals page differentiates between devices and updates the
correct DeviceInfo. 

At the moment, I have not implemented the feature in the Mojo service to track
explicit changes in the address to make sure this identifier is consistent
although this functionality is available[2]. Devices that change address are
currently considered new devices on the bluetooth-internals page.

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?rcl=...
[2]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?rcl...

[dcheng, 2017/01/06 02:45:50]

It's not necessary in this CL, but I would prefer if Device itself was
encapsulated as a Mojo interface. That would make the identity invariants easier
to understand, because things like device changed notifications would be
broadcast to the Device interface representing that device. Is this something we
can consider doing in a followup?

[mbrunson, 2017/01/06 03:09:41]

Hmm. My main concern is that Device and DeviceInfo are related but used in
different situations. A Device interface impl is only created when a user
connects to a Bluetooth device and is strongly bond to the message pipe and its
Bluetooth connection. DeviceInfo is available at all times from Adapter to
describe the bluetooth devices that the bluetooth adapter can see. These values
may be updated without a connection and are added, removed, or changed over time
whether a Device impl exists or not.

[dcheng, 2017/01/06 22:45:31]

This isn't a blocker for this CL, but one of the nice things about Mojo is that
things can be interface-associated and have an implicit identity. This is what
lets us (eventually) kill IPC routing IDs and perform many other
simplifications. It also makes code audits easier, since identity is associated
with the pipe, so you don't need to validate the name of the thing is valid,
etc.

It sounds like we'd need a separate interface for tracking device status (to
keep it distinct from the connecting to device use case), but I think it might
be worthwhile to keep this in mind, especially as it becomes clearer how this
interface will be used.

[mbrunson, 2017/01/06 23:14:37]

So, there would be some kind of message in the Adapter interface that vends a
"DeviceListener" impl that only receive messages for a specific device? I'd have
to figure out the relationship between Device and DeviceListener, but I'll keep
this in mind. I can see this working.

   // 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(DeviceInfo device);
+
+  // Called after the device hasn't been seen for 3 minutes.
+  DeviceRemoved(DeviceInfo device);
+
 };
 
 interface AdapterFactory {
   // Gets an Adapter interface. Returns null if Bluetooth is not supported.
   GetAdapter() => (Adapter? adapter);
 };