feat(hamilton): complete TCP transport client on transport/tcp - #1195
Open
cmoscy wants to merge 17 commits into
Open
feat(hamilton): complete TCP transport client on transport/tcp#1195cmoscy wants to merge 17 commits into
cmoscy wants to merge 17 commits into
Conversation
Replace the thin post-PyLabRobot#1000 stub with the HOI/HARP session client, command layer, wire types, and introspection stack so Prep/Nimbus can build on it.
Python 3.9 binds asyncio.Lock to the current loop at Socket construction, so HamiltonTCPClient cannot be created in sync TestCase methods. Use IsolatedAsyncioTestCase for those cases.
rickwierenga
force-pushed
the
hamilton-tcp-transport
branch
from
August 16, 2026 06:01
612142c to
8627f84
Compare
Member
|
also it seems like we should have a background reading thread like the star has that reads all incoming commands and then matches them to the specific command that was sent |
Member
|
until we figure out concurrency on the tcp protocol (see the locking mechanism for the star), I think we should have a lock to force one command at a time |
Member
|
also is it possible to get rid of hasattr etc.? |
…connect _send_raw retried by re-writing the command on connection errors. TimeoutError and OSError were both treated as retryable, and the read uses a 300s default timeout, so a slow motion command that timed out on the read was physically executed twice. Reconnection now belongs to the caller: stop() then setup(). No other transport in the repo self-heals a connection, and re-establishing the session cannot report whether the in-flight command completed. Session state is scoped to the connected session and fully reset by setup(): client id, sequence numbers, instrument addresses and the object registry all survived a reconnect before. setup() on a live client now raises instead of leaking the socket.
…eclared data wire_type_of() resolves an Annotated alias or bare WireType in one place, replacing four ad-hoc __metadata__ probes across messages.py and wire_types.py. TCPCommand now declares Response and uses_physical_channels as ClassVars. Response replaces a hasattr() dispatch; uses_physical_channels replaces duck-typing that inferred channel semantics by looking for a "channel" attribute on the first element of any list field. Device peers declare the flag; the transport no longer guesses. get_log_params reads dataclass fields instead of walking the __init__ signature and probing self. assert isinstance on wire-derived responses became real raises: asserts are stripped under python -O, which is exactly when a malformed frame most needs to fail loudly.
TCP-side concurrency semantics are not established for this protocol, so only one command is in flight at a time. _transact holds the lock across sequence-number allocation, build, write and the terminal read, then releases it before the response is decoded. That boundary is load-bearing: error enrichment resolves interface and method names through introspection, which sends further commands through _transact, so a lock spanning the whole of _send_raw would deadlock on the first firmware error. A test covers that path and fails with a timeout under the naive design.
Reading used to happen inline inside a command, which meant events were only observed while a command was in flight, and any non-ACK non-EVENT frame was accepted as the current command's response regardless of origin. A response arriving late, after a timeout, silently became the next command's answer. The reader owns the socket from the end of setup() until stop(). It dispatches events continuously, skips ACKs, and hands the terminal frame to the waiting command. Frames arriving with nothing waiting are dropped and logged rather than queued. The handoff is a single slot, not a keyed map: the command lock already allows one command in flight, and whether the device echoes the request sequence number is unverified. Mismatches are logged, so the pairing can be confirmed from real traffic before a keyed demux is built on it. Reads stay inline during the init and registration handshake, which exchanges Registration frames rather than commands. A reader that dies on a live connection fails the waiting command instead of hanging it.
…outing The transport had no coverage of connection lifecycle, retry, event dispatch or concurrency. Adds the regressions for the behaviour the preceding commits established: a command is written exactly once when the read fails, I/O on a disconnected client names setup(), setup() refuses to run twice, session state and sequence numbers reset between sessions, commands do not interleave, error enrichment does not deadlock against the command lock, events arrive between commands, unmatched frames are dropped rather than misdelivered, and a dead reader fails the waiting command.
Found against MLPrep firmware. Shortly after registration the device sends a HARP protocol-2 frame carrying options and no HOI body. _read_one_message routed every protocol-2 frame to CommandResponse, which unpacks a HOI header that is not there, so the reader died and every later command failed with "reader is not running". Inline reading never hit this because nothing read the socket between commands. _read_one_message now returns None for a frame with no routable message, and the reader skips unparseable frames instead of terminating. Frames are length-prefixed and consumed whole, so skipping one cannot desynchronize the stream. Also records what the same session established: the device echoes the request address and sequence number on every response (26/26), so the mismatch warning now documents a real invariant rather than an open question.
Turning a firmware error into a readable message asks the device for interface and method names. When those queries fail too, the failure was enriched the same way, which enriched again: a device answering STATUS_EXCEPTION to every request produced a RecursionError after 55 reads rather than an HoiError. Pre-dates this branch; reproduced identically at 4e71a71 and confirmed to need a device that fails Interface-0 queries, which is why healthy hardware never showed it. It surfaces exactly when the error message matters most. Enrichment is now non-re-entrant, guarded by a ContextVar so concurrent callers keep their own state. A nested entry falls back to HC_RESULT_PROTOCOL and terse addressing, so the caller still gets a real HoiError naming the failing address, interface and action.
…ilton-tcp-transport
…able Declaring protocol, interface_id, command_id and the action configuration as ClassVar forbids a subclass from redeclaring them as per-instance dataclass fields. Prep's PrepStatusRequest does exactly that by design, carrying command_id and interface_id on the instance so one class can serve many firmware methods. The ClassVar conversion was incidental to removing attribute probing and was not needed for it; Response and uses_physical_channels remain ClassVar because nothing overrides them per instance.
…waits Three reader tests queued response frames before sending the command. The reader could drain them first and correctly drop them as unmatched, leaving the command waiting until its timeout. Whether that happened depended on task scheduling: they passed on 3.14 and failed on 3.11, so CI would have caught them intermittently at best. Frames are now fed after the command registers its pending response, with an explicit wait for that registration. Also asserts the stale-frame test really did consume the frame it is about to prove was dropped, rather than passing because the reader had not run yet. Sorts imports after the error-table move, which CI checks via ruff check --select I in make format-check.
The typos CI check rejects 'unparseable'; the rest of the codebase already uses 'unparsable'.
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
2 participants
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.
Summary
Completes the Hamilton TCP/HOI client under
pylabrobot/hamilton/transport/tcp: protocol, wire types, introspection, error tables, and unit tests.Instrument-agnostic shared layer only — no Prep/Nimbus device peers. Prep (follow-up PR) builds on this.