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

Breaking change: Improve sensor mobile_data #4696

Merged

Conversation

frimtec
Copy link
Contributor

@frimtec frimtec commented Oct 4, 2024

Summary

Improve gathering data for sensor mobile_data.
The currently used API for the mobile_data sensor is marked as @UnsupportedAppUsage and is therefore not supported on newer devices (e.g. Pixel 9).
As of SDK 26 there is an API TelephonyManager#isDataEnabled that is supported on all devices.

Screenshots

Frontend not affected due to this change.

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#435
The existing documentation introduced with above pull request is still fully valid.

Any other notes

@dshokouhi
Copy link
Member

I see that permissions are changing here for a sensor, that will be a breaking change for users who have that sensor enabled but not the permission granted. Is the sensor not updating properly? on my pixel 9 pro xl its showing the correct state.

@frimtec frimtec force-pushed the feature/improve-sensor-mobile-data branch from b51d93e to 7b8630c Compare October 4, 2024 19:22
@frimtec
Copy link
Contributor Author

frimtec commented Oct 4, 2024

The state for the sensor "mobile_data" on my pixel 9 Pro is always "on" even when mobile data is disabled. I could also reproduce that behavior on a pixel 7 of a friend.
With this fix it works again on my pixel 9, as I was used it with my old pixel 4.

I have currently no idea why it still works on your pixel 9 pro xl.

The declaration of the old Android API Settings.Global.MOBILE_DATA is as follows :

        @UnsupportedAppUsage
        @Readable
        public static final String MOBILE_DATA = "mobile_data";

@UnsupportedAppUsage means:
Marks elements as internal and not intended for external use.
Avoid using unsupported APIs to ensure app stability and compatibility.

Therefore I think it makes more sense to use the public API if available (as of SDK 26).

As the new API requires permission READ_PHONE_STATE, you are right this could be a breaking change.
I updated the requiredPermissions() method to make the change as small as possible.

@frimtec frimtec force-pushed the feature/improve-sensor-mobile-data branch from 7b8630c to e7d5715 Compare October 4, 2024 19:26
@dshokouhi
Copy link
Member

My bad i did not realize that the sensor state had stopped working, I had incorrectly assumed as I had mobile data enabled that the sensor had the correct state 🙈 Now I wonder how long it was broken for 😂

@dshokouhi dshokouhi changed the title Improve sensor mobile_data Breaking change: Improve sensor mobile_data Oct 4, 2024
@dshokouhi dshokouhi merged commit 6b8e119 into home-assistant:master Oct 4, 2024
4 checks passed
@dshokouhi
Copy link
Member

Now I wonder how long it was broken for 😂

Looks like my Pixel watch had the correct state with the old method. Surprisingly it's on the same version of Android as my phone. Guess it shows the platform differences there lol. Thanks again for the PR 🙏Screenshot_20241004-151327.png

@frimtec
Copy link
Contributor Author

frimtec commented Oct 5, 2024

Looks like my Pixel watch had the correct state with the old method. Surprisingly it's on the same version of Android as my phone.

It is not the android version that is relevant whether it works or not, but the device itself. E.g. the old API still works up to android 15 on the android emulator. But more and more devices do not support the old API because it violates the security model. With the old API you can access the phone state without the required permission READ_PHONE_STATE.

Thx for merging the pull request 👍

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.

2 participants