diff --git a/custom_components/localthings/registry/subdevices.py b/custom_components/localthings/registry/subdevices.py index 20e9585..b3835c0 100644 --- a/custom_components/localthings/registry/subdevices.py +++ b/custom_components/localthings/registry/subdevices.py @@ -47,10 +47,17 @@ trying to re-confirm that claim one href at a time bought unreliable confirmation at a cost that could take the integration down with it. So the fallback now trusts the claim and clones the master's current hrefs *and* values verbatim under the prefix as this cycle's assumed state for the -sibling -- no extra RETRIEVEs at all. `coordinator._poll_subdevice_flat_hrefs` -still re-GETs each of those hrefs individually, under the prefix, on every -later summary poll (unchanged by this), correcting the assumption toward -the sibling's real values as responses arrive; an href the firmware never +sibling, with one bounded exception: it still retries a single individual +`GET //information/vs/0` -- the one href this pattern has ever +actually confirmed to answer on its own, per the paragraph above -- so the +sibling's HA device shows its own model/serial rather than the master's +cloned ones (identical model/serial across two otherwise-distinct devices +reads as a duplicate, even though their device-registry identifiers are +genuinely different). That's one extra RETRIEVE per candidate, not the ~30 +that caused issue #265. `coordinator._poll_subdevice_flat_hrefs` still +re-GETs every cloned href individually, under the prefix, on every later +summary poll (unchanged by this), correcting the assumption toward the +sibling's real values as responses arrive; an href the firmware never answers under that prefix just keeps mirroring the master indefinitely -- which, for a board proven not to expose that href at all, is what "assume it exists" has to mean in practice. See `Subdevice.flat_hrefs`. @@ -420,8 +427,8 @@ def enumerate_subdevices( # there, so trust it outright rather than spend an unbounded, # firmware-dependent number of RETRIEVEs re-confirming it href by # href: clone the master's current hrefs and values verbatim under - # this prefix as the sibling's assumed state, no further RETRIEVEs - # at all. coordinator._poll_subdevice_flat_hrefs re-GETs each of + # this prefix as the sibling's assumed state, no per-href probing + # loop at all. coordinator._poll_subdevice_flat_hrefs re-GETs each of # these hrefs individually, under the prefix, on every later summary # poll (unchanged by this), correcting the clone toward the # sibling's real values as responses arrive; an href the firmware @@ -433,6 +440,25 @@ def enumerate_subdevices( return for href in flat_hrefs: fetched[f"/{sub_id}{href}"] = resources[href] + # One exception, worth its own bounded RETRIEVE rather than trusting + # the clone: /information/vs/0 is what device_info_for() reads for + # this subdevice's own model/serial. Left cloned from the master, + # the sibling's HA device page would show the *master's* model and + # serial verbatim -- distinct device-registry identifiers (derived + # from this UUID, see device_info_for), but reading as a duplicate + # of the master to anyone looking at the two devices' info. This is + # also the one href ever actually confirmed to answer under this + # pattern's prefix on real hardware (issue #177 comment 5113518087), + # so it's worth retrying here even though the rest of the fallback + # no longer probes anything -- one extra RETRIEVE per candidate, + # not the ~30 that caused issue #265. + info_seed = (sub_id, "information", "vs", "0") + info = _get_property(sess, info_seed) + _probed(_seed_href(info_seed), info) + if info: + fetched[f"/{sub_id}/information/vs/0"] = info + if "/information/vs/0" not in flat_hrefs: + flat_hrefs = tuple(sorted({*flat_hrefs, "/information/vs/0"})) subdevices.append( Subdevice( kind="prefixed", diff --git a/tests/test_subdevice_discovery.py b/tests/test_subdevice_discovery.py index a02a051..0af4dfd 100644 --- a/tests/test_subdevice_discovery.py +++ b/tests/test_subdevice_discovery.py @@ -285,11 +285,12 @@ async def test_fac_bora_205_flat_fallback_clones_master_and_materializes( (issue #205), but issue #265 replaced the old per-href confirmation loop -- which used to hold this candidate back with only /information/vs/0 ever confirmed live under the prefix -- with an unconditional clone of - the master's own hrefs and values under the prefix. So the sibling now - materializes immediately with the master's own climate state as its - assumed starting point, rather than waiting on per-href confirmation - that this firmware may never give (this test used to assert the - opposite: that the liveness gate correctly held the candidate back).""" + the master's own hrefs and values under the prefix, plus one bounded + retry of /information/vs/0 itself. So the sibling now materializes + immediately with the master's own climate state as its assumed starting + point, but its own real, distinct model/serial (this test used to + assert the opposite: that the liveness gate correctly held the + candidate back).""" coordinator = _coordinator(hass) resources, _oic_res, _seeds = _load_device_full("airconditioner_fac_bora_205_flat") await _discover(coordinator, "airconditioner_fac_bora_205_flat") @@ -301,16 +302,26 @@ async def test_fac_bora_205_flat_fallback_clones_master_and_materializes( assert subdevice.seed_path == () assert subdevice.flat_hrefs == tuple(sorted(resources)) - # Only the seed Collection itself was ever probed over the network -- - # no more per-href confirmation loop to log. + # The seed Collection and the one bounded /information/vs/0 retry were + # probed over the network -- nothing else, no per-href confirmation loop. assert coordinator._subdevice_probes[f"/{_SUB_UUID}/device/0"] is False + assert coordinator._subdevice_probes[f"/{_SUB_UUID}/information/vs/0"] is True + allowed = {f"/{_SUB_UUID}/device/0", f"/{_SUB_UUID}/information/vs/0"} assert not any( - href.startswith(f"/{_SUB_UUID}/") and href != f"/{_SUB_UUID}/device/0" + href.startswith(f"/{_SUB_UUID}/") and href not in allowed for href in coordinator._subdevice_probes ) assert _climate_bound(coordinator, _SUB_UUID) is not None + # The confirmed /information/vs/0 reply -- not the master's cloned one -- + # is what device_info_for() reads, so the sibling's HA device shows its + # own real model/serial rather than looking like a duplicate of the + # master (issue #177 comment 5113518087's real hand-read capture). + info = coordinator.device_info_for(subdevice) + assert info["model"] == "TP2X_FAC_BORA_RAC_21K" + assert info["model"] != coordinator.device_info.get("model") + # Confirms the master itself is completely unaffected by its sibling's # Collection endpoint not answering -- same guarantee every other # subdevice test in this file relies on. diff --git a/tests/test_subdevices.py b/tests/test_subdevices.py index dc4026a..abd9704 100644 --- a/tests/test_subdevices.py +++ b/tests/test_subdevices.py @@ -302,9 +302,12 @@ def test_enumerate_prefixed_falls_back_to_cloning_master_state_when_device0_coll master href individually under the prefix to confirm one used to be the fallback, but a firmware that drops packets instead of 4.04ing turned that into a ~300s hang that took the whole config entry's setup down - with it. So the fallback no longer touches the network at all -- it + with it. So the fallback no longer loops over every master href -- it trusts the device's own subdeviceIdList claim and clones the master's - current hrefs and values verbatim under the prefix.""" + current hrefs and values verbatim under the prefix, plus one bounded + retry of /information/vs/0 specifically (see the dedicated test below); + that retry also fails to answer here, so the clone is all this + candidate ends up with.""" resources = { "/subdevices/vs/0": {"x.com.samsung.da.subdeviceIdList": [_UUID]}, "/mode/vs/0": {"m": "cool"}, @@ -324,6 +327,38 @@ def test_enumerate_prefixed_falls_back_to_cloning_master_state_when_device0_coll } +def test_enumerate_prefixed_flat_fallback_confirms_its_own_information(): + """When the one bounded retry of //information/vs/0 *does* answer, + its real reply overrides the master's cloned /information/vs/0 -- so the + sibling's device_info shows its own model/serial instead of the + master's, which would otherwise read as a duplicate device in the UI + even though the two devices' registry identifiers are genuinely + distinct (see device_info_for).""" + resources = { + "/subdevices/vs/0": {"x.com.samsung.da.subdeviceIdList": [_UUID]}, + "/information/vs/0": {"x.com.samsung.da.modelNum": "MASTER_MODEL"}, + "/mode/vs/0": {"m": "cool"}, + } + sess = _FakeSession( + { + # (_UUID, 'device', '0') absent -> Collection probe fails. + (_UUID, "information", "vs", "0"): { + "x.com.samsung.da.modelNum": "SIBLING_MODEL", + "x.com.samsung.da.serialNum": "SIBLING-SERIAL", + }, + } + ) + subdevices, extra = enumerate_subdevices(sess, resources, oic_res_links=[]) + assert len(subdevices) == 1 + subdevice = subdevices[0] + assert subdevice.flat_hrefs == ("/information/vs/0", "/mode/vs/0", "/subdevices/vs/0") + assert extra[f"/{_UUID}/information/vs/0"] == { + "x.com.samsung.da.modelNum": "SIBLING_MODEL", + "x.com.samsung.da.serialNum": "SIBLING-SERIAL", + } + assert extra[f"/{_UUID}/mode/vs/0"] == {"m": "cool"} + + def test_enumerate_prefixed_flat_fallback_is_a_no_op_with_no_master_hrefs_to_clone(): """A degenerate `resources` (nothing at all, not even /subdevices/vs/0 -- can't happen via the subdeviceIdList path but shared by _probe_prefixed @@ -338,9 +373,11 @@ def test_enumerate_prefixed_flat_fallback_is_a_no_op_with_no_master_hrefs_to_clo assert extra == {} -def test_enumerate_prefixed_flat_fallback_probe_log_only_reports_the_seed(): - """Only //device/0 is ever probed over the network now -- no more - per-href probing to log.""" +def test_enumerate_prefixed_flat_fallback_probe_log_only_reports_seed_and_information(): + """Only //device/0 (the seed) and //information/vs/0 (the one + bounded exception -- so the sibling gets its own model/serial rather + than the master's cloned ones) are ever probed over the network now -- + no more per-href probing loop to log.""" resources = { "/subdevices/vs/0": {"x.com.samsung.da.subdeviceIdList": [_UUID]}, "/mode/vs/0": {"m": "cool"}, @@ -350,9 +387,9 @@ def test_enumerate_prefixed_flat_fallback_probe_log_only_reports_the_seed(): _FakeSession({}), resources, oic_res_links=[], probe_log=probes.__setitem__ ) assert probes[f"/{_UUID}/device/0"] is False - assert not any( - href.startswith(f"/{_UUID}/") and href != f"/{_UUID}/device/0" for href in probes - ) + assert probes[f"/{_UUID}/information/vs/0"] is False + allowed = {f"/{_UUID}/device/0", f"/{_UUID}/information/vs/0"} + assert not any(href.startswith(f"/{_UUID}/") and href not in allowed for href in probes) def test_enumerate_prefixed_flat_fallback_does_not_cross_contaminate_a_second_uuid():