Creates a BattOrConnection for communicating with the BattOr

tools/battor_agent/battor_connection.h

+// 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.
+
+#ifndef TOOLS_BATTOR_AGENT_BATTOR_CONNECTION_H_
+#define TOOLS_BATTOR_AGENT_BATTOR_CONNECTION_H_
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "device/serial/serial.mojom.h"
+#include "tools/battor_agent/battor_error.h"
+#include "tools/battor_agent/battor_protocol_types.h"
+
+namespace device {
+class SerialIoHandler;
+}
+namespace net {
+class IOBuffer;
+}
+
+namespace battor {
+
+// A BattOrConnection is a wrapper around the serial connection to the BattOr
+// that handles conversion of a message to and from the byte-level BattOr
+// protocol.

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

Is there a description of BattOr protocol and how it works? Or is there a link
to such thing (e.g., a github link)?

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

Done.

+class BattOrConnection : public base::SupportsWeakPtr<BattOrConnection> {

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

Mostly a design thing here:
If you used the same Listener pattern here (have an inner Listener class, which
is imnplemented by BattOrAgent) you could avoid having an extra WeakPtr to care
about here.

The way it would work would be:
this class would keep a WeakPtr<Listener> listener_ here in its private section.
If you destroy the BattOrAgent (which owns the connection) what would happen is
that the outstanding callbacks (from Connection -> Agent) would be automatically
canceled, because the weakptr has been invalidated.

Also, that would avoid you to bind() everything when you call methods, making
the code a bit more readable and more performant.

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

I thought about this for a while (obviously, that's the system we're using for
the BattOrAgent), and couldn't decide whether it was overkill or not. I'll go
ahead and implement it.

+ public:
+  typedef base::Callback<void(bool success)> SendCallback;

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

in C++ using "using" is the preferred way for this (it's just a bit more
readable)
i.e. using SendCallback = base::Callback<void(bool success)>

same below

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

Ack. 

This actually went away after implementing the Listener interface.

+  typedef base::Callback<void(bool success,
+                              BattOrMessageType type,
+                              scoped_ptr<std::vector<char>>)> ReadCallback;
+
+  BattOrConnection(
+      scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner);
+  virtual ~BattOrConnection();
+
+  // Initializes the serial connection and calls the callback when complete.
+  void Connect(const std::string& path,
+               const base::Callback<void(bool success)> callback);

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

Ironically this has the same signature of SendCallback, but here you take the
full signature (by copy) and below (in SendBytes) you use the typedef and make
it const& (which makes sense).
Be consistent (I personally tend to like the "using" + short version here,
otherwise when you add const ... & the full signature would become unreadable)

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

Acknowledged. Not completely sure what my reasoning was here... I remember
thinking about this, but it doesn't look like I actually landed on a coherent
plan. I ended up going with the listener interface, anyhow. 

+
+  // Sends the specified message over the serial connection and calls the
+  // callback when complete. Note that the number of bytes requested should
+  // not include the start, end, or type bytes required by the BattOr protocol.
+  void SendBytes(const SendCallback& callback,

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

Small nit: I'd move the callback to be the last argument (if you decide to keep
the callbacks instead of using the listener), as it is kind of a return argument
if you think about that.
It's just more readable and intuitive if you say:
"this is what I want to write, these are the bytes, this is callback where you
tell me when you are done."
as opposite to:
"this is the callback to invoke when you are done, ah by the way these are the
bytes I want to send"

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

Ack. I ended up going with the Listener interface, but it's a fair point
nonetheless.

I definitely agree that I prefer the out parameters to go at the end of the
argument list. The problem that I ran into was that, lots of times, I'd want to
use partial application to bind the callback as a first parameter in a method
that needed to fit a certain interface. Here's an example:

void Connection::Send(const std::string& message,
          const base::Callback<void(bool success)>& callback) {
  io_handler_->Send(
      message,
      base::Bind(&Connection::InternalSendCallback, AsWeakPtr(), callback));
}

void Connection::InternalSendCallback(
    const base::Callback<void(bool success)>& callback,
    device::serial::SendError error) {
  // Do some transformation on the raw return value - here, we just convert from
  // device::serial::SendError to a success bool. 
  callback.Run(error == SEND_ERROR_NONE);
}

Because I can only do partial application to parameters at the beginning of the
parameter list, it forces me to put the callback there. 

Then, for consistency, I don't want the methods that need to be partially
applied to be the only ones with first-parameter callbacks, so I just moved all
of the callbacks to the first method.

+                 BattOrMessageType type,
+                 const char* bytes,

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

typically I see this as "const void*".
It typically makes the casting easier on the caller side.

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

Done.

+                 uint16_t bytes_to_send);
+
+  // Reads the specified number of bytes from the serial connection and calls
+  // the callback when complete. Note that the number of bytes requested should
+  // not include the start, end, or type bytes required by the BattOr protocol,
+  // and that this method may issue multiple read read requests if the message
+  // contains escape characters.
+  void ReadBytes(const ReadCallback& callback, uint16_t bytes_to_read);

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

ditto here about ordering

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

(see above)

+
+  // Flushes the serial connection with the BattOr.
+  void Flush();
+
+ protected:

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

why protected? Is there anything extending this class?

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

In battor_connection_unittest, we override CreateIoHandler() to use a fake
serial connection.

[Primiano Tucci (use gerrit), 2015/12/16 16:47:43]

ah got it, thanks
Maybe add a one line comment saying: overridden by test mockup, or similar.

[charliea (OOO until 10-5), 2015/12/16 18:27:55]

Done.

+  virtual scoped_refptr<device::SerialIoHandler> CreateIoHandler();
+
+  // IO handler capable of reading and writing from the serial connection.
+  scoped_refptr<device::SerialIoHandler> io_handler_;
+
+ private:
+  // A version of ReadMoreBytes that passes along a vector of bytes that have
+  // already been read. This is useful when we need to issue multiple reads
+  // because there were escape bytes that caused us to not read enough data
+  // in the first read.
+  void ReadMoreBytes(const ReadCallback& callback,
+                     uint16_t bytes_to_read,
+                     scoped_ptr<std::vector<char>> bytes_already_read);
+  // Internal callback for when bytes are sent.
+  void OnBytesSent(const BattOrConnection::SendCallback& callback,
+                   int expected_bytes_sent,
+                   int bytes_sent,
+                   device::serial::SendError error);
+
+  // Internal callback for when bytes are read. This method may trigger
+  // additional reads if any newly read bytes are escape bytes.
+  void OnBytesRead(const BattOrConnection::ReadCallback& callback,
+                   scoped_ptr<std::vector<char>> bytes_already_read,
+                   scoped_refptr<net::IOBuffer> buffer,
+                   int expected_bytes_read,
+                   int bytes_read,
+                   device::serial::ReceiveError error);
+
+  // Threads needed for serial communication.
+  scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(BattOrConnection);
+};
+
+}  // namespace battor
+
+#endif  // TOOLS_BATTOR_AGENT_BATTOR_CONNECTION_H_