Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1066253003:
Disable ac charging similar to usb charging (used by Nexus6) (Closed)
|
Created:
5 years, 8 months ago by fmeawad Modified:
5 years, 8 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org, aiolos (Not reviewing) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable ac charging similar to usb charging (used by Nexus6)
To disable battery updates, we disable updates from usb only. But N6 devices can also charge from ac (which they do).
This CL disables battery updates from ac as well.
BUG=474701
Committed: https://crrev.com/c4f9aab65e0d50c1cfa4a788de0033c688d6b321
Cr-Commit-Position: refs/heads/master@{#324293}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Limit BatteryDisable/EnableUpdates to only L+ devices #
Total comments: 5
Patch Set 3 : Fix Docstring #
Total comments: 3
Messages
Total messages: 27 (7 generated)
fmeawad@chromium.org changed reviewers: + jbudorick@chromium.org, rnephew@chromium.org
PTAL.
rnephew@google.com changed reviewers: + rnephew@google.com
https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... File build/android/pylib/device/battery_utils.py (left): https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:270: self._device.RunShellCommand(['dumpsys', 'battery', 'set', 'usb', '1'], This is in there because on K devices reset takes too long to run (30 seconds), and telling it it is charging before it resets is a way around that. Instead of deleting that line, I would recommend adding a line for setting ac to 1, or this will break for K devices.
On 2015/04/07 23:51:40, rnephew wrote: > https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... > File build/android/pylib/device/battery_utils.py (left): > > https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... > build/android/pylib/device/battery_utils.py:270: > self._device.RunShellCommand(['dumpsys', 'battery', 'set', 'usb', '1'], > This is in there because on K devices reset takes too long to run (30 seconds), > and telling it it is charging before it resets is a way around that. Instead of > deleting that line, I would recommend adding a line for setting ac to 1, or this > will break for K devices. We should explain why these seemingly redundant commands are here in an implementation comment.
https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... File build/android/pylib/device/battery_utils.py (left): https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:270: self._device.RunShellCommand(['dumpsys', 'battery', 'set', 'usb', '1'], On 2015/04/07 23:51:40, rnephew wrote: > This is in there because on K devices reset takes too long to run (30 seconds), > and telling it it is charging before it resets is a way around that. Instead of > deleting that line, I would recommend adding a line for setting ac to 1, or this > will break for K devices. I cannot set the ac to 1 as well as this will set the devices to have 2 power sources (which is not the case after you do a reset, as it only picks one). Will test it on a K and try to find a better solution as well as add comments as pointed by john.
https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... File build/android/pylib/device/battery_utils.py (left): https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:270: self._device.RunShellCommand(['dumpsys', 'battery', 'set', 'usb', '1'], On 2015/04/07 23:56:13, fmeawad wrote: > On 2015/04/07 23:51:40, rnephew wrote: > > This is in there because on K devices reset takes too long to run (30 > seconds), > > and telling it it is charging before it resets is a way around that. Instead > of > > deleting that line, I would recommend adding a line for setting ac to 1, or > this > > will break for K devices. > > I cannot set the ac to 1 as well as this will set the devices to have 2 power > sources (which is not the case after you do a reset, as it only picks one). > Will test it on a K and try to find a better solution as well as add comments as > pointed by john. I checked the code, and this code is never called from a K device (it is guarded by a check "dumpsys batterystats -c" and makes sure it is either revision 8 or 9 every where. I have also tried manually on a K device and it seemed to take at most 5 seconds. Given that we are deprecating most K devices, I think we can afford a few seconds on that platform for a clean solution. WDYT?
On 2015/04/08 00:54:29, fmeawad wrote: > https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... > File build/android/pylib/device/battery_utils.py (left): > > https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... > build/android/pylib/device/battery_utils.py:270: > self._device.RunShellCommand(['dumpsys', 'battery', 'set', 'usb', '1'], > On 2015/04/07 23:56:13, fmeawad wrote: > > On 2015/04/07 23:51:40, rnephew wrote: > > > This is in there because on K devices reset takes too long to run (30 > > seconds), > > > and telling it it is charging before it resets is a way around that. Instead > > of > > > deleting that line, I would recommend adding a line for setting ac to 1, or > > this > > > will break for K devices. > > > > I cannot set the ac to 1 as well as this will set the devices to have 2 power > > sources (which is not the case after you do a reset, as it only picks one). > > Will test it on a K and try to find a better solution as well as add comments > as > > pointed by john. > > I checked the code, and this code is never called from a K device (it is guarded > by a check "dumpsys batterystats -c" and makes sure it is either revision 8 or 9 > every where. > I have also tried manually on a K device and it seemed to take at most 5 > seconds. > Given that we are deprecating most K devices, I think we can afford a few > seconds on that platform for a clean solution. WDYT? Deprecating most K devices?!
On 2015/04/08 00:55:53, jbudorick wrote: > On 2015/04/08 00:54:29, fmeawad wrote: > > > https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... > > File build/android/pylib/device/battery_utils.py (left): > > > > > https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... > > build/android/pylib/device/battery_utils.py:270: > > self._device.RunShellCommand(['dumpsys', 'battery', 'set', 'usb', '1'], > > On 2015/04/07 23:56:13, fmeawad wrote: > > > On 2015/04/07 23:51:40, rnephew wrote: > > > > This is in there because on K devices reset takes too long to run (30 > > > seconds), > > > > and telling it it is charging before it resets is a way around that. > Instead > > > of > > > > deleting that line, I would recommend adding a line for setting ac to 1, > or > > > this > > > > will break for K devices. > > > > > > I cannot set the ac to 1 as well as this will set the devices to have 2 > power > > > sources (which is not the case after you do a reset, as it only picks one). > > > Will test it on a K and try to find a better solution as well as add > comments > > as > > > pointed by john. > > > > I checked the code, and this code is never called from a K device (it is > guarded > > by a check "dumpsys batterystats -c" and makes sure it is either revision 8 or > 9 > > every where. > > I have also tried manually on a K device and it seemed to take at most 5 > > seconds. > > Given that we are deprecating most K devices, I think we can afford a few > > seconds on that platform for a clean solution. WDYT? > > Deprecating most K devices?! Sorry, I meant for the perf waterfall. That information is not true everywhere, therefore not true. Sorry.
On 2015/04/08 00:57:07, fmeawad wrote: > On 2015/04/08 00:55:53, jbudorick wrote: > > On 2015/04/08 00:54:29, fmeawad wrote: > > > > > > https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... > > > File build/android/pylib/device/battery_utils.py (left): > > > > > > > > > https://codereview.chromium.org/1066253003/diff/1/build/android/pylib/device/... > > > build/android/pylib/device/battery_utils.py:270: > > > self._device.RunShellCommand(['dumpsys', 'battery', 'set', 'usb', '1'], > > > On 2015/04/07 23:56:13, fmeawad wrote: > > > > On 2015/04/07 23:51:40, rnephew wrote: > > > > > This is in there because on K devices reset takes too long to run (30 > > > > seconds), > > > > > and telling it it is charging before it resets is a way around that. > > Instead > > > > of > > > > > deleting that line, I would recommend adding a line for setting ac to 1, > > or > > > > this > > > > > will break for K devices. > > > > > > > > I cannot set the ac to 1 as well as this will set the devices to have 2 > > power > > > > sources (which is not the case after you do a reset, as it only picks > one). > > > > Will test it on a K and try to find a better solution as well as add > > comments > > > as > > > > pointed by john. > > > > > > I checked the code, and this code is never called from a K device (it is > > guarded > > > by a check "dumpsys batterystats -c" and makes sure it is either revision 8 > or > > 9 > > > every where. > > > I have also tried manually on a K device and it seemed to take at most 5 > > > seconds. > > > Given that we are deprecating most K devices, I think we can afford a few > > > seconds on that platform for a clean solution. WDYT? > > > > Deprecating most K devices?! > > Sorry, I meant for the perf waterfall. That information is not true everywhere, > therefore not true. Sorry. I kinda guessed that, but ... I dunno, that seems like ... an unusual strategy. For comparison, outside of perf, we _just_ turned down our ICS bots. I'm not as opinionated on this as I would be if battery_utils were used beyond the perf bots, but I generally prefer a slightly hairier solution that saves time. I haven't dealt much with dumpsys battery, though. Randy may have a better idea as to whether that's an acceptable compromise here.
That check exists in the telemetry code, not in this code. Right now, telemetry is the only place this is actually being used; but we cannot guarantee that will always be the case. Thats why a generic way that worked for J, K, and L was used. That being said, there is little reason to run this command on a K or J device since this is used to enable/disable battery measurements. I saw it take up to ~30 seconds on my device when I initially committed this change, so it might differ drastically from device to device; which is worrisome. 5 seconds on any test doing this will add up quick if it is ever used elsewhere.
On 2015/04/08 01:17:31, rnephew wrote: > That check exists in the telemetry code, not in this code. Right now, telemetry > is the only place this is actually being used; but we cannot guarantee that will > always be the case. Thats why a generic way that worked for J, K, and L was > used. That being said, there is little reason to run this command on a K or J > device since this is used to enable/disable battery measurements. > If that's the case, should we limit the battery measurement functions to L+? > I saw it take up to ~30 seconds on my device when I initially committed this > change, so it might differ drastically from device to device; which is > worrisome. > 5 seconds on any test doing this will add up quick if it is ever used elsewhere.
On 2015/04/08 01:21:29, jbudorick wrote: > On 2015/04/08 01:17:31, rnephew wrote: > > That check exists in the telemetry code, not in this code. Right now, > telemetry > > is the only place this is actually being used; but we cannot guarantee that > will > > always be the case. Thats why a generic way that worked for J, K, and L was > > used. That being said, there is little reason to run this command on a K or J > > device since this is used to enable/disable battery measurements. > > > > If that's the case, should we limit the battery measurement functions to L+? The original fix for K and below was done before I changed telemetry to only run the power tests on L+ devices; before they were running but just returning 0. So we could make it only work on L+ until a good reason not to comes up.
Thank you for the quick feedback and help. As per our offline discussion, we decided to limit this functionality to L+ devices only until other need appears. I have added the appropriate checks. PTAL.
lgtm w/ docstring nits https://codereview.chromium.org/1066253003/diff/20001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1066253003/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:240: if (self._device.build_version_sdk < Please add this exception to the Raises: section of the docstring. https://codereview.chromium.org/1066253003/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:276: if (self._device.build_version_sdk < same here
https://codereview.chromium.org/1066253003/diff/20001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1066253003/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:240: if (self._device.build_version_sdk < On 2015/04/08 17:43:11, jbudorick wrote: > Please add this exception to the Raises: section of the docstring. Done. https://codereview.chromium.org/1066253003/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:276: if (self._device.build_version_sdk < On 2015/04/08 17:43:11, jbudorick wrote: > same here Done. https://codereview.chromium.org/1066253003/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:305: device_errors.CommandFailedError: If device is not L or higher. Also fixed this one to be DeviceVersionError
The CQ bit was checked by fmeawad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1066253003/#ps40001 (title: "Fix Docstring")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1066253003/40001
off-by-one on the docstring fix :) I'm ok with this landing through the CQ as is and then being fixed separately. https://codereview.chromium.org/1066253003/diff/40001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1066253003/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:203: device_errors.DeviceVersionError: If device is not L or higher. er... wrong Raises section. https://codereview.chromium.org/1066253003/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:274: """ This needs a Raises section with DeviceVersionError.
https://codereview.chromium.org/1066253003/diff/40001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1066253003/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:203: device_errors.DeviceVersionError: If device is not L or higher. On 2015/04/08 18:35:23, jbudorick wrote: > er... wrong Raises section. Good catch, will fix in another CL.
The CQ bit was checked by fmeawad@chromium.org
The CQ bit was unchecked by fmeawad@chromium.org
The CQ bit was checked by fmeawad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1066253003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c4f9aab65e0d50c1cfa4a788de0033c688d6b321 Cr-Commit-Position: refs/heads/master@{#324293} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
