Implementing a BLE connection finder.

components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc

Index: components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc
diff --git a/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a7d9e9cfdc9a514cf5dcc24f21e309d0e3e14bb4
--- /dev/null
+++ b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc
@@ -0,0 +1,149 @@
+// Copyright 2015 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 "components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "device/bluetooth/bluetooth_adapter_factory.h"
+#include "device/bluetooth/bluetooth_device.h"
+#include "device/bluetooth/bluetooth_discovery_session.h"
+#include "device/bluetooth/bluetooth_uuid.h"
+
+using device::BluetoothAdapter;
+using device::BluetoothDevice;
+
+namespace {
+
+void DoNothingErrorCallback(BluetoothDevice::ConnectErrorCode error_code) {
+}
+
+}  // namespace
+
+namespace proximity_auth {
+
+BluetoothLowEnergyConnectionFinder::BluetoothLowEnergyConnectionFinder(
+    const std::string& remote_service_uuid)
+    : remote_service_uuid_(device::BluetoothUUID(remote_service_uuid)),
+      weak_ptr_factory_(this) {
+}
+
+BluetoothLowEnergyConnectionFinder::~BluetoothLowEnergyConnectionFinder() {
+  if (discovery_session_.get()) {
+    StopDiscoverySession();
+  }
+  if (adapter_) {
+    adapter_->RemoveObserver(this);
+    adapter_ = NULL;
+  }
+}
+
+void BluetoothLowEnergyConnectionFinder::Find(
+    const BluetoothDevice::GattConnectionCallback& connection_callback) {
+  if (!device::BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) {
+    VLOG(1) << "[BCF] Bluetooth is unsupported on this platform. Aborting.";
+    return;
+  }
+  DVLOG(1) << "Finding connection";
+
+  connection_callback_ = connection_callback;
+
+  device::BluetoothAdapterFactory::GetAdapter(
+      base::Bind(&BluetoothLowEnergyConnectionFinder::OnAdapterInitialized,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void BluetoothLowEnergyConnectionFinder::Find(
+    const ConnectionCallback& connection_callback) {

[msarda, 2015/04/22 15:58:13]

This method does not do what the user expects. Please add a NOTREACHED() in
here.

[sacomoto, 2015/04/24 14:04:56]

Done.

+}
+
+void BluetoothLowEnergyConnectionFinder::DeviceAdded(BluetoothAdapter* adapter,
+                                                     BluetoothDevice* device) {
+  if (device) {
+    DVLOG(1) << "New device found: " << device->GetName();
+    if (HasService(device)) {
+      CreateConnection(device);
+    }
+  }
+}
+
+void BluetoothLowEnergyConnectionFinder::OnAdapterInitialized(
+    scoped_refptr<BluetoothAdapter> adapter) {
+  DVLOG(1) << "Adapter ready";
+
+  adapter_ = adapter;
+  adapter_->AddObserver(this);
+
+  std::vector<BluetoothDevice*> devices = adapter_->GetDevices();
+  for (auto iter = devices.begin(); iter != devices.end(); iter++) {

[msarda, 2015/04/22 15:58:12]

I think we should have a single method to handle new devices. I see 2 options:
* have a method called HandleDeviceAdded that is called from here and from
DeviceAdded
* call DeviceAdded from here.

[sacomoto, 2015/04/24 14:04:56]

Done. 

I think the first option is better, because differentiating if a device was
already known or just discovery makes debugging easier.

+    if (HasService(*iter)) {
+      CreateConnection(*iter);
+    }
+  }
+
+  StartDiscoverySession();
+}
+
+void BluetoothLowEnergyConnectionFinder::OnDiscoverySessionStarted(
+    scoped_ptr<device::BluetoothDiscoverySession> discovery_session) {
+  DVLOG(1) << "Discovery session started";
+  discovery_session_ = discovery_session.Pass();
+}
+
+void BluetoothLowEnergyConnectionFinder::StartDiscoverySession() {

[msarda, 2015/04/22 15:58:12]

I expect this method should not be called when adapter_ is NULL. If so then
prefer using DCHECK(adapter_) instead of the if.

[msarda, 2015/04/22 15:58:13]

How can we be sure that this method is not called twice? I think in that case we
would call StartDiscoverySession twice and this may lead to errors.

[sacomoto, 2015/04/24 14:04:56]

Done.

[sacomoto, 2015/04/24 14:04:56]

This method does exactly what the method
"BluetoothEventRouter::StartDiscoverySession" (which implements the
startDiscovery in the bluetooth javascript API) does. That is, they also test if
there is an active discovery session before calling StartDiscoverySession, but
they also don't consider the case where we have two calls and the second call is
made while the discovery callback has not been called yet. 

Anyway, I see your point, and I think it would be better to have the Find method
in the class constructor, that way we guarantee that
"BluetoothLowEnergyConnectionFinder::StartDiscoverySession" is only called once.
But this doesn't match the ConnectionFinder interface.

+  if (!adapter_.get()) {
+    DVLOG(1) << "Adapter not initialized";
+    return;
+  }
+  if (discovery_session_.get() && discovery_session_->IsActive()) {
+    DVLOG(1) << "Discovery session already active";
+    return;
+  }
+  adapter_->StartDiscoverySession(
+      base::Bind(&BluetoothLowEnergyConnectionFinder::OnDiscoverySessionStarted,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(base::DoNothing));

[msarda, 2015/04/22 15:58:13]

We need error handling. At least let's log the errors as otherwise this will be
impossible to debug.

It may actually be wise to use VLOG(1) instead of DVLOG(1) for the BLE
connection to be able to investigate this in release.

[sacomoto, 2015/04/24 14:04:56]

Done.

+}
+
+void BluetoothLowEnergyConnectionFinder::StopDiscoverySession() {
+  DVLOG(1) << "Stopping discovery sesison";
+
+  if (!adapter_.get()) {
+    DVLOG(1) << "Adapter not initialized";
+    return;
+  }
+  if (!discovery_session_.get() || !discovery_session_->IsActive()) {
+    DVLOG(1) << "No Active discovery session";
+  }
+
+  discovery_session_->Stop(base::Bind(base::DoNothing),

[msarda, 2015/04/22 15:58:13]

I think it is wrong to reset the discovery_session_ here (it will probably be
destructed, so the callback will never be called). We should only reset it after
the callback ends.

[sacomoto, 2015/04/24 14:04:56]

Done.

+                           base::Bind(base::DoNothing));
+  discovery_session_.reset();
+}
+
+bool BluetoothLowEnergyConnectionFinder::HasService(
+    BluetoothDevice* remote_device) {
+  std::vector<device::BluetoothUUID> uuids = remote_device->GetUUIDs();
+  for (auto iter = uuids.begin(); iter != uuids.end(); iter++) {
+    if (remote_service_uuid_ == *iter) {
+      return true;
+    }
+  }
+  return false;
+}
+
+void BluetoothLowEnergyConnectionFinder::CreateConnection(
+    device::BluetoothDevice* remote_device) {
+  DVLOG(1) << "SmartLock service found ("
+           << remote_service_uuid_.canonical_value() << ")\n"
+           << "device = " << remote_device->GetAddress()
+           << ", name = " << remote_device->GetName();
+  remote_device->CreateGattConnection(connection_callback_,
+                                      base::Bind(&DoNothingErrorCallback));

[msarda, 2015/04/22 15:58:13]

Ditto: Same here about error handling.

[sacomoto, 2015/04/24 14:04:56]

Done.

+}
+
+}  // namespace proximity_auth