[battor agent] Fix the init sequence so it retries inits instead of resets.

tools/battor_agent/battor_agent.cc

Index: tools/battor_agent/battor_agent.cc
diff --git a/tools/battor_agent/battor_agent.cc b/tools/battor_agent/battor_agent.cc
index 708192d1791d700b301f1a00e3017db516ed19ff..2eb61f3e51de1c2aa164a5af638c1a3ae977ca6f 100644
--- a/tools/battor_agent/battor_agent.cc
+++ b/tools/battor_agent/battor_agent.cc
@@ -5,6 +5,7 @@
 #include "tools/battor_agent/battor_agent.h"
 
 #include <iomanip>
+#include <iostream>
 
 #include "base/bind.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -17,12 +18,15 @@ namespace battor {
 
 namespace {
 
-// The number of seconds that it takes a BattOr to reset.
-const uint8_t kBattOrResetTimeSeconds = 5;
-
 // The maximum number of times to retry when reading a message.
 const uint8_t kMaxReadAttempts = 20;
 
+// The maximum number of times to retry when init'ing a battor.

[charliea (OOO until 10-5), 2016/06/10 17:05:33]

same nit about maybe putting this above the read attempts stuff because the init
happens first

[aschulman, 2016/06/10 18:27:25]

Done.

+const uint8_t kMaxInitAttempts = 20;
+
+// The number of milliseconds to wait before trying to init again.
+const uint16_t kInitRetryDelayMilliseconds = 100;
+
 // The number of milliseconds to wait before trying to read a message again.
 const uint8_t kReadRetryDelayMilliseconds = 1;
 
@@ -31,7 +35,7 @@ const uint8_t kReadRetryDelayMilliseconds = 1;
 const uint8_t kStopTracingClockSyncDelayMilliseconds = 100;
 
 // The number of seconds allowed for a given action before timing out.
-const uint8_t kBattOrTimeoutSeconds = 10;
+const uint8_t kBattOrTimeoutSeconds = 4;
 
 // Returns true if the specified vector of bytes decodes to a message that is an
 // ack for the specified control message type.
@@ -119,7 +123,8 @@ BattOrAgent::BattOrAgent(
       listener_(listener),
       last_action_(Action::INVALID),
       command_(Command::INVALID),
-      num_read_attempts_(0) {
+      num_read_attempts_(0),
+      num_init_attempts_(0) {

[charliea (OOO until 10-5), 2016/06/10 17:05:33]

same nit (swap init and read)

[aschulman, 2016/06/10 18:27:24]

Done.

   // We don't care what thread the constructor is called on - we only care that
   // all of the other method invocations happen on the same thread.
   thread_checker_.DetachFromThread();
@@ -172,13 +177,11 @@ void BattOrAgent::OnConnectionOpened(bool success) {
     return;
   }
 
+

[charliea (OOO until 10-5), 2016/06/10 17:05:33]

nit: delete extra \n

[aschulman, 2016/06/10 18:27:25]

Done.

   switch (command_) {
     case Command::START_TRACING:
-      // TODO(charliea): Ideally, we'd just like to send an init, and the BattOr
-      // firmware can handle whether a reset is necessary or not, sending an
-      // init ack regardless. This reset can be removed once this is true.
-      // https://github.com/aschulm/battor/issues/30 tracks this.
-      PerformAction(Action::SEND_RESET);
+      num_init_attempts_ = 1;
+      PerformAction(Action::SEND_INIT);
       return;
     case Command::STOP_TRACING:
       PerformAction(Action::SEND_EEPROM_REQUEST);
@@ -206,16 +209,6 @@ void BattOrAgent::OnBytesSent(bool success) {
   }
 
   switch (last_action_) {
-    case Action::SEND_RESET:
-      // TODO(charliea): Ideally, we'd just like to send an init, and the BattOr
-      // firmware can handle whether a reset is necessary or not, sending an
-      // init ack regardless. This reset can be removed once this is true.
-      // https://github.com/aschulm/battor/issues/30 tracks this.
-
-      // Wait for the reset to happen before sending the init message.
-      PerformDelayedAction(Action::SEND_INIT, base::TimeDelta::FromSeconds(
-                                                  kBattOrResetTimeSeconds));
-      return;
     case Action::SEND_INIT:
       PerformAction(Action::READ_INIT_ACK);
       return;
@@ -266,6 +259,18 @@ void BattOrAgent::OnMessageRead(bool success,
                                                kReadRetryDelayMilliseconds));
         return;
 
+      // Retry sending an INIT if it an ACK is not received.
+      case Action::SEND_INIT:
+      case Action::READ_INIT_ACK:
+        if (++num_init_attempts_ < kMaxInitAttempts) {

[charliea (OOO until 10-5), 2016/06/10 17:05:33]

This definitely stems from problematic code that I wrote, but I think that (here
and elsewhere) it should actually be 

if (num_init_attempts++ < kMaxInitAttempts)

Logically, it makes sense that "max init attempts" is the maximum number of
times that you'd attempt an init before giving up. However, right now, if
kMaxInitAttempts = 2, then 

(++num_init_attempts < kMaxInitAttempts) will return false before the first
retry, which is problematic.

Could you change all of the ++num_init_attempts to num_init_attempts++? I'll
submit a separate CL doing the same for num_read_attempts

[aschulman, 2016/06/10 18:27:25]

Done.

+          PerformDelayedAction(Action::SEND_INIT,
+              base::TimeDelta::FromMilliseconds(kInitRetryDelayMilliseconds));
+        } else {
+          CompleteCommand(BATTOR_ERROR_INIT_MAX_RETRIES);
+        }
+
+        return;
+
       default:
         CompleteCommand(BATTOR_ERROR_RECEIVE_ERROR);
         return;
@@ -276,8 +281,14 @@ void BattOrAgent::OnMessageRead(bool success,
     case Action::READ_INIT_ACK:
       if (!IsAckOfControlCommand(type, BATTOR_CONTROL_MESSAGE_TYPE_INIT,
                                  *bytes)) {
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
-        return;
+        if (++num_init_attempts_ < kMaxInitAttempts) {
+          PerformDelayedAction(Action::SEND_INIT,
+              base::TimeDelta::FromMilliseconds(kInitRetryDelayMilliseconds));
+        } else {
+          CompleteCommand(BATTOR_ERROR_INIT_MAX_RETRIES);
+        }
+
+       return;

[charliea (OOO until 10-5), 2016/06/10 17:05:33]

I think this return; is indented wrong (by one space). 

(I'd recommend running `git cl format` to make sure that all of the C++ code in
the CL is formatted correctly)

[aschulman, 2016/06/10 18:27:24]

Done.

       }
 
       PerformAction(Action::SEND_SET_GAIN);
@@ -400,15 +411,8 @@ void BattOrAgent::PerformAction(Action action) {
       return;
 
     // The following actions are required for StartTracing:
-    case Action::SEND_RESET:
-      // Reset the BattOr to clear any preexisting state. After sending the
-      // reset signal, we need to wait for the reset to finish before issuing
-      // further commands.
-      SendControlMessage(BATTOR_CONTROL_MESSAGE_TYPE_RESET, 0, 0);
-      return;
     case Action::SEND_INIT:
-      // After resetting the BattOr, we need to make sure to flush the serial
-      // stream. Strange data may have been written into it during the reset.
+      // Clear out the serial data that may exist from prior init attempts.
       connection_->Flush();
 
       SendControlMessage(BATTOR_CONTROL_MESSAGE_TYPE_INIT, 0, 0);
@@ -483,7 +487,20 @@ void BattOrAgent::PerformDelayedAction(Action action, base::TimeDelta delay) {
 }
 
 void BattOrAgent::OnActionTimeout() {
-  CompleteCommand(BATTOR_ERROR_TIMEOUT);
+  switch (last_action_) {
+    case Action::READ_INIT_ACK:
+      // Cancel INIT ACK read, OnReadMessage() will fail and retry SEND_INIT.

[charliea (OOO until 10-5), 2016/06/10 17:05:33]

I'd put this comment inside of the if clause, because we only cancel if the
condition is met. I don't think that means you have to put braces around it
(because it's just a comment), but I'm honestly not sure.

(Also, looking around, there are plenty of places that I don't do this in this
file... sorry about that :-/ I think that this is something that I didn't notice
I was doing 6 months ago but I now notice.)

I'd also recommend scrapping the "Cancel INIT ACK read" part of the comment,
because it's just describing what's obvious in the code. I think that Chromium's
general attitude is that we prefer to use comments to describing non-intuitive
parts of the code being commented rather than for as a second means of
documenting what's happening.

e.g.
// This will cause OnMessageRead() to be called with success=false.
connection_->CancelReadMessage().

and not

// Cancels the read message.
connection_->CancelReadMessage();

[charliea (OOO until 10-5), 2016/06/10 17:05:33]

I think the function is OnMessageRead and not OnReadMessage

[aschulman, 2016/06/10 18:27:24]

Done.

[aschulman, 2016/06/10 18:27:25]

Done.

+      if (++num_init_attempts_ < kMaxInitAttempts)
+        connection_->CancelReadMessage();
+      else
+        CompleteCommand(BATTOR_ERROR_INIT_MAX_RETRIES);
+
+      return;
+
+    default:
+      CompleteCommand(BATTOR_ERROR_TIMEOUT);
+  }
+
   timeout_callback_.Cancel();
 }