Echo driver architecture: one frontend, model param, driver ABC#1
Open
c-reiter wants to merge 5 commits into
Open
Echo driver architecture: one frontend, model param, driver ABC#1c-reiter wants to merge 5 commits into
c-reiter wants to merge 5 commits into
Conversation
Comment on lines
+72
to
+73
| name: str | ||
| transfer_volume_increment_nl: float |
There was a problem hiding this comment.
I believe transfer_volume_increment_nl might be a function of model, so we can store it as a computed property
Comment on lines
+1811
to
+1812
| model: str = DEFAULT_ECHO_MODEL, | ||
| client_version: Optional[str] = None, |
There was a problem hiding this comment.
let's make model a string literal
Comment on lines
1828
to
1837
| # ``client_version`` / ``protocol_version`` / ``transfer_volume_increment_nl`` default to the | ||
| # model's values but remain overridable for newer firmware. | ||
| self.client_version = spec.client_version if client_version is None else client_version | ||
| self.protocol_version = spec.protocol_version if protocol_version is None else protocol_version | ||
| self.transfer_volume_increment_nl = ( | ||
| spec.transfer_volume_increment_nl | ||
| if transfer_volume_increment_nl is None | ||
| else transfer_volume_increment_nl | ||
| ) | ||
| self._rpc_lock = asyncio.Lock() |
There was a problem hiding this comment.
why not just store the spec rather than flattening it onto the class? it can then be accessed through self.spec.transfer_volume_increment_nl for example
| return EchoEventStream(self, reader, writer) | ||
| """Open the instrument event stream. Implemented by transport-specific subclasses.""" | ||
| raise NotImplementedError | ||
|
|
There was a problem hiding this comment.
abstract method already raises an error when called, the doctoring is enough
| class Echo525(Echo): | ||
| """Labcyte Echo 525 device frontend. | ||
| """Deprecated. Use ``Echo(host, model="Echo 525")``.""" | ||
|
|
There was a problem hiding this comment.
we don't have to have deprecation warnings for things that aren't merged yet :)
There was a problem hiding this comment.
from PLR's perspective
however if you want this for another reason happy to keep
Owner
Author
|
Thanks for the review @rickwierenga — all addressed in the latest push:
100 labcyte tests still pass. |
added 5 commits
July 2, 2026 15:24
Per maintainer guidance (target v1b1 Device/Driver conventions), collapse the Echo525 frontend/driver subclasses into a single Echo selected by model: - add an ECHO_MODELS registry (Echo 650 = 2.5 nL/3.1, Echo 525 = 25 nL/2.6); EchoDriver(model=...) resolves the increment + version defaults from it. - Echo(host, model="Echo 525") and Echo(driver=<any EchoDriver>) — the driver is now injectable (enabling chatterbox / vendor-SDK siblings), replacing the driver_class subclassing hook. - Echo525 / Echo525Driver become thin deprecation shims (warn -> model="Echo 525"). - migrate 525/picklist tests + docs to the model= API; add deprecation-alias tests. 100 labcyte tests pass. Follow-up will make EchoDriver an ABC with MedmanEchoDriver + EchoChatterboxDriver siblings.
…blings Completes the driver-architecture refactor Rick approved on the PR: - EchoDriver is now an abstract base holding all Medman protocol logic; the transport (_send_request / open_event_stream) is abstract. - MedmanEchoDriver: the concrete SOAP-over-HTTP implementation (real instrument). - EchoChatterboxDriver: hardware-free sibling that logs each RPC and returns SUCCEEDED/OK with no I/O (dry-run command sequences), injectable via Echo(driver=EchoChatterboxDriver()). - Echo builds a MedmanEchoDriver by default; Echo525Driver shim now subclasses it. - update base/525 tests + docs + API reference; 102 labcyte tests pass. This is the sibling-driver-under-one-ABC shape (Medman / chatterbox / future vendor-SDK or iDOT) injected into a single Echo frontend.
Per @rickwierenga's review on the PR: - EchoDriver stores self.spec (an EchoModel); transfer_volume_increment_nl, client_version, protocol_version, model are computed properties off it (no longer flattened onto the instance / no override kwargs). - model is typed as a Literal["Echo 650", "Echo 525"]. - abstract stubs keep only their docstring (no redundant NotImplementedError). - removed the Echo525 / Echo525Driver deprecation shims and echo525.py entirely — nothing is merged yet, so there is nothing to deprecate; the 525 is just Echo(model="Echo 525"). Updated tests, exports, docs and API ref. 100 labcyte tests pass.
- ruff format + import sorting across the touched labcyte files - type Echo(model=) and EchoDriver(model=) as the EchoModelName Literal so mypy is happy passing model through to the driver - mypy fixes in echo_mock.py (typed _load_captured_responses result; type _server as asyncio.Server so .sockets resolves; int() the port) - rename a loop var that shadowed the dataclasses.field import (F402) and drop a duplicate 'fluids' annotation (no-redef) in echo.py - remove now-unused EchoDriver import from echo_tests.py ruff (format/lint/isort), mypy (labcyte), typos and the 100 labcyte tests all pass.
c-reiter
force-pushed
the
labcyte-echo-driver-refactor
branch
from
July 2, 2026 13:25
68da3b2 to
caa72be
Compare
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.
Echo driver architecture: one frontend, model param, driver ABC
Follow-up to alexjamesgodfrey#5 implementing the structure @rickwierenga approved there - aligning the Echo with the v1b1
Device/Driverconventions (one frontend + injected backend ABC + concrete subclasses + a chatterbox sibling).Stacked on
labcyte-echo-525(alexjamesgodfrey#5), which is itself stacked on PyLabRobot#1001, so the diff here is the architecture change only.What changed
1. One
Echofrontend; the model is a parameter, not a subclass.ECHO_MODELSregistry (Echo 650= 2.5 nL / protocol 3.1,Echo 525= 25 nL / protocol 2.6).EchoDriver(model=...)resolves the increment + version defaults from it.Echo("192.168.0.25", model="Echo 525")selects the variant;Echo(driver=...)injects any driver.Echo525/Echo525Driverare now thin deprecation shims (warn →model="Echo 525"), so existing code keeps working.2.
EchoDriveris an abstract base with sibling implementations.EchoDriver(Driver, ABC)holds all the Medman protocol logic; only the transport (_send_request/open_event_stream) is abstract.MedmanEchoDriver- concrete SOAP-over-HTTP driver (real instrument).EchoChatterboxDriver- hardware-free sibling: logs each RPC and returnsSUCCEEDED/OKwith no I/O, for dry-running command sequences.Echo(driver=EchoChatterboxDriver())needs no host.EchoProtocolGenerator) stays a separate strategy onrun_picklist, orthogonal to transport.This is the seam a future vendor-SDK driver - or the iDOT you mentioned - would plug into.
Validation
EchoChatterboxDriverruns without I/O, and that the deprecatedEcho525/Echo525Driveraliases still work but warn.model=API.