Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1611533002:
tools/battor_agent: Adds a tool to find connected BattOrs (Closed)
|
Created:
4 years, 11 months ago by charliea (OOO until 10-5) Modified:
4 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiontools/battor_agent: Adds a tool to find connected BattOrs
This tool will also be used by the Chromium trace controller to find
any connected BattOrs.
BUG=542837
Committed: https://crrev.com/22f5284d4b0e007cb59effaefa7dbd82717eca79
Cr-Commit-Position: refs/heads/master@{#370985}
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : Code review #
Messages
Total messages: 29 (9 generated)
Description was changed from ========== tools/battor_agent: Adds a tool to find connected BattOrs BUG=542837 ========== to ========== tools/battor_agent: Adds a tool to find connected BattOrs This tool will also be used by the Chromium trace controller to find any connected BattOrs. BUG=542837 ==========
charliea@chromium.org changed reviewers: + zhenw@chromium.org
zhenw@chromium.org changed reviewers: + nednguyen@google.com
Maybe we also want to use battor finder directly from the telemetry side? Do we want to make it a separate binary too?
On 2016/01/20 18:05:45, Zhen Wang wrote: > Maybe we also want to use battor finder directly from the telemetry side? Do we > want to make it a separate binary too? I don't see much value in have two separate binaries. Can you elaborate?
On 2016/01/20 18:43:07, nednguyen wrote: > On 2016/01/20 18:05:45, Zhen Wang wrote: > > Maybe we also want to use battor finder directly from the telemetry side? Do > we > > want to make it a separate binary too? > > I don't see much value in have two separate binaries. Can you elaborate? When Telemetry calls to battor_agent_bin to start tracing, I don't think battor_agent_bin would return false if there is no battor connected. Therefore, Telemetry may want to check the availability of battor beforehand.
On 2016/01/20 18:46:46, Zhen Wang wrote: > On 2016/01/20 18:43:07, nednguyen wrote: > > On 2016/01/20 18:05:45, Zhen Wang wrote: > > > Maybe we also want to use battor finder directly from the telemetry side? Do > > we > > > want to make it a separate binary too? > > > > I don't see much value in have two separate binaries. Can you elaborate? > > When Telemetry calls to battor_agent_bin to start tracing, I don't think > battor_agent_bin would return false if there is no battor connected. Therefore, > Telemetry may want to check the availability of battor beforehand. Can we just have: "battor_agent_bin --list-battors" cmd
On 2016/01/20 18:48:37, nednguyen wrote: > On 2016/01/20 18:46:46, Zhen Wang wrote: > > On 2016/01/20 18:43:07, nednguyen wrote: > > > On 2016/01/20 18:05:45, Zhen Wang wrote: > > > > Maybe we also want to use battor finder directly from the telemetry side? > Do > > > we > > > > want to make it a separate binary too? > > > > > > I don't see much value in have two separate binaries. Can you elaborate? > > > > When Telemetry calls to battor_agent_bin to start tracing, I don't think > > battor_agent_bin would return false if there is no battor connected. > Therefore, > > Telemetry may want to check the availability of battor beforehand. > > Can we just have: "battor_agent_bin --list-battors" cmd I think that would also work.
On 2016/01/20 18:50:17, Zhen Wang wrote: > On 2016/01/20 18:48:37, nednguyen wrote: > > On 2016/01/20 18:46:46, Zhen Wang wrote: > > > On 2016/01/20 18:43:07, nednguyen wrote: > > > > On 2016/01/20 18:05:45, Zhen Wang wrote: > > > > > Maybe we also want to use battor finder directly from the telemetry > side? > > Do > > > > we > > > > > want to make it a separate binary too? > > > > > > > > I don't see much value in have two separate binaries. Can you elaborate? > > > > > > When Telemetry calls to battor_agent_bin to start tracing, I don't think > > > battor_agent_bin would return false if there is no battor connected. > > Therefore, > > > Telemetry may want to check the availability of battor beforehand. > > > > Can we just have: "battor_agent_bin --list-battors" cmd > > I think that would also work. Good idea. I think that this patch should lay a groundwork for either of those paths if we want to do it in the future, but my preference is probably to cross that bridge once we get there.
lgtm
The CQ bit was checked by charliea@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1611533002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1611533002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
charliea@chromium.org changed reviewers: + sky@chromium.org
+sky for mojo/public approval
LGTM I have no clue what a BattOr is. I do find it unusual that you have the dir name as battor_agent yet class name as BattOrAgent. I would expect the two to match, so either BattorAgent, or you rename the dir to batt_or_agent. https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:55: return ""; nit: std::string() https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_finder.cc (right): https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_finder.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_finder.cc:36: return ""; nit: std::string() https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_finder.h (right): https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_finder.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_finder.h:15: }; private: DISALLOW...
https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_agent_bin.cc (right): https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_agent_bin.cc:55: return ""; On 2016/01/21 22:59:20, sky wrote: > nit: std::string() Done. https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_finder.cc (right): https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_finder.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/21 22:59:20, sky wrote: > nit: 2016 Done. https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_finder.cc:36: return ""; On 2016/01/21 22:59:20, sky wrote: > nit: std::string() Done. https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... File tools/battor_agent/battor_finder.h (right): https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_finder.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/21 22:59:20, sky wrote: > nit: 2016 Done. https://codereview.chromium.org/1611533002/diff/20001/tools/battor_agent/batt... tools/battor_agent/battor_finder.h:15: }; On 2016/01/21 22:59:20, sky wrote: > private: DISALLOW... Done.
On 2016/01/21 22:59:20, sky wrote: > LGTM > I have no clue what a BattOr is. I do find it unusual that you have the dir name > as battor_agent yet class name as BattOrAgent. I would expect the two to match, > so either BattorAgent, or you rename the dir to batt_or_agent. Yea, this stems from weird capitalization in the name itself. A "BattOr" is a little device capable of taking high-frequency power samples of a device that it's connected to. We're going to be using it within Chrome to identify and fix code that's consuming lots of power. batt_or_agent looks really strange, especially because it looks like we're trying to say batt <logical or> agent. It's possible we should intentionally not capitalize correctly and just make everything "BattorAgent" instead.
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1611533002/#ps40001 (title: "Code review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1611533002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1611533002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== tools/battor_agent: Adds a tool to find connected BattOrs This tool will also be used by the Chromium trace controller to find any connected BattOrs. BUG=542837 ========== to ========== tools/battor_agent: Adds a tool to find connected BattOrs This tool will also be used by the Chromium trace controller to find any connected BattOrs. BUG=542837 Committed: https://crrev.com/22f5284d4b0e007cb59effaefa7dbd82717eca79 Cr-Commit-Position: refs/heads/master@{#370985} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/22f5284d4b0e007cb59effaefa7dbd82717eca79 Cr-Commit-Position: refs/heads/master@{#370985}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1623563002/ by shess@chromium.org. The reason for reverting is: buildbot failure in Chromium GPU on Android Debug (Nexus 6) [918/3714] LINK battor_agent ... obj/tools/battor_agent/libbattor_agent_lib.a(obj/tools/battor_agent/battor_agent_lib.battor_finder.o):battor_finder.cc:function battor::BattOrFinder::FindBattOr(): error: undefined reference to 'device::SerialDeviceEnumerator::Create()' .
Message was sent while issue was closed.
On 2016/01/22 17:52:43, Scott Hess wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/1623563002/ by mailto:shess@chromium.org. > > The reason for reverting is: buildbot failure in Chromium GPU on Android Debug > (Nexus 6) > > [918/3714] LINK battor_agent > ... > obj/tools/battor_agent/libbattor_agent_lib.a(obj/tools/battor_agent/battor_agent_lib.battor_finder.o):battor_finder.cc:function > battor::BattOrFinder::FindBattOr(): error: undefined reference to > 'device::SerialDeviceEnumerator::Create()' > . I already reverted this in https://codereview.chromium.org/1641573003/ - sorry, I didn't know there was a 1 click revert button. I'll use that in the future.
Message was sent while issue was closed.
> I already reverted this in https://codereview.chromium.org/1641573003/ - sorry, > I didn't know there was a 1 click revert button. I'll use that in the future. Ah, now I'm confusing myself disregard that last comment. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
