Bluetooth low energy connection.

components/proximity_auth/ble/bluetooth_low_energy_connection.h

Index: components/proximity_auth/ble/bluetooth_low_energy_connection.h
diff --git a/components/proximity_auth/ble/bluetooth_low_energy_connection.h b/components/proximity_auth/ble/bluetooth_low_energy_connection.h
new file mode 100644
index 0000000000000000000000000000000000000000..31bfd20c59ca87eb8e8509a422d0e73136d23f36
--- /dev/null
+++ b/components/proximity_auth/ble/bluetooth_low_energy_connection.h
@@ -0,0 +1,187 @@
+// 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.
+
+#ifndef COMPONENTS_PROXIMITY_AUTH_BLUETOOTH_LOW_ENERGY_CONNECTION_H
+#define COMPONENTS_PROXIMITY_AUTH_BLUETOOTH_LOW_ENERGY_CONNECTION_H
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "components/proximity_auth/ble/fake_wire_message.h"
+#include "components/proximity_auth/connection.h"
+#include "device/bluetooth/bluetooth_adapter.h"
+#include "device/bluetooth/bluetooth_device.h"
+#include "device/bluetooth/bluetooth_gatt_characteristic.h"
+#include "device/bluetooth/bluetooth_gatt_notify_session.h"
+#include "device/bluetooth/bluetooth_uuid.h"
+
+namespace proximity_auth {
+
+// Represents a connection with a remote device over Bluetooth low energy. The
+// connection is a persistent bidirectional channel for sending and receiving
+// wire messages. The remote device is the peripheral mode and the service
+// contains two characteristics: one to send data and another to receive it.
+class BluetoothLowEnergyConnection : public Connection,
+                                     public device::BluetoothAdapter::Observer {
+ public:
+  enum ControlSignal : uint32 {

[Tim Song, 2015/05/11 08:02:39]

nit: use an enum class instead.

[sacomoto, 2015/05/11 12:04:04]

Done.

+    kInviteToConnectSignal = 0,
+    kInvitationResponseSignal = 1,
+    kSendSignal = 2,
+    kDisconnectSignal = 3,
+  };
+
+  // Constructs a Bluetooth low energy connection to the service with
+  // |remote_service_uuid| on the |remote_device|. The GATT connection with
+  // |remote_device| must be already established and |adapter| already

[Tim Song, 2015/05/11 08:02:39]

Can you document the flow for establishing the logical connection after being
given a device connected through BLE? I noticed you have a similar comment
below. You should move that to the class comment or here.

[sacomoto, 2015/05/11 12:04:04]

Done.

+  // initialized.
+  BluetoothLowEnergyConnection(
+      const RemoteDevice& remote_device,
+      scoped_refptr<device::BluetoothAdapter> adapter,
+      device::BluetoothUUID remote_service_uuid,
+      const std::string& to_peripheral_char_uuid,
+      const std::string& from_peripheral_char_uuid,
+      scoped_ptr<device::BluetoothGattConnection> gatt_connection);
+
+  ~BluetoothLowEnergyConnection();
+
+  // proximity_auth::Connection
+  void Connect() override;
+  void Disconnect() override;
+
+ protected:
+  // proximity_auth::Connection
+  void SendMessageImpl(scoped_ptr<WireMessage> message) override;
+  scoped_ptr<WireMessage> DeserializeWireMessage(
+      bool* is_incomplete_message) override;
+
+  // device::BluetoothAdaptor::Observer
+  void DeviceRemoved(device::BluetoothAdapter* adapter,
+                     device::BluetoothDevice* device) override;
+  void GattDiscoveryCompleteForService(
+      device::BluetoothAdapter* adapter,
+      device::BluetoothGattService* service) override;
+  void GattCharacteristicAdded(
+      device::BluetoothAdapter* adapter,
+      device::BluetoothGattCharacteristic* characteristic) override;
+  void GattCharacteristicValueChanged(
+      device::BluetoothAdapter* adapter,
+      device::BluetoothGattCharacteristic* characteristic,
+      const std::vector<uint8>& value) override;
+
+ private:
+  // Sends an invite to connect signal to the peripheral if the connection
+  // is ready. That is, satisfying the conditions:
+  // (i) |to_peripheral_char_uuid_| and |from_peripheral_char_uuid_| were found;
+  // (ii) |notify_session_| was set for |from_peripheral_char_uuid_|.
+  //
+  // The asynchronous events that can casue these conditions to be
+  // satisfied are:
+  // (i) a new characteristic is discovered (HandleCharacteristicUpdate);
+  // (ii) a new notify session is started (OnNotifySessionStarted).
+  void SendInviteToConnectSignal();
+
+  // Called when there is an error writing to the remote characteristic
+  // |to_peripheral_char_uuid_|.
+  void OnWriteRemoteCharacteristicError(
+      device::BluetoothGattService::GattErrorCode error);
+
+  // Handles the discovery of a new characteristic.
+  void HandleCharacteristicUpdate(
+      device::BluetoothGattCharacteristic* characteristic);
+
+  // The connection is complete when:
+  // (i) |to_peripheral_char_uuid_| and |from_peripheral_char_uuid_| were found;
+  // (ii) |notify_session_| was set for |from_peripheral_char_uuid_|;

[Tim Song, 2015/05/11 08:02:39]

I would also add that you write an initial hello signal on the peripheral.

[sacomoto, 2015/05/11 12:04:04]

Done.

+  // (iii) kInvitationResponseSignal was received.
+  //
+  // The last condition necessariy happens after the first two.
+  // This function verifies if the connection is complete and updates
+  // the status accordingly. The only asynchronous event
+  // that can cause the connection to be completed is receiving a
+  // kInvitationResponseSignal (GattCharateristcValueChanged).
+  void CompleteConnection();
+
+  // Starts a notify session for |from_peripheral_char_uuid_|.
+  void StartNotifySession();
+
+  // Called when there is an error starting a notification session for
+  // |from_peripheral_char_uuid_| characteristic.
+  void OnNotifySessionError(device::BluetoothGattService::GattErrorCode);
+
+  // Called when a notification session is successfully started for
+  // |from_peripheral_char_uuid_| characteristic.
+  void OnNotifySessionStarted(
+      scoped_ptr<device::BluetoothGattNotifySession> notify_session);
+
+  // Stops |notify_session_|.
+  void StopNotifySession();
+
+  // Updates the value of |to_peripheral_char_id_| and
+  // |from_peripheral_char_id_|
+  // when |characteristic| was found.
+  void UpdateCharacteristicsStatus(
+      device::BluetoothGattCharacteristic* characteristic);
+
+  // Returns the Bluetooth address of the remote device.
+  const std::string& GetRemoteDeviceAddress();
+
+  // Returns the device corresponding to |remote_device_address_|.
+  device::BluetoothDevice* GetRemoteDevice();
+
+  // Returns the service corresponding to |remote_service_uuid_| in the current
+  // device.
+  device::BluetoothGattService* GetRemoteService();
+
+  // Returns the characteristic corresponding to |identifier| in the current
+  // service.
+  device::BluetoothGattCharacteristic* GetGattCharacteristic(
+      const std::string& identifier);
+
+  // Extract the signal (first 4 bytes) from a byte vector.
+  uint32 ExtractUint32(const std::vector<uint8>& bytes);
+
+  // The Bluetooth adapter over which the Bluetooth connection will be made.
+  scoped_refptr<device::BluetoothAdapter> adapter_;
+
+  // The uuid of the service it looks for to establish a GattConnection.
+  const device::BluetoothUUID remote_service_uuid_;

[Tim Song, 2015/05/11 08:02:39]

nit: Can you create a private struct that encapsulates the (UUID, ID) pair? It
would really help with readability.

Also, why is the service UUID stored as a device::BluetoothUUID but the
characteristic UUIDs as strings?

[sacomoto, 2015/05/11 12:04:04]

Done.

+
+  // The identifier of the service corresponding to |remote_service_uuid_|.
+  std::string remote_service_id_;
+
+  // Characteristic used to send data to the remote device.
+  const std::string to_peripheral_char_uuid_;
+
+  // Internal unique identifier for |to_peripheral_char_|.
+  std::string to_peripheral_char_id_;
+
+  // Characteristic used to receive data from the remote device.
+  const std::string from_peripheral_char_uuid_;
+
+  // Internal unique identifier for |from_peripheral_char_|.
+  std::string from_peripheral_char_id_;
+
+  // The GATT connection with the remote device.
+  scoped_ptr<device::BluetoothGattConnection> connection_;
+
+  // Indicates if there is a pending notification session. Used to ensure there
+  // is only one pending notify session.
+  bool notify_session_pending_;

[Tim Song, 2015/05/11 08:02:39]

Instead of having a separate bool for each state to wait for an operation to
finish, let's use an enum to track the internal state for what this object is
currently waiting on.

[sacomoto, 2015/05/11 12:04:04]

We could to this way, but I don't think it would make the code cleaner. The
problem is that we don't have a total order for all events leading to a
connection. We only have a partial order. 

The events leading to a connection are:

(i) |to_peripheral_char| discovered;
(ii) |from_peripheral_char| discovered;
(iii) notify session for |from_peripheral_char| started;
(iv) invite to connect signal sent;
(v) response received.

The last two events are ordered, (v) immediately happens after (iv). But (iv)
can be preceded by (iii) or (i). 

This happens because we don't know in which order the characteristics will be
discovered. A possible ordering is this: |from_peripheral_char| is discovered
(i), the notify session is started (iii), |to_peripheral_char| is discovered. In
this case, we have (ii) < (iii) < (i) < (iv) < (v). And we could as well have
(i) < (ii) < (iii) < (iv) < (v).    

[Tim Song, 2015/05/12 03:02:46]

Thanks, I understand the flow a lot better now. Ideally, we should refactor all
the characteristic discovery code to another class, so we will only have two
linear states to establish a ProximityAuth connection:
WAITING_FOR_CHARACTERISTICS, and WAITING_FOR_INVITATION_RESPONSE. This branching
flow can be hard to understand when you're dealing with a bunch of reentrant
functions and a bunch of booleans.

I won't ask you to do that in this CL, but it maybe something to keep in mind
when you write tests. It would be a lot easier to test the characteristic
discovery by itself.

[sacomoto, 2015/05/12 07:23:19]

Ok.

+
+  // The notify session for |from_peripheral_char|.
+  scoped_ptr<device::BluetoothGattNotifySession> notify_session_;
+
+  // Indicates if there pending response to a invite to connect signal.
+  bool connect_signal_response_pending_;
+
+  base::WeakPtrFactory<BluetoothLowEnergyConnection> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(BluetoothLowEnergyConnection);
+};
+
+}  // namespace proximity_auth
+
+#endif  // COMPONENTS_PROXIMITY_AUTH_BLUETOOTH_LOW_ENERGY_CONNECTION_H