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
Open
jevgenijostin
wants to merge
3
commits into
Python-roborock:main
Choose a base branch
from
jevgenijostin:feat/per-room-clean-sequence
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
026f6b1
feat(v1): expose per-room clean-then-mop via customize clean mode
jevgenijostin 515b9ba
fix(v1): require mop_mode in set_clean_then_mop, document repeat reset
jevgenijostin 4142f21
fix(v1): correct feature gate, require repeat, reject malformed entries
jevgenijostin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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" | ||
|
|
||
| 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: | ||
| """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, | ||
| "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() | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | ||
|
|
||
|
|
||
| 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 |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Roomstrait? I can see it is fairly complex to do so, but ultimately i think to use this effectively its going to need to get the room information fromRoomsto get the needed segment information for display, so i think there could be benefit from trying to hide those details, but its not trivial.Uh oh!
There was an error while loading. Please reload this page.
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.
@C:/Users/jevge/AppData/Local/Temp/claude/shots/review-reply2.txt