Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added battery not low and storage not low as download requirements #7138

Open
wants to merge 5 commits into
base: dev-v2
Choose a base branch
from

Conversation

jdegroot-dss
Copy link
Contributor

Added monitoring battery and storage levels in RequirementsWatcher

Added dependency on extension-workmanager to the demo app to be able to test with WorkManagerScheduler

… 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
@googlebot
Copy link

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@jdegroot-dss
Copy link
Contributor Author

@googlebot I signed it!

@ojw28
Copy link
Contributor

ojw28 commented Mar 30, 2020

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.

@jdegroot-dss
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jdegroot-dss
Copy link
Contributor Author

@ojw28 Seems like the signing had not been processed yet, good to go now! Thanks

@ojw28 ojw28 self-assigned this Mar 31, 2020
@@ -835,6 +837,30 @@ private static boolean needsStartedService(@Download.State int state) {
|| state == Download.STATE_RESTARTING;
}

private Requirements validateRequirements(Requirements requirements) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

@ojw28 ojw28 Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. Submit this with both storage and battery parts, and refactor as suggested afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ojw28 ojw28 May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The storage-not-low part is non-controversial. If there was a pull request for just that part, we could easily merge it.
  2. 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 plugged check. 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's BatteryNotLowTracker is not robust.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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!

Copy link
Contributor

@ojw28 ojw28 May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@jdegroot-dss jdegroot-dss requested a review from ojw28 May 12, 2020 09:22
Copy link
Contributor

@ojw28 ojw28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comment regarding the battery part of this change.

@jdegroot-dss
Copy link
Contributor Author

Storage part only implemented in #7395

@jdegroot-dss
Copy link
Contributor Author

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.

@ojw28
Copy link
Contributor

ojw28 commented Sep 25, 2020

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].

@jdegroot-dss
Copy link
Contributor Author

OK, good to know, thanks for the update!

@ojw28
Copy link
Contributor

ojw28 commented Jul 7, 2022

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 ojw28 assigned marcbaechinger and unassigned ojw28 Jul 7, 2022
@ojw28 ojw28 requested review from marcbaechinger and removed request for marcbaechinger July 7, 2022 14:37
@ojw28 ojw28 requested review from marcbaechinger and ojw28 and removed request for ojw28 July 7, 2022 14:40
@jdegroot-dss
Copy link
Contributor Author

Appreciate the update @ojw28!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants