Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1180443004:
[Android] Change how battery_utils determines if battery updates resume. (Closed)
|
Created:
5 years, 6 months ago by rnephew (Reviews Here) Modified:
5 years, 6 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Change how battery_utils determines if battery updates resume.
[Android] Force enable battery updates instead of resetting.
adb over wifi is broken for power measurements. This should fix it.
BUG=497238
Committed: https://crrev.com/4fc81ca8441f541c2fceacde9ade58a3fa251e5b
Cr-Commit-Position: refs/heads/master@{#333866}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 4
Patch Set 3 : formatting #Patch Set 4 : rebase + fix test broken due to rebase #
Messages
Total messages: 26 (8 generated)
rnephew@chromium.org changed reviewers: + fmeawad@chromium.org, jbudorick@chromium.org, srikumar.b@gmail.com
https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:327: if 'UPDATES STOPPED' in self._device.RunShellCommand( I really do not like scanning for text in stdout of adb shell commands, and I think it should be avoided when possible -- _especially_ when what we have works for the bots. Could we potentially do both the GetCharging check and this 'UPDATES STOPPED' check? Also, please don't do this: if condition: return False return True Instead, just do this: return not bool(condition)
https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:327: if 'UPDATES STOPPED' in self._device.RunShellCommand( On 2015/06/10 18:04:02, jbudorick wrote: > I really do not like scanning for text in stdout of adb shell commands, and I > think it should be avoided when possible -- _especially_ when what we have works > for the bots. > > Could we potentially do both the GetCharging check and this 'UPDATES STOPPED' > check? > > Also, please don't do this: > > if condition: > return False > return True > > Instead, just do this: > > return not bool(condition) The scenario where the old implementation is broken is when using adb over wifi. Since the phone will not be charging when the battery updates resume, it will fail if you check if the device is charging.
On 2015/06/10 at 18:07:19, rnephew wrote: > https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... > File build/android/pylib/device/battery_utils.py (right): > > https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... > build/android/pylib/device/battery_utils.py:327: if 'UPDATES STOPPED' in self._device.RunShellCommand( > On 2015/06/10 18:04:02, jbudorick wrote: > > I really do not like scanning for text in stdout of adb shell commands, and I > > think it should be avoided when possible -- _especially_ when what we have works > > for the bots. > > > > Could we potentially do both the GetCharging check and this 'UPDATES STOPPED' > > check? > > > > Also, please don't do this: > > > > if condition: > > return False > > return True > > > > Instead, just do this: > > > > return not bool(condition) > > The scenario where the old implementation is broken is when using adb over wifi. Since the phone will not be charging when the battery updates resume, it will fail if you check if the device is charging. return self.GetCharging() or 'UPDATES STOPPED' not in self.device._RunShellCommand(['dumpsys', 'battery'], check_return=True): ?
https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:327: if 'UPDATES STOPPED' in self._device.RunShellCommand( On 2015/06/10 18:04:02, jbudorick wrote: > I really do not like scanning for text in stdout of adb shell commands, and I > think it should be avoided when possible -- _especially_ when what we have works > for the bots. > > Could we potentially do both the GetCharging check and this 'UPDATES STOPPED' > check? > > Also, please don't do this: > > if condition: > return False > return True > > Instead, just do this: > > return not bool(condition) Done.
https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... build/android/pylib/device/battery_utils.py:327: if 'UPDATES STOPPED' in self._device.RunShellCommand( This check may work right now, But when we start running telemetry for ATV or non-battery android devices, Are these checks always effective w.r.t battery_utils? Its like checking for battery updates on a device which does not have battery.
On 2015/06/10 18:20:22, sri wrote: > https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... > File build/android/pylib/device/battery_utils.py (right): > > https://codereview.chromium.org/1180443004/diff/1/build/android/pylib/device/... > build/android/pylib/device/battery_utils.py:327: if 'UPDATES STOPPED' in > self._device.RunShellCommand( > This check may work right now, But when we start running telemetry for ATV or > non-battery android devices, Are these checks always effective w.r.t > battery_utils? Its like checking for battery updates on a device which does not > have battery. The check for the UPDATES STOPPED should work, since its asking if its capable of receiving batter updates. When you set 'dumpsys battery set (usb/ac) 0/1' you are making it stick in the 'not charging'/'charging' state, which disables the updates. I would assume that ATV and similar devices will work in a similar way at the android system level.
lgtm
On 2015/06/10 at 20:48:49, jbudorick wrote: > lgtm er, not lgtm sorry, formatting.
The CQ bit was checked by rnephew@google.com
The CQ bit was unchecked by rnephew@google.com
jbudorick@chromium.org changed reviewers: - rnephew@google.com
https://codereview.chromium.org/1180443004/diff/20001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1180443004/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:328: or not bool('UPDATES STOPPED' in self._device.RunShellCommand( I'm not crazy about having this indented four spaces with the above paren starting this statement, but I think it's legal. https://codereview.chromium.org/1180443004/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:329: ['dumpsys', 'battery'], check_return=True))) This, however, should be indented another four spaces.
https://codereview.chromium.org/1180443004/diff/20001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1180443004/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:328: or not bool('UPDATES STOPPED' in self._device.RunShellCommand( On 2015/06/10 20:50:53, jbudorick wrote: > I'm not crazy about having this indented four spaces with the above paren > starting this statement, but I think it's legal. Moved it to be matching the opening ( https://codereview.chromium.org/1180443004/diff/20001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:329: ['dumpsys', 'battery'], check_return=True))) On 2015/06/10 20:50:53, jbudorick wrote: > This, however, should be indented another four spaces. Done.
lgtm
The CQ bit was checked by rnephew@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180443004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/06/10 23:19:33, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) > mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Failed because I didn't rebase, had to add large_ouput=True to test case. Recommiting.
The CQ bit was checked by rnephew@google.com
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/1180443004/#ps60001 (title: "rebase + fix test broken due to rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180443004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4fc81ca8441f541c2fceacde9ade58a3fa251e5b Cr-Commit-Position: refs/heads/master@{#333866} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
