[BattOr] Initialize BattOr before getting firmware git hash.

tools/battor_agent/battor_agent.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.

[charliea (OOO until 10-5), 2017/01/12 17:05:50]

nit in description: s/geting/getting

[rnephew (Reviews Here), 2017/01/12 17:53:21]

Done.

 // 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"
@@ skipping 179 matching lines @@
       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:
-      PerformAction(Action::SEND_GIT_HASH_REQUEST);
+      PerformAction(Action::SEND_INIT);

[charliea (OOO until 10-5), 2017/01/12 17:05:49]

I think that, before this line, you want num_init_attempts_ = 1 (see
START_TRACING)

[rnephew (Reviews Here), 2017/01/12 17:53:21]

Done.

       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
@@ skipping 86 matching lines @@
         return;
 
       default:
         CompleteCommand(BATTOR_ERROR_RECEIVE_ERROR);
         return;
     }
   }
 
   switch (last_action_) {
     case Action::READ_INIT_ACK:
+      if (command_ == Command::GET_FIRMWARE_GIT_HASH) {

[charliea (OOO until 10-5), 2017/01/12 17:05:50]

note that the current location of this circumvents the below check to make sure
that the ack is actually of the correct form, and the retry logic associated
with that check.

I think that any logic that we have regarding moving on to the next command
should go near where we currently SEND_SET_GAIN

[rnephew (Reviews Here), 2017/01/12 17:53:21]

Done.

+        PerformAction(Action::SEND_GIT_HASH_REQUEST);
+        return;
+      }
       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);
         }
 
         return;
       }
 
       PerformAction(Action::SEND_SET_GAIN);

[charliea (OOO until 10-5), 2017/01/12 17:05:49]

Rather than adding an if statement, how about adding a switch?

Code of the form:

if (X) {
  doNextThing();
  return;
}

Y();

suggests to me that X is the exceptional case, and we'll probably want to be
doing Y() most of the time. I'm not really sure that's the case here: there are
two equally valid cases where we want to receive the init ack (when ultimately
trying to receive the firmware hash, and when ultimately trying to start
tracing). Given that, it seems like a switch statement along the lines of:

switch (command_) {
  case Command::START_TRACING:
    Y();
    return;
  case Command::GET_FIRMWARE_GIT_HASH:
    X();
    return;
  default:
    CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
    return;
}

seems like it is probably more applicable.

[rnephew (Reviews Here), 2017/01/12 17:53:21]

Done.

       return;
 
     case Action::READ_SET_GAIN_ACK:
       if (!IsAckOfControlCommand(type, BATTOR_CONTROL_MESSAGE_TYPE_SET_GAIN,
                                  *bytes)) {
         CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
         return;
       }
 
       PerformAction(Action::SEND_START_TRACING);
@@ skipping 320 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