New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 752320 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocking:
issue 760266



Sign in to add a comment

improve ec power reporting

Project Member Reported by coconutruben@chromium.org, Aug 3 2017

Issue description

Currently, when we measure power, in part we ask the ec what consumption it's seeing based on the battery's gas gauge.

https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/master/servo/drv/ec.py#284

We'd like to improve this in 2 ways:
(1) not involve the ec console, so that we don't wake the ec if it's in a low power state
(2) get more than one sample in, and report the average consumption over the last x seconds (x being ~3-6s?)

The idea is
(1) to keep a running average and have a low priority task that periodically polls the battery for its power numbers, and maintains the average
(2) I'm not sure how to do this. We'd want to have access to that information over servo.

This bug is to get input on how to accomplish those improvements, and track work on this front.
 
You can declare a HOOK_TICK function which will run every HOOK_TICK_INTERVAL. From that function, you can buffer one sample per HOOK_TICK_INTERVAL, or even keep a running min / max / mean over an indefinite period. You can begin sampling when a console function is executed, and dump the data when a different param is passed to your console function.

If you're monitoring over a short period of time and want to measure low-power idle power, be sure to run "dsleep 0" from the console before beginning sampling, to make sure "console in use timeout" doesn't prevent low-power idle. 
Blocking: 760266
I ran a couple tests on this and got the following results on soraka:
- running a full get_battery_params every HOOK_TICK (200ms) costs about 5mW
- running a voltage and current getter during every HOOK_TICK (200ms) costs about 1mW
- running a voltage and current getter every 5 HOOK_TICK (1s) costs about 140uW (so within the noise).

I think there are a couple options we have w/ polling every second:
(1) keep running avg for a set window size
(2) keep infinite running avg and reset the avg every time a user queries it
(2.1) allow for an option to query without resetting the average.

Since the use cases for this are to have secondary power number besides from ADCs, and allow for quick measurements without needing reworks (on boards without on-board adcs), I'm leaning toward option (1). Also it's much simpler.
Thoughts?

Comment 4 by sha...@chromium.org, Oct 21 2017

I'm not sure I understand your proposals, but I think it's OK to add a new feature, behind a CONFIG option, that takes power records in whichever way you think is most useful. Regarding the power impact of extra voltage / current polling, you can make it less intrusive by re-using the periodic battery measurements we take and store anyway -- of course, this makes your polling period less frequent.
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/51e9e69f386366256807e6f4ccdd258821cdcfe0

commit 51e9e69f386366256807e6f4ccdd258821cdcfe0
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Tue Jan 16 12:08:26 2018

power: introducing pwr_avg console command

pwr_avg provides an average voltage, current, and power over the last
1 minute. It's up to the battery drivers to implement this
functionality.
This change allows us to have better power tracking while minimizing
the power impact on the EC, because
- the pwr_avg command only needs to be called once every minute, and is
short, thus less expensive to parse on ECs without a UART buffer
- the work done to keep the avg is partially done by the batteries
already and it's just a question of retrieving it.

undefined on wheatley since no power debugging planned on that board.

usage:
> pwr_avg
mv = 7153
ma = -605
mw = -4327

BUG=chromium:752320
BRANCH=None
TEST=make buildall -j

Change-Id: Id1a3479d277aedf90dfa965afb4ee9136654b1cf
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/823884
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/51e9e69f386366256807e6f4ccdd258821cdcfe0/driver/battery/smart.c
[modify] https://crrev.com/51e9e69f386366256807e6f4ccdd258821cdcfe0/common/charge_state_v2.c
[modify] https://crrev.com/51e9e69f386366256807e6f4ccdd258821cdcfe0/include/config.h
[modify] https://crrev.com/51e9e69f386366256807e6f4ccdd258821cdcfe0/board/wheatley/board.h
[modify] https://crrev.com/51e9e69f386366256807e6f4ccdd258821cdcfe0/include/battery.h
[modify] https://crrev.com/51e9e69f386366256807e6f4ccdd258821cdcfe0/driver/battery/max17055.c

Blockedon: 806147
Blockedon: -806147
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 20 2018

Labels: merge-merged-firmware-eve-9584.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/1da8acbe375eba18ec0cf1a469312f77fbaf88c7

commit 1da8acbe375eba18ec0cf1a469312f77fbaf88c7
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Tue Mar 20 11:15:20 2018

power: introducing pwr_avg console command

pwr_avg provides an average voltage, current, and power over the last
1 minute. It's up to the battery drivers to implement this
functionality.
This change allows us to have better power tracking while minimizing
the power impact on the EC, because
- the pwr_avg command only needs to be called once every minute, and is
short, thus less expensive to parse on ECs without a UART buffer
- the work done to keep the avg is partially done by the batteries
already and it's just a question of retrieving it.

undefined on wheatley since no power debugging planned on that board.

usage:
> pwr_avg
mv = 7153
ma = -605
mw = -4327

BUG=chromium:752320
BRANCH=eve
TEST=make BOARD=eve -j

Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/823884
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>
(cherry picked from commit 51e9e69f386366256807e6f4ccdd258821cdcfe0)

 Conflicts:
  board/wheatley/board.h
    undefine some tasks to make buildall -j work
  driver/battery/max17055.c
    did not exist in branch, not needed

Change-Id: I5f37a6f6446fa0e4a205672cb3433827d5da948d
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/966244
Reviewed-by: Duncan Laurie <dlaurie@google.com>

[modify] https://crrev.com/1da8acbe375eba18ec0cf1a469312f77fbaf88c7/driver/battery/smart.c
[modify] https://crrev.com/1da8acbe375eba18ec0cf1a469312f77fbaf88c7/board/wheatley/board.h
[modify] https://crrev.com/1da8acbe375eba18ec0cf1a469312f77fbaf88c7/include/battery.h
[modify] https://crrev.com/1da8acbe375eba18ec0cf1a469312f77fbaf88c7/common/charge_state_v2.c
[modify] https://crrev.com/1da8acbe375eba18ec0cf1a469312f77fbaf88c7/include/config.h

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/8142d266fa53aa38e0106e06a8f7a21e9d83dfce

commit 8142d266fa53aa38e0106e06a8f7a21e9d83dfce
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Wed Mar 21 17:53:27 2018

servo: leverage pwr_avg where available

pwr_avg was recently introduced as an ec command that provides a
one minute averaged reading of current, voltage, and power. This
change introduces the ec code and xml files required to leverage
this. Now there is:
- avg_ppvar_vbat_mw
- avg_ppvar_vbat_ma
- avg_ppvar_vbat_mv

Since the command is fairly new, the code also explicitly checks
for the command being available, and returns a more informative error
if the failure is due to pwr_avg not being available on that ec image.
Any code, such as power scripts, can then decide higher up in the stack
if pwr_avg being unavailable is a deal breaker or if it's okay to
default to ppvar_vbat_mw.

BUG=chromium:752320, chromium:760267
TEST=manual
$ dut-control avg_ppvar_vbat_mw
avg_ppvar_vbat_mw:5572
(on dut without the command)
$ dut-control avg_ppvar_vbat_mw
Problem with ['avg_ppvar_vbat_mw'] :: cmd |pwr_avg| is not available on
the ec.

Change-Id: I4b9efeaa87a5c5f6ca932cfa2cec7cb135f7d388
Reviewed-on: https://chromium-review.googlesource.com/888281
Commit-Ready: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Tested-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-by: Todd Broch <tbroch@chromium.org>

[modify] https://crrev.com/8142d266fa53aa38e0106e06a8f7a21e9d83dfce/servo/data/ec_common.xml
[modify] https://crrev.com/8142d266fa53aa38e0106e06a8f7a21e9d83dfce/servo/drv/ec.py

Sign in to add a comment