-
Notifications
You must be signed in to change notification settings - Fork 57
feat: Update device manager and device to establish an MQTT subscription #409
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
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
Adds MQTT subscription support per device by introducing an MQTT “channel” abstraction, integrating it into both the device manager and individual device objects, and updating the CLI to leverage the new manager.
- Introduce
MqttChannelfor subscribing/unsubscribing to device-specific topics - Wire up MQTT sessions in
DeviceManagerandRoborockDeviceto manage connections and clean up on close - Refactor the CLI to use
create_device_managerand ensure graceful shutdown of MQTT
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/devices/test_mqtt_channel.py | Add tests for MqttChannel.subscribe behavior |
| tests/devices/test_device_manager.py | Ensure close() is called on the manager in tests |
| tests/devices/test_device.py | Test RoborockDevice.connect / close integration with channel |
| roborock/devices/mqtt_channel.py | New class to wrap MQTT subscribe/unsubscribe for a device |
| roborock/devices/device_manager.py | Pass through MQTT session, establish channels on discovery, add close() |
| roborock/devices/device.py | Add connect/close methods to manage MQTT subscriptions |
| roborock/cli.py | Refactor CLI to use create_device_manager and clean shutdown |
Comments suppressed due to low confidence (2)
roborock/devices/device_manager.py:96
- The constructor for
RoborockApiClientoriginally accepted(email, user_data). Confirm that droppinguser_datamatches the new API signature or update accordingly.
client = RoborockApiClient(email)
tests/devices/test_mqtt_channel.py:47
- Add a test case to verify that calling
subscribetwice raises the expectedValueErrorwhen already subscribed, as described in the method docstring.
async def test_mqtt_channel() -> None:
Lash-L
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.
Looks good - I'll be curious to see the next iteration. Just a couple of comments
Update the device manager and device to establish an Mqtt "channel" which is the device-specific subscription in the mqtt session. The channel will be used for decoding messages and sending/receiving RPCs.
Lots of future work needed here including: