Repository navigation
Conversation
|
On hold following the review feedback on #933: please do not review the current cumulative diff. After the read-only and map-control stacks land, this draft will be rebased and repartitioned into focused changes:
Each part will be submitted sequentially against |
# Conflicts: # roborock/devices/traits/b01/q10/maps.py # tests/devices/traits/b01/q10/test_clean_history.py
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.
I know this PR is marked as draft while waiting on the earlier PRs in the stack, so leaving high-level architectural feedback now to save rework when you rebase and split it:
-
Align
RoomCleaningTraitwith the standard Q10 push pattern:- Similar to the feedback on #937,
RoomCleaningTraitshouldn't emulate a blocking RPC with_write_lock,_pending_confirmation: asyncio.Future,asyncio.shield,_active_taskcancellation inclose(),_map_revision, andmap_changed_callbackwired intoMapsTrait. - Instead, keep
RoomCleaningTraitas a straightforward push-drivenUpdatableTrait:refresh()publishesCUSTOMER_CLEAN_REQUEST(DP 63: 0).set_settings(settings)encodes and publishesCUSTOMER_CLEAN(DP 62) without requiringrefresh()to have completed first or waiting on a confirmation future.clean(settings)publishesCUSTOMER_CLEAN,CLEAN_MODE, andSTART_CLEAN.update_from_dps()updatesself.settingsand notifies listeners.
- Similar to the feedback on #937,
-
Single
Q10RoomCleanSettings(RoborockBase)model without| intunions:- Rather than having separate
Q10RoomCleanSettingsandQ10ReportedRoomCleanSettingsmodels where every enum field is typed asEnumType | int, let's fall back to.UNKNOWNon unrecognized wire codes and use a singleQ10RoomCleanSettingsmodel.
- Rather than having separate
-
Remove hardcoded
model == "roborock.vacuum.ss07"gating:- All Q10 devices are routed by the
ssmodel prefix indevice_manager.py, and existingVacuumTraitmethods (set_clean_mode,set_fan_level,clean_zone,goto_position) are not gated toss07only. Let's removeadvanced_cleaning_supported/_raise_if_advanced_cleaning_unsupported().
- All Q10 devices are routed by the
-
Remove defensive runtime
isinstancechecks on statically typed parameters:- See inline comments in
roborock/protocols/b01_q10_protocol.pyandroborock/devices/traits/b01/q10/vacuum.py.
- See inline comments in
| if self._active_task is not None: | ||
| self._active_task.cancel() | ||
|
|
||
| async def _publish_and_confirm(self, payload: str, *, request_complete: bool = False) -> None: |
There was a problem hiding this comment.
Please remove the confirmation future (_publish_and_confirm, _pending_confirmation, _pending_payload), _write_lock, _active_task, _map_revision, _validate_room_ids, and map_changed_callback on MapsTrait, along with the corresponding concurrency/cancellation/map-invalidation tests in test_room_cleaning.py. set_settings() and clean() should simply publish commands via self._command.send(...) like other Q10 traits.
|
|
||
|
|
||
| @dataclass | ||
| class Q10ReportedRoomCleanSettings(RoborockBase): |
There was a problem hiding this comment.
Add UNKNOWN = "unknown", -1 to YXCleanLine and map unknown wire codes to .UNKNOWN in decode_room_clean_settings so we only need a single Q10RoomCleanSettings(RoborockBase) model with concrete enum types (instead of a second Q10ReportedRoomCleanSettings class with | int unions).
| """Initialize the B01Props API.""" | ||
| self._channel = channel | ||
| self.command = CommandTrait(channel) | ||
| advanced_cleaning_supported = model is None or model == "roborock.vacuum.ss07" |
There was a problem hiding this comment.
Let's remove advanced_cleaning_supported (and model here and on RoomCleaningTrait / VacuumTrait) so all Q10 (ss*) devices share the same trait behavior.
|
|
||
| async def set_clean_mode(self, mode: YXCleanType) -> None: | ||
| """Set the cleaning mode (vacuum, mop, or both).""" | ||
| if not isinstance(mode, YXCleanType) or mode is YXCleanType.UNKNOWN: |
There was a problem hiding this comment.
Remove the runtime isinstance checks on statically typed mode and level parameters.
|
|
||
| def encode_room_clean_settings(settings: Sequence[Q10RoomCleanSettings]) -> str: | ||
| """Encode the compact six-byte-per-room ``dpCustomerClean`` write.""" | ||
| if isinstance(settings, (str, bytes)): |
There was a problem hiding this comment.
Per AGENTS.md (Trust Type Annotations), please remove the runtime isinstance checks on statically typed parameters here and in decode_room_clean_settings, along with the # type: ignore[arg-type] tests in test_b01_q10_protocol.py.
This branch has not been deployed
2 participants
Q10 customized cleaning requires typed settings and device confirmation before starting a selected-room clean. This adds global water/pass/route controls, per-room settings codecs, and a confirmation-driven room-cleaning trait.
Scope and dependencies
This draft contains the cumulative map-control work from #934 and archive work from #937–#939; archive parser support from #936 is merged. Its inherited archive implementation now uses the composed
Q10CleanRecordDetailAPI and shared lifecycle fixes from the archive stack.After the preceding work lands, the planned partition is:
Position, zoned cleaning, goto, and the common-coordinate point are already upstream through #908. #965 is merged. Upstream
mainthroughb3a98b9(7.12.0) is integrated.Behavior
RoborockBasemodels. Writes accept supported enums; reads preserve unknown integer values. Command snapshots and private reported-state copies protect command targets from caller mutation.COMMON(101); cleaning mode (137) and selected-room start (201) use direct data points.roborock.vacuum.ss07model. The direct Q10 factory defaults to enabling them.Lifecycle and map validity
Room operations snapshot their inputs, capture the current map revision, and validate room IDs after acquiring the existing write lock. An operation queued across map invalidation is rejected even if a refreshed map reuses the same room IDs. Map validity is checked again after confirmation and before publishing the selected-room start.
Local saved-map apply invalidates settings before publishing. The confirmation deadline includes settings publication and the full-response request. Device teardown invalidates settings and cancels the active room command sequence; future cleanup also handles invalidation during a failed or cancelled publish.
These guards cannot retract a command already published before a map change is observed. Reads and setting confirmations have no map-generation identifier, so concurrent changes from another client still require authoritative device updates. Inherited clean-history requests retain their documented late-response ambiguity barrier.
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
6c8075d:uv run pytest -qon Python 3.13: 1,655 passed, 53 xfailed, 92 snapshots passed.uv buildpassed for both wheel and sdist.No physical-device run was performed for this revision.
Coverage includes sanitized physical fixtures, simulator dispatch, matching confirmations, timeout/cancellation, caller mutation, concurrent writes, queued operations across map invalidation, invalidation after confirmation or during mode publication, early invalidation during map apply, stalled publication, and device teardown.
Earlier Q10 S5+ validation exercised global setting round trips and a customized single-room clean, then restored the original presets and normal cleaning mode. The post-dock full-response fallback was verified on the device. An earlier wheel was exercised in Home Assistant; the hardware scenarios have not been rerun for this revision.
Home Assistant controls and destructive map operations are outside this contribution. No credentials, personal room labels, floorplans, or private raw captures are included. Related: #767.