From b7d3f86ec7f25c2548a058ca3cdecd36e27cb5b8 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 31 Jul 2026 15:17:18 -0500 Subject: [PATCH] fix: address code review feedback on issue-triage batch PR (#181, #183, #189, #196, #207, #208, #210) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit common: - Make KIDS_LOCK_GENERIC also a read-only BinarySensorDesc (device_class='lock'), flipping value_fn to not bool(v) so /kidslock/0 value=False and /kidslock/vs/0 kidsLock='Ready' render with the same polarity ('On' means open/unlocked per HA's lock device class). The old SwitchDesc form never honored device_class='lock' -- HA's switch platform only accepts 'outlet'/'switch' -- so the surface was a plain switch whose 'On' meaning drifted across boards. Tests updated. air_monitor: - Add state_class='measurement' to dust/fine_dust/super_fine_dust so the readings feed HA long-term statistics (co2 already had it). - Import _AIR_QUALITY_SENSORS from air_purifier instead of duplicating it byte-for-byte; update common.sensor_item_value's docstring to mention the third caller. by_type/__init__: drop trailing whitespace on the new 'ASM' line. translations/en.json + nl.json: move the new 'dnd' switch entry to its correct alphabetical position (after display_light, before fast_preheat). SKILL.md: add an explicit read-side rule to ยง5's educated-guesses section -- guessed unit/device_class/state_class on a SensorDesc silently mislabels readings forever with no 4.xx to catch it (unlike guessed writes, which the device rejects). The prior air_monitor docstring cited this carve-out as if it existed; now it does. tests/water_purifier (issue #196): change the ailite fixture's favorite.defaultTemperature from '85' to '50' so the test actually reproduces the reported bug -- '50' is in showList only, so a descriptor reading from supportedList would fail the assertion that the current default is in its options list. --- .claude/skills/adding-device-support/SKILL.md | 16 +++++++++ .../localthings/registry/by_type/__init__.py | 2 +- .../registry/capabilities/air_monitor.py | 34 +++++++------------ .../registry/capabilities/common.py | 33 +++++++++++------- .../localthings/translations/en.json | 6 ++-- .../localthings/translations/nl.json | 6 ++-- .../water_purifier_ailite_25k_device.json | 2 +- tests/test_common_capabilities.py | 16 ++++++--- tests/test_water_purifier_capabilities.py | 24 +++++++------ 9 files changed, 83 insertions(+), 56 deletions(-) diff --git a/.claude/skills/adding-device-support/SKILL.md b/.claude/skills/adding-device-support/SKILL.md index ad7fc9e..57078d7 100644 --- a/.claude/skills/adding-device-support/SKILL.md +++ b/.claude/skills/adding-device-support/SKILL.md @@ -297,6 +297,22 @@ to cross-check (temperature scale, minutes vs. seconds) โ€” being syntactically valid but semantically backwards is exactly the case a rejection won't catch. +The same unit caveat applies on the **read** side, but with a sharper +failure mode: a guessed `unit`, `device_class`, or `state_class` on a +`SensorDesc` silently mislabels the entity in HA forever (every reading, +every graph, every long-term statistic), with no 4.xx to catch it. The +write-rejection safety net above doesn't cover reads โ€” the device happily +returns whatever it returns. So the read-side equivalent of "bind a write +to the device's own reported range/supported-list" is: leave `unit`/ +`device_class`/`state_class` unset when the dump gives no field that +nominates one (no `supportedGrades`, no second dump to compare against, +no family member whose same field is already mapped). Match an +already-bound descriptor on a sibling family when the underlying field +and value shape are identical; otherwise expose the reading without an +HA-level interpretation and let a future reporter or dump confirm it. +See `air_monitor.AIR_QUALITY`'s docstring for the worked example +(three dust keys, no `device_class`, no `unit`). + Still never invent an entity or a write from nothing: an opaque encoded blob with no supported-values field, no range, and no idle-vs-active diff to compare against is a gap for a human, not a guess โ€” leave it unbound, or diff --git a/custom_components/localthings/registry/by_type/__init__.py b/custom_components/localthings/registry/by_type/__init__.py index 1055668..8fa9d05 100644 --- a/custom_components/localthings/registry/by_type/__init__.py +++ b/custom_components/localthings/registry/by_type/__init__.py @@ -114,7 +114,7 @@ _BOARD_TOKEN_TO_KEY: dict[str, str] = { 'VSKR': 'vacuum_station', # issue #131 -- stick-vacuum clean station 'DF': 'air_dresser', # issue #162 'VSWW': 'vacuum_station', # issue #219 - 'ASM': 'air_monitor', # issue #210 -- Air Monitor Plus + 'ASM': 'air_monitor', # issue #210 -- Air Monitor Plus } _TOKEN_SPLIT_RE = re.compile(r'[^A-Z0-9]+') diff --git a/custom_components/localthings/registry/capabilities/air_monitor.py b/custom_components/localthings/registry/capabilities/air_monitor.py index fab646a..f5f1ee5 100644 --- a/custom_components/localthings/registry/capabilities/air_monitor.py +++ b/custom_components/localthings/registry/capabilities/air_monitor.py @@ -19,40 +19,32 @@ rule. Same reasoning `air_purifier.AIR_QUALITY` already applies to this shape; index 0 is the only slot any family has ever read. Dust/FineDust/SuperFineDust aren't assigned an HA `device_class` -(pm10/pm25/pm1) despite the values reading like plausible ug/m3 -particulate readings in a physically consistent order (coarser >= finer): -Samsung's own two-tier Korean convention (i.e. "fine dust"/"ultra-fine -dust") maps only to a PM10/PM2.5 pair, and this board's three-tier -naming doesn't confirm where the extra tier or a PM1 reading actually -fits. Guessing wrong here would silently mislabel what unit a user reads -on a graph forever, not just fail one write -- the "worst case is -rejected" case for a flagged write guess doesn't cover a read-side -device_class/unit guess, per the skill's own carve-out. Exposed as plain -measurement sensors named after the device's own field instead (matching -air_purifier.AIR_QUALITY's existing precedent). +(pm10/pm25/pm1) or `unit` despite the values reading like plausible +ug/m3 particulate readings in a physically consistent order (coarser +>= finer): Samsung's own two-tier Korean convention (i.e. "fine dust"/ +"ultra-fine dust") maps only to a PM10/PM2.5 pair, and this board's +three-tier naming doesn't confirm where the extra tier or a PM1 reading +actually fits. The adding-device-support skill's read-side rule says +leave unit/device_class unset when the dump gives no field that +nominates one -- a wrong guess would silently mislabel every reading +forever, and the write-side rejection safety net doesn't cover reads. +Exposed as plain `measurement` sensors named after the device's own +field instead (matching air_purifier.AIR_QUALITY's existing precedent). """ from datetime import time as dt_time from ..capability import Capability from ..entities import BinarySensorDesc, SensorDesc, SwitchDesc, TimeDesc +from .air_purifier import _AIR_QUALITY_SENSORS from .common import int_or_none, sensor_item_value -_AIR_QUALITY_SENSORS = ( - ('dust', 'mdi:blur', 'Dust'), - ('fine_dust', 'mdi:blur', 'FineDust'), - ('super_fine_dust', 'mdi:blur', 'SuperFineDust'), - ('odor', 'mdi:scent', 'Odor'), - ('clean_level', 'mdi:air-filter', 'CleanLevel'), -) SENSORS = Capability( href='/sensors/vs/0', poll_tier='warm', entities=tuple( - # No state_class here, matching air_purifier.AIR_QUALITY's existing - # descriptors exactly -- these keys are shared catalog entries with - # that capability, so the two should behave identically. SensorDesc(key=key, field='x.com.samsung.da.items', icon=icon, + state_class='measurement', value_fn=lambda items, t=sensor_type: sensor_item_value(items, t)) for key, icon, sensor_type in _AIR_QUALITY_SENSORS ) + ( diff --git a/custom_components/localthings/registry/capabilities/common.py b/custom_components/localthings/registry/capabilities/common.py index 945903a..984a6fe 100644 --- a/custom_components/localthings/registry/capabilities/common.py +++ b/custom_components/localthings/registry/capabilities/common.py @@ -228,8 +228,9 @@ def sensor_item_value(items, sensor_type, index=0): """Pull one reading out of a `/sensors/vs/0`-style items[] list -- each item is `{type, value: [...]}`; `index` picks which slot of a possibly multi-value reading to read (index 0 is the raw measurement on every - family seen so far). Shared by range_hood.AIR_QUALITY and - air_purifier.AIR_QUALITY, which read the same resource shape.""" + family seen so far). Shared by range_hood.AIR_QUALITY, + air_purifier.AIR_QUALITY, and air_monitor.SENSORS, which all read the + same resource shape against the same {type, sensor_type} keys.""" for item in items or (): if not isinstance(item, dict): continue @@ -301,11 +302,19 @@ POWER_VS_FALLBACK = Capability( KIDS_LOCK_GENERIC = Capability( href='/kidslock/0', entities=( - SwitchDesc(key='child_lock', field='value', - device_class='lock', - value_fn=lambda v: bool(v), - write_fn=lambda p, rep, href=None: ( - ['kidslock', '0'], {'value': p == 'On'})), + # Read-only like KIDS_LOCK_VS_FALLBACK (issues #181/#183) -- not a + # SwitchDesc. SwitchDesc's `device_class='lock'` was never honored + # by HA (its switch platform only accepts 'outlet'/'switch'), + # leaving a plain switch whose 'On' state meant different things + # on different boards. As a BinarySensorDesc with `device_class='lock'`, + # both kids-lock surfaces read with the same polarity: 'On' means + # open/unlocked, per HA's lock device_class. The inversion in + # value_fn here (and in the fallback below) keeps the on-the-wire + # truth (value=False on /kidslock/0, kidsLock='Ready' on /kidslock/vs/0 + # both mean kids lock NOT active) consistent with that polarity. + BinarySensorDesc(key='child_lock', field='value', + device_class='lock', + value_fn=lambda v: not bool(v)), ), ) @@ -320,12 +329,12 @@ KIDS_LOCK_VS_FALLBACK = Capability( # confirmed this directly: writing the *correct* value ('Run') # still 4.05s, and the SmartThings app itself has no control for # it either -- the resource is genuinely read-only on this - # hardware, not just wrong-valued. binary_sensor's 'lock' device - # class is inverted from the switch reading below it used to be: - # On means open/unlocked, so value_fn flips to `v == 'Ready'`. + # hardware, not just wrong-valued. Polarity matches + # KIDS_LOCK_GENERIC above -- 'On' means open/unlocked, so + # kidsLock='Ready' (kids lock NOT active) renders as 'On'. BinarySensorDesc(key='child_lock', field='x.com.samsung.da.kidsLock', - device_class='lock', - value_fn=lambda v: v == 'Ready'), + device_class='lock', + value_fn=lambda v: v == 'Ready'), ), ) diff --git a/custom_components/localthings/translations/en.json b/custom_components/localthings/translations/en.json index b5b645a..50e4464 100644 --- a/custom_components/localthings/translations/en.json +++ b/custom_components/localthings/translations/en.json @@ -1008,12 +1008,12 @@ "defrost_delay": { "name": "Defrost delay" }, - "dnd": { - "name": "Do not disturb" - }, "display_light": { "name": "Display light" }, + "dnd": { + "name": "Do not disturb" + }, "fast_preheat": { "name": "Fast preheat" }, diff --git a/custom_components/localthings/translations/nl.json b/custom_components/localthings/translations/nl.json index 8da541c..8aff30a 100644 --- a/custom_components/localthings/translations/nl.json +++ b/custom_components/localthings/translations/nl.json @@ -1008,12 +1008,12 @@ "defrost_delay": { "name": "Ontdooien uitstellen" }, - "dnd": { - "name": "Niet storen" - }, "display_light": { "name": "Displayverlichting" }, + "dnd": { + "name": "Niet storen" + }, "fast_preheat": { "name": "Snel voorverwarmen" }, diff --git a/tests/fixtures/water_purifier_ailite_25k_device.json b/tests/fixtures/water_purifier_ailite_25k_device.json index 9ae21aa..cc97237 100644 --- a/tests/fixtures/water_purifier_ailite_25k_device.json +++ b/tests/fixtures/water_purifier_ailite_25k_device.json @@ -166,7 +166,7 @@ "href": "/favorite/hotwater/vs/0", "rep": { "x.com.samsung.da.favorite.revision": "0", - "x.com.samsung.da.favorite.defaultTemperature": "85", + "x.com.samsung.da.favorite.defaultTemperature": "50", "x.com.samsung.da.favorite.showList": [ "40", "50", diff --git a/tests/test_common_capabilities.py b/tests/test_common_capabilities.py index 7b9741b..2fdfd0d 100644 --- a/tests/test_common_capabilities.py +++ b/tests/test_common_capabilities.py @@ -232,13 +232,19 @@ class TestWmSetinfoFlags: class TestKidsLockFallback: - def test_generic_read_write(self): + def test_generic_is_read_only(self): + """Issues #181/#183: the kids-lock resource is read-only on real + hardware (writing the *correct* value still 4.05s, and no + SwitchDesc device_class='lock' is honored by HA -- the switch + platform only accepts 'outlet'/'switch'). KIDS_LOCK_GENERIC and + KIDS_LOCK_VS_FALLBACK now share the same shape (BinarySensorDesc, + device_class='lock', same key) so both surfaces render with the + same polarity: 'On' means open/unlocked.""" assert common.KIDS_LOCK_GENERIC.href == '/kidslock/0' desc = common.KIDS_LOCK_GENERIC.entities[0] - assert desc.value_fn(True) is True - path, body = desc.write_fn('On', {}) - assert path == ['kidslock', '0'] - assert body == {'value': True} + assert isinstance(desc, BinarySensorDesc) + assert desc.value_fn(False) is True # value=False -> On=Unlocked + assert desc.value_fn(True) is False # value=True -> Off=Locked def test_vs_fallback_gated(self): assert common.KIDS_LOCK_VS_FALLBACK.match_fn({}, {'/kidslock/vs/0': {}}) is True diff --git a/tests/test_water_purifier_capabilities.py b/tests/test_water_purifier_capabilities.py index 5721fd7..2f29b5a 100644 --- a/tests/test_water_purifier_capabilities.py +++ b/tests/test_water_purifier_capabilities.py @@ -370,21 +370,25 @@ def test_ailite_hot_water_temperature_gated_off_without_supported_list(): def test_ailite_favorite_hotwater_temperature_options_include_the_custom_value(): - """This board's real dump (issue #196) is the concrete case: the user - added a custom 50C value via the SmartThings app's "temperatures to - display" editor, so favorite.showList is - ['40', '50', '75', '85', '90'] while favorite.supportedList stays the - fixed ['40', '75', '85', '90'] -- '50' only ever appears in showList. - Whichever of the two the descriptor reads from, a default temperature - of '50' would render as HA's 'unknown' state unless that field's raw - option list actually contains '50'.""" + """Issue #196's concrete failure case: the user added a custom 50C value + via the SmartThings app's "temperatures to display" editor, so the + board's defaultTemperature is now '50'. showList contains '50' + ([40, 50, 75, 85, 90]) but supportedList does NOT (still the fixed + [40, 75, 85, 90]). Reading options_field='x.com.samsung.da.favorite.supportedList' + -- the old behavior -- would register a select whose options list + doesn't contain the current default, so HA would render the entity as + 'unknown'. The descriptor must read from showList so '50' is in + options.""" desc = _desc_ailite('favorite_hotwater_temperature') assert desc.options_field == 'x.com.samsung.da.favorite.showList' _, resources = _water_purifier_ailite() rep = resources['/favorite/hotwater/vs/0'] - assert rep['x.com.samsung.da.favorite.defaultTemperature'] in rep[desc.options_field] + # defaultTemperature='50' must be a member of the field the descriptor + # actually reads -- this is the precise assertion that would fail under + # the old supportedList behavior. + assert rep['x.com.samsung.da.favorite.defaultTemperature'] == '50' + assert '50' in rep[desc.options_field] assert '50' not in rep['x.com.samsung.da.favorite.supportedList'] - assert '50' in rep['x.com.samsung.da.favorite.showList'] def test_ailite_sound_mode_options_come_from_live_supported_modes():