Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions roborock/devices/traits/v1/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
command,
common,
consumeable,
customize_clean_mode,
device_features,
do_not_disturb,
dust_collection_mode,
Expand All @@ -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
Expand Down Expand Up @@ -126,6 +128,7 @@
"command",
"common",
"consumeable",
"customize_clean_mode",
"device_features",
"do_not_disturb",
"dust_collection_mode",
Expand Down Expand Up @@ -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,
Expand Down
137 changes: 137 additions & 0 deletions roborock/devices/traits/v1/customize_clean_mode.py
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.
Comment on lines +1 to +9

Copy link
Copy Markdown
Contributor

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.md Priority 1):

  1. feat(v1): expose clean-then-mop status from the device #959 is still in Draft with open design discussion around modeling "Vacuum then Mop" as a CleaningMode option (matching the vendor app's 4th mode tab, which also clears seq_type to 0 when switching to Vacuum-only or Mop-only) vs. a separate boolean property. Consequently, StatusTrait.clean_then_mop and StatusTrait.set_clean_then_mop (referenced here and on lines 75–76) do not exist on main. Please decouple the docstrings (and consider whether per-room custom mode selection should align with CleaningMode once feat(v1): expose clean-then-mop status from the device #959 settles).
  2. Please move SegmentCleanMode and CustomizeCleanModes to roborock/data/v1/v1_containers.py (re-exported via roborock.data), matching MopDryerSetting, WashTowelMode, etc., so consumers can import domain models from roborock.data and tests/conformance/test_model_conformance.py automatically covers them. (Keeping raw int | None wire fields on SegmentCleanMode like StatusV2 is great so water-slide values like 222 round-trip losslessly.)


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature gating on clean_then_mop & dataclass MRO (AGENTS.md §1 & §3):

  1. The docstring notes that on devices with is_customized_clean_supported=True and is_clean_then_mop_mode_supported=False, .clean_then_mop is None—however, nothing in the code checks is_clean_then_mop_mode_supported. If firmware without bit 93 (CLEAN_THEN_MOP_MODE) still reports "seq_type": 0 in get_customize_clean_mode, SegmentCleanMode.clean_then_mop will return False instead of None, and calling set_clean_then_mop will send "seq_type": 1 to unsupported firmware instead of raising RoborockUnsupportedFeature.
  2. Because CustomizeCleanModes is a @dataclass, its generated __init__ does not call super().__init__(), so common.V1TraitMixin.__init__ (self._rpc_channel = None) never runs (AttributeError instead of ValueError if .rpc_channel is accessed before attachment).

Suggested fix: Inject DeviceFeaturesTrait into CustomizeCleanModeTrait.__init__ in PropertiesApi.discover_features() (matching WashTowelModeTrait), call both super().__init__() and common.V1TraitMixin.__init__(self), and gate clean-then-mop read/write behavior on self._device_features_trait.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,
repeat: int = 1,
) -> None:
Comment on lines +89 to +98

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Public trait API contract & raw wire integers (AGENTS.md Priority 1):

  • Now that CustomizeCleanModeTrait is gated on is_customized_clean_supported, most mopping V1 vacuums (S7, S7 MaxV, S8, Qrevo—many without is_clean_then_mop_mode_supported) expose v1_properties.customize_clean_mode, yet its only write method is set_clean_then_mop(...).
  • Requiring callers to pass raw wire codes (fan_power: int, water_box_mode: int, mop_mode: int) exposes protocol integers on the public trait boundary rather than domain models/enums (VacuumModes, WaterModes, CleanRoutes, CleaningMode, or SegmentCleanMode). Worse, because get_customize_clean_mode starts empty ([]) and callers cannot fall back to StatusTrait while CleaningMode.CUSTOM (106/204/302) is active, a consumer configuring an uncustomized room for the first time has no valid source of wire codes without re-implementing v1_clean_modes.py (e.g. 235 on is_water_slide_mode_supported devices vs. 202 on standard devices).

Suggested fix: Expose a general per-room mode setter on CustomizeCleanModeTrait accepting SegmentCleanMode or typed mode enums (VacuumModes, WaterModes, CleanRoutes / CleaningMode), using DeviceFeaturesTrait and v1_clean_modes helpers to resolve valid device-specific defaults when a room has no prior override.

"""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,

Copy link
Copy Markdown
Contributor

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 Rooms trait? 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 from Rooms to get the needed segment information for display, so i think there could be benefit from trying to hide those details, but its not trivial.

@jevgenijostin jevgenijostin Oct 17, 2026 •

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-room override data loss & unmodeled wire keys on write (AGENTS.md Priority 2/3):

  1. Single-entry list overwrite (params=[entry]): The hardware verification in the PR description tested round-tripping a single room (segment: 7) starting from []. On V1 firmware (e.g. Qrevo / Saros models), set_customize_clean_mode replaces the customized segment table (or rejects partial lists with -10005 on models like Saros 20 Sonic), meaning sending params=[entry] for segment 7 wipes segment 4's override! Could you verify on the QV 35S with two rooms customized simultaneously whether set_customize_clean_mode([room_a]) preserves or wipes room_b? Merging the updated entry against self.modes (refreshing first if self.modes is None) and sending the full list preserves all other rooms' overrides. (Note: avoid RoborockBase.as_dict() directly for RPC params since it camelizes keys like fanPower instead of fan_power.)
  2. Dropping model-specific per-segment keys: RoborockBase.from_dict() drops unknown keys and set_clean_then_mop builds a fresh 6-key dict. On sonic/roller mop models (mop_power) or custom water flow models (custom_water_box_distance), get_customize_clean_mode includes extra fields and set_customize_clean_mode rejects payloads missing required model-specific keys (-10005), just as QV 35S rejected missing mop_mode. Modeling/preserving those fields prevents stripping required keys when updating an existing segment.

131 changes: 131 additions & 0 deletions tests/devices/traits/v1/test_customize_clean_mode.py
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use device.v1_properties.customize_clean_mode & existing testdata fixtures for discovery (AGENTS.md §5):

Two things in tests/devices/traits/v1/fixtures.py (loaded automatically via tests/devices/traits/v1/__init__.py) make full discovery testing work out of the box without reverse-engineering product_nickname or mutating private trait._rpc_channel:

  1. device and discover_features_fixture are autouse=True, so device.connect() already runs before every test here, and the default fixture (S7 MaxV / a27) already has is_customized_clean_supported == True—meaning device.v1_properties.customize_clean_mode is already discovered and wired to mock_rpc_channel!
  2. products_fixture already loads all tests/testdata/home_data_product_*.json files, and mock_data.DEVICE_PRODUCT_PAIRS already has V1 devices with is_customized_clean_supported == False (such as home_data_device_s5e.json and home_data_device_q5_max.json, verified in tests/__snapshots__/test_supported_features.ambr).
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertion mismatch in enabled=False test case:

enabled is parametrized over True and False, but line 120 always mocks the follow-up GET_CUSTOMIZE_CLEAN_MODE refresh with the static CUSTOMIZE_CLEAN_MODE_DATA (where segment 7 has seq_type: 1) and line 131 unconditionally asserts clean_then_mop is True—even when enabled=False.

Mock the refreshed payload using expected_params for segment 7 (alongside segment 4 to verify multi-room preservation) and assert:

assert customize_clean_mode_trait.by_segment[7].clean_then_mop is enabled

Loading