-
Notifications
You must be signed in to change notification settings - Fork 57
chore: refactor v1 rpc channels #609
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
This merges the two packages v1_channel and v1_rpc_channel since they are very closely related. The differences between the RPC types are now handled with `RpcStrategy` (encoding, decoding, which channel, health checking, etc). The "PayloadEncodedV1RpcChannel" is now `_send_rpc` which accepts the rpc strategy. The `V1RpcChannel` interface is moved to the `v1_protocol` module and now has a single implementation that handles everyting (the `PickFirstAvailable` logic as well as the response parsing). Overall the code is now less generalized, but is probably easier to understand since its all in a single place. Notably, all the rpc code was already tested via the v1_channel interface.
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 refactors the V1 RPC channel implementation by consolidating the v1_rpc_channel and v1_channel modules. The refactoring introduces a strategy pattern via RpcStrategy to handle different transport methods (MQTT, local) with their specific encoding, decoding, and health checking requirements.
Key Changes:
- Moved
V1RpcChannelProtocol fromv1_rpc_channel.pytov1_protocol.pyfor better organization - Introduced
RpcStrategydataclass to encapsulate transport-specific configuration (encoder, decoder, channel, health manager) - Consolidated RPC logic into a single
RpcChannelimplementation with automatic fallback between transports - Removed separate channel factory functions in favor of dynamic strategy creation methods
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
roborock/protocols/v1_protocol.py |
Added V1RpcChannel Protocol definition with overloaded send_command method to support both typed and untyped responses |
roborock/devices/v1_channel.py |
Introduced RpcStrategy and RpcChannel classes; consolidated all RPC logic including transport selection, encoding/decoding, and error handling with automatic fallback |
roborock/devices/v1_rpc_channel.py |
Deleted entire file - functionality merged into v1_channel.py |
roborock/devices/mqtt_channel.py |
Fixed docstring from "V1Channel" to "MQTT channel" for accuracy |
roborock/devices/traits/v1/common.py |
Updated import to use V1RpcChannel from v1_protocol module |
roborock/devices/traits/v1/__init__.py |
Updated import to use V1RpcChannel from v1_protocol module |
tests/devices/test_v1_device.py |
Updated import for decode_rpc_response from v1_protocol module |
tests/devices/test_v1_channel.py |
Updated mock patch location for create_map_response_decoder; updated test expectations to reflect new fallback behavior; changed test commands from CHANGE_SOUND_VOLUME to GET_STATUS to match test fixtures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This merges the two packages v1_channel and v1_rpc_channel since they are very closely related. The differences between the RPC types are now handled with
RpcStrategy(encoding, decoding, which channel, health checking, etc). The "PayloadEncodedV1RpcChannel" is now_send_rpcwhich accepts the rpc strategy.The
V1RpcChannelinterface is moved to thev1_protocolmodule and now has a single implementation that handles everyting (thePickFirstAvailablelogic as well as the response parsing).Overall the code is now less generalized, but is probably easier to understand since its all in a single place. Notably, all the rpc code was already tested via the v1_channel interface.