-
Notifications
You must be signed in to change notification settings - Fork 57
fix: fix L01 encoding and decoding #593
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
This PR fixes L01 protocol encoding and decoding in the new devices API by properly handling protocol version information in local communication channels.
Key Changes:
- Updated
encode_messageto useLocalProtocolVersionenum instead of plain strings for better type safety - Added
protocol_versionproperty toLocalChannelto expose the negotiated protocol version - Modified local RPC channel creation to pass the correct protocol version when encoding messages
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
roborock/protocols/v1_protocol.py |
Changed version parameter type from str to LocalProtocolVersion enum and updated encoding to use .value.encode() |
roborock/devices/v1_rpc_channel.py |
Updated create_local_rpc_channel to pass the negotiated protocol version from local_channel.protocol_version |
roborock/devices/local_channel.py |
Added protocol_version property that returns the negotiated version or defaults to V1; also fixed callback assignment in _update_encoder_decoder |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
roborock/devices/local_channel.py
Outdated
| format most parsing to higher-level components. | ||
| """ | ||
|
|
||
| _protocol_cache: dict[str, LocalProtocolVersion] = {} |
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.
I can move the responsibility for this further upstream if you'd like.
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.
i generally get the idea of caching things, but i'm having a hard time seeing how often this will get used. Are we creating local channels repeatedly? i would have thought once per session given once connected it will stay connected.
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.
(I would cache this like or in NetworkInfo, but i realize it could change if firmware version changes so i casee thats difficult)
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.
Right now, seemingly we are. On every reconnect. Rob was noticing that it took longer for his system to reconnect on disconnects
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.
Are we reconnecting so often that this matters in practice? Ideally we reconnect at most once per day so presumably reconnect is not the primary cost. I can see wanting to optimize startup time, though, which i don't think this does. I guess I think if we really want to cache this to improve performance then we should also cache the same way we cache network info to avoid the extra round trip on startup too?
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.
I think we should come back to the protocol cache in a separate PR, so its reverted for now.
0fcf224 to
4501307
Compare
|
Offline Lash-L also reviewed and gave an LG -- just for audit log :) (not sure how approval works on two party CL) |
Fix L01 on the new devices api