fix: Ensure immediate local connection is attempted #640
Conversation
Always start a local connection immediately to ensure initial RPCs can be sent locally. Without this, the first RPC may be sent over MQTT if the local connection didn't have a chance to start yet.
There was a problem hiding this comment.
Pull request overview
This PR ensures that device connections are initiated immediately during device discovery, preventing the first RPC from being sent over MQTT before a local connection is established. The key change is converting start_connect() from a synchronous fire-and-forget method to an async method that waits for the initial connection attempt to begin.
- Converted
start_connect()to async with a 5-second timeout for initial connection attempts - Modified device manager to await all
start_connect()tasks in parallel usingasyncio.gather() - Simplified test to rely on synchronous behavior instead of polling for connection attempts
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| roborock/devices/device.py | Converted start_connect() to async method with event-based waiting mechanism for initial connection attempt (with 5s timeout) |
| roborock/devices/device_manager.py | Updated to collect and await start_connect() tasks for all devices using asyncio.gather() |
| tests/devices/test_device_manager.py | Simplified test to assert immediate connection attempt instead of polling with sleep loop |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new_devices[duid] = new_device | ||
|
|
||
| self._devices.update(new_devices) | ||
| await asyncio.gather(*start_tasks) |
There was a problem hiding this comment.
The asyncio.gather(*start_tasks) call should include return_exceptions=True to prevent one device's connection failure from blocking the discovery of other devices. Without this parameter, if any start_connect() raises an exception that escapes the timeout, the entire discover_devices() operation will fail.
Consider changing this to:
await asyncio.gather(*start_tasks, return_exceptions=True)| await asyncio.gather(*start_tasks) | |
| await asyncio.gather(*start_tasks, return_exceptions=True) |
roborock/devices/device.py
Outdated
| This will give a moment for the first connection attempt to start so | ||
| that the device will have connections established -- however, this will | ||
| never directly fail. | ||
There was a problem hiding this comment.
Trailing whitespace should be removed from this blank line.
roborock/devices/device.py
Outdated
| start_attempt.set() | ||
|
|
||
| self._connect_task = asyncio.create_task(connect_loop()) | ||
|
|
There was a problem hiding this comment.
Trailing whitespace should be removed from this blank line.
roborock/devices/device.py
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Trailing whitespace should be removed from this blank line.
| device_products = home_data.device_products | ||
| _LOGGER.debug("Discovered %d devices %s", len(device_products), home_data) | ||
|
|
||
| # These are connected serially to avoid overwhelming the MQTT broker |
There was a problem hiding this comment.
This comment is now misleading. The code previously connected devices serially, but with the changes in this PR, devices now start connecting in parallel (via asyncio.gather() on line 94). The comment should be updated to reflect the new behavior, or removed if parallel connection is the intended behavior.
If overwhelming the MQTT broker is still a concern, consider updating the comment to explain why parallel connection is now acceptable, or implement a mechanism to limit concurrency (e.g., using a semaphore).
| # These are connected serially to avoid overwhelming the MQTT broker |
2 participants
Always start a local connection immediately to ensure initial RPCs can be sent locally. Without this, the first RPC may be sent over MQTT if the local connection didn't have a chance to start yet.
Issue #639