Repository navigation
feat(v1): expose per-room clean-then-mop via customize clean mode #984
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 all commits
026f6b1
515b9ba
4142f21
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 |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| """Trait for per-room (segment) cleaning mode overrides on Roborock devices. | ||
|
|
||
| This exposes the same "vacuum then mop" axis as `StatusTrait.clean_then_mop` | ||
| (the global, per-job flag), but scoped to a single room. The global flag is | ||
| itself persisted on the device - tracked while docked, not cleared after | ||
| every clean (see `StatusTrait.clean_then_mop`) - and a per-room override set | ||
| here is likewise persisted: it is a preference attached to that room and is | ||
| re-applied whenever the room is cleaned, until overwritten or cleared from | ||
| the vendor app. | ||
|
|
||
| Rooms without an override are simply absent from `modes` - the robot falls | ||
| back to the global settings (fan power, water box mode, mop mode and | ||
| clean-then-mop) for those. An empty list means no room currently has one. | ||
| """ | ||
|
|
||
| from dataclasses import dataclass | ||
|
|
||
| from roborock.data import RoborockBase | ||
| from roborock.devices.traits.v1 import common | ||
| from roborock.roborock_typing import RoborockCommand | ||
|
|
||
| from .common import V1ResponseData | ||
|
|
||
| _SEQ_TYPE_CLEAN_THEN_MOP = 1 | ||
|
|
||
|
|
||
| @dataclass | ||
| class SegmentCleanMode(RoborockBase): | ||
| """A single room's clean mode override, as reported by the device.""" | ||
|
|
||
| segment: int | ||
| fan_power: int | None = None | ||
| water_box_mode: int | None = None | ||
| mop_mode: int | None = None | ||
| repeat: int | None = None | ||
| seq_type: int | None = None | ||
|
|
||
| @property | ||
| def clean_then_mop(self) -> bool | None: | ||
| """Whether this room is vacuumed fully before it is mopped. | ||
|
|
||
| `None` means the device did not report a sequence type for this | ||
| room's override. | ||
| """ | ||
| if self.seq_type is None: | ||
| return None | ||
| return self.seq_type == _SEQ_TYPE_CLEAN_THEN_MOP | ||
|
|
||
|
|
||
| @dataclass | ||
| class CustomizeCleanModes(RoborockBase): | ||
| """Collection of per-room clean mode overrides.""" | ||
|
|
||
| modes: list[SegmentCleanMode] | None = None | ||
|
|
||
| @property | ||
| def by_segment(self) -> dict[int, SegmentCleanMode]: | ||
| """Index the overrides by segment id.""" | ||
| return {mode.segment: mode for mode in self.modes or []} | ||
|
|
||
|
|
||
| class CustomizeCleanModeConverter(common.V1TraitDataConverter): | ||
| """Converts get_customize_clean_mode responses to CustomizeCleanModes.""" | ||
|
|
||
| def convert(self, response: V1ResponseData) -> CustomizeCleanModes: | ||
| """Parse the response from the device into a CustomizeCleanModes.""" | ||
| if not isinstance(response, list) or not all(isinstance(entry, dict) for entry in response): | ||
| raise ValueError(f"Unexpected CustomizeCleanModes response format: {response!r}") | ||
| return CustomizeCleanModes(modes=[SegmentCleanMode.from_dict(entry) for entry in response]) | ||
|
|
||
|
|
||
| class CustomizeCleanModeTrait(CustomizeCleanModes, common.V1TraitMixin): | ||
| """Trait for reading and writing per-room clean mode overrides. | ||
|
|
||
| This is the per-room counterpart to `StatusTrait.clean_then_mop` / | ||
| `StatusTrait.set_clean_then_mop`. Gated on `is_customized_clean_supported` | ||
| - the feature that covers `get/set_customize_clean_mode` in general, not | ||
| `is_clean_then_mop_mode_supported` (which only gates the narrower | ||
| `seq_type` axis within it). A device can support per-room custom modes | ||
| without supporting clean-then-mop specifically; on such a device | ||
| `SegmentCleanMode.seq_type` (and therefore `.clean_then_mop`) is simply | ||
| `None` for every room, same as when the device never reports it. | ||
| """ | ||
|
|
||
| command = RoborockCommand.GET_CUSTOMIZE_CLEAN_MODE | ||
| converter = CustomizeCleanModeConverter() | ||
| requires_feature = "is_customized_clean_supported" | ||
|
|
||
|
Comment on lines
+72
to
+88
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. Feature gating on
Suggested fix: Inject |
||
| async def set_clean_then_mop( | ||
| self, | ||
| segment: int, | ||
| enabled: bool, | ||
| *, | ||
| fan_power: int, | ||
| water_box_mode: int, | ||
| mop_mode: int, | ||
| repeat: int = 1, | ||
| ) -> None: | ||
|
Comment on lines
+89
to
+98
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. Public trait API contract & raw wire integers (
Suggested fix: Expose a general per-room mode setter on |
||
| """Set whether a single room is vacuumed fully before it is mopped. | ||
|
|
||
| The firmware requires the room's full clean mode alongside the | ||
| sequence type, not the flag on its own, so callers pass through the | ||
| fan power / water box mode / mop mode that room already cleans with | ||
| - typically read from the room's existing `SegmentCleanMode` (via | ||
| `by_segment`) - so this call only changes the order, not the | ||
| settings. | ||
|
|
||
| Do NOT source these from `StatusTrait` as a fallback when a room has | ||
| no override yet: while the device is in a per-room custom cleaning | ||
| mode, `StatusTrait` itself reports the sentinel "customized" codes | ||
| (`fan_power=106`, `water_box_mode=204`, `mop_mode=302`) rather than | ||
| real settings, and those sentinels are not valid values to echo back | ||
| here. | ||
|
|
||
| `fan_power`, `water_box_mode` and `mop_mode` are all required: the | ||
| device rejects the request outright (`Param error, no mop_mode`) if | ||
| `mop_mode` is left out, confirmed against a real device - this isn't | ||
| a case where a missing field is silently defaulted. | ||
|
|
||
| `repeat` defaults to `1` rather than being left out when the caller | ||
| doesn't have an opinion: omitting it from the wire request is NOT a | ||
| no-op on the device, which resets it to `1` instead of preserving | ||
| whatever the room's `repeat` was set to before this call - confirmed | ||
| against a real device. Pass the room's current `repeat` explicitly if | ||
| you want a different value preserved. | ||
| """ | ||
| seq_type = _SEQ_TYPE_CLEAN_THEN_MOP if enabled else 0 | ||
| entry: dict[str, int] = { | ||
| "segment": segment, | ||
| "fan_power": fan_power, | ||
|
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. is it ok to overwrite this with 0 when not specified? It seems almost like this is intended to be used where you need to pass back the previous state of SegmentCleanMode when you want to make a change. Just curious: Did you consider trying to make this work with the
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. @C:/Users/jevge/AppData/Local/Temp/claude/shots/review-reply2.txt
Collaborator
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. Seems like the reply here didn't actually post @jevgenijostin |
||
| "water_box_mode": water_box_mode, | ||
| "mop_mode": mop_mode, | ||
| "repeat": repeat, | ||
| "seq_type": seq_type, | ||
| } | ||
| await self.rpc_channel.send_command(RoborockCommand.SET_CUSTOMIZE_CLEAN_MODE, params=[entry]) | ||
| await self.refresh() | ||
|
Comment on lines
+127
to
+137
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. Multi-room override data loss & unmodeled wire keys on write (
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| """Tests for the CustomizeCleanModeTrait class.""" | ||
|
|
||
| from unittest.mock import AsyncMock, call | ||
|
|
||
| import pytest | ||
|
|
||
| from roborock.devices.traits.v1.customize_clean_mode import ( | ||
| CustomizeCleanModeTrait, | ||
| SegmentCleanMode, | ||
| ) | ||
| from roborock.exceptions import RoborockParsingException | ||
| from roborock.roborock_typing import RoborockCommand | ||
|
|
||
| CUSTOMIZE_CLEAN_MODE_DATA = [ | ||
| {"segment": 4, "fan_power": 102, "water_box_mode": 203, "mop_mode": 300, "repeat": 1, "seq_type": 0}, | ||
| {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "repeat": 1, "seq_type": 1}, | ||
| ] | ||
|
|
||
|
|
||
| @pytest.fixture(name="customize_clean_mode_trait") | ||
| def customize_clean_mode_trait_fixture(mock_rpc_channel: AsyncMock) -> CustomizeCleanModeTrait: | ||
| """Create a CustomizeCleanModeTrait with mocked dependencies. | ||
|
|
||
| This is constructed directly rather than via the `device` fixture: unlike | ||
| the dock-gated optional traits, there is no existing test helper for | ||
| parametrizing the `new_feature_info` bit this trait is gated on, and the | ||
| other `requires_feature`-gated optional traits (child_lock, led_status, | ||
| valley_electricity_timer) do not have discovery-level tests either. | ||
| """ | ||
| trait = CustomizeCleanModeTrait() | ||
| trait._rpc_channel = mock_rpc_channel # type: ignore[assignment] | ||
| return trait | ||
|
Comment on lines
+20
to
+32
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. Use Two things in
from roborock.devices.device import RoborockDevice
from tests import mock_data
@pytest.fixture(name="customize_clean_mode_trait")
def customize_clean_mode_trait_fixture(
device: RoborockDevice,
discover_features_fixture: None,
) -> CustomizeCleanModeTrait | None:
"""Return the discovered CustomizeCleanModeTrait from the device."""
assert device.v1_properties
return device.v1_properties.customize_clean_mode
@pytest.mark.parametrize(
"device_info",
[mock_data.DEVICE_PRODUCT_PAIRS["home_data_device_s5e.json"][0]],
)
async def test_unsupported_customize_clean_mode(
customize_clean_mode_trait: CustomizeCleanModeTrait | None,
) -> None:
"""Trait is not initialized when is_customized_clean_supported is False."""
assert customize_clean_mode_trait is None |
||
|
|
||
|
|
||
| async def test_refresh(customize_clean_mode_trait: CustomizeCleanModeTrait, mock_rpc_channel: AsyncMock) -> None: | ||
| """Rooms with an override are parsed into SegmentCleanMode entries.""" | ||
| mock_rpc_channel.send_command.side_effect = [CUSTOMIZE_CLEAN_MODE_DATA] | ||
|
|
||
| await customize_clean_mode_trait.refresh() | ||
|
|
||
| mock_rpc_channel.send_command.assert_called_once_with(RoborockCommand.GET_CUSTOMIZE_CLEAN_MODE) | ||
| assert customize_clean_mode_trait.modes == [ | ||
| SegmentCleanMode(segment=4, fan_power=102, water_box_mode=203, mop_mode=300, repeat=1, seq_type=0), | ||
| SegmentCleanMode(segment=7, fan_power=101, water_box_mode=203, mop_mode=300, repeat=1, seq_type=1), | ||
| ] | ||
| assert customize_clean_mode_trait.by_segment[7].clean_then_mop is True | ||
| assert customize_clean_mode_trait.by_segment[4].clean_then_mop is False | ||
|
|
||
|
|
||
| async def test_refresh_no_overrides( | ||
| customize_clean_mode_trait: CustomizeCleanModeTrait, mock_rpc_channel: AsyncMock | ||
| ) -> None: | ||
| """An empty response means no room currently has a custom clean mode.""" | ||
| mock_rpc_channel.send_command.side_effect = [[]] | ||
|
|
||
| await customize_clean_mode_trait.refresh() | ||
|
|
||
| assert customize_clean_mode_trait.modes == [] | ||
| assert customize_clean_mode_trait.by_segment == {} | ||
|
|
||
|
|
||
| async def test_refresh_bad_format( | ||
| customize_clean_mode_trait: CustomizeCleanModeTrait, mock_rpc_channel: AsyncMock | ||
| ) -> None: | ||
| """A non-list response is a parsing error, not a silent empty result.""" | ||
| mock_rpc_channel.send_command.side_effect = [{"unexpected": "shape"}] | ||
|
|
||
| with pytest.raises(RoborockParsingException): | ||
| await customize_clean_mode_trait.refresh() | ||
|
|
||
|
|
||
| async def test_refresh_bad_list_entry( | ||
| customize_clean_mode_trait: CustomizeCleanModeTrait, mock_rpc_channel: AsyncMock | ||
| ) -> None: | ||
| """A malformed entry inside the list is a parsing error, not silently dropped.""" | ||
| mock_rpc_channel.send_command.side_effect = [["ok"]] | ||
|
|
||
| with pytest.raises(RoborockParsingException): | ||
| await customize_clean_mode_trait.refresh() | ||
|
|
||
|
|
||
| @pytest.mark.parametrize(("seq_type", "expected"), [(1, True), (0, False), (None, None)]) | ||
| def test_clean_then_mop(seq_type: int | None, expected: bool | None) -> None: | ||
| """seq_type reports whether a room is vacuumed before it is mopped.""" | ||
| mode = SegmentCleanMode(segment=1, seq_type=seq_type) | ||
|
|
||
| assert mode.clean_then_mop is expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("enabled", "repeat", "expected_params"), | ||
| [ | ||
| ( | ||
| True, | ||
| 2, | ||
| {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "repeat": 2, "seq_type": 1}, | ||
| ), | ||
| ( | ||
| True, | ||
| None, | ||
| {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "repeat": 1, "seq_type": 1}, | ||
| ), | ||
| ( | ||
| False, | ||
| None, | ||
| {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "repeat": 1, "seq_type": 0}, | ||
| ), | ||
| ], | ||
| ) | ||
| async def test_set_clean_then_mop( | ||
| customize_clean_mode_trait: CustomizeCleanModeTrait, | ||
| mock_rpc_channel: AsyncMock, | ||
| enabled: bool, | ||
| repeat: int | None, | ||
| expected_params: dict[str, int], | ||
| ) -> None: | ||
| """The setter always sends mop_mode and repeat (the device rejects a request missing | ||
| mop_mode, and silently resets repeat to 1 rather than preserving it if left out) alongside | ||
| the room's other settings and the sequence type, then refreshes.""" | ||
| mock_rpc_channel.send_command.side_effect = ["ok", CUSTOMIZE_CLEAN_MODE_DATA] | ||
| kwargs = {"fan_power": 101, "water_box_mode": 203, "mop_mode": 300} | ||
| if repeat is not None: | ||
| kwargs["repeat"] = repeat | ||
|
|
||
| await customize_clean_mode_trait.set_clean_then_mop(7, enabled, **kwargs) | ||
|
|
||
| assert mock_rpc_channel.send_command.mock_calls == [ | ||
| call(RoborockCommand.SET_CUSTOMIZE_CLEAN_MODE, params=[expected_params]), | ||
| call(RoborockCommand.GET_CUSTOMIZE_CLEAN_MODE), | ||
| ] | ||
| assert customize_clean_mode_trait.by_segment[7].clean_then_mop is True | ||
|
Comment on lines
+120
to
+131
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. Assertion mismatch in
Mock the refreshed payload using assert customize_clean_mode_trait.by_segment[7].clean_then_mop is enabled |
||
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.
Coupling to unmerged #959 & model placement (
AGENTS.mdPriority 1):CleaningModeoption (matching the vendor app's 4th mode tab, which also clearsseq_typeto0when switching to Vacuum-only or Mop-only) vs. a separate boolean property. Consequently,StatusTrait.clean_then_mopandStatusTrait.set_clean_then_mop(referenced here and on lines 75–76) do not exist onmain. Please decouple the docstrings (and consider whether per-room custom mode selection should align withCleaningModeonce feat(v1): expose clean-then-mop status from the device #959 settles).SegmentCleanModeandCustomizeCleanModestoroborock/data/v1/v1_containers.py(re-exported viaroborock.data), matchingMopDryerSetting,WashTowelMode, etc., so consumers can import domain models fromroborock.dataandtests/conformance/test_model_conformance.pyautomatically covers them. (Keeping rawint | Nonewire fields onSegmentCleanModelikeStatusV2is great so water-slide values like222round-trip losslessly.)