Repository navigation
Unify sendCommand on PhysicalDevice (closes #80) - #85
Merged
Merged
Conversation
4 of 5 tasks
dccote
added a commit
that referenced
this pull request
May 22, 2026
Every Command (TextCommand, MultilineTextCommand, DataCommand) carries
the same four conceptual operations:
requestEncoder encode an outgoing request payload (device -> wire)
requestDecoder decode an incoming request payload (wire -> mock)
replyEncoder encode an outgoing reply payload (mock -> wire)
replyDecoder decode an incoming reply payload (wire -> device)
Before this commit, those operations were named ad-hoc per subclass --
TextCommand used text_format/matchPattern/replyPattern/responseTemplate
while DataCommand used sendFormat+sendFields+sendDefaults / requestFormat+
requestFields+prefix / unpackingMask+replyDataLength / responseFormat+
responseFields. Twenty-two attribute slots describing four conceptual
roles, no two sharing a name across subclasses.
This commit gives every Command subclass the same four attribute names
for the same four operations. For TextCommand they hold strings (format
strings for encoders, regexes for decoders). For DataCommand they hold
small frozen dataclasses (DataEncoder bundles struct format + fields +
defaults; DataDecoder bundles struct format + fields + length + prefix).
Deleted attributes:
- alternatePattern: TextCommand + MultilineTextCommand kwarg that was
plumbed through readMatchingGroups but never actually set by any
Command in the codebase. Fold into replyDecoder regex if needed.
- replyHexRegex: DataCommand kwarg whose only code path raised
NotImplementedError. Dead.
Migration of every Command in the codebase to the new constructor:
- SutterDevice (4 commands): each DataCommand now wraps the struct
format + fields + defaults inside DataEncoder/DataDecoder. The MOVE
command goes from 7 keyword args + a prefix=b'M' to one tidy
requestEncoder/requestDecoder/replyDecoder triple.
- IntellidriveDevice (3 text commands), IntegraDevice (5 text/multiline
commands), EchoDevice (3 mixed commands), CoboltDevice (11 text
commands): same pattern, attribute names renamed.
- testCommandRecognition.py (38 tests), testTableDrivenDebugPort.py
(3 fixtures + 12 tests), testCommunicationPorts.py (5 tests): all
rewritten to use the new constructor signatures.
Internal property renames:
- TextCommand.effectiveMatchPattern -> effectiveDecoder
- TextCommand._autoMatchPattern (was a property) -> _autoMatchPattern()
(now a method, since it's called rather than read)
These are private/dunder-ish; the only tests that referenced them were
updated.
Verified:
- 261 passed, 203 skipped, 0 failures on the full test suite. The
skipped tests are all hardware-gated (no spectrometer / no Sutter /
no LabJack attached) and were also skipping before this commit.
Closes the first item of the redesign discussed in PR #85's review:
"TextCommand and DataCommand don't share a shape". The remaining items
(separate Command schema from per-call instance, unify error model,
mock hook contract) are left for follow-up work.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Before this commit, PhysicalDevice.sendCommand(command) took a Command
object, while SutterDevice.sendCommand(name, **params) was a parallel
override that looked the command up in the commands dict and returned
the unpacked reply. Same method name, completely different contracts.
Every new device adopting the dual-role commands pattern would have
needed its own override unless this was cleaned up.
This commit makes PhysicalDevice.sendCommand the single entry point,
matching what SutterDevice already wanted:
cmd = self.sendCommand("MOVE", x=10, y=20, z=30)
if cmd.matchGroups != (b'\r',):
...
Changes:
- commands.py: DataCommand.send now accepts **params and calls
buildSendData(**params) when sendFormat is set (else falls back
to self.data). After the reply is read, self.matchGroups is
populated via unpackReply(self.reply) so callers have the same
.matchGroups attribute they're already using on TextCommand.
- commands.py: TextCommand.send and MultilineTextCommand.send now
accept **kwargs in addition to the existing params= argument
(backward compatible). When kwargs are provided, the text_format
is .format(**kwargs)-substituted, enabling named placeholders
like 'g r{register}\n'.
- physicaldevice.py: sendCommand(command) becomes
sendCommand(name, **params). Looks up the command in
self.commands, calls cmd.send(port=self.port, **params), and
returns the Command so callers can read .reply / .matchGroups /
.exceptions / .isSentSuccessfully.
- serialport.py: SerialPort accepts a delay parameter that injects
time.sleep(delay) before each writeData and readData. This is
where Sutter's MP-285 timing margin (100 ms before each I/O)
now lives -- in the port, not in the device-specific sendCommand
override.
- sutterdevice.py: deletes the SutterDevice.sendCommand,
sendCommandBytes, and readReply overrides. Opens its SerialPort
with delay=0.1 (taking over the timing role those helpers held).
Updates doGetPosition/doMoveTo/doHome/work callers to read
cmd.matchGroups from the returned Command instead of the inline
tuple the old override returned. doInitializeDevice now uses
commands["GET_POSITION"].send(port=self.port) directly to verify
the device is talking, since sendCommand's state==Ready guard
hasn't been satisfied yet at that point.
- tests/testPhysicalDevice.py: testEchoCommands passes the name
(string) instead of the command (object), matching the new API.
- tests/testConnectSutter.py: switches from sutter.doInitializeDevice()
to sutter.initializeDevice() -- the internal-method shortcut
skipped the state transition and now trips sendCommand's guard.
Verified:
- 261 passed, 203 skipped, 0 failures on the full suite (the skips
are hardware-gated tests with no hardware attached).
- testSutterDevice (which falls back to debug when no Sutter is
connected) all 7 pass.
- testCommandRecognition (38 unit tests on Command behavior) all
pass with no changes -- the new send signatures stayed
backward compatible.
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.
Closes #80.
Summary
Before this PR, two methods called `sendCommand` existed in the codebase with completely different contracts:
The Sutter version shadowed the base via Python's MRO. Same method name, fundamentally different APIs. Every new device adopting the dual-role commands pattern that PR #73 introduced would have needed a parallel override to get the `(name, **params)` ergonomics.
This PR collapses them into a single unified `PhysicalDevice.sendCommand(name, **params)` and deletes the `SutterDevice` override along with its helper methods.
What changed
Net: +50 / −67 lines across 6 files.
Test plan
Why move timing into the port?
The Sutter MP-285's serial protocol needs ~100 ms between commands or the device gets out of sync. The old code put `time.sleep(0.1)` inside Sutter-specific helpers (`sendCommandBytes`, `readReply`), then called those from the Sutter-specific `sendCommand`. After unifying `sendCommand`, those helpers had nowhere to live except the port. The port is also the correct architectural home: timing margin is a property of the wire, not the device's command vocabulary.
Migration impact
This is a breaking change to `PhysicalDevice.sendCommand`'s signature. The two callers in this repo (`testEchoCommands` in `testPhysicalDevice.py`, plus `SutterDevice`'s own `do*` methods) are migrated in this PR. Other devices that don't currently use `PhysicalDevice.sendCommand` (`IntegraDevice` calls `cmd.send(port=self.port)` directly; `OISpectrometer` has its own `sendCommand`) are not affected.
Downstream code that relied on `PhysicalDevice.sendCommand(command)` will need to switch to `sendCommand(name)` where `name` is the key in `device.commands`.
🤖 Generated with Claude Code