tools/battor_agent: Changes the BattOr Agent binary to be interactive

tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com 's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

Does this protocol sound reasonable?

BUG=542837
Committed: https://crrev.com/879caf4d9679a07b6dae42ec6cc09a719b5046c5
Cr-Commit-Position: refs/heads/master@{#378004}

[charliea (OOO until 10-5), 2016-02-24 16:23:44]

Description was changed from

==========
tools/battor_agent: Changes the BattOr Agent binary to be a shell

This is a follow-up to nednguyen@google.com's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

BUG=542837
==========

to

==========
tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

BUG=542837
==========

[charliea (OOO until 10-5), 2016-02-24 16:23:56]

Description was changed from

==========
tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

BUG=542837
==========

to

==========
tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com 's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

BUG=542837
==========

[charliea (OOO until 10-5), 2016-02-24 16:25:02]

Description was changed from

==========
tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com 's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

BUG=542837
==========

to

==========
tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com 's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

Does this protocol sound reasonable?

BUG=542837
==========

[charliea (OOO until 10-5), 2016-02-24 16:25:03]

charliea@chromium.org changed reviewers:
+ alexandermont@chromium.org, nednguyen@google.com, zhenw@chromium.org

[nednguyen, 2016-02-24 20:17:21]

On 2016/02/24 19:28:24, charliea wrote:

The protocol makes sense to me. Though I think we should add documentation of
the protocol to the PrintUsage().

lgtm but please wait for other reviewers' stamp.

[alexandermont, 2016-02-24 22:01:48]

The protocol sounds reasonable to me.

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

https://codereview.chromium.org/1732943002/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_bin.cc:89: std::string path =
BattOrFinder::FindBattOr();
Where is the "battor-path" switch actually read? It looks like it tries to find
a path using FindBattOr, then uses that path in the SetUp, and doesn't seem to
use the path that you passed in on battor-path. Is that somewhere else in the
code, not in this file?

https://codereview.chromium.org/1732943002/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_bin.cc:98: for (;;) {
maybe while(true) instead?

https://codereview.chromium.org/1732943002/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_bin.cc:103: } else if (cmd == "StopTracing") {
documentation should mention that StopTracing makes the binary exit

[Zhen Wang, 2016-02-24 22:35:45]

lgtm

I think the "Telemetry" in the protocol should be more specific, i.e.,
PowerTracingAgent in Telemetry/systrace/etc.

Agree with Ned to document the protocol, e.g., in usage.

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

https://codereview.chromium.org/1732943002/diff/1/tools/battor_agent/battor_a...
tools/battor_agent/battor_agent_bin.cc:89: std::string path =
BattOrFinder::FindBattOr();
On 2016/02/24 22:01:48, alexandermont wrote:
> Where is the "battor-path" switch actually read? It looks like it tries to
find
> a path using FindBattOr, then uses that path in the SetUp, and doesn't seem to
> use the path that you passed in on battor-path. Is that somewhere else in the
> code, not in this file?

Yes, it is done in FindBattOr. base::CommandLine::Init() called in main()
initialized the command line switches.

[alexandermont, 2016-02-25 23:37:22]

lgtm

[charliea (OOO until 10-5), 2016-02-26 21:33:20]

On 2016/02/24 20:17:21, nednguyen wrote:
> On 2016/02/24 19:28:24, charliea wrote:
> 
> The protocol makes sense to me. Though I think we should add documentation of
> the protocol to the PrintUsage().
> 
> lgtm but please wait for other reviewers' stamp.

Good idea. I ultimately added it to the file header instead of the PrintUsage
command, though. Given its length, I figured PrintUsage should stay as short as
possible so that when someone is in the shell and types "Help", they don't get
spammed with 10 bullets about how to integrate this with a tracing architecture.

[charliea (OOO until 10-5), 2016-02-26 21:35:32]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2016-02-26 21:35:33]

The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org,
nednguyen@google.com, alexandermont@chromium.org
Link to the patchset: https://codereview.chromium.org/1732943002/#ps20001
(title: "Documented usage")

[commit-bot: I haz the power, 2016-02-26 21:38:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1732943002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1732943002/20001

[commit-bot: I haz the power, 2016-02-26 22:24:51]

Description was changed from

==========
tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com 's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

Does this protocol sound reasonable?

BUG=542837
==========

to

==========
tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com 's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

Does this protocol sound reasonable?

BUG=542837
==========

[commit-bot: I haz the power, 2016-02-26 22:24:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-26 22:26:23]

Description was changed from

==========
tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com 's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

Does this protocol sound reasonable?

BUG=542837
==========

to

==========
tools/battor_agent: Changes the BattOr Agent binary to be interactive

This is a follow-up to nednguyen@google.com 's proof-of-concept patch
(http://crrev.com/1726353002) that demonstrated that communicating with
the BattOr agent via standard input and output would be fast enough to
perform accurate clock syncs.

This is the interface that Telemetry and systrace will be using to
communicating with the BattOr, so it's important that we make sure this
works for them.

My imagined usage is:

  1) Telemetry is told to start a trace
  2) Telemetry opens up a BattOr agent binary subprocess
  3) Telemetry sends the subprocess the "StartTracing" message via STDIN
  4) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  5) If the last command was successful, Telemetry waits for the
     duration of the trace
  6) When the tracing should end, Telemetry sends the subprocess the
     "RecordClockSyncMark <marker>" message.
  7) Telemetry waits for the subprocess to write a line to STDOUT
     ("Done." if successful, some error message otherwise)
  8) If the last command was successful, Telemetry sends the subprocess
     the "StopTracing" message via STDIN
  9) Telemetry continues to read trace output lines from STDOUT until
     the binary exits with an exit code of 1 (indicating failure) or
     the "Done." line is printed to STDOUT, signaling the last line of
     the trace.

Does this protocol sound reasonable?

BUG=542837

Committed: https://crrev.com/879caf4d9679a07b6dae42ec6cc09a719b5046c5
Cr-Commit-Position: refs/heads/master@{#378004}
==========

[commit-bot: I haz the power, 2016-02-26 22:26:24]

Patchset 2 (id:??) landed as
https://crrev.com/879caf4d9679a07b6dae42ec6cc09a719b5046c5
Cr-Commit-Position: refs/heads/master@{#378004}