Weave Packet Generator

components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.cc

+// 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.
+
+#include "components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator.h"
+
+#include <string.h>
+
+#include <algorithm>
+
+#include "base/logging.h"
+
+namespace proximity_auth {
+
+BluetoothLowEnergyWeavePacketGenerator::Factory*
+    BluetoothLowEnergyWeavePacketGenerator::Factory::factory_instance_;
+
+BluetoothLowEnergyWeavePacketGenerator::Factory::Factory() {
+  factory_instance_ = this;

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

Remove this. See comment below.

[jingxuy, 2016/06/07 01:19:06]

Done.

+}
+
+std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator>
+BluetoothLowEnergyWeavePacketGenerator::Factory::NewInstance() {

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

This should be structured a bit differently. Do something like this instead
(with proper syntax):

if (factory_instance_ == nullptr) {
  factory_instance_ = new Factory();
}

return std::unique_ptr<>(factory_instance_->BuildInstance());

[jingxuy, 2016/06/07 01:19:07]

Done.

+  return std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator>(
+      factory_instance_->BuildInstance());
+}
+
+void BluetoothLowEnergyWeavePacketGenerator::Factory::SetInstanceForTesting(
+    Factory* factory) {
+  factory_instance_ = factory;
+}
+
+BluetoothLowEnergyWeavePacketGenerator*
+BluetoothLowEnergyWeavePacketGenerator::Factory::BuildInstance() {
+  return new BluetoothLowEnergyWeavePacketGenerator();
+}
+
+BluetoothLowEnergyWeavePacketGenerator::
+    BluetoothLowEnergyWeavePacketGenerator() {
+  packet_size_ = 20;

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

Please use a constant instead of "20" directly.

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

Please initialize these via an initializer list.

[jingxuy, 2016/06/07 01:19:07]

Done.

+  packet_number_ = 0;
+}
+
+std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator::Packet>
+BluetoothLowEnergyWeavePacketGenerator::CreateConnectionRequest() {
+  Packet* packet = CreateControlPacket();
+
+  // Command is a connection request.
+  SetControlCmd(packet, 0, 0);

[sacomoto, 2016/06/06 15:25:20]

Please create an enum for the command values:

enum ControlCommand {
  CONNECTION_REQUEST = 0,
  ...
}

[jingxuy, 2016/06/07 01:19:06]

Done.

+  // Min supported version is 1.

[sacomoto, 2016/06/06 15:25:20]

Please use a constant for the min supported version.

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

Just use the kWeaveVersion constant you already have.

[jingxuy, 2016/06/07 01:19:07]

Done.

+  SetIntField(packet, 1, 1);
+  // Max supported version is 1.

[sacomoto, 2016/06/06 15:25:20]

The same for the max supported version.

[jingxuy, 2016/06/07 01:19:06]

Done.

+  SetIntField(packet, 3, 1);
+  // Max packet size is 0 which means defer the decision to server.

[sacomoto, 2016/06/06 15:25:20]

The same for max packet size.

[jingxuy, 2016/06/07 01:19:06]

Done.

+  SetIntField(packet, 5, 0);
+
+  return std::unique_ptr<Packet>(packet);
+}
+
+std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator::Packet>
+BluetoothLowEnergyWeavePacketGenerator::CreateConnectionResponse() {
+  Packet* packet = CreateControlPacket();

[sacomoto, 2016/06/06 15:25:21]

See the comments above.

[jingxuy, 2016/06/07 01:19:07]

Done.

+
+  // Command is a connection response.
+  SetControlCmd(packet, 0, 1);
+  // Selected protocol version is 1.
+  SetIntField(packet, 1, 1);
+  // Selected packet size is packet_size_.
+  SetIntField(packet, 3, packet_size_);

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

I'm confused by your use of packet_size_. It's used by data packets and
connection responses? Can you please clarify this?

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

Ping.

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

Do you want me to put a comment above packet_size_?

It is kind of confusing since this class is both used by the client and server.
But the server should do some ATT_MTU detection and then call SetDataPacketSize
to set packet_size_ according to the max packet size it observes. Then the
server will send the Response message with the packet_size_ as the maximum
packet size of the connection, which would be used to chop up data packets from
either the server or the client's perspective

On the client side, the receiver should parse out the packet_size_ from the
server response and use it as maximum packet size to chop up and send data

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

How about this: just don't use packet_size_ for the connection response and use
it only for data packets. Then, for this function, have this function take an
optional parameter which defaults to kDefaultDataPacketSize.

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

We can talk more about this when you get back. I don't really understand your
comment.

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

Let me know when you are ready to discuss this.

+
+  return std::unique_ptr<Packet>(packet);
+}
+
+std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator::Packet>
+BluetoothLowEnergyWeavePacketGenerator::CreateConnectionClose(
+    ReasonForClose reason_for_close) {

[sacomoto, 2016/06/06 15:25:20]

See the comments above.

[jingxuy, 2016/06/07 01:19:07]

Done.

+  Packet* packet = CreateControlPacket();
+
+  // Command is a connection close.
+  SetControlCmd(packet, 0, 2);
+  // Setting reason for close.
+  SetIntField(packet, 1, reason_for_close);
+
+  return std::unique_ptr<Packet>(packet);
+}
+
+BluetoothLowEnergyWeavePacketGenerator::Packet*
+BluetoothLowEnergyWeavePacketGenerator::CreateControlPacket() {
+  Packet* packet = new Packet(kControlPacketSize, 0);
+
+  // Packet is a control packet.
+  SetPacketTypeBit(packet, 0, 1);

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

Use a constant for the value to pass (e.g., kControlPacketTypeValue). Same for
data packets.

[jingxuy, 2016/06/07 01:19:06]

Done.

+  SetPacketCounter(packet, 0);
+
+  return packet;
+}
+
+bool BluetoothLowEnergyWeavePacketGenerator::IsBit(uint8_t val) {

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

Why is this method used? I don't really see the value of it.

[jingxuy, 2016/06/07 01:19:07]

Done.

+  return val == 0 || val == 1;
+}
+
+void BluetoothLowEnergyWeavePacketGenerator::SetDataPacketSize(uint32_t size) {
+  packet_size_ = size;
+}
+
+std::unique_ptr<std::vector<BluetoothLowEnergyWeavePacketGenerator::Packet>>
+BluetoothLowEnergyWeavePacketGenerator::EncodeDataMessage(std::string message) {
+  uint32_t payload_size = packet_size_ - 1;

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

Please comment about why this is the case.

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

Ping.

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

Done.

+  // (message.length() + 1) + (payload_size - 1) / (payload_size).

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

Can you please explain this calculation?

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

Ping.

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

Done.

+  uint32_t num_packets = (message.length() + payload_size) / payload_size;
+  std::vector<Packet>* weave_message = new std::vector<Packet>(num_packets);
+
+  uint32_t last_byte = message.length() + 1;
+  const char* byte_message = message.c_str();
+
+  for (uint32_t i = 0; i < num_packets; ++i) {
+    Packet* packet = &weave_message->at(i);

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

Instead of at(), just use the bracket notation shorthand: []. Same everywhere
else at() is used - the bracket notation is very common in C++.

[jingxuy, 2016/06/07 01:19:05]

[] does not work with unique_ptr, hence the at()

+    uint32_t begin = payload_size * i;
+    uint32_t end = std::min(begin + payload_size, last_byte);
+
+    packet->push_back(0);
+    // This is a data packet.
+    SetPacketTypeBit(packet, 0, 0);
+    SetPacketCounter(packet, 0);
+
+    for (uint32_t i = begin; i < end; ++i) {
+      packet->push_back(byte_message[i]);
+    }
+  }
+
+  // Since even empty string has \0, weave_message is guaranteed to be nonempty.

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

weave_message isn't necessarily guaranteed to be nonempty because of that. It's
only guaranteed to be nonempty in the case that num_packets > 0. Please DCHECK()
that this is true at the top of the function.

[jingxuy, 2016/06/07 01:19:05]

the string that is passed in at least has to be an empty string which consist of
\0 character. Hence weave_message is guaranteed to be non-empty

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

That's not true. An empty string has a length of 0. '\0' is not included in a
string length computation.

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

length used in this case is the number of bytes in the string which is
string.length + 1

+  SetDataFirstBit(&weave_message->at(0), 0, 1);
+  SetDataLastBit(&weave_message->at(num_packets - 1), 0, 1);
+
+  return std::unique_ptr<std::vector<Packet>>(weave_message);
+}
+
+void BluetoothLowEnergyWeavePacketGenerator::SetIntField(Packet* packet,
+                                                         uint32_t index,
+                                                         uint16_t val) {
+  DCHECK(packet);
+  DCHECK_LT(index, packet->size());
+  DCHECK_LT(index + 1, packet->size());
+

[sacomoto, 2016/06/06 15:25:20]

In the uWeave protocol the integers sent in the control packet are encoded in
network byte order (most significative byte first, big endian).

So, before writing |val| into |packet| you should convert it to BigEndian if
necessary.

[jingxuy, 2016/06/07 01:19:05]

Done.

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

You can't assume the endianness of this machine. Please use htons().

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

what do you mean by endianness of this machine? I am fairly certain that in C
the upper byte is the upper byte independent of the machine? I am using a direct
uint16_t value not a byte stream.

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

You are both right. The byte ordering is machine dependent (although almost all
machines are little endian), and this affects how multibyte types are stored in
memory. But the endianess doesn't matter once the values are loaded in the CPU
registers (i.e. one shift to the right is equivalent to a division by 2, no
matter the endianess): 

http://stackoverflow.com/questions/7184789/does-bit-shift-depend-on-endianness

So, yes, Jing solution is correct in this case.

However, if you don't rely on bit shifts, and uses the value directly from
memory (like the code below). Then, you should take the endianess into account.

uint8_t *ptr;
ptr = (uint8_t *)&val;
packet->at(index) = ptr[0];
packet->at(index + 1) = ptr[1];

The solution above is incorrect for a little endian machine. You have to
explicitly convert the host endianess to big endian. 

uint8_t *ptr;
uint8_t big_endian_val = htons(val);
ptr = (uint8_t *)&big_endian_val;
packet->at(index) = ptr[0];
packet->at(index + 1) = ptr[1];

I would probably use this solution as it avoids bit shifts and does the
conversion explicitly, but it's a personal preference. Feel free to keep your
solution, Jing.

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

Acknowledged.

+  uint8_t upper = val >> 8 & 0xFF;
+  uint8_t lower = val & 0xFF;
+
+  packet->at(index) = upper;
+  packet->at(index + 1) = lower;
+}
+
+void BluetoothLowEnergyWeavePacketGenerator::SetPacketTypeBit(Packet* packet,

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

For all these Send*() methods, please give a description of what is happening.
For example, describe that the packet type bit is the first bit of the first
byte of the packet and add comments in your implementation that make it clear
what is going on. It should be easy to reason about your code without having to
do complex bit manipulation in your head.

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

Ping.

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

Done.

+                                                              uint32_t index,

[sacomoto, 2016/06/06 15:25:21]

Please remove the |index| parameter, as it's always 0. The packet type bit is
always the first bit of the first byte.

[jingxuy, 2016/06/07 01:19:07]

Done.

+                                                              uint8_t val) {

[sacomoto, 2016/06/06 15:25:21]

Please use |bool| instead of |uint8_t|.

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

+1

[jingxuy, 2016/06/07 01:19:07]

Done.

+  DCHECK(packet);
+  DCHECK_LT(index, packet->size());
+  DCHECK(IsBit(val));

[sacomoto, 2016/06/06 15:25:20]

Please remove the last two DCHECK's, and the |IsBit()| helper method.

[jingxuy, 2016/06/07 01:19:07]

Done.

+  packet->at(index) = (packet->at(index) & 0x7F) | (val << 7);
+}
+
+void BluetoothLowEnergyWeavePacketGenerator::SetControlCmd(Packet* packet,
+                                                           uint32_t index,

[sacomoto, 2016/06/06 15:25:21]

Please remove the |index| parameter, as it's always 0.

[jingxuy, 2016/06/07 01:19:07]

Done.

+                                                           uint8_t val) {

[sacomoto, 2016/06/06 15:25:20]

Please pass the ControlCommand enum (see a few comments above). 

[jingxuy, 2016/06/07 01:19:07]

Done.

+  DCHECK(packet);
+  DCHECK_LT(index, packet->size());
+  // Currently only commands supported are 0, 1, and 2
+  DCHECK_LT(val, 3);
+  packet->at(index) = (packet->at(index) & 0xF0) | val;
+}
+
+void BluetoothLowEnergyWeavePacketGenerator::SetPacketCounter(Packet* packet,
+                                                              uint32_t index) {

[sacomoto, 2016/06/06 15:25:21]

Please remove the |index| parameter, as it's always 0.

[jingxuy, 2016/06/07 01:19:07]

Done.

+  DCHECK(packet);
+  DCHECK_LT(index, packet->size());
+  uint8_t counter = packet_number_ % 8;

[sacomoto, 2016/06/06 15:25:20]

Please add a kMaxPacketCounter constant, and use it here.

[jingxuy, 2016/06/07 01:19:07]

Done.

+  packet->at(index) = (packet->at(index) & 0x8F) | (counter << 4);
+  packet_number_++;
+}
+
+void BluetoothLowEnergyWeavePacketGenerator::SetDataFirstBit(Packet* packet,
+                                                             uint32_t index,

[sacomoto, 2016/06/06 15:25:20]

Please remove the |index| parameter, as it's always 0.

[jingxuy, 2016/06/07 01:19:05]

Done.

+                                                             uint8_t val) {

[sacomoto, 2016/06/06 15:25:20]

Please remove the |val| parameter, as this is always 1.

[jingxuy, 2016/06/07 01:19:07]

Done.

+  DCHECK(packet);
+  DCHECK_LT(index, packet->size());
+  DCHECK(IsBit(val));
+  packet->at(index) = (packet->at(index) & 0xF7) | (val << 3);
+}
+
+void BluetoothLowEnergyWeavePacketGenerator::SetDataLastBit(Packet* packet,
+                                                            uint32_t index,
+                                                            uint8_t val) {

[sacomoto, 2016/06/06 15:25:20]

See the comments above.

[jingxuy, 2016/06/07 01:19:07]

Done.

+  DCHECK(packet);
+  DCHECK_LT(index, packet->size());
+  DCHECK(IsBit(val));
+  packet->at(index) = (packet->at(index) & 0xFB) | (val << 2);
+}
+
+}  // namespace proximity_auth