[BattOr] Retry battor_agent commands when they fail.

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 maximum number of times to retry when initializing a BattOr.
-const uint8_t kMaxInitAttempts = 20;
-
-// The maximum number of times to retry the StartTracing command.
-const uint8_t kMaxStartTracingAttempts = 5;
-
-// The number of milliseconds to wait before retrying initialization.
-const uint16_t kInitRetryDelayMilliseconds = 100;
-
-// The maximum number of times to retry when reading a message.
-const uint8_t kMaxReadAttempts = 20;
-
-// The number of milliseconds to wait before trying to read a message again.
-const uint8_t kReadRetryDelayMilliseconds = 1;
+// The maximum number of times to retry a command.
+const uint8_t kMaxCommandAttempts = 10;

[rnephew (Reviews Here), 2017/05/09 15:04:05]

Was 20 chosen for a reason for kMaxInitAttempts/kMaxReadAttempts before Charlie?
I'm guessing it was just a best guess, so moving it down to ten shouldn't be a
big deal.

[charliea (OOO until 10-5), 2017/05/09 19:58:08]

I haven't seen this code in a long, long time...

Looking back at the blame, it looks like Aaron wrote the initial retry logic, so
I defer to him here :-)

[aschulman, 2017/05/11 13:13:00]

20 was due to the super short retry interval. With the much longer retry
interval we need fewer retries. 10 gave a very low failure rate in my testing,
so it seemed like a good choice for now.

 
 // 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 = 4;
+// The number of seconds to wait before retrying a command.
+const uint16_t kCommandRetryDelaySeconds = 2;

[rnephew (Reviews Here), 2017/04/21 22:10:45]

These delays seem to be much larger than the previous
delays(InitRetryDelayMilliseconds = 100). Is it necessary for it to be so long?

[aschulman, 2017/05/11 13:12:59]

Yeah the 100 msec retry interval was too aggressive. Many INIT messages were
being dropped.

+
+// The number of seconds allowed for a control message before timing out.
+const uint8_t kBattOrControlMessageTimeoutSeconds = 2;
+
+// The number of seconds allowed for connection to open before timing out.
+const uint8_t kBattOrConnectionTimeoutSeconds = 4;
 
 // Returns true if the specified vector of bytes decodes to a message that is an
 // ack for the specified control message type.
 bool IsAckOfControlCommand(BattOrMessageType message_type,
                            BattOrControlMessageType control_message_type,
                            const vector<char>& msg) {
   if (message_type != BATTOR_MESSAGE_TYPE_CONTROL_ACK)
     return false;
 
   if (msg.size() != sizeof(BattOrControlMessageAck))
@@ skipping 66 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_init_attempts_(0),
-      num_start_tracing_attempts_(0),
-      num_read_attempts_(0) {
+      num_command_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() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // When tracing is restarted, all previous clock sync markers are invalid.
   clock_sync_markers_.clear();
   last_clock_sync_time_ = base::TimeTicks();
 
-  num_start_tracing_attempts_ = 1;
   command_ = Command::START_TRACING;
   PerformAction(Action::REQUEST_CONNECTION);
 }
 
 void BattOrAgent::StopTracing() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   command_ = Command::STOP_TRACING;
   PerformAction(Action::REQUEST_CONNECTION);
 }
 
 void BattOrAgent::RecordClockSyncMarker(const std::string& marker) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   command_ = Command::RECORD_CLOCK_SYNC_MARKER;
   pending_clock_sync_marker_ = marker;
   PerformAction(Action::REQUEST_CONNECTION);
 }
 
 void BattOrAgent::GetFirmwareGitHash() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   command_ = Command::GET_FIRMWARE_GIT_HASH;
   PerformAction(Action::REQUEST_CONNECTION);
 }
 
 void BattOrAgent::BeginConnect() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  // Start the timeout timer for opening the connection.
+  timeout_callback_.Reset(
+      base::Bind(&BattOrAgent::OnActionTimeout, AsWeakPtr()));
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, timeout_callback_.callback(),
+      base::TimeDelta::FromSeconds(kBattOrConnectionTimeoutSeconds));
+
   connection_->Open();
 }
 
 void BattOrAgent::OnConnectionOpened(bool success) {
-  // Return immediately if opening the connection already timed out.
-  if (timeout_callback_.IsCancelled())
-    return;
+  // Cancel timeout because the connection was opened in time.
   timeout_callback_.Cancel();
 
   if (!success) {
     CompleteCommand(BATTOR_ERROR_CONNECTION_FAILED);
     return;
   }
 
   switch (command_) {
     case Command::START_TRACING:
-      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::GET_FIRMWARE_GIT_HASH:
-      num_init_attempts_ = 1;
       PerformAction(Action::SEND_INIT);
       return;
     case Command::INVALID:
       NOTREACHED();
+      return;
   }
 }
 
 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_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:
-      num_read_attempts_ = 1;
       PerformAction(Action::READ_EEPROM);
       return;
     case Action::SEND_SAMPLES_REQUEST:
-      num_read_attempts_ = 1;
       PerformAction(Action::READ_CALIBRATION_FRAME);
       return;
     case Action::SEND_CURRENT_SAMPLE_REQUEST:
-      num_read_attempts_ = 1;
       PerformAction(Action::READ_CURRENT_SAMPLE);
       return;
     case Action::SEND_GIT_HASH_REQUEST:
-      num_read_attempts_ = 1;
       PerformAction(Action::READ_GIT_HASH);
       return;
     default:
-      CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
+      NOTREACHED();
+      return;
   }
 }
 
 void BattOrAgent::OnMessageRead(bool success,
                                 BattOrMessageType type,
                                 std::unique_ptr<vector<char>> bytes) {
-  // Return immediately if whatever action we were trying to perform already
-  // timed out.
-  if (timeout_callback_.IsCancelled())
-    return;
-  timeout_callback_.Cancel();
-
   if (!success) {
     switch (last_action_) {
       case Action::READ_GIT_HASH:
+      case Action::READ_INIT_ACK:
+      case Action::READ_SET_GAIN_ACK:
+      case Action::READ_START_TRACING_ACK:
       case Action::READ_EEPROM:
       case Action::READ_CALIBRATION_FRAME:
       case Action::READ_DATA_FRAME:
-      case Action::READ_CURRENT_SAMPLE:
-        if (num_read_attempts_++ > kMaxReadAttempts) {
-          CompleteCommand(BATTOR_ERROR_RECEIVE_ERROR);
-          return;
-        }
-
-        PerformDelayedAction(last_action_, base::TimeDelta::FromMilliseconds(
-                                               kReadRetryDelayMilliseconds));
+        RetryCommand();
         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) {
-          PerformDelayedAction(Action::SEND_INIT,
-              base::TimeDelta::FromMilliseconds(kInitRetryDelayMilliseconds));
-        } else {
-          CompleteCommand(BATTOR_ERROR_TOO_MANY_INIT_RETRIES);
-        }
-
-        return;
-
-      case Action::READ_START_TRACING_ACK:
-        if (num_start_tracing_attempts_++ < kMaxStartTracingAttempts) {
-          num_init_attempts_ = 1;
-          PerformAction(Action::SEND_INIT);
-        } else {
-          CompleteCommand(BATTOR_ERROR_TOO_MANY_START_TRACING_RETRIES);
-        }
-
+      case Action::READ_CURRENT_SAMPLE:
+        CompleteCommand(BATTOR_ERROR_RECEIVE_ERROR);
         return;
 
       default:
-        CompleteCommand(BATTOR_ERROR_RECEIVE_ERROR);
+        NOTREACHED();
         return;
     }
   }
 
+  // Successfully read a message, cancel any timeouts.
+  timeout_callback_.Cancel();
+
   switch (last_action_) {
     case Action::READ_INIT_ACK:
       if (!IsAckOfControlCommand(type, BATTOR_CONTROL_MESSAGE_TYPE_INIT,
                                  *bytes)) {
-        if (num_init_attempts_++ < kMaxInitAttempts) {
-          PerformDelayedAction(Action::SEND_INIT,
-              base::TimeDelta::FromMilliseconds(kInitRetryDelayMilliseconds));
-        } else {
-          CompleteCommand(BATTOR_ERROR_TOO_MANY_INIT_RETRIES);
-        }
-
+        RetryCommand();
         return;
       }
 
       switch (command_) {
         case Command::START_TRACING:
           PerformAction(Action::SEND_SET_GAIN);
           return;
         case Command::GET_FIRMWARE_GIT_HASH:
           PerformAction(Action::SEND_GIT_HASH_REQUEST);
           return;
         default:
-          CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
+          NOTREACHED();
           return;
       }
 
     case Action::READ_SET_GAIN_ACK:
       if (!IsAckOfControlCommand(type, BATTOR_CONTROL_MESSAGE_TYPE_SET_GAIN,
                                  *bytes)) {
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
+        RetryCommand();
         return;
       }
 
       PerformAction(Action::SEND_START_TRACING);
       return;
 
     case Action::READ_START_TRACING_ACK:
       if (!IsAckOfControlCommand(
               type, BATTOR_CONTROL_MESSAGE_TYPE_START_SAMPLING_SD, *bytes)) {
-        if (num_start_tracing_attempts_++ < kMaxStartTracingAttempts) {
-          num_init_attempts_ = 1;
-          PerformAction(Action::SEND_INIT);
-        } else {
-          CompleteCommand(BATTOR_ERROR_TOO_MANY_START_TRACING_RETRIES);
-        }
-
+        RetryCommand();
         return;
       }
 
       CompleteCommand(BATTOR_ERROR_NONE);
       return;
 
     case Action::READ_EEPROM: {
       battor_eeprom_ = ParseEEPROM(type, *bytes);
       if (!battor_eeprom_) {
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
+        RetryCommand();
         return;
       }
 
       // Make sure that we don't request samples until a safe amount of time has
       // elapsed since recording the last clock sync marker: we need to ensure
       // that the sample we synced to doesn't get thrown out.
       base::TimeTicks min_request_samples_time =
           last_clock_sync_time_ + base::TimeDelta::FromMilliseconds(
                                       kStopTracingClockSyncDelayMilliseconds);
       base::TimeDelta request_samples_delay = std::max(
           min_request_samples_time - base::TimeTicks::Now(), base::TimeDelta());
 
       PerformDelayedAction(Action::SEND_SAMPLES_REQUEST, request_samples_delay);
       return;
     }
     case Action::READ_CALIBRATION_FRAME: {
       BattOrFrameHeader frame_header;
       if (!ParseSampleFrame(type, *bytes, next_sequence_number_++,
                             &frame_header, &calibration_frame_)) {
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
+        RetryCommand();
         return;
       }
 
       // Make sure that the calibration frame has actual samples in it.
       if (calibration_frame_.empty()) {
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
+        RetryCommand();
         return;
       }
 
-      num_read_attempts_ = 1;
       PerformAction(Action::READ_DATA_FRAME);
       return;
     }
 
     case Action::READ_DATA_FRAME: {
       BattOrFrameHeader frame_header;
       vector<RawBattOrSample> frame;
       if (!ParseSampleFrame(type, *bytes, next_sequence_number_++,
                             &frame_header, &frame)) {
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
+        RetryCommand();
         return;
       }
 
       // Check for the empty frame the BattOr uses to indicate it's done
       // streaming samples.
       if (frame.empty()) {
+        // Cancel the next data frame timeout.
+        timeout_callback_.Cancel();
         CompleteCommand(BATTOR_ERROR_NONE);
         return;
       }
 
       samples_.insert(samples_.end(), frame.begin(), frame.end());
 
-      num_read_attempts_ = 1;
       PerformAction(Action::READ_DATA_FRAME);
       return;
     }
 
     case Action::READ_CURRENT_SAMPLE:
       if (type != BATTOR_MESSAGE_TYPE_CONTROL_ACK ||
           bytes->size() != sizeof(uint32_t)) {
         CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
         return;
       }
 
       uint32_t sample_num;
       memcpy(&sample_num, bytes->data(), sizeof(uint32_t));
       clock_sync_markers_[sample_num] = pending_clock_sync_marker_;
       last_clock_sync_time_ = base::TimeTicks::Now();
       CompleteCommand(BATTOR_ERROR_NONE);
       return;
 
     case Action::READ_GIT_HASH:
-      if (type != BATTOR_MESSAGE_TYPE_CONTROL_ACK ){
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
+      if (type != BATTOR_MESSAGE_TYPE_CONTROL_ACK) {
+        RetryCommand();
         return;
       }
+
       firmware_git_hash_ = std::string(bytes->begin(), bytes->end());
       CompleteCommand(BATTOR_ERROR_NONE);
       return;
 
     default:
-      CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
+      NOTREACHED();
+      return;
   }
 }
 
 void BattOrAgent::PerformAction(Action action) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  timeout_callback_.Reset(
-      base::Bind(&BattOrAgent::OnActionTimeout, AsWeakPtr()));
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, timeout_callback_.callback(),
-      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_INIT:
-      // Clear out the serial data that may exist from prior init attempts.
-      connection_->Flush();
-
       SendControlMessage(BATTOR_CONTROL_MESSAGE_TYPE_INIT, 0, 0);
       return;
     case Action::READ_INIT_ACK:
       connection_->ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK);
       return;
     case Action::SEND_SET_GAIN:
       // Set the BattOr's gain. Setting the gain tells the BattOr the range of
       // power measurements that we expect to see.
       SendControlMessage(BATTOR_CONTROL_MESSAGE_TYPE_SET_GAIN, BATTOR_GAIN_LOW,
                          0);
       return;
     case Action::READ_SET_GAIN_ACK:
       connection_->ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK);
       return;
     case Action::SEND_START_TRACING:
       SendControlMessage(BATTOR_CONTROL_MESSAGE_TYPE_START_SAMPLING_SD, 0, 0);
       return;
     case Action::READ_START_TRACING_ACK:
       connection_->ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK);
       return;
-
     // The following actions are required for StopTracing:
     case Action::SEND_EEPROM_REQUEST:
       // Read the BattOr's EEPROM to get calibration information that's required
       // to convert the raw samples to accurate ones.
       SendControlMessage(BATTOR_CONTROL_MESSAGE_TYPE_READ_EEPROM,
                          sizeof(BattOrEEPROM), 0);
       return;
     case Action::READ_EEPROM:
       connection_->ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK);
       return;
     case Action::SEND_SAMPLES_REQUEST:
       // Send a request to the BattOr to tell it to start streaming the samples
       // that it's stored on its SD card over the serial connection.
       SendControlMessage(BATTOR_CONTROL_MESSAGE_TYPE_READ_SD_UART, 0, 0);
       return;
     case Action::READ_CALIBRATION_FRAME:
       // Data frames are numbered starting at zero and counting up by one each
       // data frame. We keep track of the next frame sequence number we expect
       // to see to ensure we don't miss any data.
       next_sequence_number_ = 0;
+
+      // Clear stored samples from prior attempts to read sample frames.
+      samples_.clear();
+      calibration_frame_.clear();
     case Action::READ_DATA_FRAME:
       // The first frame sent back from the BattOr contains voltage and current
       // data that excludes whatever device is being measured from the
       // circuit. We use this first frame to establish a baseline voltage and
       // current.
       //
       // All further frames contain real (but uncalibrated) voltage and current
       // data.
+
+      // Start the next data frame timeout timer.
+      timeout_callback_.Reset(
+          base::Bind(&BattOrAgent::OnActionTimeout, AsWeakPtr()));
+      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE, timeout_callback_.callback(),
+          base::TimeDelta::FromSeconds(kBattOrControlMessageTimeoutSeconds));

[charliea (OOO until 10-5), 2017/05/12 22:03:42]

Could we move the timeout_callback_.Reset() and the PostDelayedTask into a
separate method, SetActionTimeout(seconds)? That would allow us to get rid of
your comment above, which I agree is currently necessary because the code itself
isn't self-documenting.

[aschulman, 2017/05/15 18:49:16]

Done.

+
       connection_->ReadMessage(BATTOR_MESSAGE_TYPE_SAMPLES);
       return;
 
     // The following actions are required for RecordClockSyncMarker:
     case Action::SEND_CURRENT_SAMPLE_REQUEST:
       SendControlMessage(BATTOR_CONTROL_MESSAGE_TYPE_READ_SAMPLE_COUNT, 0, 0);
       return;
     case Action::READ_CURRENT_SAMPLE:
       connection_->ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK);
       return;
 
     case Action::SEND_GIT_HASH_REQUEST:
-      connection_->Flush();

[rnephew (Reviews Here), 2017/05/09 15:04:05]

Do we not need to flush the connections now with these changes?

See comment in: tools/battor_agent/battor_connection_impl.cc

[aschulman, 2017/05/11 13:13:00]

see reply in: : tools/battor_agent/battor_connection_impl.cc

       SendControlMessage(
           BATTOR_CONTROL_MESSAGE_TYPE_GET_FIRMWARE_GIT_HASH, 0, 0);
       return;
 
     case Action::READ_GIT_HASH:
       connection_->ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK);
       return;
 
     case Action::INVALID:
       NOTREACHED();
+      return;
   }
 }
 
 void BattOrAgent::PerformDelayedAction(Action action, base::TimeDelta delay) {
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, base::Bind(&BattOrAgent::PerformAction, AsWeakPtr(), action),
       delay);
 }
 
 void BattOrAgent::OnActionTimeout() {
   switch (last_action_) {
     case Action::READ_INIT_ACK:
-      if (num_init_attempts_++ < kMaxInitAttempts) {
-        // OnMessageRead() will fail and retry SEND_INIT.
-        connection_->CancelReadMessage();
-      } else {
-        CompleteCommand(BATTOR_ERROR_TOO_MANY_INIT_RETRIES);
-      }
-      return;
-
-    // TODO(crbug.com/672631): There's currently a BattOr firmware bug that's
-    // causing the BattOr to reset when it's sent the START_TRACING command.
-    // When the BattOr resets, it emits 0x00 to the serial connection. This 0x00
-    // isn't long enough for the connection to consider it a full ack of the
-    // START_TRACING command, so it continues to wait for more data. We handle
-    // this case here by assuming any timeouts while waiting for the
-    // StartTracing ack are related to this bug and retrying the full
-    // initialization sequence.
+    case Action::READ_SET_GAIN_ACK:
     case Action::READ_START_TRACING_ACK:
-      if (num_start_tracing_attempts_ < kMaxStartTracingAttempts) {
-        // OnMessageRead() will fail and retry StartTracing.
-        connection_->CancelReadMessage();
-      } else {
-        CompleteCommand(BATTOR_ERROR_TOO_MANY_START_TRACING_RETRIES);
-      }
+    case Action::READ_EEPROM:
+    case Action::READ_CALIBRATION_FRAME:
+    case Action::READ_DATA_FRAME:
+    case Action::READ_GIT_HASH:
+      connection_->CancelReadMessage();
       return;
 
     default:
       CompleteCommand(BATTOR_ERROR_TIMEOUT);
       timeout_callback_.Cancel();
   }
 }
 
 void BattOrAgent::SendControlMessage(BattOrControlMessageType type,
                                      uint16_t param1,
                                      uint16_t param2) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  // Start control message respone timeout timer.

[charliea (OOO until 10-5), 2017/05/12 22:03:42]

s/respone/response, but see above comment about factoring this out

[aschulman, 2017/05/15 18:49:16]

Done.

+  timeout_callback_.Reset(
+      base::Bind(&BattOrAgent::OnActionTimeout, AsWeakPtr()));
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, timeout_callback_.callback(),
+      base::TimeDelta::FromSeconds(kBattOrControlMessageTimeoutSeconds));
+
   BattOrControlMessage msg{type, param1, param2};
   connection_->SendBytes(BATTOR_MESSAGE_TYPE_CONTROL, &msg, sizeof(msg));
 }
 
+void BattOrAgent::RetryCommand() {
+  if (++num_command_attempts_ >= kMaxCommandAttempts) {
+    CompleteCommand(BATTOR_ERROR_TOO_MANY_COMMAND_RETRIES);
+    return;
+  }
+
+  // Failed to read response to message, retry current command.
+  switch (command_) {
+    case Command::START_TRACING:
+      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

[charliea (OOO until 10-5), 2017/05/12 22:03:42]

One thing that might make this a little cleaner is if you story the task you
want to post in a closure and just post the delayed task after the switch. I
think you can do:

base::Callback<void(int)> next_command;
switch(command_) {
  case Command::START_TRACING:
    next_command = base::Bind(&BattOrAgent::StartTracing, AsWeakPtr());
    return;
  case Command::STOP_TRACING:
    next_command = base::Bind(&BattOrAgent::StopTracing, AsWeakPtr());
    return;
  ...
}

base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
    next_command, base::TimeDelta::FromSeconds(kCommandRetryDelaySeconds);

[aschulman, 2017/05/15 18:49:16]

Done.

+          FROM_HERE, base::Bind(&BattOrAgent::StartTracing, AsWeakPtr()),
+          base::TimeDelta::FromSeconds(kCommandRetryDelaySeconds));
+      return;
+    case Command::STOP_TRACING:
+      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE, base::Bind(&BattOrAgent::StopTracing, AsWeakPtr()),
+          base::TimeDelta::FromSeconds(kCommandRetryDelaySeconds));
+      return;
+    case Command::GET_FIRMWARE_GIT_HASH:
+      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE, base::Bind(&BattOrAgent::GetFirmwareGitHash, AsWeakPtr()),
+          base::TimeDelta::FromSeconds(kCommandRetryDelaySeconds));
+      return;
+    default:
+      NOTREACHED();
+      return;
+  }
+}
+
 void BattOrAgent::CompleteCommand(BattOrError error) {
   switch (command_) {
     case Command::START_TRACING:
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::Bind(&Listener::OnStartTracingComplete,
                                 base::Unretained(listener_), error));
       break;
     case Command::STOP_TRACING:
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE,
           base::Bind(&Listener::OnStopTracingComplete,
                      base::Unretained(listener_), SamplesToString(), error));
       break;
     case Command::RECORD_CLOCK_SYNC_MARKER:
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::Bind(&Listener::OnRecordClockSyncMarkerComplete,
                                 base::Unretained(listener_), error));
       break;
     case Command::GET_FIRMWARE_GIT_HASH:
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::Bind(&Listener::OnGetFirmwareGitHashComplete,
                                 base::Unretained(listener_),
                                 firmware_git_hash_, error));
       break;
     case Command::INVALID:
       NOTREACHED();
+      return;
   }
 
   last_action_ = Action::INVALID;
   command_ = Command::INVALID;
   pending_clock_sync_marker_.clear();
   battor_eeprom_.reset();
   calibration_frame_.clear();
   samples_.clear();
   next_sequence_number_ = 0;
+  num_command_attempts_ = 0;
 }
 
 std::string BattOrAgent::SamplesToString() {
   if (calibration_frame_.empty() || samples_.empty() || !battor_eeprom_)
     return "";
 
   BattOrSampleConverter converter(*battor_eeprom_, calibration_frame_);
 
   std::stringstream trace_stream;
   trace_stream << std::fixed;
@@ skipping 23 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