Weave Packet Generator

components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h

Index: components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h
diff --git a/components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h b/components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h
new file mode 100644
index 0000000000000000000000000000000000000000..4dc417f5358c86dde938e3b9b112673ec6d4139d
--- /dev/null
+++ b/components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h
@@ -0,0 +1,92 @@
+// 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_GENERATOR_H_
+#define COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_PACKET_GENERATOR_H_
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <memory>
+#include <string>
+#include <vector>
+
+namespace proximity_auth {
+// Generates the messages sent using the uWeave protocol.
+class BluetoothLowEnergyWeavePacketGenerator {
+ public:
+  class Factory {
+   public:
+    static std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator>
+    NewInstance();
+
+    // Exposed for testing.
+    static void SetInstanceForTesting(Factory* factory);
+
+   protected:
+    // Exposed for testing.
+    BluetoothLowEnergyWeavePacketGenerator* BuildInstance();

[Tim Song, 2016/06/09 22:45:35]

This should be virtual if you are overriding it in the subclass. You should also
return an std::unique_ptr rather than a raw pointer, as it isn't less efficient
and it clarifies ownership.

[jingxuy, 2016/06/11 00:49:32]

this function is protected, the function that calls it which is NewInstance
returns unique_ptr

[Kyle Horimoto, 2016/06/13 17:55:53]

You can just have NewInstance() return the unique_ptr instead of creating one
too.

[jingxuy, 2016/06/17 00:56:45]

Done.

+
+   private:
+    static Factory* factory_instance_;
+  };
+
+  // Determine whether a packet is a control packet or a data packet.
+  enum PacketType { DATA = 0x00, CONTROL = 0x01 };
+
+  // Identify the action intended by the control packet.
+  enum ControlCommand {
+    CONNECTION_REQUEST = 0x00,
+    CONNECTION_RESPONSE = 0x01,
+    CONNECTION_CLOSE = 0x02
+  };
+
+  // Sent with the ConnectionClose control packet.
+  // Identify why the client/server wished to close the connection.
+  enum ReasonForClose {
+    CLOSE_WITHOUT_ERROR = 0x00,
+    UNKNOWN_ERROR = 0x01,
+    NO_COMMON_VERSION_SUPPORTED = 0x02,
+    RECEIVED_PACKET_OUT_OF_SEQUENCE = 0x03,
+    APPLICATION_ERROR = 0x80
+  };
+
+  typedef std::vector<uint8_t> Packet;
+
+  std::unique_ptr<Packet> CreateConnectionRequest();
+  std::unique_ptr<Packet> CreateConnectionResponse();
+  std::unique_ptr<Packet> CreateConnectionClose(
+      ReasonForClose reason_for_close);
+
+  void SetDataPacketSize(uint32_t size);
+
+  std::unique_ptr<std::vector<Packet>> EncodeDataMessage(std::string message);

[Tim Song, 2016/06/09 22:45:35]

Just return a std::vector<Packet>. The compiler will optimize the returned value
to elide the copy operation:
http://stackoverflow.com/questions/3134831/in-c-is-it-still-bad-practice-to-r...

[jingxuy, 2016/06/11 00:49:32]

The reason that it's a unique_ptr right now is because Kyle thinks it's better
to clarify ownership

[Kyle Horimoto, 2016/06/13 17:55:53]

Tim is saying to return a full vector (by value) instead of a pointer (by
reference). That idea SGTM.

[jingxuy, 2016/06/17 00:56:45]

Hmmm, maybe I was unclear when I asked the question but I was asking why we
needed the unique_ptr wrapper around just std::vector and I thought you wanted a
conceptual ownership thing. Now it makes more sense.

Done.

+
+ protected:
+  BluetoothLowEnergyWeavePacketGenerator();
+
+ private:
+  Packet* CreateControlPacket();

[Tim Song, 2016/06/09 22:45:35]

Same here. I would return a unique_ptr even in private functions, as it's free
and clarifies ownership.

[jingxuy, 2016/06/11 00:49:32]

this is a private helper function, I think it's fine if it does not have very
interfacing return type

[Kyle Horimoto, 2016/06/13 17:55:53]

It's generally a best practice to use this everywhere where
dynamically-allocated memory is returned so that ownership is always clear, even
if it is for a private function.

[jingxuy, 2016/06/17 00:56:45]

Done.

+
+  // For error checking.
+  bool IsBit(uint8_t val);
+
+  void SetShortField(Packet* packet, uint32_t index, uint16_t val);
+  void SetPacketTypeBit(Packet* packet, PacketType val);
+  void SetControlCommand(Packet* packet, ControlCommand val);
+  void SetPacketCounter(Packet* packet);
+  void SetDataFirstBit(Packet* packet);
+  void SetDataLastBit(Packet* packet);
+
+  // The default data packet length is 20 unless setDataPacketLength() is called

[Tim Song, 2016/06/09 22:45:35]

nit: set should be capitalized.

[jingxuy, 2016/06/11 00:49:32]

Done.

+  // and specified otherwise.
+  uint32_t packet_size_;
+
+  // Counter for the number of packets sent, starting at 0.
+  uint32_t packet_number_;
+};
+
+}  // namespace proximity_auth
+
+#endif  // COMPONENTS_PROXIMITY_AUTH_BLE_BLUETOOTH_LOW_ENERGY_WEAVE_PACKET_GENERATOR_H_