diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index 8648d1d..97c76fb 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -46,7 +46,6 @@ from .const import ( CONF_LEAF_CERT_PEM, CONF_LEAF_KEY_PEM, CONF_LEARN_MODES, - CONF_LEARNED_MODES, CONF_MANUFACTURER, CONF_MODEL, CONF_PORT, @@ -61,7 +60,8 @@ from .const import ( PROBE_PORT_RANGE, SERVICE_WRITE_RESOURCE, ) -from .learned import LearnedModes +from .learned import persist as learned_persist +from .learned import stored as learned_stored _TEXT = TextSelector(TextSelectorConfig(type=TextSelectorType.TEXT)) _MULTILINE = TextSelector(TextSelectorConfig(type=TextSelectorType.TEXT, multiline=True)) @@ -879,13 +879,12 @@ class LocalThingsOptionsFlow(config_entries.OptionsFlow): form; the description lists what's about to be forgotten. """ coord = self._coordinator() + # learned.py owns the entry key and the persisted shape, so this + # step never parses or writes it itself -- including on an unloaded + # entry, where a malformed record would otherwise abort the one + # screen that can clear it. learned = ( - coord.learned_snapshot() - if coord is not None - # Through LearnedModes, not the raw entry value: this step is - # the one screen that can clear a malformed persisted record, - # so it must not be the one screen that trips over it. - else LearnedModes(self.config_entry.data.get(CONF_LEARNED_MODES)).snapshot() + coord.learned_snapshot() if coord is not None else learned_stored(self.config_entry) ) codes = sorted({code for codes in learned.values() for code in codes}) @@ -893,13 +892,7 @@ class LocalThingsOptionsFlow(config_entries.OptionsFlow): if coord is not None: coord.forget_learned_modes() else: - # Not loaded, so there's no store to clear -- drop the - # persisted copy directly, which is all a reload would - # restore from anyway. - self.hass.config_entries.async_update_entry( - self.config_entry, - data={**self.config_entry.data, CONF_LEARNED_MODES: {}}, - ) + learned_persist(self.hass, self.config_entry, {}) return self.async_create_entry(data=dict(self.config_entry.options)) return self.async_show_form( diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 6b3bd6d..66bc4a4 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -40,7 +40,7 @@ from .const import ( DTLS_LOCAL_PORT_BASE, SUMMARY_INTERVAL_S, ) -from .learned import LearnedModes +from .learned import LEARNABLE, LearnedModes, persist from .observe import GRACE_PERIOD_S, MODE_OBSERVE, MODE_POLL, ObserveManager from .registry import CAPABILITIES from .registry.adapter import flatten @@ -53,6 +53,7 @@ from .registry.capabilities.common import ( remote_control_required_for_write, ) from .registry.discovery import BoundEntity +from .registry.entities import ClimateDesc from .registry.identity import ( DeviceIdentity, device_display_name, @@ -247,6 +248,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # supported (issue #327), restored from the entry so one learned # last week is still offered today. See learned.py. self._learned = LearnedModes(entry.data.get(CONF_LEARNED_MODES)) + # Narrowed to this device's own climate hrefs once discovery has + # run -- see _refresh_learnable_hrefs. + self._learnable_hrefs: set[str] = set() self._observe.set_on_applied(self._on_rep_applied) self._push_pending = False self._push_pending_lock = threading.Lock() @@ -349,13 +353,23 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): if self._entry.data.get(CONF_LEARNED_MODES): self._persist_learned() - def _canonical_href(self, actual: str) -> str: - """`actual` in the namespace the registry -- and so learned.LEARNABLE - -- is written against; identity for MAIN's own hrefs (issue #177).""" - for subdevice in self.subdevices: - if subdevice.owns(actual): - return subdevice.to_canonical(actual) or actual - return actual + def _refresh_learnable_hrefs(self) -> None: + """The actual hrefs learning applies to on this device: LEARNABLE's + canonical set, narrowed to the ones a climate entity is bound to + read back (climate._supported is the only consumer) and translated + through that entity's own subdevice (issue #177). + + The href alone isn't a sufficient key here. `/mode/convenient/vs/0` + is also declared by the dehumidifier registry (explicitly + unmodeled) and the air purifier's (empty), so a global match would + persist a code for a resource that family will never offer. + """ + self._learnable_hrefs = { + bound.subdevice.to_actual(href) + for bound in self.bound + if isinstance(bound.desc, ClimateDesc) + for href in LEARNABLE + } def _on_rep_applied(self, href: str, rep: dict, source: str) -> None: """ObserveManager.set_on_applied hook. Runs on whichever thread @@ -364,23 +378,22 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): An 'optimistic' rep is the value this integration just wrote, not one the device reported, so there is nothing to learn from it.""" - if source == "optimistic" or not self.learning_enabled: + if source == "optimistic" or href not in self._learnable_hrefs: return - if self._learned.observe(self._canonical_href(href), href, rep): + if not self.learning_enabled: + return + if new := self._learned.observe(href, rep): self._log.info( "%s reported mode(s) it does not advertise as supported; " "remembering %s so they stay selectable (issue #327)", href, - self._learned.codes(href), + new, ) self.hass.add_job(self._persist_learned) @callback def _persist_learned(self) -> None: - self.hass.config_entries.async_update_entry( - self._entry, - data={**self._entry.data, CONF_LEARNED_MODES: self._learned.snapshot()}, - ) + persist(self.hass, self._entry, self._learned.snapshot()) def device_info_for(self, subdevice: Subdevice) -> DeviceInfo: """DeviceInfo for one logical subdevice on this connection (issue @@ -802,6 +815,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self.device_type_name = device_type_name self.bound = bound self._unbound_hrefs = unbound + self._refresh_learnable_hrefs() # The entry's stored identity wins; this poll's answer is only # adopted when nothing is stored (a legacy migration couldn't diff --git a/custom_components/localthings/learned.py b/custom_components/localthings/learned.py index a248dd3..b9a84b7 100644 --- a/custom_components/localthings/learned.py +++ b/custom_components/localthings/learned.py @@ -11,47 +11,37 @@ Learning is deliberately not global. A current value that isn't a selectable option is common across this corpus -- an oven idling in 'NoOperation', a fridge's /mode/vs/0 carrying capability tokens like 'WATERFILTER_DISABLE' -- and remembering one of those permanently would -put an option in the UI that the device can only reject. LEARNABLE is the -allowlist of canonical hrefs where a device-reported current mode is known -to be a genuine selectable option; every consumer of it (climate's -_supported today) must read its supported list through the coordinator. +put an option in the UI that the device can only reject. LEARNABLE names +the canonical hrefs where a reported mode is known to be genuinely +selectable, and the coordinator narrows it further to the hrefs this +device actually binds a climate entity to (see _refresh_learnable_hrefs): +the same href is declared explicitly unmodeled on a dehumidifier and +empty on an air purifier, and learning for those would persist a code +nothing ever offers. + +This module also owns the entry key the store persists under, so the +shape lives in exactly one place. """ from __future__ import annotations import threading -from dataclasses import dataclass +from .const import CONF_LEARNED_MODES from .registry.capabilities.airconditioner import HREF_CONVENIENT MODES_FIELD = "x.com.samsung.da.modes" SUPPORTED_FIELD = "x.com.samsung.da.supportedModes" - -@dataclass(frozen=True) -class LearnRule: - """Which field of a rep names the current mode, and which lists the - supported ones. Both are per-href because Samsung spells them - differently across resources (`supportedModes` on the vendor `/vs/` - ones, bare `modes`/`supportedModes` on a few OCF-shaped ones).""" - - current_field: str - supported_field: str - - -LEARNABLE: dict[str, LearnRule] = { - # Convenient (preset) mode. Reported by two independent reporters on - # ARTIK051_PRAC_20K, one of whom has three identical units where only - # the two sharing an outdoor unit hide Quiet -- so this is a firmware - # reporting gap, not a real capability difference. - HREF_CONVENIENT: LearnRule(MODES_FIELD, SUPPORTED_FIELD), -} +# Convenient (preset) mode: firmware omits an active preset (e.g. Quiet) +# from its own supportedModes -- a reporting gap, not a capability +# difference (issue #327). +LEARNABLE: frozenset[str] = frozenset({HREF_CONVENIENT}) def _codes(value) -> list[str]: - """The mode codes in a `modes`-style field, which is an array on every - board seen but a bare string on none -- tolerated anyway, since this - runs against whatever the device sends.""" + """Mode codes from a `modes`-style field, which some firmwares send as + a bare string rather than an array.""" if isinstance(value, str): return [value] if isinstance(value, (list, tuple)): @@ -73,11 +63,21 @@ def _coerce(stored) -> dict[str, list[str]]: return restored +def stored(entry) -> dict[str, list[str]]: + """What `entry` has persisted, coerced -- for a reader that can't go + through a coordinator (the options flow, on an unloaded entry).""" + return _coerce(entry.data.get(CONF_LEARNED_MODES)) + + +def persist(hass, entry, codes: dict[str, list[str]]) -> None: + """Write `codes` onto the entry. Runs on the event loop, which + async_update_entry requires.""" + hass.config_entries.async_update_entry(entry, data={**entry.data, CONF_LEARNED_MODES: codes}) + + class LearnedModes: """Per-device store of learned codes, keyed by actual (on-the-wire) href so two subdevices of one composite appliance learn separately. - One href carries one LEARNABLE rule, so the rule's fields are how a rep - is read, never part of the key. Mutated from whichever thread applied the update (the DTLS reader for an OBSERVE notify, an executor thread for a poll -- see @@ -89,9 +89,9 @@ class LearnedModes: self._lock = threading.Lock() self._learned = _coerce(stored) - def observe(self, canonical_href: str, actual_href: str, rep: dict) -> bool: - """Learn from one applied rep; True when something new was learned - (i.e. the caller should persist). + def observe(self, actual_href: str, rep: dict) -> list[str]: + """Learn from one applied rep; returns the codes newly learned, so + an empty list means there is nothing to persist. A rep that carries no supported list teaches nothing: "missing from the list" is only meaningful against a list that exists, and @@ -101,23 +101,19 @@ class LearnedModes: alone (issue #27) still sees the supported list from the last full poll. """ - rule = LEARNABLE.get(canonical_href) - if rule is None: - return False - supported = _codes(rep.get(rule.supported_field)) + supported = _codes(rep.get(SUPPORTED_FIELD)) if not supported: - return False + return [] with self._lock: known = self._learned.get(actual_href, []) new = [ code - for code in _codes(rep.get(rule.current_field)) + for code in _codes(rep.get(MODES_FIELD)) if code and code not in supported and code not in known ] - if not new: - return False - self._learned[actual_href] = [*known, *new] - return True + if new: + self._learned[actual_href] = [*known, *new] + return new def codes(self, actual_href: str) -> list[str]: with self._lock: diff --git a/custom_components/localthings/observe.py b/custom_components/localthings/observe.py index a744d5b..0c96a4b 100644 --- a/custom_components/localthings/observe.py +++ b/custom_components/localthings/observe.py @@ -78,22 +78,18 @@ class ObserveManager: # have notified. Guards only `_notified` mutations + the `wait_for`. self._notify_cond = threading.Condition() self.fallback_hrefs: set[str] = set() - # Called with (href, merged_rep, source) after every accepted - # device update, on the applying thread -- see set_on_applied. self._on_applied: Callable[[str, dict, str], None] | None = None self._refresh_task: ObserveRefreshTask | None = None self._refresh_stop: threading.Event | None = None self._refresh_thread: threading.Thread | None = None def set_on_applied(self, callback: Callable[[str, dict, str], None]) -> None: - """Register a hook run after every rep this manager accepts, with - the merged rep that reached the cache. + """Hook run after every accepted rep, on the applying thread. - Unlike StateCache.set_on_change, which reports only that - *something* changed, this hands over the href and rep -- and fires - even when the rep matched what was already cached, which the - learned-modes store (learned.py) depends on: a device sitting in - an unadvertised mode sends an unchanged rep every poll. + Unlike StateCache.set_on_change it carries the href and rep, and + fires even when the rep is unchanged -- which learned.py needs, a + device sitting in an unadvertised mode re-sending the same rep + every poll. """ self._on_applied = callback diff --git a/tests/test_airconditioner_artik051_krac.py b/tests/test_airconditioner_artik051_krac.py index 022637b..7ab0154 100644 --- a/tests/test_airconditioner_artik051_krac.py +++ b/tests/test_airconditioner_artik051_krac.py @@ -65,8 +65,6 @@ class _FakeCoordinator: self.commands.append((bound, payload)) def learned_modes(self, href): - # Nothing learned in this stub -- issue #327's store lives on the - # real coordinator; climate._supported unions it in. return [] diff --git a/tests/test_airconditioner_tp1x_rac_01001_fan.py b/tests/test_airconditioner_tp1x_rac_01001_fan.py index 274b9d4..9d53da6 100644 --- a/tests/test_airconditioner_tp1x_rac_01001_fan.py +++ b/tests/test_airconditioner_tp1x_rac_01001_fan.py @@ -49,8 +49,6 @@ class _FakeCoordinator: self.commands.append((bound, payload)) def learned_modes(self, href): - # Nothing learned in this stub -- issue #327's store lives on the - # real coordinator; climate._supported unions it in. return [] diff --git a/tests/test_climate_ac_modes.py b/tests/test_climate_ac_modes.py index da4aace..49c29ef 100644 --- a/tests/test_climate_ac_modes.py +++ b/tests/test_climate_ac_modes.py @@ -120,8 +120,6 @@ def test_fac_bora_wind_strength_codes_fit_the_standard_scale(): return self.last_resources def learned_modes(self, href): - # Nothing learned in this stub -- issue #327's store lives on - # the real coordinator; climate._supported unions it in. return [] resources = _load_device("airconditioner_fac_bora") diff --git a/tests/test_learned_modes.py b/tests/test_learned_modes.py index e52eed6..a0e8136 100644 --- a/tests/test_learned_modes.py +++ b/tests/test_learned_modes.py @@ -31,7 +31,7 @@ from custom_components.localthings.learned import ( LearnedModes, ) from custom_components.localthings.registry.entities import ClimateDesc -from tests.test_subdevice_discovery import ENTRY_DATA, _discover +from tests.test_subdevice_discovery import ENTRY_DATA, _climate_bound, _discover FIXTURE = "airconditioner_tp1x_fac_time_23k" CONVENIENT = "/mode/convenient/vs/0" @@ -50,7 +50,7 @@ QUIET_REP = {MODES_FIELD: "Quiet", SUPPORTED_FIELD: ADVERTISED} def test_learns_a_current_mode_missing_from_the_supported_list(): learned = LearnedModes() - assert learned.observe(CONVENIENT, CONVENIENT, QUIET_REP) is True + assert learned.observe(CONVENIENT, QUIET_REP) == ["Quiet"] assert learned.codes(CONVENIENT) == ["Quiet"] @@ -58,15 +58,15 @@ def test_relearning_the_same_mode_is_not_a_change(): """The device reports the same rep on every poll while it sits in the mode, so only the first one may report back as something to persist.""" learned = LearnedModes() - assert learned.observe(CONVENIENT, CONVENIENT, QUIET_REP) is True - assert learned.observe(CONVENIENT, CONVENIENT, QUIET_REP) is False + assert learned.observe(CONVENIENT, QUIET_REP) == ["Quiet"] + assert learned.observe(CONVENIENT, QUIET_REP) == [] assert learned.codes(CONVENIENT) == ["Quiet"] def test_an_advertised_mode_is_never_learned(): learned = LearnedModes() rep = {MODES_FIELD: "Sleep", SUPPORTED_FIELD: ADVERTISED} - assert learned.observe(CONVENIENT, CONVENIENT, rep) is False + assert learned.observe(CONVENIENT, rep) == [] assert learned.codes(CONVENIENT) == [] @@ -75,26 +75,23 @@ def test_a_rep_with_no_supported_list_teaches_nothing(): exists -- a board publishing none would otherwise get an option list invented out of whatever it happened to be doing.""" learned = LearnedModes() - assert learned.observe(CONVENIENT, CONVENIENT, {MODES_FIELD: "Quiet"}) is False + assert learned.observe(CONVENIENT, {MODES_FIELD: "Quiet"}) == [] assert learned.codes(CONVENIENT) == [] -def test_an_href_outside_the_allowlist_learns_nothing(): +def test_the_allowlist_is_only_the_convenient_href(): """The guard that keeps this feature off resources whose current value - isn't a selectable option: an oven idling in 'NoOperation' reports - exactly this shape on /mode/vs/0, and remembering it would put a - permanent junk option in that unit's cook-mode select.""" - learned = LearnedModes() - rep = {MODES_FIELD: "NoOperation", SUPPORTED_FIELD: ["Bake", "Broil"]} - assert learned.observe("/mode/vs/0", "/mode/vs/0", rep) is False - assert learned.snapshot() == {} + isn't a selectable option -- an oven idling in 'NoOperation' on + /mode/vs/0, whose codes would become permanent junk options in that + unit's cook-mode select.""" + assert set(LEARNABLE) == {CONVENIENT} def test_two_subdevices_learn_separately(): """Keyed by the actual on-the-wire href (issue #177), so a composite appliance's second indoor unit doesn't inherit the first's gap.""" learned = LearnedModes() - learned.observe(CONVENIENT, "/mode/convenient/vs/1", QUIET_REP) + learned.observe("/mode/convenient/vs/1", QUIET_REP) assert learned.codes("/mode/convenient/vs/1") == ["Quiet"] assert learned.codes(CONVENIENT) == [] @@ -130,7 +127,9 @@ def test_clear_forgets_everything(): def test_every_learnable_href_is_one_climate_resolves(): """climate._supported is the only consumer that unions learned codes in today, so an href added to LEARNABLE that climate never reads would - be learned, persisted, and never offered anywhere.""" + be learned, persisted, and never offered anywhere. The coordinator + enforces the device half of this (only hrefs a climate entity binds + are learnable); this is the static half.""" from custom_components.localthings.climate import ( CONVENIENT_HREF, MODE_HREF, @@ -171,8 +170,7 @@ async def _flush(hass: HomeAssistant) -> None: async def _climate(hass: HomeAssistant, entry) -> tuple[LocalThingsCoordinator, Any]: coordinator = LocalThingsCoordinator(hass, entry) await _discover(coordinator, FIXTURE) - bound = next(b for b in coordinator.bound if isinstance(b.desc, ClimateDesc)) - return coordinator, LocalThingsClimate(coordinator, bound) + return coordinator, LocalThingsClimate(coordinator, _climate_bound(coordinator, None)) async def test_the_fixture_really_does_not_advertise_quiet(hass: HomeAssistant): @@ -231,6 +229,23 @@ async def test_a_learned_preset_survives_a_restart(hass: HomeAssistant): assert "quiet" in entity.preset_modes +async def test_a_device_with_no_climate_entity_learns_nothing(hass: HomeAssistant): + """The href alone isn't a sufficient key: the dehumidifier registry + declares this same /mode/convenient/vs/0 explicitly unmodeled (no live + current-value field), so a code learned there would be persisted for a + resource nothing will ever offer.""" + entry = _entry(hass) + coordinator = LocalThingsCoordinator(hass, entry) + await _discover(coordinator, "dehumidifier") + assert not any(isinstance(b.desc, ClimateDesc) for b in coordinator.bound) + + coordinator._observe.apply( + CONVENIENT, {MODES_FIELD: "Quiet", SUPPORTED_FIELD: ADVERTISED}, source="poll" + ) + + assert coordinator.learned_snapshot() == {} + + async def test_an_optimistic_write_teaches_nothing(hass: HomeAssistant): """An optimistic cache entry is the value this integration just wrote, not something the device reported."""