Weave Packet Generator

components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.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_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.
+// Internally tracks the packet number.

[Kyle Horimoto, 2016/06/06 19:30:55]

nit: This is an implementation detail of this class, so it doesn't belong in the
class description.

[Kyle Horimoto, 2016/06/08 00:06:29]

Ping.

[jingxuy, 2016/06/08 21:30:36]

Done.

+class BluetoothLowEnergyWeavePacketGenerator {
+ public:
+  class Factory {
+   public:
+    Factory();
+
+    static std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator>
+    NewInstance();

[Kyle Horimoto, 2016/06/06 19:30:55]

nit: +4 spaces.

[Kyle Horimoto, 2016/06/08 00:06:29]

Ping.

[jingxuy, 2016/06/08 21:30:36]

I had 4 spaces but git cl format deleted them

+
+    // Exposed for testing.
+    static void SetInstanceForTesting(Factory* factory);
+
+   protected:
+    // Exposed for testing.
+    BluetoothLowEnergyWeavePacketGenerator* BuildInstance();
+
+   private:
+    static Factory* factory_instance_;

[Kyle Horimoto, 2016/06/06 19:30:55]

You need to statically initialize this to nullptr.

[jingxuy, 2016/06/08 21:30:36]

Done.

+  };
+
+  enum ReasonForClose {

[Kyle Horimoto, 2016/06/06 19:30:55]

nit: Add a description over this enum which lets readers know how it will be
used.

[Kyle Horimoto, 2016/06/08 00:06:29]

Ping.

[jingxuy, 2016/06/08 21:30:36]

Done.

+    CLOSE_WITHOUT_ERROR = 0x00,
+    UNKNOWN_ERROR = 0x01,
+    NO_COMMON_VERSION_SUPPORTED = 0x02,
+    RECEIVED_PACKET_OUT_OF_SEQUENCE = 0x03,
+    APPLICATION_ERROR = 0x80

[Kyle Horimoto, 2016/06/06 19:30:55]

nit: Why does this jump from 0x03 to 0x80?

[Kyle Horimoto, 2016/06/08 00:06:29]

Nevermind - this is part of the spec. Disregard.

[jingxuy, 2016/06/08 21:30:36]

Done.

+  };
+
+  typedef std::vector<uint8_t> Packet;

[Kyle Horimoto, 2016/06/06 19:30:55]

Hmm, are we sure this works correctly regarding endianness? Will we have to use
htonl(), etc? sacomoto/msarda/tengs - have you dealt with this before?

[jingxuy, 2016/06/08 21:30:36]

You can check out the example from the legacy implementation. I'm pretty sure
that the endianness only matters because of the protocol, like the protocol is
saying that lower bytes of short comes before upper bytes. But bytes are bytes.
They will sent and received in the same order.

[sacomoto, 2016/06/13 16:05:18]

Jing is correct. The endianess only matter for multibyte types that are sent
inside the control packets. The actual bytes sent are going to be sent and
received in the same order. 

+
+  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);
+
+ protected:
+  BluetoothLowEnergyWeavePacketGenerator();
+
+ private:
+  static const int kWeaveVersion = 1;

[Kyle Horimoto, 2016/06/06 19:30:55]

This and kControlPacketSize should be moved to the top of the .cc file in an
anonymous namespace.

[Kyle Horimoto, 2016/06/08 00:06:29]

Ping. Same with these other constants.

[jingxuy, 2016/06/08 22:43:15]

This is done but I think they need to be in the class (a different class)
because eventually it has to be a shared location for the client and the server.

+  static const int kControlPacketSize = 20;
+
+  Packet* CreateControlPacket();
+
+  // For error checking.
+  bool IsBit(uint8_t val);
+
+  void SetIntField(Packet* packet, uint32_t index, uint16_t val);
+  void SetPacketTypeBit(Packet* packet, uint32_t index, uint8_t val);
+  void SetControlCmd(Packet* packet, uint32_t index, uint8_t val);
+  void SetPacketCounter(Packet* packet, uint32_t index);
+  void SetDataFirstBit(Packet* packet, uint32_t index, uint8_t val);
+  void SetDataLastBit(Packet* packet, uint32_t index, uint8_t val);
+
+  // The default data packet length is 20 unless setDataPacketLength() is called
+  // and specified otherwise.
+  uint32_t packet_size_;

[Kyle Horimoto, 2016/06/06 19:30:55]

This is only for data packets, right? If so, please change its name accordingly.

[Kyle Horimoto, 2016/06/08 00:06:29]

Ping.

[jingxuy, 2016/06/08 21:30:36]

They are already addressed.

[Kyle Horimoto, 2016/06/09 22:55:43]

I addressed this in the .cc, but please make this data packets and change the
name.

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

Since we have this discussion elsewhere, will close this comment

+
+  // 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_