Weave Packet Receiver

components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.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_receiver.h"
+
+#include "base/logging.h"

[Tim Song, 2016/06/09 22:08:34]

Please use "components/proximity_auth/logging.h"

[jingxuy, 2016/06/09 23:29:03]

base/logging is where DCHECK is defined, should I actually switch this include?

[Tim Song, 2016/06/10 21:25:21]

Okay, it seems like you don't do any logging, so this should be fine.

[jingxuy, 2016/06/11 00:10:46]

oh actually I need to do some logging and should I use the DLOG from
base/logging or does proximity auth logging do something special

[Tim Song, 2016/06/11 00:39:45]

The proximity_auth specific logger PA_LOG is a wrapper over the regular logging
library, so we can distinguish which logs are from our component.

[jingxuy, 2016/06/17 18:59:51]

Done.

+
+using proximity_auth::BluetoothLowEnergyWeavePacketGenerator;
+
+namespace proximity_auth {
+namespace {
+typedef BluetoothLowEnergyWeavePacketGenerator::PacketType PacketType;
+typedef BluetoothLowEnergyWeavePacketGenerator::ControlPacketCommand
+    ControlPacketCommand;
+typedef BluetoothLowEnergyWeavePacketGenerator::ReasonForClose ReasonForClose;
+
+const uint8_t kMaxPacketCounter = 8;
+}  // namespace
+
+BluetoothLowEnergyWeavePacketReceiver::Factory*
+    BluetoothLowEnergyWeavePacketReceiver::Factory::factory_instance_ = nullptr;
+
+std::unique_ptr<BluetoothLowEnergyWeavePacketReceiver>
+BluetoothLowEnergyWeavePacketReceiver::Factory::NewInstance() {
+  if (factory_instance_ == nullptr) {
+    factory_instance_ = new Factory();
+  }
+  return std::unique_ptr<BluetoothLowEnergyWeavePacketReceiver>(
+      factory_instance_->BuildInstance());
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::Factory::SetInstanceForTesting(
+    Factory* factory) {
+  factory_instance_ = factory;
+}
+
+BluetoothLowEnergyWeavePacketReceiver*
+BluetoothLowEnergyWeavePacketReceiver::Factory::BuildInstance() {
+  return new BluetoothLowEnergyWeavePacketReceiver();
+}
+
+BluetoothLowEnergyWeavePacketReceiver::BluetoothLowEnergyWeavePacketReceiver()
+    : packet_size_(0),
+      packet_number_(0),
+      state_(State::EXCHANGING_CONTROLS),
+      reason_for_close_(ReasonForClose::CLOSE_WITHOUT_ERROR) {}
+
+BluetoothLowEnergyWeavePacketReceiver::
+    ~BluetoothLowEnergyWeavePacketReceiver() {}
+
+BluetoothLowEnergyWeavePacketReceiver::State
+BluetoothLowEnergyWeavePacketReceiver::GetState() {
+  return state_;
+}
+
+uint32_t BluetoothLowEnergyWeavePacketReceiver::GetPacketSize() {
+  return packet_size_;
+}
+
+ReasonForClose BluetoothLowEnergyWeavePacketReceiver::GetReasonForClose() {
+  return reason_for_close_;
+}
+
+std::string BluetoothLowEnergyWeavePacketReceiver::GetDataMessage() {
+  return std::string(data_message_.begin(), data_message_.end());

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

Check if state_ == DATA_READY?

[jingxuy, 2016/06/09 23:29:03]

This entire section has been rewritten.

+}
+
+void BluetoothLowEnergyWeavePacketReceiver::ReceivePacket(
+    const Packet& packet) {
+  if (state_ == State::ERROR) {
+    return;
+  }
+
+  VerifyPacketCounter(packet);
+
+  if (state_ == State::ERROR) {
+    return;
+  }
+
+  PacketType type = GetPacketType(packet);
+
+  switch (type) {
+    case PacketType::CONTROL:
+      switch (GetControlCommand(packet)) {

[Tim Song, 2016/06/09 22:08:34]

Please refactor this switch to its own function.

[jingxuy, 2016/06/09 23:29:03]

Done.

+        case ControlPacketCommand::CONNECTION_REQUEST:
+          if (state_ == State::RECEIVING_DATA) {

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

Shouldn't this be checking that state_ != EXCHANGING_CONTROLS? You should also
do a state transition here.

[jingxuy, 2016/06/09 23:29:03]

Done.

+            state_ = State::ERROR;
+            return;
+          }
+          // Currently just ignoring the version stuff because it's always 1.
+          packet_size_ = GetShortField(packet, 5);
+          // TODO(jingxuy): what do I do with the 13 bytes of data?

[Tim Song, 2016/06/09 22:08:34]

I don't think you'll get any extra data. The packet that you receive doesn't
have to be of the maximum packet size.

[jingxuy, 2016/06/09 23:29:03]

This is some uWeave stuff that all control packets are 20 bytes and that extra
data could be tagged at the end. I was more asking Gustavo of this

[Tim Song, 2016/06/10 21:25:21]

The Android and Mac implementations do not append anything after the control
signal. I don't think you should worry about it for now.

[jingxuy, 2016/06/11 00:10:46]

yah, I think worrying about it actually creates more problem that I might need
another state. But I think the receiver should be more generalized to be able to
deal with that data since the protocol allows it.

+          break;
+        case ControlPacketCommand::CONNECTION_RESPONSE:
+          if (state_ == State::RECEIVING_DATA) {

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

shouldn't this be checking that state_ != EXCHANGING_CONTROLS? You should also
do a state transition here.

[jingxuy, 2016/06/09 23:29:03]

Done.

+            state_ = State::ERROR;
+            return;
+          }
+          // Currently just ignoring the version stuff because it's always 1.
+          packet_size_ = GetShortField(packet, 3);
+          // TODO(jingxuy): what do I do with the 15 bytes of data?

[Tim Song, 2016/06/09 22:08:34]

same here.

[jingxuy, 2016/06/09 23:29:03]

Done.

+          break;
+        case ControlPacketCommand::CONNECTION_CLOSE:

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

check that the current state is valid.

[jingxuy, 2016/06/09 23:29:03]

Done.

+          reason_for_close_ =
+              static_cast<ReasonForClose>(GetShortField(packet, 1));
+          state_ = State::CONNECTION_CLOSED;
+          break;
+        default:
+          state_ = State::ERROR;
+          return;
+      }
+      break;
+    case PacketType::DATA:

[Tim Song, 2016/06/09 22:08:34]

Also refactor this block into its own function.

[jingxuy, 2016/06/09 23:29:04]

Done.

+      if (IsFirstDataPacket(packet)) {
+        if (state_ != State::EXCHANGING_CONTROLS) {

[Tim Song, 2016/06/09 22:08:34]

Shouldn't this state you check here be DATA_READY?

[jingxuy, 2016/06/09 23:29:03]

Done.

+          state_ = State::ERROR;
+          return;
+        }
+        data_message_.clear();
+        state_ = State::RECEIVING_DATA;
+      }
+
+      if (state_ != State::RECEIVING_DATA) {
+        state_ = State::ERROR;
+        return;
+      }
+
+      if (IsLastDataPacket(packet)) {
+        state_ = State::DATA_READY;
+      }
+
+      // Append to data_message_ bytes 1 through end of the packet.
+      data_message_.insert(data_message_.end(), packet.begin() + 1,
+                           packet.end());
+      break;
+    default:
+      state_ = State::ERROR;
+      return;
+  }
+}
+
+uint16_t BluetoothLowEnergyWeavePacketReceiver::GetShortField(
+    const Packet& packet,
+    uint32_t index) {
+  DCHECK_LT(index, packet.size());

[Tim Song, 2016/06/09 22:08:34]

I don't think you should DCHECK this condition, as we are getting these packets
from the remote device. We should deal with invalid packets (state_ = ERROR?)
rather than crashing.

[jingxuy, 2016/06/09 23:29:03]

This is an internal function. I'm just dchecking to make sure that user didn't
call it with anything invalid. In the new version of code, control won't even
reach here if the state is ERROR.

+  DCHECK_LT(index + 1, packet.size());
+
+  // packet[index + 1] is the upper byte and packet[index] is the lower byte.
+  return (packet[index + 1] << 8) | packet[index];
+}
+
+PacketType BluetoothLowEnergyWeavePacketReceiver::GetPacketType(
+    const Packet& packet) {
+  // Packet type is stored in the highest bit of the first byte.
+  return static_cast<PacketType>((packet[0] >> 7) & 1);
+}
+
+ControlPacketCommand BluetoothLowEnergyWeavePacketReceiver::GetControlCommand(
+    const Packet& packet) {
+  // Control command is stored in the lower 4 bits of the first byte.
+  return static_cast<ControlPacketCommand>((packet[0] & 0x0F));
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::VerifyPacketCounter(
+    const Packet& packet) {
+  // Packet counter is bits 4, 5, and 6 of the first byte.
+  uint8_t count = (packet[0] >> 4) & 7;
+
+  if (count != (packet_number_ % kMaxPacketCounter)) {
+    state_ = State::ERROR;
+    return;
+  }
+  packet_number_++;
+}
+
+bool BluetoothLowEnergyWeavePacketReceiver::IsFirstDataPacket(
+    const Packet& packet) {
+  // Bit determining whether the packet is first pacet is bit 3.

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

nit: s/pacet/packet/

[jingxuy, 2016/06/09 23:29:04]

Done.

+  return (packet[0] >> 3) & 1;
+}
+
+bool BluetoothLowEnergyWeavePacketReceiver::IsLastDataPacket(
+    const Packet& packet) {
+  // Bit determining whether the packet is first pacet is bit 2.
+  return (packet[0] >> 2) & 1;
+}
+
+}  // namespace proximity_auth