Skip to content

Fix the issues found in the review of 1.0.3 - #9

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

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

Conversation

@DAB-LABS

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

Copy link
Copy Markdown
Owner

I had 1.0.3 reviewed again, this time by someone who drove the real network path our tests fake. They found two things.

First, if a device's network socket went bad (the interface bounced, the host got a new address), the library kept using the dead socket and every request timed out until the device was reloaded. The original opened a fresh socket per call, so it healed on its own. Second, a hostname that did not resolve made the library wait out the full timeout and, for pings, silently send nothing, where the original failed at once with the error Home Assistant maps to "invalid host".

Both are fixed, and we carried a fix from an open upstream pull request for the A2 air quality sensor, which its author tested on a real unit, but we have no way to confirm. Tested on Python 3.13 and 3.14, 269 tests, plus a live run against an RM4 Pro that included a forced socket failure and a bad hostname.

Coding assistant's notes on this change:

The datagram protocol now puts socket errors on the receive queue, so a waiting request fails at once with that OSError for send failures (no route, address gone), and any request that fails for a network reason drops the endpoint so the next call opens a fresh socket. ICMP port unreachable is treated as silence, since the original's unconnected socket never saw it, so a host that is up but is not a Broadlink device still times out as before rather than surfacing a new error type. scan(), ping() and send_setup_packet() resolve their destination once with loop.getaddrinfo and let socket.gaierror propagate; nothing resolves on the event loop any more.

The A2 request frame now follows the SP4/LB1 layout (length 12, four-byte data length), which is byte for byte the packet upstream mjg59#826 tested; it is the one oracle case re-recorded on purpose, noted in the harness, with Co-authored-by credit. Also in this change: xdiscover() closes the scan() generator it wraps; captured_at no longer takes part in CapturedSignal equality; async generator functions are annotated AsyncGenerator, which has the aclose() we call; the TICK docstring matches the README; a new loopback test module drives the real endpoint including 20 concurrent requests and the ICMP path; the README says which errors discover() and hello() raise. Still deferred to 1.1: py.typed and mypy in CI, and moving the CLIs into the package so they are linted.

A fourth review, of 1.0.3, drove the real socket path that the test suite
fakes and found two things the original library did that this one had
lost: a socket that goes bad is replaced, and a hostname that does not
resolve fails at once. This fixes both, carries one device fix from
upstream, and takes the small items along. Tested on 3.13 and 3.14, 269
tests under -X dev with ResourceWarning as an error, and live against an
RM4 Pro including a forced socket failure and a bad hostname.

Technical details:
- _Protocol.error_received and connection_lost put the error on the
  receive queue, so a waiting request fails at once with that OSError
  (send failures: no route, address gone) instead of waiting out the
  timeout. ICMP port unreachable is logged and treated as silence, which
  is what the original's unconnected socket saw, so the timeout decides
  as before.
- A request that fails for a network reason drops the endpoint; the next
  call opens a fresh socket, as the original did by opening one per call.
- scan(), ping() and send_setup_packet() resolve the destination once
  with loop.getaddrinfo and let socket.gaierror propagate; a send failure
  in ping() and setup() is raised. Nothing resolves on the loop any more.
- a2._send uses the SP4/LB1 frame layout (length 12, four-byte data
  length). The bytes match upstream mjg59#826, tested by its author on a real
  A2; the one oracle case re-recorded on purpose and noted in harness.py.
- xdiscover() closes the scan() generator it wraps; captured_at is
  excluded from CapturedSignal equality; async generator functions are
  annotated AsyncGenerator (mypy 40 to 37); TICK docstring matches the
  README.
- tests/test_loopback.py drives the real datagram endpoint, including 20
  concurrent requests and the ICMP path on loopback.
- README: errors discover()/hello() raise, the socket drop on failure,
  where CaptureInProgressError can come from.
- Version 1.0.4.

Co-authored-by: kanshurichard <29994770+kanshurichard@users.noreply.github.com>
@DAB-LABS
DAB-LABS merged commit f9b6075 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