Added battery not low and storage not low as download requirements #7138
Conversation
… Requirements Added monitoring battery and storage levels in RequirementsWatcher Added dependency on extension-workmanager to the demo app to be able to test with WorkManagerScheduler
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
I don't see a signed CLA on our side (for yourself, or even for your company as a whole). If you're stuck, please let us know. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@ojw28 Seems like the signing had not been processed yet, good to go now! Thanks |
| @@ -835,6 +837,30 @@ private static boolean needsStartedService(@Download.State int state) { | |||
| || state == Download.STATE_RESTARTING; | |||
| } | |||
|
|
|||
| private Requirements validateRequirements(Requirements requirements) { | |||
There was a problem hiding this comment.
I think Scheduler needs to expose a proper API for this, since you're currently only handling one particular case (e.g., the deprecated JobDispatcherScheduler isn't handled correctly, and neither are custom Scheduler implementations).
You could have, for example, a Requirements Scheduler.getSupportedRequirements(Requirements) that returns the argument if everything is supported, or a new Requirements containing the supported subset otherwise. The caller can see what's changed using Requirements.getRequirements on the original & return objects.
There was a problem hiding this comment.
Got it, that seems like a better approach, will change.
| * Implementation taken from the the WorkManager source. | ||
| * @see <a href="https://android.googlesource.com/platform/frameworks/support/+/androidx-master-dev/work/workmanager/src/main/java/androidx/work/impl/constraints/trackers/BatteryNotLowTracker.java">BatteryNotLowTracker</a> | ||
| */ | ||
| private boolean isBatteryNotLow(Context context) { |
There was a problem hiding this comment.
The extent this is decoupled from RequirementsWatcher makes me pretty uncomfortable. If ACTION_BATTERY_LOW and ACTION_BATTERY_OK aren't fired at exactly the right time either side of the 15% magic battery percentage value, then the RequirementsWatcher isn't going to re-evaluate the Requirements correctly, and there wont necessarily be another intent fired to cause re-evaluation after that. The file you reference is interesting in that it effectively lets the ACTION_BATTERY_LOW and ACTION_BATTERY_OK intents override what the ACTION_BATTERY_CHANGED intent says, which is only used for an initial evaluation.
How thoroughly did you test this, and are you certain it's always going to work? Is it possible to use ACTION_BATTERY_LOW and ACTION_BATTERY_OK for the evaluation, rather than ACTION_BATTERY_CHANGED? I'm guessing not because they don't appear to be sticky, but please double check.
As a more general point, I think we should probably move evaluation of the requirements out of Requirements and into RequirementsWatcher, to let us do things like modify the evaluated state when we receive ACTION_BATTERY_LOW and ACTION_BATTERY_OK, rather than just having them trigger a re-evaluation.
Depending on how confident you are about the battery part of this change, I see two paths forward here:
- Scope this change down to just cover the storage part. Then refactor as suggested above as a second change. Then add the battery part as a third change.
- Submit this with both storage and battery parts, and refactor as suggested afterwards.
There was a problem hiding this comment.
If it happens infrequently, it may be acceptable to use ACTION_BATTERY_CHANGED in RequirementsWatcher to get them aligned. This presumably also solves the problem below about the plugged state, since I'm guessing that is fired when the plugged state changes?
There was a problem hiding this comment.
I will retest the battery part more thoroughly, I've been testing on emulators and the Intents fired consistently between ACTION_BATTERY_LOW and ACTION_BATTERY_OK. What I did not test is a device where the battery low percentage is not 15% (my personal device starts warning at 20% for example). Good idea to look into the frequency of ACTION_BATTERY_CHANGED as well, will get back to you on the battery part.
I'd prefer to submit the storage and battery changes, including refactor, as one change if that's OK with you.
There was a problem hiding this comment.
One change sounds fine provided you can get the battery part to a state where it looks robust, without the need to additional refactoring. If additional refactoring is needed then we should start splitting it into smaller changes, which are easier to review and merge.
There was a problem hiding this comment.
Did some more thorough testing on the battery part and have the following findings:
Is it possible to use ACTION_BATTERY_LOW and ACTION_BATTERY_OK for the evaluation, rather than ACTION_BATTERY_CHANGED?
This is indeed not possible as ACTION_BATTERY_LOW and ACTION_BATTERY_OK don't contain the intent extras needed for the evaluation.
If it happens infrequently, it may be acceptable to use ACTION_BATTERY_CHANGED in RequirementsWatcher to get them aligned
ACTION_BATTERY_CHANGED fires on every "tick" of the battery level, as well as on connect and disconnect from a power source as you expected. The intent does fire more often but it is a more robust solution as ACTION_BATTERY_LOW and ACTION_BATTERY_OK seem to be device specific with respect to when they fire. For example on another test device the battery is only considered low when there's 10% left. Additionally I've seen some instances where ACTION_BATTERY_OK doesn't fire immediately after the battery level goes above the low threshold. If we use ACTION_BATTERY_CHANGED the behaviour will be consistent across devices using the same magic value we choose as the low threshold.
| int level = intent.getIntExtra(BatteryManager.EXTRA_LEVEL, -1); | ||
| int scale = intent.getIntExtra(BatteryManager.EXTRA_SCALE, -1); | ||
| float batteryPercentage = level / (float) scale; | ||
| return (plugged != BATTERY_PLUGGED_NONE |
There was a problem hiding this comment.
If this is part of the logic, you'll want RequirementsWatcher to trigger a re-evaluation when the battery is plugged or unplugged. I can't see anything that does this, however, unless ACTION_BATTERY_OKAY and ACTION_BATTERY_LOW are fired when the battery is plugged or unplugged when under 15% (if this does happen, it doesn't appear to be documented in the Android Javadoc).
Do you need to change if (requirements.isChargingRequired()) { on L95 of RequirementsWatcher to also evaluate to true if isBatteryNotLowRequired?
If this is a bug, it's probably also a bug in the referenced BatteryNotLowTracker class?
There was a problem hiding this comment.
You're right that this indeed seems to be a bug in the BatteryNotLowTracker as there is no re-evaluation after a device is connected to power. Changing the intent filter to use ACTION_BATTERY_CHANGED fixed this issue.
There was a problem hiding this comment.
I spoke to the maintainers of BatteryNotLowTracker and they don't think looking at plugged should have been part of the evaluation at all, and so have removed it.
There remains an additional issue that by using ACTION_BATTERY_CHANGED for the initial evaluation, but using ACTION_BATTERY_OKAY and ACTION_BATTERY_LOW for detecting state changes, it's possible that BatteryNotLowTracker can end up out of sync with the true battery state if the underlying platform uses some constant other than 15% to fire the latter two intents. Furthermore, they cannot use ACTION_BATTERY_CHANGED because it can't be used with a broadcast receiver, and they cannot use the other intents for the initial evaluation because they're not sticky. It seems like a platform change would be needed to solve this problem robustly.
Given this, do we want to add support for battery-not-low at all? It's not supported by two of the ExoPlayer Scheduler implementations, and we think that the third one does not support it in a robust way, and that it's unlikely to be possible for that to be fixed short of a new Android platform release. So it feels we're adding a feature that we know is not robust under any circumstance...
There was a problem hiding this comment.
Furthermore, they cannot use ACTION_BATTERY_CHANGED because it can't be used with a broadcast receiver
Could you elaborate on this point a bit more? Based on the documentation for ACTION_BATTERY_CHANGED it seems it can only not be used to register in the manifest, but can be explicitly registered for (as also done in the revised RequirementsWatcher. But maybe I'm missing something here.
Additionally, are there plans to remove support for battery not low for the WorkManager as well?
Given this, do we want to add support for battery-not-low at all?
As you say there's no robust solution for battery not low, so it seems reasonable to remove support for it until the necessary platform changes are made. Shall I close this PR and reopen a new one with just the storage not low requirement?
There was a problem hiding this comment.
Could you elaborate on this point a bit more? Based on the documentation for ACTION_BATTERY_CHANGED it seems it can only not be used to register in the manifest, but can be explicitly registered for (as also done in the revised RequirementsWatcher. But maybe I'm missing something here.
Right, sorry, that wasn't clear. What I meant is that they can't listen register a receiver in the manifest, which is what they need to do.
Additionally, are there plans to remove support for battery not low for the WorkManager as well?
There's an active discussion happening now about what to do about work manager. From the latest update it seems the 15% magic value for firing ACTION_BATTERY_LOW and ACTION_BATTERY_OK may be enforced by a device certification (CTS) test. If that's the case, their solution will probably be robust after all, once the plugged check has been removed. It still seems worth trying to validate this across some different devices, to make sure. I know you mentioned you have a test device where battery is only considered low when there's 10% left. Were you seeing ACTION_BATTERY_LOW and ACTION_BATTERY_OK fired at 10%, or were they still fired at 15% (as we'd expect if there's a device certification test) and was this just the phone UI using a different value?
If we're able to validate the 15% magic value across different devices, including the one where you saw 10% being used for something, then it seems OK that we would add support in ExoPlayer. We will at least need to wait until a new release of workmanager to pick up the change that removes the plugged check though, and also remove it from the ExoPlayer code to keep the two in sync.
Apologies for the gradual sharing of new information; the discussions internally have been ongoing, so I'm just sharing things as I find out about them.
In terms of next steps:
- The storage-not-low part is non-controversial. If there was a pull request for just that part, we could easily merge it.
- For battery, it seems we need to validate the 15% value for when the non-sticky intents are fired, particularly on the device where you saw 10% being used for something. Our main concern here is to avoid adding a feature if it's not robust. If you can successfully validate the 15% value for the non-sticky intents, then we can merge the battery part too once WorkManager is updated to remove their
pluggedcheck. In this case, keeping both in the same pull request is reasonable if you want to avoid splitting it, with the caveat that we can't merge it until the new WorkManager release exists. In the case that validation isn't successful, I don't think we should add support because that would imply WorkManager'sBatteryNotLowTrackeris not robust.
There was a problem hiding this comment.
Were you seeing
ACTION_BATTERY_LOWandACTION_BATTERY_OKfired at 10%, or were they still fired at 15% (as we'd expect if there's a device certification test) and was this just the phone UI using a different value?
On that test device ACTION_BATTERY_LOW is fired at 10% battery remaining and ACTION_BATTERY_OK when the battery is back to 15%. The latter is only fired when the battery went down to 10% first, i.e. when it goes down to 14% and then back to 15% ACTION_BATTERY_OK is not fired. Maybe this is not a certified device (it's a Huawei Mate 20)?
In any case it seems that not every device abides the 15% rule, so in terms of next steps I'll put up a new PR with just the storage not low requirement. If something changes with the battery monitoring implementation we could try adding the battery requirement as well?
Apologies for the gradual sharing of new information; the discussions internally have been ongoing, so I'm just sharing things as I find out about them.
No worries, I appreciate you sharing the info!
There was a problem hiding this comment.
In any case it seems that not every device abides the 15% rule, so in terms of next steps I'll put up a new PR with just the storage not low requirement. If something changes with the battery monitoring implementation we could try adding the battery requirement as well?
Sounds good to me! Maybe just keep this one open as well so that we have the battery stuff to hand for if/when we want to use it.
On that test device ACTION_BATTERY_LOW is fired at 10% battery remaining and ACTION_BATTERY_OK when the battery is back to 15%. The latter is only fired when the battery went down to 10% first, i.e. when it goes down to 14% and then back to 15% ACTION_BATTERY_OK is not fired. Maybe this is not a certified device (it's a Huawei Mate 20)?
Interesting. I will follow up.
There was a problem hiding this comment.
On that test device ACTION_BATTERY_LOW is fired at 10% battery remaining and ACTION_BATTERY_OK when the battery is back to 15%. The latter is only fired when the battery went down to 10% first, i.e. when it goes down to 14% and then back to 15% ACTION_BATTERY_OK is not fired. Maybe this is not a certified device (it's a Huawei Mate 20)?
If the battery goes from 9% to 11% to 9%, is ACTION_BATTERY_LOW fired again in that case? Or does it behave similarly to ACTION_BATTERY_OK and only fire if the battery goes up above 15% in-between?
There was a problem hiding this comment.
In that case ACTION_BATTERY_LOW is fired again when the battery goes down from 11% to 9%.
Forgot to mention that neither ACTION_BATTERY_LOW nor ACTION_BATTERY_OK fire when the device is connected to a power source (which might be expected).
Implemented getSupportedRequirements for schedulers
…TERY_CHANGED action
|
Storage part only implemented in #7395 |
|
Hi @ojw28, I was wondering if there is an update on the battery monitoring issue we discussed in the PR? We would still benefit of having that requirement in ExoPlayer. |
|
I think we're currently stuck with it not really being possible to robustly track battery state at the application layer. I have an internal bug open about that, but no progress so far [Internal ref: b/156962128]. |
|
OK, good to know, thanks for the update! |
|
There was never any followup from the platform team about this issue, so I think it remains the case that it's not possible to robustly track battery state at the application layer, unfortunately. |
ojw28
requested review from
marcbaechinger
and removed request for
marcbaechinger
ojw28
requested review from
marcbaechinger and
ojw28
and removed request for
ojw28
|
Appreciate the update @ojw28! |
Added monitoring battery and storage levels in RequirementsWatcher
Added dependency on extension-workmanager to the demo app to be able to test with WorkManagerScheduler