-
Notifications
You must be signed in to change notification settings - Fork 55
fix: make cache non-global #122
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
Conversation
|
That was global because I didn't want to user the local client reference when doing some checks in the camera(map) entity. I think that was laziness from the beginning so I'm fine with changing it |
humbertogontijo
left a comment
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.
LGTM
|
Any particular reason for the name "api timing" ? |
I originally included some changes for keepalive in this pr too, but I hadn't really tested them so I didn't want to include them. A user posted that in their correspondence with Roborock - that if there are over 100 request in an hour, there is a chance of getting the 24 hour ban (humbertogontijo/homeassistant-roborock#480 (comment)) So I started to try to decrease the amount of calls we make, but honestly it is not a very easy task/ may basically be impossible |
|
The easiest way would be to just add a throttle to send_commad. And log if it is throttling |
I'd like to be able to prevent/ predict throttling and handle it more so than just a clean cut off |
Yeah. I get it. But I do think that this number is not realistic. The integration refreshes the map every 5 seconds(12 request/minute or 720 request/hour - who know the size of someone else home haha) when it is cleaning, putting it over the edge, and literally almost no one complained about blocks |
So I've been playing around with the listeners and I think the cache may actually need to be global. As the listeners are cloud only as far as I know, and they need to update the cache and then the coordinator needs to update the entities, but if you are using the local api as the base api for the coordinator, then updating the cache for the cloud api (when the listeners run) - wont make any difference - as we still update them based off of the local api's cache. Gonna play around a bit more and see what I think is best, but wanted to bring it up in case you had any thoughts @humbertogontijo |
|
Thinking about it - the approach I would like to take is a global listener dictionary of dictionary When we get data we check to see if we have listeners for the duid, we then check if the data protocol id is in the dictionary and if it is, we pass the updated value to the function. This would allow us to be able to just update the entities that are affected by the update, and now have to refresh all of the states. |
|
What are these listeners youre talking about? |
|
The listeners you have in the package and custom components. Update the data when Matt topic publishes a message that was not the result of a command |
|
Got it. We can change it back to global |
A few things
We will no longer attempt to get values if we know we don't support them
The caches were global, and that meant they would end up referencing the api for whichever client was created first, not necessarily the specific client we want