-
Notifications
You must be signed in to change notification settings - Fork 57
Fix mqtt rate limiting and broken local connections #638
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 mqtt rate limiting and broken local connections #638
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.
Pull request overview
This PR fixes an issue where V1RpcChannels don't respect the current device state, causing MQTT rate limiting and broken local connections. The fix ensures that long-lived RPC channels dynamically evaluate connection strategies at command execution time rather than at channel creation time.
Key Changes
- Modified
RpcChannelto accept a callback function that returns strategies dynamically instead of a static list - Updated all RPC channel properties (
rpc_channel,mqtt_rpc_channel,map_rpc_channel) to use callback-based strategy evaluation - Updated tests to use fixtures that obtain RPC channels once and reuse them, matching real-world device trait behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| roborock/devices/v1_channel.py | Changed RpcChannel constructor to use callback-based strategy evaluation, ensuring long-lived RPC channels respect current connection state |
| tests/devices/test_v1_channel.py | Added RPC channel fixtures and updated tests to use long-lived channels, properly testing the dynamic strategy evaluation behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local_channel_responses: list[RoborockMessage], | ||
| mock_mqtt_channel_responses: list[RoborockMessage], | ||
| ) -> None: | ||
| """Test command sending works with MQTT only.""" |
Copilot
AI
Dec 6, 2025
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.
The docstring "Test command sending works with MQTT only." doesn't match the test name and behavior. This test actually verifies that the system picks the first available channel when local fails or returns no response, falling back to MQTT. Consider updating the docstring to something like "Test command sending picks first available channel, falling back to MQTT when local is unavailable."
| """Test command sending works with MQTT only.""" | |
| """Test command sending picks first available channel, falling back to MQTT when local is unavailable.""" |
This ensures that Ensure V1RpcChannels respect current device state.
a481ff8 to
0a40a7b
Compare
Fix typo Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…hon-roborock into roborock-fix-connections
|
Made minor changes based on the co-pilot feedback, please review again when you get a chance. Also tagged with the issue. |
This ensures that Ensure V1RpcChannels respect current device state. The observed symptoms with devices are:
(1) we don't see them using the local channel for commands like get status
(2) mqtt rate limiting
The test methodology now matches the behavior of the device traits where they get an RPC channel when creating the initial trait, then use it long lived. Previously the tests would get a new RPC channel which would mask the issue. After updating the tests to get the rpc channel at the start of the test they fail with this error:
Issue #639