Repository navigation
Conversation
|
On hold following the review feedback on #933: please do not review the current cumulative diff. Once the read-only map stack lands, this draft will be rebased and repartitioned into focused changes:
Each part will be submitted sequentially against |
|
@hCoureau This branch has conflicts that must be resolved. |
|
Resolved the merge conflicts with current Validation: 1,150 tests passed, 92 snapshots passed, all pre-commit hooks passed, and sdist/wheel builds passed. GitHub CI was triggered for the new head. Required maintainer approval still applies. #933 has already merged. This branch still contains the stacked Q10 archive/obstacle/current-room work, which remains visible for maintainer review. Conflict resolution and validation performed with OpenAI Codex assistance. |
|
@allenporter PR #934 has been conflict-free since Sep 10, and everything checks out:
Ready for your review and merge approval when you get to it. |
# Conflicts: # roborock/devices/traits/b01/q10/maps.py # tests/devices/traits/b01/q10/test_clean_history.py
|
Physical-device validation of the restriction/wall write path, on an ss07
Not tested: room rename, saved-map apply. Happy to run this branch itself on Update: also exercised add and delete from a client through the same dpCommon |
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
Note: Conducting this code review following our /review skill using Antigravity (AGY) and Gemini 4 Argon.
The wire encoding for DP 54 (restricted zones), DP 56 (virtual walls), DP 58 (room rename), and DP 61 (apply) looks well-verified against device captures.
Main areas to align with our repository conventions:
-
Public domain models (
RoborockBase) and resilient enums (RoborockEnum):Q10RestrictionTypeshould subclassRoborockEnumwithunknown = -1(rather thanIntEnum) so unknown wire codes fall back toQ10RestrictionType.unknowninstead of catchingValueErrorand typingQ10RestrictedZone.typeasint.Q10RestrictedZoneandQ10VirtualWallare public trait models onMapContentTraitand should be@dataclassmodels subclassingRoborockBase(onlyQ10RoborockPointis exempt fromRoborockBasefor coordinate hashability).
-
Remove defensive runtime
isinstancechecks on statically typed parameters:- See inline comments in
roborock/protocols/b01_q10_protocol.py.
- See inline comments in
-
Remove
is_replaceable_zone_blobprecondition onset_restricted_zones:set_restricted_zones(zones)replaces the complete restricted-zone list on the device without merging existing state, so it shouldn't requireself._map_dps.restricted_zone_upto have been received first (matchingset_virtual_walls).
-
Test cleanup:
- Remove the duplicate
test_upstream_*tests intests/data/b01_q10/test_b01_q10_containers.pyand use the fake channel instead ofpatch.object(q10_api.command, "send")intests/devices/traits/b01/q10/test_map.py.
- Remove the duplicate
| vertices: list[tuple[int, int]] = field(default_factory=list) | ||
|
|
||
|
|
||
| class Q10RestrictionType(IntEnum): |
There was a problem hiding this comment.
Q10RestrictionType should inherit from RoborockEnum with unknown = -1 (in roborock/data/b01_q10/b01_q10_code_mappings.py), and Q10RestrictedZone / Q10VirtualWall should be @dataclass models subclassing RoborockBase with type: Q10RestrictionType.
|
|
||
| async def set_restricted_zones(self, zones: Sequence[Q10RestrictedZone]) -> None: | ||
| """Replace the complete restricted-zone collection.""" | ||
| if not is_replaceable_zone_blob(self._map_dps.restricted_zone_up): |
There was a problem hiding this comment.
set_restricted_zones replaces the complete zone collection on the device without reading or merging self._map_dps.restricted_zone_up. Let's remove is_replaceable_zone_blob so set_restricted_zones can be called directly just like set_virtual_walls.
|
|
||
| def encode_restricted_zones(zones: Sequence[Q10RestrictedZone]) -> str: | ||
| """Encode the complete Q10 restricted-zone collection.""" | ||
| if isinstance(zones, (str, bytes)) or len(zones) > 255: |
There was a problem hiding this comment.
Per AGENTS.md (Trust Type Annotations), please remove the runtime isinstance checks on statically typed parameters (isinstance(zones, (str, bytes)), isinstance(zone, Q10RestrictedZone), isinstance(zone.type, ...), isinstance(walls, ...), isinstance(wall, ...), isinstance(room_id, ...), isinstance(name, str)) across encode_restricted_zones, encode_virtual_walls, and encode_room_name, along with the # type: ignore[arg-type] type-error tests in test_b01_q10_protocol.py and test_b01_q10_containers.py.
| ], | ||
| ) | ||
| def test_q10_roborock_point_vector_conversion(vector: tuple[int, int], expected: Q10RoborockPoint) -> None: | ||
| def test_upstream_q10_roborock_point_vector_conversion(vector: tuple[int, int], expected: Q10RoborockPoint) -> None: |
There was a problem hiding this comment.
It looks like conflict resolution renamed the existing tests on main to test_upstream_* and added duplicate tests above them. Please deduplicate these tests.
| } | ||
| ) | ||
|
|
||
| with patch.object(q10_api.command, "send") as send: |
There was a problem hiding this comment.
Please use the fake channel fixture (mock_channel.published_commands) rather than patch.object(q10_api.command, "send") here and in the restriction/wall/room-name setter tests below.
This branch has not been deployed
4 participants
Q10 map management needs typed overlay data and commands that preserve the device's authoritative state. This adds read-only restrictions and walls, full-collection replacement setters, room rename, and saved-map apply.
Scope and dependencies
This is a cumulative branch containing the archive, obstacle, and current-room work from #937–#939; archive parser support from #936 is merged. It now uses the composed
Q10CleanRecordDetailAPI and shares the archive lifecycle fixes with those PRs.After the archive stack lands, the planned partition is:
The common-coordinate
Q10RoborockPoint, current position, zoned cleaning, and goto support are already upstream through #908. This branch reuses that coordinate convention. #965 is merged. Upstreammainthroughb3a98b9(7.12.0) is integrated.Behavior
applychanges the active map asynchronously;selectrequests a preview.Allen’s follow-up feedback on merged #936 is addressed by separate clean-record and saved-map decoder tests, with no conditional assertions.
Validation
Validation of
fe0bfaa:uv run pytest -qon Python 3.13: 1,575 passed, 53 xfailed, 92 snapshots passed.uv buildpassed for both wheel and sdist.No physical-device run was performed for this revision.
Earlier physical Q10 S5+ validation exercised temporary no-go zones, virtual walls, room rename, and saved-map apply/readback, then restored the original values. A single saved map prevented validation of a cross-map transition. An earlier wheel was also exercised in Home Assistant.
Limitations
Inherited archive requests now have bounded waits and teardown recovery. Clean-history selection remains blocked after an uncertain outcome until the abandoned response is drained, because responses carry no record ID. Room split/merge, map deletion/reset, and map rename are outside this contribution. Customized cleaning controls are tracked in #935.
No credentials, personal room labels, floorplans, or private packet captures are included. Related: #767.