81dbc6fa03bfaae80eef8b72d674bb50186b63de
7
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Marc Billow
|
1abaad7f40 |
Fix issues found by an Opus review of the smartthings-local upgrade
An independent review of the last two commits' diff turned up five real
problems and one CI-breaking one. Fixed all of them:
- tests/test_coordinator_error_handling.py assigned directly onto
coordinator instance attributes (coordinator._poll_once = dict), which
`ty check custom_components tests` -- what CI actually runs, not the
narrower `ty check custom_components` this branch had only been
spot-checked against -- flags as invalid-assignment. Switched to
monkeypatch.setattr, matching every other new test on this branch.
- coordinator.py's subdevice-enumeration failure comment claimed the
probe "retries naturally next cycle." It doesn't: _run_discovery sets
self._discovered = True unconditionally later in the same cycle, which
is what gates the whole block, so a failure here is a first-and-only
attempt, not a retried one -- a composite appliance's sibling
subdevices are missing for the config entry's lifetime until reload.
(A separate flag to retry wouldn't actually fix that either: every
platform's async_setup_entry enumerates coordinator.bound exactly
once, so a later-successful enumeration still couldn't add entities
without a reload.) Corrected the comment and raised debug to warning,
since the effect is silent and permanent otherwise.
- config_flow.py's new diagnostic-handshake fallback (_resolve_alert)
was being called once per failing candidate, inside _handshake_and_read's
scan loop -- contradicting _diagnostic_alert's own docstring ("only
runs once every real candidate has already failed"). Two real costs:
up to CLIENTHELLO_PROBE_TIMEOUT_S extra latency per failing port on
the sweep-fallback path (several candidates), and -- more seriously --
the diagnostic commits DTLS association state on each port it touches,
which can make _probe_and_validate's own CertRejected re-mint retry
(a fresh _handshake_and_read call against that same scan) time out
against the very port it just polluted, per the RFC 6347 §4.2.8
concern already documented elsewhere in this file. Moved to a new
_diagnose_failures helper called once, after the loop, against the
single best (confirmed-live) candidate.
- _resolve_alert also ignored the alert's level: ProbeResult.alert is
set for a received alert of either severity, but only a fatal one (2)
means the appliance broke off the handshake over it -- a warning
(e.g. close_notify) was being read as a rejection reason. Older
library exception text never had this ambiguity (OpenSSL only renders
an exception for a fatal alert), so this was a bug the redaction
fallback introduced. Now filters to level == 2.
- async_raw_read's new HomeAssistantError wrapper reported a translated
error but left a confirmed-dead session installed, so every
subsequent read/write would keep failing identically for up to the
next full poll interval. Now closes the session on any non-TimeoutError
failure, matching _poll_once's own posture.
- async_raw_write_sequence's verify_after fallback (vcode, vrep = 0, {})
is indistinguishable from a real 4.04 by raw_code alone. Added a
read_error field so a caller can tell "couldn't verify" from "the
device said no."
Tests: 8 new/extended (once-not-per-port diagnostic count, alert-level
filtering, the warning log + permanent-loss framing, session-closing on
both async_raw_read and the verify_after path, read_error surfacing).
Full suite (1527 tests), ruff, ruff format, and -- critically --
`ty check custom_components tests` (the CI-matching invocation) all pass.
|
||
|
Marc Billow
|
f949ff04c2 |
coordinator: close four uncaught-exception gaps around smartthings_local
A follow-up review of the 0.1.6 upgrade found four call sites where a library exception (new typed one or the old bare ConnectionError/ TimeoutError it replaced) could escape this integration's own reconnect/logging or a service call's translation layer entirely, instead of being handled the way equivalent failures already are elsewhere in this file: - _attempt_observe_mode's own _connect_session() reconnect (fires only when the session was closed out from under it concurrently) had no try/except, and neither did either of its two call sites in _async_update_data. A failure there escaped uncaught: HA's DataUpdateCoordinator has its own final safety net so nothing crashed the config entry, but non-TimeoutError failures logged a full ERROR traceback instead of this integration's deliberately quiet "poll failed, reconnecting" voice, and skipped its own reconnect bookkeeping entirely. Fixed by catching around just the connect call (the only unguarded raise path in the method -- subscribe_hrefs/ await_observe_notifies already handle their own failures), landing in the same "give up on push this cycle" state abandon_observe_attempt() already produces for the subscribe-failed and stale-session branches. Deliberately does not touch _close_session() (self._session is already None here -- _connect_session only ever publishes it after a full success), _reconnect_is_frequent() (that window records the poll path's own reconnects; feeding it a secondary path's failure would over-trigger its warning threshold), or _resubscribe_due (that flag means "a live session nothing has tried yet" -- setting it here would re-enter the doomed handshake every cycle instead of letting _last_observe_attempt_ts pace the retry). - _enumerate_subdevices_blocking's _connect_session() call (first discovery only) had the same gap. Fixed the same way: log and fall through on the resources _poll_once already returned this cycle, rather than losing first discovery over a failed subdevice probe. - async_raw_read (backing the read_resource service) had no exception handling at all -- a session/network failure during a live debug read reached the service caller as a raw, untranslated library exception, unlike write_resource's equivalent path. Now wrapped the same way async_send_command/async_raw_write_sequence already are, raising HomeAssistantError with a new debug_read_failed translation key (added to all 7 shipped locales). - async_raw_write_sequence's verify_after tail sat outside the method's own try/except, so a failed confirmation read discarded the write results that had already landed by throwing past them. Now caught per-href inside the verify loop instead: a failed read is treated the same as a 4.04/empty one (held=None, "couldn't verify" -- not lost or misreported as a revert), and the rest of the batch still gets checked. Design for the first fix (the trickiest -- it's mid-lock, and has to interact correctly with observe-mode state and the poll path's own bookkeeping without corrupting either) was worked through with a dedicated review pass before implementing. Tests: new coverage for all four (test_coordinator.py's test_attempt_observe_mode_survives_a_failed_reconnect, a new test_coordinator_error_handling.py for the subdevice-enumeration case, and two additions to test_services.py for the read-service and verify_after cases). Full suite (1521 tests), ruff, and ty all pass. |
||
|
Marc Billow
|
cbe881818b |
test_services: widen _get_reps' value type to match queue_get
queue_get accepts dict | list since #335's Collection test started queueing a batch list, but _get_reps was still typed list[dict] -- ty flagged the list.append() as invalid. No behavior change. |
||
|
Marc Billow
|
e5cd212a34 |
read_resource: a Collection's list body is not an empty resource (#335)
`_raw_read_blocking` decoded the CBOR body and kept it only when it was a
Property map, so a Collection -- which answers the `[devcol rep, {href,
rep}, ...]` batch `parse_device0_batch` reads -- came back as `2.05` with
`rep: {}`. That renders as "the resource exists and has nothing in it",
which is the opposite of what a populated batch means, and `/device/0`
itself would have read the same way.
It cost a real result: issue #335's board answers `/sec/devices` (the
`x.com.samsung.devcol` sibling of `/device/0`, and the one remaining place
a composite appliance could be enumerating its indoor units) with exactly
that empty-looking 2.05, and it was nearly written off as a dead end.
The read path now returns the decoded body alongside `rep`, and the service
response carries it as `body` whenever it isn't the map `rep` already has --
omitted for the ordinary case rather than duplicating every rep in every
response. Records the probe round this came out of: indexed leaves 4.04 on
that board, and the UUID prefix confirmed routable by a positive control, so
Patterns A/B/C are ruled out there on evidence rather than on absence.
|
||
|
Marc Billow
|
6ee60beae9 |
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. |
||
|
Marc Billow
|
a6d818dfc0 |
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. |
||
|
Marc Billow
|
5dbe990c1d |
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. |