Conversation
There was a problem hiding this comment.
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.
I can move the responsibility for this further upstream if you'd like.
There was a problem hiding this comment.
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.
(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.
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.
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.
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