Prototype of fix for mac FTDI crash where the entire trace is read even if there is an error (e.g.,…

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 6fc6ea17983b09ed662e617b9906fc6c3dbc2f94..5b29c4994e8578e5da218bf22a7ccc28ccfaba23 100644
--- a/tools/battor_agent/battor_agent.cc
+++ b/tools/battor_agent/battor_agent.cc
@@ -5,6 +5,7 @@
 #include "tools/battor_agent/battor_agent.h"
 
 #include <iomanip>
+#include <iostream>
 
 #include "base/bind.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -23,12 +24,6 @@ const uint8_t kMaxInitAttempts = 20;
 // The number of milliseconds to wait before trying to init again.
 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 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;
@@ -91,12 +86,8 @@ bool ParseSampleFrame(BattOrMessageType type,
   memcpy(frame_header, frame_ptr, sizeof(BattOrFrameHeader));
   frame_ptr += sizeof(BattOrFrameHeader);
 
-  if (frame_header->sequence_number != expected_sequence_number) {
-    LOG(WARNING) << "Unexpected sequence number: wanted "
-                 << expected_sequence_number << ", but got "
-                 << frame_header->sequence_number << ".";
+  if (frame_header->sequence_number != expected_sequence_number)
     return false;
-  }
 
   size_t remaining_bytes = msg.size() - sizeof(BattOrFrameHeader);
   if (remaining_bytes != frame_header->length)
@@ -122,8 +113,7 @@ BattOrAgent::BattOrAgent(
       listener_(listener),
       last_action_(Action::INVALID),
       command_(Command::INVALID),
-      num_init_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();
@@ -217,15 +207,12 @@ void BattOrAgent::OnBytesSent(bool success) {
       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;
     default:
@@ -245,18 +232,13 @@ void BattOrAgent::OnMessageRead(bool success,
   if (!success) {
     switch (last_action_) {
       case Action::READ_EEPROM:
+      case Action::READ_CURRENT_SAMPLE:
+        CompleteCommand(BATTOR_ERROR_RECEIVE_ERROR);
+        return;
       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));
+        CompleteCommand(BATTOR_ERROR_RECEIVE_ERROR);
         return;
-
       // Retry sending an INIT if it an ACK is not received.
       case Action::SEND_INIT:
       case Action::READ_INIT_ACK:
@@ -335,17 +317,16 @@ void BattOrAgent::OnMessageRead(bool success,
       BattOrFrameHeader frame_header;
       if (!ParseSampleFrame(type, *bytes, next_sequence_number_++,
                             &frame_header, &calibration_frame_)) {
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
-        return;
-      }
-
-      // Make sure that the calibration frame has actual samples in it.
-      if (calibration_frame_.empty()) {
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
-        return;
+        if (read_frame_error_ == BATTOR_ERROR_NONE)
+          read_frame_error_ = BATTOR_ERROR_UNEXPECTED_MESSAGE;
+      } else {
+        // Make sure that the calibration frame has actual samples in it.
+        if (calibration_frame_.empty()) {
+          if (read_frame_error_ == BATTOR_ERROR_NONE)
+            read_frame_error_ = BATTOR_ERROR_UNEXPECTED_MESSAGE;
+        }
       }
 
-      num_read_attempts_ = 1;
       PerformAction(Action::READ_DATA_FRAME);
       return;
     }
@@ -355,20 +336,19 @@ void BattOrAgent::OnMessageRead(bool success,
       vector<RawBattOrSample> frame;
       if (!ParseSampleFrame(type, *bytes, next_sequence_number_++,
                             &frame_header, &frame)) {
-        CompleteCommand(BATTOR_ERROR_UNEXPECTED_MESSAGE);
-        return;
-      }
+        if (read_frame_error_ == BATTOR_ERROR_NONE)
+          read_frame_error_ = BATTOR_ERROR_UNEXPECTED_MESSAGE;
+      } else {
+        // Check for the empty frame the BattOr uses to indicate it's done
+        // streaming samples.
+        if (frame.empty()) {
+          CompleteCommand(BATTOR_ERROR_NONE);
+          return;
+        }
 
-      // Check for the empty frame the BattOr uses to indicate it's done
-      // streaming samples.
-      if (frame.empty()) {
-        CompleteCommand(BATTOR_ERROR_NONE);
-        return;
+        samples_.insert(samples_.end(), frame.begin(), frame.end());
       }
 
-      samples_.insert(samples_.end(), frame.begin(), frame.end());
-
-      num_read_attempts_ = 1;
       PerformAction(Action::READ_DATA_FRAME);
       return;
     }
@@ -454,6 +434,9 @@ void BattOrAgent::PerformAction(Action action) {
       // 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;
+
+      // Initialize error that could be encountered while reading data.
+      read_frame_error_ = BATTOR_ERROR_NONE;
     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
@@ -493,7 +476,14 @@ void BattOrAgent::OnActionTimeout() {
       } else {
         CompleteCommand(BATTOR_ERROR_TOO_MANY_INIT_RETRIES);
       }
-
+      return;
+    case Action::READ_CALIBRATION_FRAME:
+    case Action::READ_DATA_FRAME:
+      // Fail with the first frame read error error if one was encountered.
+      if (read_frame_error_ != BATTOR_ERROR_NONE)
+        CompleteCommand(read_frame_error_);
+      else
+        CompleteCommand(BATTOR_ERROR_TIMEOUT);
       return;
 
     default:
@@ -520,6 +510,9 @@ void BattOrAgent::CompleteCommand(BattOrError error) {
                                 base::Unretained(listener_), error));
       break;
     case Command::STOP_TRACING:
+      if (read_frame_error_ != BATTOR_ERROR_NONE)
+        error = read_frame_error_;
+
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE,
           base::Bind(&Listener::OnStopTracingComplete,