feat(hamilton): complete TCP transport client on transport/tcp

CHANGELOG.md

@@ -17,9 +17,22 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 - `Plate`: optional `stacking_z_height` parameter -- the per-plate vertical pitch when plates are stacked directly on top of each other (`size_z` minus the nesting overlap), mirroring `NestedTipRack.stacking_z_height`. Because it is a physical dimension, plates that differ in it no longer compare equal; `Plate` also now serializes `stacking_z_height` and the pre-existing `plate_type` so both round-trip through `deserialize`/`copy`. (#1110)
 - `ResourceStack`: bare plates stacked in the z direction now nest into one another by their `stacking_z_height` (a stack of `N` identical plates is `size_z + (N - 1) * stacking_z_height` tall, for both `get_size_z()` and child placement). Plates without a `stacking_z_height`, and plates wearing a lid, do not nest, so existing behaviour is unchanged. (#1112)
 
+- Background reader task on `pylabrobot.hamilton.transport.tcp.HamiltonTCPClient` that owns the socket for the session, so `on_event` subscribers receive events between commands and a response arriving with no command waiting is dropped and logged instead of being handed to the next command (#1195).
+- Command serialization on `HamiltonTCPClient`: one command is in flight at a time. The lock spans write through terminal response and is released before the response is decoded, because error enrichment sends further commands through the same path (#1195).
+- `ObjectRegistry.clear()` (`pylabrobot.hamilton.transport.tcp.introspection`), used to drop path and address mappings that are scoped to a single connected session (#1195).
+
 ### Fixed
 
 - Imported `unittest.mock` in `pylabrobot/centrifuge/centrifuge_tests.py` (pre-existing bug that prevented the test class from running).
+- `HamiltonTCPClient` no longer retransmits a command after a failed read. A read timeout on a slow motion command previously re-sent it, which could execute the motion twice (#1195).
+- `HamiltonTCPClient.setup()` now resets all per-session state (client id, sequence numbers, instrument addresses, object registry) rather than carrying it into the new session, and refuses to run on an already-connected client instead of leaking the socket (#1195).
+- `HamiltonTCPClient` no longer recurses without bound when a device fails the introspection queries that error enrichment itself issues. Enrichment is now non-re-entrant and falls back to the static HC_RESULT tables, so a degraded instrument yields a terse error instead of a `RecursionError` (#1195).
+- `HamiltonTCPClient` no longer fails every command after the device sends a HARP control frame. MLPrep firmware sends one (options, no HOI body) shortly after registration; it was parsed as a command response and killed the reader. Frames with no routable message are skipped, as are unparsable frames, which is safe because frames are length-prefixed and consumed whole (#1195).
+
+### Changed
+
+- `HamiltonTCPClient` no longer reconnects automatically; `auto_reconnect` and `max_reconnect_attempts` are gone from its constructor. Recovery is `await client.stop()` followed by `await client.setup()`, matching every other transport in the library. `is_connected` remains for callers implementing their own policy (#1195).
+- `TCPCommand` declares `Response` and `uses_physical_channels` as class attributes instead of the transport inferring them by attribute probing. Commands with per-channel firmware errors must set `uses_physical_channels = True` to raise `ChannelizedError` (#1195).
 
 ## 0.2.1
 

pylabrobot/hamilton/nimbus/__init__.py

@@ -0,0 +1 @@
+"""Hamilton Nimbus support."""

pylabrobot/hamilton/prep/__init__.py

@@ -0,0 +1 @@
+"""Hamilton Prep support."""

pylabrobot/hamilton/transport/__init__.py

@@ -0,0 +1 @@
+"""Transport implementations for Hamilton devices."""

pylabrobot/hamilton/transport/tcp/__init__.py

@@ -1,24 +1,13 @@
 """Shared Hamilton TCP protocol layer for TCP-based instruments (Nimbus, Prep, etc.)."""
 
-from pylabrobot.hamilton.transport.tcp.commands import HamiltonCommand
-from pylabrobot.hamilton.transport.tcp.introspection import HamiltonIntrospection
-from pylabrobot.hamilton.transport.tcp.messages import (
-  CommandMessage,
-  CommandResponse,
-  HoiParams,
-  HoiParamsParser,
-  InitMessage,
-  InitResponse,
-  RegistrationMessage,
-  RegistrationResponse,
-)
-from pylabrobot.hamilton.transport.tcp.packets import Address, HarpPacket, HoiPacket, IpPacket
-from pylabrobot.hamilton.transport.tcp.protocol import (
-  HamiltonDataType,
-  HamiltonProtocol,
-  HarpTransportableProtocol,
-  Hoi2Action,
-  HoiRequestId,
-  RegistrationActionCode,
-  RegistrationOptionType,
-)
+from pylabrobot.hamilton.transport.tcp.commands import TCPCommand
+from pylabrobot.hamilton.transport.tcp.hoi_error import HoiError
+from pylabrobot.hamilton.transport.tcp.packets import Address
+from pylabrobot.hamilton.transport.tcp.tcp import HamiltonTCPClient
+
+__all__ = [
+  "Address",
+  "HamiltonTCPClient",
+  "HoiError",
+  "TCPCommand",
+]

pylabrobot/hamilton/transport/tcp/commands.py

@@ -1,64 +1,77 @@
-"""Hamilton command architecture using new simplified TCP stack.
+"""Command layer for Hamilton TCP.
 
-This module provides the HamiltonCommand base class that uses the new refactored
-architecture: Wire -> HoiParams -> Packets -> Messages -> Commands.
+TCPCommand base: build_parameters() returns HoiParams; interpret_response()
+auto-decodes success responses via nested Response dataclasses (wire-type
+annotations and parse_into_struct). Wire → HoiParams → Packets → Messages → Commands.
 """
 
 from __future__ import annotations
 
-import inspect
-from typing import Optional
+from dataclasses import fields, is_dataclass
+from typing import Any, ClassVar, Optional
 
 from pylabrobot.hamilton.transport.tcp.messages import (
   CommandMessage,
   CommandResponse,
   HoiParams,
+  interpret_hoi_success_payload,
+  log_hoi_result_entries,
+  split_hoi_params_after_warning_prefix,
 )
 from pylabrobot.hamilton.transport.tcp.packets import Address
 from pylabrobot.hamilton.transport.tcp.protocol import HamiltonProtocol
+from pylabrobot.hamilton.transport.tcp.wire_types import HcResultEntry
 
 
-class HamiltonCommand:
-  """Base class for Hamilton commands using new simplified architecture.
+class TCPCommand:
+  """Base class for Hamilton TCP commands.
 
-  This replaces the old HamiltonCommand from tcp_codec.py with a cleaner design:
-  - Explicitly uses CommandMessage for building packets
-  - build_parameters() returns HoiParams object (not bytes)
-  - Uses Address instead of ObjectAddress
-  - Cleaner separation of concerns
+  Preferred usage: define commands as ``@dataclass`` subclasses with
+  ``Annotated`` wire-type fields.  ``build_parameters()`` and
+  ``interpret_response()`` are handled automatically by the base class.
 
-  Example:
-      class MyCommand(HamiltonCommand):
+  Example::
+
+      @dataclass
+      class MyCommand(TCPCommand):
           protocol = HamiltonProtocol.OBJECT_DISCOVERY
           interface_id = 0
           command_id = 42
 
-          def __init__(self, dest: Address, value: int):
-              super().__init__(dest)
-              self.value = value
-
-          def build_parameters(self) -> HoiParams:
-              return HoiParams().i32(self.value)
+          dest: Address          # infrastructure field — not serialised
+          value: Annotated[int, I32]   # wire field — serialised in order
 
-          @classmethod
-          def parse_response_parameters(cls, data: bytes) -> dict:
-              parser = HoiParamsParser(data)
-              _, result = parser.parse_next()
-              return {'result': result}
+          @dataclass
+          class Response:
+              result: Annotated[int, U32]
   """
 
   # Class-level attributes that subclasses must override
+  # Not ClassVar: subclasses may redeclare these as per-instance dataclass fields
+  # when the value varies per command instance rather than per type (see
+  # PrepStatusRequest, which carries command_id on the instance).
   protocol: Optional[HamiltonProtocol] = None
   interface_id: Optional[int] = None
   command_id: Optional[int] = None
 
-  # Action configuration (can be overridden by subclasses)
+  # Nested dataclass describing the success payload, shadowed by subclasses that
+  # declare one. None means the command decodes its own response via
+  # parse_response_parameters().
+  Response: ClassVar[Optional[type]] = None
+
+  # Whether STATUS_EXCEPTION entries map onto PLR channel indices. Commands that
+  # carry per-channel wire parameters set this so the client raises
+  # ChannelizedError; everything else raises HoiError rather than attributing an
+  # instrument-wide fault to a synthetic ch0.
+  uses_physical_channels: ClassVar[bool] = False
+
+  # Action configuration (can be overridden by subclasses, per type or per instance)
   action_code: int = 3  # Default: COMMAND_REQUEST
   harp_protocol: int = 2  # Default: HOI2
   ip_protocol: int = 6  # Default: OBJECT_DISCOVERY
 
   def __init__(self, dest: Address):
-    """Initialize Hamilton command.
+    """Initialize TCP command.
 
     Args:
       dest: Destination address for this command
@@ -78,33 +91,35 @@ def __init__(self, dest: Address):
   def build_parameters(self) -> HoiParams:
     """Build HOI parameters for this command.
 
-    Override this method in subclasses to provide command-specific parameters.
-    Return a HoiParams object (not bytes!).
+    Default: serializes all ``Annotated`` wire-type fields on ``self`` via
+    ``HoiParams.from_struct``.  On non-dataclass subclasses ``from_struct``
+    finds no fields and returns an empty ``HoiParams``, preserving the old
+    behaviour.  Override only when the wire layout cannot be expressed with
+    ``Annotated`` field declarations.
 
     Returns:
       HoiParams object with command parameters
     """
+    if is_dataclass(self):
+      return HoiParams.from_struct(self)
     return HoiParams()
 
   def get_log_params(self) -> dict:
     """Get parameters to log for this command.
 
-    Lazily computes the parameters by inspecting the __init__ signature
-    and reading current attribute values from self.
+    Reads the declared dataclass fields. Non-dataclass subclasses declare no
+    fields and log nothing, matching ``build_parameters``.
 
     Subclasses can override to customize formatting (e.g., unit conversions,
     array truncation).
 
     Returns:
       Dictionary of parameter names to values
     """
-    exclude = {"self", "dest"}
-    sig = inspect.signature(type(self).__init__)
-    params = {}
-    for param_name in sig.parameters:
-      if param_name not in exclude and hasattr(self, param_name):
-        params[param_name] = getattr(self, param_name)
-    return params
+    if not is_dataclass(self):
+      return {}
+    exclude = {"dest", "dest_address"}
+    return {f.name: getattr(self, f.name) for f in fields(self) if f.name not in exclude}
 
   def build(
     self, src: Optional[Address] = None, seq: Optional[int] = None, response_required: bool = True
@@ -150,19 +165,64 @@ def build(
     # Build final packet
     return msg.build(source, sequence, harp_response_required=response_required)
 
-  def interpret_response(self, response: CommandResponse) -> Optional[dict]:
-    """Interpret success response.
+  def _channel_index_for_entry(self, entry_index: int, entry: HcResultEntry) -> Optional[int]:
+    """Map a ``HcResultEntry`` to a 0-indexed PLR channel, or ``None`` to skip.
 
-    This is the new interface used by the backend. Default implementation
-    directly calls parse_response_parameters for efficiency.
+    Default: the entry's position in the HoiResult — firmware populates arrays
+    in active-channel order. ``NimbusCommand`` / ``PrepCommand`` override this
+    to translate the active-channel ordinal into the caller's 0-indexed channel
+    via ``channels_involved`` bitmask or per-channel struct-array reflection.
+    """
+    return entry_index
+
+  def interpret_response(self, response: CommandResponse) -> Any:
+    """Pure decoder for a success response — never raises on channel errors.
+
+    For ``STATUS_WARNING`` / ``COMMAND_WARNING`` frames, strips the leading
+    summary + formatted-string prefix (per ``SystemController.SendAndReceive``)
+    and logs entries parsed via ``HoiDecoder2.GetHcResults``. For plain
+    ``STATUS_RESPONSE`` / ``COMMAND_RESPONSE`` frames, decodes the Response
+    dataclass directly — the firmware emits exactly the fields declared in
+    the interface yaml, with no HoiResult trailer. HoiResult only rides on
+    warning (prefix) or exception (separate payload, handled in
+    ``send_command``) frames.
+
+    Fatal (non-success, non-warning) entries from a warning frame surface
+    through ``fatal_entries_by_channel`` and are lifted into a
+    ``ChannelizedError`` by the backend — this decoder stays pure.
+    """
+    eff, _prefix = self._strip_warning_prefix(response)
+    return interpret_hoi_success_payload(self, eff)
 
-    Args:
-      response: CommandResponse from network
+  def fatal_entries_by_channel(self, response: CommandResponse) -> dict[int, HcResultEntry]:
+    """Return fatal entries keyed by 0-indexed PLR channel.
 
-    Returns:
-      Dictionary with parsed response data, or None if no data to extract
+    Only non-success, non-warning entries from a warning-frame prefix are
+    included; warnings remain log-only. Exception frames are handled
+    separately in ``send_command`` via :func:`~pylabrobot.hamilton.transport.tcp.hoi_error.parse_hamilton_error_entry`.
+
+    ``entry_index`` passed to ``_channel_index_for_entry`` is the position of
+    the entry in the *original* entries list (i.e. active-channel ordinal),
+    not among fatal entries only — so bitmask / struct-array overrides can
+    map ordinal → channel correctly even when earlier channels warned.
     """
-    return self.parse_response_parameters(response.hoi.params)
+    _eff, prefix_entries = self._strip_warning_prefix(response)
+    per_channel: dict[int, HcResultEntry] = {}
+    for i, entry in enumerate(prefix_entries):
+      if entry.is_success:
+        continue
+      ch = self._channel_index_for_entry(i, entry)
+      if ch is None:
+        continue
+      per_channel[ch] = entry
+    return per_channel
+
+  def _strip_warning_prefix(self, response: CommandResponse) -> tuple[bytes, list[HcResultEntry]]:
+    """Strip the warning-frame HoiResult prefix, if present. Logs entries."""
+    raw = response.hoi.params
+    eff, prefix_entries = split_hoi_params_after_warning_prefix(response.hoi.action_code, raw)
+    log_hoi_result_entries(type(self).__name__, prefix_entries, source="HOI prefix")
+    return eff, prefix_entries
 
   @classmethod
   def parse_response_parameters(cls, data: bytes) -> Optional[dict]:
@@ -177,3 +237,19 @@ def parse_response_parameters(cls, data: bytes) -> Optional[dict]:
       Dictionary with parsed response data, or None if no data to extract
     """
     return None
+
+
+def hamilton_error_for_entry(entry: HcResultEntry, description: str) -> Exception:
+  """Wrap an ``HcResultEntry`` in a ``RuntimeError`` using a pre-resolved description.
+
+  ``description`` is sourced from the device itself via Interface 0 method 5
+  (``EnumInfo``) — see ``HamiltonTCPClient._describe_entry``. The returned
+  exception has ``.entry`` attached so callers can dispatch on
+  ``entry.result`` / ``entry.interface_id`` / ``entry.address``.
+  """
+  err = RuntimeError(
+    f"{description} (HcResult=0x{entry.result:04X}) "
+    f"at {entry.address} iface={entry.interface_id} action={entry.action_id}"
+  )
+  err.entry = entry  # type: ignore[attr-defined]
+  return err