[BattOr] Initialize BattOr before getting firmware git hash.

[BattOr] Initialize BattOr before getting firmware git hash.

BUG=679375
Review-Url: https://codereview.chromium.org/2622003005
Cr-Commit-Position: refs/heads/master@{#443318}
Committed: https://chromium.googlesource.com/chromium/src/+/fc4c96b992fc256fe2bb8819ba6a600580b25900

[rnephew (Reviews Here), 2017-01-11 17:55:00]

rnephew@chromium.org changed reviewers:
+ charliea@chromium.org

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

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
File tools/battor_agent/battor_agent.cc (left):

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent.cc:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
nit in description: s/geting/getting

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
File tools/battor_agent/battor_agent.cc (right):

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent.cc:201: PerformAction(Action::SEND_INIT);
I think that, before this line, you want num_init_attempts_ = 1 (see
START_TRACING)

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent.cc:308: if (command_ ==
Command::GET_FIRMWARE_GIT_HASH) {
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

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent.cc:324: PerformAction(Action::SEND_SET_GAIN);
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.

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
File tools/battor_agent/battor_agent_unittest.cc (right):

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_unittest.cc:397: TEST_F(BattOrAgentTest,
StartTracingFailsIfInitSendFails) {
Seems like we probably want a similar test for getting the git hash?

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_unittest.cc:405: TEST_F(BattOrAgentTest,
StartTracingFailsIfInitAckReadFails) {
same

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_unittest.cc:419: TEST_F(BattOrAgentTest,
StartTracingFailsIfInitWrongAckRead) {
same

[rnephew (Reviews Here), 2017-01-12 17:37:06]

Description was changed from

==========
[BattOr] Initialize BattOr before geting firmware git hash.

BUG=679375
==========

to

==========
[BattOr] Initialize BattOr before getting firmware git hash.

BUG=679375
==========

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

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
File tools/battor_agent/battor_agent.cc (left):

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent.cc:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
On 2017/01/12 17:05:50, charliea wrote:
> nit in description: s/geting/getting

Done.

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
File tools/battor_agent/battor_agent.cc (right):

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent.cc:201: PerformAction(Action::SEND_INIT);
On 2017/01/12 17:05:49, charliea wrote:
> I think that, before this line, you want num_init_attempts_ = 1 (see
> START_TRACING)

Done.

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent.cc:308: if (command_ ==
Command::GET_FIRMWARE_GIT_HASH) {
On 2017/01/12 17:05:50, charliea wrote:
> 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

Done.

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent.cc:324: PerformAction(Action::SEND_SET_GAIN);
On 2017/01/12 17:05:49, charliea wrote:
> 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.

Done.

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
File tools/battor_agent/battor_agent_unittest.cc (right):

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_unittest.cc:397: TEST_F(BattOrAgentTest,
StartTracingFailsIfInitSendFails) {
On 2017/01/12 17:05:50, charliea wrote:
> Seems like we probably want a similar test for getting the git hash?

Done.

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_unittest.cc:405: TEST_F(BattOrAgentTest,
StartTracingFailsIfInitAckReadFails) {
On 2017/01/12 17:05:50, charliea wrote:
> same

Done.

https://codereview.chromium.org/2622003005/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_unittest.cc:419: TEST_F(BattOrAgentTest,
StartTracingFailsIfInitWrongAckRead) {
On 2017/01/12 17:05:50, charliea wrote:
> same

Done.

[charliea (OOO until 10-5), 2017-01-12 18:16:16]

lgtm w/ nit

https://codereview.chromium.org/2622003005/diff/40001/tools/battor_agent/batt...
File tools/battor_agent/battor_agent_unittest.cc (right):

https://codereview.chromium.org/2622003005/diff/40001/tools/battor_agent/batt...
tools/battor_agent/battor_agent_unittest.cc:1068: // Bytes will be sent becaue
INIT will be retried.
s/becaue/because

[rnephew (Reviews Here), 2017-01-12 18:17:57]

https://codereview.chromium.org/2622003005/diff/40001/tools/battor_agent/batt...
File tools/battor_agent/battor_agent_unittest.cc (right):

https://codereview.chromium.org/2622003005/diff/40001/tools/battor_agent/batt...
tools/battor_agent/battor_agent_unittest.cc:1068: // Bytes will be sent becaue
INIT will be retried.
On 2017/01/12 18:16:16, charliea wrote:
> s/becaue/because

Done.

[rnephew (Reviews Here), 2017-01-12 18:18:01]

The CQ bit was checked by rnephew@chromium.org

[rnephew (Reviews Here), 2017-01-12 18:18:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
charliea@chromium.org
Link to the patchset: https://codereview.chromium.org/2622003005/#ps60001
(title: "fix spelling")

[commit-bot: I haz the power, 2017-01-12 18:18:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 19:15:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484245081773750,
"parent_rev": "815414a7c5ce12982f36ec9c45f59e19cf003f22", "commit_rev":
"fc4c96b992fc256fe2bb8819ba6a600580b25900"}

[commit-bot: I haz the power, 2017-01-12 19:15:42]

Description was changed from

==========
[BattOr] Initialize BattOr before getting firmware git hash.

BUG=679375
==========

to

==========
[BattOr] Initialize BattOr before getting firmware git hash.

BUG=679375

Review-Url: https://codereview.chromium.org/2622003005
Cr-Commit-Position: refs/heads/master@{#443318}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc4c96b992fc256fe2bb8819ba6a...
==========

[commit-bot: I haz the power, 2017-01-12 19:15:44]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/fc4c96b992fc256fe2bb8819ba6a...