From e2dcc75ed5c61e86b7ba7816ea75cb98796f283f Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 7 Aug 2026 14:50:24 +0000 Subject: [PATCH] Address Opus review findings on the device-support commits above - Fix a real bug: airconditioner.SOUND_MODE had no exists_fn, so on boards (issue #319's FAC) that never report a live 'mode' value, entity.py's default field-presence gate silently kept the select from ever registering in HA -- while adapter.flatten() (what the golden/tests read) has no such gate, so the tests passed while documenting behavior the opposite of what shipped. Gate on supportedModes' presence instead. - Add airconditioner.MDS_ABSENCE_CLEAN for the CAC-class board's /mds/absenceclean/vs/0 -- byte-identical shape to issue #319's /csi/absenceclean/vs/0, confirmed rather than guessed, closing one more of that board's documented coverage-gap hrefs. - Add missing translation state labels (all 6 languages) for edge_lighting_mode/edge_lighting_color/indicator_light_mode's raw device codes, so they render as real words instead of a raw '3000K' -> '3000 K' fallback. - Fix an orphaned comment above SOUND_MODE that actually described the unrelated DISPLAY reuse, and correct two inaccurate rationale comments: the sound/voice ignore reason claimed a distinction from SOUND_MODE that this same dump contradicts, and the /csi/* ignore block's 'same reasoning as air_purifier.COVERAGE' precedent only actually covers 1 of its 5 hrefs. - Correct the false 'no board-token match' claim in the FAC test file and golden-regression docstring -- 'FAC' is a real _BOARD_TOKEN_TO_KEY entry (for_device_by_model alone already resolves this board); add a test that actually exercises that path, which nothing previously did despite the docstring's claim. - Drop a tautological burner-slot test that only re-asserted what the golden regression test already covers via the same code path. --- .../registry/by_type/airconditioner.py | 1 + .../registry/capabilities/airconditioner.py | 66 ++++++++++++++----- .../localthings/translations/cs.json | 21 +++++- .../localthings/translations/en.json | 21 +++++- .../localthings/translations/es.json | 21 +++++- .../localthings/translations/it.json | 21 +++++- .../localthings/translations/ko.json | 21 +++++- .../localthings/translations/nl.json | 21 +++++- tests/fixtures/golden/airconditioner_cac.json | 1 + ...st_airconditioner_ailp_fac_capabilities.py | 65 ++++++++++++++---- tests/test_airconditioner_cac.py | 27 ++++++-- .../test_gas_cooktop_tp2x_ks_capabilities.py | 17 ++--- tests/test_golden_regression.py | 15 +++-- 13 files changed, 250 insertions(+), 68 deletions(-) diff --git a/custom_components/localthings/registry/by_type/airconditioner.py b/custom_components/localthings/registry/by_type/airconditioner.py index 0ca07c3..15f6ea0 100644 --- a/custom_components/localthings/registry/by_type/airconditioner.py +++ b/custom_components/localthings/registry/by_type/airconditioner.py @@ -54,6 +54,7 @@ REGISTRY = DeviceRegistry( air_purifier.SOUND_VOLUME, airconditioner.SOUND_MODE, airconditioner.ABSENCE_CLEAN, + airconditioner.MDS_ABSENCE_CLEAN, airconditioner.ENERGY_SAVING, airconditioner.EDGE_LIGHTING, airconditioner.LIGHT_STATEFUL, diff --git a/custom_components/localthings/registry/capabilities/airconditioner.py b/custom_components/localthings/registry/capabilities/airconditioner.py index 482f4e7..ae5f636 100644 --- a/custom_components/localthings/registry/capabilities/airconditioner.py +++ b/custom_components/localthings/registry/capabilities/airconditioner.py @@ -1167,13 +1167,11 @@ HUMIDITY = Capability( ), ) -# TP1X_DA-AC-FAC-class additions (issue #319): resources the CAC-class board -# (issue #191) also reports but left as a documented gap -- this dump gives -# the live supportedModes/mode shapes those needed. +# TP1X_DA-AC-FAC-class additions (issue #319): most of these hrefs are the +# same shapes air_purifier.py already models on the sibling TP1X_DA-AC-AIR +# board (DISPLAY/SOUND_OUTPUT/SOUND_VOLUME, reused directly in the +# registry); SOUND_MODE and the two below are genuinely new. -# Same {mode, supportedModes: [On, Off]} shape as air_purifier.DISPLAY on the -# sibling TP1X_DA-AC-AIR board; shares that capability rather than -# duplicating it. SOUND_MODE = Capability( href="/settings/sound/mode/vs/0", poll_tier="cold", @@ -1182,12 +1180,22 @@ SOUND_MODE = Capability( # vocabulary, so this shares that catalog entry -- but reads the # live supportedModes field rather than laundry's static tuple, # since this resource carries one (issue #319). + # + # exists_fn is required, not optional here: this board's rep never + # reports a live 'mode' value ({"supportedModes": [...]} only), and + # entity.py's default field-presence gate would otherwise keep the + # select from ever registering -- adapter.flatten() (what the + # golden/tests read) has no such gate, so it would look bound while + # silently absent from HA. Register on supportedModes' presence + # instead; current_option reads unknown until the device reports + # 'mode' live. SelectDesc( key="sound_mode", field="mode", icon="mdi:volume-high", entity_category="config", options_field="supportedModes", + exists_fn=lambda rep, resources: bool(rep.get("supportedModes")), write_fn=lambda p, rep, href=None: ( ["settings", "sound", "mode", "vs", "0"], {"mode": p}, @@ -1216,6 +1224,30 @@ ABSENCE_CLEAN = Capability( ), ) +# The CAC-class board (issue #191) reports the identical {mode, +# supportedModes: [On, Off]} shape under /mds/absenceclean/vs/0 instead -- +# confirmed against that board's own fixture, not guessed. Shares +# ABSENCE_CLEAN's key/translation: no dump has ever reported both hrefs +# together, so there's nothing for the two to collide over in +# adapter.flatten(). +MDS_ABSENCE_CLEAN = Capability( + href="/mds/absenceclean/vs/0", + poll_tier="cold", + entities=( + SwitchDesc( + key="absence_clean", + field="mode", + icon="mdi:broom", + entity_category="config", + value_fn=lambda v: v == "On", + write_fn=lambda p, rep, href=None: ( + ["mds", "absenceclean", "vs", "0"], + {"mode": "On" if p == "On" else "Off"}, + ), + ), + ), +) + # Energy-saving schedule (issue #319). `mode` is a device-chosen preset # (e.g. Cooling_60/Off_180) with no confirmed unit for the trailing number # (minutes seen elsewhere on this board are unprefixed ints, not @@ -1415,19 +1447,23 @@ _AC_IGNORED = [ # registry.subdevices.enumerate_subdevices, hence the entry here rather # than a coverage gap. "/multidevice/vs/0", - # TP1X_DA-AC-FAC-class-only (issue #319), same reasoning as - # air_purifier.COVERAGE's duplicate of these hrefs -- scoped here rather - # than promoted to the global ignore list since it'd collide with - # families that do bind some of these. + # TP1X_DA-AC-FAC-class-only (issue #319) -- scoped here rather than + # promoted to the global ignore list since it'd collide with families + # that do bind some of these. Only /dnd/autosleep/vs/0 has a precedent + # (air_purifier.COVERAGE ignores the same href for the same reason); + # the rest are new, each with its own reason below. "/dnd/autosleep/vs/0", # every field its inert default; needs a schedule editor "/outdoorsharing/vs/0", # empty on this dump -- outdoor-unit sharing plumbing "/lifestyle/survey/vs/0", # {list: [""]} placeholder, nothing to expose - # supportedVoices list only -- no live selection field to read a current - # value from, unlike SOUND_MODE/SOUND_OUTPUT above. + # supportedVoices carries opaque numeric voice-pack IDs ("100"/"101") + # with no live current-selection field and, unlike SOUND_MODE's + # self-descriptive mute/tone/voice codes, no confirmed human-readable + # meaning to expose them under -- don't guess. "/settings/sound/voice/vs/0", - # rssi/wifiFrequency (network housekeeping) plus an unconfirmed - # 48-slot absenceInfo P/A history blob with no documented meaning -- - # don't guess what it encodes. + # rssi/wifiFrequency (network housekeeping); lastEnergySavingTime and + # cleaningStartTime are inert '1900-01-00' placeholders on this dump; + # absenceInfo is an unconfirmed 48-slot P/A history blob with no + # documented meaning -- don't guess what it encodes. "/csi/information/vs/0", ] diff --git a/custom_components/localthings/translations/cs.json b/custom_components/localthings/translations/cs.json index 6853ed7..b8779b4 100644 --- a/custom_components/localthings/translations/cs.json +++ b/custom_components/localthings/translations/cs.json @@ -670,13 +670,28 @@ "name": "Teplota mrazicí zóny" }, "edge_lighting_mode": { - "name": "Režim okrajového osvětlení" + "name": "Režim okrajového osvětlení", + "state": { + "smart": "Chytrý", + "high": "Vysoký", + "low": "Nízký" + } }, "edge_lighting_color": { - "name": "Barva okrajového osvětlení" + "name": "Barva okrajového osvětlení", + "state": { + "3000k": "3000 K", + "4000k": "4000 K", + "6500k": "6500 K" + } }, "indicator_light_mode": { - "name": "Režim kontrolky" + "name": "Režim kontrolky", + "state": { + "smart": "Chytrý", + "high": "Vysoký", + "low": "Nízký" + } } }, "sensor": { diff --git a/custom_components/localthings/translations/en.json b/custom_components/localthings/translations/en.json index b1f3e08..ced1466 100644 --- a/custom_components/localthings/translations/en.json +++ b/custom_components/localthings/translations/en.json @@ -670,13 +670,28 @@ "name": "Freezer temperature" }, "edge_lighting_mode": { - "name": "Edge lighting mode" + "name": "Edge lighting mode", + "state": { + "smart": "Smart", + "high": "High", + "low": "Low" + } }, "edge_lighting_color": { - "name": "Edge lighting color" + "name": "Edge lighting color", + "state": { + "3000k": "3000 K", + "4000k": "4000 K", + "6500k": "6500 K" + } }, "indicator_light_mode": { - "name": "Indicator light mode" + "name": "Indicator light mode", + "state": { + "smart": "Smart", + "high": "High", + "low": "Low" + } } }, "sensor": { diff --git a/custom_components/localthings/translations/es.json b/custom_components/localthings/translations/es.json index 84a86ae..6b6d579 100644 --- a/custom_components/localthings/translations/es.json +++ b/custom_components/localthings/translations/es.json @@ -795,13 +795,28 @@ "name": "Temperatura del congelador" }, "edge_lighting_mode": { - "name": "Modo de iluminación perimetral" + "name": "Modo de iluminación perimetral", + "state": { + "smart": "Inteligente", + "high": "Alto", + "low": "Bajo" + } }, "edge_lighting_color": { - "name": "Color de iluminación perimetral" + "name": "Color de iluminación perimetral", + "state": { + "3000k": "3000 K", + "4000k": "4000 K", + "6500k": "6500 K" + } }, "indicator_light_mode": { - "name": "Modo de luz indicadora" + "name": "Modo de luz indicadora", + "state": { + "smart": "Inteligente", + "high": "Alto", + "low": "Bajo" + } } }, "sensor": { diff --git a/custom_components/localthings/translations/it.json b/custom_components/localthings/translations/it.json index df2c7b8..d8bb417 100644 --- a/custom_components/localthings/translations/it.json +++ b/custom_components/localthings/translations/it.json @@ -670,13 +670,28 @@ "name": "Temperatura freezer" }, "edge_lighting_mode": { - "name": "Modalità illuminazione perimetrale" + "name": "Modalità illuminazione perimetrale", + "state": { + "smart": "Smart", + "high": "Alta", + "low": "Bassa" + } }, "edge_lighting_color": { - "name": "Colore illuminazione perimetrale" + "name": "Colore illuminazione perimetrale", + "state": { + "3000k": "3000 K", + "4000k": "4000 K", + "6500k": "6500 K" + } }, "indicator_light_mode": { - "name": "Modalità spia luminosa" + "name": "Modalità spia luminosa", + "state": { + "smart": "Smart", + "high": "Alta", + "low": "Bassa" + } } }, "sensor": { diff --git a/custom_components/localthings/translations/ko.json b/custom_components/localthings/translations/ko.json index c018827..15456a7 100644 --- a/custom_components/localthings/translations/ko.json +++ b/custom_components/localthings/translations/ko.json @@ -670,13 +670,28 @@ "name": "냉동실 온도" }, "edge_lighting_mode": { - "name": "엣지 라이팅 모드" + "name": "엣지 라이팅 모드", + "state": { + "smart": "스마트", + "high": "높음", + "low": "낮음" + } }, "edge_lighting_color": { - "name": "엣지 라이팅 색상" + "name": "엣지 라이팅 색상", + "state": { + "3000k": "3000 K", + "4000k": "4000 K", + "6500k": "6500 K" + } }, "indicator_light_mode": { - "name": "표시등 모드" + "name": "표시등 모드", + "state": { + "smart": "스마트", + "high": "높음", + "low": "낮음" + } } }, "sensor": { diff --git a/custom_components/localthings/translations/nl.json b/custom_components/localthings/translations/nl.json index 8bb5965..c7703d9 100644 --- a/custom_components/localthings/translations/nl.json +++ b/custom_components/localthings/translations/nl.json @@ -670,13 +670,28 @@ "name": "Temperatuur vriesgedeelte" }, "edge_lighting_mode": { - "name": "Randverlichtingsmodus" + "name": "Randverlichtingsmodus", + "state": { + "smart": "Slim", + "high": "Hoog", + "low": "Laag" + } }, "edge_lighting_color": { - "name": "Randverlichtingskleur" + "name": "Randverlichtingskleur", + "state": { + "3000k": "3000 K", + "4000k": "4000 K", + "6500k": "6500 K" + } }, "indicator_light_mode": { - "name": "Indicatorlampjemodus" + "name": "Indicatorlampjemodus", + "state": { + "smart": "Slim", + "high": "Hoog", + "low": "Laag" + } } }, "sensor": { diff --git a/tests/fixtures/golden/airconditioner_cac.json b/tests/fixtures/golden/airconditioner_cac.json index 2426c0e..754794b 100644 --- a/tests/fixtures/golden/airconditioner_cac.json +++ b/tests/fixtures/golden/airconditioner_cac.json @@ -1,5 +1,6 @@ { "state_keys": [ + "absence_clean", "absence_power_saving_active", "absence_power_saving_mode", "air_filter_pm1_status", diff --git a/tests/test_airconditioner_ailp_fac_capabilities.py b/tests/test_airconditioner_ailp_fac_capabilities.py index b98869b..5af0f20 100644 --- a/tests/test_airconditioner_ailp_fac_capabilities.py +++ b/tests/test_airconditioner_ailp_fac_capabilities.py @@ -1,21 +1,42 @@ """Tests for the AILP_DA-AC-FAC-02011_0000 air conditioner (issue #319). -This board has no `_BOARD_TOKEN_TO_KEY` entry (routes via `/oic/d`'s -`oic.d.airconditioner` type only) and reports several resources the sibling -TP1X_DA-AC-CAC-01001 board (issue #191) left as a documented gap: -/display/vs/0 and the /settings/sound/* trio (now shared with -air_purifier.py's identical shapes), plus two genuinely new hrefs -(/csi/absenceclean/vs/0, /csi/energysaving/vs/0). +This board resolves to the airconditioner registry two independent ways -- +modelNum's 'FAC' board token (for_device_by_model) and /oic/d's +oic.d.airconditioner type (for_device_by_oic_type) agree -- and reports +several resources the sibling TP1X_DA-AC-CAC-01001 board (issue #191) left +as a documented gap: /display/vs/0 and the /settings/sound/* trio (now +shared with air_purifier.py's identical shapes), plus two genuinely new +hrefs (/csi/absenceclean/vs/0, /csi/energysaving/vs/0). """ +from typing import cast + +from custom_components.localthings.coordinator import LocalThingsCoordinator +from custom_components.localthings.entity import _is_included from custom_components.localthings.registry.adapter import flatten -from custom_components.localthings.registry.by_type import for_device_by_oic_type, resolve +from custom_components.localthings.registry.by_type import ( + for_device_by_model, + for_device_by_oic_type, + resolve, +) from custom_components.localthings.registry.discovery import discover from tests.conftest import _load_device _DEVICE_TYPES = ("oic.wk.d", "oic.d.airconditioner") +class _FakeCoordinator: + """Minimal stand-in for entity._is_included's coordinator dependency -- + same shape as test_entity.py's own fake, kept local rather than shared + across test modules.""" + + def __init__(self, last_resources): + self.last_resources = last_resources + + def canonical_resources(self, subdevice): + return self.last_resources + + def _resources(): return _load_device("airconditioner_ailp_fac") @@ -31,6 +52,19 @@ def test_oic_device_type_resolves_to_airconditioner_registry(): assert reg is not None and reg.name == "airconditioner" +def test_board_token_resolves_to_airconditioner_registry(): + """'FAC' is a real _BOARD_TOKEN_TO_KEY entry -- for_device_by_model + alone (no device_types) already resolves this board correctly, same as + for_device_by_oic_type above; resolve()'s device_types-agreement isn't + covering an otherwise-unreachable path.""" + resources = _resources() + info = resources["/information/vs/0"] + reg = for_device_by_model( + info["x.com.samsung.da.modelNum"], info["x.com.samsung.da.description"] + ) + assert reg is not None and reg.name == "airconditioner" + + def test_no_unbound_hrefs(): resources = _resources() reg = resolve(resources, device_types=_DEVICE_TYPES) @@ -39,15 +73,18 @@ def test_no_unbound_hrefs(): assert unbound == [] -def test_sound_mode_bound_with_live_supported_modes(): +def test_sound_mode_registers_despite_no_live_mode_value(): """This dump has no current `mode` value yet, only supportedModes - (mute/tone/voice) -- confirms the select is bound (present in state, - even if unknown) and its options come from the live field rather than - a static tuple (see laundry.SOUND_MODE's docstring for why that's - preferred whenever the resource carries one).""" + (mute/tone/voice). entity.py's default field-presence gate would + otherwise keep the select from ever being created in HA even though + adapter.flatten() (checked below) has no such gate and would look + bound regardless -- exercise the real _is_included gate, not just + flatten(), so a regression here can't hide behind that gap again.""" bound, resources = _bound() - desc = next(b.desc for b in bound if b.desc.key == "sound_mode") - assert desc.options_field == "supportedModes" + entity = next(b for b in bound if b.desc.key == "sound_mode") + assert entity.desc.options_field == "supportedModes" + coord = cast(LocalThingsCoordinator, _FakeCoordinator(resources)) + assert _is_included(entity, coord) is True state = flatten(bound, resources) assert "sound_mode" in state assert state["sound_mode"] is None diff --git a/tests/test_airconditioner_cac.py b/tests/test_airconditioner_cac.py index 7aa0a64..ec0346f 100644 --- a/tests/test_airconditioner_cac.py +++ b/tests/test_airconditioner_cac.py @@ -6,11 +6,11 @@ token -- this board was the one exception (its oneUiVersion self-reports "7.0 Air conditioner", but 'CAC' had never been added to the board-token table), so it silently fell back to common caps and lost its climate entity. -This dump is NOT fully covered yet -- three hrefs remain unbound -(absence-clean, `/settings/sound/optimization/vs/0`, smart-sensing-cooling), -all genuinely new to this board generation. That's a real device-support -gap, left documented here rather than guessed at, per the 'don't guess' -rule -- fixing the routing regression was the scope of #191. +This dump is NOT fully covered yet -- two hrefs remain unbound +(`/settings/sound/optimization/vs/0`, smart-sensing-cooling), both +genuinely new to this board generation. That's a real device-support gap, +left documented here rather than guessed at, per the 'don't guess' rule -- +fixing the routing regression was the scope of #191. /settings/sound/mode/vs/0, /settings/sound/output/vs/0 and /settings/sound/volume/vs/0 used to be on this list too, until issue #319 @@ -23,6 +23,11 @@ until issue #288 (six System A/C cassette units on this same board) gave real dump evidence for both -- airconditioner.EDGE_LIGHTING and LIGHT_STATEFUL now cover them. +/mds/absenceclean/vs/0 used to be on this list too -- its {mode, +supportedModes: [On, Off]} shape is byte-identical to issue #319's +/csi/absenceclean/vs/0, confirmed on that sibling board rather than +guessed, so airconditioner.MDS_ABSENCE_CLEAN now covers it too. + /uvled/vs/0 and /filter/airdustPM1filter/vs/0 used to be on this list too, until issue #270 (TP1X_FAC_TIME_23K) added real capabilities for both -- this board's own live filterUsage/filterStatus data on the PM1 filter binds @@ -37,7 +42,6 @@ from tests.conftest import _load_device _STILL_UNBOUND = frozenset( { - "/mds/absenceclean/vs/0", "/settings/sound/optimization/vs/0", "/smartsensingcooling/vs/0", } @@ -70,6 +74,17 @@ def test_documented_coverage_gap_is_exactly_this_set(): assert set(unbound) == _STILL_UNBOUND +def test_mds_absenceclean_shares_csi_absenceclean_key(): + """/mds/absenceclean/vs/0's mode=='Off' on this dump -- confirms + MDS_ABSENCE_CLEAN actually binds (not just that the href stops + reporting as unbound).""" + resources = _resources() + reg = _reg(resources) + bound = discover(resources, reg.capabilities, reg.pattern_capabilities) + state = flatten(bound, resources) + assert state["absence_clean"] is False + + def test_non_legacy_board_uses_the_generic_energy_scale(): """This board reports /wind/strength/vs/0 (not /airflow/vs/0), so is_legacy_board() is False and it must use the plain wh_to_kwh scale, diff --git a/tests/test_gas_cooktop_tp2x_ks_capabilities.py b/tests/test_gas_cooktop_tp2x_ks_capabilities.py index 7677618..340675e 100644 --- a/tests/test_gas_cooktop_tp2x_ks_capabilities.py +++ b/tests/test_gas_cooktop_tp2x_ks_capabilities.py @@ -45,11 +45,12 @@ def test_alarm_code_and_child_lock_bound(): assert state["child_lock"] is True -def test_advertises_six_burner_slots_only_three_of_which_are_real(): - """Not a bug to fix -- the board's own /mode/vs/0 options genuinely - list six OperationState slots (issue #314); the reporter's physical - cooktop only has three. Locks in the current, documented behavior.""" - bound, resources = _bound() - state = flatten(bound, resources) - for i in range(6): - assert f"burner_{i}_state" in state +# The reporter's original complaint -- six advertised burner slots when +# only three are physically present -- isn't tested here beyond what the +# golden regression test already locks in (burner_0_state..burner_5_state +# all present). It's expected, not a bug: the registry declares a generous +# static superset of slots per cooktop.py's own comment, and this board's +# /mode/vs/0 rep carries no field distinguishing a real slot from an +# advertised-but-nonexistent one for a test to assert against -- the user +# disabling the three extra entities is the correct fix, not something the +# integration can filter automatically. diff --git a/tests/test_golden_regression.py b/tests/test_golden_regression.py index dd6932d..1d453f0 100644 --- a/tests/test_golden_regression.py +++ b/tests/test_golden_regression.py @@ -135,9 +135,11 @@ def test_registry_reproduces_golden_state_keys_for_airconditioner(): def test_registry_reproduces_golden_state_keys_for_airconditioner_ailp_fac(): - """AILP_DA-AC-FAC-02011_0000 (issue #319) has no board-token match -- - resolves purely via /oic/d's oic.d.airconditioner type, exercising the - device_types-only path through resolve().""" + """AILP_DA-AC-FAC-02011_0000 (issue #319) resolves both ways -- + modelNum's 'FAC' board token and /oic/d's oic.d.airconditioner type + independently agree on the airconditioner registry. Passes + device_types through resolve() to exercise that agreement, not because + the board token is absent.""" from tests.conftest import _load_device resources = _load_device("airconditioner_ailp_fac") @@ -1173,10 +1175,9 @@ def test_registry_reproduces_golden_state_keys_for_airconditioner_cac(): 0.16.0 when oneUiVersion detection was dropped, since 'CAC' had never been added to the modelNum board-token table. Resolved via the new 'CAC' token onto the existing airconditioner registry. Not fully covered yet -- - three hrefs remain unbound (absence-clean, sound-optimization, - smart-sensing-cooling), all genuinely new to this board generation and - out of scope for the routing fix; see test_airconditioner_cac.py for - the documented gap.""" + two hrefs remain unbound (sound-optimization, smart-sensing-cooling), + both genuinely new to this board generation and out of scope for the + routing fix; see test_airconditioner_cac.py for the documented gap.""" from tests.conftest import _load_device resources = _load_device("airconditioner_cac")