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

tools/battor_agent/battor_agent.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_agent.h"
 
 #include <iomanip>
 
 #include "base/bind.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "tools/battor_agent/battor_connection_impl.h"
 #include "tools/battor_agent/battor_sample_converter.h"
 
 using std::vector;
 
 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;
 
 // The amount of time we need to wait after recording a clock sync marker in
 // order to ensure that the sample we synced to doesn't get thrown out.
 const uint8_t kStopTracingClockSyncDelayMilliseconds = 100;
 
 // The number of seconds allowed for a given action before timing out.
 const uint8_t kBattOrTimeoutSeconds = 10;
 
@@ skipping 76 matching lines @@
     Listener* listener,
     scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> ui_thread_task_runner)
     : connection_(new BattOrConnectionImpl(path,
                                            this,
                                            file_thread_task_runner,
                                            ui_thread_task_runner)),
       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();
 }
 
 BattOrAgent::~BattOrAgent() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 void BattOrAgent::StartTracing() {
@@ skipping 34 matching lines @@
     return;
   timeout_callback_.Cancel();
 
   if (!success) {
     CompleteCommand(BATTOR_ERROR_CONNECTION_FAILED);
     return;
   }
 
   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);
       return;
     case Command::RECORD_CLOCK_SYNC_MARKER:
       PerformAction(Action::SEND_CURRENT_SAMPLE_REQUEST);
       return;
     case Command::INVALID:
       NOTREACHED();
   }
 }
 
 void BattOrAgent::OnBytesSent(bool success) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Return immediately if whatever action we were trying to perform already
   // timed out.
   if (timeout_callback_.IsCancelled())
     return;
   timeout_callback_.Cancel();
 
   if (!success) {
     CompleteCommand(BATTOR_ERROR_SEND_ERROR);
     return;
   }
 
   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;
     case Action::SEND_SET_GAIN:
       PerformAction(Action::READ_SET_GAIN_ACK);
       return;
     case Action::SEND_START_TRACING:
       PerformAction(Action::READ_START_TRACING_ACK);
       return;
     case Action::SEND_EEPROM_REQUEST:
@@ skipping 30 matching lines @@
       case Action::READ_CURRENT_SAMPLE:
         if (++num_read_attempts_ > kMaxReadAttempts) {
           CompleteCommand(BATTOR_ERROR_RECEIVE_ERROR);
           return;
         }
 
         PerformDelayedAction(last_action_, base::TimeDelta::FromMilliseconds(
                                                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;
     }
   }
 
   switch (last_action_) {
     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;
       }
 
       PerformAction(Action::SEND_SET_GAIN);
       return;
 
     case Action::READ_SET_GAIN_ACK:
       if (!IsAckOfControlCommand(type, BATTOR_CONTROL_MESSAGE_TYPE_SET_GAIN,
                                  *bytes)) {
         CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
@@ skipping 103 matching lines @@
       base::TimeDelta::FromSeconds(kBattOrTimeoutSeconds));
 
   last_action_ = action;
 
   switch (action) {
     case Action::REQUEST_CONNECTION:
       BeginConnect();
       return;
 
     // The following actions are required for StartTracing:
     case Action::SEND_RESET:

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

This can be deleted

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

Done.

       // 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.
       connection_->Flush();
 
@@ skipping 62 matching lines @@
   }
 }
 
 void BattOrAgent::PerformDelayedAction(Action action, base::TimeDelta delay) {
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, base::Bind(&BattOrAgent::PerformAction, AsWeakPtr(), action),
       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,
                                      uint16_t param1,
                                      uint16_t param2) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   BattOrControlMessage msg{type, param1, param2};
   connection_->SendBytes(BATTOR_MESSAGE_TYPE_CONTROL, &msg, sizeof(msg));
 }
@@ skipping 63 matching lines @@
     if (clock_sync_marker != clock_sync_markers_.end())
       trace_stream << " <" << clock_sync_marker->second << ">";
 
     trace_stream << std::endl;
   }
 
   return trace_stream.str();
 }
 
 }  // namespace battor