fix: remove dnd timer and valley electricity from props #75
Conversation
|
The cause of the error is that UnknownMethodException is being raised, but it is being treated as if it is a timeout, so it attempts to connect again |
|
also seems like we were scheduling a new ping everytime we reconnected, so the amount of pings grew everytime we reconnected. |
|
What issues you are mentioning here? |
|
humbertogontijo/homeassistant-roborock#379 Any user who doesn't have valley timer - the version wasn't working |
roborock/local_api.py
Outdated
| except RoborockException: | ||
| pass | ||
| if self.keep_alive_task is not None: | ||
| self.keep_alive_task.cancel() |
There was a problem hiding this comment.
Is this needed? The task will end after next line
There was a problem hiding this comment.
My understanding is that call later returns a obj that can cancel the original callback, but when you call call_later it will run the action regardless of if the response gets overwritten.
I could be wrong though, I Didn’t extensively test
There was a problem hiding this comment.
Yes. But this is cancelling it during execution. Right after ping was already called and next one is being scheduled
I think there is a cancel already when disconnected
There was a problem hiding this comment.
That's the case if the keep_alive_func is called by the call_later in keep alive func, but if keep_alive_func is called in async connect, then the existing keep alive task still exist and will continue to populate itself, but the new one will also be populating itself.
I just removed it for now though because I am not confident.
|
Yeah, using a non coordinated entity is the way to go |
| self.dnd_timer: DnDTimer | None = None | ||
| self.valley_timer: ValleyElectricityTimer | None = None |
There was a problem hiding this comment.
Not sure if this is the best way, but I'm trying to avoid doing the coordinator, and rather than place it in device info, cause then I'd have to make a new function call on top of get_dnd_timer to update it, rather than just setting it here.
There was a problem hiding this comment.
I thought about something similar. Having an object to hold all executed queries with evicting and caching based on GET / (SET / CHANGE) commands.
You can check them out in roborock_typing CacheableCommands
There was a problem hiding this comment.
Oh that's nice and probably much better long term than this
|
I'll accept this since 1.0.7 is broken due to props not being compatible with all devices |
|
Ugh not sure why semantic release is still failing |
|
Yeah. Maybe we need a commit message with current version? |
2 participants
Ok figured it out, the issue is with 24.0 of python-roborock.
Something with the valley timer and dnd timer being in the get_props.
Which in my opinion, we shouldn't be doing. If you look at how I do it in core, for entities outside of status, I have non-coordinated entities, these entities make the api call directly, that way if they aren't supported or if the user disables that entity, that call wont get made. Or If I update that entity, i.e. a switch on, I can just request the new status of that entity, rather than doing an entire coordinator refresh.
The issue stems with the valley timer, not sure why but that is causing all of the issues.
But for now I'm removing these from props as long as that is fine with you. And if I have time, I will try to rework the switches on homeassistant-roborock to match core as long as this is okay with you