Weave Packet Receiver

components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h

+// 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.
+
+#ifndef COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_PACKET_RECEIVER_H_
+#define COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_PACKET_RECEIVER_H_
+
+#include <stdint.h>
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h"
+
+namespace proximity_auth {
+// Receive the messages sent with uWeave protocol.
+// TODO(jingxuy): move the enums from BluetoothLowEnergyWeavePacketGenerator to
+// a shared location
+// TODO(jingxuy): a complete state transition comment
+class BluetoothLowEnergyWeavePacketReceiver {
+ public:
+  enum ReceiverType { CLIENT, SERVER };
+
+  class Factory {
+   public:
+    static std::unique_ptr<BluetoothLowEnergyWeavePacketReceiver> NewInstance(
+        ReceiverType receiver_type);
+
+    // Exposed for testing.
+    static void SetInstanceForTesting(Factory* factory);
+
+   protected:
+    // Exposed for testing.
+    BluetoothLowEnergyWeavePacketReceiver* BuildInstance(
+        ReceiverType receiver_type);
+
+   private:
+    static Factory* factory_instance_;
+  };
+
+  enum State {
+    CONNECTING = 0x00,

[Kyle Horimoto, 2016/06/20 23:25:40]

Please add descriptions to each state which make it clear what each one means.
For example, if someone who has never seen this file before looks at this enum,
will he/she be able to understand what "waiting" means? Will he/she be able to
understand what the difference between "receiving data" and "data ready" mean?
Please make it very clear exactly what circumstances apply to each state.

[jingxuy, 2016/06/21 01:19:15]

It was a TODO that I didn't get to. Done.

+    WAITING = 0x01,
+    RECEIVING_DATA = 0x02,
+    DATA_READY = 0x03,
+    CONNECTION_CLOSED = 0x04,
+    ERROR = 0x05
+  };
+
+  ~BluetoothLowEnergyWeavePacketReceiver();
+
+  typedef std::vector<uint8_t> Packet;
+
+  // Get the receiver’s state.
+  State GetState();
+
+  // Return the packet size that the receiver parsed out of request/response.

[Kyle Horimoto, 2016/06/20 23:25:40]

Please list the conditions under which this can be called. For example, you
couldn't call this before the connection has been established.

[jingxuy, 2016/06/21 01:19:15]

This is always valid, see comment in cc file

[Kyle Horimoto, 2016/06/21 18:13:37]

Readers of this class should only need to look at the .h file to glean this
information. Please place it in the .h file.

[jingxuy, 2016/06/22 01:42:23]

I meant refer to my reply comment to you in the cc file explaining this function
is always defined.

+  uint16_t GetMaxPacketSize();
+
+  // Will crash unless receiver is in State::CONNECTION_CLOSED.
+  BluetoothLowEnergyWeavePacketGenerator::ReasonForClose GetReasonForClose();

[Kyle Horimoto, 2016/06/20 23:25:40]

Please add documentation to the GetReason{For|To}Close() functions. It is
unclear to a reader what the difference between these two is, and it is also
unclear what exactly is returned by these.

For example, you could say something like:

Returns the reason that the connection was closed by the remote device. The
reason is provided as part of a "connection close" control packet received via
ReceivePacket(). This function should only be called when the state is
CONNECTION_CLOSED, which occurs after the "connection close" packet is received.
If this function is called when the state is not CONNECTION_CLOSED, an error is
thrown.

[jingxuy, 2016/06/21 01:19:15]

Done.

+
+  // Will crash unless receiver is in State::ERROR.
+  // The reason that the receiver decided to enter ERROR state.
+  BluetoothLowEnergyWeavePacketGenerator::ReasonForClose GetReasonToClose();
+
+  // Will crash unless receiver is in State::DATA_READY.
+  // NOTE: if this function is not called in DATA_READY state and the receiver
+  // transitions out of that state, the data will be gone!
+  std::string GetDataMessage();

[Kyle Horimoto, 2016/06/20 23:25:40]

Likewise, please document this function.

[jingxuy, 2016/06/22 01:42:23]

Done.

+
+  // Add a packet that's just been received over Connection to the receiver.
+  State ReceivePacket(const Packet& packet);
+
+ protected:
+  explicit BluetoothLowEnergyWeavePacketReceiver(ReceiverType receiver_type);
+
+ private:
+  void ReceiveFirstPacket(const Packet& packet);
+  void ReceiveNonFirstPacket(const Packet& packet, bool expect_first_packet);
+
+  void ReceiveConnectionRequest(const Packet& packet);
+  void ReceiveConnectionResponse(const Packet& packet);
+  void ReceiveConnectionClose(const Packet& packet);
+  void AppendData(const Packet& packet, uint32_t byte_offset);
+
+  uint16_t GetShortField(const Packet& packet, uint32_t byte_offset);
+  uint8_t GetPacketType(const Packet& packet);
+  uint8_t GetControlCommand(const Packet& packet);
+  void VerifyPacketCounter(const Packet& packet);
+  bool IsFirstDataPacket(const Packet& packet);
+  bool IsLastDataPacket(const Packet& packet);
+  bool IsLowerTwoBitsCleared(const Packet& packet);

[Kyle Horimoto, 2016/06/20 23:25:40]

nit: s/Is/Are/

[jingxuy, 2016/06/21 01:19:14]

Done.

+
+  void Error(

[Kyle Horimoto, 2016/06/20 23:25:40]

nit: s/Error/TransitionToErrorState/

[jingxuy, 2016/06/21 01:19:14]

I changed it to ThrowError

[Kyle Horimoto, 2016/06/21 02:10:26]

This function doesn't throw an error, though. Please use another name.

[Kyle Horimoto, 2016/06/21 18:13:37]

Ping.

[jingxuy, 2016/06/22 01:42:23]

Done.

+      std::string error_message,
+      BluetoothLowEnergyWeavePacketGenerator::ReasonForClose reason_to_close);
+
+  void SetMaxPacketSize(uint16_t packet_size);
+  uint16_t GetConceptualMaxPacketSize();
+
+  // Identify whether the receiver is for a client or a server.
+  ReceiverType receiver_type_;
+
+  // Max packet size of the connection.
+  // Default is 0 which means the server will determine the size by observing
+  // ATT_MTU of the client.
+  uint16_t max_packet_size_;
+
+  // Counter for the number of packets sent, starting at 0.
+  uint32_t packet_number_;

[Kyle Horimoto, 2016/06/20 23:25:40]

nit: How about next_packet_number_ instead? Otherwise, it could be confusing
whether this was the last value sent or the next value to use.

[jingxuy, 2016/06/21 01:19:15]

Done.

+
+  // Current state of the receiver.
+  // Certain functions will only return valid value if the receiver is in the
+  // appropriate state.
+  State state_;
+
+  // The reason why the connection was closed by the sender if any.
+  BluetoothLowEnergyWeavePacketGenerator::ReasonForClose reason_for_close_;
+
+  // The reason why the receiver is in an erronous state if any.
+  BluetoothLowEnergyWeavePacketGenerator::ReasonForClose reason_to_close_;
+
+  // The data message if there is one.
+  Packet data_message_;
+};
+
+}  // namespace proximity_auth
+
+#endif  // COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_PACKET_RECEIVER_H_