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.
This commit is contained in:
Marc Billow
@@ -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"
|
||||
}
|
||||
|
||||
@@ -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'),
|
||||
|
||||
+5
@@ -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"
|
||||
]
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
]
|
||||
}
|
||||
|
||||
@@ -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():
|
||||
|
||||
Reference in New Issue
Block a user