When Autophone overworks a device due to rapid fire jobs, it can drain the device battery even though the device is charged via usb. Autophone's worker.py should check the device's battery status and give it a break if the battery level drops too low. $ adb -s 015d2109f533fa13 shell 'dumpsys battery' Current Battery Service state: AC powered: false USB powered: true Wireless powered: false status: 2 health: 2 present: true level: 39 scale: 100 voltage:3802 temperature: 290 technology: Li-ion
Created attachment 8453844 [details] [diff] [review] bug-1031008-battery-status.patch Mark, this incorporates the changes to adb.py already checked into mozdevice in Bug 1035254 with one addition of removing the extraneous pid = None in process_exist.
Attachment #8453844 - Flags: review?(mcote)
Comment on attachment 8453844 [details] [diff] [review] bug-1031008-battery-status.patch Review of attachment 8453844 [details] [diff] [review]: ----------------------------------------------------------------- This is great, thanks. I'm going to r+ it because it should work as is, but I have one comment for your consideration. ::: worker.py @@ +394,5 @@ > + self.phone_cfg['phoneid'], > + phonetest.PhoneTestMessage.CHARGING, > + build_metadata['blddate'], > + repo)) > + time.sleep(900) I think it might be better to put this after a job runs instead of before. If we're reasonably sure that our tests are the only things that can trigger battery drain, then recharging after a test means that we can potentially use idle time, whereas if we run it before, we'll always end up delaying a job if the battery is low.
Attachment #8453844 - Flags: review?(mcote) → review+
Good point. I think the original idea was to make sure the battery was charged before the test, but I don't think that is that big of a concern and using potential idle time for charging is a good use. In practical terms, the batteries only appear to drain quickly when doing a large number of builds, particularly when retesting ranges where we already have some results since we can do installs then find we already have results and bail to the next job. So in high drain scenarios there won't be much idle time anyway. But I agree and will make the change. Thanks!
Hmmm. Looking at how I would do this means wrapping the call to run_tests in a try...finally and placing the battery check and delay in the finally clause. I need to do that in order to handle the case where an uncaught exception might terminate the test (such as an ADBTimeout). We wouldn't want a recurring exception to prevent the battery from recharging. But if I do that, it could delay the reporting of the error for several hours while the device recharges.
I went ahead and pushed the original patch but will leave this open for a follow up to better use idle time for charging. I wanted to get the b2g-inbound stuff landed sooner than later. https://github.com/mozilla/autophone/commit/50538162ab3962706b98a0e9db0606ffce977009
(In reply to Bob Clary [:bc:] from comment #4) > Hmmm. Looking at how I would do this means wrapping the call to run_tests in > a try...finally and placing the battery check and delay in the finally > clause. I need to do that in order to handle the case where an uncaught > exception might terminate the test (such as an ADBTimeout). We wouldn't want > a recurring exception to prevent the battery from recharging. But if I do > that, it could delay the reporting of the error for several hours while the > device recharges. Charging at the end doesn't really change that, does it? If we charge after every test, and idle periods don't drain the battery, then the battery should be roughly full at the beginning of each test run (at least above the min value), as it would be if we charged before the test. The only problem then would be if a single test took the battery from roughly full to absolutely zero, but that could happen even if we kept the charging section at the top of the test.
Regardless, yeah, this can be done (if we decide to) as a follow-up.
Mark and I talked about this a bit and decided that the idle phone would be charging anyway and that making sure uncaught exceptions in the test do not cause an 'infinite restart' which would drain the battery is also important. I'll go ahead and mark this fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.