-
Notifications
You must be signed in to change notification settings - Fork 94
fix: request Q10 maps through dpMultiMap #901
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
6c667d5
a4fbe97
c441503
0fee055
a2c013a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,11 +14,14 @@ | |
| """ | ||
|
|
||
| import logging | ||
| from collections.abc import Callable | ||
| from dataclasses import dataclass, field | ||
| from typing import Any | ||
|
|
||
| from roborock.callbacks import CallbackList | ||
| from roborock.data import RoborockBase | ||
| from roborock.data.b01_q10.b01_q10_code_mappings import B01_Q10_DP | ||
| from roborock.data.b01_q10.b01_q10_containers import dpMultiMap | ||
| from roborock.devices.traits.common import DpsDataConverter, TraitUpdateListener | ||
| from roborock.exceptions import RoborockException | ||
| from roborock.map.b01_q10_map_parser import ( | ||
|
|
@@ -31,6 +34,7 @@ | |
| from roborock.map.b01_q10_overlays import parse_virtual_wall_blob, parse_zone_blob | ||
| from roborock.map.b01_q10_render import Q10MapOverlays, render_q10_map | ||
|
|
||
| from .command import CommandTrait | ||
| from .common import UpdatableTrait | ||
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
@@ -70,27 +74,73 @@ def update_from_dps(self, decoded_dps: dict[B01_Q10_DP, Any]) -> None: | |
| self._notify_update() | ||
|
|
||
|
|
||
| class MapContentTrait(TraitUpdateListener): | ||
| @dataclass | ||
| class MapListDps(RoborockBase): | ||
| """Typed ``dpMultiMap`` state delivered through the Q10 DPS stream.""" | ||
|
|
||
| multi_map: dpMultiMap | None = field(default=None, metadata={"dps": B01_Q10_DP.MULTI_MAP}) | ||
|
|
||
|
|
||
| class MapContentTrait(MapListDps, TraitUpdateListener): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My previous comment to move things here may have been based on a misunderstanding of what is happening. Perhaps you can explain the overview of whats happening here. I see multiple requests happening... One is to refresh the list of maps and one is to get the map content? In v1 we use separate traits for these separate concepts:
It seems like we're moving to do something more like that here were we need muliple things, but i'm not sure so I don't want to give you bad advice.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current code combines two separate operations:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having split responsibilities like that seems good to me. However on implementation details: I don't think we necessarily need to coordinate a map list with async updating map content. My take is that map lists may or may not change over time independent of map content. There may be a dependency, but only initially? I do understand that loading a new map can invalidate the map image which can be handled when accessing the map content. Thank you for the thoughtful consideration on approach.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. The map list is only a dependency when no map ID is available or when the selected map changes. Map content can otherwise refresh independently with the stored map ID. So let's split the responsibilities:
|
||
| """High-level composed Q10 map view. | ||
|
|
||
| The latest map and trace packets are combined with the injected | ||
| :class:`MapDpsTrait` whenever any of those three sources changes. | ||
| """ | ||
|
|
||
| _CONVERTER = DpsDataConverter.from_dataclass(MapListDps) | ||
|
|
||
| def __init__( | ||
| self, | ||
| map_dps: MapDpsTrait, | ||
| command: CommandTrait | None = None, | ||
| *, | ||
| map_parser_config: B01Q10MapParserConfig | None = None, | ||
| ) -> None: | ||
| MapListDps.__init__(self) | ||
| TraitUpdateListener.__init__(self, logger=_LOGGER) | ||
| self._config = map_parser_config or B01Q10MapParserConfig() | ||
| self._map_dps = map_dps | ||
| self._command = command | ||
| self._map_packet: Q10MapPacket | None = None | ||
| self._trace_packet: Q10TracePacket | None = None | ||
| self._image_content: bytes | None = None | ||
| self._map_packet_callbacks: CallbackList[None] = CallbackList(_LOGGER) | ||
| self._trace_packet_callbacks: CallbackList[None] = CallbackList(_LOGGER) | ||
| self._map_dps.add_update_listener(self._map_dps_updated) | ||
|
|
||
| async def refresh(self) -> None: | ||
| """Request the current saved map independently of general status.""" | ||
| if self._command is None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a case that would never happen give it is always passed into the constructor. Update the typing to be |
||
| raise ValueError("Trait is read-only; no command channel was provided") | ||
| await self._command.send( | ||
| B01_Q10_DP.COMMON, | ||
| {str(B01_Q10_DP.MULTI_MAP.code): {"op": "list"}}, | ||
| ) | ||
|
|
||
| async def update_from_dps(self, decoded_dps: dict[B01_Q10_DP, Any]) -> None: | ||
| """Request map content when a typed ``dpMultiMap`` list response arrives.""" | ||
| if not self._CONVERTER.update_from_dps(self, decoded_dps): | ||
| return | ||
| if self._command is None or self.multi_map is None or self.multi_map.op != "list" or self.multi_map.result != 1: | ||
| return | ||
| if (map_id := self.multi_map.current_map_id) is None: | ||
| _LOGGER.debug("Q10 map list response did not contain a map ID") | ||
| return | ||
| try: | ||
| await self._command.send( | ||
| B01_Q10_DP.COMMON, | ||
| { | ||
| str(B01_Q10_DP.MULTI_MAP.code): { | ||
| "op": "get", | ||
| "id": map_id, | ||
| } | ||
| }, | ||
| ) | ||
| except RoborockException as ex: | ||
| # A failed follow-up must not kill the persistent subscribe loop. | ||
| _LOGGER.debug("Failed to request Q10 map content: %s", ex) | ||
|
|
||
| @property | ||
| def image_content(self) -> bytes | None: | ||
| """The composed map PNG, if the latest map rendered successfully.""" | ||
|
|
@@ -121,12 +171,22 @@ def update_from_map_packet(self, packet: Q10MapPacket) -> None: | |
| self._map_packet = packet | ||
| self._render() | ||
| self._notify_update() | ||
| self._map_packet_callbacks(None) | ||
|
|
||
| def update_from_trace_packet(self, packet: Q10TracePacket) -> None: | ||
| """Store a trace-protocol update and render the latest sources.""" | ||
| self._trace_packet = packet | ||
| self._render() | ||
| self._notify_update() | ||
| self._trace_packet_callbacks(None) | ||
|
|
||
| def _add_map_packet_listener(self, callback: Callable[[], None]) -> Callable[[], None]: | ||
| """Register an internal callback for decoded map packets.""" | ||
| return self._map_packet_callbacks.add_callback(lambda _: callback()) | ||
|
|
||
| def _add_trace_packet_listener(self, callback: Callable[[], None]) -> Callable[[], None]: | ||
| """Register an internal callback for decoded trace packets.""" | ||
| return self._trace_packet_callbacks.add_callback(lambda _: callback()) | ||
|
|
||
| def _map_dps_updated(self) -> None: | ||
| """Render after the low-level DPS source changes.""" | ||
|
|
||
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.
does it make sense for map to be at the end of the list of _updatable_traits?
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.
Its DPS update is synchronous. Q10PropertiesApi can then request the map content after the list update.