cros-usb-pd-charger driver should provide current_now in sysfs |
||||
Issue descriptionPresently when examining powerd logs with device on AC using USB PD charger you'll see, On AC (USB_PD, 0.000A at 20.1V, max 2.2A at 20.0V) with battery at 94% (displayed as 96%), 5.043/5.385Ah at 1.157A, 8m21s until full or similarly in power_supply_info, # power_supply_info Device: Line Power path: /sys/class/power_supply/CROS_USB_PD_CHARGER0 online: yes type: USB_PD enum type: AC voltage (V): 20.214 current (A): 0 max voltage (V): 20 max current (A): 2.25 active source: CROS_USB_PD_CHARGER0 available sources: CROS_USB_PD_CHARGER0* [2109/100] supports dual-role: yes and the '0.000A at 20.1V' or 'current (A): 0' is due to: ls /sys/class/power_supply/CROS_USB_PD_CHARGER0/current_now ls: cannot access '/sys/class/power_supply/CROS_USB_PD_CHARGER0/current_now': No such file or directory Not that the current is actually zero. Additionally we have that information available it seems, # ectool chargestate show ac = 1 chg_voltage = 8752mV chg_current = 2688mA chg_input_current = 2112mA batt_state_of_charge = 97% Ideally it would be great to advertise all these 'chargestate' values but driver stands now we could at least ad 'chg_input_current' == <sysfs>/current_now (POWER_SUPPLY_PROP_CURRENT_NOW) as its the complement to voltage_now where its the voltage input to the charger.
,
Feb 1 2018
We typically do not have the ability to measure instantaneous input current, which is why we're not reporting it. If some boards do have that ability, I agree it makes sense to rev EC_CMD_USB_PD_POWER_INFO to include the additional info.
,
May 2 2018
,
May 2 2018
I wonder if it would make sense to get powerd / power_supply_info to report this more sanely too, when it's not available.
,
May 3 2018
Re reporting more sanely, are you envisioning something like "unknown" instead of "0" if current_now is missing?
,
May 3 2018
For power_supply_info: sure? Or blank. (I see several blank results from 'power_supply_info' on Scarlet.) For powerd logs: we could just avoid printing it? Or some other indication of 'unknown'.
,
May 10 2018
I'll update the output and then assign this back to Benson.
,
May 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/9502ea551a8ed5668d6d7ad855795381d32acf0d commit 9502ea551a8ed5668d6d7ad855795381d32acf0d Author: Daniel Erat <derat@chromium.org> Date: Sat May 12 06:22:23 2018 power: Don't report missing current or voltage as 0. If the current_now or voltage_now sysfs nodes are missing from a line power supply, make powerd omit them from "On AC" log messages and make power_supply_info print "unknown". We formerly treated missing nodes as 0, making it impossible to tell whether that was the actual value that was seen or not. BUG=chromium:807753 TEST=updated unit tests; also verified that caroline now logs the following instead of a 0 current: "On AC (USB_PD, 14.6V, max 2.0A at 15.0V) with ..." and that power_supply_info prints: "current (A): unknown" Change-Id: I8bd182ded113e3d677dfe092fdf53ed32401f5a9 Reviewed-on: https://chromium-review.googlesource.com/1055790 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Brian Norris <briannorris@chromium.org> [modify] https://crrev.com/9502ea551a8ed5668d6d7ad855795381d32acf0d/power_manager/tools/power_supply_info.cc [modify] https://crrev.com/9502ea551a8ed5668d6d7ad855795381d32acf0d/power_manager/powerd/system/power_supply.cc [modify] https://crrev.com/9502ea551a8ed5668d6d7ad855795381d32acf0d/power_manager/powerd/system/power_supply_unittest.cc [modify] https://crrev.com/9502ea551a8ed5668d6d7ad855795381d32acf0d/power_manager/powerd/system/power_supply.h
,
May 12 2018
Back to Benson for looking into making the driver report current_now. |
||||
►
Sign in to add a comment |
||||
Comment 1 by bleung@chromium.org
, Feb 1 2018Owner: bleung@chromium.org
Status: Assigned (was: Untriaged)
Question for shawnn... Here's the response values from the ec around usb charging: struct __ec_align2 usb_chg_measures { uint16_t voltage_max; uint16_t voltage_now; uint16_t current_max; uint16_t current_lim; }; Note that we don't actually have one that represents instantaneous current. Is that a hardware limitation? I'd definitely like to add something for current_now, but it's not clear we get that info in a meaningful way.