Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1672543002:
[Android] Detect if device is not charging properly. (Closed)
|
Created:
4 years, 10 months ago by rnephew (Reviews Here) Modified:
4 years, 10 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+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] Detect if device is not charging properly.
Reboot device during provisioning if it is not charging correctly.
After rebooting if it is still not charging correctly, blacklist.
BUG=567786
Committed: https://crrev.com/0204d425fccda3e68040c5937bf18f2e919bd188
Cr-Commit-Position: refs/heads/master@{#375007}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Messages
Total messages: 24 (4 generated)
rnephew@chromium.org changed reviewers: + dtu@chromium.org, jbudorick@chromium.org, mikecase@chromium.org
https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... build/android/devil/android/battery_utils.py:443: def ChargeDeviceToLevel(self, level, wait_period=60, reboot_count=10, this interface is a bit convoluted, and I think it makes BatteryUtils responsible for too much. Instead, wdyt about the following: - make ChargeDeviceToLevel responsible for detecting whether a device's charge level has reached a steady state below the desired level. If so, raise an exception. - make the caller implement the reboot / blacklist functionality.
https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... build/android/devil/android/battery_utils.py:443: def ChargeDeviceToLevel(self, level, wait_period=60, reboot_count=10, On 2016/02/04 23:38:21, jbudorick wrote: > this interface is a bit convoluted, and I think it makes BatteryUtils > responsible for too much. Instead, wdyt about the following: > - make ChargeDeviceToLevel responsible for detecting whether a device's charge > level has reached a steady state below the desired level. If so, raise an > exception. > - make the caller implement the reboot / blacklist functionality. Something I do not like about that is that the calling site doesnt have context of what is going on in ChargeDeviceToLevel. It would make it hard for the case where it returns 50, 50, 50, reboot, 60, 60, 60, reboot, 70, 70, 70, reboot. If the reboot/blacklist logic is delegated to the calling site I'm afraid it would go more like: 50, 50, 50, reboot, 60, 60, 60, blacklist ChargeDeviceToLevel could be refactored to return the amount of current charge though.
On 2016/02/04 23:43:56, rnephew1 wrote: > https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... > File build/android/devil/android/battery_utils.py (right): > > https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... > build/android/devil/android/battery_utils.py:443: def ChargeDeviceToLevel(self, > level, wait_period=60, reboot_count=10, > On 2016/02/04 23:38:21, jbudorick wrote: > > this interface is a bit convoluted, and I think it makes BatteryUtils > > responsible for too much. Instead, wdyt about the following: > > - make ChargeDeviceToLevel responsible for detecting whether a device's > charge > > level has reached a steady state below the desired level. If so, raise an > > exception. > > - make the caller implement the reboot / blacklist functionality. > > Something I do not like about that is that the calling site doesnt have context > of what is going on in ChargeDeviceToLevel. > > It would make it hard for the case where it returns > > 50, 50, 50, reboot, 60, 60, 60, reboot, 70, 70, 70, reboot. > If the reboot/blacklist logic is delegated to the calling site I'm afraid it > would go more like: > 50, 50, 50, reboot, 60, 60, 60, blacklist > ChargeDeviceToLevel could be refactored to return the amount of current charge > though. Actually that wouldn't work, since it would be raising an exception; not returning.
https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... build/android/devil/android/battery_utils.py:443: def ChargeDeviceToLevel(self, level, wait_period=60, reboot_count=10, On 2016/02/04 23:43:56, rnephew1 wrote: > On 2016/02/04 23:38:21, jbudorick wrote: > > this interface is a bit convoluted, and I think it makes BatteryUtils > > responsible for too much. Instead, wdyt about the following: > > - make ChargeDeviceToLevel responsible for detecting whether a device's > charge > > level has reached a steady state below the desired level. If so, raise an > > exception. > > - make the caller implement the reboot / blacklist functionality. > > Something I do not like about that is that the calling site doesnt have context > of what is going on in ChargeDeviceToLevel. Hopefully, it can stay that way :) > > It would make it hard for the case where it returns > > 50, 50, 50, reboot, 60, 60, 60, reboot, 70, 70, 70, reboot. > If the reboot/blacklist logic is delegated to the calling site I'm afraid it > would go more like: > 50, 50, 50, reboot, 60, 60, 60, blacklist > ChargeDeviceToLevel could be refactored to return the amount of current charge > though. I'm fine with the latter behavior and would certainly prefer starting there.
On 2016/02/04 23:46:08, jbudorick wrote: > https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... > File build/android/devil/android/battery_utils.py (right): > > https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... > build/android/devil/android/battery_utils.py:443: def ChargeDeviceToLevel(self, > level, wait_period=60, reboot_count=10, > On 2016/02/04 23:43:56, rnephew1 wrote: > > On 2016/02/04 23:38:21, jbudorick wrote: > > > this interface is a bit convoluted, and I think it makes BatteryUtils > > > responsible for too much. Instead, wdyt about the following: > > > - make ChargeDeviceToLevel responsible for detecting whether a device's > > charge > > > level has reached a steady state below the desired level. If so, raise an > > > exception. > > > - make the caller implement the reboot / blacklist functionality. > > > > Something I do not like about that is that the calling site doesnt have > context > > of what is going on in ChargeDeviceToLevel. > > Hopefully, it can stay that way :) > > > > > It would make it hard for the case where it returns > > > > 50, 50, 50, reboot, 60, 60, 60, reboot, 70, 70, 70, reboot. > > If the reboot/blacklist logic is delegated to the calling site I'm afraid it > > would go more like: > > 50, 50, 50, reboot, 60, 60, 60, blacklist > > ChargeDeviceToLevel could be refactored to return the amount of current charge > > though. > > I'm fine with the latter behavior and would certainly prefer starting there. re charge level: I imagine that, if we detect steady state, the exception that gets thrown will be of a type of your creation (vs, say, Exception or plain CommandFailedError or what have you). In that case, you can include the charge level.
https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... build/android/devil/android/battery_utils.py:450: reboot_count: Number of times checking charge level where it goes down in my opinion these values should be in seconds and not in counts (you could even immediately convert them by like reboot_count = reboot_period/wait_period or something). And if you take what John said you could replace "reboot_count" and "blacklist_count" with like "charge_failure_wait_period" it really doesnt matter very much though, like at all
https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... build/android/devil/android/battery_utils.py:450: reboot_count: Number of times checking charge level where it goes down On 2016/02/04 23:57:19, mikecase wrote: > in my opinion these values should be in seconds and not in counts (you could > even immediately convert > them by like reboot_count = reboot_period/wait_period or something). And if you > take what John said > you could replace "reboot_count" and "blacklist_count" with like > "charge_failure_wait_period" I wouldn't make the steady-state determination user-configurable at all. > > it really doesnt matter very much though, like at all
https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... build/android/devil/android/battery_utils.py:443: def ChargeDeviceToLevel(self, level, wait_period=60, reboot_count=10, On 2016/02/04 23:46:08, jbudorick wrote: > On 2016/02/04 23:43:56, rnephew1 wrote: > > On 2016/02/04 23:38:21, jbudorick wrote: > > > this interface is a bit convoluted, and I think it makes BatteryUtils > > > responsible for too much. Instead, wdyt about the following: > > > - make ChargeDeviceToLevel responsible for detecting whether a device's > > charge > > > level has reached a steady state below the desired level. If so, raise an > > > exception. > > > - make the caller implement the reboot / blacklist functionality. > > > > Something I do not like about that is that the calling site doesnt have > context > > of what is going on in ChargeDeviceToLevel. > > Hopefully, it can stay that way :) > > > > > It would make it hard for the case where it returns > > > > 50, 50, 50, reboot, 60, 60, 60, reboot, 70, 70, 70, reboot. > > If the reboot/blacklist logic is delegated to the calling site I'm afraid it > > would go more like: > > 50, 50, 50, reboot, 60, 60, 60, blacklist > > ChargeDeviceToLevel could be refactored to return the amount of current charge > > though. > > I'm fine with the latter behavior and would certainly prefer starting there. Done. https://codereview.chromium.org/1672543002/diff/1/build/android/devil/android... build/android/devil/android/battery_utils.py:450: reboot_count: Number of times checking charge level where it goes down On 2016/02/04 23:59:11, jbudorick wrote: > On 2016/02/04 23:57:19, mikecase wrote: > > in my opinion these values should be in seconds and not in counts (you could > > even immediately convert > > them by like reboot_count = reboot_period/wait_period or something). And if > you > > take what John said > > you could replace "reboot_count" and "blacklist_count" with like > > "charge_failure_wait_period" > > I wouldn't make the steady-state determination user-configurable at all. > > > > > it really doesnt matter very much though, like at all > Done.
https://codereview.chromium.org/1672543002/diff/40001/build/android/devil/and... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/40001/build/android/devil/and... build/android/devil/android/battery_utils.py:145: self._charge_failure_count = 0 These two variables are local to ChargeDeviceToLevel (though they aren't local to a thread) and should be handled there. https://codereview.chromium.org/1672543002/diff/40001/build/android/devil/and... File build/android/devil/android/device_errors.py (right): https://codereview.chromium.org/1672543002/diff/40001/build/android/devil/and... build/android/devil/android/device_errors.py:118: nit: +1 blank line https://codereview.chromium.org/1672543002/diff/40001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1672543002/diff/40001/build/android/provision... build/android/provision_devices.py:135: except device_errors.CommandFailedError: Not catching the exception below will result in the device getting blacklisted here. https://codereview.chromium.org/1672543002/diff/40001/build/android/provision... build/android/provision_devices.py:347: except device_errors.DeviceChargingError: We shouldn't catch this or handle the blacklist here. https://codereview.chromium.org/1672543002/diff/40001/build/android/provision... build/android/provision_devices.py:351: except device_errors.CommandFailedError: We shouldn't catch this either.
https://codereview.chromium.org/1672543002/diff/40001/build/android/devil/and... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/40001/build/android/devil/and... build/android/devil/android/battery_utils.py:145: self._charge_failure_count = 0 On 2016/02/05 01:40:43, jbudorick wrote: > These two variables are local to ChargeDeviceToLevel (though they aren't local > to a thread) and should be handled there. Switched to dict so it can be handled locally. https://codereview.chromium.org/1672543002/diff/40001/build/android/devil/and... File build/android/devil/android/device_errors.py (right): https://codereview.chromium.org/1672543002/diff/40001/build/android/devil/and... build/android/devil/android/device_errors.py:118: On 2016/02/05 01:40:43, jbudorick wrote: > nit: +1 blank line Done. https://codereview.chromium.org/1672543002/diff/40001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1672543002/diff/40001/build/android/provision... build/android/provision_devices.py:135: except device_errors.CommandFailedError: On 2016/02/05 01:40:43, jbudorick wrote: > Not catching the exception below will result in the device getting blacklisted > here. Acknowledged. https://codereview.chromium.org/1672543002/diff/40001/build/android/provision... build/android/provision_devices.py:347: except device_errors.DeviceChargingError: On 2016/02/05 01:40:43, jbudorick wrote: > We shouldn't catch this or handle the blacklist here. Done. https://codereview.chromium.org/1672543002/diff/40001/build/android/provision... build/android/provision_devices.py:351: except device_errors.CommandFailedError: On 2016/02/05 01:40:43, jbudorick wrote: > We shouldn't catch this either. Done.
Ping. https://build.chromium.org/p/chromium.perf/builders/Android%20Nexus6%20Perf%2... is consistently getting stuck.
lgtm with two nits/comments. https://codereview.chromium.org/1672543002/diff/60001/build/android/devil/and... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/60001/build/android/devil/and... build/android/devil/android/battery_utils.py:466: charge_status['charge_failure_count'] += 1 This would make more sense inside an "else" clause I think. https://codereview.chromium.org/1672543002/diff/60001/build/android/devil/and... build/android/devil/android/battery_utils.py:469: raise device_errors.DeviceChargingError('Device not charging properly.') I would include the charge level inside the error message. Or even more data, like a list of previous charge values or something. Could be helpful in debugging.
https://codereview.chromium.org/1672543002/diff/60001/build/android/devil/and... File build/android/devil/android/battery_utils.py (right): https://codereview.chromium.org/1672543002/diff/60001/build/android/devil/and... build/android/devil/android/battery_utils.py:466: charge_status['charge_failure_count'] += 1 On 2016/02/10 16:46:20, mikecase wrote: > This would make more sense inside an "else" clause I think. Done. https://codereview.chromium.org/1672543002/diff/60001/build/android/devil/and... build/android/devil/android/battery_utils.py:469: raise device_errors.DeviceChargingError('Device not charging properly.') On 2016/02/10 16:46:20, mikecase wrote: > I would include the charge level inside the error message. Or even > more data, like a list of previous charge values or something. > > Could be helpful in debugging. Done.
https://codereview.chromium.org/1672543002/diff/80001/build/android/devil/and... File build/android/devil/android/battery_utils_test.py (right): https://codereview.chromium.org/1672543002/diff/80001/build/android/devil/and... build/android/devil/android/battery_utils_test.py:266: battery_utils._MAX_CHARGE_ERROR = old_max If your test passes, _MAX_CHARGE_ERROR won't be reset. https://codereview.chromium.org/1672543002/diff/80001/build/android/devil/and... build/android/devil/android/battery_utils_test.py:279: battery_utils._MAX_CHARGE_ERROR = old_max same
https://codereview.chromium.org/1672543002/diff/80001/build/android/devil/and... File build/android/devil/android/battery_utils_test.py (right): https://codereview.chromium.org/1672543002/diff/80001/build/android/devil/and... build/android/devil/android/battery_utils_test.py:266: battery_utils._MAX_CHARGE_ERROR = old_max On 2016/02/11 19:05:44, jbudorick wrote: > If your test passes, _MAX_CHARGE_ERROR won't be reset. Done. https://codereview.chromium.org/1672543002/diff/80001/build/android/devil/and... build/android/devil/android/battery_utils_test.py:279: battery_utils._MAX_CHARGE_ERROR = old_max On 2016/02/11 19:05:44, jbudorick wrote: > same Done.
lgtm
The CQ bit was checked by rnephew@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/1672543002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1672543002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672543002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Detect if device is not charging properly. Reboot device during provisioning if it is not charging correctly. After rebooting if it is still not charging correctly, blacklist. BUG=567786 ========== to ========== [Android] Detect if device is not charging properly. Reboot device during provisioning if it is not charging correctly. After rebooting if it is still not charging correctly, blacklist. BUG=567786 Committed: https://crrev.com/0204d425fccda3e68040c5937bf18f2e919bd188 Cr-Commit-Position: refs/heads/master@{#375007} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0204d425fccda3e68040c5937bf18f2e919bd188 Cr-Commit-Position: refs/heads/master@{#375007} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
