-
Notifications
You must be signed in to change notification settings - Fork 51
fix: improve new v1 apis to use mqtt lazily and work entirely locally #491
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
fix: improve new v1 apis to use mqtt lazily and work entirely locally #491
Conversation
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.
Honestly looks good, one small nitpick, feel free to ignore.
establish mqtt connections on demand only when needed on first subscription. we can decide to poke the mqtt session explicitly if we prefer, but it won't be done by the device manager now
We may want to poke it explicitly in HA for the sake of getting protocol updates, but I think this good library wise and we can figure out protocol updates later.
roborock/devices/v1_channel.py
Outdated
if local_connect_failures > 1: | ||
await asyncio.sleep(reconnect_backoff.total_seconds()) | ||
reconnect_backoff = min(reconnect_backoff * RECONNECT_MULTIPLIER, MAX_RECONNECT_INTERVAL) | ||
|
||
# First failure refreshes cache. Subsequent failures use the cache | ||
# until the refresh interval expires. | ||
use_cache = True | ||
if local_connect_failures == 1: | ||
use_cache = False |
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.
could simplify by setting use_cache = True inside the if statement for >1. One fewer if statement
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.
although i guess you'll have to move that elif below.
You don't have to do this if you don't want to. If you think it hurts readability or understandability, just ignore
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.
Yeah i looked at doing that before, but its like this because there are a couple scenarios for deciding if cache should be used that it makes sense to do them at once. So, I decided to instead extract to a helper function so at least this flow is a little easier to read.
The new v1 channel will:
This makes the local background task (per device0 now work similarly to the mqtt session task.
The background logic is difficult to unit test without races, so it is not tested. I will do additional manual testing of complex scenarios for errors when this is integrated into Home Assistant.