Skip to content

Refactor WaterModes to use display_name instead of value aliases #990

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.

Sign up for GitHub

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

Draft
swastiktawate wants to merge 1 commit into
base: main
Choose a base branch
from
+55 −27
Draft

Refactor WaterModes to use display_name instead of value aliases #990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 19 additions & 25 deletions roborock/data/v1/v1_clean_modes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@ class WaterModes(RoborockModeEnum):
SMART_MODE = ("smart_mode", 209)
SMART_WATER = ("smart_water", 210)
PURE_WATER_FLOW_START = ("slight", 221)
PURE_WATER_FLOW_SMALL = ("low", 225)
PURE_WATER_FLOW_SMALL = ("pure_water_flow_small", 225, "low")
PURE_WATER_FLOW_GENTLE = ("gentle", 230)
PURE_WATER_FLOW_MIDDLE = ("medium", 235)
PURE_WATER_FLOW_MIDDLE = ("pure_water_flow_middle", 235, "medium")
PURE_WATER_FLOW_LARGE = ("moderate", 240)
PURE_WATER_SUPER_BEGIN = ("high", 245)
PURE_WATER_FLOW_END = ("extreme", 250)
PURE_WATER_SUPER_BEGIN = ("pure_water_super_begin", 245, "high")
PURE_WATER_FLOW_END = ("pure_water_flow_end", 250, "extreme")


class WashTowelModes(RoborockModeEnum):
Expand Down Expand Up @@ -201,7 +201,7 @@ def get_water_modes(features: DeviceFeatures) -> list[WaterModes]:
def get_water_slide_mode_name(code: int) -> str | None:
"""Get the app's range name for a water slide code, or None if out of range."""
mode = _resolve_water_slide_mode(code)
return mode.value if mode is not None else None
return mode.display_name if mode is not None else None


def _resolve_water_slide_mode(code: int) -> WaterModes | None:
Expand All @@ -220,15 +220,8 @@ def _resolve_water_slide_mode(code: int) -> WaterModes | None:


def get_water_mode_mapping(features: DeviceFeatures) -> dict[int, str]:
"""Get water mode mapping by supported feature set.

WaterModes contains aliases for multiple codes that share the same value
string (e.g. low can be 201 or 225). For water slide mode devices we need
explicit code mapping to preserve those slide-specific codes.
"""
if features.is_water_slide_mode_supported:
return {code: mode.value for code, mode in WATER_SLIDE_MODE_MAPPING.items()}
return {mode.code: mode.value for mode in get_water_modes(features)}
"""Get water mode mapping by supported feature set."""
return {mode.code: mode.display_name for mode in get_water_modes(features)}


def get_cleaning_mode_options(features: DeviceFeatures) -> list[CleaningMode]:
Expand Down Expand Up @@ -309,14 +302,7 @@ def get_cleaning_mode_parameters(cleaning_mode: CleaningMode, features: DeviceFe
raise RoborockUnsupportedFeature(f"Cleaning mode {cleaning_mode.value!r} is not supported")

fan_power, water_box_mode, mop_mode = _get_clean_motor_mode_params(cleaning_mode, features)
water_code = water_box_mode.code
if features.is_water_slide_mode_supported:
# Aliased water enums retain legacy codes; the device mapping preserves
# slide-specific wire values without changing the user-facing names.
water_code = next(
code for code, name in get_water_mode_mapping(features).items() if name == water_box_mode.value
)
params: dict[str, int] = {"fan_power": fan_power.code, "water_box_mode": water_code}
params: dict[str, int] = {"fan_power": fan_power.code, "water_box_mode": water_box_mode.code}
if features.is_clean_route_setting_supported:
params["mop_mode"] = mop_mode.code
return [params]
Expand All @@ -342,6 +328,16 @@ def _resolve_clean_mode(value: int | VacuumModes | None, features: DeviceFeature
return VacuumModes.from_code_optional(value)


def _resolve_water_mode(value: int | WaterModes | None, features: DeviceFeatures) -> WaterModes | None:
"""Resolve a water mode code, accounting for water-slide range codes."""
if value is None or isinstance(value, WaterModes):
return value
mode = WaterModes.from_code_optional(value)
if features.is_water_slide_mode_supported:
mode = _resolve_water_slide_mode(value) or mode
return mode
Comment on lines +331 to +338

@Lash-L Lash-L Oct 31, 2026

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.

Now that 225, 235, 245, and 250 are registered in WaterModes._code_map_, WaterModes.from_code_optional(value) resolves the 7 representative water-slide codes (221, 225, 230, 235, 240, 245, 250) even when features.is_water_slide_mode_supported is False, while the other 23 intermediate slider codes (222, 223, 248, etc.) return None.

This should guard the slider range (221 <= value <= 250) behind features.is_water_slide_mode_supported so all 30 slider codes behave consistently on devices without water-slide support:

Suggested change
def _resolve_water_mode(value: int | WaterModes | None, features: DeviceFeatures) -> WaterModes | None:
"""Resolve a water mode code, accounting for water-slide range codes."""
if value is None or isinstance(value, WaterModes):
return value
mode = WaterModes.from_code_optional(value)
if features.is_water_slide_mode_supported:
mode = _resolve_water_slide_mode(value) or mode
return mode
def _resolve_water_mode(value: int | WaterModes | None, features: DeviceFeatures) -> WaterModes | None:
"""Resolve a water mode code, accounting for water-slide range codes."""
if value is None or isinstance(value, WaterModes):
return value
if 221 <= value <= 250:
return _resolve_water_slide_mode(value) if features.is_water_slide_mode_supported else None
mode = WaterModes.from_code_optional(value)
if features.is_water_slide_mode_supported:
mode = _resolve_water_slide_mode(value) or mode
return mode



def get_current_cleaning_mode(
clean_mode: int | VacuumModes | None,
water_mode: int | WaterModes | None,
Expand All @@ -352,9 +348,7 @@ def get_current_cleaning_mode(
if not features.is_support_water_mode:
return None
clean_mode_enum = _resolve_clean_mode(clean_mode, features)
water_mode_enum = _resolve_mode_code(water_mode, WaterModes)
if features.is_water_slide_mode_supported and isinstance(water_mode, int):
water_mode_enum = _resolve_water_slide_mode(water_mode) or water_mode_enum
water_mode_enum = _resolve_water_mode(water_mode, features)
mop_mode_enum = _resolve_mode_code(mop_mode, CleanRoutes)
if clean_mode_enum is None or water_mode_enum is None:
return None
Expand Down
34 changes: 34 additions & 0 deletions tests/conformance/test_enum_conformance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,37 @@ def test_known_missing_unknown_baseline_inventory() -> None:
f"Enums in KNOWN_ENUMS_MISSING_UNKNOWN are no longer missing 'unknown': {stale_in_baseline}. "
"Please remove them from KNOWN_ENUMS_MISSING_UNKNOWN."
)


# Baseline inventory of RoborockModeEnum subclasses that intentionally define
# multiple names sharing one value string (e.g. fault descriptions shared
# across several firmware codes). These enums accept the silent aliasing
# behavior; every other mode enum must give each member a distinct value
# (using display_name for the shared user-facing name) so no code is lost.
KNOWN_MODE_ENUMS_WITH_VALUE_ALIASES = {
"roborock.data.b01_q7.b01_q7_code_mappings.B01Fault",
}


@pytest.mark.parametrize("mode_enum_cls", to_pytest_params(_ALL_MODE_ENUMS))
def test_roborock_mode_enum_no_silent_value_aliases(mode_enum_cls: type[RoborockModeEnum]) -> None:
"""RoborockModeEnum subclasses must not define silent value aliases.

StrEnum collapses members sharing a value string into one, silently
discarding the other member's code. Give each member a distinct value and
use display_name for the shared user-facing name instead.
"""
fqn = f"{mode_enum_cls.__module__}.{mode_enum_cls.__name__}"
if fqn in KNOWN_MODE_ENUMS_WITH_VALUE_ALIASES:
return
Comment on lines +171 to +186

@Lash-L Lash-L Oct 31, 2026

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.

B01Fault has the same underlying issue where F_512 (512), F_514 (514), and F_518 (518) are silently dropped from B01Fault.from_code_optional() due to duplicate StrEnum values. Instead of an early return with no staleness check, this should use marks_by_fqn with pytest.mark.xfail(..., strict=True) on to_pytest_params (matching KNOWN_ENUMS_MISSING_UNKNOWN above) so the baseline automatically fails when B01Fault is fixed.

seen: set[int] = set()
aliases = []
for name, member in mode_enum_cls.__members__.items():
if id(member) in seen:
aliases.append(name)
seen.add(id(member))
assert not aliases, (
f"{fqn} defines silent value aliases: {aliases}. "
"Members sharing a value string collapse into one and lose their codes; "
"use display_name for the shared user-facing name instead."
)
4 changes: 2 additions & 2 deletions tests/devices/traits/v1/test_status.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ def test_current_cleaning_mode_water_code_fallback(

@pytest.mark.parametrize("water_mode", [WaterModes.MEDIUM, WaterModes.PURE_WATER_FLOW_MIDDLE])
def test_current_cleaning_mode_water_slide_accepts_enums(water_mode: WaterModes) -> None:
"""Enum inputs remain valid even when they alias a legacy water mode."""
"""Enum inputs resolve to distinct members, each keeping its own wire code."""
status_trait = _create_cleaning_mode_status_trait(is_water_slide_mode_supported=True)
assert (
get_current_cleaning_mode(102, water_mode, 303, status_trait._device_features_trait) == CleaningMode.VAC_AND_MOP
Expand Down Expand Up @@ -778,7 +778,7 @@ def test_water_slide_mode_mapping() -> None:
245: "high",
250: "extreme",
}
assert [mode.value for mode in status_trait.water_mode_options] == [
assert [mode.display_name for mode in status_trait.water_mode_options] == [
"off",
"slight",
"low",
Expand Down
Loading