Port Hamilton Vantage to new Device/Driver/Backend architecture#984
Open
rickwierenga wants to merge 11 commits intov1b1from
Open
Port Hamilton Vantage to new Device/Driver/Backend architecture#984rickwierenga wants to merge 11 commits intov1b1from
rickwierenga wants to merge 11 commits intov1b1from
Conversation
rickwierenga
force-pushed
the
port-vantage-v1b1-arch
branch
from
62108ee to
c6b9278
Compare
Decomposes the legacy monolithic VantageBackend (~5,334 lines) into the same layered architecture established by the STAR port: - VantageDriver(HamiltonLiquidHandler) — USB I/O, firmware protocol, setup - VantagePIPBackend(PIPBackend) — independent channel operations - VantageHead96Backend(Head96Backend) — 96-head operations - IPGBackend(OrientableGripperArmBackend) — plate gripper - VantageXArm, VantageLoadingCover — auxiliary subsystems - VantageChatterboxDriver — mock driver for testing - Vantage(Device) — user-facing device wiring capabilities All firmware commands verified parameter-by-parameter against the legacy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rickwierenga
force-pushed
the
port-vantage-v1b1-arch
branch
from
c6b9278 to
7bcbdbd
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Ports Hamilton Vantage support to the newer layered Device → Driver → CapabilityBackend architecture (consistent with the STAR port), while also advancing broader repo-wide refactors around capabilities, deprecation shims, and documentation updates.
Changes:
- Added new Hamilton Vantage driver/device/backends plus unit tests for firmware parsing + error conversion.
- Introduced/expanded the new
pylabrobot.capabilities.armsAPI and deprecated the legacypylabrobot.armsimport surface (plus doc updates). - Added multiple deprecation shims for legacy top-level modules and improved serialization for some resources.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pylabrobot/tilting/init.py | Deprecation shim re-exporting legacy tilting. |
| pylabrobot/thermocycling/init.py | Deprecation shim re-exporting legacy thermocycling. |
| pylabrobot/temperature_controlling/init.py | Deprecation shim re-exporting legacy temperature controlling. |
| pylabrobot/storage/init.py | Deprecation shim re-exporting legacy storage. |
| pylabrobot/shaking/init.py | Deprecation shim re-exporting legacy shaking. |
| pylabrobot/sealing/init.py | Deprecation shim re-exporting legacy sealing. |
| pylabrobot/scales/init.py | Deprecation shim re-exporting legacy scales. |
| pylabrobot/resources/trough.py | Adds serialize() overrides for trough metadata. |
| pylabrobot/resources/plate.py | Adds serialize() override to include non-default plate type. |
| pylabrobot/pumps/init.py | Deprecation shim re-exporting legacy pumps. |
| pylabrobot/powder_dispensing/init.py | Deprecation shim re-exporting legacy powder dispensing. |
| pylabrobot/plate_washing/init.py | Deprecation shim re-exporting legacy plate washing. |
| pylabrobot/plate_reading/init.py | Deprecation shim re-exporting legacy plate reading. |
| pylabrobot/peeling/init.py | Deprecation shim re-exporting legacy peeling. |
| pylabrobot/only_fans/init.py | Deprecation shim re-exporting legacy “only_fans”. |
| pylabrobot/microscopy/init.py | Deprecation shim re-exporting legacy microscopes. |
| pylabrobot/machines/init.py | Deprecation shim re-exporting legacy machines. |
| pylabrobot/liquid_handling/standard.py | Deprecation shim for legacy liquid handling standard ops. |
| pylabrobot/liquid_handling/liquid_classes/tecan.py | Deprecation shim re-exporting legacy Tecan liquid classes. |
| pylabrobot/liquid_handling/liquid_classes/hamilton/star.py | Deprecation shim re-exporting legacy STAR liquid classes. |
| pylabrobot/liquid_handling/liquid_classes/hamilton/init.py | Deprecation shim re-exporting legacy Hamilton liquid classes. |
| pylabrobot/liquid_handling/liquid_classes/init.py | Deprecation shim re-exporting legacy liquid classes package. |
| pylabrobot/liquid_handling/backends/tecan/init.py | Deprecation shim re-exporting legacy Tecan backends. |
| pylabrobot/liquid_handling/backends/opentrons_backend.py | Deprecation shim re-exporting legacy Opentrons backend. |
| pylabrobot/liquid_handling/backends/hamilton/STAR_chatterbox.py | Deprecation shim re-exporting legacy STAR chatterbox backend. |
| pylabrobot/liquid_handling/backends/hamilton/STAR_backend.py | Deprecation shim re-exporting legacy STAR backend. |
| pylabrobot/liquid_handling/backends/hamilton/init.py | Deprecation shim re-exporting legacy Hamilton backends. |
| pylabrobot/liquid_handling/backends/chatterbox.py | Deprecation shim re-exporting legacy chatterbox backend. |
| pylabrobot/liquid_handling/backends/init.py | Deprecation shim re-exporting legacy backends package. |
| pylabrobot/liquid_handling/init.py | Deprecation shim re-exporting legacy liquid_handling package. |
| pylabrobot/legacy/machines/machine.py | Makes legacy Machine.stop() idempotent if setup never completed. |
| pylabrobot/legacy/liquid_handling/liquid_classes/hamilton/vantage.py | Updates legacy liquid class import to new canonical location. |
| pylabrobot/legacy/liquid_handling/liquid_classes/hamilton/star.py | Updates legacy liquid class import to new canonical location. |
| pylabrobot/legacy/liquid_handling/liquid_classes/hamilton/init.py | Legacy shim updated to new canonical liquid class locations. |
| pylabrobot/legacy/liquid_handling/liquid_classes/init.py | Legacy shim updated to new canonical liquid class locations. |
| pylabrobot/legacy/liquid_handling/errors.py | Re-exports shared liquid-handling error classes from capabilities. |
| pylabrobot/legacy/liquid_handling/backends/hamilton/STAR_backend.py | Delegates certain Head96/iSWAP operations to new backend APIs + doc tweaks. |
| pylabrobot/io/usb.py | Makes USB stop() idempotent (no error when already disconnected). |
| pylabrobot/heating_shaking/init.py | Deprecation shim re-exporting legacy heating_shaking. |
| pylabrobot/hamilton/liquid_handlers/vantage/x_arm.py | New Vantage X-arm helper (firmware command wrapper). |
| pylabrobot/hamilton/liquid_handlers/vantage/vantage.py | New Vantage(Device) wiring PIP/Head96/IPG capabilities. |
| pylabrobot/hamilton/liquid_handlers/vantage/tests/test_fw_parsing.py | Unit tests for Vantage firmware string parsing. |
| pylabrobot/hamilton/liquid_handlers/vantage/tests/test_errors.py | Unit tests for Vantage firmware error parsing + conversion. |
| pylabrobot/hamilton/liquid_handlers/vantage/tests/init.py | Test package marker for Vantage tests. |
| pylabrobot/hamilton/liquid_handlers/vantage/loading_cover.py | New loading cover helper (firmware command wrapper). |
| pylabrobot/hamilton/liquid_handlers/vantage/ipg.py | New IPG arm backend implementing OrientableGripperArmBackend. |
| pylabrobot/hamilton/liquid_handlers/vantage/head96_backend.py | New Vantage Core96/Head96 backend mapping high-level ops to firmware. |
| pylabrobot/hamilton/liquid_handlers/vantage/fw_parsing.py | New Vantage firmware response parser. |
| pylabrobot/hamilton/liquid_handlers/vantage/errors.py | New Vantage firmware error dictionaries, parsing, and PLR conversion. |
| pylabrobot/hamilton/liquid_handlers/vantage/driver.py | New Vantage driver built atop HamiltonLiquidHandler with setup/stop flow. |
| pylabrobot/hamilton/liquid_handlers/vantage/chatterbox.py | New Vantage chatterbox driver for no-hardware development. |
| pylabrobot/hamilton/liquid_handlers/vantage/init.py | Exposes Vantage driver/device entry points. |
| pylabrobot/hamilton/liquid_handlers/star/star.py | Updates STAR device imports to new arms capability modules. |
| pylabrobot/hamilton/liquid_handlers/star/pip_backend.py | Updates STAR liquid class imports to new canonical module. |
| pylabrobot/hamilton/liquid_handlers/star/liquid_classes/star_classes.py | Updates STAR liquid class imports to new canonical HamiltonLiquidClass. |
| pylabrobot/hamilton/liquid_handlers/star/liquid_classes/init.py | New public entry point for STAR liquid classes. |
| pylabrobot/hamilton/liquid_handlers/star/iswap.py | Updates imports to new arms capability modules. |
| pylabrobot/hamilton/liquid_handlers/star/head96_backend.py | Adds lifecycle + utilities for STAR Head96 backend and refactors position logic. |
| pylabrobot/hamilton/liquid_handlers/star/fw_parsing.py | Adds firmware-version-to-date parsing helper. |
| pylabrobot/hamilton/liquid_handlers/star/driver.py | Clarifies subsystem lifecycle ownership (notably head96). |
| pylabrobot/hamilton/liquid_handlers/star/core.py | Updates imports to new arms capability modules. |
| pylabrobot/hamilton/liquid_handlers/liquid_class.py | Introduces canonical HamiltonLiquidClass implementation for new architecture. |
| pylabrobot/hamilton/liquid_classes/hamilton/init.py | Removes old liquid class re-export surface. |
| pylabrobot/hamilton/liquid_classes/init.py | Removes old liquid class re-export surface. |
| pylabrobot/hamilton/lh/vantage/liquid_classes.py | Updates Vantage liquid class import to new canonical HamiltonLiquidClass. |
| pylabrobot/hamilton/lh/vantage/init.py | Package marker for Vantage LH module. |
| pylabrobot/hamilton/lh/init.py | Package marker for pylabrobot.hamilton.lh. |
| pylabrobot/device.py | Makes Device.stop() idempotent if setup never completed. |
| pylabrobot/centrifuge/init.py | Deprecation shim re-exporting legacy centrifuge. |
| pylabrobot/capabilities/weighing/weighing.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/tilting/tilting.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/temperature_controlling/temperature_controller.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/shaking/shaking.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/sealing/sealing.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/peeling/peeling.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/humidity_controlling/humidity_controller.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/fan_control/fan_control.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/centrifuging/centrifuging.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/barcode_scanning/barcode_scanning.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/automated_retrieval/automated_retrieval.py | Adds capability-ready guard decorators to public methods. |
| pylabrobot/capabilities/arms/standard.py | Adds canonical arms “standard types” (directions, locations, move/drop structs). |
| pylabrobot/capabilities/arms/orientable_arm.py | New orientable arm capability frontend. |
| pylabrobot/capabilities/arms/backend.py | New arms backend interfaces/mixins for capability architecture. |
| pylabrobot/capabilities/arms/articulated_arm.py | New articulated arm capability frontend. |
| pylabrobot/capabilities/arms/arm.py | New canonical arm capability frontend (base + gripper arm). |
| pylabrobot/capabilities/arms/arm_tests.py | New unit tests validating arm coordinate computations. |
| pylabrobot/capabilities/arms/architecture.md | New internal architecture documentation for arms capability stack. |
| pylabrobot/capabilities/arms/init.py | Public export surface for new arms capability module. |
| pylabrobot/brooks/precise_flex.py | Updates imports to new arms capability modules. |
| pylabrobot/barcode_scanners/init.py | Deprecation shim re-exporting legacy barcode scanners. |
| pylabrobot/arms/standard.py | Deprecation shim re-exporting new arms standard types. |
| pylabrobot/arms/orientable_arm.py | Deprecation shim re-exporting new orientable arm. |
| pylabrobot/arms/backend.py | Deprecation shim re-exporting new arms backend interfaces. |
| pylabrobot/arms/articulated_arm.py | Deprecation shim re-exporting new articulated arm. |
| pylabrobot/arms/arm.py | Deprecation shim re-exporting new arm frontend implementation. |
| pylabrobot/arms/init.py | Deprecation shim re-exporting new pylabrobot.capabilities.arms. |
| docs/user_guide/machine-agnostic-features/writing-robot-agnostic-protocols.md | Updates strictness docs to legacy module paths. |
| docs/user_guide/hamilton/star/iswap.ipynb | Updates iSWAP user guide references to new arms module paths. |
| docs/user_guide/hamilton/star/core-grippers.ipynb | Updates core grippers user guide references to new arms module paths. |
| docs/user_guide/capabilities/dispensing/index.md | Fixes relative links in dispensing docs. |
| docs/user_guide/capabilities/arms.md | Updates arms docs to new capability module paths. |
| docs/user_guide/brooks/precise_flex/hello-world.ipynb | Updates PreciseFlex docs to new arms module paths. |
| docs/user_guide/01_material-handling/temperature-controllers/temperature-controllers.rst | Updates temperature controller docs to legacy module paths. |
| docs/resources/introduction.md | Updates docs to legacy machine paths for examples. |
| docs/resources/index.md | Fixes/updates resource documentation links. |
| docs/resources/container/container.rst | Updates LiquidHandler reference to legacy module path. |
| docs/contributor_guide/visualizer.md | Updates ChatterboxBackend reference to legacy module path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+169
to
+181
| core96_initialized = await self.core96_request_initialization_status() | ||
| if not core96_initialized and not skip_core96: | ||
| self.head96 = VantageHead96Backend(self) | ||
| await self.core96_initialize( | ||
| x_position=7347, | ||
| y_position=2684, | ||
| minimal_traverse_height_at_begin_of_command=int(self._traversal_height * 10), | ||
| minimal_height_at_command_end=int(self._traversal_height * 10), | ||
| end_z_deposit_position=2420, | ||
| ) | ||
| else: | ||
| # Even if already initialized, create the backend. | ||
| self.head96 = VantageHead96Backend(self) if not skip_core96 else None |
There was a problem hiding this comment.
setup(skip_core96=True) still calls core96_request_initialization_status() unconditionally. If the 96-head module is not installed (a common reason to pass skip_core96=True), this status query may itself raise a firmware error, defeating the purpose of the skip flag. Consider guarding the status query behind if not skip_core96: (and leaving self.head96 = None), or catching the specific "module not available" firmware error and treating it as not installed when skip_core96=True.
Suggested change
| core96_initialized = await self.core96_request_initialization_status() | |
| if not core96_initialized and not skip_core96: | |
| self.head96 = VantageHead96Backend(self) | |
| await self.core96_initialize( | |
| x_position=7347, | |
| y_position=2684, | |
| minimal_traverse_height_at_begin_of_command=int(self._traversal_height * 10), | |
| minimal_height_at_command_end=int(self._traversal_height * 10), | |
| end_z_deposit_position=2420, | |
| ) | |
| else: | |
| # Even if already initialized, create the backend. | |
| self.head96 = VantageHead96Backend(self) if not skip_core96 else None | |
| if skip_core96: | |
| self.head96 = None | |
| else: | |
| core96_initialized = await self.core96_request_initialization_status() | |
| if not core96_initialized: | |
| self.head96 = VantageHead96Backend(self) | |
| await self.core96_initialize( | |
| x_position=7347, | |
| y_position=2684, | |
| minimal_traverse_height_at_begin_of_command=int(self._traversal_height * 10), | |
| minimal_height_at_command_end=int(self._traversal_height * 10), | |
| end_z_deposit_position=2420, | |
| ) | |
| else: | |
| # Even if already initialized, create the backend. | |
| self.head96 = VantageHead96Backend(self) |
Comment on lines
+149
to
+154
| pip_channels_initialized = await self.pip_request_initialization_status() | ||
| if not pip_channels_initialized or any(tip_presences): | ||
| await self.pip_initialize( | ||
| x_position=[7095] * self.num_channels, | ||
| y_position=[3891, 3623, 3355, 3087, 2819, 2551, 2283, 2016], | ||
| begin_z_deposit_position=[int(self._traversal_height * 10)] * self.num_channels, |
There was a problem hiding this comment.
pip_initialize() is passed y_position as a hard-coded 8-element list while other arguments are sized to self.num_channels. If query_tip_presence() ever returns a channel count != 8, this will produce inconsistent parameter lengths and likely a malformed firmware command. Either assert/validate that self.num_channels == 8 for Vantage PIP, or generate y_position dynamically based on the detected channel count.
Comment on lines
+163
to
+167
| async def open_gripper( | ||
| self, gripper_width: float, backend_params: Optional[BackendParams] = None | ||
| ) -> None: | ||
| await self.driver.send_command(module="A1RM", command="DO") | ||
|
|
There was a problem hiding this comment.
open_gripper() ignores the gripper_width argument and always sends a fixed A1RM:DO command. If IPG only supports fully-open, this should be documented and the parameter should be validated/removed at the interface layer (or clamped) to avoid giving callers a false sense of control.
| pass | ||
|
|
||
| async def is_gripper_closed(self, backend_params: Optional[BackendParams] = None) -> bool: | ||
| return not self._parked |
There was a problem hiding this comment.
is_gripper_closed() returns not self._parked, but _parked is tracking parking state (set in park() / get_parking_status()), not actual gripper closure or whether a plate is held. This makes the method report incorrect values (e.g., open_gripper() doesn't update _parked, so the result won't change). Consider implementing a firmware query for hold/closed state (similar to STAR iSWAP C0:QP), or raising NotImplementedError until a reliable signal is available.
Suggested change
| return not self._parked | |
| raise NotImplementedError( | |
| "is_gripper_closed is not implemented for the Vantage IPG because the current " | |
| "backend does not have a reliable firmware query for gripper closed/held state. " | |
| "Parking status cannot be used as a proxy for gripper closure." | |
| ) |
Comment on lines
+21
to
+22
| >>> parse_vantage_fw_string("id0xs30 -100 +1 1000", {"id": "int", "x": "[int]"}) | ||
| {"id": 0, "x": [30, -100, 1, 1000]} |
There was a problem hiding this comment.
The docstring example uses mismatched keys ({"id": "int", "x": "[int]"}) but the example string contains xs.... As written, the example would not work and may confuse users. Update the example to use consistent parameter names (e.g. {"xs": "[int]"}) and expected output keys.
Suggested change
| >>> parse_vantage_fw_string("id0xs30 -100 +1 1000", {"id": "int", "x": "[int]"}) | |
| {"id": 0, "x": [30, -100, 1, 1000]} | |
| >>> parse_vantage_fw_string("id0xs30 -100 +1 1000", {"id": "int", "xs": "[int]"}) | |
| {"id": 0, "xs": [30, -100, 1, 1000]} |
Follows the same pattern as STARBackend: the legacy class creates a VantageDriver in __init__, delegates send_command/setup/stop to it, and exposes typed property accessors for the new subsystems. All 21 legacy tests pass unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace ~275 lines of inline definitions (error dicts, VantageFirmwareError, parse_vantage_fw_string, vantage_response_string_to_error) with re-exports from the new vantage modules. Existing imports continue to work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ership - All new backend methods use standard PLR units (mm, uL, uL/s, s) with firmware conversion (*10, *100) at the send_command boundary - Legacy VantageBackend fully delegated: every method forwards to new backends with unit conversion, zero methods still use send_command directly - Subsystems own their setup: each _on_setup() checks init status and initializes if needed (moved out of driver.setup()) - Added VantageCoreGripper backend (A1PM:DG/DR/DH/DO/DJ) - IPG rewritten: public firmware methods (grip_plate, put_plate, etc.), press_on_distance properly threaded through, halt/is_gripper_closed raise NotImplementedError - X-arm methods renamed to match STAR (move_to, move_to_safe) - disco_mode and russian_roulette moved to VantageDriver - Test files renamed to *_tests.py to match pytest.ini - FIXME comments on pre-existing bugs (er0 substring, channel 10-16 parsing, hardcoded 8 y-positions) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+575
to
+576
| minimal_traverse_height_at_begin_of_command=list(mth or [th]) * len(ops), | ||
| minimal_height_at_command_end=list(mhe or [th]) * len(ops), |
There was a problem hiding this comment.
In pick_up_tips, minimal_traverse_height_at_begin_of_command / minimal_height_at_command_end are built with list(mth or [th]) * len(ops). If the caller provides a list (e.g. one value per involved channel), multiplying it by len(ops) will duplicate entries and can make _assemble_command() raise Too many values for tip pattern .... Build these lists as mth if mth is not None else [th]*len(ops) (same for mhe).
Suggested change
| minimal_traverse_height_at_begin_of_command=list(mth or [th]) * len(ops), | |
| minimal_height_at_command_end=list(mhe or [th]) * len(ops), | |
| minimal_traverse_height_at_begin_of_command=mth if mth is not None else [th] * len(ops), | |
| minimal_height_at_command_end=mhe if mhe is not None else [th] * len(ops), |
Comment on lines
+619
to
+623
| begin_z_deposit_position=[max_z + 10] * len(ops), | ||
| end_z_deposit_position=[max_z] * len(ops), | ||
| minimal_traverse_height_at_begin_of_command=list(mth or [th]) * len(ops), | ||
| minimal_height_at_command_end=list(mhe or [th]) * len(ops), | ||
| tip_handling_method=[0] * len(ops), |
There was a problem hiding this comment.
Same list-multiplication issue as in pick_up_tips: list(mth or [th]) * len(ops) / list(mhe or [th]) * len(ops) will over-expand user-provided per-channel lists and can break command assembly. Use the provided list as-is (after validating length) or default to [th] * len(ops).
Comment on lines
+711
to
+712
| minimal_traverse_height_at_begin_of_command=list(mth or [th]) * len(ops), | ||
| minimal_height_at_command_end=list(mhe or [th]) * len(ops), |
There was a problem hiding this comment.
In aspirate, minimal_traverse_height_at_begin_of_command / minimal_height_at_command_end are built via list(mth or [th]) * len(ops), which duplicates entries when the user passes a list. This can cause _assemble_command() to raise due to a value count mismatch with tip_pattern. Prefer mth if mth is not None else [th]*len(ops) (and same for mhe).
Suggested change
| minimal_traverse_height_at_begin_of_command=list(mth or [th]) * len(ops), | |
| minimal_height_at_command_end=list(mhe or [th]) * len(ops), | |
| minimal_traverse_height_at_begin_of_command=mth if mth is not None else [th] * len(ops), | |
| minimal_height_at_command_end=mhe if mhe is not None else [th] * len(ops), |
Comment on lines
+851
to
+855
| ), | ||
| tube_2nd_section_ratio=list(backend_params.tube_2nd_section_ratio or [0] * len(ops)), | ||
| minimal_traverse_height_at_begin_of_command=list(mth or [th]) * len(ops), | ||
| minimal_height_at_command_end=list(mhe or [th]) * len(ops), | ||
| dispense_volume=volumes, |
There was a problem hiding this comment.
In dispense, minimal_traverse_height_at_begin_of_command / minimal_height_at_command_end are constructed with list(mth or [th]) * len(ops), which will over-expand any user-supplied list and can break one-hot list expansion in _assemble_command(). Use the provided list directly (with validation) or default to [th] * len(ops).
Comment on lines
+242
to
+249
| # FIXME: hardcoded for 8 channels. Will break on 4/12/16-channel Vantages. | ||
| # Pre-existing limitation from legacy. | ||
| default_y_positions = [389.1, 362.3, 335.5, 308.7, 281.9, 255.1, 228.3, 201.6] | ||
| n = self.driver.num_channels | ||
| th = self.driver.traversal_height | ||
| await self.driver.pip_initialize( | ||
| x_position=[709.5] * n, | ||
| y_position=default_y_positions, |
There was a problem hiding this comment.
_on_setup() hardcodes default_y_positions for 8 channels but then uses n = self.driver.num_channels for all the other arrays. On 4/12/16-channel instruments this will generate a DI command with a mismatched yp length (and the firmware fill behavior will be incorrect). Either derive Y positions for the detected channel count, or slice/extend the defaults deterministically based on n.
Suggested change
| # FIXME: hardcoded for 8 channels. Will break on 4/12/16-channel Vantages. | |
| # Pre-existing limitation from legacy. | |
| default_y_positions = [389.1, 362.3, 335.5, 308.7, 281.9, 255.1, 228.3, 201.6] | |
| n = self.driver.num_channels | |
| th = self.driver.traversal_height | |
| await self.driver.pip_initialize( | |
| x_position=[709.5] * n, | |
| y_position=default_y_positions, | |
| default_y_positions = [389.1, 362.3, 335.5, 308.7, 281.9, 255.1, 228.3, 201.6] | |
| n = self.driver.num_channels | |
| if n <= len(default_y_positions): | |
| y_positions = default_y_positions[:n] | |
| else: | |
| channel_pitch = default_y_positions[-2] - default_y_positions[-1] | |
| y_positions = default_y_positions + [ | |
| round(default_y_positions[-1] - channel_pitch * i, 1) for i in range(1, n - 7) | |
| ] | |
| th = self.driver.traversal_height | |
| await self.driver.pip_initialize( | |
| x_position=[709.5] * n, | |
| y_position=y_positions, |
Comment on lines
+256
to
+274
| # FIXME: regex [A-Z0-9]{2}[0-9]{2} only captures 2-char module IDs, so channels | ||
| # 10-16 (P10, P11, ...) can never match. Pre-existing bug from legacy. | ||
| error_format = r"[A-Z0-9]{2}[0-9]{2}" | ||
| error_string = parse_vantage_fw_string(string, {"es": "str"})["es"] | ||
| error_codes = re.findall(error_format, error_string) | ||
| errors: Dict[str, str] = {} | ||
| num_channels = 16 | ||
| for error in error_codes: | ||
| module, error_code = error[:2], error[2:] | ||
| error_code_int = int(error_code) | ||
| for channel in range(1, num_channels + 1): | ||
| if module == f"P{channel}": | ||
| errors[f"Pipetting channel {channel}"] = pip_errors.get(error_code_int, "Unknown error") | ||
| elif module in ("H0", "HM"): | ||
| errors["Core 96"] = core96_errors.get(error_code_int, "Unknown error") | ||
| elif module == "RM": | ||
| errors["IPG"] = ipg_errors.get(error_code_int, "Unknown error") | ||
| elif module == "AM": | ||
| errors["Cover"] = "Unknown error" |
There was a problem hiding this comment.
vantage_response_string_to_error() currently cannot parse pipetting channel errors for channels 10-16 because error_format only matches two-character module IDs and the code truncates the module to error[:2]. This makes errors like P1070 ambiguous with P170 and they will be misreported or dropped. Consider parsing the error token as something like P<channel><code> (variable-length channel number) and extracting channel/code via a regex with capture groups, then mapping without the inner for channel in range(...) loop.
Suggested change
| # FIXME: regex [A-Z0-9]{2}[0-9]{2} only captures 2-char module IDs, so channels | |
| # 10-16 (P10, P11, ...) can never match. Pre-existing bug from legacy. | |
| error_format = r"[A-Z0-9]{2}[0-9]{2}" | |
| error_string = parse_vantage_fw_string(string, {"es": "str"})["es"] | |
| error_codes = re.findall(error_format, error_string) | |
| errors: Dict[str, str] = {} | |
| num_channels = 16 | |
| for error in error_codes: | |
| module, error_code = error[:2], error[2:] | |
| error_code_int = int(error_code) | |
| for channel in range(1, num_channels + 1): | |
| if module == f"P{channel}": | |
| errors[f"Pipetting channel {channel}"] = pip_errors.get(error_code_int, "Unknown error") | |
| elif module in ("H0", "HM"): | |
| errors["Core 96"] = core96_errors.get(error_code_int, "Unknown error") | |
| elif module == "RM": | |
| errors["IPG"] = ipg_errors.get(error_code_int, "Unknown error") | |
| elif module == "AM": | |
| errors["Cover"] = "Unknown error" | |
| error_string = parse_vantage_fw_string(string, {"es": "str"})["es"] | |
| errors: Dict[str, str] = {} | |
| # Parse either: | |
| # - pipetting channel errors: P<channel><code>, where channel is 1-16 and code is 2 digits | |
| # - other module errors already handled by this function: <module><code> | |
| error_pattern = re.compile( | |
| r"P(?P<pip_channel>1[0-6]|[1-9])(?P<pip_code>\d{2})|" | |
| r"(?P<module>H0|HM|RM|AM)(?P<module_code>\d{2})" | |
| ) | |
| for match in error_pattern.finditer(error_string): | |
| pip_channel = match.group("pip_channel") | |
| if pip_channel is not None: | |
| channel = int(pip_channel) | |
| error_code_int = int(match.group("pip_code")) | |
| errors[f"Pipetting channel {channel}"] = pip_errors.get( | |
| error_code_int, "Unknown error" | |
| ) | |
| continue | |
| module = match.group("module") | |
| error_code_int = int(match.group("module_code")) | |
| if module in ("H0", "HM"): | |
| errors["Core 96"] = core96_errors.get(error_code_int, "Unknown error") | |
| elif module == "RM": | |
| errors["IPG"] = ipg_errors.get(error_code_int, "Unknown error") | |
| elif module == "AM": | |
| errors["Cover"] = "Unknown error" |
Comment on lines
+256
to
+274
| # FIXME: regex [A-Z0-9]{2}[0-9]{2} only captures 2-char module IDs, so channels | ||
| # 10-16 (P10, P11, ...) can never match. Pre-existing bug from legacy. | ||
| error_format = r"[A-Z0-9]{2}[0-9]{2}" | ||
| error_string = parse_vantage_fw_string(string, {"es": "str"})["es"] | ||
| error_codes = re.findall(error_format, error_string) | ||
| errors: Dict[str, str] = {} | ||
| num_channels = 16 | ||
| for error in error_codes: | ||
| module, error_code = error[:2], error[2:] | ||
| error_code_int = int(error_code) | ||
| for channel in range(1, num_channels + 1): | ||
| if module == f"P{channel}": | ||
| errors[f"Pipetting channel {channel}"] = pip_errors.get(error_code_int, "Unknown error") | ||
| elif module in ("H0", "HM"): | ||
| errors["Core 96"] = core96_errors.get(error_code_int, "Unknown error") | ||
| elif module == "RM": | ||
| errors["IPG"] = ipg_errors.get(error_code_int, "Unknown error") | ||
| elif module == "AM": | ||
| errors["Cover"] = "Unknown error" |
There was a problem hiding this comment.
The module dispatch inside vantage_response_string_to_error() is nested inside for channel in range(1, num_channels + 1), but only the P{channel} case actually depends on channel. For Core96/IPG/Cover branches this loop repeats the same assignment 16 times. After fixing the channel parsing, move the non-PIP module handling outside the channel loop (or remove the loop entirely) to avoid redundant work and reduce the risk of subtle overwrites.
Suggested change
| # FIXME: regex [A-Z0-9]{2}[0-9]{2} only captures 2-char module IDs, so channels | |
| # 10-16 (P10, P11, ...) can never match. Pre-existing bug from legacy. | |
| error_format = r"[A-Z0-9]{2}[0-9]{2}" | |
| error_string = parse_vantage_fw_string(string, {"es": "str"})["es"] | |
| error_codes = re.findall(error_format, error_string) | |
| errors: Dict[str, str] = {} | |
| num_channels = 16 | |
| for error in error_codes: | |
| module, error_code = error[:2], error[2:] | |
| error_code_int = int(error_code) | |
| for channel in range(1, num_channels + 1): | |
| if module == f"P{channel}": | |
| errors[f"Pipetting channel {channel}"] = pip_errors.get(error_code_int, "Unknown error") | |
| elif module in ("H0", "HM"): | |
| errors["Core 96"] = core96_errors.get(error_code_int, "Unknown error") | |
| elif module == "RM": | |
| errors["IPG"] = ipg_errors.get(error_code_int, "Unknown error") | |
| elif module == "AM": | |
| errors["Cover"] = "Unknown error" | |
| error_format = r"(P(?:[1-9]|1[0-6])|H0|HM|RM|AM)(\d{2})" | |
| error_string = parse_vantage_fw_string(string, {"es": "str"})["es"] | |
| error_codes = re.findall(error_format, error_string) | |
| errors: Dict[str, str] = {} | |
| for module, error_code in error_codes: | |
| error_code_int = int(error_code) | |
| if module.startswith("P"): | |
| channel = int(module[1:]) | |
| errors[f"Pipetting channel {channel}"] = pip_errors.get(error_code_int, "Unknown error") | |
| elif module in ("H0", "HM"): | |
| errors["Core 96"] = core96_errors.get(error_code_int, "Unknown error") | |
| elif module == "RM": | |
| errors["IPG"] = ipg_errors.get(error_code_int, "Unknown error") | |
| elif module == "AM": | |
| errors["Cover"] = "Unknown error" |
Comment on lines
+126
to
+128
| # FIXME: "er0" substring check also suppresses er01-er09 (error codes 1-9). | ||
| # Pre-existing bug from legacy. Needs proper regex-based error detection. | ||
| if "er" in resp and "er0" not in resp: |
There was a problem hiding this comment.
check_fw_string_error() uses a substring check ("er0" not in resp) which will incorrectly treat error codes er01..er09 as success because they contain the substring er0. This will silently drop real firmware errors. Use a regex or parse the er field as an integer and compare to 0 (e.g. match er(\d+)).
Suggested change
| # FIXME: "er0" substring check also suppresses er01-er09 (error codes 1-9). | |
| # Pre-existing bug from legacy. Needs proper regex-based error detection. | |
| if "er" in resp and "er0" not in resp: | |
| parsed = parse_vantage_fw_string(resp, {"er": "int"}) | |
| if "er" in parsed and parsed["er"] is not None and int(parsed["er"]) != 0: |
Comment on lines
+30
to
+49
| if fmt is None: | ||
| fmt = {} | ||
|
|
||
| if not isinstance(fmt, dict): | ||
| raise TypeError(f"invalid fmt for fmt: expected dict, got {type(fmt)}") | ||
|
|
||
| if "id" not in fmt: | ||
| fmt["id"] = "int" | ||
|
|
||
| for key, data_type in fmt.items(): | ||
| if data_type == "int": | ||
| matches = re.findall(rf"{key}([-+]?\d+)", s) | ||
| if len(matches) != 1: | ||
| raise ValueError(f"Expected exactly one match for {key} in {s}") | ||
| parsed[key] = int(matches[0]) | ||
| elif data_type == "str": | ||
| matches = re.findall(rf"{key}\"(.*)\"", s) | ||
| if len(matches) != 1: | ||
| raise ValueError(f"Expected exactly one match for {key} in {s}") | ||
| parsed[key] = matches[0] |
There was a problem hiding this comment.
parse_vantage_fw_string() uses a greedy regex for string fields ({key}"(.*)"). If the firmware response contains multiple quoted fields, this can over-capture across fields and mis-parse the value. Prefer a non-greedy pattern (e.g. .*?) or a negated class (e.g. [^\"]*). Also, the function mutates the caller-provided fmt dict by inserting id; consider copying fmt first to avoid surprising side-effects for reused format dicts.
Comment on lines
+430
to
+452
| async def disco_mode(self): | ||
| """Easter egg.""" | ||
| for _ in range(69): | ||
| r, g, b = random.randint(30, 100), random.randint(30, 100), random.randint(30, 100) | ||
| await self.set_led_color("on", intensity=100, white=0, red=r, green=g, blue=b, uv=0) | ||
| await asyncio.sleep(0.1) | ||
|
|
||
| async def russian_roulette(self): | ||
| """Dangerous easter egg.""" | ||
| sure = input( | ||
| "Are you sure you want to play Russian Roulette? This will turn on the uv-light " | ||
| "with a probability of 1/6. (yes/no) " | ||
| ) | ||
| if sure.lower() != "yes": | ||
| print("boring") | ||
| return | ||
|
|
||
| if random.randint(1, 6) == 6: | ||
| await self.set_led_color("on", intensity=100, white=100, red=100, green=0, blue=0, uv=100) | ||
| print("You lost.") | ||
| else: | ||
| await self.set_led_color("on", intensity=100, white=100, red=0, green=100, blue=0, uv=0) | ||
| print("You won.") |
There was a problem hiding this comment.
disco_mode() / russian_roulette() embed interactive input() + print() calls and deliberately enable UV output with randomized behavior. As part of a hardware driver, this is unsafe for production use and can deadlock non-interactive runtimes (e.g. services, notebooks, CI). These should be removed from the driver, or moved behind an explicit debug-only module/CLI entrypoint that is not shipped as part of the device API.
…amples Fixes from Copilot review: - list(mth or [th]) * len(ops) duplicated user-provided lists — use conditional instead - fw_parsing greedy regex (.*) over-captures across quoted fields — use ([^"]*) - fw_parsing mutated caller's fmt dict by inserting "id" — copy first - Docstring examples had mismatched keys and missing id prefix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…atio Found by adversarial review agents cross-referencing against legacy firmware parameter validation ranges: - pre_wetting_volume (oa): firmware uses 0.1uL, was *100, fixed to *10 - mix_volume (mv): firmware uses 0.1uL, was *100, fixed to *10 - stop_back_volume (rv): firmware uses 0.1uL, was *100, fixed to *10 - tube_2nd_section_ratio (zr): dimensionless, was *10, fixed to no multiplier Affected: _pip_aspirate, _pip_dispense, _core96_aspiration_of_liquid, _core96_dispensing_of_liquid, _core96_wash_tips, and their legacy delegations. The DM command (simultaneous_aspiration_dispensation) already had these correct. Also: document that open_gripper ignores gripper_width (IPG only supports full release). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace 28 instances of `x or default` with `x if x is not None else default` where x is Optional[float] and 0.0 is a legitimate value. Most impactful: settling_time=0.0 no longer silently becomes 5s, blow_out_air_volume=0.0 no longer gets liquid class default. Affects pip_backend, head96_backend, core gripper, and driver. - Restore dropped params in discard_core_gripper_tool: tip_type, begin_z_deposit_position, end_z_deposit_position now forwarded. - Restore dropped first_pip_channel_node_no in release_object/open_gripper. - Fix blink_interval=0 silently becoming 750ms. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
VantageCoreGripper.open_gripper had an extra parameter violating the CanGrip interface. Moved first_pip_channel_node_no into OpenGripperParams so the method signature matches the abstract interface exactly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…owns - Add core_grippers() context manager to Vantage device following STAR pattern. Raises NotImplementedError until the tool pickup firmware command is reverse-engineered. - VantageCoreGripper: close_gripper, halt, park, is_gripper_closed, request_gripper_location all raise NotImplementedError with clear messages instead of silently doing nothing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…IP helpers - Lifecycle: pip/head96/ipg removed from VantageDriver._subsystems; the Vantage device's capability frontends drive their _on_setup/_on_stop. Legacy VantageBackend.setup/stop now calls those hooks explicitly so the legacy path still initializes the hardware. Vantage.stop runs unconditionally so a partial setup still releases the USB connection. Vantage.setup gains skip_loading_cover/skip_core96/skip_ipg knobs and forwards them to the driver. - Mix params: restore mix_position_in_z_direction_from_liquid_surface, surface_following_distance_during_mixing, and TODO_DA_5/TODO_DD_2 as BackendParams fields on PIP + Head96 aspirate/dispense, with firmware unit conversion at the send_command boundary; legacy wrapper forwards them. - PIP helpers inlined: tip pickup/drop and aspirate/dispense paths now call driver.send_command directly with explicit parameter-range validation instead of going through private helper wrappers. - IPG safety: default grip_strength lowered 100 -> 81 to avoid crushing thin-skirted labware. press_on_distance (zi) accepted for API compatibility but no longer forwarded to firmware since the parameter is uncharacterised on real hardware. - Polish: NotImplementedError stubs on VantageCoreGripper and IPGBackend gain docstrings explaining why each is unported (implicit in another command, uncharacterised, etc.). Chatterbox comment rewritten to explain why _on_setup is skipped on subsystems. VantageXArm exported. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- docs/user_guide/hamilton/vantage/: new section with an index and a hello-world notebook walking through Vantage setup, deck layout, tip handling, aspiration/dispensing, IPG plate transport, and teardown against the chatterbox driver. - docs/user_guide/hamilton/index.md: link the new Vantage section from the Hamilton index. - docs/api/pylabrobot.hamilton.rst: autosummary + autoclass entries for Vantage, VantagePIPBackend, VantageHead96Backend, IPGBackend and their BackendParams (PickUp/DropTips, Aspirate/Dispense, PickUp/Drop). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
VantageBackend(~5,334 lines) into the same layered Device → Driver → CapabilityBackend architecture established by the STAR portpylabrobot/hamilton/liquid_handlers/vantage/totaling ~2,900 linesNew files
driver.pyVantageDriver(HamiltonLiquidHandler)— USB I/O, firmware protocol, setup/teardownpip_backend.pyVantagePIPBackend(PIPBackend)— independent channel tip/aspirate/dispensehead96_backend.pyVantageHead96Backend(Head96Backend)— 96-head operationsipg.pyIPGBackend(OrientableGripperArmBackend)— integrated plate grippervantage.pyVantage(Device)— user-facing device wiring capabilities to backendschatterbox.pyVantageChatterboxDriver— mock driver for testing without hardwarefw_parsing.pyparse_vantage_fw_string()— dict-based firmware response parsererrors.pyVantageFirmwareError, PLR error conversionx_arm.pyVantageXArm— X-arm positioning (A1XM)loading_cover.pyVantageLoadingCover— loading cover control (I1AM)Test plan
Vantage(deck, chatterbox=True)→ setup → stop lifecycle works_setup_finished)🤖 Generated with Claude Code