Weave Packet Generator

components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc

Index: components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc
diff --git a/components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc b/components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6c843bbf5438c0083f0c067d0bd775d85e6b9720
--- /dev/null
+++ b/components/proximity_auth/ble/bluetooth_low_energy_weave_packet_generator_unittest.cc
@@ -0,0 +1,314 @@
+// 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 <algorithm>
+#include <string>
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+using proximity_auth::BluetoothLowEnergyWeavePacketGenerator;

[Kyle Horimoto, 2016/06/17 04:04:12]

I don't think we should do this. Generally it's only advisable to use "using" if
the namespace is really long and makes your code difficult to write on one <=80
character line.

[Kyle Horimoto, 2016/06/20 18:01:23]

Ping.

[jingxuy, 2016/06/20 21:29:32]

Because of the helper function, there will be a lot of
proximity_auth::BluetoothLowEnergyWeavePacketGenerator repeated. So I thought a
using is appropriate

[Kyle Horimoto, 2016/06/20 21:52:17]

Repeating proximity_auth:: isn't a bad thing - it makes it clear what type you
are referring to. In general, you should not do this, but if this results in
cleaner looking code (since you won't have to use line wraps as often in the
middle of a single line of code), then it's fine by me.

[jingxuy, 2016/06/21 01:46:56]

Done.

+typedef BluetoothLowEnergyWeavePacketGenerator::ReasonForClose ReasonForClose;
+
+const uint16_t kDefaultPacketSize = 20;
+const uint8_t kWeaveVersion = 1;
+
+void TestConnectionCloseWithReason(ReasonForClose reason_for_close,
+                                   uint16_t expected_reason_for_close) {
+  BluetoothLowEnergyWeavePacketGenerator::Factory factory{};
+
+  std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator> generator =
+      factory.NewInstance();
+
+  std::vector<uint8_t> packet =
+      generator->CreateConnectionClose(reason_for_close);
+
+  const uint16_t kCloseSize = 3;
+  std::vector<uint8_t> expected(kCloseSize, 0);
+  // uWeave Header:
+  // 1--- ---- : type = 1 (control packet)
+  // -000 ---- : counter = 0
+  // ---- 0010 : command = 2 (close)
+  // 1000 0010 = 0x82
+  expected[0] = 0x82;
+  expected[1] = expected_reason_for_close;

[Kyle Horimoto, 2016/06/17 04:04:12]

It looks like the connection close packet is 3 bytes (1-byte header and 2-byte
reason). Shouldn't we be setting expected[2] in this case?

[Kyle Horimoto, 2016/06/20 18:01:23]

Ping.

[jingxuy, 2016/06/20 21:29:32]

it's the network endian

[Kyle Horimoto, 2016/06/20 21:52:17]

In that case, please read both values into a uint16_t and use htons() and
ntohs() to do this. Your code shouldn't make assumptions about what the host- or
network-endianness are.

Please also incorporate use of these functions into your implementation file.
Thanks!

[jingxuy, 2016/06/21 01:46:56]

what do you mean? This is specified by the weave spec. We had a discussion about
this with Gustavo earlier in the generator. If you are talking about I couldn't
assume network-endianness, I don't think it would ever be reasonable for
network-endianness to change.

[Kyle Horimoto, 2016/06/21 02:15:29]

Network endianness is big-endian, though. Shouldn't they be in the opposite
order?

[jingxuy, 2016/06/21 06:03:11]

You can take a look at the cc file that Gustavo okayed. I am pretty sure the
lower byte comes first in the packet.

[Kyle Horimoto, 2016/06/21 17:17:30]

I just chatted with Gustavo, and he confirmed that the byte next to the header
should be 0.

But my point is that we shouldn't even have to think about this - the whole
point of ntohs() and htons() is that we just use those functions and let them
decide how the bytes should be ordered. Please use those functions here so that
we know we are doing the correct thing and so that someone reading this code at
a later time does not have to think about this to decide whether the bytes are
in the right order or not.

[sacomoto, 2016/06/21 17:39:22]

The _upper_ byte of the short comes first. In your implementation of the
BluetoothLowEnergyWeavePacketGenerator::SetIntField() you are also doing it:

packet->at(index) = upper;
packet->at(index + 1) = lower;

As I said in the other comment, I agree with Kyle, I prefer a solution
explicitly doing the conversions with htons().

[jingxuy, 2016/06/21 17:47:31]

No that was my original implementation. Then Gustavo corrected me and now it is
this
packet->at(byte_offset) = lower;
packet->at(byte_offset + 1) = upper;

[jingxuy, 2016/06/21 18:13:07]

I'm under the impression that endianness overcomplicates a very simple
operation. I didn't even think endianness is relevant until Gustavo brought it
up. I think it's fairly straight forward how to convert int into bytes and the
other way around without assuming anything about endianness of the machine. And
the packets endianness is very intuitive.

[Kyle Horimoto, 2016/06/21 18:39:12]

It is necessary to consider the endianness of the message as it would result in
incorrect values if we did not. Sorry, but you have to make sure the bytes are
in the right order.

[jingxuy, 2016/06/21 22:32:11]

This is resolved as the current implementation is okay but htons is more
readible.

+
+  EXPECT_EQ(expected, packet);
+}
+
+uint8_t GetCounterFromHeader(uint8_t header) {
+  return (header >> 4) & 7;
+}
+
+uint8_t GetPacketType(uint8_t header) {
+  return (header >> 7) & 1;
+}
+
+}  // namespace
+
+namespace proximity_auth {
+
+class ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest
+    : public testing::Test {
+ protected:
+  ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(
+      ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest);
+};
+
+TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest,
+       CreateConnectionRequestTest) {
+  BluetoothLowEnergyWeavePacketGenerator::Factory factory{};
+
+  std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator> generator =
+      factory.NewInstance();
+
+  std::vector<uint8_t> packet = generator->CreateConnectionRequest();

[Kyle Horimoto, 2016/06/17 04:04:12]

Can we just use Packet instead of vector<>? Same elsewhere.

[Kyle Horimoto, 2016/06/20 18:01:23]

Ping.

[jingxuy, 2016/06/20 21:29:32]

I think vector makes it more clear what the return type of the class actually is
and what functions are defined on it.

[Kyle Horimoto, 2016/06/20 21:52:17]

If so, you should get rid of your Packet typedef entirely. If you make a type to
be used, you should use it. Using it in some places and not using it in other
places results in a lack of uniformity which makes your code harder to read. So,
either remove it entirely or use it everywhere where it is applicable.

[jingxuy, 2016/06/21 01:46:56]

I was intending it to be used as a shorthand and conceptually clearer type . It
should only be used inside the h and cc file and shouldn't be used by an
external code. I will think about this...

[Kyle Horimoto, 2016/06/21 17:17:30]

Ping.

[jingxuy, 2016/06/21 18:13:07]

Done.

+
+  const uint16_t kRequestSize = 7;
+  std::vector<uint8_t> expected(kRequestSize, 0);
+  // uWeave Header:
+  // 1--- ---- :  type = 1 (control packet)
+  // -000 ---- : counter = 0
+  // ---- 0000 : command = 0 (request)
+  // 1000 0000 = 0x80
+  expected[0] = 0x80;
+  expected[1] = kWeaveVersion;
+  expected[3] = kWeaveVersion;
+
+  EXPECT_EQ(expected, packet);
+}
+
+TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest,
+       CreateConnectionResponseTest) {
+  BluetoothLowEnergyWeavePacketGenerator::Factory factory{};
+
+  std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator> generator =
+      factory.NewInstance();
+
+  // Use the default packet size.
+  std::vector<uint8_t> packet = generator->CreateConnectionResponse();
+
+  const uint16_t kResponseSize = 5;
+  std::vector<uint8_t> expected_default(kResponseSize, 0);
+  // uWeave Header:
+  // 1--- ---- : type = 1 (control packet)
+  // -000 ---- : counter = 0
+  // ---- 0001 : command = 1 (response)
+  // 1000 0001 = 0x81
+  expected_default[0] = 0x81;
+  expected_default[1] = kWeaveVersion;
+  expected_default[3] = kDefaultPacketSize;
+
+  EXPECT_EQ(expected_default, packet);
+
+  // Use a selected packet size.
+  const uint16_t kSelectedPacketSize = 30;
+  generator->SetDataPacketSize(kSelectedPacketSize);
+
+  packet = generator->CreateConnectionResponse();
+
+  std::vector<uint8_t> expected_selected(kResponseSize, 0);
+  // uWeave Header:
+  // 1--- ---- : type = 1 (control packet)
+  // -001 ---- : counter = 1
+  // ---- 0001 : command = 1 (response)
+  // 1001 0001 = 0x91
+  expected_selected[0] = 0x91;
+  expected_selected[1] = kWeaveVersion;
+  expected_selected[3] = kSelectedPacketSize;
+  EXPECT_EQ(expected_selected, packet);
+}
+
+TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest,
+       CreateConnectionCloseTest) {
+  // Reason for close spec of uWeave.
+  // 0x00: Close without error
+  // 0x01: Unknown error
+  // 0x02: No common version supported
+  // 0x03: Received packet out of sequence
+  // 0x80: Application error
+
+  TestConnectionCloseWithReason(ReasonForClose::CLOSE_WITHOUT_ERROR, 0x00);
+  TestConnectionCloseWithReason(ReasonForClose::UNKNOWN_ERROR, 0x01);
+  TestConnectionCloseWithReason(ReasonForClose::NO_COMMON_VERSION_SUPPORTED,
+                                0x02);
+  TestConnectionCloseWithReason(ReasonForClose::RECEIVED_PACKET_OUT_OF_SEQUENCE,
+                                0x03);
+  TestConnectionCloseWithReason(ReasonForClose::APPLICATION_ERROR, 0x80);
+}
+
+TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest,
+       EncodeDataMessageWithDefaultPacketSizeTest) {
+  BluetoothLowEnergyWeavePacketGenerator::Factory factory{};
+
+  std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator> generator =
+      factory.NewInstance();
+
+  std::string data = "abcdefghijklmnopqrstuvwxyz";
+
+  std::vector<std::vector<uint8_t>> packets =
+      generator->EncodeDataMessage(data);
+
+  std::vector<std::vector<uint8_t>> expected(2);
+
+  // uWeave Header:
+  // 0--- ---- : type = 0 (data packet)
+  // -000 ---- : counter = 0
+  // ---- 1--- : first packet = true
+  // ---- -0-- : last packet = false
+  // ---- --00 : defined by uWeave to be 0
+  // 0000 1000 = 0x08
+  expected[0].push_back(0x08);
+  expected[0].push_back('a');
+  expected[0].push_back('b');
+  expected[0].push_back('c');
+  expected[0].push_back('d');
+  expected[0].push_back('e');
+  expected[0].push_back('f');
+  expected[0].push_back('g');
+  expected[0].push_back('h');
+  expected[0].push_back('i');
+  expected[0].push_back('j');
+  expected[0].push_back('k');
+  expected[0].push_back('l');
+  expected[0].push_back('m');
+  expected[0].push_back('n');
+  expected[0].push_back('o');
+  expected[0].push_back('p');
+  expected[0].push_back('q');
+  expected[0].push_back('r');
+  expected[0].push_back('s');
+
+  // uWeave Header:
+  // 0--- ---- : type = 0 (data packet)
+  // -001 ---- : counter = 1
+  // ---- 0--- : first packet = false
+  // ---- -1-- : last packet = true
+  // ---- --00 : defined by uWeave to be 0
+  // 0001 0100 = 0x14
+  expected[1].push_back(0x14);
+  expected[1].push_back('t');
+  expected[1].push_back('u');
+  expected[1].push_back('v');
+  expected[1].push_back('w');
+  expected[1].push_back('x');
+  expected[1].push_back('y');
+  expected[1].push_back('z');
+
+  EXPECT_EQ(expected, packets);
+}
+
+TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest,
+       EncodeDataMessageWithSelectedPacketSizeTest) {
+  BluetoothLowEnergyWeavePacketGenerator::Factory factory{};
+
+  std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator> generator =
+      factory.NewInstance();
+
+  std::string data = "abcdefg";
+
+  generator->SetDataPacketSize(4);
+
+  std::vector<std::vector<uint8_t>> packets =
+      generator->EncodeDataMessage(data);
+
+  std::vector<std::vector<uint8_t>> expected(3);
+
+  // uWeave Header:
+  // 0--- ---- : type = 0 (data packet)
+  // -000 ---- : counter = 0
+  // ---- 1--- : first packet = true
+  // ---- -0-- : last packet = false
+  // ---- --00 : defined by uWeave to be 0
+  // 0000 1000 = 0x08
+  expected[0].push_back(0x08);
+  expected[0].push_back('a');
+  expected[0].push_back('b');
+  expected[0].push_back('c');
+
+  // uWeave Header:
+  // 0--- ---- : type = 0 (data packet)
+  // -001 ---- : counter = 1
+  // ---- 0--- : first packet = false
+  // ---- -0-- : last packet = false
+  // ---- --00 : defined by uWeave to be 0
+  // 0001 0000 = 0x10
+  expected[1].push_back(0x10);
+  expected[1].push_back('d');
+  expected[1].push_back('e');
+  expected[1].push_back('f');
+
+  // uWeave Header:
+  // 0--- ---- : type = 0 (data packet)
+  // -010 ---- : counter = 2
+  // ---- 0--- : first packet = false
+  // ---- -1-- : last packet = true
+  // ---- --00 : defined by uWeave to be 0
+  // 0010 0100 = 0x24
+  expected[2].push_back(0x24);
+  expected[2].push_back('g');
+
+  EXPECT_EQ(expected, packets);
+}
+
+TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest,
+       PacketCounterForMixedPacketTypesTest) {
+  BluetoothLowEnergyWeavePacketGenerator::Factory factory{};
+
+  std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator> generator =
+      factory.NewInstance();
+
+  std::vector<uint8_t> packet = generator->CreateConnectionRequest();
+
+  EXPECT_EQ(0, GetCounterFromHeader(packet[0]));
+
+  packet = generator->CreateConnectionResponse();
+
+  EXPECT_EQ(1, GetCounterFromHeader(packet[0]));
+
+  std::string data = "a";
+  std::vector<std::vector<uint8_t>> packets =
+      generator->EncodeDataMessage(data);
+
+  EXPECT_EQ(2, GetCounterFromHeader(packets[0][0]));
+
+  packet = generator->CreateConnectionClose(ReasonForClose::UNKNOWN_ERROR);
+
+  EXPECT_EQ(3, GetCounterFromHeader(packet[0]));
+}
+
+TEST_F(ProximityAuthBluetoothLowEnergyWeavePacketGeneratorTest,
+       PacketCounterWrappedAroundTest) {
+  BluetoothLowEnergyWeavePacketGenerator::Factory factory{};
+
+  std::unique_ptr<BluetoothLowEnergyWeavePacketGenerator> generator =
+      factory.NewInstance();
+
+  const uint8_t kDataLength = 8;
+  std::string data(8, 'a');
+
+  generator->SetDataPacketSize(2);
+
+  std::vector<std::vector<uint8_t>> packets =
+      generator->EncodeDataMessage(data);
+
+  std::vector<std::vector<uint8_t>> expected(kDataLength);
+
+  for (uint8_t i = 0; i < kDataLength; ++i) {

[Kyle Horimoto, 2016/06/17 04:04:12]

This doesn't actually test it wrapping around. You need to have at least 9
packets for it to wrap around.

[Kyle Horimoto, 2016/06/20 18:01:23]

Ping.

[jingxuy, 2016/06/20 21:29:32]

Done.

[Kyle Horimoto, 2016/06/20 21:52:17]

I don't see a change here. Did you forget to upload it?

[jingxuy, 2016/06/21 01:46:56]

Done.

[Kyle Horimoto, 2016/06/21 02:15:29]

Your newest patchset that you just uploaded doesn't have the unit test anymore
:/

[jingxuy, 2016/06/21 06:03:10]

Done.

+    uint8_t header = packets[i][0];
+    EXPECT_EQ(i, GetCounterFromHeader(header));
+    EXPECT_EQ(0, GetPacketType(header));
+  }
+}
+
+}  // namespace proximity_auth