From 026f6b1cba9df77357c31d7adea99b882b1bbc15 Mon Sep 17 00:00:00 2001 From: Jevgeni Jostin Date: Sat, 17 Oct 2026 17:12:49 +0200 Subject: [PATCH 1/4] feat(v1): expose per-room clean-then-mop via customize clean mode Follow-up to #959, which models the global get_status seq_type flag but explicitly leaves the per-room get/set_customize_clean_mode layer alone. Adds a CustomizeCleanModeTrait exposing each room's SegmentCleanMode (fan_power/water_box_mode/mop_mode/repeat/seq_type) and a set_clean_then_mop(segment, enabled, ...) setter, mirroring StatusTrait.clean_then_mop at room granularity. --- roborock/devices/traits/v1/__init__.py | 4 + .../devices/traits/v1/customize_clean_mode.py | 117 ++++++++++++++++++ .../traits/v1/test_customize_clean_mode.py | 112 +++++++++++++++++ 3 files changed, 233 insertions(+) create mode 100644 roborock/devices/traits/v1/customize_clean_mode.py create mode 100644 tests/devices/traits/v1/test_customize_clean_mode.py diff --git a/roborock/devices/traits/v1/__init__.py b/roborock/devices/traits/v1/__init__.py index e3f93d4b7..4ea9b64d8 100644 --- a/roborock/devices/traits/v1/__init__.py +++ b/roborock/devices/traits/v1/__init__.py @@ -74,6 +74,7 @@ command, common, consumeable, + customize_clean_mode, device_features, do_not_disturb, dust_collection_mode, @@ -98,6 +99,7 @@ from .command import CommandTrait from .common import V1TraitMixin from .consumeable import ConsumableTrait +from .customize_clean_mode import CustomizeCleanModeTrait from .device_features import DeviceFeaturesTrait from .do_not_disturb import DoNotDisturbTrait from .dust_collection_mode import DustCollectionModeTrait @@ -126,6 +128,7 @@ "command", "common", "consumeable", + "customize_clean_mode", "device_features", "do_not_disturb", "dust_collection_mode", @@ -179,6 +182,7 @@ class PropertiesApi(Trait): smart_wash_params: SmartWashParamsTrait | None = None obstacle_photos: ObstaclePhotoTrait | None = None mop_dryer: MopDryerTrait | None = None + customize_clean_mode: CustomizeCleanModeTrait | None = None def __init__( self, diff --git a/roborock/devices/traits/v1/customize_clean_mode.py b/roborock/devices/traits/v1/customize_clean_mode.py new file mode 100644 index 000000000..e348cc306 --- /dev/null +++ b/roborock/devices/traits/v1/customize_clean_mode.py @@ -0,0 +1,117 @@ +"""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. Where the global +flag is per-job and clears after every clean, a per-room override set here +is persisted on the device: 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): + raise ValueError(f"Unexpected CustomizeCleanModes response format: {response!r}") + return CustomizeCleanModes( + modes=[SegmentCleanMode.from_dict(entry) for entry in response if isinstance(entry, dict)] + ) + + +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`. Both are gated by the same device + feature: no device has been found that supports the global toggle but + not the per-room one, since the vendor app presents them as a single + "vacuum then mop" switch that can be applied globally or per room. + """ + + command = RoborockCommand.GET_CUSTOMIZE_CLEAN_MODE + converter = CustomizeCleanModeConverter() + requires_feature = "is_clean_then_mop_mode_supported" + + async def set_clean_then_mop( + self, + segment: int, + enabled: bool, + *, + fan_power: int, + water_box_mode: int, + mop_mode: int | None = None, + repeat: int | None = None, + ) -> 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 (and optionally mop mode / repeat) that + room already cleans with - typically read from `StatusTrait` - so + this call only changes the order, not the settings. + """ + 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, + "seq_type": seq_type, + } + if mop_mode is not None: + entry["mop_mode"] = mop_mode + if repeat is not None: + entry["repeat"] = repeat + await self.rpc_channel.send_command(RoborockCommand.SET_CUSTOMIZE_CLEAN_MODE, params=[entry]) + await self.refresh() diff --git a/tests/devices/traits/v1/test_customize_clean_mode.py b/tests/devices/traits/v1/test_customize_clean_mode.py new file mode 100644 index 000000000..46f056c67 --- /dev/null +++ b/tests/devices/traits/v1/test_customize_clean_mode.py @@ -0,0 +1,112 @@ +"""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() + + +@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", "mop_mode", "repeat", "expected_params"), + [ + ( + True, + 300, + 1, + {"segment": 7, "fan_power": 101, "water_box_mode": 203, "seq_type": 1, "mop_mode": 300, "repeat": 1}, + ), + (True, None, None, {"segment": 7, "fan_power": 101, "water_box_mode": 203, "seq_type": 1}), + (False, 300, None, {"segment": 7, "fan_power": 101, "water_box_mode": 203, "seq_type": 0, "mop_mode": 300}), + ], +) +async def test_set_clean_then_mop( + customize_clean_mode_trait: CustomizeCleanModeTrait, + mock_rpc_channel: AsyncMock, + enabled: bool, + mop_mode: int | None, + repeat: int | None, + expected_params: dict[str, int], +) -> None: + """The setter sends the room's full clean mode alongside the sequence type, then refreshes.""" + mock_rpc_channel.send_command.side_effect = ["ok", CUSTOMIZE_CLEAN_MODE_DATA] + + await customize_clean_mode_trait.set_clean_then_mop( + 7, enabled, fan_power=101, water_box_mode=203, mop_mode=mop_mode, repeat=repeat + ) + + 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 From 515b9ba57367522103c0c60572177c4b831d0bd7 Mon Sep 17 00:00:00 2001 From: Jevgeni Jostin Date: Sat, 17 Oct 2026 22:43:56 +0200 Subject: [PATCH 2/4] fix(v1): require mop_mode in set_clean_then_mop, document repeat reset Tested against a real device: omitting mop_mode makes the device reject the request outright ("Param error, no mop_mode"), it is not silently defaulted. Omitting repeat does not preserve the room's existing value either - the device resets it to 1. Address review feedback on #984. --- .../devices/traits/v1/customize_clean_mode.py | 23 ++++++++++++++----- .../traits/v1/test_customize_clean_mode.py | 15 ++++++------ 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/roborock/devices/traits/v1/customize_clean_mode.py b/roborock/devices/traits/v1/customize_clean_mode.py index e348cc306..8ebf87d73 100644 --- a/roborock/devices/traits/v1/customize_clean_mode.py +++ b/roborock/devices/traits/v1/customize_clean_mode.py @@ -91,26 +91,37 @@ async def set_clean_then_mop( *, fan_power: int, water_box_mode: int, - mop_mode: int | None = None, + mop_mode: int, repeat: int | None = None, ) -> 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 (and optionally mop mode / repeat) that - room already cleans with - typically read from `StatusTrait` - so - this call only changes the order, not the settings. + fan power / water box mode / mop mode that room already cleans with + - typically read from the room's existing `SegmentCleanMode` (via + `by_segment`) or, if the room has no override yet, from `StatusTrait` + - so this call only changes the order, not the settings. + + `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` is the one genuinely optional field, but omitting it is NOT + a no-op: the device resets it to its own default (observed: 1) + rather than preserving whatever the room's `repeat` was set to + before this call - also confirmed against a real device. Pass the + room's current `repeat` explicitly if you want it 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, "seq_type": seq_type, } - if mop_mode is not None: - entry["mop_mode"] = mop_mode if repeat is not None: entry["repeat"] = repeat await self.rpc_channel.send_command(RoborockCommand.SET_CUSTOMIZE_CLEAN_MODE, params=[entry]) diff --git a/tests/devices/traits/v1/test_customize_clean_mode.py b/tests/devices/traits/v1/test_customize_clean_mode.py index 46f056c67..9ad652288 100644 --- a/tests/devices/traits/v1/test_customize_clean_mode.py +++ b/tests/devices/traits/v1/test_customize_clean_mode.py @@ -78,31 +78,30 @@ def test_clean_then_mop(seq_type: int | None, expected: bool | None) -> None: @pytest.mark.parametrize( - ("enabled", "mop_mode", "repeat", "expected_params"), + ("enabled", "repeat", "expected_params"), [ ( True, - 300, 1, - {"segment": 7, "fan_power": 101, "water_box_mode": 203, "seq_type": 1, "mop_mode": 300, "repeat": 1}, + {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "seq_type": 1, "repeat": 1}, ), - (True, None, None, {"segment": 7, "fan_power": 101, "water_box_mode": 203, "seq_type": 1}), - (False, 300, None, {"segment": 7, "fan_power": 101, "water_box_mode": 203, "seq_type": 0, "mop_mode": 300}), + (True, None, {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "seq_type": 1}), + (False, None, {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "seq_type": 0}), ], ) async def test_set_clean_then_mop( customize_clean_mode_trait: CustomizeCleanModeTrait, mock_rpc_channel: AsyncMock, enabled: bool, - mop_mode: int | None, repeat: int | None, expected_params: dict[str, int], ) -> None: - """The setter sends the room's full clean mode alongside the sequence type, then refreshes.""" + """The setter always sends mop_mode (the device rejects the request without it) alongside + the room's other settings and the sequence type, then refreshes.""" mock_rpc_channel.send_command.side_effect = ["ok", CUSTOMIZE_CLEAN_MODE_DATA] await customize_clean_mode_trait.set_clean_then_mop( - 7, enabled, fan_power=101, water_box_mode=203, mop_mode=mop_mode, repeat=repeat + 7, enabled, fan_power=101, water_box_mode=203, mop_mode=300, repeat=repeat ) assert mock_rpc_channel.send_command.mock_calls == [ From 4142f219d6ffa7bc8758cdbf30e36f1d0bb5d39d Mon Sep 17 00:00:00 2001 From: Jevgeni Jostin Date: Sun, 18 Oct 2026 08:08:49 +0200 Subject: [PATCH 3/4] fix(v1): correct feature gate, require repeat, reject malformed entries - Gate CustomizeCleanModeTrait on is_customized_clean_supported (the feature that actually covers get/set_customize_clean_mode) instead of is_clean_then_mop_mode_supported (which only gates the seq_type axis within it) - a device can support per-room custom modes without clean-then-mop specifically. - repeat now defaults to 1 instead of being left out of the wire request when unset: omitting it resets the room's repeat to 1 on the device rather than preserving the previous value, confirmed against a real device, so leaving it Optional was a footgun. - CustomizeCleanModeConverter now rejects a response list containing a non-dict entry instead of silently dropping it. - Corrected the module docstring's claim that the global clean_then_mop flag is per-job and clears after every clean - it's persisted, per StatusTrait.clean_then_mop's own docstring from #959. - Documented that StatusTrait reports sentinel "customized" codes (fan_power=106/water_box_mode=204/mop_mode=302) while a per-room custom mode is active, so it must not be used as a fallback source for those values when calling set_clean_then_mop on a room with no prior override. Addresses review feedback on #984. --- .../devices/traits/v1/customize_clean_mode.py | 53 +++++++++++-------- .../traits/v1/test_customize_clean_mode.py | 36 ++++++++++--- 2 files changed, 59 insertions(+), 30 deletions(-) diff --git a/roborock/devices/traits/v1/customize_clean_mode.py b/roborock/devices/traits/v1/customize_clean_mode.py index 8ebf87d73..e71f76f9b 100644 --- a/roborock/devices/traits/v1/customize_clean_mode.py +++ b/roborock/devices/traits/v1/customize_clean_mode.py @@ -1,9 +1,10 @@ """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. Where the global -flag is per-job and clears after every clean, a per-room override set here -is persisted on the device: it is a preference attached to that room and is +(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. @@ -63,26 +64,27 @@ class CustomizeCleanModeConverter(common.V1TraitDataConverter): def convert(self, response: V1ResponseData) -> CustomizeCleanModes: """Parse the response from the device into a CustomizeCleanModes.""" - if not isinstance(response, list): + 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 if isinstance(entry, dict)] - ) + 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`. Both are gated by the same device - feature: no device has been found that supports the global toggle but - not the per-room one, since the vendor app presents them as a single - "vacuum then mop" switch that can be applied globally or per room. + `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_clean_then_mop_mode_supported" + requires_feature = "is_customized_clean_supported" async def set_clean_then_mop( self, @@ -92,7 +94,7 @@ async def set_clean_then_mop( fan_power: int, water_box_mode: int, mop_mode: int, - repeat: int | None = None, + repeat: int = 1, ) -> None: """Set whether a single room is vacuumed fully before it is mopped. @@ -100,19 +102,27 @@ async def set_clean_then_mop( 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`) or, if the room has no override yet, from `StatusTrait` - - so this call only changes the order, not the settings. + `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` is the one genuinely optional field, but omitting it is NOT - a no-op: the device resets it to its own default (observed: 1) - rather than preserving whatever the room's `repeat` was set to - before this call - also confirmed against a real device. Pass the - room's current `repeat` explicitly if you want it preserved. + `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] = { @@ -120,9 +130,8 @@ async def set_clean_then_mop( "fan_power": fan_power, "water_box_mode": water_box_mode, "mop_mode": mop_mode, + "repeat": repeat, "seq_type": seq_type, } - if repeat is not None: - entry["repeat"] = repeat await self.rpc_channel.send_command(RoborockCommand.SET_CUSTOMIZE_CLEAN_MODE, params=[entry]) await self.refresh() diff --git a/tests/devices/traits/v1/test_customize_clean_mode.py b/tests/devices/traits/v1/test_customize_clean_mode.py index 9ad652288..447221eaa 100644 --- a/tests/devices/traits/v1/test_customize_clean_mode.py +++ b/tests/devices/traits/v1/test_customize_clean_mode.py @@ -69,6 +69,16 @@ async def test_refresh_bad_format( 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.""" @@ -82,11 +92,19 @@ def test_clean_then_mop(seq_type: int | None, expected: bool | None) -> None: [ ( True, - 1, - {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "seq_type": 1, "repeat": 1}, + 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}, ), - (True, None, {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "seq_type": 1}), - (False, None, {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "seq_type": 0}), ], ) async def test_set_clean_then_mop( @@ -96,13 +114,15 @@ async def test_set_clean_then_mop( repeat: int | None, expected_params: dict[str, int], ) -> None: - """The setter always sends mop_mode (the device rejects the request without it) alongside + """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, fan_power=101, water_box_mode=203, mop_mode=300, 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]), From a1b3279014c2128b2a7362b33f12141491710af8 Mon Sep 17 00:00:00 2001 From: Jevgeni Jostin Date: Tue, 3 Nov 2026 08:39:34 +0100 Subject: [PATCH 4/4] fix(v1): serialize writes and round-trip unmodeled fields losslessly Addresses a second review round on #984 (allenporter, 2026-11-01) plus two rounds of independent review by a second AI reviewer, both of which reproduced real bugs rather than just raising theoretical concerns: - Move SegmentCleanMode/CustomizeCleanModes to data/v1/v1_containers.py (re-exported via roborock.data), matching how other trait container models (WashTowelMode, MopDryerSetting, etc.) are organized. Decouple the module docstring from unmerged #959 (StatusTrait.clean_then_mop doesn't exist on main). - CustomizeCleanModeTrait now takes a DeviceFeaturesTrait in __init__ (mirroring WashTowelModeTrait) instead of relying on the dataclass MRO to run V1TraitMixin.__init__, which it silently didn't. set_clean_then_mop now raises RoborockUnsupportedFeature up front if is_clean_then_mop_mode_supported is False, instead of writing seq_type to firmware that doesn't support it - many per-room-custom-mode devices (S7 MaxV, S8, Qrevo) don't support clean-then-mop specifically. - set_clean_then_mop's fan_power/water_box_mode/mop_mode now accept the typed VacuumModes/WaterModes/CleanRoutes enums (or a raw int, for codes this library doesn't model) instead of only raw ints. - The read-merge-write sequence (refresh, build payload, SET, refresh) is now serialized per trait instance via an asyncio.Lock. Reproduced the race without it: two concurrent calls enabling different rooms each read the same stale table, and the second call's write silently reverted the first call's room. - set_clean_then_mop sends the full known table on every write, not a single-entry list: SET_CUSTOMIZE_CLEAN_MODE replaces the whole customized-segment table on at least some firmware (first-hand Saros 20 Sonic report: https://github.com/orgs/home-assistant/discussions/3998), so a single-entry write silently wiped other rooms' overrides. - Untouched rooms are round-tripped from their raw wire dict (now cached by refresh(), keyed by the canonical typed segment id - not the raw dict's own possibly-differently-cased/typed "segment" key, which reproducibly desynced the cache from by_segment) rather than reconstructed from typed SegmentCleanMode fields, so fields this library doesn't model at all (not just the two now added to the dataclass) survive a write instead of being silently dropped. A response with two entries resolving to the same segment id now raises RoborockParsingException instead of one silently overwriting the other in the cache. - Tests: switched the fixture to real device discovery (device + discover_features_fixture, Saros 10R for the clean-then-mop-supported path, S7 MaxV default for the unsupported path, S5E for is_customized_clean_supported=False) instead of constructing the trait directly and poking private attributes. Fixed the enabled=False case never actually being exercised (the mocked post-write refresh was a static always-enabled fixture regardless of what was written). Added regression tests for multi-room preservation, unmodeled-field preservation (including on an untouched room), concurrent-call serialization (verified to actually fail without the lock), and the three segment-identity cases above (also verified to fail against the prior raw-indexing approach). Addresses review feedback on #984. --- roborock/data/v1/v1_containers.py | 50 +++ roborock/devices/traits/v1/__init__.py | 7 + .../devices/traits/v1/customize_clean_mode.py | 203 ++++++--- .../traits/v1/test_customize_clean_mode.py | 407 ++++++++++++++++-- 4 files changed, 566 insertions(+), 101 deletions(-) diff --git a/roborock/data/v1/v1_containers.py b/roborock/data/v1/v1_containers.py index 46ff7d49f..65bcba64a 100644 --- a/roborock/data/v1/v1_containers.py +++ b/roborock/data/v1/v1_containers.py @@ -515,6 +515,56 @@ class WashTowelMode(RoborockBase): wash_mode: WashTowelModes | None = None +SEQ_TYPE_CLEAN_THEN_MOP = 1 + + +@dataclass +class SegmentCleanMode(RoborockBase): + """A single room's clean mode override, as reported by the device. + + `fan_power` / `water_box_mode` / `mop_mode` / `seq_type` are kept as raw + `int | None` wire codes rather than typed enums, matching `StatusV2`, so + model-specific values (e.g. water-slide codes like 222) round-trip + losslessly even for codes this library doesn't otherwise model. + + `mop_power` and `custom_water_box_distance` are only reported by some + models (roller-mop / custom-water-flow devices); they are `None` on + models that don't report them. + """ + + 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 + mop_power: int | None = None + custom_water_box_distance: 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 []} + + @dataclass class NetworkInfo(RoborockBase): ip: str diff --git a/roborock/devices/traits/v1/__init__.py b/roborock/devices/traits/v1/__init__.py index 4ea9b64d8..d8af621b2 100644 --- a/roborock/devices/traits/v1/__init__.py +++ b/roborock/devices/traits/v1/__init__.py @@ -288,6 +288,13 @@ async def discover_features(self) -> None: wash_towel_mode._rpc_channel = self._get_rpc_channel(wash_towel_mode) # type: ignore[assignment] self.wash_towel_mode = wash_towel_mode + if self.customize_clean_mode is None and self._is_supported( + CustomizeCleanModeTrait, "customize_clean_mode", dock_features + ): + customize_clean_mode = CustomizeCleanModeTrait(self.device_features) + customize_clean_mode._rpc_channel = self._get_rpc_channel(customize_clean_mode) # type: ignore[assignment] + self.customize_clean_mode = customize_clean_mode + if self.obstacle_photos is None and self._is_supported(ObstaclePhotoTrait, "obstacle_photos", dock_features): obstacle_photos = ObstaclePhotoTrait(self._rpc_channel) obstacle_photos._rpc_channel = self._get_rpc_channel(obstacle_photos) diff --git a/roborock/devices/traits/v1/customize_clean_mode.py b/roborock/devices/traits/v1/customize_clean_mode.py index e71f76f9b..d4949da8b 100644 --- a/roborock/devices/traits/v1/customize_clean_mode.py +++ b/roborock/devices/traits/v1/customize_clean_mode.py @@ -1,62 +1,27 @@ """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. + +A per-room override set via `set_clean_then_mop` is persisted on the device: +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. """ -from dataclasses import dataclass +import asyncio -from roborock.data import RoborockBase +from roborock.data import CustomizeCleanModes, SegmentCleanMode +from roborock.data.v1.v1_clean_modes import CleanRoutes, VacuumModes, WaterModes +from roborock.data.v1.v1_containers import SEQ_TYPE_CLEAN_THEN_MOP from roborock.devices.traits.v1 import common +from roborock.devices.traits.v1.device_features import DeviceFeaturesTrait +from roborock.exceptions import RoborockParsingException, RoborockUnsupportedFeature 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 []} +_WRITE_KEYS = ("segment", "fan_power", "water_box_mode", "mop_mode", "repeat", "seq_type") class CustomizeCleanModeConverter(common.V1TraitDataConverter): @@ -72,28 +37,80 @@ def convert(self, response: V1ResponseData) -> CustomizeCleanModes: 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. + 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. `set_clean_then_mop` additionally checks + `is_clean_then_mop_mode_supported` itself before writing, since writing + `seq_type` to firmware that doesn't support it is not just a no-op read. """ command = RoborockCommand.GET_CUSTOMIZE_CLEAN_MODE converter = CustomizeCleanModeConverter() requires_feature = "is_customized_clean_supported" + def __init__(self, device_features: DeviceFeaturesTrait) -> None: + super().__init__() + common.V1TraitMixin.__init__(self) + self._device_features = device_features + self._raw_by_segment: dict[int, dict] = {} + self._write_lock = asyncio.Lock() + + async def refresh(self) -> None: + """Refresh the contents of this trait. + + Overrides `V1TraitMixin.refresh()` to additionally retain each + room's raw wire dict, by segment - `set_clean_then_mop` round-trips + untouched rooms from this raw data rather than the typed + `SegmentCleanMode` fields, so per-model keys this library doesn't + model (or a value it failed to parse) are preserved on write instead + of being silently dropped. + """ + response = await self.rpc_channel.send_command(self.command) + try: + new_data = self.converter.convert(response) + except (TypeError, ValueError) as err: + raise RoborockParsingException( + trait_name=type(self).__name__, + command=self.command, + payload=response, + inner_error=err, + ) from err + assert isinstance(response, list) and new_data.modes is not None # converter.convert already validated this + # Key the raw cache off the canonical TYPED segment id (int, already + # decamelized/coerced by SegmentCleanMode.from_dict), not the raw + # dict's own "segment" key - a device that ever sent a differently + # cased key (e.g. "Segment") or a stringified id ("7" instead of 7) + # would otherwise desync the raw cache from `by_segment`, so this + # same room would be silently missing from (or duplicated in) a + # later write. `response` and `new_data.modes` are the same length, + # in the same order, by construction (one SegmentCleanMode per raw + # entry, no filtering) - zip(strict=True) turns any future change to + # that invariant into a loud error instead of silent misalignment. + raw_by_segment: dict[int, dict] = {} + for raw_entry, mode in zip(response, new_data.modes, strict=True): + if mode.segment in raw_by_segment: + raise RoborockParsingException( + trait_name=type(self).__name__, + command=self.command, + payload=response, + inner_error=ValueError(f"Duplicate segment {mode.segment} in response, cannot safely write"), + ) + raw_by_segment[mode.segment] = raw_entry + common.merge_trait_values(self, new_data) + self._raw_by_segment = raw_by_segment + async def set_clean_then_mop( self, segment: int, enabled: bool, *, - fan_power: int, - water_box_mode: int, - mop_mode: int, + fan_power: VacuumModes | int, + water_box_mode: WaterModes | int, + mop_mode: CleanRoutes | int, repeat: int = 1, ) -> None: """Set whether a single room is vacuumed fully before it is mopped. @@ -103,7 +120,9 @@ async def set_clean_then_mop( 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. + settings. `fan_power`, `water_box_mode` and `mop_mode` accept either + the typed mode enum or a raw wire int (for codes this library + doesn't model, e.g. water-slide values). 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 @@ -123,15 +142,61 @@ async def set_clean_then_mop( 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. + + Raises `RoborockUnsupportedFeature` if the device doesn't support + clean-then-mop at all (`is_clean_then_mop_mode_supported`), even if + it does support per-room custom modes in general. + + This whole read-merge-write sequence is serialized per trait + instance (concurrent calls queue rather than racing): the device's + SET_CUSTOMIZE_CLEAN_MODE replaces the whole per-room table rather + than patching one entry (see below), so two overlapping calls each + building their own full table from the same stale read would + otherwise silently revert each other's room. """ - 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() + if not self._device_features.is_clean_then_mop_mode_supported: + raise RoborockUnsupportedFeature("Device does not support clean-then-mop") + + async with self._write_lock: + # Always refresh immediately before building the write, even if + # `self.modes` is already populated: this merges-and-replaces + # the whole table (see below), so writing from a stale cache + # could silently revert another room's override changed since + # the last refresh (e.g. from the vendor app) back to its old + # value. + await self.refresh() + + seq_type = SEQ_TYPE_CLEAN_THEN_MOP if enabled else 0 + new_entry = { + "segment": segment, + "fan_power": fan_power.code if isinstance(fan_power, VacuumModes) else fan_power, + "water_box_mode": water_box_mode.code if isinstance(water_box_mode, WaterModes) else water_box_mode, + "mop_mode": mop_mode.code if isinstance(mop_mode, CleanRoutes) else mop_mode, + "repeat": repeat, + "seq_type": seq_type, + } + # Start from this room's raw wire dict (if it already had an + # override) so any field this library doesn't model - or + # couldn't parse - survives the write, and only overlay the keys + # this call actually changes. A room with no prior override + # gets only the known fields; the device fills in its own + # per-model defaults for the rest, same as the original + # single-entry write did. + raw_entries = dict(self._raw_by_segment) + raw_entries[segment] = {**raw_entries.get(segment, {}), **new_entry} + + # Send the full known table, not just the changed room: on at + # least some V1 firmware (confirmed first-hand for a Saros 20 + # Sonic, see https://github.com/orgs/home-assistant/discussions/3998), + # SET_CUSTOMIZE_CLEAN_MODE replaces the customized segment table + # rather than patching a single entry, so sending only + # `[new_entry]` would silently wipe every other room's + # override. This has not been re-verified against real hardware + # for THIS device since this fix was written (no second + # customized room available to test with); please confirm on a + # device with two or more customized rooms before relying on + # it. + await self.rpc_channel.send_command( + RoborockCommand.SET_CUSTOMIZE_CLEAN_MODE, params=list(raw_entries.values()) + ) + await self.refresh() diff --git a/tests/devices/traits/v1/test_customize_clean_mode.py b/tests/devices/traits/v1/test_customize_clean_mode.py index 447221eaa..90e51aa20 100644 --- a/tests/devices/traits/v1/test_customize_clean_mode.py +++ b/tests/devices/traits/v1/test_customize_clean_mode.py @@ -1,39 +1,54 @@ """Tests for the CustomizeCleanModeTrait class.""" +import asyncio 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.data import HomeDataDevice, SegmentCleanMode +from roborock.data.v1.v1_clean_modes import CleanRoutes, VacuumModes, WaterModes +from roborock.devices.device import RoborockDevice +from roborock.devices.traits.v1.customize_clean_mode import CustomizeCleanModeTrait +from roborock.exceptions import RoborockParsingException, RoborockUnsupportedFeature from roborock.roborock_typing import RoborockCommand +from tests import mock_data 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}, ] +# S7 MaxV / a27 (the suite's default device) supports per-room custom modes +# but not clean-then-mop specifically - real devices, used below for +# test_set_clean_then_mop_unsupported without needing to fake it. +# Saros 10R is the only fixture device with BOTH features True - used for +# every test that actually exercises set_clean_then_mop's happy path. +SAROS_10R = mock_data.DEVICE_PRODUCT_PAIRS["home_data_device_saros_10r.json"][0] +S5E = mock_data.DEVICE_PRODUCT_PAIRS["home_data_device_s5e.json"][0] + @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 +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", [S5E]) +async def test_unsupported_customize_clean_mode( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, + device_info: HomeDataDevice, +) -> 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: +async def test_refresh(customize_clean_mode_trait: CustomizeCleanModeTrait | None, mock_rpc_channel: AsyncMock) -> None: """Rooms with an override are parsed into SegmentCleanMode entries.""" + assert customize_clean_mode_trait is not None mock_rpc_channel.send_command.side_effect = [CUSTOMIZE_CLEAN_MODE_DATA] await customize_clean_mode_trait.refresh() @@ -48,9 +63,10 @@ async def test_refresh(customize_clean_mode_trait: CustomizeCleanModeTrait, mock async def test_refresh_no_overrides( - customize_clean_mode_trait: CustomizeCleanModeTrait, mock_rpc_channel: AsyncMock + customize_clean_mode_trait: CustomizeCleanModeTrait | None, mock_rpc_channel: AsyncMock ) -> None: """An empty response means no room currently has a custom clean mode.""" + assert customize_clean_mode_trait is not None mock_rpc_channel.send_command.side_effect = [[]] await customize_clean_mode_trait.refresh() @@ -60,9 +76,10 @@ async def test_refresh_no_overrides( async def test_refresh_bad_format( - customize_clean_mode_trait: CustomizeCleanModeTrait, mock_rpc_channel: AsyncMock + customize_clean_mode_trait: CustomizeCleanModeTrait | None, mock_rpc_channel: AsyncMock ) -> None: """A non-list response is a parsing error, not a silent empty result.""" + assert customize_clean_mode_trait is not None mock_rpc_channel.send_command.side_effect = [{"unexpected": "shape"}] with pytest.raises(RoborockParsingException): @@ -70,15 +87,39 @@ async def test_refresh_bad_format( async def test_refresh_bad_list_entry( - customize_clean_mode_trait: CustomizeCleanModeTrait, mock_rpc_channel: AsyncMock + customize_clean_mode_trait: CustomizeCleanModeTrait | None, mock_rpc_channel: AsyncMock ) -> None: """A malformed entry inside the list is a parsing error, not silently dropped.""" + assert customize_clean_mode_trait is not None mock_rpc_channel.send_command.side_effect = [["ok"]] with pytest.raises(RoborockParsingException): await customize_clean_mode_trait.refresh() +async def test_refresh_duplicate_segment_raises( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, mock_rpc_channel: AsyncMock +) -> None: + """Two entries resolving to the same segment id is an ambiguous table, not silently collapsed. + + Regression test for a real gap found in review: the raw-preservation + cache used to be built by a plain dict comprehension, which would + silently keep only the LAST entry for a duplicated segment id and lose + the other - an ambiguous table should be treated as unsafe to write + from, not quietly resolved. + """ + assert customize_clean_mode_trait is not None + mock_rpc_channel.send_command.side_effect = [ + [ + {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "repeat": 1, "seq_type": 1}, + {"segment": 7, "fan_power": 102, "water_box_mode": 203, "mop_mode": 300, "repeat": 1, "seq_type": 0}, + ] + ] + + 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.""" @@ -87,14 +128,11 @@ def test_clean_then_mop(seq_type: int | None, expected: bool | None) -> None: assert mode.clean_then_mop is expected +@pytest.mark.parametrize("device_info", [SAROS_10R]) @pytest.mark.parametrize( - ("enabled", "repeat", "expected_params"), + ("enabled", "repeat", "expected_entry"), [ - ( - True, - 2, - {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "repeat": 2, "seq_type": 1}, - ), + (True, 2, {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "repeat": 2, "seq_type": 1}), ( True, None, @@ -108,16 +146,39 @@ def test_clean_then_mop(seq_type: int | None, expected: bool | None) -> None: ], ) async def test_set_clean_then_mop( - customize_clean_mode_trait: CustomizeCleanModeTrait, + customize_clean_mode_trait: CustomizeCleanModeTrait | None, mock_rpc_channel: AsyncMock, + device_info: HomeDataDevice, enabled: bool, repeat: int | None, - expected_params: dict[str, int], + expected_entry: 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] + assert customize_clean_mode_trait is not None + # Refresh is called internally to fetch the existing table before writing, + # then again after the write - two GET responses are consumed. The second + # (post-write) response reflects what was actually written for segment 7, + # not a static fixture - otherwise this test can't catch a setter that + # sends the wrong seq_type (see the enabled=False case, which caught a + # real bug: the original test's post-write mock was the same static + # all-enabled fixture regardless of `enabled`, so clean_then_mop was + # always asserted True even when enabled=False was never actually + # exercised). + refreshed_segment_4 = { + "segment": 4, + "fan_power": 102, + "water_box_mode": 203, + "mop_mode": 300, + "repeat": 1, + "seq_type": 0, + } + mock_rpc_channel.send_command.side_effect = [ + CUSTOMIZE_CLEAN_MODE_DATA, + "ok", + [refreshed_segment_4, expected_entry], + ] kwargs = {"fan_power": 101, "water_box_mode": 203, "mop_mode": 300} if repeat is not None: kwargs["repeat"] = repeat @@ -125,7 +186,289 @@ async def test_set_clean_then_mop( 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), + call( + RoborockCommand.SET_CUSTOMIZE_CLEAN_MODE, + params=[ + {"segment": 4, "fan_power": 102, "water_box_mode": 203, "mop_mode": 300, "repeat": 1, "seq_type": 0}, + expected_entry, + ], + ), call(RoborockCommand.GET_CUSTOMIZE_CLEAN_MODE), ] - assert customize_clean_mode_trait.by_segment[7].clean_then_mop is True + assert customize_clean_mode_trait.by_segment[7].clean_then_mop is enabled + + +@pytest.mark.parametrize("device_info", [SAROS_10R]) +async def test_set_clean_then_mop_accepts_typed_modes( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, + mock_rpc_channel: AsyncMock, + device_info: HomeDataDevice, +) -> None: + """fan_power/water_box_mode/mop_mode accept the typed mode enums, not just raw ints.""" + assert customize_clean_mode_trait is not None + mock_rpc_channel.send_command.side_effect = [CUSTOMIZE_CLEAN_MODE_DATA, "ok", CUSTOMIZE_CLEAN_MODE_DATA] + + await customize_clean_mode_trait.set_clean_then_mop( + 7, + True, + fan_power=VacuumModes.BALANCED, + water_box_mode=WaterModes.HIGH, + mop_mode=CleanRoutes.DEEP, + ) + + sent_params = mock_rpc_channel.send_command.mock_calls[1].kwargs["params"] + segment_7 = next(entry for entry in sent_params if entry["segment"] == 7) + assert segment_7 == { + "segment": 7, + "fan_power": VacuumModes.BALANCED.code, + "water_box_mode": WaterModes.HIGH.code, + "mop_mode": CleanRoutes.DEEP.code, + "repeat": 1, + "seq_type": 1, + } + + +@pytest.mark.parametrize("device_info", [SAROS_10R]) +async def test_set_clean_then_mop_preserves_other_rooms( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, + mock_rpc_channel: AsyncMock, + device_info: HomeDataDevice, +) -> None: + """Updating one room's override must not drop another room's existing override. + + Some V1 firmware replaces the whole customized-segment table rather than + patching a single entry, so sending only the changed room would silently + wipe other rooms. The full known table (segment 4 untouched, segment 7 + updated) must be sent on every write. + """ + assert customize_clean_mode_trait is not None + mock_rpc_channel.send_command.side_effect = [CUSTOMIZE_CLEAN_MODE_DATA, "ok", CUSTOMIZE_CLEAN_MODE_DATA] + + await customize_clean_mode_trait.set_clean_then_mop(7, False, fan_power=101, water_box_mode=203, mop_mode=300) + + sent_params = mock_rpc_channel.send_command.mock_calls[1].kwargs["params"] + assert {entry["segment"] for entry in sent_params} == {4, 7} + segment_4 = next(entry for entry in sent_params if entry["segment"] == 4) + assert segment_4 == { + "segment": 4, + "fan_power": 102, + "water_box_mode": 203, + "mop_mode": 300, + "repeat": 1, + "seq_type": 0, + } + + +@pytest.mark.parametrize("device_info", [SAROS_10R]) +async def test_set_clean_then_mop_preserves_model_specific_fields( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, + mock_rpc_channel: AsyncMock, + device_info: HomeDataDevice, +) -> None: + """Model-specific fields (e.g. mop_power) on the room being updated must not be stripped.""" + assert customize_clean_mode_trait is not None + data_with_mop_power = [ + { + "segment": 7, + "fan_power": 101, + "water_box_mode": 203, + "mop_mode": 300, + "repeat": 1, + "seq_type": 0, + "mop_power": 50, + }, + ] + mock_rpc_channel.send_command.side_effect = [data_with_mop_power, "ok", data_with_mop_power] + + await customize_clean_mode_trait.set_clean_then_mop(7, True, fan_power=101, water_box_mode=203, mop_mode=300) + + sent_params = mock_rpc_channel.send_command.mock_calls[1].kwargs["params"] + assert sent_params == [ + { + "segment": 7, + "fan_power": 101, + "water_box_mode": 203, + "mop_mode": 300, + "repeat": 1, + "seq_type": 1, + "mop_power": 50, + } + ] + + +@pytest.mark.parametrize("device_info", [SAROS_10R]) +async def test_set_clean_then_mop_preserves_unknown_field_on_untouched_room( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, + mock_rpc_channel: AsyncMock, + device_info: HomeDataDevice, +) -> None: + """A wire field this library doesn't model at all must still survive on an untouched room. + + Regression test for a real gap found in review: adding named fields + (mop_power, custom_water_box_distance) to SegmentCleanMode only fixes + those two keys specifically, not the general problem of truly unknown + per-model keys. The setter must round-trip untouched rooms from their + raw wire dict, not reconstruct them from typed fields. + """ + assert customize_clean_mode_trait is not None + data_with_unknown_field = [ + { + "segment": 4, + "fan_power": 102, + "water_box_mode": 203, + "mop_mode": 300, + "repeat": 1, + "seq_type": 0, + "some_future_field_this_library_does_not_model": 999, + }, + {"segment": 7, "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "repeat": 1, "seq_type": 1}, + ] + mock_rpc_channel.send_command.side_effect = [data_with_unknown_field, "ok", data_with_unknown_field] + + await customize_clean_mode_trait.set_clean_then_mop(7, False, fan_power=101, water_box_mode=203, mop_mode=300) + + sent_params = mock_rpc_channel.send_command.mock_calls[1].kwargs["params"] + segment_4 = next(entry for entry in sent_params if entry["segment"] == 4) + assert segment_4["some_future_field_this_library_does_not_model"] == 999 + + +@pytest.mark.parametrize("device_info", [SAROS_10R]) +async def test_set_clean_then_mop_untouched_room_survives_mismatched_key_casing( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, + mock_rpc_channel: AsyncMock, + device_info: HomeDataDevice, +) -> None: + """An untouched room must not vanish from the write just because its raw dict spells + "segment" differently (e.g. "Segment"). + + Regression test for a real gap found in review: the raw-preservation + cache used to be keyed by the literal `entry["segment"]` lookup, which + silently excluded this room entirely (case-sensitive dict access) even + though typed parsing (which decamelizes keys) correctly recognized it as + segment 4 - so updating a DIFFERENT room would drop room 4 from the + write without any error. The cache must be keyed by the canonical typed + segment id instead. + """ + assert customize_clean_mode_trait is not None + data_with_capitalized_key = [ + {"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}, + ] + mock_rpc_channel.send_command.side_effect = [data_with_capitalized_key, "ok", data_with_capitalized_key] + + await customize_clean_mode_trait.set_clean_then_mop(7, False, fan_power=101, water_box_mode=203, mop_mode=300) + + sent_params = mock_rpc_channel.send_command.mock_calls[1].kwargs["params"] + assert {entry.get("segment", entry.get("Segment")) for entry in sent_params} == {4, 7} + + +@pytest.mark.parametrize("device_info", [SAROS_10R]) +async def test_set_clean_then_mop_segment_id_as_string_does_not_duplicate( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, + mock_rpc_channel: AsyncMock, + device_info: HomeDataDevice, +) -> None: + """A room reported with a stringified segment id ("7" instead of 7) must not end up sent + twice - once under the original string key, once under the new integer key. + + Regression test for a real gap found in review: the raw cache used to be + keyed by the raw, unconverted value, while the write path looks the room + up by the caller-supplied int `segment`. A string/int mismatch meant + `raw_entries.get(segment)` missed the existing raw entry (so model- + specific fields on THIS room would also have been silently dropped) and + the old string-keyed entry was never removed, so both ended up in the + final SET payload for what the device would see as the same room. + """ + assert customize_clean_mode_trait is not None + data_with_string_segment = [ + {"segment": "7", "fan_power": 101, "water_box_mode": 203, "mop_mode": 300, "repeat": 1, "seq_type": 0}, + ] + mock_rpc_channel.send_command.side_effect = [data_with_string_segment, "ok", data_with_string_segment] + + await customize_clean_mode_trait.set_clean_then_mop(7, True, fan_power=101, water_box_mode=203, mop_mode=300) + + sent_params = mock_rpc_channel.send_command.mock_calls[1].kwargs["params"] + assert len(sent_params) == 1 + assert sent_params[0]["segment"] == 7 + assert sent_params[0]["seq_type"] == 1 + + +async def test_set_clean_then_mop_unsupported( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, + mock_rpc_channel: AsyncMock, +) -> None: + """Writing clean-then-mop on a device that doesn't support it raises, not silently sends seq_type. + + Uses the default device fixture (S7 MaxV / a27): it genuinely supports + per-room custom modes but not clean-then-mop specifically, matching real + hardware, rather than faking unsupport on a device that does support it. + """ + assert customize_clean_mode_trait is not None + + with pytest.raises(RoborockUnsupportedFeature): + await customize_clean_mode_trait.set_clean_then_mop(7, True, fan_power=101, water_box_mode=203, mop_mode=300) + + mock_rpc_channel.send_command.assert_not_called() + + +@pytest.mark.parametrize("device_info", [SAROS_10R]) +async def test_set_clean_then_mop_concurrent_calls_do_not_clobber( + customize_clean_mode_trait: CustomizeCleanModeTrait | None, + mock_rpc_channel: AsyncMock, + device_info: HomeDataDevice, +) -> None: + """Two concurrent set_clean_then_mop calls for different rooms must not revert each other. + + Regression test for a real race found in review: without serializing the + read-merge-write sequence, two concurrent calls (room 4, room 7) can each + read the same starting table, then the second call's full-table write + overwrites the first call's change because it was built from a stale + snapshot that didn't see the first write yet. + """ + assert customize_clean_mode_trait is not None + + # A minimal stateful fake device: GET returns the current table, SET + # replaces it - same contract the real device has. This is what makes + # the test meaningful: a non-stateful mock can't distinguish "read a + # stale snapshot" from "read the latest state", which is exactly the + # bug being guarded against. + device_table = {entry["segment"]: dict(entry) for entry in CUSTOMIZE_CLEAN_MODE_DATA} + + # Without forcing it, asyncio won't actually interleave these two calls + # at all: a plain async mock with no real suspension point lets the + # first task run to completion before the second one gets scheduled, so + # the race never has a chance to manifest either way - the lock's effect + # couldn't be observed one way or the other. Force a genuine suspension + # point on every call instead so the two tasks CAN interleave at the + # event loop's discretion; whether they actually end up clobbering each + # other then depends entirely on whether the lock serializes the whole + # read-merge-write transaction (it must: the lock makes `set_clean_then_mop` + # block here, not just at this one call, until the other task's whole + # transaction - all 3 round trips - has finished). + async def _send_command(command: RoborockCommand, params: list[dict] | None = None) -> object: + await asyncio.sleep(0) + if command is RoborockCommand.GET_CUSTOMIZE_CLEAN_MODE: + return list(device_table.values()) + assert command is RoborockCommand.SET_CUSTOMIZE_CLEAN_MODE + assert params is not None + device_table.clear() + device_table.update({entry["segment"]: entry for entry in params}) + return "ok" + + mock_rpc_channel.send_command.side_effect = _send_command + + await asyncio.gather( + customize_clean_mode_trait.set_clean_then_mop(4, True, fan_power=102, water_box_mode=203, mop_mode=300), + customize_clean_mode_trait.set_clean_then_mop(7, False, fan_power=101, water_box_mode=203, mop_mode=300), + ) + + set_calls = [ + c for c in mock_rpc_channel.send_command.mock_calls if c.args[:1] == (RoborockCommand.SET_CUSTOMIZE_CLEAN_MODE,) + ] + assert len(set_calls) == 2 + # Final device state must have BOTH intended changes - room 4 enabled, + # room 7 disabled. Without the lock, the second call's write (built from + # a stale pre-first-call read) would revert whichever room the first + # call changed. + assert {segment: entry["seq_type"] for segment, entry in device_table.items()} == {4: 1, 7: 0}