Bring the client back to parity with the ESPHome port - #39
Merged
Conversation
added 18 commits
August 20, 2026 10:33
Two byte-level facts settle a lot of what this package had wrong, and both
were verified against an ALPHA HWR (family 52, type 7, version 2) rather
than inherited from the ESPHome port.
The APDU head is a length, not an opcode
----------------------------------------
Byte 5 is 0booLLLLLL: an operation (GET/SET/INFO) or an acknowledgement,
then the payload's byte count. `byte5 == len(frame) - 8` holds for every
reply measured here - Class 7 strings, all four setpoint ranges, the
schedule overview, the clock, the temperature-range config, and all three
telemetry registers.
So the set {0x30, 0x2B, 0x14, 0x2E, 0x2D, 0x09}, carried here as
"register-read operation specifiers" and used to select a payload offset,
is really the payload lengths 48, 43, 20, 46, 45 and 9. It happened to
work because 48, 43 and 20 are exactly the motor, flow and temperature
replies; any other object of those sizes was mis-sliced.
And 0x81 is not an acknowledgement carrying an error code. It is
10 000001 - Unknown Data Item, one payload byte - and that byte names the
item the pump did not recognise. A refused write therefore read as
accepted whenever the item was 0x00, which is the case this pump produces.
Matching on the literal set {0x01, 0x81} also meant 0xC1 (Illegal
Operation) and 0x40 (Unknown Class) matched nothing and died by timeout,
so the log said "no response" about a pump that had answered in
milliseconds. The test is now a declared length of <= 1 - not == 1,
because Unknown Class declares zero and is an eight-byte frame.
A response carries no Object ID and no Sub-ID
---------------------------------------------
Bytes 6-9 are [00][TypeH][TypeL][Version]: the type of the object
answered. Object 86 sub-ids 13, 15, 17 and 39 all answer 00 01 2d 01,
because all four are type 301 version 1 - so matching discriminates types,
not instances, and a chain reading siblings has to stop at the first
failure.
The rule that accepted the two fields in either order is gone. They are
one type field; every measured reply matches in wire order, so nothing
needed it, and accepting a transpose let unrelated objects answer each
other. The ESPHome port additionally treats type_high == 0 as a wildcard;
that is not adopted, because zero is a real value (the schedule overview
answers 00 00 da 01) and it would reopen the collision already on record
between the temperature-range config and an event log entry.
Payloads are bounded by what the APDU declares
----------------------------------------------
A telegram may carry several APDUs, so slicing to [-2] reported the next
APDU - and its CRC - as this one's payload. Every payload is now bounded
by 6 + declared length, and ParsedFrame.multi_apdu says when there was
more in the telegram than the frame reports.
Fixtures
--------
The test vectors and the mock pump were built from the assumptions above
and so agreed with the code and disagreed with the pump. Both had the
destination and source addresses in request order on a response, a shape
the hardware never sends; the mock also chose its APDU heads as constants
and computed CRCs with calc_crc16, which omits the final XOR and matches
no frame in either direction. Nothing noticed, because no inbound CRC was
ever verified.
TEST_VECTORS is now recordings. tests/wire.py builds correct frames for
the cases that need one - its class10_ack() independently reproduces
2405f8e70a0100aea2, the canonical acknowledgement.
Telemetry now routes on the measured types rather than on (87, 69),
(93, 290) and (93, 300), which are the addresses *requested*; no reply
carries those, so every case fell through to a raw-frame fallback and the
routing table was never exercised by a frame the pump could send.
…sked for
Three places compared a reply against the Object and Sub-ID that were
requested - the decoder's routing table, the stream-detection flags, and
the frame parser's telemetry set. A reply carries neither, so none of
them could ever match a frame the pump sends:
- TelemetryDecoder.decode() fell through every case to a raw-frame
fallback, which is what has actually been decoding telemetry.
- _has_motor_state_stream and _has_flow_stream were never set by a
notification, so the polling path they exist to suppress ran whether or
not the pump was already streaming.
Measured 2026-08-20 by issuing each read and recording bytes 6-9:
motor state type 3 v1 (48-byte reply)
flow / head type 0x3502 v2 (43-byte reply)
temperatures type 0x1602 v2 (20-byte reply)
alarms type 0x3A01 v2 (9-byte reply)
warnings type 0x3A01 v2 - the same
Alarms and warnings are indistinguishable in a reply: reading 88/0 and
88/11 returned byte-identical frames. So they are deliberately absent
from the router, which cannot label what came back. Only the caller that
issued the read knows, and read_alarms() already decodes them itself.
The pump's own three-byte [00][00][size] header is stripped by a new
ParsedFrame.object_body. The previous code reached the same bytes by
selecting offset 13 when byte 5 was in {0x30, 0x2B, 0x14, 0x2E, 0x2D,
0x09} - which are the payload lengths of the three telemetry registers,
so it worked for exactly those three and mis-sliced anything else of the
same size.
Fixtures across the suite were built from the same wrong reading and so
agreed with the code rather than with the pump. They now use tests/wire.py
or a recording.
… anyone reads it
Reassembly
----------
0x27 was accepted as an inbound frame start, described as "request/echo".
The pump does not echo: all 22,062 pump-to-phone frames in the reference
corpus start 0x24 and none start 0x27.
A frame start now begins a new packet only when reassembly is not already
under way. A mid-frame fragment can perfectly well begin 0x24 - it is an
ordinary payload byte - and treating it as a start discarded the frame in
progress and dispatched the fragment as a runt. Verified here: a reply
whose payload is ten 0x24 bytes now reassembles byte-identically.
Frames that stop arriving are abandoned after a second, so a truncated
one cannot wedge the buffer for the life of the connection.
The declared length is now bounded at both ends. There was no minimum, so
a length byte of 0x00 declared a four-byte frame and any notification
"completed" instantly. The floor is 5, from the nine-byte Class 10
acknowledgement 24 05 F8 E7 0A 01 00 AE A2. The ceiling was 256, three
short of the 257-byte maximum telegram, and it ran *after* the packet had
been queued and every handler called; it now runs before, and drops only
the partial frame - losing frame sync says nothing about whether the pump
will answer commands already sent.
A second frame arriving in the same notification is fed back rather than
swallowed into the first one's payload.
CRC
---
The CRC was computed and never read. validate_frame_integrity() was its
only consumer and had no call site, so every write verdict was decided by
reading unverified bytes back. Frames are now trimmed to their declared
length - the completion test is >=, so trailing bytes sit outside what
the CRC covers - and dropped if the checksum does not match, with a
counter behind it.
Class 7 strings
---------------
The header is six bytes, not seven: byte 5 is the string's byte count and
the text starts at offset 6. The reply does not echo the string ID.
Reading from offset 7 dropped the first character of every string. Two
were patched up afterwards and so looked fine - "LPHA HWR" had an "A"
prepended, and a serial reading "0000479" had a "1" prepended, which was
right for this unit only by luck. The versions had no such patch and have
been shipping a character short. Measured on the pump, before and after:
software 2601618V04.02.01.02539 -> 92601618V04.02.01.02539
hardware 2601617V01.03.00.00469 -> 92601617V01.03.00.00469
BLE 2811431V06.00.01.00001 -> 92811431V06.00.01.00001
and the name and serial are now right without being rewritten.
The count is checked against the frame length and logged on mismatch, but
deliberately not used to bound the read: it is radio-supplied.
Several fixtures encoded the truncated strings, having been copied out of
this client's own output, so they agreed with the bug.
Verified end to end against the pump: device info, telemetry and a mode
read all correct over a live link, with zero CRC failures.
… it refused
The dedicated Class 10 setpoint write was refused, always
--------------------------------------------------------
_set_class10_setpoint() built [0A][84][SubH][SubL][ObjH][ObjL][f32] -
sub-id first, where every Class 10 SET this pump accepts is object first.
So it named object 0x00. Sending the exact frame the method produced:
-> 27 0C E7 F8 0A 84 00 27 00 56 38 84 4F 4B EA CC
<- 24 07 F8 E7 0A 81 00 4F 40 4E 81
0x81 is Unknown Data Item with one payload byte, and that byte is 0x00:
the object it could not find. Every setpoint write this client has made
since the method existed was refused, invisibly, because the send was
fire-and-forget and _send_with_retry() reports success even on a timeout.
Correcting the address would not have helped. Sub-ids 13, 15, 17 and 39
are type 301, a 28-byte struct of seven floats, and a SET to a typed
object must carry the type word and size ahead of the body.
It is also unnecessary: the fused Object 86 Sub 6 request already carries
the setpoint, which is how the GO app sets one - 25 times in the capture
corpus, each followed by an Object 84 Sub 1 overview commit.
The bounds were wrong on every mode, in both directions
-------------------------------------------------------
The pump publishes them in the type 301 objects at Object 86 sub 13, 15,
17 and 39. Read from it:
constant speed 1650 - 3671 RPM (validated against 500-4500)
constant pressure 1.000 - 2.450 m (validated against 0.5-10.0)
proportional pressure 2.599 - 4.569 m (validated against 0.5-10.0)
constant flow 0.114 - 2.498 m³/h (validated against 0.1-10.0)
Proportional pressure was the worst: a 0.5 m floor against a real 2.6 m,
in a range that does not overlap constant pressure's - and the two shared
one constant here. Verified after the change: 1.5 m proportional pressure
is now refused and 3.0 m accepted.
The chain is sequential and stops at the first failure. All four objects
answer with the same type code, so the transport cannot tell their replies
apart; carrying on would hand read N's late reply to read N+1 and bound
constant pressure by constant speed's 1650-3671 read as Pascals.
The old constants stay as a fallback, used only until the pump has been
read, because refusing a setpoint the pump would have taken is worse than
letting it clamp one it dislikes.
set_flow_limit wrote to the wrong object entirely
-------------------------------------------------
It wrote Object 86 Sub 39 - the constant-flow setpoint range - with a
GPM-scaled value, through the refused frame above. The real limiters are
type 895/896/897 at Object 86 sub 600/620/640, and reading all sixty
declared sub-ids shows only two exist: every one past the second answers
OPERATION_FAILED. The name enum gives MaxFlow = 1, MinFlow = 2, so the
instances are per limiter, not per mode.
It is replaced by read_limiters() and `alpha-hwr control limiters`. The
write is not reimplemented: enabling a limiter silently caps delivered
flow, and that is not a change to make as a side effect of a protocol
sync. On this pump both are disabled and neither is limiting, which is
why setpoints here are delivered as written.
Answers ESPHome issue #274, and the measurement half of #276.
Ten packets went out on every connection, documented as a three-stage
unlock. Decoded under the rule that byte 5 is 0booLLLLLL, all four
distinct packets are reads:
27 07 E7 F8 02 03 94 95 96 EB 47 Class 2 GET of unit_family,
unit_type, unit_version
27 07 E7 F8 0A 03 56 00 06 C5 5A Class 10 GET of Object 86 Sub 6
27 05 E7 F8 05 C1 4B C3 82 INFO query, Class 5 item 0x4B
27 05 E7 F8 0B C1 0F D0 C3 INFO query, Class 11 item 0x0F
The "unlock code 0x96" was a length field. 0x03 is an APDU head declaring
a GET with three payload bytes, and 94 95 96 are three item IDs. A read
cannot change device state, so an unlock was never something these bytes
could perform - and every reply was discarded unread anyway.
Verified on the bench: with none of them sent, a bare connect-and-
subscribe link answered all five Class 7 strings, every Class 10 object
read this client makes, and the three telemetry registers. Device info,
telemetry and a mode read all come back correct over a link that has
written nothing but the reads themselves.
The 750 ms of inter-stage delays went with them. They were transcribed
from this client's own sleep() calls and then written up as pump timing
requirements; nothing ever measured them. The 500 ms settle before the
first command stays, because that is about the radio rather than GENI.
The four packets are kept as constants: they are real captures and make
good frame-assembly vectors. They are now labelled as the reads they are,
here and in the generated test-vector page.
authenticate() keeps its name - callers use it, and the session still has
a state to move through - but it no longer returns False for a "handshake
failure" that could not have happened. A pump that will not answer shows
up as an unanswered read, which is where it can be diagnosed.
…er for 400 ms
Measured against an ALPHA HWR on 2026-08-20, with no-op write-backs to
Object 84 Sub 1 and Object 91 Sub 430 so nothing changed on the pump:
* The SET itself draws no reply. Not a late one - none. Zero frames in
six seconds, three runs, both objects.
* Nothing else is answered either, for 200-400 ms afterwards. A read at
+50, +100 or +200 ms goes unanswered (0/3 each); the same read at
+400 ms and beyond answers in ~55 ms (3/3 each). The link stays up and
the write is applied.
Both comments in this client said the opposite - that the acknowledgement
"usually lands after the response window has closed", explained as a
two-phase commit. Nothing lands, and the real effect is the quiet period,
which neither comment mentioned.
This also explains the Grundfos GO app's afterSetSendPause of 2500 ms
(esphome-alpha-hwr #250): a conservative version of the same rule.
The ESPHome port concluded the opposite from the capture corpus - that
every Class 10 write is answered in 36-193 ms - and that number looks like
an artifact of how it was derived. geni_capture_scan pairs each frame with
the next one in the opposite direction, and the GO app polls continuously,
so the frame arriving 36 ms after a SET is the reply to a GET sent *before*
it. 36 ms is also below the 200 ms floor measured here, which no reply to
the SET could be.
What this client was doing
--------------------------
Waiting a full second for each of those acknowledgements, then proceeding.
The wait was pure loss, and the client depended on it without knowing:
a second is longer than 400 ms, so the accidental delay was the only thing
keeping the next frame out of the deaf window. Removing the wait without
adding the quiet period would have started dropping reads - and the reads
in question include the one that fetches the limits tail a temperature
write must echo back.
The quiet period is now armed in Transport.write(), keyed off the frame's
own class and APDU operation bits rather than a list of addresses, so no
call site can forget it. A temperature-range write settles accepted in
2.34 s, verified by readback.
Doctests
--------
185 of 279 were failing. Seven were real:
encode_float_be(1.5) claimed b'\x3f\xc0\x00\x00'; Python prints ?
decode_float_be(b'\x00\x00') claimed to print None, which it does not
build_command_info(0x02, 0x45) claimed '27050e7f8020345...'
a 3-byte register read's frame length was given as 9; it is 11
three Session examples used a session nobody had built
one expected a ConnectionError without a traceback
The rest were never executable - await at the top level, or a client that
does not exist - so they now carry # doctest: +SKIP and say what they are.
tests/test_doctests.py runs the remainder, with a floor on the count so
the failure mode cannot come back as "skip everything".
…does Settles what the post-SET quiet period actually suppresses. The window was measured on hardware - the pump answers nothing for 200-400 ms after a Class 10 SET - but whether a *write* sent inside it is processed or dropped was left open, because the direct test is confounded: a raw Obj 91 Sub 430 write does not persist without the Obj 86 Sub 10 mode request in front of it, whatever the commit timing. The captures and the decompiled app answer it instead. The GO app sends consecutive SETs back to back. Across the corpus there are 289 consecutive SET-to-SET pairs, min 43 ms, median 62, and 267 of them under 200 ms - well inside the window. The tightest include 84/1000 -> 84/1001 at 43 ms, which is a schedule layer upload, and 86/10 -> 91/430 at 54 ms. Those uploads write five layers and commit, and they work. If a SET inside the window were dropped, every schedule upload would lose four of its five layers and every single-event save four of its five slots. The app's own scheduler says the same thing directly. In DongleHelper.handleOutgoingQueue the pause is armed only when a SET is followed by a non-SET, and the else branch sets noSentBefore = 0L - it clears the pause outright for a following write. The 2500 ms is a read guard, not a write guard. GENIbus agrees in principle: the Application Programming Manual promises a reply per request and notes "the SET operation never returns anything but the APDU Head", so the pause is about being able to read a reply rather than about the write landing. This pump is more silent than that - it returns not even the head - but the shape of the rule holds. So the hold is skipped when the next frame out is itself a Class 10 SET. A five-layer schedule upload was otherwise going to spend two seconds waiting for nothing. Verified on the pump afterwards: a temperature-range write still settles accepted, confirmed by readback, and the schedule flag is unchanged.
…rt read Two halves of the same defect, and the second is only reachable once the first is fixed. Nothing woke a pending read when the link dropped ------------------------------------------------- read_response() waited on the queue with a timeout and nothing else, so a caller sat out its full three seconds for a pump that had gone. It now races the reply against a link-down event, and reports a drop as a drop rather than as a timeout: 0.1 s instead of 3.0 s in the test that pins it. The flag is cleared when notifications start, so a reconnect does not inherit it, and any partial frame from the old link goes with it. A frame that lands in the gap between the wait ending and the result being read is put back on the queue rather than lost with the cancelled task. A partial chain looked exactly like less data ---------------------------------------------- get_all_entries() skipped entries it could not read - correctly, because a log with twelve entries reports the other eight as unreadable - and a dropped link took the same path. The result was a short list and "Retrieved 5/20", which is what a five-entry log looks like. Nothing downstream could tell them apart. get_trend_data() had the same shape: three of its four series are legitimately None on some pumps, so a half-built collection did not look wrong either. Both now let ConnectionError through instead of degrading it to "no data", and both say how far they got. Measured on the pump by dropping the link 0.35 s into a full event-log read: Pump disconnected while reading the event log after 4 of 20 entries: Pump disconnected from BLE while reading Object 88/10204 against the 20 entries in 1.3 s that a healthy link returns. The end-of-loop check is not redundant with the raise: a read already answered when the drop lands returns normally, so the loop can run to completion over a link that died half way. One ConnectionError, not two ----------------------------- This surfaced while fixing the above. The package shadows the builtin name, and which class `raise ConnectionError(...)` produced depended on whether that file happened to import exceptions.py - base.py and client.py raised the package's, session.py and time.py the builtin - so no single except clause caught both, and importing exceptions.py into a module silently changed the type it raised. It now subclasses both.
Retracting two claims made three commits ago, and the code built on them.
What I claimed, from a hand-rolled bleak probe:
* a Class 10 SET is never acknowledged - zero frames in six seconds,
three runs, two objects;
* the pump then answers nothing at all for 200-400 ms.
Both are artifacts of the probe. It wrote each GENI frame in a single
write_gatt_char call, and this pump ignores a frame that is not split into
20-byte GATT writes - whatever the ATT MTU has been negotiated to. On this
link the MTU is 65, so a 27-byte frame fits at the ATT layer and still does
nothing:
Object 84 Sub 1 SET, one 27-byte write -> no reply
the same bytes, chunked at 20 -> acked in 111 ms
Object 84 Sub 1 GET, 11 bytes, one write -> answered in 69 ms
Reads are 11 bytes and fit, which is why every read in that probe worked
and every write did not. The writes were never arriving. The "deaf window"
was the pump's own reassembly timer recovering from the truncated frame -
the same mechanism as esphome-alpha-hwr #200, seen from the other side.
Measured properly, through this client, capturing every inbound frame
during a temperature-range write:
Object 86 Sub 10 +119.6 ms 24 05 F8 E7 0A 01 00 AE A2
Object 91 Sub 430 +119.8 ms 24 05 F8 E7 0A 01 00 AE A2
Object 84 Sub 1 +89.8 ms 24 05 F8 E7 0A 01 00 AE A2
That agrees with the capture corpus (36-193 ms, nothing over 295 anywhere)
and with the GENIbus manual, which says a SET returns nothing but the APDU
head - which is exactly this nine-byte frame. The ESPHome port's conclusion
was right and my doubt about it was not.
So POST_SET_QUIET and the skip-the-hold-between-writes logic are gone, and
the acknowledgement wait is back, with SET_ACK_TIMEOUT = 0.4 chosen against
the numbers above rather than against a story. A temperature-range write
settles accepted in 1.68 s, down from 2.34 s under the quiet period.
What survives, and is worth keeping: BLE_MTU_LIMIT = 20 is a pump
requirement rather than a guess about the radio. That is now written down
and pinned by tests, because it is exactly the kind of constant somebody
optimises away after checking the negotiated MTU.
The lesson for the bench notes: a negative result from a hand-rolled probe
is only as good as the probe. Check it can do something known to work
before believing what it says is impossible.
…p kept Single events ------------- The APDU head was 0xB3 - SET with 51 payload bytes - borrowed from the schedule layer write, whose 53-byte APDU really does carry 51. A single event carries 19, so it is 0x93. The capture corpus settles it: all 29 single-event writes the GO app makes use 0x93, all 8 layer writes use 0xB3. The pump takes either, so nothing was visibly failing. Writes now read the slot back and compare the ACTION byte along with the window and the enabled flag. ACTION is half the meaning: 0x01 holds the pump off across the window - that is what a vacation is - and 0x02 runs it once, so a confirm without it settles a vacation as written while the pump is scheduled to run for a week. Skipped on a clear, which disables the slot whatever it held. clear_vacation() ignored the clock, clearing the first enabled Stop event in slot order. A finished vacation in an early slot therefore shadowed a live one: success reported, pump still off. find_free_slot() one method up had always been clocked - the asymmetry was the bug. A wholly-past window is refused rather than spending one of five slots on an event that can never run; one already underway is still accepted, since only the end decides that. Slot bounds are two checks in a deliberate order: the protocol envelope first and without reading the pump (sub-id 900 + slot, and the schedule layers start at 1000, so slot 100 is layer 0), then the pump's own count from the overview. The other order blames the link for an argument that could never have been right. Timestamps are bounded to the uint32 the wire carries. build_apdu used to raise OverflowError from inside a try that caught only read errors. What was checked and left alone ------------------------------- The ESPHome port's DST readback fix does not apply. Its bug is a consequence of converting the pump's local-Unix timestamps to UTC, and this encoding does not convert - it stamps the wall-clock fields as though they were UTC and reads them back the same way, which is how the pump stores them. Verified at 15-minute steps across both 2026 US Pacific transitions: 0 mismatches, and neither function mentions a timezone. There is now a test saying so, because "fixing" it by adding a UTC conversion is how the other implementation acquired the bug. Midnight-crossing windows were already accepted (only degenerate begin == end is refused), the schedule enable leg was already confirmed by readback, and find_free_slot already read every slot before choosing. Clock ----- The write is Object 94 Sub 100, type 321 version 2. The constant holding its first six bytes was named _TYPE_322_HEADER; 322 is the type the read of Sub 101 answers with, pasted onto the write. Those six bytes are also not a header - they are the tail of the address, the size field and the struct's leading byte, which the comment now shows against the emitted frame. The confirm window is documented rather than tightened. dt is sampled before the frame goes out, so the pump legitimately reads behind it; measured over five runs the pump landed exactly on the requested second every time and read 0.8-1.6 s behind the host by readback, which is the client's own latency. Tightening the 5 s bound would start failing on it. Event-log timestamps were checked against the 2000-epoch floor history.py applies and do not need it: they decode as true Unix epochs (2026 dates) straight off the wire.
A dropped frame is the system working - a bad CRC caught is a corrupted frame that did not become a write verdict. What was missing is any way to know it happened: a link quietly shedding frames is indistinguishable, from outside, from a client that occasionally times out for no reason. Only CRC failures were counted. There are five other paths that discard inbound bytes, and they are now counted separately, because collapsing them would make a framing bug look like radio interference: crc_failures a corrupted link runt_length_drops a peer declaring a length no telegram can have unsolicited_fragments bytes that start no frame - usually lost sync stale_partials a frame that stopped arriving overflow_drops reassembly past the maximum telegram queue_full_drops responses arriving faster than they are read Exposed together as transport.frame_drops. This is the counter esphome-alpha-hwr #260 asks for, in the shape that issue describes.
23,579 frames over 25 minutes, nothing dropped on any of the six paths. The 95% upper bound on the drop rate is better than one in 7,860; at the one-in-5,900 the ESPHome port saw, four drops would have been expected. Replaces a 167-frame figure that could not tell zero from one in 5,900, and says plainly what it does not establish: the ESPHome number counts log lines rather than frames, and a radio result on a macOS host does not transfer to an ESP32.
The unresolved question was whether the event log's timestamps are true
UTC or local-as-UTC like single events. The GENI profile settles it, and
the answer is structural rather than a measurement that could have gone
either way.
* DateTimeActual (type 322) carries dst_status, and the bench unit
reports SummerTime. A device tracking whether it is in summer time is
keeping local time; UTC has no summer.
* DaylightSavingTime (type 323, Object 94 Sub 102) reads enabled on the
bench unit, second Sunday of March to first Sunday of November, with a
60-minute offset - the US rule. The pump shifts its own clock.
* There is no timezone or UTC-offset field anywhere in the profile.
Searching the whole of geni_profile_52_7.xml for timezone, UTC, GMT or
offset returns electrical offsets and alarm names and nothing else.
The third point is what decides the timestamps. The pump compares a stored
begin/end against its own clock and has no offset to relate two bases, so
they must be in the same one, and its clock's base is local. Read against
the pump: its clock matched host local time to the second.
That agrees with the earlier behavioural finding - an event written under
this encoding started four seconds from its intended wall clock - and
explains why, rather than leaving it as a lucky guess.
What was wrong here
-------------------
set_clock and the single-event encoding were already right. The event log
and the trend history decoded with fromtimestamp(ts, tz=UTC), which gives
the correct digits attached to the wrong instant: .astimezone() on one
shifts it by the local offset and produces a time the pump never meant.
All four surfaces now go through alpha_hwr.pump_time, and a test asserts
that none of them decodes a timestamp itself - checked against the parsed
code rather than the text, so the prose explaining why not does not trip
its own rule.
Why it is worth a module of its own
-----------------------------------
This is an interoperability rule, not a preference. The GO app, the
ESPHome component and this library all write the same clock, and the pump
cannot say which base a value arrived in. Two clients disagreeing is worse
than either being wrong alone: one sets the clock, the other resets it by
the local offset, and every stored schedule fires at the wrong hour. The
module says so, at length, next to the code it governs.
Also recorded: because the pump shifts its own clock at a DST transition,
a stored event keeps its wall clock across the boundary - an 07:00 event
stays at 07:00. Almost certainly the intent, and another thing true-UTC
storage would break.
Five independent datetime.now() calls decided single-event slot expiry, window validity, and what to write to the pump's own clock. Nothing was wrong with any of them individually. But esphome-alpha-hwr #262 was caused by one caller substituting the wrong timestamp for "now", and #270 files the general condition: independent notions of now are what make that class of bug easy to reintroduce. They now share pump_time.now(), which is documented as answering a specific question - the wall clock the *pump* runs on - as against "when did this host event happen", which telemetry stamps and session timings ask and which is correctly UTC-aware. Mixing those two is the mistake. No "clock not set" sentinel, and none needed: a host always has a clock. The ESPHome port needs one because an ESP32 may genuinely not know the time, so its rule - a picker that cannot tell the time refuses to guess - has no analogue here. Recorded rather than left to be inferred from the absence.
Caught by mypy immediately after the accessor consolidation: renaming the five datetime.now() call sites hit a local variable in _clock_view that was also called pump_time, so pump_time.now() resolved against the pump's clock value rather than the module. Renamed the local to pump_clock. The None check also moves ahead of the first use, where it should have been - the shadowing had put a method call on a possibly-None value above its own guard.
Reverses the pre-wire refusal added with the setpoint-range work. The
bounds it checked were right - they are the pump's own - and checking them
was still wrong.
This pump does not reject a setpoint it dislikes. It takes it and clamps
it, and reports what it stored. Measured through the client against a
published range of 1650-3671 RPM:
4000 RPM -> clamped stored 3671
2000 RPM -> accepted stored 2000
600 RPM -> clamped stored 1650
which reproduces the 0.7.0 note ("600 RPM is stored as 1650 and 4400
stores 3671") from a separate run. Letting the pump answer tells the caller
what happened; refusing tells them only what we guessed.
There is a second reason, raised by @jfriend00 on esphome-alpha-hwr #276,
and it is the one that makes this a correctness matter rather than a
preference. With a flow limiter enabled **there is no maximum speed**. The
pump accepts the speed setpoint and then manages the actual run speed to
hold the flow bound, and where it settles is a property of the
installation's hydraulics rather than of the pump - on their loop a 3000
RPM request delivered 1885. No number is the bound there, and the type-301
range does not know it, because it is the *factory* range. A check that
looked authoritative would be wrong in a way this client cannot detect.
So the published range is now an explanation rather than a gate. It goes
in the settle detail when a clamp happens, and only when it came from the
pump:
clamped: pump stored 3671; its range for this mode is 1650-3671 RPM
Still refused before the wire: a value that is not a number. There is
nothing there for the pump to clamp to, and the all-ones float doubles as
the SETPOINT_KEEP sentinel, so a NaN would read as "leave the setpoint
alone" - a write that silently does nothing rather than one that fails.
This also tidies the reason the pre-wire check existed at all. It was there
so that a bare False from a setter could not mean both "out of range" and
"the link failed", which are opposite answers to "should this be retried".
With the range check gone, False means the second and only the second.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR syncs alpha-hwr’s protocol understanding and client behavior with recent bench-verified findings, primarily by correctly treating the APDU head as a combined operation/ack + payload-length field and by matching Class 10 replies on type rather than requested object/sub-id.
Changes:
- Corrects Class 10 parsing/matching/routing to use APDU-head length semantics and reply type codes, replacing multiple test fixtures with captured-on-wire frames.
- Removes the previously-assumed authentication “handshake”, enforces inbound CRC validation, and adds transport frame-drop observability.
- Introduces a shared local-wall-clock timebase (
pump_time) and tightens write verification + partial-read error handling; updates CLI/docs accordingly.
Reviewed changes
Copilot reviewed 66 out of 66 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/wire.py | New realistic frame builder + captures |
| tests/unit/test_pump_time.py | Enforces naive local timebase usage |
| tests/unit/services/test_write_operation.py | Setpoint clamping + NaN refusal tests |
| tests/unit/services/test_single_event_rules.py | Single-event write rules/regressions |
| tests/unit/services/test_partial_reads.py | Drop-mid-chain now raises |
| tests/unit/services/test_cache_sync.py | Cache sync includes range reads |
| tests/unit/protocol/test_telemetry_decoder.py | Decode via captured frames/types |
| tests/unit/protocol/test_matcher.py | Type-order matching semantics |
| tests/unit/protocol/test_frame_parser.py | Type fields + length/CRC assertions |
| tests/unit/core/test_transport_write.py | MTU chunking + drop counters tests |
| tests/unit/core/test_device_info_service.py | Class 7 replies shaped correctly |
| tests/unit/core/test_base_service.py | Class 7 string offset fix tests |
| tests/unit/core/test_authentication.py | Pins “authenticate writes nothing” |
| tests/test_temperature_control.py | Replaces flow-limit write w/ read_limiters |
| tests/test_telemetry_integration.py | Integration uses class10_reply builder |
| tests/test_schedule_service.py | Write ACK semantics clarified |
| tests/test_protocol_expanded.py | Removes invalid synthetic Class 10 frames |
| tests/test_packet_structure.py | Type-based assertions + object_body |
| tests/test_packet_creation.py | Uses class10_reply for telemetry vectors |
| tests/test_mock_pump.py | Decode through router/object_body |
| tests/test_doctests.py | Doctest enforcement + floor |
| tests/test_device_info.py | Captured Class 7 string fixtures |
| tests/test_control_service_extended.py | Out-of-range reaches pump semantics |
| tests/test_client_telemetry.py | Notifications via class10_reply builder |
| tests/test_advanced_telemetry.py | Captured frames + alarms/warnings handling |
| tests/reference/test_protocol_vectors.py | Fixes length/address/APDU-head fixtures |
| tests/mocks/mock_pump.py | Mock emits correct lengths/CRC/type fields |
| src/alpha_hwr/services/write_operation.py | NaN invalid; clamp reported w/ range |
| src/alpha_hwr/services/time.py | Uses pump_time.now(); clarifies type |
| src/alpha_hwr/services/telemetry.py | Stream detection by reply type |
| src/alpha_hwr/services/single_event.py | Uses pump_time + confirm/readback rules |
| src/alpha_hwr/services/schedule.py | Adds measured SET ack timeout |
| src/alpha_hwr/services/history.py | Decodes timestamps via pump_time; raises on drop |
| src/alpha_hwr/services/event_log.py | Raises on link drop mid-read |
| src/alpha_hwr/services/device_info.py | Removes string “patchups”; fixes offset |
| src/alpha_hwr/services/configuration.py | Doctest skips for non-runnable examples |
| src/alpha_hwr/services/base.py | Class 7 decode offset + docs |
| src/alpha_hwr/pump_time.py | New shared wall-clock codec |
| src/alpha_hwr/protocol/telemetry_decoder.py | Type-based routing; captured-based tests |
| src/alpha_hwr/protocol/matcher.py | Type-based matching; short-ack via length |
| src/alpha_hwr/protocol/frame_builder.py | Doctest fixes; correct lengths |
| src/alpha_hwr/protocol/codec.py | Doctest output corrections |
| src/alpha_hwr/protocol/apdu.py | New APDU-head semantics helpers |
| src/alpha_hwr/exceptions.py | ConnectionError unification (builtin + custom) |
| src/alpha_hwr/event_log.py | pump_time decoding for timestamps |
| src/alpha_hwr/core/session.py | Doctest corrections/traceback expectations |
| src/alpha_hwr/core/authentication.py | Removes handshake; settle-only behavior |
| src/alpha_hwr/constants.py | Doctest skips updated |
| src/alpha_hwr/client.py | Doctest skips updated |
| src/alpha_hwr/cli/common.py | Doctest skips updated |
| src/alpha_hwr/cli/commands/control.py | CLI: remove set-flow-limit; add limiters |
| src/alpha_hwr/cli/commands/clock.py | Uses pump_time.now(); variable rename |
| scripts/generate_test_vectors.py | Updates opening-packet labels |
| pyproject.toml | Ruff per-file DTZ ignores for pump-time tests |
| docs/reimplementation/test_vectors.md | Updates “handshake” terminology |
| docs/reimplementation/layer_by_layer.md | Docs: authenticate sends nothing |
| docs/protocol/packet_traces/01_connection.md | Troubleshooting: bonding guidance |
| docs/protocol/control_modes.md | CLI/docs updates for limiters |
| docs/protocol/connection.md | Docs: no handshake sent |
| docs/guides/control_modes.md | CLI examples updated |
| docs/guides/cli_guide.md | CLI command list/examples updated |
Suppressed comments (1)
src/alpha_hwr/protocol/telemetry_decoder.py:726
- The default branch still calls decode_register_read_response() and decode_legacy_packet(), but those helpers interpret packet[5] as an OpSpec/opcode. After the APDU-head fix, packet[5] is a length field, so these fallbacks can misclassify unrelated Class 10 replies whose payload length happens to be 0x30/0x2B/0x14/etc (the exact bug this PR describes). Safer behavior is to return {} for unknown types (or implement a new fallback keyed on type codes).
case _:
# Unknown standard object - try register-read response decoder first
logger.debug(
f"Unknown telemetry object ({frame.obj_id}, {frame.sub_id}), "
f"trying register-read response decoder"
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
676
to
+680
|
|
||
| Examples: | ||
| >>> # Decode any telemetry frame automatically | ||
| >>> frame = FrameParser.parse_frame(notification_data) | ||
| >>> telemetry = TelemetryDecoder.decode(frame) | ||
| >>> if telemetry: | ||
| >>> frame = FrameParser.parse_frame(notification_data) # doctest: +SKIP | ||
| >>> telemetry = TelemetryDecoder.decode(frame) # doctest: +SKIP |
Owner
Author
There was a problem hiding this comment.
Fixed in 15b0648, and it ran further than the one docstring — thanks, this was a good catch.
Also stale in the same block: a Raises: ValueError: If frame is not a valid Class 10 frame that the function does not do. It returns an empty dict, and callers depend on that, because it means a notification stream carrying objects this does not route needs no filtering before it gets here.
And four Examples blocks elsewhere in the file still told a reader to branch on frame.obj_id == 87 and frame.sub_id == 69. They were +SKIPped so they could not fail, but this repository's docstrings are a porting reference, so those were teaching the exact pattern this branch removed from production.
The part I would not have found without pulling this thread. The module docstring's type numbers — 256, 565, 534, 570 — turned out to be right all along, and my naming was the misleading half.
Bytes 6-9 are [00][TypeH][TypeL][Version], so the type spans bytes 7-8 and the version is byte 9. type_high / type_low_ver split those same four bytes into two 16-bit halves one byte off that boundary — inherited from the ESPHome port, kept deliberately because comparing both halves is equivalent to comparing type and version together. That is true, and the matcher relies on it. But "type_low_ver 0x2F01" is not a type; it is the second byte of type 303 and its version, and I had propagated that phrasing into commit messages and docs.
ParsedFrame.object_type / .object_version now decode at the real boundary, and every captured frame lands on the number the vendor's own profile names:
00 01 00 03 -> 256 v3 ProtectedMotorStateDetails
00 02 35 02 -> 565 v2 PumpedMediaRelatedProcessValuesExtended
00 02 16 02 -> 534 v2 MediaTemperatureInfo
00 02 3a 01 -> 570 v1 FaultsByArrayExtended
00 01 2f 01 -> 303 v1 operation status
00 00 da 01 -> 218 v1 ClockProgramOverview
Pinned by a test that checks the decode against those profile definitions rather than against itself. The pair form stays in the matcher, where it is correct and load-bearing; prose now quotes the real type.
added 2 commits
August 20, 2026 17:24
CI runs BasedPyright as well as mypy, which is why this passed locally and
failed there. Six errors, all in the transport write tests, all mine:
* transport.client.write_gatt_char.call_args_list - client is typed
BleakClient, so the method is a MethodType and has no call_args_list;
* five calls passing None as the characteristic argument of
_notification_callback, which is typed BleakGATTCharacteristic.
Both came from a helper I added that rebuilt a mock client and a sleep
recorder the file already had. The fix is mostly deletion: the tests now
use the existing ble_client / transport fixtures and written_chunks(), and
the sleep recorder is a fixture rather than a fourth hand-rolled copy.
What survives is one `notify()` helper, because the cast there is real -
bleak passes a characteristic and the callback ignores it, so a test has
nothing meaningful to supply and building one to discard would be theatre.
The cast says so rather than hiding it.
Two smaller things the checker was right about: bytearray is not bytes
under this configuration, and the drop-counter tests need to be async even
though they await nothing, because _notification_callback reads
asyncio.get_event_loop().time().
Verified with basedpyright over every file this branch touches, not just
the one that failed.
…perly
Copilot flagged that TelemetryDecoder.decode()'s docstring still described
routing by Object and Sub-ID after the implementation moved to routing by
the reply's type. Correct, and the same drift ran further than the one
docstring.
Fixed there: the routing description, and a `Raises: ValueError` that the
function does not do - it returns an empty dict, which is the behaviour
callers depend on for a notification stream carrying objects it does not
know. The body now uses type_high / type_low_ver rather than the
deprecated obj_id / sub_id aliases.
Fixed alongside: four `Examples` blocks that still told a reader to branch
on `frame.obj_id == 87 and frame.sub_id == 69`. They were `+SKIP`ped so
they could not fail, but this repository's docstrings are a porting
reference and those were teaching the pattern this branch removed from
production. The alarms example now shows the call-site decode, since
alarms and warnings share a type and cannot be routed automatically.
And a naming trap I introduced
------------------------------
Chasing the above turned up that the module docstring's type numbers - 256,
565, 534, 570 - were right all along, and my names were the misleading
part.
Bytes 6-9 are [00][TypeH][TypeL][Version], so the type spans bytes 7-8 and
the version is byte 9. type_high and type_low_ver split those same four
bytes into two 16-bit halves one byte off that boundary - inherited from
the ESPHome port, which kept it deliberately because comparing both halves
is equivalent to comparing type and version together. True, and the matcher
relies on it. But "type_low_ver 0x2F01" is not a type; it is the second
byte of type 303 and its version.
ParsedFrame.object_type / .object_version decode at the real boundary, and
every captured frame lands on the number the vendor's own profile names:
00 01 00 03 -> 256 v3 ProtectedMotorStateDetails
00 02 35 02 -> 565 v2 PumpedMediaRelatedProcessValuesExtended
00 02 16 02 -> 534 v2 MediaTemperatureInfo
00 02 3a 01 -> 570 v1 FaultsByArrayExtended
00 01 2f 01 -> 303 v1 operation status
00 00 da 01 -> 218 v1 ClockProgramOverview
Pinned by a test that checks the decode against those profile definitions
rather than against itself. The pair form stays where it belongs, in the
matcher; prose quotes the real type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings the client back in step with
esphome-alpha-hwr, which has had 219 commits of bench work since the last sync (60e6f89, 2026-08-04). Every protocol claim here was measured against the pump rather than ported on trust, and several turned out differently from the ESPHome port's conclusions in both directions.The two decodes everything else follows from
The APDU head is a length, not an opcode. Byte 5 is
0booLLLLLL: operation or acknowledgement in the top two bits, payload byte count in the low six.byte5 == len - 8on every frame measured here.Two live bugs fell out. The set
{0x30, 0x2B, 0x14, 0x2E, 0x2D, 0x09}, carried as "register-read operation specifiers" and used to pick a payload offset, is really the payload lengths 48, 43, 20, 46, 45 and 9 — it worked because 48, 43 and 20 are exactly the three telemetry replies, and mis-sliced anything else that size. And0x81was read as an acknowledgement carrying an error code; it is Unknown Data Item, whose payload byte names the offending item, so a refused write read as accepted whenever that item was0x00— the case this pump produces.A response carries no Object ID and no Sub-ID. Bytes 6-9 are
[00][TypeH][TypeL][Version]. Matching discriminates types, not instances: Object 86 sub-ids 13, 15, 17 and 39 all answer00 01 2d 01, and alarms and warnings answer identically to each other. The "accept the two fields in either order" rule is gone — every measured reply matches in wire order, so nothing needed it, and accepting a transpose let unrelated objects answer each other's reads.Defects that were wrong in the field
→ 27 0C E7 F8 0A 84 00 27 00 56 …← 24 07 F8 E7 0A 81 00 …— Unknown Data Item, item0x00. It addressed sub-id first where every Class 10 SET the pump takes is object first. Invisible because the send was fire-and-forget and the retry helper reported success on timeout.software 2601618V… → 92601618V…. TheAprepended toLPHA HWRand the1prepended to the serial were patching the same off-by-one — the serial one correct only by luck.Retrieved 5/20is exactly what a five-entry log looks like. Now raises, naming how far it got.validate_frame_integrity()had no call site, so every write verdict was decided from unverified bytes.Where this diverges from the ESPHome port
Three places, each with the reasoning recorded next to the code:
type_high == 0as a wildcard. Zero is a real value — the schedule overview answers00 00 da 01— and it would reopen the collision already on record between the temperature-range config and an event log entry.One retraction, visible in the history
Commits
4fa639c,1ffa49aandcbf2cd3conclude that Class 10 SETs are never acknowledged and that the pump goes deaf for 200–400 ms afterwards. Both are wrong, andd2a671ereverses them and the code built on them.The cause is worth knowing: my probe wrote each GENI frame in a single
write_gatt_charcall, and this pump ignores a frame that is not split into 20-byte GATT writes, whatever the ATT MTU has been negotiated to — it is 65 here, so 27 bytes fits at the ATT layer and still does nothing. Reads are 11 bytes and fit, so every read in that probe worked and every write silently did not. Measured properly, every SET is acknowledged in 90–120 ms.BLE_MTU_LIMIT = 20is therefore a pump requirement, not a guess about the radio. That is now documented and pinned by tests, since it is exactly the constant someone optimises away after checking the negotiated MTU.I have left those commits in rather than rewriting them, because the correction explains a failure mode worth keeping: a negative result from a hand-rolled probe is only as good as the probe.
Verification
# doctest: +SKIPand a floor test stops the failure mode returning as "skip everything".calc_crc16(no final XOR, which matches no frame in either direction). They agreed with the code and disagreed with the pump.tests/wire.pybuilds correct frames;TEST_VECTORSare now recordings.docs/protocol/bench_findings.mdrecords what was measured and how, including the two findings I got wrong and why.Answering the ESPHome tracker
Findings from this work are posted on esphome-alpha-hwr issues #244, #250, #259, #260, #263, #265, #267, #268, #269, #270, #274, #276, #277 and #278 — including the limiter survey (#274), the pump's local-time model (#263), and the retraction above (#250).