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

Clean up location manager after making updates suspend #4818

Merged

Conversation

dshokouhi
Copy link
Member

@dshokouhi dshokouhi commented Nov 14, 2024

Summary

Noticed that we had a few places to clean up and remove overlapping suspend calls

also added in a flush location call to only be called when we need to restart location services to clear out the queue, its also recommended we call it as we use setMaxWaitTime() so this felt like the most appropriate place to call it.

geofences are also limited to 100 per app so adding in 2 condition checks there. Probably needs a docs update for this one and combined with high accuracy expanded mode will be fun to write 🙃

https://developer.android.com/develop/sensors-and-location/location/geofencing

Screenshots

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#1141

Any other notes

Tested these changes overnight to ensure it all still works as expected

@jpelgrom
Copy link
Member

By removing launch in a lot of places the code now waits/runs in sequence instead of doing this async. I feel like launch was done on purpose in a couple of places, especially for multiserver setups where we want to send to all and don't want to wait for server A to timeout before trying server B. Was this change intentional on your end?

@dshokouhi
Copy link
Member Author

dshokouhi commented Nov 19, 2024

By removing launch in a lot of places the code now waits/runs in sequence instead of doing this async. I feel like launch was done on purpose in a couple of places, especially for multiserver setups where we want to send to all and don't want to wait for server A to timeout before trying server B. Was this change intentional on your end?

well i did not consider that was trying to remove overlapping coroutines

think the latest commit should address this and leave just the setup pieces and notification commands as just suspend along with ensuring we dont hit the 100 geofence limit

@jpelgrom
Copy link
Member

OK I think this is good now but am always a bit hesitant with location tracking changes.

Do you want to update the docs to mention the geofence limit? (And also specifically how it impacts with the extended zones for high accuracy)

@dshokouhi
Copy link
Member Author

OK I think this is good now but am always a bit hesitant with location tracking changes.

agreed they are not fun to review nor make changes on

Do you want to update the docs to mention the geofence limit? (And also specifically how it impacts with the extended zones for high accuracy)

yes we should do that, let me write something up when i get a free moment and link it here

Copy link
Member

@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

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

Moving initial setup to not run async looks good to me, but should probably get one beta cycle before being pushed

@dshokouhi
Copy link
Member Author

lets put this in the beta cycle now

@dshokouhi dshokouhi merged commit f4098f2 into home-assistant:master Jan 21, 2025
4 checks passed
runningcode pushed a commit to runningcode/android that referenced this pull request Jan 21, 2025
…t#4818)

* Clean up location manager after making updates suspend

* Restore launch blocks for location updates
@splichy
Copy link

splichy commented Feb 9, 2025

Hi, it seems extended zone high accuracy mode no longer works after update to 2025.2.1
I was comming into geofence location two times today and high accuracy mode wasn't triggered as usual - phone was awake running waze, so GPS was active. So I have tried to remove and add high accuracy zone and will try tomorrow again.

@jpelgrom
Copy link
Member

jpelgrom commented Feb 9, 2025

@splichy If you have an issue, please open a new issue and add the provided information. PR comments are not a good place to discuss potential issues.

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.

3 participants