[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..d4e633106b8c739398ea7b9e12fd63886f3bf718 100644
--- a/tools/battor_agent/battor_agent.cc
+++ b/tools/battor_agent/battor_agent.cc
@@ -17,12 +17,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.
+const uint8_t kMaxInitAttempts = 20;
+
+// The number of milliseconds to wait before trying to init again.
+const uint8_t kInitRetryDelayMilliseconds = 100;
+
 // The number of milliseconds to wait before trying to read a message again.
 const uint8_t kReadRetryDelayMilliseconds = 1;
 
@@ -119,7 +122,8 @@ BattOrAgent::BattOrAgent(
       listener_(listener),
       last_action_(Action::INVALID),
       command_(Command::INVALID),
-      num_read_attempts_(0) {
+      num_read_attempts_(0),
+      num_init_attempts_(0) {
   // 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();
@@ -174,11 +178,8 @@ void BattOrAgent::OnConnectionOpened(bool success) {
 
   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 +207,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 +257,21 @@ void BattOrAgent::OnMessageRead(bool success,
                                                kReadRetryDelayMilliseconds));
         return;
 
+      // Reset the timeout because there is a failed read after it is canceled.
+      case Action::SEND_INIT:

[charliea (OOO until 10-5), 2016/05/23 14:08:52]

I'm not sure I understand why this part is necessary: could you explain?

I also think it might be problematic. All of the other paths through
OnMessageRead() either end with CompleteCommand (which finishes the current
command and returns control elsewhere) or Perform[Delayed]Action, which executes
a follow-up command. This path doesn't do either, though, and seems like it's
just going to lead to the program hanging.

[aschulman, 2016/06/10 07:22:59]

You are right this is confusing, especially since the timer is reset by
PerformAction.

+        timeout_callback_.Reset(
+            base::Bind(&BattOrAgent::OnActionTimeout, AsWeakPtr()));
+        return;
+
+      // Retry sending an INIT if it an ACK is not received.
+      case Action::READ_INIT_ACK:
+        if (++num_init_attempts_ < kMaxInitAttempts)

[charliea (OOO until 10-5), 2016/05/23 14:08:51]

nit: any time that there are multiple lines inside of a scope, you need braces
around it

nit2: any time that an if() clause has braces, the else() clause also needs
braces

so this should be:

if (++num_init_attempts_ < kMaxInitAttempts) {
  ...
} else {
  ...
}

[aschulman, 2016/06/10 07:22:59]

Done.

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

[charliea (OOO until 10-5), 2016/05/23 14:08:51]

(same not about braces as above)

[aschulman, 2016/06/10 07:22:59]

Done.

+              base::TimeDelta::FromMilliseconds(kInitRetryDelayMilliseconds));
+        else
+          CompleteCommand(BATTOR_ERROR_INIT_ERROR);
         return;
       }
 
@@ -483,8 +493,21 @@ void BattOrAgent::PerformDelayedAction(Action action, base::TimeDelta delay) {
 }
 
 void BattOrAgent::OnActionTimeout() {
-  CompleteCommand(BATTOR_ERROR_TIMEOUT);
-  timeout_callback_.Cancel();
+  switch (last_action_) {

[charliea (OOO until 10-5), 2016/05/23 14:08:51]

nit: when it makes sense, Chromium prefers to "early exit" for special cases and
have the lowest indentation level by the "normal path"

that would make this:

if (last_action_ == Action::READ_INIT_ACK) {
  if (++num_init_attempts_ < kMaxInitAttempts) {
    connection_->CancelReadMessage();
    PerformAction(Action::SEND_INIT);
  } else {
    CompleteCommand(BATTOR_ERROR_INIT_ERROR);
  }

  return;
}

CompleteCommand(BATTOR_ERROR_TIMEOUT);
timeout_callback_.Cancel();

[aschulman, 2016/06/10 07:22:59]

Done.

+    case Action::READ_INIT_ACK:
+      if (++num_init_attempts_ < kMaxInitAttempts)
+      {
+        connection_->CancelReadMessage();
+        PerformAction(Action::SEND_INIT);
+      }
+      else
+        CompleteCommand(BATTOR_ERROR_INIT_ERROR);
+      return;
+
+    default:
+      CompleteCommand(BATTOR_ERROR_TIMEOUT);
+      timeout_callback_.Cancel();
+  }
 }
 
 void BattOrAgent::SendControlMessage(BattOrControlMessageType type,