[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 <iostream>
 
 #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.

[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;
 
 // 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;
+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.
 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 67 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) {

[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();
 }
 
 BattOrAgent::~BattOrAgent() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 void BattOrAgent::StartTracing() {
@@ skipping 32 matching lines @@
   // Return immediately if opening the connection already timed out.
   if (timeout_callback_.IsCancelled())
     return;
   timeout_callback_.Cancel();
 
   if (!success) {
     CompleteCommand(BATTOR_ERROR_CONNECTION_FAILED);
     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);
       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;
 
+      // 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;
     }
   }
 
   switch (last_action_) {
     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);
       return;
 
     case Action::READ_SET_GAIN_ACK:
       if (!IsAckOfControlCommand(type, BATTOR_CONTROL_MESSAGE_TYPE_SET_GAIN,
                                  *bytes)) {
         CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
         return;
@@ skipping 102 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:
-      // 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);
       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.
@@ skipping 54 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);
+  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();
 }
 
 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 64 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