diff --git a/.claude/skills/adding-device-support/SKILL.md b/.claude/skills/adding-device-support/SKILL.md index 1ef6355..6002702 100644 --- a/.claude/skills/adding-device-support/SKILL.md +++ b/.claude/skills/adding-device-support/SKILL.md @@ -400,8 +400,11 @@ the dump in this order; each step rules out a different cause. more of its hrefs are confirmed live; that's the gate working as intended, not a bug to chase. 4. **`multidevice.numofsubdevice`** — the board's own count, where it - reports one. Disagreement with `len(subdevices) + len(subdevices_skipped)` - is a strong hint, not proof; only one board family is known to expose it. + reports one. `coordinator._run_discovery` compares it against + `len(materialized) + 1` (materialized subdevices plus the master) and + only warns on disagreement — `subdevices_skipped` entries don't count + toward either side, since they never materialized. A strong hint, not + proof; only one board family is known to expose it. 5. **Which pattern is this board?** `identity.resources['/oic/res']` listing `/device/1`, `/device/2` means indexed siblings. `resources['/subdevices/ vs/0']` carrying a `subdeviceIdList` means a UUID-prefixed tree, and that diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 88000b2..536391e 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -415,7 +415,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): if sess is None: return {} if subdevice.flat_hrefs: - return self._poll_subdevice_flat_hrefs(subdevice) + return self._poll_subdevice_flat_hrefs(subdevice, sess) try: code, payload = sess.get(list(subdevice.seed_path), timeout=10.0) if code == 0x45 and payload: @@ -426,23 +426,40 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._log.debug("subdevice %s seed poll failed: %s", subdevice.key, e) return {} - def _poll_subdevice_flat_hrefs(self, subdevice: Subdevice) -> dict[str, dict]: + def _poll_subdevice_flat_hrefs(self, subdevice: Subdevice, sess) -> dict[str, dict]: """Re-poll a flat-mode prefixed subdevice's hrefs individually (issue #205) -- it has no Collection endpoint to batch-refresh through (see enumerate_subdevices' fallback), so each canonical href confirmed at enumeration time gets its own GET under the subdevice's prefix. A href failing to answer this cycle just drops out of the result, same "never let a sibling's flakiness fail the - master's poll" posture as the Collection path above.""" - sess = self._session + master's poll" posture as the Collection path above. + + Takes `sess` from the caller (already None-checked there) rather + than re-reading self._session -- async_close() can null that + without holding _session_lock, and pace()/get() both need a live + session on every iteration, not just the first. + + Skips any href already covered by the hot/warm sub-poll tiers + (self._hot_hrefs/_warm_hrefs, in the same actual/on-the-wire form + this method builds) -- those are already refreshed every 3s/6s by + _run_subpolls, strictly more current than this once-per-summary-poll + pass could offer, so re-fetching them here would only add GETs + without adding freshness. A subdevice with many confirmed hrefs + (unlike a Collection batch, which is always one GET regardless of + count) is otherwise a summary-poll cost that scales with its href + count.""" + skip = set(self._hot_hrefs) | set(self._warm_hrefs) result: dict[str, dict] = {} first = True for href in subdevice.flat_hrefs: - if not first: - sess.pace() - first = False actual = subdevice.to_actual(href) + if actual in skip: + continue try: + if not first: + sess.pace() + first = False path = [s for s in actual.strip('/').split('/')] code, payload = sess.get(path, timeout=10.0) if code == 0x45 and payload: diff --git a/custom_components/localthings/registry/subdevices.py b/custom_components/localthings/registry/subdevices.py index 01fe1b8..f89635d 100644 --- a/custom_components/localthings/registry/subdevices.py +++ b/custom_components/localthings/registry/subdevices.py @@ -18,17 +18,22 @@ Pattern B -- UUID-prefixed tree (`TP2X_FAC_BORA_21K`, jhkwon19's board). `/oic/res` hides the whole appliance tree; `/device/0`'s batch instead carries `x.com.samsung.da.subdeviceIdList` on `/subdevices/vs/0`, and that same UUID appears as a literal href prefix in `/oic/res` -(`//file/list/vs/0`, ...). On jhkwon19's own first unit, `GET -//device/0` returned the second subdevice's own Collection batch, -confirmed live to carry a different model/serial than the master -(`TP2X_FAC_BORA_RAC_21K`, the wall-mounted subdevice, vs. the master's -`TP2X_FAC_BORA_21K`, the floor subdevice) -- but issue #205, a second -TP2X_FAC_BORA_21K unit, showed that same `//device/0` probe coming -back empty, so it isn't a property of the board family, only of the -individual unit/firmware. When it's empty, `enumerate_subdevices` falls back -to probing every href the master itself answered this cycle individually -under the UUID prefix, on the assumption that a composite device's siblings -share the master's resource surface -- see `Subdevice.flat_hrefs`. +(`//file/list/vs/0`, ...). What's actually been confirmed live on +jhkwon19's unit is narrower than early issue #177 writeups suggested: a +single individual `GET //information/vs/0` was read by hand through +the debug panel and came back carrying a different model/serial than the +master (`TP2X_FAC_BORA_RAC_21K`, the wall-mounted subdevice, vs. the +master's `TP2X_FAC_BORA_21K`, the floor subdevice) -- real evidence a +second subdevice exists at that prefix, but not evidence that `GET +//device/0` (the Collection batch PR #199 built this pattern's seed +around) itself returns anything. Issue #205, the same unit on a later +version, is that assumption failing: `//device/0` comes back empty. +So `enumerate_subdevices` tries it first (a future board might genuinely +expose it) and falls back, when it's empty, to probing every href the +master itself answered this cycle individually under the UUID prefix -- +the only thing ever actually confirmed to work for this pattern -- on the +assumption that a composite device's siblings share the master's resource +surface. See `Subdevice.flat_hrefs`. Both are "the same thing wearing different clothes": a logical subdevice is a seed collection path to poll, plus an href transform between the canonical @@ -351,6 +356,17 @@ def enumerate_subdevices( # this UUID's prefix, and keep whichever ones answer. Each is a # plain tolerated-404 RETRIEVE, same posture as every other probe in # this function. + # + # Known gap, not yet guarded against: a firmware that answers *any* + # request under an unrecognized prefix (echoing the master's own + # state back rather than 4.04ing) would pass every one of these + # probes and, if the echoed state also clears discover_partitioned's + # liveness gate, materialize a phantom duplicate of the master + # rather than a real sibling. Every board seen so far genuinely + # 4.04s on paths it doesn't own (issue #205's own unit answered only + # 1 of 31 probes), so this hasn't been built -- the one place it + # could hook in later is comparing a candidate's confirmed reps + # against the master's own values for those same canonical hrefs. flat_hrefs = [] first = True for href in sorted(resources): diff --git a/tests/fixtures/airconditioner_fac_bora_205_flat_device.json b/tests/fixtures/airconditioner_fac_bora_205_flat_device.json index bc4bc1f..0cfb0e6 100644 --- a/tests/fixtures/airconditioner_fac_bora_205_flat_device.json +++ b/tests/fixtures/airconditioner_fac_bora_205_flat_device.json @@ -716,5 +716,5 @@ "x.com.samsung.da.serialNum": "TEST-SUBDEVICE-SERIAL-0000" } }, - "seeds_note": "device0 and oic_res are real, captured from jhkwon19's issue #205 report. subdeviceIdList in device0's /subdevices/vs/0 is restored to the real ['6c2dff6d-ee5c-dad1-6a5e-000000000001'] (HA's diagnostics download redacts it, matching redact.py's 'deviceid' substring rule, not because it's account data -- same restoration airconditioner_fac_bora_2in1_device.json documents for the same field), and every other value is otherwise exactly what HA's download redacts (serials/otnDUID etc.) against the same physical TP2X_FAC_BORA_21K unit as airconditioner_fac_bora_2in1_device.json -- same subdeviceIdList UUID, same wall subdevice. Filed specifically because, contrary to the assumption that fixture's seed batch was built on, this unit's /6c2dff6d-ee5c-dad1-6a5e-000000000001/device/0 does NOT answer (subdevice_probes in the report shows it False), so the Collection-batch fallback this fixture exercises (registry.subdevices.enumerate_subdevices' per-href probe, issue #205) is what has to find the subdevice instead. The one probes entry, /6c2dff6d-ee5c-dad1-6a5e-000000000001/information/vs/0, is the same real capture already used in airconditioner_fac_bora_2in1_device.json's seed batch (issue #177 comment 5113518087) -- the only href ever actually confirmed to answer under this UUID prefix. No other /6c2dff6d-ee5c-dad1-6a5e-000000000001/* href has been confirmed live yet, so none are seeded here; this fixture's expected outcome is the candidate being found by the flat-href probe but then correctly held back by discover_partitioned's liveness gate (information alone binds no entity), matching where the real issue stands -- not a materialized climate entity, which would require guessing at unconfirmed hrefs." + "seeds_note": "device0 and oic_res are real, captured from jhkwon19's issue #205 report. subdeviceIdList in device0's /subdevices/vs/0 is restored to the real ['6c2dff6d-ee5c-dad1-6a5e-000000000001'] (HA's diagnostics download redacts it, matching redact.py's 'deviceid' substring rule, not because it's account data -- same restoration airconditioner_fac_bora_2in1_device.json documents for the same field), and every other value is otherwise exactly what HA's download redacts (serials/otnDUID etc.) against the same physical TP2X_FAC_BORA_21K unit as airconditioner_fac_bora_2in1_device.json -- same subdeviceIdList UUID, same wall subdevice. Filed specifically because, contrary to the assumption that fixture's seed batch was built on, this unit's /6c2dff6d-ee5c-dad1-6a5e-000000000001/device/0 does NOT answer (subdevice_probes in the report shows it False), so the Collection-batch fallback this fixture exercises (registry.subdevices.enumerate_subdevices' per-href probe, issue #205) is what has to find the subdevice instead. The one probes entry, /6c2dff6d-ee5c-dad1-6a5e-000000000001/information/vs/0, is the same real capture already used in airconditioner_fac_bora_2in1_device.json's seed batch (issue #177 comment 5113518087) -- the only href ever actually confirmed to answer under this UUID prefix. No other /6c2dff6d-ee5c-dad1-6a5e-000000000001/* href has been confirmed live yet, so none are seeded here; this fixture's expected outcome is the candidate being found by the flat-href probe but then correctly held back by discover_partitioned's liveness gate (information alone binds no entity), matching where the real issue stands -- not a materialized climate entity, which would require guessing at unconfirmed hrefs. The probe's serialNum (\"TEST-SUBDEVICE-SERIAL-0000\") is a hand-placed placeholder for the real value, not HA's own redaction output -- same placeholder airconditioner_fac_bora_2in1_device.json uses for the identical field." } diff --git a/tests/fixtures/golden/airconditioner_fac_bora_205_flat.json b/tests/fixtures/golden/airconditioner_fac_bora_205_flat.json new file mode 100644 index 0000000..aca70e7 --- /dev/null +++ b/tests/fixtures/golden/airconditioner_fac_bora_205_flat.json @@ -0,0 +1,20 @@ +{ + "state_keys": [ + "air_filter_status", + "air_filter_threshold", + "air_filter_usage", + "air_filter_usage_hours", + "alarm_code", + "auto_clean", + "beep", + "climate", + "current_temperature_c", + "diagnosis_status", + "energy_kwh", + "firmware_update", + "humidity", + "power_energy_kwh", + "power_watts", + "tropical_night_mode" + ] +} diff --git a/tests/test_golden_regression.py b/tests/test_golden_regression.py index 8752069..540ffed 100644 --- a/tests/test_golden_regression.py +++ b/tests/test_golden_regression.py @@ -961,6 +961,30 @@ def test_registry_reproduces_golden_state_keys_for_airconditioner_fac_bora_2in1( ) +def test_registry_reproduces_golden_state_keys_for_airconditioner_fac_bora_205_flat(): + """jhkwon19's same physical TP2X_FAC_BORA_21K unit as the _2in1 fixture + above, but a later capture (issue #205) where //device/0 doesn't + answer -- contrary to what that fixture's own seed batch assumed the + Collection endpoint would do. device0/oic_res are real; the only + UUID-prefixed data is the one href ever actually confirmed live + (/information/vs/0, same real capture the _2in1 fixture uses), fed + through registry.subdevices.enumerate_subdevices' per-href flat + fallback instead of a Collection batch. /information/vs/0 alone binds + no entity, so the candidate is found but never materializes -- this + golden has no `subdevice_...`-prefixed keys at all, same shape as + tests/fixtures/golden/airconditioner_fac_bora.json, which is the point: + a device whose sibling can't yet be confirmed live must regress to + exactly the master-only state, never a phantom or partial subdevice.""" + name = 'airconditioner_fac_bora_205_flat' + golden = json.loads((GOLDEN / f'{name}.json').read_text()) + state_keys = _new_subdevice_aware_state_keys(name) + assert set(state_keys) == set(golden['state_keys']), ( + f"state_keys mismatch:\n" + f" extra: {sorted(set(state_keys) - set(golden['state_keys']))}\n" + f" missing: {sorted(set(golden['state_keys']) - set(state_keys))}" + ) + + def test_registry_reproduces_golden_state_keys_for_air_purifier_avt_ww(): """AVT-WW-TP1-23-AXX500 (issue #190) -- next-gen BESPOKE Cube Air board; reports device_type 'unknown' with empty oneUiVersion because 'VTWW' as a diff --git a/tests/test_subdevice_discovery.py b/tests/test_subdevice_discovery.py index 02498ae..4657c15 100644 --- a/tests/test_subdevice_discovery.py +++ b/tests/test_subdevice_discovery.py @@ -36,12 +36,14 @@ def _coordinator(hass: HomeAssistant) -> LocalThingsCoordinator: return LocalThingsCoordinator(hass, entry) -async def _discover(coordinator: LocalThingsCoordinator, name: str) -> None: +async def _discover_with( + coordinator: LocalThingsCoordinator, resources: dict, oic_res, seeds: dict, +) -> None: """Run the same two-step sequence _async_update_data's first cycle does - (enumerate, then discover) against fixture data, without the polling/ - reconnect machinery around it -- see coordinator.py's - _enumerate_subdevices_blocking/_run_discovery.""" - resources, oic_res, seeds = _load_device_full(name) + (enumerate, then discover) against arbitrary resources/oic_res/seeds, + without the polling/reconnect machinery around it -- see coordinator.py's + _enumerate_subdevices_blocking/_run_discovery. `_discover` below is the + fixture-file-backed convenience wrapper most tests want.""" coordinator._session = FakeCoapSession(seeds) # _connect_session (skipped here -- the session is pre-set) is what # normally populates _identity via read_identity; set it directly with @@ -67,6 +69,12 @@ async def _discover(coordinator: LocalThingsCoordinator, name: str) -> None: coordinator._observe.apply(href, rep, source='poll') +async def _discover(coordinator: LocalThingsCoordinator, name: str) -> None: + """Fixture-file-backed convenience wrapper around _discover_with.""" + resources, oic_res, seeds = _load_device_full(name) + await _discover_with(coordinator, resources, oic_res, seeds) + + def _climate_bound(coordinator, subdevice_key: str): from custom_components.localthings.registry.subdevices import MAIN for b in coordinator.bound: @@ -220,6 +228,90 @@ async def test_fac_bora_205_flat_fallback_finds_candidate_but_gate_holds_it_back assert _climate_bound(coordinator, None) is not None +async def test_flat_subdevice_materializes_and_repolls_end_to_end(hass: HomeAssistant): + """Synthetic (not a real capture, unlike the fixture-driven test above) -- + exercises the one path nothing else covers: a flat-mode prefixed + subdevice with *enough* confirmed hrefs to actually pass + discover_partitioned's liveness gate and materialize a real climate + entity, then a subsequent _poll_subdevice_seed re-poll refreshing its + state all the way through to canonical_resources -- the path a real + resolution of issue #205 (once more hrefs are confirmed live for some + unit) would actually need. + + Also pins _poll_subdevice_flat_hrefs' hot/warm skip: climate-critical + hrefs (power/mode/temperature) land on the warm tier by discovery's own + rules, so they're already kept fresh every few seconds by _run_subpolls + -- re-fetching them again on this once-per-summary-poll pass would only + add GETs, not freshness, so they're the ones this method must skip. + /option/autoclean/vs/0 is cold-tier and is what actually needs this + path.""" + resources, oic_res, _real_seeds = _load_device_full('airconditioner_fac_bora_2in1') + seeds = { + # No (_SUB_UUID, 'device', '0') entry -- forces the flat fallback, + # same as the real issue #205 capture above, but this time with + # enough hrefs answering to actually materialize. power/mode/ + # temperature values copied verbatim from that fixture's own (real) + # Collection-batch seed, just served individually instead of + # batched, to isolate "does flat mode produce the same result as + # Collection mode" as the only variable. + f'/{_SUB_UUID}/power/vs/0': {'x.com.samsung.da.power': 'On'}, + f'/{_SUB_UUID}/mode/vs/0': { + 'x.com.samsung.da.supportedModes': ['Cool', 'Dry', 'Wind', 'Auto'], + 'x.com.samsung.da.modes': ['Cool'], + 'x.com.samsung.da.options': [], + }, + f'/{_SUB_UUID}/temperature/current/0': { + 'range': [18.0, 30.0], 'units': 'C', 'temperature': 26.0, + }, + f'/{_SUB_UUID}/temperature/desired/0': { + 'range': [18.0, 30.0], 'units': 'C', 'temperature': 24.0, + }, + # Cold-tier -- not covered by _run_subpolls, so this is the href + # that actually depends on _poll_subdevice_flat_hrefs to ever + # refresh at all. + f'/{_SUB_UUID}/option/autoclean/vs/0': { + 'x.com.samsung.da.settingStatus': 'Off', + }, + } + coordinator = _coordinator(hass) + await _discover_with(coordinator, resources, oic_res, seeds) + + assert [su.key for su in coordinator.subdevices] == [_SUB_UUID] + subdevice = coordinator.subdevices[0] + assert subdevice.seed_path == () + assert subdevice.flat_hrefs != () + + sub_climate = _climate_bound(coordinator, _SUB_UUID) + assert sub_climate is not None + + # Re-poll: a fresh reading under the prefix should reach + # canonical_resources through _poll_subdevice_seed's flat-mode branch, + # not just sit frozen at the one-time enumeration snapshot. + coordinator._session.seeds[f'/{_SUB_UUID}/temperature/current/0'] = { + 'range': [18.0, 30.0], 'units': 'C', 'temperature': 27.5, + } + coordinator._session.seeds[f'/{_SUB_UUID}/option/autoclean/vs/0'] = { + 'x.com.samsung.da.settingStatus': 'On', + } + refreshed = coordinator._poll_subdevice_seed(subdevice) + + # The warm-tier temperature href is skipped here -- already covered by + # _run_subpolls at a faster cadence -- so it does NOT show up refreshed + # through this path. + assert f'/{_SUB_UUID}/temperature/current/0' not in refreshed + assert refreshed == { + f'/{_SUB_UUID}/option/autoclean/vs/0': {'x.com.samsung.da.settingStatus': 'On'}, + } + + for href, rep in refreshed.items(): + coordinator._observe.apply(href, rep, source='poll') + res = coordinator.canonical_resources(subdevice) + assert res['/option/autoclean/vs/0']['x.com.samsung.da.settingStatus'] == 'On' + # Confirms the skip is about redundant re-fetching, not stale data -- + # the warm-tier value from initial discovery is still there, untouched. + assert res['/temperature/current/0']['temperature'] == 26.0 + + class _FakeCollectionSession: """Minimal session that only ever answers a Collection GET -- used to prove the flat-mode re-poll path (issue #205) is only taken when @@ -295,6 +387,33 @@ def test_poll_subdevice_seed_flat_mode_polls_each_href_individually( ] +def test_poll_subdevice_seed_flat_mode_skips_hrefs_covered_by_hot_warm_subpolls( + hass: HomeAssistant, +): + """A flat href already in the hot/warm sub-poll tiers is refreshed every + few seconds by _run_subpolls -- re-fetching it again on this + once-per-summary-poll pass would only add GETs, not freshness, so + _poll_subdevice_flat_hrefs must not even attempt it.""" + from custom_components.localthings.registry.subdevices import Subdevice + + coordinator = _coordinator(hass) + sess = _FakeCollectionSession({ + (_SUB_UUID, 'mode', 'vs', '0'): {'mode': 'cool'}, + (_SUB_UUID, 'power', 'vs', '0'): {'power': 'On'}, + }) + coordinator._session = sess + coordinator._warm_hrefs = [f'/{_SUB_UUID}/mode/vs/0'] + subdevice = Subdevice( + kind='prefixed', key=_SUB_UUID, seed_path=(), + flat_hrefs=('/mode/vs/0', '/power/vs/0'), + ) + + result = coordinator._poll_subdevice_seed(subdevice) + + assert result == {f'/{_SUB_UUID}/power/vs/0': {'power': 'On'}} + assert sess.calls == [(_SUB_UUID, 'power', 'vs', '0')] + + async def test_multidevice_probe_never_reaches_discovery_or_the_cache( hass: HomeAssistant, ): diff --git a/tests/test_subdevices.py b/tests/test_subdevices.py index 00abf40..f157cfb 100644 --- a/tests/test_subdevices.py +++ b/tests/test_subdevices.py @@ -318,6 +318,51 @@ def test_enumerate_prefixed_flat_fallback_probe_log_reports_every_href_tried(): assert probes[f'/{_UUID}/mode/vs/0'] is True +def test_enumerate_prefixed_flat_fallback_with_no_master_hrefs_to_probe_is_a_no_op(): + """The master itself having nothing but /subdevices/vs/0 in its own + resources this cycle (e.g. a very first, mostly-empty poll) must not + crash the fallback loop -- the only href in `resources` is + /subdevices/vs/0 itself, which the fake session doesn't answer under + the prefix either, so nothing materializes.""" + resources = { + '/subdevices/vs/0': {'x.com.samsung.da.subdeviceIdList': [_UUID]}, + } + subdevices, extra = enumerate_subdevices(_FakeSession({}), resources, oic_res_links=[]) + assert subdevices == [] + assert extra == {} + + +def test_enumerate_prefixed_flat_fallback_does_not_cross_contaminate_a_second_uuid(): + """Two prefixed candidates in the same subdeviceIdList, one whose + Collection endpoint works and one that needs the flat fallback -- each + must end up with only its own hrefs, no bleed between them.""" + uuid_a, uuid_b = _UUID, '11111111-1111-1111-1111-111111111111' + resources = { + '/subdevices/vs/0': {'x.com.samsung.da.subdeviceIdList': [uuid_a, uuid_b]}, + '/mode/vs/0': {'m': 'cool'}, + } + sess = _FakeSession({ + (uuid_a, 'device', '0'): [ + _DEVCOL_REP, {'href': '/mode/vs/0', 'rep': {'m': 'a-collection'}}, + ], + # uuid_b's Collection deliberately absent -> falls back to the flat + # per-href probe. + (uuid_b, 'mode', 'vs', '0'): {'m': 'b-flat'}, + }) + subdevices, extra = enumerate_subdevices(sess, resources, oic_res_links=[]) + + by_key = {u.key: u for u in subdevices} + assert set(by_key) == {uuid_a, uuid_b} + assert by_key[uuid_a].seed_path == (uuid_a, 'device', '0') + assert by_key[uuid_a].flat_hrefs == () + assert by_key[uuid_b].seed_path == () + assert by_key[uuid_b].flat_hrefs == ('/mode/vs/0',) + assert extra == { + f'/{uuid_a}/mode/vs/0': {'m': 'a-collection'}, + f'/{uuid_b}/mode/vs/0': {'m': 'b-flat'}, + } + + def test_enumerate_prefixed_tolerates_redacted_string_id_list(): """subdeviceIdList matches redact.py's 'deviceid' substring rule, and the real airconditioner_fac_bora_device.json fixture carries the literal