Creates a BattOrConnection for communicating with the BattOr

tools/battor_agent/battor_protocol_types.h

Index: tools/battor_agent/battor_protocol_types.h
diff --git a/tools/battor_agent/battor_protocol_types.h b/tools/battor_agent/battor_protocol_types.h
new file mode 100644
index 0000000000000000000000000000000000000000..47125f46e8a6f26cc30c5e3dc0165c406dca1dda
--- /dev/null
+++ b/tools/battor_agent/battor_protocol_types.h
@@ -0,0 +1,77 @@
+// 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_PROTOCOL_H_
+#define TOOLS_BATTOR_AGENT_BATTOR_PROTOCOL_H_
+
+namespace battor {
+
+// Control characters in the BattOr protocol.
+enum BattOrControlByte : uint8_t {

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

I wonder if all these should be c++11 enum classes, i.e.
enum class BattOrControlByte :  uint8_t { 
 START = 0,
 END = 1
 ...
 MAX = 2
}

this would also avoid to repeat the BATTOR_...TYPE prefix, as the scoped would
be enclosed in BattorControlByte (e.g., BattorControlByte::START)

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

Definitely not an expert in this area, but it seems like this might be one of
not-very-many cases where we actually want loosely typed (non-class) enums
because we want it to be really easy to use the enums as their hex values. The
strongly typed class enums seem to excel at being hard to use as integers, which
is exactly what we want to do.

For example, with loosely typed enums, we can do:

  if ((*message)[0] != BATTOR_CONTROL_BYTE_START) {

whereas, with strongly typed enums, we'd have to do:

  if ((*message[0] != static_cast<char>(BattOrControlByte::Start))) {

at least for me, the version w/o the cast seems less ugly.

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

ah ok got it, makes sense

+  // Indicates the start of a message in the protocol. All other instances of
+  // this byte must be escaped (with BATTOR_SPECIAL_BYTE_ESCAPE).
+  BATTOR_SPECIAL_BYTE_START = 0x00,
+  // Indicates the end of a message in the protocol. All other instances of
+  // this byte must be escaped (with BATTOR_SPECIAL_BYTE_ESCAPE).
+  BATTOR_SPECIAL_BYTE_END,

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

I think explicitly writing the values here would make it easier to understand
the protocol, especially when BattOrMessageType follows BattOrControlByte
values.

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

+1

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

Done.

+  // Indicates that the next byte should not be interpreted as a special
+  // character, but should instead be interpreted as itself.
+  BATTOR_SPECIAL_BYTE_ESCAPE,
+};
+
+// Types of BattOr messages that can be sent.
+enum BattOrMessageType : uint8_t {
+  // Indicates a control message sent from the client to the BattOr to tell the
+  // BattOr to do something.
+  BATTOR_MESSAGE_TYPE_CONTROL = 0x03,
+  // Indicates a control message ack sent from the BattOr back to the client to
+  // signal that the BattOr received the control message.
+  BATTOR_MESSAGE_TYPE_CONTROL_ACK,
+  // Indicates that the message contains Voltage and current measurements.
+  BATTOR_MESSAGE_TYPE_SAMPLES,
+  // TODO(charliea): Figure out what this is.
+  BATTOR_MESSAGE_TYPE_PRINT,
+};
+
+// Types of BattOr control messages that can be sent.
+enum BattOrControlMessageType : uint8_t {
+  // Tells the BattOr to initialize itself.
+  BATTOR_CONTROL_MESSAGE_TYPE_INIT = 0x00,
+  // Sets the current measurement's gain.
+  BATTOR_CONTROL_MESSAGE_TYPE_SET_GAIN,
+  // Tells the BattOr to start taking samples and sending them over the
+  // connection.
+  BATTOR_CONTROL_MESSAGE_TYPE_START_SAMPLING_UART,
+  // Tells the BattOr to start taking samples and storing them on its SD card.
+  BATTOR_CONTROL_MESSAGE_TYPE_START_SAMPLING_SD,
+  // Tells the BattOr to start streaming the samples stored on its SD card over
+  // the connection.
+  BATTOR_CONTROL_MESSAGE_TYPE_READ_SD_UART,
+  // Tells the BattOr to send its EEPROM contents over the serial connection.
+  BATTOR_CONTROL_MESSAGE_TYPE_READ_EEPROM,
+  // Tells the BattOr to reset itself.
+  BATTOR_CONTROL_MESSAGE_TYPE_RESET,
+  // Tells the BattOr to run a self test.
+  BATTOR_CONTROL_MESSAGE_TYPE_SELF_TEST,
+};
+
+// See: BattOrMessageType::BATTOR_MESSAGE_TYPE_CONTROL above.
+struct BattOrControlMessage {

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

Not really a huge fan of packed structs, make the code generally harder to read
an debug.
Since you are using this only in one place, maybe you want to just pack it
explicitly (by writing the individual bytes?)

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

In our case, the BattOr firmware expects the struct to be packed, which means
that we need to do it in order to ensure that what we send over the wire is
compatible. It seems like manually packing the struct doesn't really get us much
besides added code complexity. 

I added a comment above stating why the packing is necessary - do you think
that's good enough?

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

ok makes sense

+  BattOrControlMessageType type;
+  uint16_t param1;
+  uint16_t param2;
+} __attribute__((packed));
+
+// See: BattOrMessageType::BATTOR_MESSAGE_TYPE_CONTROL_ACK above.
+struct BattOrControlMessageAck {

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

nothing seems to use this

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

Ah, sorry. I'll add this back in when the next CL uses it.

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

ah nevermind if you are going to use it.
I though was a leftover

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

SG. In that case, I went ahead and added it back in.

+  BattOrControlMessageType type;
+  uint8_t param;
+} __attribute__((packed));
+
+// The gain level for the BattOr to use.
+enum BattOrGain : uint8_t { BATTOR_GAIN_LOW = 0, BATTOR_GAIN_HIGH };
+
+}  // namespace battor
+
+#endif  // TOOLS_BATTOR_AGENT_BATTOR_PROTOCOL_H_