Creates a BattOrConnection for communicating with the BattOr

tools/battor_agent/battor_connection.cc

+// Copyright 2015 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 "tools/battor_agent/battor_connection.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "device/serial/buffer.h"
+#include "device/serial/serial_io_handler.h"
+#include "net/base/io_buffer.h"
+
+using std::vector;
+
+namespace battor {
+
+namespace {
+
+// Serial configuration parameters for the BattOr.
+const uint32 kBattOrBitrate = 2000000;

[Primiano Tucci (use gerrit), 2015/12/15 11:07:20]

nit s/uint32/uint32_t/ :the non stdint versions are deprecated

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Done.

+const device::serial::DataBits kBattOrDataBits =
+    device::serial::DATA_BITS_EIGHT;
+const device::serial::ParityBit kBattOrParityBit =
+    device::serial::PARITY_BIT_NONE;
+const device::serial::StopBits kBattOrStopBit = device::serial::STOP_BITS_ONE;
+const bool kBattOrCtsFlowControl = true;
+const bool kBattOrHasCtsFlowControl = true;
+const uint32 kMaxMessageSize = 50000;

[Primiano Tucci (use gerrit), 2015/12/15 11:07:20]

ditto uint32_t

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Done.

+
+// Parses the specified message, extracting:
+//
+//   - The message content (the bytes after the start byte and message type but
+//     before the end byte)
+//   - Whether the message is complete
+//   - Whether the message is valid so far
+//   - The type of the message
+//   - The number of escape bytes contained in the message

[Zhen Wang, 2015/12/14 23:39:46]

Can you make the argument match the explanation? For example:

- message: ...
- message_content: ...
...

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Done. I also consolidated is_complete and is_invalid_so_far into a single
concept (an enum, MessageHealth).

+void ParseMessage(const vector<char>* message,

[Primiano Tucci (use gerrit), 2015/12/15 11:07:20]

ordering of arguments:
input arguments should come first and be of the form "const...&" unless there is
a reason not to.
output arguments should come last and be *.

Maybe s/message_content/parsed_content/ ?

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Done.

+                  vector<char>* message_content,
+                  bool* is_complete,
+                  bool* is_valid_so_far,
+                  BattOrMessageType* type,
+                  uint16_t* escape_byte_count) {
+  *is_complete = false;
+  *is_valid_so_far = true;
+  *type = BATTOR_MESSAGE_TYPE_CONTROL;
+  *escape_byte_count = 0;
+  message_content->reserve(message->size());
+
+  if (message->size() == 0)
+    return;
+
+  // The first byte is the start byte.
+  if ((*message)[0] != BATTOR_SPECIAL_BYTE_START) {
+    *is_valid_so_far = false;
+    return;
+  }
+
+  if (message->size() == 1)
+    return;
+
+  // The second byte specifies the message type.
+  *type = static_cast<BattOrMessageType>((*message)[1]);

[Primiano Tucci (use gerrit), 2015/12/15 11:07:19]

shouldn't you CHECK that *type isn't > MESSAGE_TYPE_MAX (or similar)

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Done. I didn't CHECK though, because it's possible that the bytes got garbled
going over the wire - I signaled an invalid state instead.

+
+  // After that comes the message data.
+  bool escape_next_byte = false;
+  for (size_t i = 2; i < message->size(); i++) {
+    if (i >= kMaxMessageSize) {
+      *is_valid_so_far = false;
+      return;
+    }
+
+    char next_byte = (*message)[i];
+
+    if (escape_next_byte) {
+      message_content->push_back(next_byte);
+      escape_next_byte = false;
+      continue;
+    }
+
+    switch (next_byte) {
+      case BATTOR_SPECIAL_BYTE_START:
+        // Two start bytes in a message is invalid.
+        *is_valid_so_far = false;
+        return;
+
+      case BATTOR_SPECIAL_BYTE_END:
+        // We've found the end of the message. Make sure that we're at the end
+        // of the message.
+        *is_complete = true;
+
+        // We're only parsing a single message here. If there are additional
+        // bytes after this byte, what we've received so far is *not* a valid
+        // message.
+        *is_valid_so_far = (i == message->size() - 1);
+        return;
+
+      case BATTOR_SPECIAL_BYTE_ESCAPE:
+        escape_next_byte = true;
+        (*escape_byte_count)++;
+        continue;
+
+      default:
+        message_content->push_back(next_byte);
+    }
+  }
+}
+
+}  // namespace
+
+BattOrConnection::BattOrConnection(
+    scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner)
+    : file_thread_task_runner_(file_thread_task_runner),
+      ui_thread_task_runner_(ui_thread_task_runner) {}
+
+BattOrConnection::~BattOrConnection() {}
+
+void BattOrConnection::Connect(
+    const std::string& path,
+    const base::Callback<void(bool success)> callback) {
+  io_handler_ = CreateIoHandler();
+
+  device::serial::ConnectionOptions options;
+  options.bitrate = kBattOrBitrate;
+  options.data_bits = kBattOrDataBits;
+  options.parity_bit = kBattOrParityBit;
+  options.stop_bits = kBattOrStopBit;
+  options.cts_flow_control = kBattOrCtsFlowControl;
+  options.has_cts_flow_control = kBattOrHasCtsFlowControl;
+
+  io_handler_->Open(path, options, callback);
+}
+
+void BattOrConnection::SendBytes(const SendCallback& callback,
+                                   BattOrMessageType type,

[Zhen Wang, 2015/12/14 23:39:46]

wrong indent

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Doh. Looks like I forgot to rerun git cl format after running git add
battor_communicator*

+                                   const char* bytes,
+                                   uint16_t bytes_to_send) {

[Zhen Wang, 2015/12/14 23:39:46]

DCHECK the length of bytes == bytes_to_send.

[Primiano Tucci (use gerrit), 2015/12/15 11:07:19]

well, how if they are not a string (read: can genenuinely contain
non-terminating 0s)?

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Yea, I don't think that it's possible to determine the size of bytes (otherwise,
I'd remove the bytes_to_send parameter altogether).

+  vector<char> data_vector;

[Primiano Tucci (use gerrit), 2015/12/15 11:07:19]

just "data" should be enough. It's clear its a vector :)

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Ah, sorry about that. At some point, the incoming parameter was named "data", so
I named the vector "data_vector" to differentiate.

+
+  // BattOr messages have the following format:
+  //
+  //   0x00               (1 byte start marker)
+  //   uint8_t            (1 byte header indicating the message type)
+  //   data               (message data, with 0x00s and 0x01s escaped with 0x02)
+  //   0x01               (1 byte end marker)
+  //
+  // We have to encode any data we're sending in this format. Because of this,
+  // we should reserve a send buffer with 3 bytes (start, header, end) and twice
+  // as many bytes as we're sending, because each raw data byte might need to be
+  // escaped.
+  data_vector.reserve(2 * bytes_to_send + 3);
+
+  data_vector.push_back(BATTOR_SPECIAL_BYTE_START);
+  data_vector.push_back(type);
+
+  for (int i = 0; i < bytes_to_send; i++) {
+    if (bytes[i] == BATTOR_SPECIAL_BYTE_START ||
+        bytes[i] == BATTOR_SPECIAL_BYTE_END)
+      data_vector.push_back(BATTOR_SPECIAL_BYTE_ESCAPE);

[Zhen Wang, 2015/12/14 23:39:46]

Add {...} when if clause takes more than 1 line.

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Done.

+
+    data_vector.push_back(bytes[i]);
+  }
+
+  data_vector.push_back(BATTOR_SPECIAL_BYTE_END);
+
+  io_handler_->Write(make_scoped_ptr(new device::SendBuffer(
+      data_vector, base::Bind(&BattOrConnection::OnBytesSent, AsWeakPtr(),
+                              callback, data_vector.size()))));
+}
+
+void BattOrConnection::ReadBytes(const ReadCallback& callback,
+                                   uint16_t bytes_to_read) {

[Zhen Wang, 2015/12/14 23:39:44]

wrong indent

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Doh.

+  // Add 3 bytes to however many bytes the caller requested to account for the
+  // start, type, and end bytes that are included in every BattOr message.
+  bytes_to_read += 3;
+
+  scoped_ptr<vector<char>> bytes_already_read(new vector<char>());
+  ReadMoreBytes(callback, bytes_to_read, bytes_already_read.Pass());
+}
+
+void BattOrConnection::Flush() {
+  io_handler_->Flush();
+}
+
+scoped_refptr<device::SerialIoHandler> BattOrConnection::CreateIoHandler() {
+  return device::SerialIoHandler::Create(file_thread_task_runner_,
+                                         ui_thread_task_runner_);
+}
+
+void BattOrConnection::ReadMoreBytes(
+    const ReadCallback& callback,
+    uint16_t bytes_to_read,
+    scoped_ptr<vector<char>> bytes_already_read) {

[Primiano Tucci (use gerrit), 2015/12/15 11:07:19]

I wonder if this bytes_alread_read is really an argument here or is part of the
state of your connection class ?
you keep hopping this back and forth

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Good point. I added a new private variable named pending_read_buffer_ and this
seemed to clean things up a lot.

+  auto io_buffer = make_scoped_refptr(new net::IOBuffer((size_t)bytes_to_read));

[Primiano Tucci (use gerrit), 2015/12/15 11:07:19]

no c-style casts, use static_cast?
(actually why don't you use size_t everywhere instead of int16_t?)

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

Done, although this did involve some casting in order to match a few of the
callbacks that had interfaces dictated by device/serial. 

Also, there are a few places I would've liked to check for overflow before
casting, but it turns out that problem is surprisingly difficult

+  auto on_receive_buffer_filled =
+      base::Bind(&BattOrConnection::OnBytesRead, AsWeakPtr(), callback,
+                 base::Passed(&bytes_already_read), io_buffer, bytes_to_read);
+
+  io_handler_->Read(make_scoped_ptr(new device::ReceiveBuffer(
+      io_buffer, bytes_to_read, on_receive_buffer_filled)));
+}
+
+void BattOrConnection::OnBytesSent(
+    const BattOrConnection::SendCallback& callback,
+    int expected_bytes_sent,
+    int bytes_sent,
+    device::serial::SendError error) {
+  bool success = (error == device::serial::SEND_ERROR_NONE) &&
+                 (expected_bytes_sent == bytes_sent);
+  callback.Run(success);
+}
+
+void BattOrConnection::OnBytesRead(
+    const BattOrConnection::ReadCallback& callback,
+    scoped_ptr<vector<char>> bytes_already_read,
+    scoped_refptr<net::IOBuffer> buffer,
+    int expected_bytes_read,
+    int bytes_read,
+    device::serial::ReceiveError error) {
+  if ((bytes_read < expected_bytes_read) ||
+      (error != device::serial::RECEIVE_ERROR_NONE)) {
+    callback.Run(false, BATTOR_MESSAGE_TYPE_CONTROL, bytes_already_read.Pass());
+    return;
+  }
+
+  scoped_ptr<vector<char>> message_content(new vector<char>());
+  bool is_complete;
+  bool is_valid_so_far;
+  BattOrMessageType type;
+  uint16_t escape_byte_count;
+
+  ParseMessage(bytes_already_read.get(), message_content.get(), &is_complete,
+               &is_valid_so_far, &type, &escape_byte_count);
+
+  // Parse the piece of the message that we already had in order to determine
+  // how many escape bytes we've already read.

[Zhen Wang, 2015/12/14 23:39:46]

Move the comment to above ParseMessage.

Some more thoughts:
It looks like we only need escape_byte_count here. And we only need some other
info in the below ParseMessage. Those two have different purposes. Can we make
two functions, each handle a different task? Those two functions can share the
same parsing logic.

[charliea (OOO until 10-5), 2015/12/15 23:50:04]

I had thought about that, but I was trying to avoid another reparsing the of the
message. I think that I've gotten around this by having another temporary
variable (alongside the newly-created pending_read_buffer_). Let me know what
you think.

+  uint16_t escape_byte_already_requested_count = escape_byte_count;
+
+  message_content->clear();
+  bytes_already_read->insert(bytes_already_read->end(), buffer->data(),
+                             buffer->data() + bytes_read);
+
+  // Reparse the message with the new bytes added on in order to determine how
+  // many new escape bytes we've received and whether the message is complete.
+  ParseMessage(bytes_already_read.get(), message_content.get(), &is_complete,
+               &is_valid_so_far, &type, &escape_byte_count);
+
+  if (!is_valid_so_far) {
+    // If we already have an invalid message, there's no sense in continuing to
+    // process it.
+    callback.Run(false, BATTOR_MESSAGE_TYPE_CONTROL, bytes_already_read.Pass());
+    return;
+  }
+
+  uint16_t additional_escape_bytes =
+      escape_byte_count - escape_byte_already_requested_count;
+  if (additional_escape_bytes > 0) {
+    // When the caller requested that we read X additional bytes, they weren't
+    // taking into account any escape bytes that we received. Because we got
+    // some escape bytes, we need to fire off another read to get the rest of
+    // the data.
+    ReadMoreBytes(callback, additional_escape_bytes, bytes_already_read.Pass());
+    return;
+  }
+
+  if (!is_complete)
+    // If everything is valid and we didn't see any escape bytes, then we should
+    // have the whole message. If we don't, the message was malformed.
+    callback.Run(false, BATTOR_MESSAGE_TYPE_CONTROL, bytes_already_read.Pass());
+
+  // If we've gotten this far, we've received the whole, well-formed message.
+  callback.Run(true, type, message_content.Pass());
+}
+
+}  // namespace battor