[Android] Convert battery utilities to DeviceUtils.

[Android] Convert battery utilities to DeviceUtils.

BUG=459712
Committed: https://crrev.com/d3f50b0218788376ec29400561e5731754ce1020
Cr-Commit-Position: refs/heads/master@{#317843}

[jbudorick, 2015-02-20 17:22:26]

jbudorick@chromium.org changed reviewers:
+ navabi@chromium.org, perezju@chromium.org

[perezju, 2015-02-23 10:43:32]

Nice to see we're getting rid of more old_interface's :)

https://codereview.chromium.org/945883003/diff/1/build/android/provision_devi...
File build/android/provision_devices.py (right):

https://codereview.chromium.org/945883003/diff/1/build/android/provision_devi...
build/android/provision_devices.py:156: battery_level =
int(device.GetBatteryInfo().get('level', 100))
I see this was also on the previous version of the code, but why do we need a
default of 100? Do some devices not return the battery level? Should we at least
log a warning when this is the case?

https://codereview.chromium.org/945883003/diff/1/build/android/provision_devi...
build/android/provision_devices.py:193: device.SetUsbCharging(True)
There seems to be a change of logic w.r.t. previous version:

- we did not try to enable usb charging if either: the device was already
charging somehow (e.g. via AC or wireless), or we did not find suitable commands
to enable usb charging.
- we now try to enable usb charging regardless of the current state, and an
exception is raised if we're an unable to find usb charging commands.

Both changes seem reasonable to me, just wanted to make sure that these are
indeed intended (and we don't expect to see devices where the required usb
commands are not available).

https://codereview.chromium.org/945883003/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/945883003/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1414: for line in
self.RunShellCommand(['dumpsys', 'battery'])[1:]:
check_return=True?

https://codereview.chromium.org/945883003/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1442: if 'usb_charging' not in
self._cache:
should we call it usb_charging_config or usb_charging_commands? First I thought
this was going to cache the current enabled/disabled status of usb charging.

https://codereview.chromium.org/945883003/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1456: while self.GetUsbCharging() !=
enabled:
This seems a bit blunt. Should we use WaitFor?

[jbudorick, 2015-02-24 15:44:53]

https://codereview.chromium.org/945883003/diff/1/build/android/provision_devi...
File build/android/provision_devices.py (right):

https://codereview.chromium.org/945883003/diff/1/build/android/provision_devi...
build/android/provision_devices.py:156: battery_level =
int(device.GetBatteryInfo().get('level', 100))
On 2015/02/23 10:43:32, perezju wrote:
> I see this was also on the previous version of the code, but why do we need a
> default of 100? Do some devices not return the battery level? Should we at
least
> log a warning when this is the case?

I'm not sure when or why the default was added, but I imagine it was something
along those lines. I've added a warning in case battery_level isn't available.

https://codereview.chromium.org/945883003/diff/1/build/android/provision_devi...
build/android/provision_devices.py:193: device.SetUsbCharging(True)
On 2015/02/23 10:43:32, perezju wrote:
> There seems to be a change of logic w.r.t. previous version:
> 
> - we did not try to enable usb charging if either: the device was already
> charging somehow (e.g. via AC or wireless), or we did not find suitable
commands
> to enable usb charging.
> - we now try to enable usb charging regardless of the current state, and an
> exception is raised if we're an unable to find usb charging commands.
> 
> Both changes seem reasonable to me, just wanted to make sure that these are
> indeed intended (and we don't expect to see devices where the required usb
> commands are not available).

These changes were both intentional, but on reconsidering (and running locally
with an N7), it's probably better to have provision_devices catch and log the
exception if we can't enable charging. After all, if we don't have the commands,
charging probably wasn't disabled in the first place.

https://codereview.chromium.org/945883003/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/945883003/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1414: for line in
self.RunShellCommand(['dumpsys', 'battery'])[1:]:
On 2015/02/23 10:43:32, perezju wrote:
> check_return=True?

Nice catch. Done.

https://codereview.chromium.org/945883003/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1442: if 'usb_charging' not in
self._cache:
On 2015/02/23 10:43:32, perezju wrote:
> should we call it usb_charging_config or usb_charging_commands? First I
thought
> this was going to cache the current enabled/disabled status of usb charging.

Ah, hadn't thought of that. Switched to 'usb_charging_config'.

https://codereview.chromium.org/945883003/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1456: while self.GetUsbCharging() !=
enabled:
On 2015/02/23 10:43:32, perezju wrote:
> This seems a bit blunt. Should we use WaitFor?

Ha, it's certainly more than a bit blunt.

Switched to WaitFor.

[perezju, 2015-02-24 15:54:33]

lgtm, thanks for the changes!

[perezju, 2015-02-24 15:56:55]

On 2015/02/24 15:54:33, perezju wrote:
> lgtm, thanks for the changes!

That is, as long as the trybot passes. Funny, we stumbled against the:
"KeyError: 'temperature'" from bug 455216?

[jbudorick, 2015-02-24 16:00:31]

On 2015/02/24 15:56:55, perezju wrote:
> On 2015/02/24 15:54:33, perezju wrote:
> > lgtm, thanks for the changes!
> 
> That is, as long as the trybot passes. Funny, we stumbled against the:
> "KeyError: 'temperature'" from bug 455216?

that's in bb_device_status_check, which was not changed in this patch (although
it does rely on the battery data, obviously)

[jbudorick, 2015-02-24 16:25:07]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-02-24 16:25:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/945883003/20001

[commit-bot: I haz the power, 2015-02-24 18:52:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-24 18:53:26]

Patchset 2 (id:??) landed as
https://crrev.com/d3f50b0218788376ec29400561e5731754ce1020
Cr-Commit-Position: refs/heads/master@{#317843}