-
-
Notifications
You must be signed in to change notification settings - Fork 677
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
Restart health services if no updates received in 30 minutes #3126
Restart health services if no updates received in 30 minutes #3126
Conversation
I feel for this PR we should get some feedback from users but at the same time we know restart logic like this works pretty well since we do something similar with location updates and other sensors. At least in my own experience I have yet to see the updates stop like the users are mentioning. Will leave as a draft for now to see if we can get some feedback otherwise will mark as ready for review (unless we feel this is good enough to add anyways then lets just move forward with ensuring we are getting updates). |
I think I finally managed to catch this issue. What it appears to be happening is that health services crashes, once it recovers it seems to register back but not all data is restored.
After this no log statements were printed for daily calories which updates once per minute on a Pixel Watch. So this intended fix should be good here. Not entirely sure how to reproduce this as in this case teh issue happened moving my watch off and on charger. |
I have tried to replicate this issue by force stopping Health Services however the call back is restored properly then and data continues to flow in. This crash of health services must be occurring elsewhere. The restart logic should take care of this.
|
submitted a bug to the library as well, hopefully they can reproduce it 🤞 https://issuetracker.google.com/issues/261119535 For now I think for us the proper path forward is to use this restart logic like we do elsewhere. |
Been running this debug build for several days to see if I can catch a moment where production stops and debug continues. The only time I saw it happen was after an app update (as strange as that sounds) where I needed to force stop the app and open it again. The debug build was able to recover on its own without interference. I am not sure what happens during an app update where updates seemingly stop, maybe its similar to the above issue where Health Services attempts to restore the connection and it fails? At the same time we also attempt to register with health services, either way if there is a conflict its good that we correct it on our end. So the only real concern here would be if a user only has activity state enabled because in those cases we will probably re-register the callback often. That may potentially also impact any of the daily sensors that don't report often either. There is no harm in registering and unregistering the callback so its good to keep things current like we do with location updates on the phone. |
Does re-registering also deliver new activity info to the app? If so, if I understand it correctly, it will basically force an update of the sensors every 30 min which seems wasteful and a check should be added to see if the value changed before attempting to push an update in |
Yes we do get an update immediately for this sensor
So we actually keep track of the callback registration to prevent a loop. The issue I was trying to explain was that Activity state only updates when the state changes and this sensor really only updates 2-3 times a day, maybe more if a user is exercising. So about every 30 minutes we will be re-registering the callback if only this sensor is enabled, the other health services sensors seem to report a little more frequently. |
This is good, but I wasn't suggesting there is a loop, just unnecessary updates.
An immediate update + re-registering every 30 minutes if only the activity state sensor is enabled for health services (because it does not update frequently enough) means that a new value is delivered to Because re-registering happens because of the sensor worker this means two updates shortly after another, one of which likely doesn't have any differences. |
Ah yes I see what you mean, ok I will add a check to determine if an update needs to be sent immediately or not since we do force an update when exercise type changes too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't do full testing without a real watch, but the code looks good to me and it follows the same approach as used for making sure other callbacks don't get stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been testing this the past few days (since Dec 1st) and all has been good even seemed to help the main version to keep updating I uninstalled the debug version yesterday (12/05/22) and overnight the Main HA app stopped pushing activity state updates
Summary
This is an attempt to fix #3112 by keeping track of the last update received and restarting health services if its been 30 minutes since an update. I feel 30 minutes will be a good time period as there are really no other ways to detect the updates stopping. More than likely if a user only has User Activity State they will probably see the callback registering more frequently as a result of this change. I suspect when this issue occurs it impacts all things registered with the callback so best to include activity state.
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes