fix: address code review feedback on issue-triage batch PR (#181, #183, #189, #196, #207, #208, #210)
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.
This commit is contained in:
Marc Billow
@@ -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
|
||||
|
||||
@@ -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]+')
|
||||
|
||||
@@ -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
|
||||
) + (
|
||||
|
||||
@@ -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'),
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
@@ -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"
|
||||
},
|
||||
|
||||
@@ -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"
|
||||
},
|
||||
|
||||
+1
-1
@@ -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",
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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():
|
||||
|
||||
Reference in New Issue
Block a user