From e5ee69fefbcb412b55cfc1a047cfb5e7dbb7eec9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20C=C3=B4t=C3=A9?= Date: Mon, 18 May 2026 20:36:31 -0400 Subject: [PATCH] Unify sendCommand on PhysicalDevice (fixes #80) 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) --- hardwarelibrary/communication/commands.py | 14 +++-- hardwarelibrary/communication/serialport.py | 7 ++- hardwarelibrary/motion/sutterdevice.py | 67 +++++---------------- hardwarelibrary/physicaldevice.py | 14 ++++- hardwarelibrary/tests/testConnectSutter.py | 4 +- hardwarelibrary/tests/testPhysicalDevice.py | 8 +-- 6 files changed, 49 insertions(+), 65 deletions(-) diff --git a/hardwarelibrary/communication/commands.py b/hardwarelibrary/communication/commands.py index 2e542404..9716207e 100644 --- a/hardwarelibrary/communication/commands.py +++ b/hardwarelibrary/communication/commands.py @@ -254,10 +254,12 @@ def formatResponse(self, result): return bytearray(formatted.encode('utf-8')) return super().formatResponse(result) - def send(self, port, params=None) -> bool: + def send(self, port, params=None, **kwargs) -> bool: try: if params is not None: textCommand = self.requestEncoder.format(params) + elif kwargs: + textCommand = self.requestEncoder.format(**kwargs) else: textCommand = self.requestEncoder @@ -312,10 +314,12 @@ def __init__(self, name, def payload(self): return self.requestEncoder - def send(self, port, params=None) -> bool: + def send(self, port, params=None, **kwargs) -> bool: try: if params is not None: textCommand = self.requestEncoder.format(params) + elif kwargs: + textCommand = self.requestEncoder.format(**kwargs) else: textCommand = self.requestEncoder @@ -475,12 +479,14 @@ def unpackReply(self, replyBytes): return replyBytes return struct.unpack(self.replyDecoder.format, replyBytes) - def send(self, port) -> bool: + def send(self, port, **params) -> bool: try: - port.writeData(data=self.data, endPoint=self.endPoints[0]) + data = self.buildSendData(**params) + port.writeData(data=data, endPoint=self.endPoints[0]) self.isSent = True if self.replyDecoder is not None and self.replyDecoder.length > 0: self.reply = port.readData(length=self.replyDecoder.length) + self.matchGroups = self.unpackReply(self.reply) self.isSentSuccessfully = True except Exception as err: self.exceptions.append(err) diff --git a/hardwarelibrary/communication/serialport.py b/hardwarelibrary/communication/serialport.py index dc3a7c70..d916778a 100644 --- a/hardwarelibrary/communication/serialport.py +++ b/hardwarelibrary/communication/serialport.py @@ -26,7 +26,7 @@ class SerialPort(CommunicationPort): or the find_url.py script from the distribution. More info: https://eblot.github.io/pyftdi/api/usbtools.html You have to add any custom VID/PID when using tools (but they are added here in SerialPort) @line 30. """ - def __init__(self, idVendor=None, idProduct=None, serialNumber=None, portPath=None, port=None): + def __init__(self, idVendor=None, idProduct=None, serialNumber=None, portPath=None, port=None, delay=0): CommunicationPort.__init__(self) try: @@ -46,6 +46,7 @@ def __init__(self, idVendor=None, idProduct=None, serialNumber=None, portPath=No port.close() self.port = None # direct port, must be closed. + self.delay = delay # seconds to wait before each read/write (Sutter MP-285 needs ~0.1) @classmethod def matchSinglePort(cls, idVendor=None, idProduct=None, serialNumber=None): @@ -195,6 +196,8 @@ def readString(self, endPoint=0): def readData(self, length, endPoint=0) -> bytearray: with self.portLock: + if self.delay > 0: + time.sleep(self.delay) data = self.port.read(length) if len(data) != length: raise CommunicationReadTimeout("Only obtained {0}".format(data)) @@ -203,6 +206,8 @@ def readData(self, length, endPoint=0) -> bytearray: def writeData(self, data, endPoint=0) -> int: with self.portLock: + if self.delay > 0: + time.sleep(self.delay) nBytesWritten = self.port.write(data) if nBytesWritten != len(data): raise IOError("Not all bytes written to port") diff --git a/hardwarelibrary/motion/sutterdevice.py b/hardwarelibrary/motion/sutterdevice.py index a2eb3da1..d4e38052 100644 --- a/hardwarelibrary/motion/sutterdevice.py +++ b/hardwarelibrary/motion/sutterdevice.py @@ -68,13 +68,15 @@ def doInitializeDevice(self): if portPath is None: raise PhysicalDevice.UnableToInitialize("No Sutter Device connected") - self.port = SerialPort(portPath=portPath) + self.port = SerialPort(portPath=portPath, delay=0.1) self.port.open(baudRate=128000, timeout=10) if self.port is None: raise PhysicalDevice.UnableToInitialize("Cannot allocate port for serial '{0}'".format(self.serialNumber)) - self.positionInMicrosteps() + # Verify the device is talking. Direct .send() bypasses sendCommand's + # state check, which would reject because state is not yet Ready here. + self.commands["GET_POSITION"].send(port=self.port) except Exception as error: if self.port is not None: @@ -86,60 +88,21 @@ def doShutdownDevice(self): self.port.close() self.port = None - def sendCommandBytes(self, commandBytes): - """ The function to write a command to the endpoint. It will initialize the device - if it is not alread initialized. On failure, it will warn and shutdown.""" - if self.port is None: - self.initializeDevice() - - time.sleep(0.1) - nBytesWritten = self.port.writeData(commandBytes) - if nBytesWritten != len(commandBytes): - raise Exception(f"Unable to send command {commandBytes} to device.") - - def readReply(self, size, format) -> tuple: - """ The function to read a reply from the endpoint. It will initialize the device - if it is not already initialized. On failure, it will warn and shutdown. - It will unpack the reply into a tuple. - """ - if format is None: - raise Exception("You must provide a format for unpacking in readReply") - - if self.port is None: - self.initializeDevice() - - time.sleep(0.1) - replyBytes = self.port.readData(size) - if len(replyBytes) != size: - raise Exception(f"Not enough bytes read in readReply {replyBytes}") - - # print(replyBytes, format) - # print(unpack(format, replyBytes)) - return unpack(format, replyBytes) - - def sendCommand(self, name, **params): - """Send a command from the commands dict and return the unpacked reply.""" - cmd = self.commands[name] - data = cmd.buildSendData(**params) - self.sendCommandBytes(data) - if cmd.replyDecoder is not None and cmd.replyDecoder.length > 0: - return self.readReply(cmd.replyDecoder.length, cmd.replyDecoder.format) - return None - def positionInMicrosteps(self) -> (int, int, int): # for compatibility return self.doGetPosition() def doGetPosition(self) -> (int, int, int): """ Returns the position in microsteps """ - (x, y, z) = self.sendCommand("GET_POSITION") + cmd = self.sendCommand("GET_POSITION") + (x, y, z) = cmd.matchGroups return (x, y, z) def doMoveTo(self, position): """ Move to a position in microsteps """ x, y, z = position - reply = self.sendCommand("MOVE", x=int(x), y=int(y), z=int(z)) - if reply != (b'\r',): - raise Exception(f"Expected carriage return, but got '{reply}' instead.") + cmd = self.sendCommand("MOVE", x=int(x), y=int(y), z=int(z)) + if cmd.matchGroups != (b'\r',): + raise Exception(f"Expected carriage return, but got '{cmd.matchGroups}' instead.") def doMoveBy(self, displacement): dx, dy, dz = displacement @@ -150,15 +113,15 @@ def doMoveBy(self, displacement): raise Exception("Unable to read position from device") def doHome(self): - reply = self.sendCommand("HOME") - if reply != (b'\r',): - raise Exception(f"Expected carriage return, but got {reply} instead.") + cmd = self.sendCommand("HOME") + if cmd.matchGroups != (b'\r',): + raise Exception(f"Expected carriage return, but got {cmd.matchGroups} instead.") def work(self): self.home() - reply = self.sendCommand("WORK") - if reply != (b'\r',): - raise Exception(f"Expected carriage return, but got {reply} instead.") + cmd = self.sendCommand("WORK") + if cmd.matchGroups != (b'\r',): + raise Exception(f"Expected carriage return, but got {cmd.matchGroups} instead.") class DebugSerialPort(TableDrivenDebugPort): diff --git a/hardwarelibrary/physicaldevice.py b/hardwarelibrary/physicaldevice.py index b82e2a11..79c45705 100644 --- a/hardwarelibrary/physicaldevice.py +++ b/hardwarelibrary/physicaldevice.py @@ -181,11 +181,21 @@ def stopBackgroundStatusUpdates(self): else: raise RuntimeError("No status loop running") - def sendCommand(self, command): + def sendCommand(self, name, **params): + """Look up the named command in self.commands, send it through + self.port, and return the Command object so callers can read + .reply / .matchGroups / .exceptions / .isSentSuccessfully. + + Params are passed through to Command.send: TextCommand uses them + for .format(**params) substitution into text_format; DataCommand + uses them for buildSendData(**params) when sendFormat is set. + """ if self.state != DeviceState.Ready: raise PhysicalDevice.NotInitialized - command.send(port=self.port) + command = self.commands[name] + command.send(port=self.port, **params) + return command @classmethod def any(cls): diff --git a/hardwarelibrary/tests/testConnectSutter.py b/hardwarelibrary/tests/testConnectSutter.py index 6b49abe7..c373f979 100644 --- a/hardwarelibrary/tests/testConnectSutter.py +++ b/hardwarelibrary/tests/testConnectSutter.py @@ -9,7 +9,7 @@ class TestConnectSutter(unittest.TestCase): def testConnectDebugSutter(self): sutter = SutterDevice(serialNumber="debug") # The port is not open until initializeDevice() - sutter.doInitializeDevice() + sutter.initializeDevice() position = sutter.position() @@ -19,7 +19,7 @@ def testConnectDebugSutter(self): def testMoveToWithDebugSutter(self): sutter = SutterDevice("debug") - sutter.doInitializeDevice() + sutter.initializeDevice() sutter.moveTo((0, 100, 4000)) position = sutter.position() diff --git a/hardwarelibrary/tests/testPhysicalDevice.py b/hardwarelibrary/tests/testPhysicalDevice.py index e03ab774..f7ccc88c 100644 --- a/hardwarelibrary/tests/testPhysicalDevice.py +++ b/hardwarelibrary/tests/testPhysicalDevice.py @@ -263,9 +263,9 @@ def testEchoCommands(self): self.device.initializeDevice() for name, command in self.device.commands.items(): try: - self.device.sendCommand(command) + self.device.sendCommand(name) except Exception as err: - self.fail("Unable to send command {0} to device {1}: {2}".format(command.name, self.device, err)) + self.fail("Unable to send command {0} to device {1}: {2}".format(name, self.device, err)) self.device.shutdownDevice() class TestDebugEchoPhysicalDevice(BaseTestCases.TestPhysicalDeviceBase): @@ -277,9 +277,9 @@ def testEchoCommands(self): self.device.initializeDevice() for name, command in self.device.commands.items(): try: - self.device.sendCommand(command) + self.device.sendCommand(name) except Exception as err: - self.fail("Unable to send command {0} to device {1}: {2}".format(command.name, self.device, err)) + self.fail("Unable to send command {0} to device {1}: {2}".format(name, self.device, err)) self.device.shutdownDevice()