Skip to content

Fix the issues found in the review of 1.0.4 - #10

Merged
DAB-LABS merged 1 commit into
masterfrom
fixes-1.0.5
Sep 6, 2026
Merged

Fix the issues found in the review of 1.0.4#10
DAB-LABS merged 1 commit into
masterfrom
fixes-1.0.5

Conversation

@DAB-LABS

@DAB-LABS DAB-LABS commented Sep 6, 2026

Copy link
Copy Markdown
Owner

I had 1.0.4 reviewed a fifth time. The library came through clean, the review found one more way the new socket could report an error the original never did.

The socket change is small. When a device is off, some routers answer for it with a "host unreachable" message. The original library never saw those and simply timed out, and Home Assistant gives a timeout a few tries before marking a device unavailable, but marks it unavailable at once on a socket error. So those messages are now treated as silence, the way the original behaved. I measured this on the bench first: on my network neither case actually produced the message, so this is insurance for networks that do.

The capture change replaces the mechanism that decides whether a capture window is free with a plain flag. Same behaviour, same tests. Tested on Python 3.13 and 3.14, 272 tests, plus a live run against an RM4 Pro.

Coding assistant's notes on this change:

EHOSTUNREACH and Windows's ConnectionResetError join ECONNREFUSED in the "silence" branch of _exchange: logged, the timeout decides, the socket is dropped afterwards so the next call heals. Bench measurement before the change: an on-link address with no host behind it (ARP incomplete) and an off-subnet address both produced NetworkTimeoutError after the full timeout on the test network, with no ICMP surfacing.

The capture window claim is now a flag set on the window's first iteration and cleared in the generator's finally, which asyncio's finalizer runs when a dropped generator is collected; the weak reference and ag_frame inspection are gone. The existing tests (dropped window does not block, held window is refused, paused consumer keeps its window, unstarted window does not block) pin the behaviour unchanged. One visible difference: CaptureInProgressError always comes from the new window's first iteration now, never from the capture() call itself. Also: discover() closes the xdiscover() generator it drains; a comment explains why the RM Max sits in rmpro (upstream mjg59#838's text says rm4pro, its tested diff says rmpro); the README notes that a device belongs to the event loop it first talks on. Typing, py.typed and the CLI move are deliberately left for a later release.

A fifth review, of 1.0.4, found one more place where the connected socket
could surface an error the original library never raised, and made a fair
case that the capture window claim was more machinery than the job needed.
This fixes the first, simplifies the second, and takes two small items
along. No change to the wire format or the public API. Tested on 3.13 and
3.14, 272 tests under -X dev with ResourceWarning as an error, oracle
fixtures unchanged, and live against an RM4 Pro.

Technical details:
- EHOSTUNREACH and Windows's ConnectionResetError are treated as silence
  in _exchange, as ECONNREFUSED already was: logged, the timeout decides,
  the socket is dropped afterwards. Home Assistant tolerates a timeout for
  a few polls but marks a device unavailable on the first OSError, and the
  original's unconnected socket never saw any of these. Measured on the
  bench before changing anything: neither an on-link address with no host
  nor an off-subnet one produced the ICMP here, so this is insurance for
  networks that do.
- The window claim is a flag set on first iteration and cleared in the
  generator's finally (so asyncio's finalizer releases a dropped window),
  replacing the weakref and ag_frame inspection. Same behaviour, same
  tests; CaptureInProgressError now always comes from the first
  iteration.
- discover() closes the xdiscover() generator it drains.
- A comment explains why the RM Max sits in rmpro (upstream mjg59#838).
- README: a device belongs to the event loop it first talks on.
- Version 1.0.5.
@DAB-LABS
DAB-LABS merged commit 52bb44a into master Sep 6, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant