From bbf3f3f8336f3b7a718719de58109082756bc194 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Tue, 28 Jul 2026 19:07:59 +0000 Subject: [PATCH] Revert unsound air-quality gating from #170, cover remaining fan-speed icons An Opus review of merged PR #170 found the cleanLevel-scalar existence gate on AIR_QUALITY doesn't hold up as a general rule: three fixtures in this repo (air_purifier_device.json, air_purifier_vtww_device.json, range_hood_device.json) carry genuinely populated Dust/FineDust/ SuperFineDust readings with no such scalar, so requiring it risks silently dropping real air-quality readings on AC hardware this repo hasn't seen yet. Reverted _has_sensor_type to item-type presence only (as before #170) and moved the #166 fix to enabled_default=False on all five entities instead -- same conservative, non-existence-gated treatment already used for tropical_night_mode and the fridge/cooktop precedents it was modeled on. Golden fixtures and tests updated to match; the five sensors are bound-but-disabled on windfree/#17-style boards again rather than unbound. Also added icons for the AC fan_mode values #170 missed -- the raw numeric labels ("1".."5") that TP1X_DA-AC-RAC-01001 and the window-AC board report instead of turbo/max -- and swapped the whole fan-speed icon family to mdi:fan-speed-1/2/3 for a more purpose-built look than the generic speedometer, applied consistently to both the AC climate card and the air purifier fan. Fixed motiondirect/motionindirect to match core's smartthings integration's arrow pairing (previously inverted). Known limitation, not fixed here: enabled_default only affects newly registered entities. Anyone who already has tropical_night_mode or the five air-quality sensors enabled from #164 (a narrow window before this fix, but real) won't see them auto-disable -- they'd need to disable them by hand in Settings > Devices > Entities. A real fix needs a one-time entity-registry migration, which this integration has no existing infrastructure or test coverage for; scoping that felt like its own follow-up rather than something to bolt on here. --- custom_components/localthings/icons.json | 17 ++-- .../registry/capabilities/airconditioner.py | 64 ++++++++------- tests/fixtures/golden/airconditioner.json | 5 ++ .../golden/airconditioner_windfree.json | 5 ++ tests/test_airconditioner_capabilities.py | 78 ++++++++----------- 5 files changed, 84 insertions(+), 85 deletions(-) diff --git a/custom_components/localthings/icons.json b/custom_components/localthings/icons.json index 47061c6..df7f537 100644 --- a/custom_components/localthings/icons.json +++ b/custom_components/localthings/icons.json @@ -5,8 +5,13 @@ "state_attributes": { "fan_mode": { "state": { - "turbo": "mdi:speedometer", - "max": "mdi:speedometer" + "1": "mdi:fan-speed-1", + "2": "mdi:fan-speed-1", + "3": "mdi:fan-speed-2", + "4": "mdi:fan-speed-3", + "5": "mdi:fan-speed-3", + "turbo": "mdi:fan-speed-3", + "max": "mdi:fan-speed-3" } }, "preset_mode": { @@ -18,8 +23,8 @@ "nano": "mdi:weather-dust", "nanosleep": "mdi:sleep", "longwind": "mdi:weather-windy", - "motiondirect": "mdi:account-arrow-right", - "motionindirect": "mdi:account-arrow-left", + "motiondirect": "mdi:account-arrow-left", + "motionindirect": "mdi:account-arrow-right", "drycomfort": "mdi:water-percent", "2step": "mdi:stairs" } @@ -33,8 +38,8 @@ "preset_mode": { "state": { "smart": "mdi:brain", - "max": "mdi:speedometer", - "mid": "mdi:speedometer-medium", + "max": "mdi:fan-speed-3", + "mid": "mdi:fan-speed-2", "windfree": "mdi:weather-dust", "sleep": "mdi:sleep" } diff --git a/custom_components/localthings/registry/capabilities/airconditioner.py b/custom_components/localthings/registry/capabilities/airconditioner.py index ac159d8..c9c8761 100644 --- a/custom_components/localthings/registry/capabilities/airconditioner.py +++ b/custom_components/localthings/registry/capabilities/airconditioner.py @@ -128,27 +128,25 @@ def _sensor_item_value(items, type_): def _has_sensor_type(type_): - """Item-type presence AND a corroborating top-level - x.com.samsung.da.cleanLevel scalar on the same /sensors/vs/0 rep. + """True when the /sensors/vs/0 items[] array lists an item of this type. - Item-type presence alone isn't a real capability signal: issue #166 - (ARxxTXFCAWKNEU, board ARTIK051_PRAC_20K) lists all five item types with - permanent zero values on both its units, the same shape as this repo's - other ARTIK051_PRAC_20K dumps (the original issue #17 dump and the - windfree fixture -- verified against the *same* board revision, per its - /information/vs/0) -- yet the #166 reporter confirmed none of these - sensors are physically present. The top-level cleanLevel scalar (separate - from the CleanLevel item inside items[]) is only ever present alongside - genuinely populated readings in every dump on record: present on - tp1x_da_ac_rac_01011 (real AC, clean_level=1) and the tp1x_da_ac_air air - purifier fixture (all five types real/nonzero), absent on every - ARTIK051_PRAC_20K dump (all zero, including both #166 units). A small - sample, but a consistent one and the only signal found that actually - explains the #166 report -- gate on it rather than leaving the always- - present item type to imply a capability that may not exist.""" + This proves the type is *listed*, not that the reading is real: issue + #166 (ARxxTXFCAWKNEU, board ARTIK051_PRAC_20K) lists all five item types + with permanent zero values on both its units, and the reporter confirmed + none of these sensors are physically present. A top-level + x.com.samsung.da.cleanLevel scalar (separate from the CleanLevel item) + looked like a corroborating "this reading is real" signal at first -- + present alongside genuinely populated readings on tp1x_da_ac_rac_01011 + and the tp1x_da_ac_air air purifier fixture, absent on every all-zero + ARTIK051_PRAC_20K dump including both #166 units -- but it doesn't hold + up as a general rule: air_purifier_device.json (ARTIK051_TVTL_18K), + air_purifier_vtww_device.json, and range_hood_device.json all carry + genuinely populated, non-AC-family Dust/FineDust/SuperFineDust readings + with no such scalar. So this stays item-type presence only -- the + entities are disabled by default instead (see AIR_QUALITY below) rather + than existence-gated on a signal that would silently drop real readings + on hardware this repo hasn't seen yet.""" def fn(rep, resources): - if 'x.com.samsung.da.cleanLevel' not in rep: - return False return any(isinstance(i, dict) and i.get('x.com.samsung.da.type') == type_ for i in (rep.get('x.com.samsung.da.items') or [])) return fn @@ -737,20 +735,18 @@ HUMIDITY = Capability( # diagnostics (see _sensor_item_value for the 2-element ambiguity and why only # v[0] is taken). No unit is advertised on the resource, so no device_class. # -# _has_sensor_type requires that same top-level cleanLevel scalar, not just -# item-type presence: issue #166 (ARxxTXFCAWKNEU, board ARTIK051_PRAC_20K) -# reports all five item types on both its units, values permanently -# '0'/['0','0'] -- the exact same shape as the issue #17 dump AIR_QUALITY was -# first verified against and the WindFree fixture (see -# test_air_quality_sensors_from_sensors_vs_items) -- both the *same board -# revision* per /information/vs/0, so that "verification" never actually -# proved a real sensor either. Item-type presence alone is Samsung's OCF -# scaffolding listing every known sensor type regardless of physical -# capability, not a capability signal. The top-level scalar is: it's present, -# with genuinely populated readings, on every dump with a confirmed-real -# sensor (tp1x_da_ac_rac_01011, and the tp1x_da_ac_air air purifier fixture), -# and absent on every all-zero ARTIK051_PRAC_20K dump on record, including -# both #166 units. Gate on it. +# exists_fn (_has_sensor_type) only proves the item *type* is listed, not +# that the unit actually carries that sensor: issue #166 (ARxxTXFCAWKNEU, +# board ARTIK051_PRAC_20K) reports all five item types on both its units, +# values permanently '0'/['0','0'] -- yet the reporter confirmed none apply +# to their model. A tighter existence gate was tried (requiring the +# corroborating cleanLevel scalar above) but doesn't hold up as a general +# rule -- see _has_sensor_type's docstring -- and risks silently dropping +# real readings on hardware that reports them without that scalar. So these +# stay bound whenever the type is listed, same as before #166, and disabled +# by default instead (same precedent as fridge.rack_count / +# cooktop.paired_hood_model / this file's own tropical_night_mode): units +# that do have the sensor can enable it themselves. AIR_QUALITY = Capability( href='/sensors/vs/0', poll_tier='cold', @@ -759,11 +755,13 @@ AIR_QUALITY = Capability( icon='mdi:broom', entity_category='diagnostic', state_class='measurement', exists_fn=_has_sensor_type('CleanLevel'), + enabled_default=False, value_fn=lambda items: _int(_sensor_item_value(items, 'CleanLevel'))), *tuple( SensorDesc(key=key, field='x.com.samsung.da.items', icon=icon, entity_category='diagnostic', exists_fn=_has_sensor_type(type_), + enabled_default=False, value_fn=lambda items, t=type_: _sensor_item_value(items, t)) for key, icon, type_ in ( ('odor', 'mdi:weather-windy', 'Odor'), diff --git a/tests/fixtures/golden/airconditioner.json b/tests/fixtures/golden/airconditioner.json index 7d07080..ffb4c56 100644 --- a/tests/fixtures/golden/airconditioner.json +++ b/tests/fixtures/golden/airconditioner.json @@ -7,14 +7,19 @@ "alarm_code", "auto_clean", "beep", + "clean_level", "climate", "current_temperature_c", "diagnosis_status", "display_light", + "dust", "energy_kwh", "energy_saved_kwh", + "fine_dust", "humidity", + "odor", "power_watts", + "super_fine_dust", "tropical_night_mode" ] } diff --git a/tests/fixtures/golden/airconditioner_windfree.json b/tests/fixtures/golden/airconditioner_windfree.json index 1b7e382..2407a44 100644 --- a/tests/fixtures/golden/airconditioner_windfree.json +++ b/tests/fixtures/golden/airconditioner_windfree.json @@ -7,13 +7,18 @@ "alarm_code", "auto_clean", "beep", + "clean_level", "climate", "current_temperature_c", "diagnosis_status", "display_light", + "dust", "energy_kwh", + "fine_dust", "humidity", + "odor", "power_watts", + "super_fine_dust", "tropical_night_mode" ] } diff --git a/tests/test_airconditioner_capabilities.py b/tests/test_airconditioner_capabilities.py index d556f5b..6c3e46e 100644 --- a/tests/test_airconditioner_capabilities.py +++ b/tests/test_airconditioner_capabilities.py @@ -674,56 +674,42 @@ def test_air_quality_sensors_from_sensors_vs_items(): by a top-level cleanLevel scalar, so it's an int measurement; the others are string diagnostics. Dust/FineDust/SuperFineDust carry a 2-element array whose second element is unconfirmed -- v[0] is taken as the reading - (see _sensor_item_value). Exercised on tp1x_da_ac_rac_01011, the fixture - proven real by the corroborating scalar (see - test_air_quality_present_with_corroborating_clean_level_scalar) -- - windfree no longer applies here post-#166 fix, since it lacks that - scalar and binds no air-quality entities at all.""" - reg, resources = _ac_tp1x() - state = flatten( - discover(resources, reg.capabilities, reg.pattern_capabilities), resources) - assert state['clean_level'] == 1 # numeric (int), corroborated - for key in ('dust', 'fine_dust', 'super_fine_dust'): - assert state[key] == '0' # string diagnostic - - -def test_air_quality_absent_without_corroborating_clean_level_scalar(): - """Issue #166 (ARxxTXFCAWKNEU, board ARTIK051_PRAC_20K): /sensors/vs/0 - lists all five item types, values permanently '0'/['0', '0'] -- the exact - same shape as the WindFree fixture, which is the *same board revision* - (see /information/vs/0: both report modelNum - 'ARTIK051_PRAC_20K|10217841|...') that this capability was originally - verified against. The reporter confirmed none of these sensors are - physically present on their unit, which means that original - "verification" never actually proved a real sensor either -- item-type - presence is Samsung's OCF scaffolding, not a capability signal (see - _has_sensor_type). The tell that's actually reliable: a top-level - x.com.samsung.da.cleanLevel scalar (separate from the CleanLevel item), - present only alongside genuinely populated readings on every dump on - record. WindFree lacks it -- so post-fix, none of the five entities - should bind there at all, matching the #166 report.""" + (see _sensor_item_value).""" reg, resources = _ac_windfree() state = flatten( discover(resources, reg.capabilities, reg.pattern_capabilities), resources) - assert 'x.com.samsung.da.cleanLevel' not in resources['/sensors/vs/0'] + assert state['clean_level'] == 0 # numeric (int), corroborated + for key in ('odor', 'dust', 'fine_dust', 'super_fine_dust'): + assert state[key] == '0' # string diagnostic + # tp1x_da_ac_rac_01011 is the only fixture with a non-zero air-quality + # reading -- the one that catches a value_fn regression. + reg2, resources2 = _ac_tp1x() + state2 = flatten( + discover(resources2, reg2.capabilities, reg2.pattern_capabilities), resources2) + assert state2['clean_level'] == 1 + + +def test_air_quality_disabled_by_default(): + """Issue #166 (ARxxTXFCAWKNEU, board ARTIK051_PRAC_20K): /sensors/vs/0 + lists all five item types with permanent zero values on both its units -- + the exact same shape as the WindFree/issue #17 dumps this capability was + first verified against -- yet the reporter confirmed none of these + sensors are physically present on their model. + + A tighter exists_fn (requiring a corroborating top-level + x.com.samsung.da.cleanLevel scalar) was tried and reverted: it looked + like a real signal against this repo's AC fixtures, but + air_purifier_device.json, air_purifier_vtww_device.json, and + range_hood_device.json all carry genuinely populated Dust/FineDust/ + SuperFineDust readings with no such scalar, so requiring it would + silently drop real readings on hardware this repo hasn't seen yet on an + AC. These stay bound whenever the item type is listed (see + _has_sensor_type) and disabled by default instead, same precedent as + fridge.rack_count / cooktop.paired_hood_model / tropical_night_mode -- + units that do have the sensor can enable it themselves.""" for key in ('clean_level', 'odor', 'dust', 'fine_dust', 'super_fine_dust'): - assert key not in state, key - - -def test_air_quality_present_with_corroborating_clean_level_scalar(): - """tp1x_da_ac_rac_01011 carries the top-level cleanLevel scalar alongside - a genuinely populated CleanLevel reading -- the one fixture in this repo - proven real rather than placeholder, so its air-quality entities must - still bind post-fix. It has no Odor item at all (unrelated to the scalar - gate -- item-type absence, not zero-value ambiguity).""" - reg, resources = _ac_tp1x() - assert resources['/sensors/vs/0']['x.com.samsung.da.cleanLevel'] == '1' - state = flatten( - discover(resources, reg.capabilities, reg.pattern_capabilities), resources) - assert state['clean_level'] == 1 - for key in ('dust', 'fine_dust', 'super_fine_dust'): - assert key in state - assert 'odor' not in state + desc = next(e for e in airconditioner.AIR_QUALITY.entities if e.key == key) + assert desc.enabled_default is False, key def test_air_quality_absent_when_no_sensor_items():