Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
993733002:
Add new method to disable android device charging. (Closed)
|
Created:
5 years, 9 months ago by rnephew (Reviews Here) Modified:
5 years, 9 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd new method to disable android device charging.
This method makes the device think that it is not charging. This is useful
for getting battery data from dumpsys.
BUG=
Committed: https://crrev.com/b4490b70f969cab0598d75edc1572c5075c1453f
Cr-Commit-Position: refs/heads/master@{#320991}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 19
Patch Set 5 : #
Total comments: 10
Patch Set 6 : #
Total comments: 3
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Messages
Total messages: 39 (7 generated)
rnephew@google.com changed reviewers: + jbudorick@chromium.org, klundberg@chromium.org, rnephew@google.com
jbudorick@chromium.org changed reviewers: + perezju@chromium.org
+perezju, since he's also written a lot in device_utils.py https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:1419: if line != _DUMPSYS_BATTERY_DISABLED: If this line appears, is it the only thing printed? Will we see valid dumpsys information alongside this message? If this is the only thing printed, we should handle this differently. Also, exact text contents is liable to change between Android versions. This could flake if someone decides to make minor style changes to the line (e.g. switch to a 4 space indent). Can we change what we're looking for to account for things like that? https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:1445: def SoftSetCharging(self, enabled, timeout=None, retries=None): I don't think this should be separate from SetCharging. I think that we should always to this in SetCharging _and_ do the specific commands for N4/N5 if we're handling one of those devices. Also, whatever you add or change in here should be tested in device_utils_test.py.
https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:1419: if line != _DUMPSYS_BATTERY_DISABLED: On 2015/03/09 21:49:55, jbudorick wrote: > If this line appears, is it the only thing printed? Will we see valid dumpsys > information alongside this message? > > If this is the only thing printed, we should handle this differently. > > Also, exact text contents is liable to change between Android versions. This > could flake if someone decides to make minor style changes to the line (e.g. > switch to a 4 space indent). Can we change what we're looking for to account for > things like that? The output looks like this: Current Battery Service state: (UPDATES STOPPED -- use 'reset' to restart) AC powered: false USB powered: false Wireless powered: false status: 5 health: 2 present: true level: 100 scale: 100 voltage: 4317 temperature: 269 technology: Li-ion I can change it to a regular expression, or just have it skip lines that do not fit the label: value paradigm. https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:1445: def SoftSetCharging(self, enabled, timeout=None, retries=None): On 2015/03/09 21:49:55, jbudorick wrote: > I don't think this should be separate from SetCharging. I think that we should > always to this in SetCharging _and_ do the specific commands for N4/N5 if we're > handling one of those devices. > > Also, whatever you add or change in here should be tested in > device_utils_test.py. Done.
Basically: DeviceUtils is a very bad place to handle an unexpected condition silently. https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:1419: if line != _DUMPSYS_BATTERY_DISABLED: On 2015/03/09 23:58:09, rnephew wrote: > On 2015/03/09 21:49:55, jbudorick wrote: > > If this line appears, is it the only thing printed? Will we see valid dumpsys > > information alongside this message? > > > > If this is the only thing printed, we should handle this differently. > > > > Also, exact text contents is liable to change between Android versions. This > > could flake if someone decides to make minor style changes to the line (e.g. > > switch to a 4 space indent). Can we change what we're looking for to account > for > > things like that? > > The output looks like this: > Current Battery Service state: > (UPDATES STOPPED -- use 'reset' to restart) > AC powered: false > USB powered: false > Wireless powered: false > status: 5 > health: 2 > present: true > level: 100 > scale: 100 > voltage: 4317 > temperature: 269 > technology: Li-ion > I can change it to a regular expression, or just have it skip lines that do not > fit the label: value paradigm. Interesting, that's not what I expected. I don't think we should silently skip. I think we should handle three possibilities: - the line contains "UPDATES STOPPED": log a warning, as you had here before - the line fits the key: value paradigm, in which case we handle it as we are currently - neither, in which case we log a warning about an unrecognized line in the dumpsys output. https://codereview.chromium.org/993733002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1463: self._cache['charging_config'] = _DEFAULT_CHARGING_COMMANDS This isn't quite what I suggested in the last CL. I think we should do _both_ the default command _and_ the specific commands on N4 and N5, and just do the default command otherwise. This _only_ does the default command if there's no specific command.
https://codereview.chromium.org/993733002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1425: result[k.strip()] = v.strip() I think you should just log a warning on all skipped lines. https://codereview.chromium.org/993733002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1463: self._cache['charging_config'] = _DEFAULT_CHARGING_COMMANDS On 2015/03/10 00:04:43, jbudorick wrote: > This isn't quite what I suggested in the last CL. I think we should do _both_ > the default command _and_ the specific commands on N4 and N5, and just do the > default command otherwise. This _only_ does the default command if there's no > specific command. I'm not entirely sure I understand what these dumpsys enable/disable commands are supposed to do, or how do we plan to use them. From what I can gather, "set usb 0" will "freeze" the values reported by GetBatteryInfo until we do a "reset". Is that right? Why would we want to do this every time we disable charging? Wouldn't we usually want to disable charging but expect GetBatteryInfo to keep returning accurate values regarding, e.g., current charge levels?
https://codereview.chromium.org/993733002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1463: self._cache['charging_config'] = _DEFAULT_CHARGING_COMMANDS On 2015/03/10 09:46:09, perezju wrote: > On 2015/03/10 00:04:43, jbudorick wrote: > > This isn't quite what I suggested in the last CL. I think we should do _both_ > > the default command _and_ the specific commands on N4 and N5, and just do the > > default command otherwise. This _only_ does the default command if there's no > > specific command. > > I'm not entirely sure I understand what these dumpsys enable/disable commands > are supposed to do, or how do we plan to use them. > > From what I can gather, "set usb 0" will "freeze" the values reported by > GetBatteryInfo until we do a "reset". Is that right? > > Why would we want to do this every time we disable charging? Wouldn't we usually > want to disable charging but expect GetBatteryInfo to keep returning accurate > values regarding, e.g., current charge levels? AFAIK this actually affects charging to some degree as well (maybe only on specific OS versions, in which case we should tune this to those). rnephew may be able to explain in more detail...
On 2015/03/10 13:22:36, jbudorick wrote: > https://codereview.chromium.org/993733002/diff/20001/build/android/pylib/devi... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/993733002/diff/20001/build/android/pylib/devi... > build/android/pylib/device/device_utils.py:1463: self._cache['charging_config'] > = _DEFAULT_CHARGING_COMMANDS > On 2015/03/10 09:46:09, perezju wrote: > > On 2015/03/10 00:04:43, jbudorick wrote: > > > This isn't quite what I suggested in the last CL. I think we should do > _both_ > > > the default command _and_ the specific commands on N4 and N5, and just do > the > > > default command otherwise. This _only_ does the default command if there's > no > > > specific command. > > > > I'm not entirely sure I understand what these dumpsys enable/disable commands > > are supposed to do, or how do we plan to use them. > > > > From what I can gather, "set usb 0" will "freeze" the values reported by > > GetBatteryInfo until we do a "reset". Is that right? > > > > Why would we want to do this every time we disable charging? Wouldn't we > usually > > want to disable charging but expect GetBatteryInfo to keep returning accurate > > values regarding, e.g., current charge levels? > > AFAIK this actually affects charging to some degree as well (maybe only on > specific OS versions, in which case we should tune this to those). rnephew may > be able to explain in more detail... This document can give more information on why we want to do this. https://docs.google.com/document/d/1yAcZ4kkJxZkHM6UuYb2osK9kjEyJtvl0K73GUSyIs... But, it doesn't actually affect charging. It just makes the device think that its not charging. This causes dumpsys to start collecting battery information, since it collects power data from the last time it was charged.
https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/1/build/android/pylib/device/d... build/android/pylib/device/device_utils.py:1419: if line != _DUMPSYS_BATTERY_DISABLED: On 2015/03/10 00:04:43, jbudorick wrote: > On 2015/03/09 23:58:09, rnephew wrote: > > On 2015/03/09 21:49:55, jbudorick wrote: > > > If this line appears, is it the only thing printed? Will we see valid > dumpsys > > > information alongside this message? > > > > > > If this is the only thing printed, we should handle this differently. > > > > > > Also, exact text contents is liable to change between Android versions. This > > > could flake if someone decides to make minor style changes to the line (e.g. > > > switch to a 4 space indent). Can we change what we're looking for to account > > for > > > things like that? > > > > The output looks like this: > > Current Battery Service state: > > (UPDATES STOPPED -- use 'reset' to restart) > > AC powered: false > > USB powered: false > > Wireless powered: false > > status: 5 > > health: 2 > > present: true > > level: 100 > > scale: 100 > > voltage: 4317 > > temperature: 269 > > technology: Li-ion > > I can change it to a regular expression, or just have it skip lines that do > not > > fit the label: value paradigm. > > Interesting, that's not what I expected. > > I don't think we should silently skip. I think we should handle three > possibilities: > - the line contains "UPDATES STOPPED": log a warning, as you had here before > - the line fits the key: value paradigm, in which case we handle it as we are > currently > - neither, in which case we log a warning about an unrecognized line in the > dumpsys output. Done.
On 2015/03/10 13:27:25, rnephew wrote: > This document can give more information on why we want to do this. > https://docs.google.com/document/d/1yAcZ4kkJxZkHM6UuYb2osK9kjEyJtvl0K73GUSyIs... > > But, it doesn't actually affect charging. It just makes the device think that > its not charging. This causes dumpsys to start collecting battery information, > since it collects power data from the last time it was charged. After browsing through that document, I understand this will be basically a hack to make the higher level power manager *think* that the device is not charging, and start keeping track of the power consumed from that point onward, which is presumably what we want to measure. The low level kernel will not be affected, however, and the device will keep charging normally. All that seems fine, but then I *really* think this shouldn't be part of the commands to enable/disable charging; unless we expect *all* clients to be using these commands for that exact same measuring purpose. And even then, we might want to rename them to something like "StartBatteryMeasurement" or something like that. Otherwise, I don't see the need for these functions to even exist within device utils. The client implementing the battery measurements could run these with RunShellCommand and explain their rationale as needed.
On 2015/03/11 10:40:56, perezju wrote: > On 2015/03/10 13:27:25, rnephew wrote: > > This document can give more information on why we want to do this. > > > https://docs.google.com/document/d/1yAcZ4kkJxZkHM6UuYb2osK9kjEyJtvl0K73GUSyIs... > > > > But, it doesn't actually affect charging. It just makes the device think that > > its not charging. This causes dumpsys to start collecting battery information, > > since it collects power data from the last time it was charged. > > After browsing through that document, I understand this will be basically a hack > to make the higher level power manager *think* that the device is not charging, > and start keeping track of the power consumed from that point onward, which is > presumably what we want to measure. > > The low level kernel will not be affected, however, and the device will keep > charging normally. > > All that seems fine, but then I *really* think this shouldn't be part of the > commands to enable/disable charging; unless we expect *all* clients to be using > these commands for that exact same measuring purpose. And even then, we might > want to rename them to something like "StartBatteryMeasurement" or something > like that. > > Otherwise, I don't see the need for these functions to even exist within > device utils. The client implementing the battery measurements could run these > with RunShellCommand and explain their rationale as needed. IMHO, we should have functions specifically for this (in device utils or some other class) as we might use this in different context and having functions for this is much easier to maintain (also if the way we are doing this changes).
Noticed a few minor things when I looked at this today. https://codereview.chromium.org/993733002/diff/40001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1423: # If usb charging has been disabled, an extra line of header exists. I'm seeing double :-) Please remove one of the comments. https://codereview.chromium.org/993733002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1425: logging.warning('Dumpsys battery not recieving updates. ' s/recieving/receiving
On 2015/03/11 14:24:02, klundberg wrote: > On 2015/03/11 10:40:56, perezju wrote: > > On 2015/03/10 13:27:25, rnephew wrote: > > > This document can give more information on why we want to do this. > > > > > > https://docs.google.com/document/d/1yAcZ4kkJxZkHM6UuYb2osK9kjEyJtvl0K73GUSyIs... > > > > > > But, it doesn't actually affect charging. It just makes the device think > that > > > its not charging. This causes dumpsys to start collecting battery > information, > > > since it collects power data from the last time it was charged. > > > > After browsing through that document, I understand this will be basically a > hack > > to make the higher level power manager *think* that the device is not > charging, > > and start keeping track of the power consumed from that point onward, which is > > presumably what we want to measure. > > > > The low level kernel will not be affected, however, and the device will keep > > charging normally. > > > > All that seems fine, but then I *really* think this shouldn't be part of the > > commands to enable/disable charging; unless we expect *all* clients to be > using > > these commands for that exact same measuring purpose. And even then, we might > > want to rename them to something like "StartBatteryMeasurement" or something > > like that. > > > > Otherwise, I don't see the need for these functions to even exist within > > device utils. The client implementing the battery measurements could run these > > with RunShellCommand and explain their rationale as needed. Device utils is where disable charging is at, and it an action dealing with the state of the device; so I figured it was a good place to put this. Since its likely that we will like collect power data in more than one place, I don't like the idea of running it through RunShellCommand where its needed, in case the preferred method of enabling battery reporting changes in the future. > > IMHO, we should have functions specifically for this (in device utils or some > other class) as we might use this in different context and having functions for > this is much easier to maintain (also if the way we are doing this changes). It was in its own function before, I will put it back. I dont think John understood that it wouldn't really disable charging when he suggested that I merge the functions.
On 2015/03/11 14:52:12, rnephew wrote:
> On 2015/03/11 14:24:02, klundberg wrote:
> > IMHO, we should have functions specifically for this (in device utils or
some
> > other class) as we might use this in different context and having functions
> for
> > this is much easier to maintain (also if the way we are doing this changes).
>
> It was in its own function before, I will put it back. I dont think John
> understood that it wouldn't really disable charging when he suggested that I
> merge the functions.
Thanks makes sense. So, yes, we probably should have this function, but it
shouldn't be merged with SetCharging because it's not really enabling/disabling
usb charging.
Now, only a suggestion (and maybe not for this CL), but if the plan is to always
do something like:
"set usb 0", run test, measure, "reset"
then perhaps the right approach is to implement it as a context manager, so that
the "reset" part is always executed even if we abort due to exceptions.
On 2015/03/11 14:52:12, rnephew wrote: > On 2015/03/11 14:24:02, klundberg wrote: > > On 2015/03/11 10:40:56, perezju wrote: > > > On 2015/03/10 13:27:25, rnephew wrote: > > > > This document can give more information on why we want to do this. > > > > > > > > > > https://docs.google.com/document/d/1yAcZ4kkJxZkHM6UuYb2osK9kjEyJtvl0K73GUSyIs... > > > > > > > > But, it doesn't actually affect charging. It just makes the device think > > that > > > > its not charging. This causes dumpsys to start collecting battery > > information, > > > > since it collects power data from the last time it was charged. > > > > > > After browsing through that document, I understand this will be basically a > > hack > > > to make the higher level power manager *think* that the device is not > > charging, > > > and start keeping track of the power consumed from that point onward, which > is > > > presumably what we want to measure. > > > > > > The low level kernel will not be affected, however, and the device will keep > > > charging normally. > > > > > > All that seems fine, but then I *really* think this shouldn't be part of the > > > commands to enable/disable charging; unless we expect *all* clients to be > > using > > > these commands for that exact same measuring purpose. And even then, we > might > > > want to rename them to something like "StartBatteryMeasurement" or something > > > like that. > > > > > > Otherwise, I don't see the need for these functions to even exist within > > > device utils. The client implementing the battery measurements could run > these > > > with RunShellCommand and explain their rationale as needed. > > Device utils is where disable charging is at, and it an action dealing with the > state of the device; so I figured it was a good place to put this. Since its > likely that we will like collect power data in more than one place, I don't like > the idea of running it through RunShellCommand where its needed, in case the > preferred method of enabling battery reporting changes in the future. > The charging functions live in DeviceUtils primarily because of the caching functionality. If we had a way to expose that publicly, I'd probably extract the charging functions to some "BatteryUtils" class implemented on top of DeviceUtils. > > > > IMHO, we should have functions specifically for this (in device utils or some > > other class) as we might use this in different context and having functions > for > > this is much easier to maintain (also if the way we are doing this changes). > > It was in its own function before, I will put it back. I dont think John > understood that it wouldn't really disable charging when he suggested that I > merge the functions. Yeah, I misunderstood.
Made it into a context manager. https://codereview.chromium.org/993733002/diff/40001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1423: # If usb charging has been disabled, an extra line of header exists. On 2015/03/11 14:27:14, klundberg wrote: > I'm seeing double :-) > > Please remove one of the comments. Done. https://codereview.chromium.org/993733002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1425: logging.warning('Dumpsys battery not recieving updates. ' On 2015/03/11 14:27:14, klundberg wrote: > s/recieving/receiving Done.
https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:77: } I think this is no longer used, so you can remove it. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1482: @decorators.WithTimeoutAndRetriesFromInstance() Not sure, but maybe we should remove the timeout retries from here, so that timeouts and retries are handled by the individual RunShellCommand calls? John, what do you think of this? https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1485: """Enables or disables the power collection since last charge. Update the doc string, and maybe add a small of example of how the context manager would be used? https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1491: def set_and_verify_battery_measurement(): I think it will be cleaner/more readable (also in the logs) if you define two different functions "start_battery_measurement" and "finish_battery_measurement" which you then wait-for in the try-finally bellow. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1496: command = 'dumpsys batterystats --reset && dumpsys battery set usb 0' split this into two calls to RunShellCommand, and pass the command as a list of arguments (i.e. ['dumpsys', 'batterystats', '--reset'], etc.) https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1502: command = 'dumpsys battery reset' ditto, use a list of arguments instead of a string. A string is fine if you really need to use shell features, but otherwise a list of arguments is preferred.
https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:12: from contextlib import contextmanager import contextlib, then use @contextlib.contextmanager https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1482: @decorators.WithTimeoutAndRetriesFromInstance() On 2015/03/12 09:37:14, perezju wrote: > Not sure, but maybe we should remove the timeout retries from here, so that > timeouts and retries are handled by the individual RunShellCommand calls? John, > what do you think of this? I think I've never seen this decorator applied to a context manager :) I'm not actually sure how it would work, though if I had to guess, I'd say that it probably winds up running both __enter__ and __exit__ on (separate) TimeoutRetry threads. I think the issue with just letting RunShellCommand handle timeouts and retries is that the two WaitFor calls would no longer be inside TimeoutRetry threads, and in this case, I think we want them there. I think the context manager function should be dead simple -- something like: @contextlib.contextmanager def BatteryMeasurement(self): try: self._DisableBatteryUpdates() yield finally: self._EnableBatteryUpdates() and then have both of the helper functions decorated with the timeout/retries decorator. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1491: def set_and_verify_battery_measurement(): On 2015/03/12 09:37:14, perezju wrote: > I think it will be cleaner/more readable (also in the logs) if you define two > different functions "start_battery_measurement" and "finish_battery_measurement" > which you then wait-for in the try-finally bellow. agreed https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1492: self.RunShellCommand(command, check_return=True) Note that what you've done here with command is sneaky at best.
https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1482: @decorators.WithTimeoutAndRetriesFromInstance() On 2015/03/12 13:25:57, jbudorick wrote: > On 2015/03/12 09:37:14, perezju wrote: > > Not sure, but maybe we should remove the timeout retries from here, so that > > timeouts and retries are handled by the individual RunShellCommand calls? > John, > > what do you think of this? > > I think I've never seen this decorator applied to a context manager :) > > I'm not actually sure how it would work, though if I had to guess, I'd say that > it probably winds up running both __enter__ and __exit__ on (separate) > TimeoutRetry threads. > > I think the issue with just letting RunShellCommand handle timeouts and retries > is that the two WaitFor calls would no longer be inside TimeoutRetry threads, > and in this case, I think we want them there. > > I think the context manager function should be dead simple -- something like: > > @contextlib.contextmanager > def BatteryMeasurement(self): > try: > self._DisableBatteryUpdates() > yield > finally: > self._EnableBatteryUpdates() > > and then have both of the helper functions decorated with the timeout/retries > decorator. +1
I also made the context manager throw an exception if it is on a device that is < L and made it check that when it resets battery data, that power measurements read 0 before moving forward. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:12: from contextlib import contextmanager On 2015/03/12 13:25:57, jbudorick wrote: > import contextlib, then use @contextlib.contextmanager Done. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:77: } On 2015/03/12 09:37:14, perezju wrote: > I think this is no longer used, so you can remove it. Done. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1482: @decorators.WithTimeoutAndRetriesFromInstance() On 2015/03/12 13:43:26, perezju wrote: > On 2015/03/12 13:25:57, jbudorick wrote: > > On 2015/03/12 09:37:14, perezju wrote: > > > Not sure, but maybe we should remove the timeout retries from here, so that > > > timeouts and retries are handled by the individual RunShellCommand calls? > > John, > > > what do you think of this? > > > > I think I've never seen this decorator applied to a context manager :) > > > > I'm not actually sure how it would work, though if I had to guess, I'd say > that > > it probably winds up running both __enter__ and __exit__ on (separate) > > TimeoutRetry threads. > > > > I think the issue with just letting RunShellCommand handle timeouts and > retries > > is that the two WaitFor calls would no longer be inside TimeoutRetry threads, > > and in this case, I think we want them there. > > > > I think the context manager function should be dead simple -- something like: > > > > @contextlib.contextmanager > > def BatteryMeasurement(self): > > try: > > self._DisableBatteryUpdates() > > yield > > finally: > > self._EnableBatteryUpdates() > > > > and then have both of the helper functions decorated with the timeout/retries > > decorator. > > +1 Done. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1485: """Enables or disables the power collection since last charge. On 2015/03/12 09:37:14, perezju wrote: > Update the doc string, and maybe add a small of example of how the context > manager would be used? Done. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1491: def set_and_verify_battery_measurement(): On 2015/03/12 13:25:57, jbudorick wrote: > On 2015/03/12 09:37:14, perezju wrote: > > I think it will be cleaner/more readable (also in the logs) if you define two > > different functions "start_battery_measurement" and > "finish_battery_measurement" > > which you then wait-for in the try-finally bellow. > > agreed Also added a separate reset_battery_data because of a possible yet unlikely race condition where it resets the data and turns off power, but the device does not yet report that power is turned off and the timeout occurs and on subsequent retries it would check that power data is reset, but nothing would read 0 since its collecting data. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1492: self.RunShellCommand(command, check_return=True) On 2015/03/12 13:25:57, jbudorick wrote: > Note that what you've done here with command is sneaky at best. Acknowledged. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1496: command = 'dumpsys batterystats --reset && dumpsys battery set usb 0' On 2015/03/12 09:37:14, perezju wrote: > split this into two calls to RunShellCommand, and pass the command as a list of > arguments (i.e. ['dumpsys', 'batterystats', '--reset'], etc.) Done. https://codereview.chromium.org/993733002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1502: command = 'dumpsys battery reset' On 2015/03/12 09:37:14, perezju wrote: > ditto, use a list of arguments instead of a string. A string is fine if you > really need to use shell features, but otherwise a list of arguments is > preferred. Done.
https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1494: def _DisableBatteryUpdates(self, timeout=None, retries=None): Thanks, this is looking a lot better. I'm still a bit puzzled, however, about the intended interaction between the retries and the wait-fors. Do we really need to re-run the command to, say, disable usb while waiting for GetCharging to return False? In other words, I think we should be using WaitFor to wait for expected things to happen (if they don't tend to happen immediately), not to retry running commands when the expected thing doesn't seem to be happening. Maybe something like this would be better? @decorators.WithTimeoutAndRetriesFromInstance() def _DisableBatteryUpdates(self, ...): def battery_updates_disabled(): return self.GetCharging() is False self.RunShellCommand(...) # dumpsys batterystats --reset battery_data = # dumpsys batterystats --charged --checkin if some_non_zero_pwi_found: raise device_errors.CommandFailedError(...) self.RunShellCommand(...) # dumpsys battery reset timeout_retry.WaitFor(battery_updates_disabled, wait_period=1) This way we try to set up everything as needed once, and then just repeatedly query the device until charging becomes off. If everything works as expected, each command is run once and we're done. Now if something doesn't work as expected---one of the commands failed, we found a non_zero_pwi after resetting stats, or we waited for charging to become off but it didn't happen before the timeout---an exception will be raised, and the retry mechanism will try to re-run the whole sequence again. Does that make sense, or am I missing something? https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1505: self.RunShellCommand(['dumpsys', 'battery', 'reset'], check_return=True) ditto as above https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1514: nit: maybe the example would look a bit better with some annotations: Example usage: with device.BatteryMeasurement(): browser_actions() get_power_data() # reports usage within this block https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1524: if self.build_version_sdk < constants.ANDROID_SDK_VERSION_CODES.LOLLIPOP: nit: extra space after '<' https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils_test.py:1476: mock.ANY, retries=0, single_line=True, I'm never quite sure what is the "right" level of abstraction for these tests; but maybe we could change some of these mock.ANY's to the actual commands?
How would you guys feel about making the _Disable/EnableBatteryUpdates public, rather than private methods. We are going to try to switch telemetry to this way, but I do not think as its implemented now, it can use the context manager. I'm sure there are other places with similar issues as well. https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1494: def _DisableBatteryUpdates(self, timeout=None, retries=None): On 2015/03/13 10:19:06, perezju wrote: > Thanks, this is looking a lot better. I'm still a bit puzzled, however, about > the intended interaction between the retries and the wait-fors. Do we really > need to re-run the command to, say, disable usb while waiting for GetCharging to > return False? > > In other words, I think we should be using WaitFor to wait for expected things > to happen (if they don't tend to happen immediately), not to retry running > commands when the expected thing doesn't seem to be happening. > > Maybe something like this would be better? > > @decorators.WithTimeoutAndRetriesFromInstance() > def _DisableBatteryUpdates(self, ...): > def battery_updates_disabled(): > return self.GetCharging() is False > > self.RunShellCommand(...) # dumpsys batterystats --reset > battery_data = # dumpsys batterystats --charged --checkin > if some_non_zero_pwi_found: > raise device_errors.CommandFailedError(...) > self.RunShellCommand(...) # dumpsys battery reset > timeout_retry.WaitFor(battery_updates_disabled, wait_period=1) > > This way we try to set up everything as needed once, and then just repeatedly > query the device until charging becomes off. If everything works as expected, > each command is run once and we're done. > > Now if something doesn't work as expected---one of the commands failed, we found > a non_zero_pwi after resetting stats, or we waited for charging to become off > but it didn't happen before the timeout---an exception will be raised, and the > retry mechanism will try to re-run the whole sequence again. > > Does that make sense, or am I missing something? Done. https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1505: self.RunShellCommand(['dumpsys', 'battery', 'reset'], check_return=True) On 2015/03/13 10:19:07, perezju wrote: > ditto as above Done. https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1514: On 2015/03/13 10:19:06, perezju wrote: > nit: maybe the example would look a bit better with some annotations: > > Example usage: > > with device.BatteryMeasurement(): > browser_actions() > get_power_data() # reports usage within this block Done. https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:1524: if self.build_version_sdk < constants.ANDROID_SDK_VERSION_CODES.LOLLIPOP: On 2015/03/13 10:19:07, perezju wrote: > nit: extra space after '<' Done. https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/993733002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils_test.py:1476: mock.ANY, retries=0, single_line=True, On 2015/03/13 10:19:07, perezju wrote: > I'm never quite sure what is the "right" level of abstraction for these tests; > but maybe we could change some of these mock.ANY's to the actual commands? Done.
Thanks for the fixes. This lgtm with some nits. > How would you guys feel about making the _Disable/EnableBatteryUpdates public, > rather than private methods. I'm OK with that. But don't forget to add docstrings for them and maybe also a TODO to eventually turn them back into private when telemetry is able to use the context managed version. https://codereview.chromium.org/993733002/diff/100001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (left): https://codereview.chromium.org/993733002/diff/100001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:72: nit: this line went missing https://codereview.chromium.org/993733002/diff/100001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/100001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1513: # measurements are collected nit: I think these comments could be improved a bit. I understand that battery measurements within the context manager block will report usage relative to the start of the block ("as if" the device had stopped charging then); while battery measurements taken after exiting the block will revert to report live usage (and do not aggregate data that would otherwise be collected "since last charge"). Is this more or less accurate? Could you update the comments to better reflect the intended behavior?
https://codereview.chromium.org/993733002/diff/100001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/100001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1513: # measurements are collected On 2015/03/17 12:52:19, perezju wrote: > nit: I think these comments could be improved a bit. I understand that battery > measurements within the context manager block will report usage relative to the > start of the block ("as if" the device had stopped charging then); while battery > measurements taken after exiting the block will revert to report live usage (and > do not aggregate data that would otherwise be collected "since last charge"). > > Is this more or less accurate? Could you update the comments to better reflect > the intended behavior? Done.
The CQ bit was checked by rnephew@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org Link to the patchset: https://codereview.chromium.org/993733002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/993733002/120001
The CQ bit was unchecked by jbudorick@chromium.org
I pulled you out of the CQ because of the index error. https://codereview.chromium.org/993733002/diff/120001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/120001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1493: '--checkin'], check_return=True) nit: formatting. Can --checkin fit on the line above? https://codereview.chromium.org/993733002/diff/120001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1496: if l[3] == 'pwi' and l[1] != 0: This should check the length of the list returned by l.split before using the indices. The expected format should also be better documented here.
https://codereview.chromium.org/993733002/diff/120001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/120001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1493: '--checkin'], check_return=True) On 2015/03/17 20:03:51, jbudorick wrote: > nit: formatting. Can --checkin fit on the line above? Done. https://codereview.chromium.org/993733002/diff/120001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1496: if l[3] == 'pwi' and l[1] != 0: On 2015/03/17 20:03:51, jbudorick wrote: > This should check the length of the list returned by l.split before using the > indices. > > The expected format should also be better documented here. Done.
https://codereview.chromium.org/993733002/diff/140001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/140001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1498: if (len(l) >= PWI_POWER_INDEX and l[ROW_TYPE_INDEX] == 'pwi' nit: should be >, not >=
On 2015/03/17 20:39:31, jbudorick wrote: that is, lgtm w/ nit > https://codereview.chromium.org/993733002/diff/140001/build/android/pylib/dev... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/993733002/diff/140001/build/android/pylib/dev... > build/android/pylib/device/device_utils.py:1498: if (len(l) >= PWI_POWER_INDEX > and l[ROW_TYPE_INDEX] == 'pwi' > nit: should be >, not >=
https://codereview.chromium.org/993733002/diff/140001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/993733002/diff/140001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:1498: if (len(l) >= PWI_POWER_INDEX and l[ROW_TYPE_INDEX] == 'pwi' On 2015/03/17 20:39:31, jbudorick wrote: > nit: should be >, not >= Done.
The CQ bit was checked by rnephew@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/993733002/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/993733002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b4490b70f969cab0598d75edc1572c5075c1453f Cr-Commit-Position: refs/heads/master@{#320991} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
