From 4f3bdde6e5ba97ff3242aee7fb40078eb43ad66a Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Sat, 8 Aug 2026 19:42:20 +0000 Subject: [PATCH] Address review findings on the learned-modes store Flatten the store to {href: [codes]}. One href carries one LEARNABLE rule, so keying the codes by the rule's supported field too let the write side (rule.supported_field) and both read sides (the module-level SUPPORTED_FIELD) disagree the moment a rule used a different field -- codes learned and persisted, then never offered. The options flow's reset step read the persisted value raw in the entry-not-loaded branch, so malformed data aborted the one screen that can clear it; route it through LearnedModes like every other reader. For the same reason forget_learned_modes() now persists whenever the entry carries a record, not only when the in-memory store had one: a record _coerce rejected at startup exists only on the entry. --- custom_components/localthings/config_flow.py | 10 ++-- custom_components/localthings/const.py | 2 +- custom_components/localthings/coordinator.py | 17 ++++--- custom_components/localthings/learned.py | 40 ++++++---------- tests/localthings/test_config_flow.py | 16 +++++-- tests/test_airconditioner_artik051_krac.py | 2 +- .../test_airconditioner_tp1x_rac_01001_fan.py | 2 +- tests/test_climate_ac_modes.py | 2 +- tests/test_learned_modes.py | 47 ++++++++++++------- 9 files changed, 78 insertions(+), 60 deletions(-) diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index c5f52be..8648d1d 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -61,6 +61,7 @@ from .const import ( PROBE_PORT_RANGE, SERVICE_WRITE_RESOURCE, ) +from .learned import LearnedModes _TEXT = TextSelector(TextSelectorConfig(type=TextSelectorType.TEXT)) _MULTILINE = TextSelector(TextSelectorConfig(type=TextSelectorType.TEXT, multiline=True)) @@ -881,11 +882,12 @@ class LocalThingsOptionsFlow(config_entries.OptionsFlow): learned = ( coord.learned_snapshot() if coord is not None - else self.config_entry.data.get(CONF_LEARNED_MODES) or {} - ) - codes = sorted( - {code for fields in learned.values() for value in fields.values() for code in value} + # 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() ) + codes = sorted({code for codes in learned.values() for code in codes}) if user_input is not None: if coord is not None: diff --git a/custom_components/localthings/const.py b/custom_components/localthings/const.py index 8784455..3e62fcc 100644 --- a/custom_components/localthings/const.py +++ b/custom_components/localthings/const.py @@ -38,7 +38,7 @@ CONF_DEVICE_TYPE = "device_type" # in the same resource's supportedModes (issue #327). Stored on the entry # rather than kept in memory so a mode the device only names while it is # active survives a restart -- see learned.py. Shape: -# {actual_href: {supported_field: [code, ...]}}. +# {actual_href: [code, ...]}. CONF_LEARNED_MODES = "learned_modes" # Options-flow key: whether learned modes are remembered and offered. diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 35a8c99..6b3bd6d 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 SUPPORTED_FIELD, LearnedModes +from .learned import LearnedModes from .observe import GRACE_PERIOD_S, MODE_OBSERVE, MODE_POLL, ObserveManager from .registry import CAPABILITIES from .registry.adapter import flatten @@ -322,7 +322,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): def learning_enabled(self) -> bool: return bool(self._entry.options.get(CONF_LEARN_MODES, DEFAULT_LEARN_MODES)) - def learned_modes(self, actual_href: str, field: str = SUPPORTED_FIELD) -> list[str]: + def learned_modes(self, actual_href: str) -> list[str]: """Codes learned for `actual_href`, or [] while the option is off. Gating the read here rather than only the write is what makes the @@ -331,17 +331,22 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): (the options flow's reset step is for that).""" if not self.learning_enabled: return [] - return self._learned.codes(actual_href, field) + return self._learned.codes(actual_href) - def learned_snapshot(self) -> dict[str, dict[str, list[str]]]: + def learned_snapshot(self) -> dict[str, list[str]]: """Everything learned, option state ignored -- for diagnostics and the options flow, both of which need to show what is remembered even when it isn't currently being offered.""" return self._learned.snapshot() def forget_learned_modes(self) -> None: - """Drop every learned mode, here and on the entry.""" - if self._learned.clear(): + """Drop every learned mode, here and on the entry. + + Persists even when the in-memory store was already empty: a record + _coerce rejected at startup exists only on the entry, and this is + the one control that can clear it.""" + self._learned.clear() + if self._entry.data.get(CONF_LEARNED_MODES): self._persist_learned() def _canonical_href(self, actual: str) -> str: diff --git a/custom_components/localthings/learned.py b/custom_components/localthings/learned.py index d613645..a248dd3 100644 --- a/custom_components/localthings/learned.py +++ b/custom_components/localthings/learned.py @@ -59,28 +59,25 @@ def _codes(value) -> list[str]: return [] -def _coerce(stored) -> dict[str, dict[str, list[str]]]: +def _coerce(stored) -> dict[str, list[str]]: """Restore the persisted map, dropping anything that isn't the shape this module writes. It round-trips through the config entry as plain JSON, and a hand-edited .storage file shouldn't be able to crash setup.""" - restored: dict[str, dict[str, list[str]]] = {} if not isinstance(stored, dict): - return restored - for href, fields in stored.items(): - if not isinstance(href, str) or not isinstance(fields, dict): - continue - for field, codes in fields.items(): - if not isinstance(field, str): - continue - if valid := [c for c in _codes(codes) if c]: - restored.setdefault(href, {})[field] = valid + return {} + restored = {} + for href, codes in stored.items(): + if isinstance(href, str) and (valid := [c for c in _codes(codes) if c]): + restored[href] = valid return restored 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 @@ -111,7 +108,7 @@ class LearnedModes: if not supported: return False with self._lock: - known = self._learned.get(actual_href, {}).get(rule.supported_field, []) + known = self._learned.get(actual_href, []) new = [ code for code in _codes(rep.get(rule.current_field)) @@ -119,24 +116,17 @@ class LearnedModes: ] if not new: return False - self._learned.setdefault(actual_href, {})[rule.supported_field] = [*known, *new] + self._learned[actual_href] = [*known, *new] return True - def codes(self, actual_href: str, field: str = SUPPORTED_FIELD) -> list[str]: + def codes(self, actual_href: str) -> list[str]: with self._lock: - return list(self._learned.get(actual_href, {}).get(field, ())) + return list(self._learned.get(actual_href, ())) - def snapshot(self) -> dict[str, dict[str, list[str]]]: + def snapshot(self) -> dict[str, list[str]]: with self._lock: - return { - href: {f: list(c) for f, c in fields.items()} - for href, fields in self._learned.items() - } + return {href: list(codes) for href, codes in self._learned.items()} - def clear(self) -> bool: - """Forget everything; True when there was something to forget.""" + def clear(self) -> None: with self._lock: - if not self._learned: - return False self._learned = {} - return True diff --git a/tests/localthings/test_config_flow.py b/tests/localthings/test_config_flow.py index dbc4434..a3d8e85 100644 --- a/tests/localthings/test_config_flow.py +++ b/tests/localthings/test_config_flow.py @@ -1013,12 +1013,22 @@ async def test_learned_modes_option_can_be_turned_off(hass: HomeAssistant) -> No assert entry.options[CONF_LEARN_MODES] is False -async def test_forget_learned_modes_clears_the_entry(hass: HomeAssistant) -> None: +@pytest.mark.parametrize( + ("stored", "listed"), + [ + ({"/mode/convenient/vs/0": ["Quiet"]}, "Quiet"), + # Malformed -- nothing writes this shape, but a hand-edited + # .storage can hold it, and this step is the one screen that can + # clear it, so it must not be the one screen that trips over it. + ({"/mode/convenient/vs/0": None}, "(none)"), + ], +) +async def test_forget_learned_modes_clears_the_entry(hass: HomeAssistant, stored, listed) -> None: """The reset step works on an unloaded entry too, by dropping the persisted copy directly -- that's all a reload would restore from.""" entry = MockConfigEntry( domain=DOMAIN, - data={**ENTRY_DATA, CONF_LEARNED_MODES: {"/mode/convenient/vs/0": {"f": ["Quiet"]}}}, + data={**ENTRY_DATA, CONF_LEARNED_MODES: stored}, unique_id=f"localthings_{MOCK_SERIAL}", ) entry.add_to_hass(hass) @@ -1028,7 +1038,7 @@ async def test_forget_learned_modes_clears_the_entry(hass: HomeAssistant) -> Non result["flow_id"], user_input={"next_step_id": "forget_learned_modes"} ) assert result["type"] == FlowResultType.FORM - assert result["description_placeholders"] == {"codes": "Quiet"} + assert result["description_placeholders"] == {"codes": listed} result = await hass.config_entries.options.async_configure(result["flow_id"], user_input={}) diff --git a/tests/test_airconditioner_artik051_krac.py b/tests/test_airconditioner_artik051_krac.py index f318772..022637b 100644 --- a/tests/test_airconditioner_artik051_krac.py +++ b/tests/test_airconditioner_artik051_krac.py @@ -64,7 +64,7 @@ class _FakeCoordinator: async def async_send_command(self, bound, payload): self.commands.append((bound, payload)) - def learned_modes(self, href, field=None): + 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 f3088cf..274b9d4 100644 --- a/tests/test_airconditioner_tp1x_rac_01001_fan.py +++ b/tests/test_airconditioner_tp1x_rac_01001_fan.py @@ -48,7 +48,7 @@ class _FakeCoordinator: async def async_send_command(self, bound, payload): self.commands.append((bound, payload)) - def learned_modes(self, href, field=None): + 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 5f8955d..da4aace 100644 --- a/tests/test_climate_ac_modes.py +++ b/tests/test_climate_ac_modes.py @@ -119,7 +119,7 @@ def test_fac_bora_wind_strength_codes_fit_the_standard_scale(): # canonical view is just the raw snapshot (issue #177). return self.last_resources - def learned_modes(self, href, field=None): + 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_learned_modes.py b/tests/test_learned_modes.py index 54e52bb..e52eed6 100644 --- a/tests/test_learned_modes.py +++ b/tests/test_learned_modes.py @@ -104,9 +104,10 @@ def test_two_subdevices_learn_separately(): [ None, "not-a-dict", - {"/mode/convenient/vs/0": "not-a-dict"}, - {"/mode/convenient/vs/0": {SUPPORTED_FIELD: [""]}}, - {"/mode/convenient/vs/0": {SUPPORTED_FIELD: [1, 2]}}, + {"/mode/convenient/vs/0": None}, + {"/mode/convenient/vs/0": [""]}, + {"/mode/convenient/vs/0": [1, 2]}, + {"/mode/convenient/vs/0": {"x.com.samsung.da.supportedModes": ["Quiet"]}}, ], ) def test_malformed_stored_data_restores_as_empty(stored): @@ -116,15 +117,14 @@ def test_malformed_stored_data_restores_as_empty(stored): def test_well_formed_stored_data_restores(): - learned = LearnedModes({CONVENIENT: {SUPPORTED_FIELD: ["Quiet"]}}) + learned = LearnedModes({CONVENIENT: ["Quiet"]}) assert learned.codes(CONVENIENT) == ["Quiet"] -def test_clear_reports_whether_there_was_anything_to_forget(): - learned = LearnedModes({CONVENIENT: {SUPPORTED_FIELD: ["Quiet"]}}) - assert learned.clear() is True +def test_clear_forgets_everything(): + learned = LearnedModes({CONVENIENT: ["Quiet"]}) + learned.clear() assert learned.snapshot() == {} - assert learned.clear() is False def test_every_learnable_href_is_one_climate_resolves(): @@ -215,14 +215,14 @@ async def test_learning_persists_onto_the_config_entry(hass: HomeAssistant): coordinator._observe.apply(CONVENIENT, QUIET_REP, source="poll") await _flush(hass) - assert entry.data[CONF_LEARNED_MODES] == {CONVENIENT: {SUPPORTED_FIELD: ["Quiet"]}} + assert entry.data[CONF_LEARNED_MODES] == {CONVENIENT: ["Quiet"]} async def test_a_learned_preset_survives_a_restart(hass: HomeAssistant): """The point of persisting: the unit only names Quiet while it is in Quiet, so a restart in any other mode would otherwise lose it until someone reached for the physical remote again.""" - entry = _entry(hass, data={CONF_LEARNED_MODES: {CONVENIENT: {SUPPORTED_FIELD: ["Quiet"]}}}) + entry = _entry(hass, data={CONF_LEARNED_MODES: {CONVENIENT: ["Quiet"]}}) _, entity = await _climate(hass, entry) # Nothing applied this run -- the fixture's own rep says Off, and its @@ -255,7 +255,7 @@ async def test_the_option_turns_off_both_halves(hass: HomeAssistant): nothing already learned is offered.""" entry = _entry( hass, - data={CONF_LEARNED_MODES: {CONVENIENT: {SUPPORTED_FIELD: ["Smart"]}}}, + data={CONF_LEARNED_MODES: {CONVENIENT: ["Smart"]}}, options={CONF_LEARN_MODES: False}, ) coordinator, entity = await _climate(hass, entry) @@ -266,12 +266,12 @@ async def test_the_option_turns_off_both_halves(hass: HomeAssistant): assert "quiet" not in entity.preset_modes # Kept, not discarded -- turning the option back on restores it, and # nothing new was written while it was off. - assert coordinator.learned_snapshot() == {CONVENIENT: {SUPPORTED_FIELD: ["Smart"]}} - assert entry.data[CONF_LEARNED_MODES] == {CONVENIENT: {SUPPORTED_FIELD: ["Smart"]}} + assert coordinator.learned_snapshot() == {CONVENIENT: ["Smart"]} + assert entry.data[CONF_LEARNED_MODES] == {CONVENIENT: ["Smart"]} async def test_forgetting_clears_the_store_and_the_entry(hass: HomeAssistant): - entry = _entry(hass, data={CONF_LEARNED_MODES: {CONVENIENT: {SUPPORTED_FIELD: ["Quiet"]}}}) + entry = _entry(hass, data={CONF_LEARNED_MODES: {CONVENIENT: ["Quiet"]}}) coordinator, entity = await _climate(hass, entry) coordinator.forget_learned_modes() @@ -281,6 +281,20 @@ async def test_forgetting_clears_the_store_and_the_entry(hass: HomeAssistant): assert entry.data[CONF_LEARNED_MODES] == {} +async def test_forgetting_clears_a_record_the_store_rejected(hass: HomeAssistant): + """A malformed persisted record is dropped on restore, so the store is + empty while the entry still holds it. Forget has to reach it anyway -- + it's the only control that can.""" + entry = _entry(hass, data={CONF_LEARNED_MODES: {CONVENIENT: None}}) + coordinator, _ = await _climate(hass, entry) + assert coordinator.learned_snapshot() == {} + + coordinator.forget_learned_modes() + await _flush(hass) + + assert entry.data[CONF_LEARNED_MODES] == {} + + async def test_diagnostics_report_what_was_learned( hass: HomeAssistant, enable_custom_integrations, @@ -298,8 +312,5 @@ async def test_diagnostics_report_what_was_learned( diag = await async_get_config_entry_diagnostics(hass, cast(Any, entry)) - assert diag["learned_modes"] == { - "enabled": True, - "codes": {CONVENIENT: {SUPPORTED_FIELD: ["Quiet"]}}, - } + assert diag["learned_modes"] == {"enabled": True, "codes": {CONVENIENT: ["Quiet"]}} assert diag["resources"][CONVENIENT][SUPPORTED_FIELD] == ADVERTISED