From 5dbe990c1d06c3aaea3c54ebecdfedeaceee1db3 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 7 Aug 2026 22:08:28 +0000 Subject: [PATCH 1/5] Add write_resource/read_resource services for probing write contracts (issue #300) The options-flow "Debug write" panel could only ever do one write to one href per pass -- not enough for the issue #300 wall oven, whose board discards settings writes while idle and only keeps them once a cycle is already running. Finding what starts a cycle needs an ordered sequence of writes across resources, with real settle delays between them, and a way to check afterward whether anything actually held. - coordinator.py: async_raw_write_sequence owns a whole ordered sequence under one _session_lock hold (so a poll can't interleave mid-sequence), with per-step settle and an optional delayed verify_after re-read done outside the lock. async_raw_write is now a one-item wrapper over it, so tests/test_coordinator_raw_write.py keeps passing unmodified. Also adds async_raw_read, a live GET bypassing the cache -- staleness is exactly what makes revert-testing unreliable. - services.py (new): the two HA services. Device-target resolution scans loaded coordinators' MAIN/subdevice identifiers and requires exactly one match, so an area/label target can't silently fan a raw write out across several appliances. Canonical->actual href translation happens here, not in the coordinator, which stays subdevice-agnostic. - services.yaml (new): selectors/descriptions for both services, inline per HA's custom-integration support -- keeps translations/en.json's mirror test (test_translations.py) green without touching all 6 languages for a services block. New exception keys (write caps, device target resolution) still went into translations/*.json's existing exceptions section, mirrored across all 6 languages. - __init__.py: adds async_setup to register the services once, process-wide. - config_flow.py: the debug panel's async_step_debug_edit now calls write_resource instead of coord.async_raw_write directly, so there is exactly one code path that performs a raw write. - README.md: new Part 5 documenting both services, with a worked write_resource example; points the capability-gap section at them. tests/test_services.py (new): sequencing/ordering, settle timing, changed vs. held (the reverted case is issue #300's own symptom), exactly-one- device resolution, subdevice href translation, validation caps, and the options-flow panel end to end through the service. --- README.md | 71 +++ custom_components/localthings/__init__.py | 9 + custom_components/localthings/config_flow.py | 29 +- custom_components/localthings/const.py | 6 + custom_components/localthings/coordinator.py | 211 +++++++- custom_components/localthings/services.py | 209 ++++++++ custom_components/localthings/services.yaml | 67 +++ .../localthings/translations/cs.json | 18 + .../localthings/translations/en.json | 18 + .../localthings/translations/es.json | 18 + .../localthings/translations/it.json | 18 + .../localthings/translations/ko.json | 18 + .../localthings/translations/nl.json | 18 + tests/localthings/test_config_flow.py | 39 +- tests/test_services.py | 458 ++++++++++++++++++ 15 files changed, 1180 insertions(+), 27 deletions(-) create mode 100644 custom_components/localthings/services.py create mode 100644 custom_components/localthings/services.yaml create mode 100644 tests/test_services.py diff --git a/README.md b/README.md index 8d7b1e1..54ea6be 100644 --- a/README.md +++ b/README.md @@ -98,6 +98,70 @@ Each device has its own **Configure** option in Settings > Devices & Services, u --- +## Part 5: Reading and writing resources directly + +Two HA actions, `localthings.write_resource` and `localthings.read_resource`, talk to a device's OCF resources directly instead of through this integration's entity model. They exist for two overlapping jobs: pinning down a device-specific write contract (the reverse-engineering work `docs/investigations/` and the provenance comments throughout `registry/capabilities/` are all about), and driving a resource this integration doesn't model as an entity yet, without waiting on a release. + +Both target a device (`target: device:`, filtered to `integration: localthings` in the picker) and resolve to exactly one appliance — targeting an area or label that expands to more than one LocalThings device is rejected rather than silently fanned out across all of them. `href` is always canonical (e.g. `/mode/vs/0`); if the device you targeted is a subdevice — an oven's second cavity, an AC's second indoor unit — it's translated to the real on-the-wire href for you (`/mode/vs/1`, say), and the response reports both forms so there's no ambiguity about what was actually sent. + +`write_resource` exists because a single write, one at a time, isn't enough to probe some boards. Issue #300's Samsung wall oven answers `2.04 Changed` to a settings write while idle and then silently reverts it — the write only sticks once a cycle is already running. Finding what actually triggers a cycle needs an *ordered sequence* of writes to different resources, with real delays between them, and a way to check afterward whether anything actually held: + +```yaml +action: localthings.write_resource +target: + device_id: abc123... +data: + writes: + - href: /course/vs/0 + payload: + x.com.samsung.da.course: "01" + settle: 3 + - href: /mode/vs/0 + payload: + x.com.samsung.da.mode: Bake + settle: 5 + - href: /power/vs/0 + payload: + x.com.samsung.da.power: "On" + verify_after: 30 +``` + +Each write in `writes` (1-10 of them) needs `href` and a non-empty `payload`, sent verbatim as a partial-rep POST — this bypasses the remote-control-off block and every `write_fn`/`validate_fn` a normal entity write goes through, and sends exactly the fields you give it, so it can misconfigure your appliance if you get it wrong. `settle` (0-30s, default 0) is how long to wait *after* that write before starting the next one. The whole sequence runs under a single lock, so a routine poll can't land in the middle of it and blur which write is responsible for what the device does next. + +The response has one `results` entry per write, with `before`/`after` reps and a `changed` flag (every key/value in `payload` present and equal in the immediate readback): + +```json +{ + "device_id": "abc123...", + "results": [ + {"href": "/course/vs/0", "actual_href": "/course/vs/0", "code": "2.04", "raw_code": 68, + "accepted": true, "before": {...}, "after": {...}, "changed": true}, + ... + ], + "verified": { + "/power/vs/0": {"code": "2.05", "raw_code": 69, "rep": {...}, "held": false} + } +} +``` + +`verify_after` (0-60s, default 0, omit to skip) is what actually answers the "did it stick" question: after the sequence finishes, it waits that long and then re-reads every distinct href the sequence touched, reporting the result under `verified`, keyed by canonical href. `changed` tells you the write was accepted and reflected immediately; `held` tells you whether it was still there N seconds later, or whether the board quietly put it back — issue #300's exact symptom. Where an href was written more than once in a sequence, `held` compares against the *last* payload sent to it. + +`read_resource` is the read half, and it's deliberately not just a cache lookup: + +```yaml +action: localthings.read_resource +target: + device_id: abc123... +data: + href: /mode/vs/0 +``` + +returning `{"href", "actual_href", "code", "raw_code", "rep"}` off a **live GET straight from the device**, not the cache — which can be up to a poll interval stale, exactly the staleness that would make `held` above meaningless. Omit `href` and you get `{"resources": {href: rep, ...}}`, the cached snapshot of everything this integration currently tracks on that device, with no GET at all — useful for seeing what's there before you start writing to it, without hammering the appliance. + +The **Debug write** panel under a device's Configure menu (Part 4) is the friendlier single-write path over this same machinery — pick an href, type a payload, see the result — for when you don't need a sequence. + +--- + ## Development ### Docker Compose dev environment @@ -135,6 +199,8 @@ custom_components/localthings/ coordinator.py Polling + push update coordination, stale-state fallback, write dispatch observe.py CoAP OBSERVE (push-mode) support layered on the coordinator diagnostics.py Redacted diagnostics download (device state + coverage metadata) + services.py write_resource/read_resource actions (device resolution, href translation) + services.yaml Selectors/descriptions for the two services above const.py Domain, config keys, probe ports entity.py Base entity wiring capability registry -> HA entity sensor.py / binary_sensor.py / switch.py / number.py / select.py / button.py / time.py / fan.py / climate.py / water_heater.py @@ -172,6 +238,11 @@ email, access tokens, device IDs, MAC addresses, serial numbers) before it's gen directly to a new issue using the linked device-support template. This is the fastest way to help add or expand support for hardware the maintainers don't have. +When a diagnostics dump alone isn't enough to pin down how a resource actually behaves — whether a write sticks, +what order things need to happen in, whether the device reverts a change on its own — the `localthings.write_resource` +and `localthings.read_resource` actions from Part 5 are the tool for probing it directly and reporting back what +you found. + --- ## Adding a new appliance type diff --git a/custom_components/localthings/__init__.py b/custom_components/localthings/__init__.py index c893622..0fa67f8 100644 --- a/custom_components/localthings/__init__.py +++ b/custom_components/localthings/__init__.py @@ -10,14 +10,23 @@ from homeassistant.core import Event, HomeAssistant, callback from homeassistant.exceptions import ConfigEntryNotReady from homeassistant.helpers import device_registry as dr from homeassistant.helpers import entity_registry as er +from homeassistant.helpers.typing import ConfigType from .const import CONF_HOST, CONF_PORT, CONF_SERIAL, DOMAIN, PLATFORMS from .coordinator import LocalThingsCoordinator from .registry.identity import resolve_serial +from .services import async_setup_services _LOGGER = logging.getLogger(__name__) +async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool: + # Services are process-global, registered once here rather than per + # config entry (issue #300) -- see services.async_setup_services. + async_setup_services(hass) + return True + + def _serial_from_unique_id(entry: ConfigEntry) -> str: """The device identity a pre-v2 entry was created with. diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index ae7faa7..e88f1a5 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -20,6 +20,7 @@ import voluptuous as vol from homeassistant import config_entries from homeassistant.config_entries import ConfigFlowResult from homeassistant.core import callback +from homeassistant.helpers import device_registry as dr from homeassistant.helpers.selector import ( NumberSelector, NumberSelectorConfig, @@ -55,6 +56,7 @@ from .const import ( PROBE_GET_TIMEOUT_S, PROBE_MAX_WORKERS, PROBE_PORT_RANGE, + SERVICE_WRITE_RESOURCE, ) _TEXT = TextSelector(TextSelectorConfig(type=TextSelectorType.TEXT)) @@ -925,8 +927,33 @@ class LocalThingsOptionsFlow(config_entries.OptionsFlow): return self._show_debug_edit_form( href, current, {"payload": "empty_payload"}, payload ) + # Goes through the write_resource service (issue #300), not + # coord.async_raw_write directly, so there is exactly one code + # path that performs a raw write. MAIN's own device -- the + # panel's href dropdown already lists actual hrefs off + # coord.last_resources, and MAIN.to_actual is identity, so + # this preserves the panel's existing behavior byte for byte. + dev = dr.async_get(self.hass).async_get_device( + identifiers=coord.device_info["identifiers"] + ) + if dev is None: + return self.async_abort(reason="not_loaded") try: - code, new_rep = await coord.async_raw_write(href, payload) + response = await self.hass.services.async_call( + DOMAIN, + SERVICE_WRITE_RESOURCE, + {"writes": [{"href": href, "payload": payload}]}, + target={"device_id": dev.id}, + blocking=True, + return_response=True, + ) + results = (response or {}).get("results") + first = results[0] if isinstance(results, list) and results else None + raw_code = first.get("raw_code") if isinstance(first, dict) else None + after = first.get("after") if isinstance(first, dict) else None + if not isinstance(raw_code, int) or not isinstance(after, dict): + raise RuntimeError("write_resource service returned an unexpected shape") + code, new_rep = raw_code, after except Exception: _LOGGER.exception("debug raw write failed for %s", href) return self._show_debug_edit_form(href, current, {"base": "write_failed"}, payload) diff --git a/custom_components/localthings/const.py b/custom_components/localthings/const.py index b2b876a..a23b047 100644 --- a/custom_components/localthings/const.py +++ b/custom_components/localthings/const.py @@ -96,3 +96,9 @@ SUMMARY_INTERVAL_S = 30.0 DEVICE_SUPPORT_ISSUE_URL = ( "https://github.com/mbillow/localthings/issues/new?template=device-support.yml" ) + +# Service names (services.py), shared with config_flow.py so the +# options-flow debug panel calls the exact same service a user could call +# from an automation (issue #300) -- one code path performs a raw write. +SERVICE_WRITE_RESOURCE = "write_resource" +SERVICE_READ_RESOURCE = "read_resource" diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 45832de..9c1a09e 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -100,6 +100,60 @@ def _local_source_port(host: str) -> int: return DTLS_LOCAL_PORT_BASE + offset +# Debug raw write/read caps (issue #300) -- generous enough for a real +# probing session (the wall-oven reporter's own sequences run well under +# 10 steps) while bounding how long one service call can hold up polling. +_DEBUG_MAX_WRITES = 10 +_DEBUG_MAX_SETTLE_S = 30.0 +_DEBUG_MAX_VERIFY_AFTER_S = 60.0 + + +def _href_to_path_segs(href: str) -> list[str]: + """'/mode/vs/0' -> ['mode', 'vs', '0'], the shape `sess.get`/`sess.post` + take. Shared by every raw debug read/write path.""" + return [s for s in str(href).strip("/").split("/") if s] + + +def _coap_code_str(code: int) -> str: + """Raw CoAP response code -> its 'C.DD' rendering (e.g. 0x44 -> '2.04'), + the class/detail split RFC 7252 §12.1.2 defines. `raw_code` is kept + alongside it in every debug response so a caller can format it + differently.""" + return f"{code >> 5}.{code & 0x1F:02d}" + + +def _coap_accepted(code: int) -> bool: + """True for a 2.xx class CoAP response.""" + return (code >> 5) == 2 + + +def _validate_debug_write_item(item: dict) -> tuple[list[str], str, dict, float]: + """The checks `async_raw_write` has always applied to a single write + (issue #54), reused per-item by `async_raw_write_sequence` (issue + #300). Payload is checked before href, matching the original + single-write order -- not load-bearing for any test, just avoiding a + silent behavior change in the refactor.""" + payload = item.get("payload") + if not isinstance(payload, dict) or not payload: + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="debug_payload_empty", + ) + path_segs = _href_to_path_segs(item.get("href", "")) + if not path_segs: + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="resource_href_required", + ) + settle = item.get("settle") or 0.0 + if not 0 <= settle <= _DEBUG_MAX_SETTLE_S: + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="debug_settle_out_of_range", + ) + return path_segs, "/" + "/".join(path_segs), payload, settle + + class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): """Manages one Samsung appliance: session, discovery, polling.""" @@ -1119,10 +1173,12 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): await self.async_request_refresh() # ------------------------------------------------------------------ - # Debug raw write (issue #54): a power-user escape hatch for the - # options-flow debug panel to POST an arbitrary partial body without a - # new release. Deliberately bypasses the remote-control block and all - # write_fn/validate_fn above -- use with care. + # Debug raw write/read (issue #54, extended for issue #300): a + # power-user escape hatch shared by the options-flow debug panel and + # the write_resource/read_resource services (services.py) for probing + # a device's write contract directly. Deliberately bypasses the + # remote-control block and all write_fn/validate_fn above -- use with + # care. # ------------------------------------------------------------------ def _raw_write_blocking(self, path_segs: list[str], body: dict, href: str) -> tuple[int, dict]: @@ -1148,17 +1204,34 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._log.debug("raw write follow-up read failed: %s", e) return code, new_rep - async def async_raw_write(self, href: str, body: dict) -> tuple[int, dict]: - """Debug-only arbitrary write (issue #54). Bypasses remote-control - and write_fn/validate_fn; sends `body` verbatim as a partial-rep - PATCH to `href`. Returns (coap_code, new_rep) read back right - after.""" - if not isinstance(body, dict) or not body: - raise ServiceValidationError( - translation_domain=DOMAIN, - translation_key="debug_payload_empty", - ) - path_segs = [s for s in href.strip("/").split("/") if s] + def _raw_read_blocking(self, path_segs: list[str], href: str) -> tuple[int, dict]: + """Debug primitive: a live GET, deliberately bypassing the cache + (issue #300) -- the cache can be up to a poll interval stale, + exactly the staleness that makes testing whether a write held or + got silently reverted by the board unreliable. Blocking -- runs in + executor.""" + if self._session is None: + self._connect_session() + sess = self._session + if sess is None: + raise RuntimeError("no session") + code, payload = sess.get(path_segs, timeout=10.0) + rep: dict = {} + if code == 0x45 and payload: + try: + body = cbor2.loads(payload) + except Exception as e: + self._log.debug("raw read decode failed for %s: %s", href, e) + body = None + if isinstance(body, dict): + self._observe.apply(href, body, source="poll") + rep = body + return code, rep + + async def async_raw_read(self, href: str) -> tuple[int, dict]: + """Debug-only live GET (issue #300, backs the read_resource + service). Same href validation as async_raw_write.""" + path_segs = _href_to_path_segs(href) if not path_segs: raise ServiceValidationError( translation_domain=DOMAIN, @@ -1166,10 +1239,110 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): ) norm_href = "/" + "/".join(path_segs) async with self._session_lock: - code, new_rep = await self.hass.async_add_executor_job( - self._raw_write_blocking, path_segs, body, norm_href + return await self.hass.async_add_executor_job( + self._raw_read_blocking, path_segs, norm_href ) + + async def async_raw_write_sequence( + self, writes: list[dict], *, verify_after: float = 0.0 + ) -> dict[str, Any]: + """Debug-only ordered multi-write (issue #300): a Samsung wall oven + board discards settings writes while idle and only keeps them once + a cycle is already running, which no single-write debug pass can + probe for. This owns the whole sequence -- every write shares one + `_session_lock` hold, so a poll can never interleave mid-sequence + and blur which write is responsible for what the device does next. + + `writes` are already on-the-wire hrefs: subdevice translation + (canonical -> actual) is services.py's job, not this method's -- + this primitive has no notion of subdevices, same as the original + single-write async_raw_write never did. + + `async_raw_write` below delegates here with a one-item sequence, so + its signature/return and tests/test_coordinator_raw_write.py stay + unchanged. + """ + if not writes or len(writes) > _DEBUG_MAX_WRITES: + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="debug_too_many_writes", + ) + if not 0 <= verify_after <= _DEBUG_MAX_VERIFY_AFTER_S: + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="debug_verify_after_out_of_range", + ) + # Validate every item before touching the session -- a rejected + # call must fail before any write goes out (same posture the + # single-write path has always had; see + # test_raw_write_validation_errors_do_not_touch_the_session). + parsed = [_validate_debug_write_item(w) for w in writes] + + results: list[dict[str, Any]] = [] + last_payload_by_href: dict[str, dict] = {} + async with self._session_lock: + for i, (path_segs, href, payload, settle) in enumerate(parsed): + before = self.resource(href) + code, after = await self.hass.async_add_executor_job( + self._raw_write_blocking, path_segs, payload, href + ) + last_payload_by_href[href] = payload + results.append( + { + "href": href, + "code": _coap_code_str(code), + "raw_code": code, + "accepted": _coap_accepted(code), + "before": before, + "after": after, + "changed": all(after.get(k) == v for k, v in payload.items()), + } + ) + # settle is "how long to wait before the next write" -- the + # last item has no next write, so it gets no wait here; + # verify_after (below) is the equivalent wait after the + # sequence as a whole. + if settle and i < len(parsed) - 1: + await asyncio.sleep(settle) + + response: dict[str, Any] = {"results": results} + if verify_after > 0: + # Released, not held, across this wait: holding _session_lock + # through up to 60s would stall the summary poll for that whole + # window (same reasoning as _attempt_observe_mode's grace wait, + # issue #294). A poll interleaving here is harmless -- just + # another read of the same hrefs. + await asyncio.sleep(verify_after) + verified: dict[str, Any] = {} + async with self._session_lock: + for href in dict.fromkeys(r["href"] for r in results): + vcode, vrep = await self.hass.async_add_executor_job( + self._raw_read_blocking, _href_to_path_segs(href), href + ) + verified[href] = { + "code": _coap_code_str(vcode), + "raw_code": vcode, + "rep": vrep, + "held": all( + vrep.get(k) == v for k, v in last_payload_by_href[href].items() + ), + } + response["verified"] = verified + # Hasten a summary poll so entities on other resources catch up - # too -- a debug write can affect siblings, not just its href. + # too -- a debug write can affect siblings, not just its href. Once + # per sequence, not per write: the whole point of ordering writes + # under one lock hold is to control exactly what the device sees + # and when, which a refresh racing in mid-sequence would undermine. await self.async_request_refresh() - return code, new_rep + return response + + async def async_raw_write(self, href: str, body: dict) -> tuple[int, dict]: + """Debug-only arbitrary write (issue #54). Bypasses remote-control + and write_fn/validate_fn; sends `body` verbatim as a partial-rep + PATCH to `href`. Returns (coap_code, new_rep) read back right + after -- a thin single-write wrapper over async_raw_write_sequence + (issue #300).""" + sequence = await self.async_raw_write_sequence([{"href": href, "payload": body}]) + only = sequence["results"][0] + return only["raw_code"], only["after"] diff --git a/custom_components/localthings/services.py b/custom_components/localthings/services.py new file mode 100644 index 0000000..968324f --- /dev/null +++ b/custom_components/localthings/services.py @@ -0,0 +1,209 @@ +"""Home Assistant services for direct OCF resource read/write access +(issue #300): a raw-transport escape hatch for reverse-engineering a +device's write contract -- an ordered multi-write sequence with settle +delays and a delayed re-read, which the single-write options-flow debug +panel can't express. Both sit on the same coordinator primitives the panel +now calls too (config_flow.py), so there is exactly one code path that +performs a raw write. + +Kept thin on purpose: session/lock ownership lives on the coordinator +(coordinator.py). This module only resolves the service call's device +target to a `(coordinator, subdevice)` pair, translates canonical hrefs +through that subdevice, and shapes the response. +""" + +from __future__ import annotations + +from typing import Any, cast + +import voluptuous as vol +from homeassistant.core import HomeAssistant, ServiceCall, ServiceResponse, SupportsResponse +from homeassistant.exceptions import ServiceValidationError +from homeassistant.helpers import config_validation as cv +from homeassistant.helpers import device_registry as dr + +from .const import DOMAIN, SERVICE_READ_RESOURCE, SERVICE_WRITE_RESOURCE +from .coordinator import LocalThingsCoordinator +from .registry.subdevices import MAIN, Subdevice + +ATTR_HREF = "href" +ATTR_PAYLOAD = "payload" +ATTR_SETTLE = "settle" +ATTR_WRITES = "writes" +ATTR_VERIFY_AFTER = "verify_after" +ATTR_DEVICE_ID = "device_id" + +_WRITE_ITEM_SCHEMA = vol.Schema( + { + vol.Required(ATTR_HREF): str, + # Not `dict` here: a non-dict payload must fail the same way an + # empty one does -- coordinator.async_raw_write_sequence's + # ServiceValidationError -- not a raw schema vol.Invalid, so every + # caller sees one consistent error shape regardless of which rule + # a bad payload tripped. + vol.Required(ATTR_PAYLOAD): object, + vol.Optional(ATTR_SETTLE): vol.Coerce(float), + } +) + +# Structural validation only (types, and unwrapping a bare dict into a +# one-item list) -- the semantic checks (non-empty payload, non-root href, +# the 1..10/settle/verify_after ranges) live on +# LocalThingsCoordinator.async_raw_write_sequence, so every caller gets the +# same ServiceValidationError + translation key regardless of whether it +# reached the primitive through this service, the options-flow panel, or a +# future caller. +_WRITE_RESOURCE_SCHEMA = vol.Schema( + { + **cv.TARGET_SERVICE_FIELDS, + vol.Required(ATTR_WRITES): vol.All(cv.ensure_list, [_WRITE_ITEM_SCHEMA]), + vol.Optional(ATTR_VERIFY_AFTER): vol.Coerce(float), + } +) + +_READ_RESOURCE_SCHEMA = vol.Schema( + { + **cv.TARGET_SERVICE_FIELDS, + vol.Optional(ATTR_HREF): str, + } +) + + +def _resolve_target( + hass: HomeAssistant, call: ServiceCall +) -> tuple[LocalThingsCoordinator, Subdevice, str]: + """The one `(coordinator, subdevice, device_id)` a service call's + device target names. + + Deliberately strict about count, not just presence: the `target: + device:` selector in services.yaml still lets a user pick an area or + label in the picker, and the frontend expands that into a `device_id` + list before the call reaches here -- more than one entry means an + area/label fanned this out across several appliances, which a raw + debug write must never do silently (issue #300). + """ + device_ids = cv.ensure_list(call.data.get(ATTR_DEVICE_ID) or []) + if len(device_ids) != 1: + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="service_device_target_invalid", + ) + device_id = device_ids[0] + + dev_reg = dr.async_get(hass) + device = dev_reg.async_get(device_id) + if device is None: + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="service_device_not_found", + ) + + for coordinator in hass.data.get(DOMAIN, {}).values(): + if device.identifiers & coordinator.device_info.get("identifiers", set()): + return coordinator, MAIN, device_id + for sub in coordinator.subdevices: + if device.identifiers & coordinator.device_info_for(sub).get("identifiers", set()): + return coordinator, sub, device_id + + raise ServiceValidationError( + translation_domain=DOMAIN, + translation_key="service_device_not_loaded", + ) + + +async def _async_write_resource(hass: HomeAssistant, call: ServiceCall) -> ServiceResponse: + coordinator, subdevice, device_id = _resolve_target(hass, call) + writes_in: list[dict[str, Any]] = call.data[ATTR_WRITES] + + # Canonical -> actual translation happens here, not in the coordinator + # (issue #177): the coordinator's raw-write primitive has no notion of + # subdevices, only the hrefs it's told to hit -- identity transform for + # MAIN, so a device targeting MAIN behaves exactly as it always has. + raw_writes = [ + { + "href": subdevice.to_actual(w[ATTR_HREF]), + "payload": w.get(ATTR_PAYLOAD), + "settle": w.get(ATTR_SETTLE, 0.0), + } + for w in writes_in + ] + sequence = await coordinator.async_raw_write_sequence( + raw_writes, verify_after=call.data.get(ATTR_VERIFY_AFTER, 0.0) + ) + + results = [ + { + "href": original[ATTR_HREF], + "actual_href": result["href"], + "code": result["code"], + "raw_code": result["raw_code"], + "accepted": result["accepted"], + "before": result["before"], + "after": result["after"], + "changed": result["changed"], + } + for original, result in zip(writes_in, sequence["results"], strict=True) + ] + response: dict[str, Any] = {"device_id": device_id, "results": results} + if "verified" in sequence: + canonical_by_actual = {subdevice.to_actual(w[ATTR_HREF]): w[ATTR_HREF] for w in writes_in} + response["verified"] = { + canonical_by_actual.get(actual_href, actual_href): verified + for actual_href, verified in sequence["verified"].items() + } + return response + + +async def _async_read_resource(hass: HomeAssistant, call: ServiceCall) -> ServiceResponse: + coordinator, subdevice, _device_id = _resolve_target(hass, call) + href = call.data.get(ATTR_HREF) + if not href: + # No href -> the cached snapshot, not a live sweep of every known + # href: lets a user enumerate what exists without hammering the + # device (see this module's docstring and the coordinator's + # canonical_resources). + snapshot: dict[str, Any] = {"resources": coordinator.canonical_resources(subdevice)} + return cast(ServiceResponse, snapshot) + + actual_href = subdevice.to_actual(href) + code, rep = await coordinator.async_raw_read(actual_href) + read_result: dict[str, Any] = { + "href": href, + "actual_href": actual_href, + "code": f"{code >> 5}.{code & 0x1F:02d}", + "raw_code": code, + "rep": rep, + } + return cast(ServiceResponse, read_result) + + +def async_setup_services(hass: HomeAssistant) -> None: + """Register the write_resource/read_resource services (issue #300). + + Called once from `async_setup`, not per config entry: services are + process-global, and `hass.services.async_register` on an + already-registered name just replaces the handler, so re-registering + on every entry setup would silently rebind to whichever entry loaded + last. `async_unload_entry` must never call the inverse of this. + """ + + async def _handle_write(call: ServiceCall) -> ServiceResponse: + return await _async_write_resource(hass, call) + + async def _handle_read(call: ServiceCall) -> ServiceResponse: + return await _async_read_resource(hass, call) + + hass.services.async_register( + DOMAIN, + SERVICE_WRITE_RESOURCE, + _handle_write, + schema=_WRITE_RESOURCE_SCHEMA, + supports_response=SupportsResponse.OPTIONAL, + ) + hass.services.async_register( + DOMAIN, + SERVICE_READ_RESOURCE, + _handle_read, + schema=_READ_RESOURCE_SCHEMA, + supports_response=SupportsResponse.ONLY, + ) diff --git a/custom_components/localthings/services.yaml b/custom_components/localthings/services.yaml new file mode 100644 index 0000000..5fe0cb5 --- /dev/null +++ b/custom_components/localthings/services.yaml @@ -0,0 +1,67 @@ +write_resource: + name: Write resource + description: >- + Send one or more raw partial-rep writes straight to a device's OCF + resources, in order, with an optional settle delay between steps and a + delayed re-read at the end -- for reverse-engineering a device's write + contract (issue #300), not day-to-day control. This bypasses the + remote-control-off block and every write_fn/validate_fn a normal entity + write goes through, and sends exactly the fields you give it verbatim: + it can misconfigure your appliance. Prefer a real entity, or the Debug + write panel in the integration's Configure menu, for anything this + integration already models. + target: + device: + integration: localthings + fields: + writes: + name: Writes + description: >- + 1-10 writes to perform in order. Each item needs href (the + canonical resource, e.g. /mode/vs/0) and payload (a non-empty + object sent verbatim as a partial-rep POST); settle is how many + seconds to wait after that write before starting the next one + (0-30, default 0). + required: true + example: >- + [{"href": "/mode/vs/0", "payload": {"x.com.samsung.da.mode": + "Bake"}, "settle": 3}] + selector: + object: + verify_after: + name: Verify after + description: >- + Seconds to wait after the whole sequence finishes before + re-reading every href touched, to see whether the values held or + were reverted by the board. 0 (default) skips verification. + required: false + default: 0 + selector: + number: + min: 0 + max: 60 + step: 0.5 + unit_of_measurement: seconds + mode: box + +read_resource: + name: Read resource + description: >- + Read a device's OCF resources directly, bypassing this integration's + entity model. Give an href for a live GET straight from the device -- + deliberately not the cache, which can be up to a poll interval stale -- + or omit it to get the cached snapshot of every resource this + integration currently tracks on that device. + target: + device: + integration: localthings + fields: + href: + name: Resource href + description: >- + Canonical resource href to read (e.g. /mode/vs/0). Omit to get the + cached snapshot of every tracked resource instead of a live GET. + required: false + example: /mode/vs/0 + selector: + text: diff --git a/custom_components/localthings/translations/cs.json b/custom_components/localthings/translations/cs.json index 3aa10d4..6c059c5 100644 --- a/custom_components/localthings/translations/cs.json +++ b/custom_components/localthings/translations/cs.json @@ -1417,6 +1417,24 @@ }, "command_failed": { "message": "Příkaz pro {href} selhal i po opětovném připojení: {error}" + }, + "debug_too_many_writes": { + "message": "Zadejte 1 až 10 zápisů." + }, + "debug_settle_out_of_range": { + "message": "Hodnota settle musí být mezi 0 a 30 sekundami." + }, + "debug_verify_after_out_of_range": { + "message": "Hodnota verify_after musí být mezi 0 a 60 sekundami." + }, + "service_device_target_invalid": { + "message": "Tato služba vyžaduje přesně jedno cílové zařízení." + }, + "service_device_not_found": { + "message": "Pro tento cíl nebylo nalezeno žádné odpovídající zařízení." + }, + "service_device_not_loaded": { + "message": "Toto zařízení ještě není připojeno. Zkuste to znovu, až se načte." } } } diff --git a/custom_components/localthings/translations/en.json b/custom_components/localthings/translations/en.json index a935f0f..490746a 100644 --- a/custom_components/localthings/translations/en.json +++ b/custom_components/localthings/translations/en.json @@ -1417,6 +1417,24 @@ }, "command_failed": { "message": "The command to {href} failed even after reconnecting: {error}" + }, + "debug_too_many_writes": { + "message": "Provide between 1 and 10 writes." + }, + "debug_settle_out_of_range": { + "message": "settle must be between 0 and 30 seconds." + }, + "debug_verify_after_out_of_range": { + "message": "verify_after must be between 0 and 60 seconds." + }, + "service_device_target_invalid": { + "message": "This service requires exactly one target device." + }, + "service_device_not_found": { + "message": "No matching device was found for that target." + }, + "service_device_not_loaded": { + "message": "This device isn't connected yet. Try again once it has loaded." } } } diff --git a/custom_components/localthings/translations/es.json b/custom_components/localthings/translations/es.json index 86afd5f..f9129c6 100644 --- a/custom_components/localthings/translations/es.json +++ b/custom_components/localthings/translations/es.json @@ -122,6 +122,24 @@ }, "command_failed": { "message": "El comando para {href} falló incluso después de reconectar: {error}" + }, + "debug_too_many_writes": { + "message": "Proporciona entre 1 y 10 escrituras." + }, + "debug_settle_out_of_range": { + "message": "settle debe estar entre 0 y 30 segundos." + }, + "debug_verify_after_out_of_range": { + "message": "verify_after debe estar entre 0 y 60 segundos." + }, + "service_device_target_invalid": { + "message": "Este servicio requiere exactamente un dispositivo de destino." + }, + "service_device_not_found": { + "message": "No se encontró ningún dispositivo coincidente para ese destino." + }, + "service_device_not_loaded": { + "message": "Este dispositivo aún no está conectado. Vuelve a intentarlo cuando se haya cargado." } }, "entity": { diff --git a/custom_components/localthings/translations/it.json b/custom_components/localthings/translations/it.json index 28289a1..7a13971 100644 --- a/custom_components/localthings/translations/it.json +++ b/custom_components/localthings/translations/it.json @@ -1417,6 +1417,24 @@ }, "command_failed": { "message": "Il comando per {href} è fallito anche dopo la riconnessione: {error}" + }, + "debug_too_many_writes": { + "message": "Specificare da 1 a 10 scritture." + }, + "debug_settle_out_of_range": { + "message": "settle deve essere compreso tra 0 e 30 secondi." + }, + "debug_verify_after_out_of_range": { + "message": "verify_after deve essere compreso tra 0 e 60 secondi." + }, + "service_device_target_invalid": { + "message": "Questo servizio richiede esattamente un dispositivo di destinazione." + }, + "service_device_not_found": { + "message": "Nessun dispositivo corrispondente trovato per quella destinazione." + }, + "service_device_not_loaded": { + "message": "Questo dispositivo non è ancora connesso. Riprova una volta caricato." } } } diff --git a/custom_components/localthings/translations/ko.json b/custom_components/localthings/translations/ko.json index 2fdc93f..ae2ab7b 100644 --- a/custom_components/localthings/translations/ko.json +++ b/custom_components/localthings/translations/ko.json @@ -1417,6 +1417,24 @@ }, "command_failed": { "message": "재연결 후에도 {href} 명령이 실패했습니다: {error}" + }, + "debug_too_many_writes": { + "message": "1~10개의 쓰기를 지정하세요." + }, + "debug_settle_out_of_range": { + "message": "settle 값은 0에서 30초 사이여야 합니다." + }, + "debug_verify_after_out_of_range": { + "message": "verify_after 값은 0에서 60초 사이여야 합니다." + }, + "service_device_target_invalid": { + "message": "이 서비스는 정확히 하나의 대상 기기가 필요합니다." + }, + "service_device_not_found": { + "message": "해당 대상에 일치하는 기기를 찾을 수 없습니다." + }, + "service_device_not_loaded": { + "message": "이 기기는 아직 연결되지 않았습니다. 로드된 후 다시 시도하세요." } } } diff --git a/custom_components/localthings/translations/nl.json b/custom_components/localthings/translations/nl.json index ed23a30..d335ab3 100644 --- a/custom_components/localthings/translations/nl.json +++ b/custom_components/localthings/translations/nl.json @@ -1417,6 +1417,24 @@ }, "command_failed": { "message": "Het commando naar {href} is ook na opnieuw verbinden mislukt: {error}" + }, + "debug_too_many_writes": { + "message": "Geef tussen de 1 en 10 schrijfacties op." + }, + "debug_settle_out_of_range": { + "message": "settle moet tussen 0 en 30 seconden liggen." + }, + "debug_verify_after_out_of_range": { + "message": "verify_after moet tussen 0 en 60 seconden liggen." + }, + "service_device_target_invalid": { + "message": "Deze service vereist precies één doelapparaat." + }, + "service_device_not_found": { + "message": "Er is geen bijpassend apparaat gevonden voor dat doel." + }, + "service_device_not_loaded": { + "message": "Dit apparaat is nog niet verbonden. Probeer het opnieuw zodra het is geladen." } } } diff --git a/tests/localthings/test_config_flow.py b/tests/localthings/test_config_flow.py index 55ddad2..ad265d4 100644 --- a/tests/localthings/test_config_flow.py +++ b/tests/localthings/test_config_flow.py @@ -1036,9 +1036,10 @@ async def test_options_flow_debug_edit_writes_and_shows_result( hass: HomeAssistant, mock_coordinator_session, ) -> None: - """Picking an href, then submitting a payload, drives - coordinator.async_raw_write and lands on the result menu with the - device's response.""" + """Picking an href, then submitting a payload, calls the write_resource + service (issue #300) -- which drives + coordinator.async_raw_write_sequence -- and lands on the result menu + with the device's response.""" entry = MockConfigEntry(domain=DOMAIN, data=ENTRY_DATA, unique_id=f"localthings_{MOCK_SERIAL}") entry.add_to_hass(hass) await hass.config_entries.async_setup(entry.entry_id) @@ -1056,8 +1057,20 @@ async def test_options_flow_debug_edit_writes_and_shows_result( assert result["step_id"] == "debug_edit" with patch( - "custom_components.localthings.coordinator.LocalThingsCoordinator.async_raw_write", - return_value=(0x44, {"a": 1}), + "custom_components.localthings.coordinator.LocalThingsCoordinator.async_raw_write_sequence", + return_value={ + "results": [ + { + "href": "/washer/vs/0", + "code": "2.04", + "raw_code": 0x44, + "accepted": True, + "before": {}, + "after": {"a": 1}, + "changed": True, + } + ] + }, ): result = await hass.config_entries.options.async_configure( result["flow_id"], @@ -1123,8 +1136,20 @@ async def test_options_flow_finish_preserves_existing_options( user_input={"href": "/washer/vs/0"}, ) with patch( - "custom_components.localthings.coordinator.LocalThingsCoordinator.async_raw_write", - return_value=(0x44, {"a": 1}), + "custom_components.localthings.coordinator.LocalThingsCoordinator.async_raw_write_sequence", + return_value={ + "results": [ + { + "href": "/washer/vs/0", + "code": "2.04", + "raw_code": 0x44, + "accepted": True, + "before": {}, + "after": {"a": 1}, + "changed": True, + } + ] + }, ): result = await hass.config_entries.options.async_configure( result["flow_id"], diff --git a/tests/test_services.py b/tests/test_services.py new file mode 100644 index 0000000..4f63d9c --- /dev/null +++ b/tests/test_services.py @@ -0,0 +1,458 @@ +"""Tests for the write_resource/read_resource services (issue #300) and the +options-flow debug panel now that it goes through write_resource instead of +calling coordinator.async_raw_write directly (config_flow.py). See +tests/test_coordinator_raw_write.py for the underlying single-write +primitive's own session/validation tests -- these focus on the service +layer: device target resolution, subdevice href translation, ordered +multi-write sequencing, settle/verify_after, and the options-flow rewiring. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, patch + +import cbor2 +import pytest +from homeassistant.core import HomeAssistant +from homeassistant.exceptions import ServiceValidationError +from homeassistant.helpers import device_registry as dr +from pytest_homeassistant_custom_component.common import MockConfigEntry + +from custom_components.localthings.config_flow import LocalThingsOptionsFlow +from custom_components.localthings.const import ( + CONF_HOST, + CONF_LEAF_CERT_PEM, + CONF_LEAF_KEY_PEM, + CONF_PORT, + DOMAIN, + SERVICE_READ_RESOURCE, + SERVICE_WRITE_RESOURCE, +) +from custom_components.localthings.coordinator import LocalThingsCoordinator +from custom_components.localthings.registry.subdevices import Subdevice +from custom_components.localthings.services import async_setup_services + +ENTRY_DATA = { + CONF_HOST: "10.0.0.199", + CONF_PORT: 49154, + CONF_LEAF_CERT_PEM: "-----BEGIN CERTIFICATE-----\nTEST-LEAF\n-----END CERTIFICATE-----", + CONF_LEAF_KEY_PEM: "-----BEGIN PRIVATE KEY-----\nTEST-LEAF-KEY\n-----END PRIVATE KEY-----", +} + +_SLEEP_TARGET = "custom_components.localthings.coordinator.asyncio.sleep" + + +class _FakeSession: + """Stand-in for DtlsCoapSession: records every POST verbatim and answers + GET from a per-href queue of canned representations, so a test can model + a value that changes across successive reads of the same href (the + write's own follow-up GET vs. a later verify_after re-read) -- no real + DTLS/network involved. Modeled on test_coordinator_raw_write.py's + _FakeRawWriteSession, extended for multi-step sequences.""" + + def __init__(self, post_code: int = 0x44): + self.post_calls: list[tuple[list[str], bytes]] = [] + self.get_calls: list[list[str]] = [] + self._post_code = post_code + self._get_reps: dict[str, list[dict]] = {} + + def queue_get(self, href: str, rep: dict) -> None: + """Queue one more canned rep for `href`'s next GET. Once an href's + queue is down to one entry, that entry keeps answering every + further GET -- a test only needs to queue the values that + actually change across calls.""" + self._get_reps.setdefault(href.strip("/"), []).append(rep) + + def post(self, path_segs, payload, timeout=None): + self.post_calls.append((list(path_segs), payload)) + return self._post_code, b"" + + def get(self, path_segs, timeout=None): + self.get_calls.append(list(path_segs)) + key = "/".join(path_segs) + queue = self._get_reps.get(key) + if not queue: + return 0x45, cbor2.dumps({}) + rep = queue.pop(0) if len(queue) > 1 else queue[0] + return 0x45, cbor2.dumps(rep) + + def pace(self): + pass + + +@pytest.fixture +def coordinator(hass: HomeAssistant) -> LocalThingsCoordinator: + entry = MockConfigEntry( + domain=DOMAIN, + data=ENTRY_DATA, + unique_id="localthings_SERVICES-TEST", + ) + entry.add_to_hass(hass) + coord = LocalThingsCoordinator(hass, entry) + # async_raw_write_sequence kicks a refresh after the write; a real + # refresh would try to poll a session that doesn't exist for this unit + # test, so replace it with a no-op the same way + # test_coordinator_raw_write.py does. + coord.async_request_refresh = AsyncMock() + hass.data.setdefault(DOMAIN, {})[entry.entry_id] = coord + async_setup_services(hass) + return coord + + +@pytest.fixture +def device_id(hass: HomeAssistant, coordinator: LocalThingsCoordinator) -> str: + """Register MAIN's HA device under the same identifiers a real config + entry setup would have used (coordinator.device_info).""" + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=coordinator._entry.entry_id, + identifiers=coordinator.device_info["identifiers"], + ) + return device.id + + +async def _call_write(hass: HomeAssistant, device: str, **data): + return await hass.services.async_call( + DOMAIN, + SERVICE_WRITE_RESOURCE, + data, + target={"device_id": device}, + blocking=True, + return_response=True, + ) + + +async def _call_read(hass: HomeAssistant, device: str, **data): + return await hass.services.async_call( + DOMAIN, + SERVICE_READ_RESOURCE, + data, + target={"device_id": device}, + blocking=True, + return_response=True, + ) + + +# ---------------------------------------------------------------------- +# write_resource: ordered multi-write sequencing +# ---------------------------------------------------------------------- + + +async def test_write_resource_posts_in_order_with_exact_bodies(hass, coordinator, device_id): + fake = _FakeSession(post_code=0x44) + coordinator._session = fake + + writes = [ + {"href": "/mode/vs/0", "payload": {"a": 1}}, + {"href": "/washer/vs/0", "payload": {"b": 2}}, + {"href": "/mode/vs/0", "payload": {"a": 3}}, + ] + response = await _call_write(hass, device_id, writes=writes) + + assert [path for path, _ in fake.post_calls] == [ + ["mode", "vs", "0"], + ["washer", "vs", "0"], + ["mode", "vs", "0"], + ] + assert [cbor2.loads(body) for _, body in fake.post_calls] == [ + {"a": 1}, + {"b": 2}, + {"a": 3}, + ] + assert response is not None + assert response["device_id"] == device_id + assert len(response["results"]) == 3 + for result, write in zip(response["results"], writes, strict=True): + assert result["href"] == write["href"] + assert result["actual_href"] == write["href"] # MAIN -> identity transform + assert result["raw_code"] == 0x44 + assert result["code"] == "2.04" + assert result["accepted"] is True + coordinator.async_request_refresh.assert_awaited_once() + + +async def test_write_resource_settle_honored_between_writes(hass, coordinator, device_id): + """settle waits *before the next write*, not after the last one -- + asserted on the recorded durations rather than a real sleep.""" + fake = _FakeSession() + coordinator._session = fake + + writes = [ + {"href": "/a/vs/0", "payload": {"x": 1}, "settle": 2}, + {"href": "/b/vs/0", "payload": {"x": 2}, "settle": 5}, + {"href": "/c/vs/0", "payload": {"x": 3}, "settle": 9}, + ] + with patch(_SLEEP_TARGET, new_callable=AsyncMock) as mock_sleep: + await _call_write(hass, device_id, writes=writes) + + assert [call.args[0] for call in mock_sleep.call_args_list] == [2.0, 5.0] + + +async def test_write_resource_changed_true_when_readback_matches_payload( + hass, coordinator, device_id +): + fake = _FakeSession() + fake.queue_get("mode/vs/0", {"x.field": "target"}) + coordinator._session = fake + + response = await _call_write( + hass, device_id, writes=[{"href": "/mode/vs/0", "payload": {"x.field": "target"}}] + ) + + assert response["results"][0]["changed"] is True + + +async def test_write_resource_changed_false_when_readback_differs(hass, coordinator, device_id): + fake = _FakeSession() + fake.queue_get("mode/vs/0", {"x.field": "unchanged"}) + coordinator._session = fake + + response = await _call_write( + hass, device_id, writes=[{"href": "/mode/vs/0", "payload": {"x.field": "target"}}] + ) + + assert response["results"][0]["changed"] is False + + +# ---------------------------------------------------------------------- +# verify_after: the delayed re-read this feature exists for (issue #300) +# ---------------------------------------------------------------------- + + +async def test_write_resource_verify_after_reports_held(hass, coordinator, device_id): + fake = _FakeSession() + fake.queue_get("mode/vs/0", {"x.field": "target"}) # write's own follow-up read + fake.queue_get("mode/vs/0", {"x.field": "target"}) # verify_after re-read: held + coordinator._session = fake + + with patch(_SLEEP_TARGET, new_callable=AsyncMock) as mock_sleep: + response = await _call_write( + hass, + device_id, + writes=[{"href": "/mode/vs/0", "payload": {"x.field": "target"}}], + verify_after=30, + ) + + assert 30.0 in [call.args[0] for call in mock_sleep.call_args_list] + verified = response["verified"]["/mode/vs/0"] + assert verified["held"] is True + assert verified["rep"] == {"x.field": "target"} + + +async def test_write_resource_verify_after_reports_reverted(hass, coordinator, device_id): + """Issue #300's own symptom: a Samsung wall oven board answers 2.04 + Changed to a settings write while idle and then silently reverts it + once the follow-up read has already come back clean. verify_after's + whole purpose is catching exactly this -- `changed` (immediate) and + `held` (delayed) must be able to disagree.""" + fake = _FakeSession() + fake.queue_get("mode/vs/0", {"x.field": "target"}) # looks accepted right after the write + fake.queue_get("mode/vs/0", {"x.field": "original"}) # reverted by the time verify_after fires + coordinator._session = fake + + with patch(_SLEEP_TARGET, new_callable=AsyncMock): + response = await _call_write( + hass, + device_id, + writes=[{"href": "/mode/vs/0", "payload": {"x.field": "target"}}], + verify_after=30, + ) + + assert response["results"][0]["changed"] is True + verified = response["verified"]["/mode/vs/0"] + assert verified["held"] is False + assert verified["rep"] == {"x.field": "original"} + + +async def test_write_resource_no_verified_key_when_verify_after_is_zero( + hass, coordinator, device_id +): + fake = _FakeSession() + coordinator._session = fake + + response = await _call_write( + hass, device_id, writes=[{"href": "/mode/vs/0", "payload": {"x": 1}}] + ) + + assert "verified" not in response + + +# ---------------------------------------------------------------------- +# Device target resolution +# ---------------------------------------------------------------------- + + +async def test_write_resource_zero_devices_rejected(hass, coordinator): + with pytest.raises(ServiceValidationError): + await hass.services.async_call( + DOMAIN, + SERVICE_WRITE_RESOURCE, + {"writes": [{"href": "/a/vs/0", "payload": {"x": 1}}]}, + blocking=True, + return_response=True, + ) + + +async def test_write_resource_two_devices_rejected(hass, coordinator, device_id): + with pytest.raises(ServiceValidationError): + await hass.services.async_call( + DOMAIN, + SERVICE_WRITE_RESOURCE, + {"writes": [{"href": "/a/vs/0", "payload": {"x": 1}}]}, + target={"device_id": [device_id, "some-other-unrelated-device-id"]}, + blocking=True, + return_response=True, + ) + + +async def test_write_resource_unresolvable_device_rejected(hass, coordinator): + """The device exists in the registry but no loaded coordinator claims + it -- e.g. a device belonging to a different integration entirely.""" + dev_reg = dr.async_get(hass) + other_entry = MockConfigEntry(domain="other_domain") + other_entry.add_to_hass(hass) + device = dev_reg.async_get_or_create( + config_entry_id=other_entry.entry_id, + identifiers={("other_domain", "unrelated")}, + ) + with pytest.raises(ServiceValidationError): + await _call_write(hass, device.id, writes=[{"href": "/a/vs/0", "payload": {"x": 1}}]) + + +# ---------------------------------------------------------------------- +# Subdevice href translation (issue #177) +# ---------------------------------------------------------------------- + + +async def test_write_resource_translates_href_for_indexed_subdevice(hass, coordinator): + sub = Subdevice(kind="indexed", key="1", seed_path=("device", "1")) + coordinator.subdevices = [sub] + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=coordinator._entry.entry_id, + identifiers=coordinator.device_info_for(sub)["identifiers"], + ) + fake = _FakeSession() + coordinator._session = fake + + response = await _call_write( + hass, device.id, writes=[{"href": "/mode/vs/0", "payload": {"x": 1}}] + ) + + assert fake.post_calls[0][0] == ["mode", "vs", "1"] + result = response["results"][0] + assert result["href"] == "/mode/vs/0" + assert result["actual_href"] == "/mode/vs/1" + + +# ---------------------------------------------------------------------- +# Validation caps +# ---------------------------------------------------------------------- + + +async def test_write_resource_rejects_more_than_ten_writes(hass, coordinator, device_id): + writes = [{"href": f"/x/vs/{i}", "payload": {"a": i}} for i in range(11)] + with pytest.raises(ServiceValidationError): + await _call_write(hass, device_id, writes=writes) + + +async def test_write_resource_rejects_settle_above_thirty_seconds(hass, coordinator, device_id): + with pytest.raises(ServiceValidationError): + await _call_write( + hass, + device_id, + writes=[{"href": "/a/vs/0", "payload": {"x": 1}, "settle": 31}], + ) + + +async def test_write_resource_rejects_empty_payload(hass, coordinator, device_id): + with pytest.raises(ServiceValidationError): + await _call_write(hass, device_id, writes=[{"href": "/a/vs/0", "payload": {}}]) + + +async def test_write_resource_rejects_non_dict_payload(hass, coordinator, device_id): + with pytest.raises(ServiceValidationError): + await _call_write( + hass, device_id, writes=[{"href": "/a/vs/0", "payload": ["not", "a", "dict"]}] + ) + + +async def test_write_resource_rejects_empty_href(hass, coordinator, device_id): + with pytest.raises(ServiceValidationError): + await _call_write(hass, device_id, writes=[{"href": "", "payload": {"x": 1}}]) + + +# ---------------------------------------------------------------------- +# read_resource +# ---------------------------------------------------------------------- + + +async def test_read_resource_with_href_does_live_get(hass, coordinator, device_id): + fake = _FakeSession() + fake.queue_get("mode/vs/0", {"x.field": "live"}) + coordinator._session = fake + + response = await _call_read(hass, device_id, href="/mode/vs/0") + + assert fake.get_calls == [["mode", "vs", "0"]] + assert response["href"] == "/mode/vs/0" + assert response["actual_href"] == "/mode/vs/0" + assert response["rep"] == {"x.field": "live"} + + +async def test_read_resource_without_href_returns_cached_snapshot_and_does_not_get( + hass, coordinator, device_id +): + fake = _FakeSession() + coordinator._session = fake + # Seed the cache the way a poll would -- the fake session's `get` is + # never touched by this test. + coordinator._observe.apply("/mode/vs/0", {"x.field": "cached"}, source="poll") + + response = await _call_read(hass, device_id) + + assert fake.get_calls == [] + assert response["resources"]["/mode/vs/0"] == {"x.field": "cached"} + + +# ---------------------------------------------------------------------- +# Options-flow debug panel, rewired onto write_resource (issue #300) +# ---------------------------------------------------------------------- + + +async def test_options_flow_debug_write_goes_through_write_resource_service( + hass, coordinator, device_id +): + """Drives LocalThingsOptionsFlow's real step methods directly (rather + than through hass.config_entries.options.async_init/async_configure, + which needs the integration resolvable through HA's loader -- not + exercised by any test in this repo and orthogonal to what this test is + actually checking): the panel's async_step_debug_edit must call the + write_resource service and get back a result the rest of the flow can + render, not coord.async_raw_write directly.""" + fake = _FakeSession(post_code=0x44) + fake.queue_get("course/vs/0", {"x.field": "after"}) + coordinator._session = fake + + flow = LocalThingsOptionsFlow() + flow.hass = hass + flow.handler = coordinator._entry.entry_id + + write_result = await flow.async_step_debug_write({"href": "/course/vs/0"}) + assert write_result["step_id"] == "debug_edit" + + edit_result = await flow.async_step_debug_edit({"payload": {"x.com.samsung.da.field": "value"}}) + assert edit_result["step_id"] == "debug_result" + placeholders = edit_result["description_placeholders"] + assert placeholders is not None + assert placeholders["code"] == "2.04 (0x44)" + + assert len(fake.post_calls) == 1 + posted_path, posted_body = fake.post_calls[0] + assert posted_path == ["course", "vs", "0"] + assert cbor2.loads(posted_body) == {"x.com.samsung.da.field": "value"} + + assert len(fake.post_calls) == 1 + posted_path, posted_body = fake.post_calls[0] + assert posted_path == ["course", "vs", "0"] + assert cbor2.loads(posted_body) == {"x.com.samsung.da.field": "value"} From cec3dd4a68ba1e09ac97b5bd04aaa6066ad72363 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 7 Aug 2026 22:58:37 +0000 Subject: [PATCH 2/5] docs: use real field shapes in the write_resource examples The README's worked example and services.yaml's field example both wrote `x.com.samsung.da.mode: "Bake"` to /mode/vs/0 -- singular, and a bare string. That resource takes `modes` as an array (issue #300's own dump shows `["NoOperation"]`), so both examples were a shape the device would have ignored, in the one place a user is most likely to copy from. The README's other two steps were invented the same way; replaced with the mode -> state: Run sequence issue #300 is actually trying to prove out. Also adds a short note that payloads go out verbatim, so field names and types have to match what the resource really uses, pointing at read_resource with no href as the way to check first -- and aligns the two new Repo layout rows with the column their neighbors use. --- README.md | 20 +++++++++----------- custom_components/localthings/services.yaml | 4 ++-- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 54ea6be..11fa0b6 100644 --- a/README.md +++ b/README.md @@ -112,20 +112,18 @@ target: device_id: abc123... data: writes: - - href: /course/vs/0 - payload: - x.com.samsung.da.course: "01" - settle: 3 - href: /mode/vs/0 payload: - x.com.samsung.da.mode: Bake + x.com.samsung.da.modes: ["Bake"] settle: 5 - - href: /power/vs/0 + - href: /operational/state/vs/0 payload: - x.com.samsung.da.power: "On" + x.com.samsung.da.state: "Run" verify_after: 30 ``` +Mind the shapes: what you write is sent verbatim, so the field names and types have to be the ones that resource actually uses. `/mode/vs/0` takes `modes` as an *array* on this board; a bare string, or the singular `mode`, is a different field the device will simply ignore. `read_resource` (below) with no `href` is the quickest way to see the real shape of everything before you write to any of it. + Each write in `writes` (1-10 of them) needs `href` and a non-empty `payload`, sent verbatim as a partial-rep POST — this bypasses the remote-control-off block and every `write_fn`/`validate_fn` a normal entity write goes through, and sends exactly the fields you give it, so it can misconfigure your appliance if you get it wrong. `settle` (0-30s, default 0) is how long to wait *after* that write before starting the next one. The whole sequence runs under a single lock, so a routine poll can't land in the middle of it and blur which write is responsible for what the device does next. The response has one `results` entry per write, with `before`/`after` reps and a `changed` flag (every key/value in `payload` present and equal in the immediate readback): @@ -134,12 +132,12 @@ The response has one `results` entry per write, with `before`/`after` reps and a { "device_id": "abc123...", "results": [ - {"href": "/course/vs/0", "actual_href": "/course/vs/0", "code": "2.04", "raw_code": 68, + {"href": "/mode/vs/0", "actual_href": "/mode/vs/0", "code": "2.04", "raw_code": 68, "accepted": true, "before": {...}, "after": {...}, "changed": true}, ... ], "verified": { - "/power/vs/0": {"code": "2.05", "raw_code": 69, "rep": {...}, "held": false} + "/mode/vs/0": {"code": "2.05", "raw_code": 69, "rep": {...}, "held": false} } } ``` @@ -199,8 +197,8 @@ custom_components/localthings/ coordinator.py Polling + push update coordination, stale-state fallback, write dispatch observe.py CoAP OBSERVE (push-mode) support layered on the coordinator diagnostics.py Redacted diagnostics download (device state + coverage metadata) - services.py write_resource/read_resource actions (device resolution, href translation) - services.yaml Selectors/descriptions for the two services above + services.py write_resource/read_resource actions (device resolution, href translation) + services.yaml Selectors/descriptions for the two services above const.py Domain, config keys, probe ports entity.py Base entity wiring capability registry -> HA entity sensor.py / binary_sensor.py / switch.py / number.py / select.py / button.py / time.py / fan.py / climate.py / water_heater.py diff --git a/custom_components/localthings/services.yaml b/custom_components/localthings/services.yaml index 5fe0cb5..4afcbdb 100644 --- a/custom_components/localthings/services.yaml +++ b/custom_components/localthings/services.yaml @@ -24,8 +24,8 @@ write_resource: (0-30, default 0). required: true example: >- - [{"href": "/mode/vs/0", "payload": {"x.com.samsung.da.mode": - "Bake"}, "settle": 3}] + [{"href": "/mode/vs/0", "payload": {"x.com.samsung.da.modes": + ["Bake"]}, "settle": 3}] selector: object: verify_after: From a6d818dfc0eef92c27acac6f0f0a9c29b07dbcb5 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 7 Aug 2026 22:58:37 +0000 Subject: [PATCH 3/5] Fix four review findings in the raw write/read services - services.py: normalize an href before handing it to Subdevice.to_actual. That transform is textual and rewrites only a trailing '0' segment, so '/mode/vs/0/' passed through it untouched and normalized downstream to the master's '/mode/vs/0' -- landing the write on the wrong oven cavity while still answering 2.04, with nothing in the response to give it away. Same order now on the read path. - services.py: key `verified` off those same normalized canonicals. It was built from un-normalized to_actual output against the coordinator's normalized hrefs, so a non-canonical input missed the lookup and handed back actual hrefs where the documented contract promises canonical ones. - coordinator.py: report `held: None` when the verify re-read itself didn't come back. A non-2.05 yields an empty rep, against which every payload comparison is False, so a 4.04 or dropped read was reported as `held: false` -- indistinguishable from the board reverting the write, which is the one distinction verify_after exists to draw. - coordinator.py: on a mid-sequence failure, say how many writes landed and which, and still kick the refresh. Raising bare threw that away, and the appliance is left holding a partial sequence. Also documents why `settle` waits inside the session lock while verify_after's wait deliberately doesn't: a poll landing between two writes is exactly what the sequence exists to rule out, and the caps bound the worst case at 10 x 30s. --- README.md | 4 +- custom_components/localthings/coordinator.py | 103 +++++++++++---- custom_components/localthings/services.py | 27 ++-- tests/test_services.py | 125 ++++++++++++++++++- 4 files changed, 223 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 11fa0b6..28aaaf7 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,9 @@ The response has one `results` entry per write, with `before`/`after` reps and a } ``` -`verify_after` (0-60s, default 0, omit to skip) is what actually answers the "did it stick" question: after the sequence finishes, it waits that long and then re-reads every distinct href the sequence touched, reporting the result under `verified`, keyed by canonical href. `changed` tells you the write was accepted and reflected immediately; `held` tells you whether it was still there N seconds later, or whether the board quietly put it back — issue #300's exact symptom. Where an href was written more than once in a sequence, `held` compares against the *last* payload sent to it. +`verify_after` (0-60s, default 0, omit to skip) is what actually answers the "did it stick" question: after the sequence finishes, it waits that long and then re-reads every distinct href the sequence touched, reporting the result under `verified`, keyed by canonical href. `changed` tells you the write was accepted and reflected immediately; `held` tells you whether it was still there N seconds later, or whether the board quietly put it back — issue #300's exact symptom. Where an href was written more than once in a sequence, `held` compares against the *last* payload sent to it. A `held` of `null` means the re-read itself didn't come back (check `code` next to it) — unknown, deliberately not reported as a revert. + +If the session drops partway through a sequence, the action raises rather than returning, and the error names how many writes completed and which — the appliance is left holding a partial sequence, so knowing where it stopped is the difference between a usable result and starting over blind. `read_resource` is the read half, and it's deliberately not just a cache lookup: diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 9c1a09e..5c3b502 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -114,6 +114,22 @@ def _href_to_path_segs(href: str) -> list[str]: return [s for s in str(href).strip("/").split("/") if s] +def normalize_href(href: str) -> str: + """A user-typed href in the one canonical spelling the rest of the debug + path assumes: exactly one leading slash, no trailing slash, no empty + segments. + + Public because services.py has to normalize *before* handing an href to + `Subdevice.to_actual` (issue #300). That transform is purely textual -- + an indexed subdevice rewrites only a trailing '0' segment -- so + '/mode/vs/0/' slips through it unchanged and then normalizes here to + '/mode/vs/0', silently landing the write on the master's resource + instead of the subdevice's while still reporting 2.04. Normalizing on + the way in makes the two agree. + """ + return "/" + "/".join(_href_to_path_segs(href)) + + def _coap_code_str(code: int) -> str: """Raw CoAP response code -> its 'C.DD' rendering (e.g. 0x44 -> '2.04'), the class/detail split RFC 7252 §12.1.2 defines. `raw_code` is kept @@ -1280,30 +1296,55 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): results: list[dict[str, Any]] = [] last_payload_by_href: dict[str, dict] = {} - async with self._session_lock: - for i, (path_segs, href, payload, settle) in enumerate(parsed): - before = self.resource(href) - code, after = await self.hass.async_add_executor_job( - self._raw_write_blocking, path_segs, payload, href - ) - last_payload_by_href[href] = payload - results.append( - { - "href": href, - "code": _coap_code_str(code), - "raw_code": code, - "accepted": _coap_accepted(code), - "before": before, - "after": after, - "changed": all(after.get(k) == v for k, v in payload.items()), - } - ) - # settle is "how long to wait before the next write" -- the - # last item has no next write, so it gets no wait here; - # verify_after (below) is the equivalent wait after the - # sequence as a whole. - if settle and i < len(parsed) - 1: - await asyncio.sleep(settle) + try: + async with self._session_lock: + for i, (path_segs, href, payload, settle) in enumerate(parsed): + before = self.resource(href) + code, after = await self.hass.async_add_executor_job( + self._raw_write_blocking, path_segs, payload, href + ) + last_payload_by_href[href] = payload + results.append( + { + "href": href, + "code": _coap_code_str(code), + "raw_code": code, + "accepted": _coap_accepted(code), + "before": before, + "after": after, + "changed": all(after.get(k) == v for k, v in payload.items()), + } + ) + # settle is "how long to wait before the next write" -- the + # last item has no next write, so it gets no wait here; + # verify_after (below) is the equivalent wait after the + # sequence as a whole. + # + # Held across this wait, unlike verify_after's below: a + # poll landing between two writes is exactly what this + # sequence exists to rule out, since it blurs which write + # the appliance was reacting to. Bounded by the caps above + # -- 10 writes x 30s is the worst a caller can ask for. + if settle and i < len(parsed) - 1: + await asyncio.sleep(settle) + except Exception as err: + # A session drop partway leaves the appliance holding whatever + # already landed. Raising bare would throw that away, and knowing + # which writes got through is the difference between a usable + # probe result and having to start the sequence over blind. + done = ", ".join(r["href"] for r in results) or "none" + self._log.warning( + "raw write sequence failed after %d of %d writes (completed: %s): %s", + len(results), + len(parsed), + done, + err, + ) + await self.async_request_refresh() + raise HomeAssistantError( + f"Raw write sequence failed after {len(results)} of {len(parsed)} writes " + f"(completed: {done}). The appliance may be holding a partial sequence." + ) from err response: dict[str, Any] = {"results": results} if verify_after > 0: @@ -1319,12 +1360,22 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): vcode, vrep = await self.hass.async_add_executor_job( self._raw_read_blocking, _href_to_path_segs(href), href ) + # `held` is None, not False, when the re-read itself + # didn't come back with a representation to compare. + # _raw_read_blocking answers a non-2.05 with an empty + # rep, against which every payload comparison is False -- + # so a 4.04 or a dropped read would otherwise be reported + # as "the board reverted your write", which is the exact + # distinction verify_after exists to draw. + read_ok = _coap_accepted(vcode) and bool(vrep) verified[href] = { "code": _coap_code_str(vcode), "raw_code": vcode, "rep": vrep, - "held": all( - vrep.get(k) == v for k, v in last_payload_by_href[href].items() + "held": ( + all(vrep.get(k) == v for k, v in last_payload_by_href[href].items()) + if read_ok + else None ), } response["verified"] = verified diff --git a/custom_components/localthings/services.py b/custom_components/localthings/services.py index 968324f..82f260d 100644 --- a/custom_components/localthings/services.py +++ b/custom_components/localthings/services.py @@ -23,7 +23,7 @@ from homeassistant.helpers import config_validation as cv from homeassistant.helpers import device_registry as dr from .const import DOMAIN, SERVICE_READ_RESOURCE, SERVICE_WRITE_RESOURCE -from .coordinator import LocalThingsCoordinator +from .coordinator import LocalThingsCoordinator, normalize_href from .registry.subdevices import MAIN, Subdevice ATTR_HREF = "href" @@ -119,13 +119,19 @@ async def _async_write_resource(hass: HomeAssistant, call: ServiceCall) -> Servi # (issue #177): the coordinator's raw-write primitive has no notion of # subdevices, only the hrefs it's told to hit -- identity transform for # MAIN, so a device targeting MAIN behaves exactly as it always has. + # + # Normalized first: to_actual is a textual transform, so an un-normalized + # '/mode/vs/0/' passes through it untouched and only gets tidied up + # downstream, quietly hitting the master's resource instead of the + # subdevice's (see coordinator.normalize_href). + canonicals = [normalize_href(w[ATTR_HREF]) for w in writes_in] raw_writes = [ { - "href": subdevice.to_actual(w[ATTR_HREF]), + "href": subdevice.to_actual(canonical), "payload": w.get(ATTR_PAYLOAD), "settle": w.get(ATTR_SETTLE, 0.0), } - for w in writes_in + for canonical, w in zip(canonicals, writes_in, strict=True) ] sequence = await coordinator.async_raw_write_sequence( raw_writes, verify_after=call.data.get(ATTR_VERIFY_AFTER, 0.0) @@ -133,7 +139,7 @@ async def _async_write_resource(hass: HomeAssistant, call: ServiceCall) -> Servi results = [ { - "href": original[ATTR_HREF], + "href": canonical, "actual_href": result["href"], "code": result["code"], "raw_code": result["raw_code"], @@ -142,11 +148,14 @@ async def _async_write_resource(hass: HomeAssistant, call: ServiceCall) -> Servi "after": result["after"], "changed": result["changed"], } - for original, result in zip(writes_in, sequence["results"], strict=True) + for canonical, result in zip(canonicals, sequence["results"], strict=True) ] response: dict[str, Any] = {"device_id": device_id, "results": results} if "verified" in sequence: - canonical_by_actual = {subdevice.to_actual(w[ATTR_HREF]): w[ATTR_HREF] for w in writes_in} + # Keyed off the same normalized canonicals the sequence was built + # from, so the lookup can't miss and hand back an actual href where + # the documented contract promises a canonical one. + canonical_by_actual = {subdevice.to_actual(c): c for c in canonicals} response["verified"] = { canonical_by_actual.get(actual_href, actual_href): verified for actual_href, verified in sequence["verified"].items() @@ -165,10 +174,12 @@ async def _async_read_resource(hass: HomeAssistant, call: ServiceCall) -> Servic snapshot: dict[str, Any] = {"resources": coordinator.canonical_resources(subdevice)} return cast(ServiceResponse, snapshot) - actual_href = subdevice.to_actual(href) + # Same normalize-before-translate order as the write path above. + canonical = normalize_href(href) + actual_href = subdevice.to_actual(canonical) code, rep = await coordinator.async_raw_read(actual_href) read_result: dict[str, Any] = { - "href": href, + "href": canonical, "actual_href": actual_href, "code": f"{code >> 5}.{code & 0x1F:02d}", "raw_code": code, diff --git a/tests/test_services.py b/tests/test_services.py index 4f63d9c..ab4414e 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -14,7 +14,7 @@ from unittest.mock import AsyncMock, patch import cbor2 import pytest from homeassistant.core import HomeAssistant -from homeassistant.exceptions import ServiceValidationError +from homeassistant.exceptions import HomeAssistantError, ServiceValidationError from homeassistant.helpers import device_registry as dr from pytest_homeassistant_custom_component.common import MockConfigEntry @@ -345,6 +345,129 @@ async def test_write_resource_translates_href_for_indexed_subdevice(hass, coordi assert result["actual_href"] == "/mode/vs/1" +async def test_write_resource_normalizes_href_before_subdevice_translation(hass, coordinator): + """A trailing slash must not silently retarget the write at the master. + + `Subdevice.to_actual` is a textual transform that rewrites only a + trailing '0' segment, so '/mode/vs/0/' passes through it unchanged and + then normalizes downstream to the master's '/mode/vs/0' -- landing on + the wrong cavity while still answering 2.04. Nothing in the response + would have given that away. + """ + sub = Subdevice(kind="indexed", key="1", seed_path=("device", "1")) + coordinator.subdevices = [sub] + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=coordinator._entry.entry_id, + identifiers=coordinator.device_info_for(sub)["identifiers"], + ) + fake = _FakeSession() + coordinator._session = fake + + response = await _call_write( + hass, device.id, writes=[{"href": "/mode/vs/0/", "payload": {"x": 1}}] + ) + + assert fake.post_calls[0][0] == ["mode", "vs", "1"] + result = response["results"][0] + assert result["href"] == "/mode/vs/0" + assert result["actual_href"] == "/mode/vs/1" + + +async def test_write_resource_verified_is_keyed_by_canonical_href_for_subdevice(hass, coordinator): + """`verified` promises canonical keys; the coordinator reports actual + ones, so the translation back has to line up with what was sent.""" + sub = Subdevice(kind="indexed", key="1", seed_path=("device", "1")) + coordinator.subdevices = [sub] + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=coordinator._entry.entry_id, + identifiers=coordinator.device_info_for(sub)["identifiers"], + ) + fake = _FakeSession() + fake.queue_get("/mode/vs/1", {"x": 1}) + coordinator._session = fake + + response = await _call_write( + hass, + device.id, + writes=[{"href": "/mode/vs/0", "payload": {"x": 1}}], + verify_after=1, + ) + + assert list(response["verified"]) == ["/mode/vs/0"] + assert response["verified"]["/mode/vs/0"]["held"] is True + + +async def test_write_resource_failure_midway_names_the_writes_that_landed( + hass, coordinator, device_id +): + """A drop partway through leaves the appliance holding a partial + sequence; the error has to say how far it got, or the operator can't + tell what state the device is in without starting over blind.""" + + class _DropsOnSecondWrite(_FakeSession): + def post(self, path_segs, payload, timeout=None): + if len(self.post_calls) == 1: + raise OSError("session dropped") + return super().post(path_segs, payload, timeout) + + coordinator._session = _DropsOnSecondWrite() + + with pytest.raises(HomeAssistantError) as excinfo: + await _call_write( + hass, + device_id, + writes=[ + {"href": "/mode/vs/0", "payload": {"a": 1}}, + {"href": "/temperatures/vs/0", "payload": {"b": 2}}, + {"href": "/operational/state/vs/0", "payload": {"c": 3}}, + ], + ) + + message = str(excinfo.value) + assert "1 of 3" in message + assert "/mode/vs/0" in message + # Entities still get pulled back in line with whatever did land. + coordinator.async_request_refresh.assert_awaited() + + +async def test_write_resource_held_is_none_when_the_verify_read_fails(hass, coordinator, device_id): + """A re-read that doesn't come back is not evidence of a revert. + + _raw_read_blocking answers a non-2.05 with an empty rep, and every + payload comparison against {} is False -- so without this, a 4.04 or a + dropped verify read is reported as `held: false`, indistinguishable + from the board quietly putting the old value back. That distinction is + the whole reason verify_after exists (issue #300). + """ + + class _FailingVerifyRead(_FakeSession): + def get(self, path_segs, timeout=None): + self.get_calls.append(list(path_segs)) + # The write's own follow-up GET succeeds; the later verify + # re-read of the same href is the one that fails. + if len(self.get_calls) == 1: + return 0x45, cbor2.dumps({"x": 1}) + return 0x84, b"" + + coordinator._session = _FailingVerifyRead() + + response = await _call_write( + hass, + device_id, + writes=[{"href": "/mode/vs/0", "payload": {"x": 1}}], + verify_after=1, + ) + + verified = response["verified"]["/mode/vs/0"] + assert verified["held"] is None + assert verified["code"] == "4.04" + # The immediate readback still saw the value land -- only the delayed + # confirmation is unknown, and the two must not be conflated. + assert response["results"][0]["changed"] is True + + # ---------------------------------------------------------------------- # Validation caps # ---------------------------------------------------------------------- From fffe923afcbde3c1a2977b02f376fdc13986c6d1 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 7 Aug 2026 22:58:48 +0000 Subject: [PATCH 4/5] Take the device as a field, not a service target Hassfest rejects a filtered device target outright ("Services do not support device filters on target, use a device selector instead"), and an unfiltered one would offer every device in the installation. Both services now take device_id as a required field with a device selector scoped to this integration -- the shape fully_kiosk, guardian and unifi already use. No schema change needed: cv.TARGET_SERVICE_FIELDS already accepts device_id, so the options-flow panel's target= call keeps working. Also trims the comments added with the review fixes back to the one or two sentences CONTRIBUTING asks for. --- README.md | 8 ++-- custom_components/localthings/coordinator.py | 45 ++++++-------------- custom_components/localthings/services.py | 15 +++---- custom_components/localthings/services.yaml | 20 ++++++--- 4 files changed, 35 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index 28aaaf7..6d69cdc 100644 --- a/README.md +++ b/README.md @@ -102,15 +102,14 @@ Each device has its own **Configure** option in Settings > Devices & Services, u Two HA actions, `localthings.write_resource` and `localthings.read_resource`, talk to a device's OCF resources directly instead of through this integration's entity model. They exist for two overlapping jobs: pinning down a device-specific write contract (the reverse-engineering work `docs/investigations/` and the provenance comments throughout `registry/capabilities/` are all about), and driving a resource this integration doesn't model as an entity yet, without waiting on a release. -Both target a device (`target: device:`, filtered to `integration: localthings` in the picker) and resolve to exactly one appliance — targeting an area or label that expands to more than one LocalThings device is rejected rather than silently fanned out across all of them. `href` is always canonical (e.g. `/mode/vs/0`); if the device you targeted is a subdevice — an oven's second cavity, an AC's second indoor unit — it's translated to the real on-the-wire href for you (`/mode/vs/1`, say), and the response reports both forms so there's no ambiguity about what was actually sent. +Both take a `device_id` (a device picker filtered to this integration) and resolve to exactly one appliance — a target that expands to more than one LocalThings device is rejected rather than silently fanned out across all of them. `href` is always canonical (e.g. `/mode/vs/0`); if the device you targeted is a subdevice — an oven's second cavity, an AC's second indoor unit — it's translated to the real on-the-wire href for you (`/mode/vs/1`, say), and the response reports both forms so there's no ambiguity about what was actually sent. `write_resource` exists because a single write, one at a time, isn't enough to probe some boards. Issue #300's Samsung wall oven answers `2.04 Changed` to a settings write while idle and then silently reverts it — the write only sticks once a cycle is already running. Finding what actually triggers a cycle needs an *ordered sequence* of writes to different resources, with real delays between them, and a way to check afterward whether anything actually held: ```yaml action: localthings.write_resource -target: - device_id: abc123... data: + device_id: abc123... writes: - href: /mode/vs/0 payload: @@ -150,9 +149,8 @@ If the session drops partway through a sequence, the action raises rather than r ```yaml action: localthings.read_resource -target: - device_id: abc123... data: + device_id: abc123... href: /mode/vs/0 ``` diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 5c3b502..37f6136 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -115,18 +115,10 @@ def _href_to_path_segs(href: str) -> list[str]: def normalize_href(href: str) -> str: - """A user-typed href in the one canonical spelling the rest of the debug - path assumes: exactly one leading slash, no trailing slash, no empty - segments. - - Public because services.py has to normalize *before* handing an href to - `Subdevice.to_actual` (issue #300). That transform is purely textual -- - an indexed subdevice rewrites only a trailing '0' segment -- so - '/mode/vs/0/' slips through it unchanged and then normalizes here to - '/mode/vs/0', silently landing the write on the master's resource - instead of the subdevice's while still reporting 2.04. Normalizing on - the way in makes the two agree. - """ + """A user-typed href in one canonical spelling. Public because + services.py must normalize before `Subdevice.to_actual`, which rewrites + only a trailing '0' segment: '/mode/vs/0/' slips through it unchanged + and would land on the master's resource, not the subdevice's.""" return "/" + "/".join(_href_to_path_segs(href)) @@ -1315,23 +1307,15 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): "changed": all(after.get(k) == v for k, v in payload.items()), } ) - # settle is "how long to wait before the next write" -- the - # last item has no next write, so it gets no wait here; - # verify_after (below) is the equivalent wait after the - # sequence as a whole. - # - # Held across this wait, unlike verify_after's below: a - # poll landing between two writes is exactly what this - # sequence exists to rule out, since it blurs which write - # the appliance was reacting to. Bounded by the caps above - # -- 10 writes x 30s is the worst a caller can ask for. + # The lock is deliberately held across this wait, unlike + # verify_after's below: a poll landing between two writes + # blurs which one the appliance reacted to, which is what + # the sequence exists to rule out. The caps bound it. if settle and i < len(parsed) - 1: await asyncio.sleep(settle) except Exception as err: - # A session drop partway leaves the appliance holding whatever - # already landed. Raising bare would throw that away, and knowing - # which writes got through is the difference between a usable - # probe result and having to start the sequence over blind. + # A drop partway leaves the appliance holding whatever already + # landed, so the error has to say which writes got through. done = ", ".join(r["href"] for r in results) or "none" self._log.warning( "raw write sequence failed after %d of %d writes (completed: %s): %s", @@ -1360,12 +1344,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): vcode, vrep = await self.hass.async_add_executor_job( self._raw_read_blocking, _href_to_path_segs(href), href ) - # `held` is None, not False, when the re-read itself - # didn't come back with a representation to compare. - # _raw_read_blocking answers a non-2.05 with an empty - # rep, against which every payload comparison is False -- - # so a 4.04 or a dropped read would otherwise be reported - # as "the board reverted your write", which is the exact + # None, not False, when the re-read brought back nothing + # to compare: every comparison against an empty rep is + # False, which would report a 4.04 as a revert -- the one # distinction verify_after exists to draw. read_ok = _coap_accepted(vcode) and bool(vrep) verified[href] = { diff --git a/custom_components/localthings/services.py b/custom_components/localthings/services.py index 82f260d..73d4c55 100644 --- a/custom_components/localthings/services.py +++ b/custom_components/localthings/services.py @@ -116,14 +116,9 @@ async def _async_write_resource(hass: HomeAssistant, call: ServiceCall) -> Servi writes_in: list[dict[str, Any]] = call.data[ATTR_WRITES] # Canonical -> actual translation happens here, not in the coordinator - # (issue #177): the coordinator's raw-write primitive has no notion of - # subdevices, only the hrefs it's told to hit -- identity transform for - # MAIN, so a device targeting MAIN behaves exactly as it always has. - # - # Normalized first: to_actual is a textual transform, so an un-normalized - # '/mode/vs/0/' passes through it untouched and only gets tidied up - # downstream, quietly hitting the master's resource instead of the - # subdevice's (see coordinator.normalize_href). + # (issue #177), whose raw-write primitive has no notion of subdevices -- + # identity transform for MAIN. Normalized first, or a trailing slash + # slips past to_actual onto the master (see coordinator.normalize_href). canonicals = [normalize_href(w[ATTR_HREF]) for w in writes_in] raw_writes = [ { @@ -153,8 +148,8 @@ async def _async_write_resource(hass: HomeAssistant, call: ServiceCall) -> Servi response: dict[str, Any] = {"device_id": device_id, "results": results} if "verified" in sequence: # Keyed off the same normalized canonicals the sequence was built - # from, so the lookup can't miss and hand back an actual href where - # the documented contract promises a canonical one. + # from, so the lookup can't miss and return an actual href where the + # contract promises a canonical one. canonical_by_actual = {subdevice.to_actual(c): c for c in canonicals} response["verified"] = { canonical_by_actual.get(actual_href, actual_href): verified diff --git a/custom_components/localthings/services.yaml b/custom_components/localthings/services.yaml index 4afcbdb..b08ef21 100644 --- a/custom_components/localthings/services.yaml +++ b/custom_components/localthings/services.yaml @@ -10,10 +10,14 @@ write_resource: it can misconfigure your appliance. Prefer a real entity, or the Debug write panel in the integration's Configure menu, for anything this integration already models. - target: - device: - integration: localthings fields: + device_id: + name: Device + description: The appliance to write to, or one of its subdevices. + required: true + selector: + device: + integration: localthings writes: name: Writes description: >- @@ -52,10 +56,14 @@ read_resource: deliberately not the cache, which can be up to a poll interval stale -- or omit it to get the cached snapshot of every resource this integration currently tracks on that device. - target: - device: - integration: localthings fields: + device_id: + name: Device + description: The appliance to read from, or one of its subdevices. + required: true + selector: + device: + integration: localthings href: name: Resource href description: >- From 6ee60beae95835ce11c3b206d44287c494fabcbd Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Fri, 7 Aug 2026 23:46:28 +0000 Subject: [PATCH 5/5] Make holding the session across a sequence the caller's choice Holding _session_lock for a whole write sequence buys certainty about what the appliance saw and when, but blocks every poll and entity write for the sequence's full length -- up to 10 x 30s. Which of those matters more depends on what is being probed, so it is now hold_session_lock on async_raw_write_sequence and a field on the service, defaulting to the holding behavior that shipped. Off, the lock is taken per write and released across the settle waits, so entities keep updating through a long sequence. Exactly one of the two context managers is ever the real lock -- asyncio.Lock isn't reentrant. Tests assert the lock's actual state during the settle wait in both modes, rather than just that the flag is accepted. --- README.md | 4 +- custom_components/localthings/coordinator.py | 40 +++++++++----- custom_components/localthings/services.py | 6 ++- custom_components/localthings/services.yaml | 13 +++++ tests/test_services.py | 55 ++++++++++++++++++++ 5 files changed, 104 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 6d69cdc..0a7833b 100644 --- a/README.md +++ b/README.md @@ -123,7 +123,9 @@ data: Mind the shapes: what you write is sent verbatim, so the field names and types have to be the ones that resource actually uses. `/mode/vs/0` takes `modes` as an *array* on this board; a bare string, or the singular `mode`, is a different field the device will simply ignore. `read_resource` (below) with no `href` is the quickest way to see the real shape of everything before you write to any of it. -Each write in `writes` (1-10 of them) needs `href` and a non-empty `payload`, sent verbatim as a partial-rep POST — this bypasses the remote-control-off block and every `write_fn`/`validate_fn` a normal entity write goes through, and sends exactly the fields you give it, so it can misconfigure your appliance if you get it wrong. `settle` (0-30s, default 0) is how long to wait *after* that write before starting the next one. The whole sequence runs under a single lock, so a routine poll can't land in the middle of it and blur which write is responsible for what the device does next. +Each write in `writes` (1-10 of them) needs `href` and a non-empty `payload`, sent verbatim as a partial-rep POST — this bypasses the remote-control-off block and every `write_fn`/`validate_fn` a normal entity write goes through, and sends exactly the fields you give it, so it can misconfigure your appliance if you get it wrong. `settle` (0-30s, default 0) is how long to wait *after* that write before starting the next one. + +By default the whole sequence holds the device session from the first write to the last, settle delays included, so a routine poll or another entity's write can't land between two steps and blur which write the appliance was reacting to. The cost is that nothing else on that device updates until the sequence ends — up to 10 × 30s if you ask for the maximum of both. Set `hold_session_lock: false` to take the session per write and release it across the waits instead, trading that certainty for a device whose entities keep updating throughout. The response has one `results` entry per write, with `before`/`after` reps and a `changed` flag (every key/value in `payload` present and equal in the immediate readback): diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 37f6136..4325187 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -1252,14 +1252,24 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): ) async def async_raw_write_sequence( - self, writes: list[dict], *, verify_after: float = 0.0 + self, + writes: list[dict], + *, + verify_after: float = 0.0, + hold_session_lock: bool = True, ) -> dict[str, Any]: """Debug-only ordered multi-write (issue #300): a Samsung wall oven board discards settings writes while idle and only keeps them once a cycle is already running, which no single-write debug pass can - probe for. This owns the whole sequence -- every write shares one - `_session_lock` hold, so a poll can never interleave mid-sequence - and blur which write is responsible for what the device does next. + probe for. + + `hold_session_lock` (default) keeps `_session_lock` for the whole + sequence, settle waits included, so nothing interleaves between + steps and blurs which write the appliance reacted to -- at the cost + of blocking polls and entity writes for the sequence's full length + (up to 10 x 30s). Pass False to take the lock per write and release + it across the waits, for a long sequence where a stalled poll costs + more than an interleaved read. `writes` are already on-the-wire hrefs: subdevice translation (canonical -> actual) is services.py's job, not this method's -- @@ -1288,13 +1298,20 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): results: list[dict[str, Any]] = [] last_payload_by_href: dict[str, dict] = {} + # Exactly one of these is the real lock, never both -- asyncio.Lock + # isn't reentrant, so nesting the same one would deadlock. + outer_lock = self._session_lock if hold_session_lock else contextlib.nullcontext() try: - async with self._session_lock: + async with outer_lock: for i, (path_segs, href, payload, settle) in enumerate(parsed): - before = self.resource(href) - code, after = await self.hass.async_add_executor_job( - self._raw_write_blocking, path_segs, payload, href + per_write_lock = ( + contextlib.nullcontext() if hold_session_lock else self._session_lock ) + async with per_write_lock: + before = self.resource(href) + code, after = await self.hass.async_add_executor_job( + self._raw_write_blocking, path_segs, payload, href + ) last_payload_by_href[href] = payload results.append( { @@ -1307,10 +1324,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): "changed": all(after.get(k) == v for k, v in payload.items()), } ) - # The lock is deliberately held across this wait, unlike - # verify_after's below: a poll landing between two writes - # blurs which one the appliance reacted to, which is what - # the sequence exists to rule out. The caps bound it. + # Under the default this wait happens inside the lock, so + # nothing lands between two writes to blur which one the + # appliance reacted to; see hold_session_lock above. if settle and i < len(parsed) - 1: await asyncio.sleep(settle) except Exception as err: diff --git a/custom_components/localthings/services.py b/custom_components/localthings/services.py index 73d4c55..c124b9d 100644 --- a/custom_components/localthings/services.py +++ b/custom_components/localthings/services.py @@ -31,6 +31,7 @@ ATTR_PAYLOAD = "payload" ATTR_SETTLE = "settle" ATTR_WRITES = "writes" ATTR_VERIFY_AFTER = "verify_after" +ATTR_HOLD_SESSION_LOCK = "hold_session_lock" ATTR_DEVICE_ID = "device_id" _WRITE_ITEM_SCHEMA = vol.Schema( @@ -58,6 +59,7 @@ _WRITE_RESOURCE_SCHEMA = vol.Schema( **cv.TARGET_SERVICE_FIELDS, vol.Required(ATTR_WRITES): vol.All(cv.ensure_list, [_WRITE_ITEM_SCHEMA]), vol.Optional(ATTR_VERIFY_AFTER): vol.Coerce(float), + vol.Optional(ATTR_HOLD_SESSION_LOCK): cv.boolean, } ) @@ -129,7 +131,9 @@ async def _async_write_resource(hass: HomeAssistant, call: ServiceCall) -> Servi for canonical, w in zip(canonicals, writes_in, strict=True) ] sequence = await coordinator.async_raw_write_sequence( - raw_writes, verify_after=call.data.get(ATTR_VERIFY_AFTER, 0.0) + raw_writes, + verify_after=call.data.get(ATTR_VERIFY_AFTER, 0.0), + hold_session_lock=call.data.get(ATTR_HOLD_SESSION_LOCK, True), ) results = [ diff --git a/custom_components/localthings/services.yaml b/custom_components/localthings/services.yaml index b08ef21..03b870c 100644 --- a/custom_components/localthings/services.yaml +++ b/custom_components/localthings/services.yaml @@ -32,6 +32,19 @@ write_resource: ["Bake"]}, "settle": 3}] selector: object: + hold_session_lock: + name: Hold the session for the whole sequence + description: >- + Keep the device session for the entire sequence, settle delays + included, so nothing else -- a routine poll, another entity's write + -- can land between two steps and blur which write the appliance + was reacting to. On by default. Turning it off takes the session + per write and frees it across the waits, which lets entities keep + updating during a long sequence at the cost of that certainty. + required: false + default: true + selector: + boolean: verify_after: name: Verify after description: >- diff --git a/tests/test_services.py b/tests/test_services.py index ab4414e..e8f05e4 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -188,6 +188,61 @@ async def test_write_resource_settle_honored_between_writes(hass, coordinator, d assert [call.args[0] for call in mock_sleep.call_args_list] == [2.0, 5.0] +async def test_write_resource_holds_the_session_across_settle_by_default( + hass, coordinator, device_id +): + """The default keeps the session for the whole sequence so nothing lands + between two writes -- asserted on the lock's real state during the wait, + not merely on the flag being accepted.""" + coordinator._session = _FakeSession() + locked_during_settle = [] + + async def _record(_delay): + locked_during_settle.append(coordinator._session_lock.locked()) + + with patch(_SLEEP_TARGET, new=_record): + await _call_write( + hass, + device_id, + writes=[ + {"href": "/a/vs/0", "payload": {"x": 1}, "settle": 2}, + {"href": "/b/vs/0", "payload": {"x": 2}}, + ], + ) + + assert locked_during_settle == [True] + # ...and it's handed back afterward, rather than leaked to the next call. + assert not coordinator._session_lock.locked() + + +async def test_write_resource_releases_the_session_across_settle_when_asked( + hass, coordinator, device_id +): + """hold_session_lock=False frees the session across the waits, so polls + and entity writes keep working through a long sequence.""" + coordinator._session = _FakeSession() + locked_during_settle = [] + + async def _record(_delay): + locked_during_settle.append(coordinator._session_lock.locked()) + + with patch(_SLEEP_TARGET, new=_record): + response = await _call_write( + hass, + device_id, + writes=[ + {"href": "/a/vs/0", "payload": {"x": 1}, "settle": 2}, + {"href": "/b/vs/0", "payload": {"x": 2}}, + ], + hold_session_lock=False, + ) + + assert locked_during_settle == [False] + assert not coordinator._session_lock.locked() + # Both writes still go out, in order -- only the locking differs. + assert [r["href"] for r in response["results"]] == ["/a/vs/0", "/b/vs/0"] + + async def test_write_resource_changed_true_when_readback_matches_payload( hass, coordinator, device_id ):