From cc3ce12aa453ca773ecc2fcb1f440b34247a142b Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Tue, 28 Jul 2026 02:10:01 +0000 Subject: [PATCH] Fix range hood fan power targeting and kimchi mode write validation - _speed_zero_is_off now keys off the hood resource's own settableMinFanSpeed/supportedFanSpeed fields instead of asking whether the device has any power resource at all, so a combi appliance's cavity /power/0 can no longer be toggled off by turning off just the vent fan. - async_turn_on() no longer resets an already-running fan to its lowest speed when called without a percentage. - _has_separate_power() reads through the O(1) resource cache instead of copying the full resource snapshot on every property access. - _kimchi_mode_write rejects values the compartment didn't advertise in supportMode instead of writing them blind. - kimchi_ripening_status no longer lowercases its value, since it has no enum catalog entry to translate the lowercased token back through. - Documented why KIMCHI_DOOR_GENERIC isn't deduped against the /doors/vs/0 aggregate fallback on the one fixture that reports both. Adds regression tests for the combi-appliance power targeting, the already-on turn_on no-op, kimchi mode write validation, and a kimchi select display/write casing round-trip; a translation-coverage guard for kimchi_zone_mode codes mirroring the existing AC preset one. --- custom_components/localthings/fan.py | 69 ++++++++++++------ .../localthings/registry/by_type/microwave.py | 2 +- .../registry/capabilities/fridge.py | 12 +++- tests/test_fridge_capabilities.py | 70 +++++++++++++++++-- tests/test_golden_regression.py | 2 +- tests/test_range_hood_fan.py | 65 +++++++++++++++++ tests/test_translations.py | 24 +++++++ 7 files changed, 212 insertions(+), 32 deletions(-) diff --git a/custom_components/localthings/fan.py b/custom_components/localthings/fan.py index dbc73c1..9c9d623 100644 --- a/custom_components/localthings/fan.py +++ b/custom_components/localthings/fan.py @@ -37,6 +37,8 @@ POWER_HREF = '/power/0' POWER_VS_HREF = '/power/vs/0' _FAN_SPEED_FIELD = 'x.com.samsung.da.hood.fanSpeed' _SUPPORTED_FAN_SPEED_FIELD = 'x.com.samsung.da.hood.supportedFanSpeed' +_MIN_FAN_SPEED_FIELD = 'x.com.samsung.da.hood.settableMinFanSpeed' +_OFF_SPEED_CODE = '0' _MODES_FIELD = 'x.com.samsung.da.modes' _SUPPORTED_MODES_FIELD = 'x.com.samsung.da.supportedModes' @@ -65,12 +67,20 @@ class LocalThingsRangeHoodFan(LocalThingsEntity, FanEntity): """A hood fan combining sibling power and fan-speed resources. Some boards that reuse this capability (built-in microwave vent fans, - issues #137/#142) report no sibling `/power/0` or `/power/vs/0` resource - at all -- fan speed 0 is itself the off state there, with no separate - power toggle to write. `_has_separate_power` detects that shape and - switches every method below to drive off/on purely through the - fanSpeed field, including '0' in the ordered speed codes as the off - step instead of assuming every advertised code is an active speed. + issues #137/#142) report no sibling `/power/0` or `/power/vs/0` + resource at all -- fan speed 0 is itself the off state there, with no + separate power toggle to write. `_speed_zero_is_off` detects that + shape from the hood resource's own settableMinFanSpeed/ + supportedFanSpeed fields and switches every method below to drive + off/on purely through the fanSpeed field, including '0' in the + ordered speed codes as the off step instead of assuming every + advertised code is an active speed. + + This is deliberately not the same question as `_has_separate_power`, + which only proves *some* power resource exists on the device -- on a + combi appliance (e.g. an over-the-range microwave) that resource can + belong to the cavity, not the vent fan, and toggling it from here + would turn off the whole appliance instead of just the fan. """ _enable_turn_on_off_backwards_compatibility = False @@ -88,8 +98,19 @@ class LocalThingsRangeHoodFan(LocalThingsEntity, FanEntity): return self.coordinator.resource(href) or {} def _has_separate_power(self) -> bool: - resources = self.coordinator.last_resources - return POWER_HREF in resources or POWER_VS_HREF in resources + return bool(self._rep(POWER_HREF)) or bool(self._rep(POWER_VS_HREF)) + + def _speed_zero_is_off(self) -> bool: + """Whether fan speed '0' is itself this hood's off step, with no + separate power resource to toggle. The board says so directly: + settableMinFanSpeed '0', or '0' inside supportedFanSpeed. The + standalone hood's codes start at 14 and it carries a real /power + resource instead, so this is False there.""" + rep = self._rep(self._bound.href) + return ( + str(rep.get(_MIN_FAN_SPEED_FIELD, '')) == _OFF_SPEED_CODE + or _OFF_SPEED_CODE in self._all_speed_codes() + ) def _all_speed_codes(self) -> list[str]: rep = self._rep(self._bound.href) @@ -97,14 +118,14 @@ class LocalThingsRangeHoodFan(LocalThingsEntity, FanEntity): def _active_speed_codes(self) -> list[str]: codes = self._all_speed_codes() - if self._has_separate_power(): - # Power is carried by the separate /power resource. fanSpeed - # retains the selected setting while power is off (as the - # lamp's `current` field does), so every advertised code is an - # active ordered speed. - return codes - # No separate power resource: '0' is the off step, not a speed. - return [code for code in codes if code != '0'] + if self._speed_zero_is_off(): + # No separate power resource: '0' is the off step, not a speed. + return [code for code in codes if code != _OFF_SPEED_CODE] + # Power is carried by the separate /power resource. fanSpeed + # retains the selected setting while power is off (as the + # lamp's `current` field does), so every advertised code is an + # active ordered speed. + return codes def _power_payload(self, enabled: bool) -> tuple[str, bool, str]: """Target whichever power resource this hood actually exposes.""" @@ -114,9 +135,9 @@ class LocalThingsRangeHoodFan(LocalThingsEntity, FanEntity): @property def is_on(self) -> bool: - if not self._has_separate_power(): + if self._speed_zero_is_off(): current = str(self._rep(self._bound.href).get(_FAN_SPEED_FIELD, '0')) - return current not in ('', '0') + return current not in ('', _OFF_SPEED_CODE) rep = self._rep(POWER_HREF) if 'value' in rep: return bool(rep.get('value')) @@ -142,10 +163,14 @@ class LocalThingsRangeHoodFan(LocalThingsEntity, FanEntity): self, percentage: int | None = None, preset_mode: str | None = None, **kwargs, ) -> None: - if not self._has_separate_power(): + if self._speed_zero_is_off(): if percentage is not None: await self.async_set_percentage(percentage) return + if self.is_on: + # Already running: no percentage given means "just turn on", + # not "reset to the lowest speed". + return codes = self._active_speed_codes() if codes: await self.coordinator.async_send_command(self._bound, ('speed', codes[0])) @@ -157,8 +182,8 @@ class LocalThingsRangeHoodFan(LocalThingsEntity, FanEntity): await self.async_set_percentage(percentage) async def async_turn_off(self, **kwargs) -> None: - if not self._has_separate_power(): - await self.coordinator.async_send_command(self._bound, ('speed', '0')) + if self._speed_zero_is_off(): + await self.coordinator.async_send_command(self._bound, ('speed', _OFF_SPEED_CODE)) return await self.coordinator.async_send_command( self._bound, self._power_payload(False), @@ -171,7 +196,7 @@ class LocalThingsRangeHoodFan(LocalThingsEntity, FanEntity): codes = self._active_speed_codes() if not codes: return - if self._has_separate_power() and not self.is_on: + if not self._speed_zero_is_off() and not self.is_on: await self.coordinator.async_send_command( self._bound, self._power_payload(True), ) diff --git a/custom_components/localthings/registry/by_type/microwave.py b/custom_components/localthings/registry/by_type/microwave.py index cbe85fa..7e76a9a 100644 --- a/custom_components/localthings/registry/by_type/microwave.py +++ b/custom_components/localthings/registry/by_type/microwave.py @@ -14,7 +14,7 @@ shape a standalone range hood reports it in -- reused directly from range_hood.py rather than duplicated. Unlike a standalone hood, this dump has no sibling `/power/0` or `/power/vs/0` resource; fan.py's LocalThingsRangeHoodFan falls back to treating fan speed 0 as off in that -case (see its `_has_separate_power` check). +case (see its `_speed_zero_is_off` check). """ from ..capabilities import common, ignored, microwave, oven, range_hood from ._base import DeviceRegistry, _build diff --git a/custom_components/localthings/registry/capabilities/fridge.py b/custom_components/localthings/registry/capabilities/fridge.py index e90666b..14e7436 100644 --- a/custom_components/localthings/registry/capabilities/fridge.py +++ b/custom_components/localthings/registry/capabilities/fridge.py @@ -675,7 +675,7 @@ DOOR_GENERIC = Capability( # --------------------------------------------------------------------------- def _kimchi_mode_write(p, rep, href=None): - if not href: + if not href or p not in (rep.get('x.com.samsung.da.supportMode') or ()): return None return [s for s in href.strip('/').split('/') if s], { 'x.com.samsung.da.currentMode': p, @@ -697,8 +697,7 @@ KIMCHI_ZONE = Capability( SensorDesc(key='ripening_status', field='x.com.samsung.da.ripeStatus', use_instance_name=True, icon='mdi:progress-clock', translation_key='kimchi_ripening_status', - entity_category='diagnostic', - value_fn=lambda v: v.lower() if isinstance(v, str) else v), + entity_category='diagnostic'), SensorDesc(key='ripening_remaining', field='x.com.samsung.da.ripeRemaintime', use_instance_name=True, icon='mdi:timer-sand', translation_key='kimchi_ripening_remaining', @@ -721,6 +720,13 @@ KIMCHI_DOOR_GENERIC = Capability( strip_prefix_in_key=True, poll_tier='hot', entities=( + # Not deduped against DOORS_FALLBACK below: on the one reporter + # (refrigerator_tp2x_ref_20k_kimchi) this binds alongside, the + # /doors/vs/0 aggregate carries a single generic item (id "4", no + # /door/ siblings for DOORS_FALLBACK's match_fn to see) + # that doesn't share this compartment's "top" instance numbering -- + # a distinct main-cabinet door, not this kimchi drawer's own contact + # switch reported twice. BinarySensorDesc(key='open', rep_fn=_door_open_state, translation_key='instance_open', use_instance_name=True, device_class='door'), diff --git a/tests/test_fridge_capabilities.py b/tests/test_fridge_capabilities.py index 5ce2c1d..3b6399e 100644 --- a/tests/test_fridge_capabilities.py +++ b/tests/test_fridge_capabilities.py @@ -241,25 +241,85 @@ class TestKimchiZone: def test_write_derives_path_from_href(self): desc = fridge.KIMCHI_ZONE.entities[0] + rep = {'x.com.samsung.da.supportMode': ['KIMCHI_STORAGE_COLD']} path, body = desc.write_fn( - 'KIMCHI_STORAGE_COLD', {}, href='/status/kimchi/middle/vs/0') + 'KIMCHI_STORAGE_COLD', rep, href='/status/kimchi/middle/vs/0') assert path == ['status', 'kimchi', 'middle', 'vs', '0'] assert body == {'x.com.samsung.da.currentMode': 'KIMCHI_STORAGE_COLD'} def test_write_without_href_is_rejected(self): desc = fridge.KIMCHI_ZONE.entities[0] - assert desc.write_fn('KIMCHI_STORAGE_COLD', {}) is None + rep = {'x.com.samsung.da.supportMode': ['KIMCHI_STORAGE_COLD']} + assert desc.write_fn('KIMCHI_STORAGE_COLD', rep) is None - def test_ripening_status_is_lowercased(self): + def test_write_rejects_value_outside_supportmode(self): + """A value the compartment never advertised is rejected rather than + written blind -- this write path is unconfirmed against real + hardware (module docstring above KIMCHI_ZONE), so a bad value here + is a food-safety-adjacent outcome, not just a cosmetic one.""" + desc = fridge.KIMCHI_ZONE.entities[0] + rep = {'x.com.samsung.da.supportMode': ['KIMCHI_STORAGE_COLD']} + assert desc.write_fn( + 'KIMCHI_STORAGE_WARM', rep, href='/status/kimchi/middle/vs/0', + ) is None + + def test_ripening_status_passes_through_device_value(self): + """No device_class='enum' catalog entry exists for this sensor, so + lowercasing it would only make the raw device token un-translatable + by HA -- pass the device's own casing straight through instead.""" desc = next(e for e in fridge.KIMCHI_ZONE.entities if e.key == 'ripening_status') - assert desc.value_fn('Off') == 'off' - assert desc.value_fn(None) is None + assert desc.value_fn('Off') == 'Off' def test_door_reuses_open_state_helper(self): desc = fridge.KIMCHI_DOOR_GENERIC.entities[0] assert desc.rep_fn({'x.com.samsung.da.openState': 'Open'}) is True assert desc.rep_fn({'x.com.samsung.da.openState': 'Close'}) is False + async def test_zone_mode_select_round_trips_through_display_casing(self): + """kimchi_zone_mode's displayed value (lowercase, catalog-translated) + and the raw device code it writes back can silently drift apart -- + this is the one place that casing conversion could break. Runs + through the real discovery/select pipeline against the tp2x_ref_20k + kimchi fixture rather than a hand-built descriptor, so it also + catches use_instance_name key derivation going wrong.""" + from custom_components.localthings.registry.adapter import flatten + from custom_components.localthings.registry.by_type import refrigerator + from custom_components.localthings.registry.discovery import discover + from custom_components.localthings.registry.entities import SelectDesc + from custom_components.localthings.select import LocalThingsSelect + from tests.conftest import _load_device + + resources = _load_device('refrigerator_tp2x_ref_20k_kimchi') + bound = discover( + resources, refrigerator.REGISTRY.capabilities, + refrigerator.REGISTRY.pattern_capabilities, + ) + mode_bound = next( + b for b in bound + if isinstance(b.desc, SelectDesc) and b.href == '/status/kimchi/middle/vs/0' + ) + + class _FakeCoordinator: + device_serial = 'TEST-SERIAL' + + def __init__(self, resources, data): + self.last_resources = resources + self.data = data + self.commands = [] + + async def async_send_command(self, bound, value): + self.commands.append(value) + + coordinator = _FakeCoordinator(resources, flatten(bound, resources)) + entity = LocalThingsSelect(coordinator, mode_bound) + + assert entity.current_option == 'kimchi_storage_normal' + assert 'kimchi_storage_cold' in entity.options + + await entity.async_select_option('kimchi_storage_cold') + + assert coordinator.commands == ['KIMCHI_STORAGE_COLD'] + class TestArtik051AndTp2xFixturesHaveCompleteCoverage: """issue #20 (ARTIK051_REF_17K) and #26 (TP2X_REF_20K) both triggered diff --git a/tests/test_golden_regression.py b/tests/test_golden_regression.py index a13fb98..09650ff 100644 --- a/tests/test_golden_regression.py +++ b/tests/test_golden_regression.py @@ -615,7 +615,7 @@ def test_registry_reproduces_golden_state_keys_for_microwave_me7500d(): a standalone hood this board has no sibling `/power/0` or `/power/vs/0` resource, so fan.py's LocalThingsRangeHoodFan treats fan speed 0 as the off state instead of writing a separate power - resource -- see its `_has_separate_power` check. This board also has + resource -- see its `_speed_zero_is_off` check. This board also has no `/temperatures/vs/0` or `x.com.samsung.da.hood.autoOperation` field, unlike MW7300B, so `setpoint`/`current_temp_c` and `automatic_operation` are correctly absent here.""" diff --git a/tests/test_range_hood_fan.py b/tests/test_range_hood_fan.py index 4da948b..a1fbf4d 100644 --- a/tests/test_range_hood_fan.py +++ b/tests/test_range_hood_fan.py @@ -98,12 +98,43 @@ async def test_power_write_falls_back_to_vendor_resource(): # itself the off state. # --------------------------------------------------------------------------- +def test_standalone_hood_has_separate_power(): + """Explicit converse of the microwave case below: guards the + discriminator itself, not just its downstream effects, so a future + change to it fails loudly here instead of only via behavioral drift.""" + entity = _entity(_load_device('range_hood')) + assert entity._has_separate_power() is True + assert entity._speed_zero_is_off() is False + + def test_microwave_vent_fan_has_no_separate_power_resource(): resources = _load_device('microwave_me7500d') assert '/power/0' not in resources assert '/power/vs/0' not in resources entity = _microwave_entity(resources) assert entity._has_separate_power() is False + assert entity._speed_zero_is_off() is True + + +async def test_combi_microwave_with_cavity_power_still_treats_zero_speed_as_fan_off(): + """A combi over-the-range microwave can report a /power/0 resource for + the cavity while the vent fan still has no power resource of its own + (settableMinFanSpeed '0' -- same board shape as microwave_me7500d). + _speed_zero_is_off must key off the hood resource itself, not merely + "some power resource exists on this device", so turning the fan off + writes fan speed rather than the shared cavity power resource.""" + resources = _load_device('microwave_me7500d') + resources['/power/0'] = {'value': True} + resources['/hood/fanspeed/vs/0']['x.com.samsung.da.hood.fanSpeed'] = '2' + coordinator = _FakeCoordinator(resources) + entity = _microwave_entity(resources, coordinator) + + assert entity._has_separate_power() is True + assert entity._speed_zero_is_off() is True + + await entity.async_turn_off() + + assert coordinator.commands[-1][1] == ('speed', '0') def test_microwave_vent_fan_off_state_excludes_zero_from_speed_codes(): @@ -153,3 +184,37 @@ async def test_microwave_vent_fan_set_percentage_writes_speed_only(): await entity.async_set_percentage(100) assert coordinator.commands == [(entity._bound, ('speed', '4'))] + + +async def test_microwave_vent_fan_turn_on_with_percentage_writes_speed_directly(): + resources = _load_device('microwave_me7500d') + coordinator = _FakeCoordinator(resources) + entity = _microwave_entity(resources, coordinator) + + await entity.async_turn_on(percentage=75) + + assert coordinator.commands == [(entity._bound, ('speed', '3'))] + + +async def test_microwave_vent_fan_turn_on_without_percentage_when_already_on_is_a_noop(): + """A scene or automation calling fan.turn_on on an already-running vent + fan must not reset it to the lowest speed.""" + resources = _load_device('microwave_me7500d') + resources['/hood/fanspeed/vs/0']['x.com.samsung.da.hood.fanSpeed'] = '3' + coordinator = _FakeCoordinator(resources) + entity = _microwave_entity(resources, coordinator) + + await entity.async_turn_on() + + assert coordinator.commands == [] + + +async def test_microwave_vent_fan_set_percentage_zero_turns_off(): + resources = _load_device('microwave_me7500d') + resources['/hood/fanspeed/vs/0']['x.com.samsung.da.hood.fanSpeed'] = '2' + coordinator = _FakeCoordinator(resources) + entity = _microwave_entity(resources, coordinator) + + await entity.async_set_percentage(0) + + assert coordinator.commands[-1][1] == ('speed', '0') diff --git a/tests/test_translations.py b/tests/test_translations.py index 15b58a8..94c879a 100644 --- a/tests/test_translations.py +++ b/tests/test_translations.py @@ -210,3 +210,27 @@ def test_every_ac_convenient_mode_code_has_a_preset_label(): missing.append((path.name, code)) assert missing == [] + +def test_every_kimchi_zone_supportmode_code_has_a_state_label(): + """Same guard as the AC preset one above, for KIMCHI_ZONE's + kimchi_zone_mode select (fridge.py, issue #26): the write path resolved + from options_field is fully dynamic too, so an unlabelled supportMode + code across any /status/kimchi//vs/0 resource would silently + render as its raw device token instead of the translated state. + """ + state_labels = set( + _load("en")["entity"]["select"]["kimchi_zone_mode"]["state"] + ) + fixtures_dir = Path(__file__).parent / "fixtures" + missing = [] + for path in sorted(fixtures_dir.glob("*_device.json")): + dump = json.loads(path.read_text()) + for item in dump.get("device0", []): + href = item.get("href", "") + if not (href.startswith("/status/kimchi/") and href.endswith("/vs/0")): + continue + for code in item["rep"].get("x.com.samsung.da.supportMode", []): + if code.lower() not in state_labels: + missing.append((path.name, href, code)) + assert missing == [] +