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

Issue 674338 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature



Sign in to add a comment

power_manager : Gather power supply type as a part of UMA metrics

Project Member Reported by bleung@chromium.org, Dec 15 2016

Issue description

It would be very useful to know which kind of external power supply the population uses.

Specifically, for our USB-C laptops, this can be anything in the set of : 
["Mains", "USB", "USB_DCP", "USB_CDP", "USB_C", "USB_PD", "USB_PD_DRP"]

We should add a stat for power_supply type.
 

Comment 1 by derat@chromium.org, Dec 15 2016

Components: OS>Kernel>Power
Owner: derat@chromium.org
Status: Assigned (was: Available)
I can take a look at this.

The strings that powerd currently understands from the sysfs "type" field appear to be the following:

Reported as low-power USB changes for non-dual-role ports:
  USB
  USB_DCP
  USB_CDP
  USB_ACA (not on your list)

Treated as dual-role (just for the purpose of determining sink status and active-by-default):
  USB_PD_DRP

It should be easy enough to just create histogram values for all of the strings in your list and map accordingly -- I'm just mentioning the above in case powerd's existing logic also needs to be updated.

Comment 2 by bleung@google.com, Dec 15 2016

Thanks, Dan. We should almost never see USB_ACA as that's not something that shows up in any of the power supply drivers that we use.

"Mains" does show up, though. We use it to categorize Apple proprietary chargers, but "Mains" is also the generic type that shows up on non-USB and non-Type-C laptops as the generic AC adapter (barrel jack, whathaveyou).


Comment 3 by bleung@google.com, Dec 15 2016

In addition to the power supply type, it may also be useful to grab voltage and max current as well.

Just a couple of examples from my most recent powerd.LATEST log : 

On USB (USB_DCP, 0.000A at 5.3V, max 2.0A at 5.0V) with battery at 100%, 9.245/9.245Ah at 1.009A, full
On AC (USB_PD, 0.000A at 20.6V, max 3.0A at 20.0V) with battery at 100%, 9.245/9.245Ah at 0.000A, full


In the cases above, it's useful to have the power supply string definitely, but also the other bits of information would be great to have on a histogram too...

SPecifically, the max 2.0A at 5.0V together as one piece of information (that tells us that we're charging at 10W) is useful, as is 3.0A at 20.0V, which tells us that we're charging at 60W. 

Having just the current information by itself is less useful unless it's linked to the voltage.

Comment 4 by derat@chromium.org, Dec 15 2016

Some other notes from chatting just now:

- The zero instantaneous current is expected, as some drivers don't measure it.
- powerd should continue to log both instantaneous current and voltage.
- We should have a metric for max power (max current times max voltage).
- A metric that just contains max voltage would be useful too.

Comment 5 by bleung@google.com, Dec 15 2016

One more potential one to add : 

A metric tracking the active source and available sources would be interesting too!

run power_supply_info. It lists one active source, and potentially multiple available sources : 

Device: Line Power
  path:                    /sys/class/power_supply/CROS_USB_PD_CHARGER1
  online:                  yes
  type:                    USB_PD
  enum type:               AC
  voltage (V):             18.148
  current (A):             0
  max voltage (V):         19
  max current (A):         3
  active source:           CROS_USB_PD_CHARGER1
  available sources:       CROS_USB_PD_CHARGER0* [0/9a00], CROS_USB_PD_CHARGER1* [43e/9a30]
  supports dual-role:      yes

Having these metrics would allow us to know which port is used more often, and how often people plug in two chargers (obscure, I know, but I love data!).

Comment 6 by derat@chromium.org, Dec 15 2016

Sure thing. We have a bunch of possible positions:

    enum Port {
      // The location of the port is unknown, or there's only one port.
      UNKNOWN = 0;

      // Various positions on the device. The first word describes the side of
      // the device where the port is located while the second clarifies the
      // position. For example, LEFT_BACK means the farthest-back port on the
      // left side, while BACK_LEFT means the leftmost port on the back of the
      // device.
      LEFT        = 1;
      RIGHT       = 2;
      BACK        = 3;
      FRONT       = 4;
      LEFT_FRONT  = 5;
      LEFT_BACK   = 6;
      RIGHT_FRONT = 7;
      RIGHT_BACK  = 8;
      BACK_LEFT   = 9;
      BACK_RIGHT  = 10;

      // Next value to use: 11
    }

The charging_ports pref (only set for samus, as far as I can tell -- that might be a bug) maps power supply names like CROS_USB_PD_CHARGER0 to positions like LEFT.

One approach would be to have a "connected charging ports" histogram with values like NONE, FIRST, SECOND, and FIRST_SECOND (or maybe ALL). Then sysfs power supplies would be mapped to histogram values based on their position in the pref, e.g. FIRST if only CROS_USB_PD_CHARGER0 is connected on samus.

Otherwise, if we always plan to name ports similarly, I could just extract 0 or 1 or whatever from the end of the power supply name.

Comment 7 by bleung@google.com, Dec 16 2016

I think the plan for the cros_usbpd_charger driver is to always name them as you have seen them, but in the future maybe we will have a different driver that names things differently.

It definitely seems like a bug that Samus is the only system that uses the charging_ports pref.

I'll file bugs for Sentry and Chell (the two other systems that have multiple Type-C ports to my knowledge released) as well, and add the pref too.

Will adding the pref mean the Settings UI will have the right names for these ports too?

Comment 8 by derat@chromium.org, Dec 16 2016

Thanks! Yes, I believe that settings is actually the only thing that the pref is used for.

Comment 9 by derat@chromium.org, Dec 17 2016

Status: Started (was: Assigned)
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/869ff78cca4e978426044b701051e028caa87adb

commit 869ff78cca4e978426044b701051e028caa87adb
Author: derat <derat@chromium.org>
Date: Mon Dec 19 23:25:49 2016

Document Power.PowerSupply{MaxPower,MaxVoltage,Type} metrics

Document three power-supply-related histograms reported by
the powerd process on Chrome OS:

Power.PowerSupplyMaxPower
Power.PowerSupplyMaxVoltage
Power.PowerSupplyType

https://chromium-review.googlesource.com/#/c/422055/ makes
powerd report the histograms.

BUG= 674338 

Review-Url: https://codereview.chromium.org/2592503002
Cr-Commit-Position: refs/heads/master@{#439610}

[modify] https://crrev.com/869ff78cca4e978426044b701051e028caa87adb/tools/metrics/histograms/histograms.xml

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/bae4e4a0d8e6711a70a05933eba3bfc419976e4c

commit bae4e4a0d8e6711a70a05933eba3bfc419976e4c
Author: Daniel Erat <derat@chromium.org>
Date: Sat Dec 17 04:13:49 2016

power: Add charger-related metrics.

Add several histograms containing information about the
active external power supply:

Power.PowerSupplyType
Power.PowerSupplyMaxVoltage
Power.PowerSupplyMaxPower

Also add a comment and warning to
MetricsSender::SendMetric() mentioning that
constraint-violating samples may be silently discarded by
Chrome.

BUG= chromium:674338 
TEST=added unit tests; also viewed chrome://histograms on a
     samus device and verified that all three histograms are
     updated periodically while a charger is connected

Change-Id: I34abf83a4a4ef2048869f020f944e5e7ef8ab170
Reviewed-on: https://chromium-review.googlesource.com/422055
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>

[modify] https://crrev.com/bae4e4a0d8e6711a70a05933eba3bfc419976e4c/power_manager/powerd/metrics_collector.cc
[modify] https://crrev.com/bae4e4a0d8e6711a70a05933eba3bfc419976e4c/power_manager/powerd/system/power_supply.h
[modify] https://crrev.com/bae4e4a0d8e6711a70a05933eba3bfc419976e4c/power_manager/common/metrics_constants.cc
[modify] https://crrev.com/bae4e4a0d8e6711a70a05933eba3bfc419976e4c/power_manager/common/metrics_sender.cc
[modify] https://crrev.com/bae4e4a0d8e6711a70a05933eba3bfc419976e4c/power_manager/powerd/system/power_supply.cc
[modify] https://crrev.com/bae4e4a0d8e6711a70a05933eba3bfc419976e4c/power_manager/powerd/metrics_collector_unittest.cc
[modify] https://crrev.com/bae4e4a0d8e6711a70a05933eba3bfc419976e4c/power_manager/common/metrics_sender.h
[modify] https://crrev.com/bae4e4a0d8e6711a70a05933eba3bfc419976e4c/power_manager/common/metrics_constants.h

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/ec60575959c6d8d5c34bc4da30c4fb147e8f6c7d

commit ec60575959c6d8d5c34bc4da30c4fb147e8f6c7d
Author: Daniel Erat <derat@chromium.org>
Date: Thu Dec 22 17:16:07 2016

power: Make PowerStatus include info about all ports.

powerd's PowerStatus struct formerly contained information
only about ports that had power sources connected to them
since that was all that was needed by the UI.

To make it easier to add metrics about which ports are
connected and disconnected in a future change, collect
information about all ports instead. The
PowerSupplyProperties protobufs that are sent to Chrome and
the power_supply_info helper program continue to only
include information about ports with connected power
sources.

BUG= chromium:674338 
TEST=updated and added tests; also checked that
     power_supply_info still works and added debugging and
     checked that dedicated sources and dual-role devices
     are detected on samus when connecting zinger or ryu

Change-Id: I7943952a0133b0001aa863cc651ed5a9c922bc4a
Reviewed-on: https://chromium-review.googlesource.com/423233
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>

[modify] https://crrev.com/ec60575959c6d8d5c34bc4da30c4fb147e8f6c7d/power_manager/tools/power_supply_info.cc
[modify] https://crrev.com/ec60575959c6d8d5c34bc4da30c4fb147e8f6c7d/power_manager/powerd/system/power_supply.cc
[modify] https://crrev.com/ec60575959c6d8d5c34bc4da30c4fb147e8f6c7d/power_manager/powerd/system/power_supply_unittest.cc
[modify] https://crrev.com/ec60575959c6d8d5c34bc4da30c4fb147e8f6c7d/power_manager/powerd/system/power_supply.h

Project Member

Comment 13 by bugdroid1@chromium.org, Jan 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/769bfbde2134ce75abd6c3323b341e30a536aba1

commit 769bfbde2134ce75abd6c3323b341e30a536aba1
Author: derat <derat@chromium.org>
Date: Mon Jan 16 15:54:57 2017

chromeos: Document Power.ConnectedChargingPorts histogram.

Document a new Power.ConnectedChargingPorts histogram added
by https://chromium-review.googlesource.com/c/426381/ .

BUG= 674338 

Review-Url: https://codereview.chromium.org/2620343004
Cr-Commit-Position: refs/heads/master@{#443902}

[modify] https://crrev.com/769bfbde2134ce75abd6c3323b341e30a536aba1/tools/metrics/histograms/histograms.xml

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/99f6edf9edd9530523b63e7b8e44572953902974

commit 99f6edf9edd9530523b63e7b8e44572953902974
Author: Daniel Erat <derat@chromium.org>
Date: Tue Jan 10 02:11:00 2017

power: Add Power.ConnectedChargingPorts histogram.

Make powerd report a histogram describing which charging
ports, if any, are connected.

BUG= chromium:674338 
TEST=added tests; also connected zinger to both ports on
     samus while watching chrome://histograms

Change-Id: Ida7fca62e0fba1daaef6c4549b97a451fc6dd8e1
Reviewed-on: https://chromium-review.googlesource.com/426381
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>

[modify] https://crrev.com/99f6edf9edd9530523b63e7b8e44572953902974/power_manager/powerd/metrics_collector_unittest.cc
[modify] https://crrev.com/99f6edf9edd9530523b63e7b8e44572953902974/power_manager/powerd/metrics_collector.cc
[modify] https://crrev.com/99f6edf9edd9530523b63e7b8e44572953902974/power_manager/common/metrics_constants.cc
[modify] https://crrev.com/99f6edf9edd9530523b63e7b8e44572953902974/power_manager/common/metrics_constants.h

Comment 15 by derat@chromium.org, Jan 17 2017

Status: Fixed (was: Started)

Comment 16 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 17 by son...@google.com, Apr 4 2017

Status: Verified (was: Fixed)
Verified on build 9334.33.0

Sign in to add a comment