feat: add protocol updates #731
Conversation
|
|
||
| # pickle files | ||
| *.p | ||
| *.pickle |
There was a problem hiding this comment.
unrelated but i have a few of these in my repo and just wanted to stop them from filling up my git diff
There was a problem hiding this comment.
Pull request overview
This PR adds real-time protocol update handling to enable the library to receive and process device state updates pushed via MQTT. The key insight is that protocol updates (data points) are only sent via cloud/MQTT, not through local connections, so the MQTT channel must always be subscribed to receive these notifications.
- Added a callback system to V1TraitMixin allowing consumers to be notified when traits are updated via protocol messages
- Modified V1Channel to always subscribe to MQTT for protocol updates, even when a local connection is established (local is used for RPC commands only)
- Implemented protocol message parsing in RoborockDevice to update status trait fields (error_code, state, battery, charge_status) based on incoming data points
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| roborock/devices/traits/v1/common.py | Adds callback infrastructure (add_update_callback, notify_update) to V1TraitMixin for notifying listeners when traits are updated |
| roborock/devices/rpc/v1_channel.py | Updates subscription logic to always connect to MQTT for protocol updates, not just as a fallback when local connection fails |
| roborock/devices/device.py | Implements protocol message parsing in _on_message and _handle_protocol_update to extract data points and update status trait fields |
| .gitignore | Adds pickle file patterns to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def add_update_callback(self, callback: V1TraitUpdateCallback) -> Callable[[], None]: | ||
| """Add a callback to be notified when the trait is updated. | ||
| The callback will be called with the updated trait instance whenever | ||
| a protocol message updates the trait. | ||
| Returns: | ||
| A callable that can be used to remove the callback. | ||
| """ | ||
| return self._update_callbacks.add_callback(callback) | ||
|
|
||
| def notify_update(self) -> None: | ||
| """Notify all registered callbacks that the trait has been updated.""" | ||
| self._update_callbacks(self) |
There was a problem hiding this comment.
The new callback functionality (add_update_callback and notify_update methods) lacks test coverage. Given that this repository has comprehensive automated testing, tests should be added to verify that callbacks are properly invoked when traits are updated via protocol messages.
| # Only process messages that can contain protocol updates | ||
| # RPC_RESPONSE (102), GENERAL_REQUEST (4), and GENERAL_RESPONSE (5) | ||
| if message.protocol not in { | ||
| RoborockMessageProtocol.RPC_RESPONSE, | ||
| RoborockMessageProtocol.GENERAL_RESPONSE, | ||
| }: | ||
| return | ||
|
|
||
| if not message.payload: | ||
| return | ||
|
|
||
| try: | ||
| payload = json.loads(message.payload.decode()) | ||
| dps = payload.get("dps", {}) | ||
|
|
||
| if not dps: | ||
| return | ||
|
|
||
| # Process each data point in the message | ||
| for data_point_number, data_point in dps.items(): | ||
| # Skip RPC responses (102) as they're handled by the RPC channel | ||
| if data_point_number == "102": | ||
| continue | ||
|
|
||
| try: | ||
| data_protocol = RoborockDataProtocol(int(data_point_number)) | ||
| self._logger.debug(f"Got device update for {data_protocol.name}: {data_point}") | ||
| self._handle_protocol_update(data_protocol, data_point) | ||
| except ValueError: | ||
| # Unknown protocol number | ||
| self._logger.debug( | ||
| f"Got unknown data protocol {data_point_number}, data: {data_point}. " | ||
| f"This may allow for faster updates in the future." | ||
| ) | ||
| except (json.JSONDecodeError, UnicodeDecodeError, KeyError) as ex: | ||
| self._logger.debug(f"Failed to parse protocol message: {ex}") | ||
|
|
||
| def _handle_protocol_update(self, protocol: RoborockDataProtocol, data_point: Any) -> None: | ||
| """Handle a protocol update for a specific data protocol. | ||
| Args: | ||
| protocol: The data protocol number. | ||
| data_point: The data value for this protocol. | ||
| """ | ||
| # Handle status protocol updates | ||
| if protocol in ROBOROCK_DATA_STATUS_PROTOCOL and self.v1_properties and self.v1_properties.status: | ||
| # Update the specific field in the status trait | ||
| match protocol: | ||
| case RoborockDataProtocol.ERROR_CODE: | ||
| self.v1_properties.status.error_code = RoborockErrorCode(data_point) | ||
| case RoborockDataProtocol.STATE: | ||
| self.v1_properties.status.state = RoborockStateCode(data_point) | ||
| case RoborockDataProtocol.BATTERY: | ||
| self.v1_properties.status.battery = data_point | ||
| case RoborockDataProtocol.CHARGE_STATUS: | ||
| self.v1_properties.status.charge_status = data_point | ||
| case _: | ||
| # There is also fan power and water box mode, but for now those are skipped | ||
| return | ||
|
|
||
| self._logger.debug("Updated status.%s to %s", protocol.name.lower(), data_point) | ||
| self.v1_properties.status.notify_update() |
There was a problem hiding this comment.
The protocol update parsing logic introduced in _on_message and _handle_protocol_update lacks test coverage. Given that this repository has comprehensive automated testing, tests should be added to verify that protocol messages are correctly parsed and that the appropriate trait fields are updated when data points are received.
| match protocol: | ||
| case RoborockDataProtocol.ERROR_CODE: | ||
| self.v1_properties.status.error_code = RoborockErrorCode(data_point) | ||
| case RoborockDataProtocol.STATE: | ||
| self.v1_properties.status.state = RoborockStateCode(data_point) | ||
| case RoborockDataProtocol.BATTERY: | ||
| self.v1_properties.status.battery = data_point | ||
| case RoborockDataProtocol.CHARGE_STATUS: | ||
| self.v1_properties.status.charge_status = data_point | ||
| case _: | ||
| # There is also fan power and water box mode, but for now those are skipped | ||
| return |
There was a problem hiding this comment.
The match statement only handles ERROR_CODE, STATE, BATTERY, and CHARGE_STATUS protocols, but ROBOROCK_DATA_STATUS_PROTOCOL also includes FAN_POWER and WATER_BOX_MODE (as mentioned in the comment on line 294). These two protocols will match the condition on line 282 but then fall through to the default case without updating the trait or calling notify_update. This creates inconsistent behavior where some status protocols trigger updates while others silently do nothing. Either handle these protocols explicitly or exclude them from the condition on line 282.
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
Great, will be useful to add this.
My initial thought is that for the logic currently in the device: This is all v1 specific logic so it should either live in the v1 channel, or in the trait, and not in the device which is more generic. It can make it's own subscriber on the MQTT channel to also avoid needing to exclude anything from local messages.
For your question about granularity i think we can separate two things:
(1) I do think having the traits have the listener makes sense. Unclear to me that it needs to pass "self" however, i think that can be omitted and let the caller track which trait it subscribed to if needed.
(2) We can separately decide if we want the coordinator to listen to all traits and notify all entities or if we want the traits themselves to subscribe to updates from the entity. But as an example where we don't have to decide that now (We could have the coordinator listen to all updates from all traits and the notify all entities, if we wanted it to work that way.). Point being from an API perspective its not locking in anything.
| # Update the specific field in the status trait | ||
| match protocol: | ||
| case RoborockDataProtocol.ERROR_CODE: | ||
| self.v1_properties.status.error_code = RoborockErrorCode(data_point) |
There was a problem hiding this comment.
I think the decision about which field in the trait to update should live inside the specific trait. I think we should pass in the protocol and data point and let it decide to update or not or which field to update.
Unresolved questions:
closes #625