diff --git a/docs/changes/newsfragments/8677.breaking b/docs/changes/newsfragments/8677.breaking new file mode 100644 index 00000000000..de0fcb58e05 --- /dev/null +++ b/docs/changes/newsfragments/8677.breaking @@ -0,0 +1,2 @@ +``GalilDMC4133Controller.absolute_position`` is now read-only. Setting it previously only +updated the cached value without moving the motors, and now raises an error. diff --git a/docs/changes/newsfragments/8677.improved_driver b/docs/changes/newsfragments/8677.improved_driver new file mode 100644 index 00000000000..58712f32d8a --- /dev/null +++ b/docs/changes/newsfragments/8677.improved_driver @@ -0,0 +1,7 @@ +All queries of the Galil DMC-41x3 driver now retry the empty or bare ``:`` replies that gclib +can return under rapid polling, instead of only ``is_in_motion`` and ``absolute_position``. +Non-empty replies that cannot be parsed, such as an absolute position with fewer than three +values, now raise immediately instead of being retried. ``GalilDMC4133Motor.on_off_status`` and +``off_when_error_occurs`` now parse the whole reply instead of only its first character. +``GalilMotionController.close`` now also closes the QCoDeS instrument, so a closed controller +can be created again with the same name. diff --git a/src/qcodes/instrument_drivers/Galil/dmc_41x3.py b/src/qcodes/instrument_drivers/Galil/dmc_41x3.py index 758a06424be..813f39c5e18 100644 --- a/src/qcodes/instrument_drivers/Galil/dmc_41x3.py +++ b/src/qcodes/instrument_drivers/Galil/dmc_41x3.py @@ -36,15 +36,22 @@ _REPLY_RETRIES = 10 +def _strip_prompt(response: str) -> str: + """ + Remove whitespace and the ``:`` prompt that gclib can leave around a reply. + """ + return response.strip().strip(":").strip() + + def _parse_motion_flag(response: str) -> int | None: """ - Parse the reply to ``MG _BG``. + Parse a single reply to ``MG _BG`` while polling for motion. Under rapid polling gclib can return the bare ``:`` prompt or an empty string instead of a number. Return ``None`` for such transient or - otherwise unparsable replies so that the caller can retry. + otherwise unparsable replies so that the caller can poll again. """ - cleaned = response.strip().strip(":").strip() + cleaned = _strip_prompt(response) if not cleaned: return None try: @@ -53,24 +60,22 @@ def _parse_motion_flag(response: str) -> int | None: return None -def _parse_position_reply(response: str) -> dict[str, int] | None: +def _parse_position_reply(response: str) -> dict[str, int]: """ Parse the reply to ``PA ?,?,?`` into a dictionary with keys ``"A"``, ``"B"`` and ``"C"``. - The controller returns exactly three numeric positions separated by commas and - sometimes terminated by the ``:`` prompt, e.g. ``"100, 200, 300"``. - Return ``None`` for a bare ``:`` prompt, an empty reply or a malformed - reply so that the caller can retry. + The controller returns exactly three numeric positions separated by + commas, e.g. ``"100, 200, 300"``. + + Raises: + ValueError: if the reply does not contain three numbers. + """ - cleaned = response.strip().removesuffix(":").strip() - parts = cleaned.split(",") + parts = _strip_prompt(response).split(",") if len(parts) != 3: - return None - try: - a, b, c = (int(float(part)) for part in parts) - except (ValueError, OverflowError): - return None + raise ValueError(f"Expected three positions, got {response!r}") + a, b, c = (int(float(part)) for part in parts) return {"A": a, "B": b, "C": c} @@ -158,8 +163,30 @@ def write_raw(self, cmd: str) -> None: def ask_raw(self, cmd: str) -> str: """ Asks/Reads data from Galil motion controller + + Under rapid polling gclib can return the bare ``:`` prompt or an + empty string instead of the reply. Such replies are retried a few + times before giving up. The prompt is removed from the reply. + + Raises: + RuntimeError: if the controller keeps returning empty replies. + """ - return self.g.GCommand(cmd + "\r") + for _ in range(_REPLY_RETRIES): + response = self._ask_once(cmd) + if response: + return response + time.sleep(_POLL_INTERVAL_S) + raise RuntimeError( + f"Got only empty replies to {cmd!r} after {_REPLY_RETRIES} attempts" + ) + + def _ask_once(self, cmd: str) -> str: + """ + Ask once without retrying an empty reply. The ``:`` prompt is removed + from the reply. + """ + return _strip_prompt(self.g.GCommand(cmd + "\r")) def timeout(self, val: int) -> None: """ @@ -178,7 +205,13 @@ def close(self) -> None: """ Close connection to the instrument """ - self.g.GClose() + try: + # the connection is removed by Instrument.close, so a second close + # has nothing to close + if hasattr(self, "g"): + self.g.GClose() + finally: + super().close() def motion_complete(self, axes: str) -> None: """ @@ -446,9 +479,7 @@ def _get_off_when_error_occurs(self) -> int: Gets the status if motor is automatically set to turn off when error occurs. """ - val = self.ask(f"MG _OE{self._axis}") - - return int(val[0]) + return int(float(self.ask(f"MG _OE{self._axis}"))) def _set_off_when_error_occurs(self, val: int) -> None: """ @@ -456,25 +487,25 @@ def _set_off_when_error_occurs(self, val: int) -> None: """ self.write(f"OE{self._axis}={val}") - def _set_deceleration(self, val: str) -> None: + def _set_deceleration(self, val: int) -> None: """ Set deceleration for the motor's motion """ self.write(f"DC{self._axis}={val}") - def _set_acceleration(self, val: str) -> None: + def _set_acceleration(self, val: int) -> None: """ Set acceleration for the motor's motion """ self.write(f"AC{self._axis}={val}") - def _set_speed(self, val: str) -> None: + def _set_speed(self, val: int) -> None: """ Sets speed for motor's motion """ self.write(f"SP{self._axis}={val}") - def _set_relative_position(self, val: str) -> None: + def _set_relative_position(self, val: int) -> None: """ Sets relative position """ @@ -490,8 +521,7 @@ def on_off_status(self) -> str: """ Tells motor on off status """ - val = self.ask(f"MG _MO{self._axis}") - if val[0] == "1": + if int(float(self.ask(f"MG _MO{self._axis}"))) == 1: return "off" else: return "on" @@ -512,20 +542,8 @@ def is_in_motion(self) -> int: """ Checks if the motor is in motion or not. return 1, if motor is in motion otherwise 0 - - Transient empty replies of the controller are retried. """ - response = "" - for _ in range(_REPLY_RETRIES): - response = self.ask(f"MG _BG{self._axis}") - flag = _parse_motion_flag(response) - if flag is not None: - return flag - time.sleep(_POLL_INTERVAL_S) - raise RuntimeError( - f"Could not parse motion flag of axis {self._axis} after retries: " - f"{response!r}" - ) + return int(float(self.ask(f"MG _BG{self._axis}"))) def wait_till_motor_motion_complete( self, timeout_s: float = _DEFAULT_MOTION_TIMEOUT_S @@ -577,10 +595,10 @@ def __init__( ) """sets number of decimals in the format of the position""" - self.absolute_position: Parameter = self.add_parameter( + self.absolute_position: Parameter[dict[str, int], Self] = self.add_parameter( "absolute_position", get_cmd=self._get_absolute_position, - set_cmd=None, + set_cmd=False, unit="quadrature counts", docstring="gets absolute position of the motors from the set origin", ) @@ -635,19 +653,8 @@ def _set_default_update_time(self) -> None: def _get_absolute_position(self) -> dict[str, int]: """ Gets absolution position of the motors from the defined origin - - Transient empty replies of the controller are retried. """ - response = "" - for _ in range(_REPLY_RETRIES): - response = self.ask("PA ?,?,?") - parsed = _parse_position_reply(response) - if parsed is not None: - return parsed - time.sleep(_POLL_INTERVAL_S) - raise RuntimeError( - f"Could not parse absolute position reply after retries: {response!r}" - ) + return _parse_position_reply(self.ask("PA ?,?,?")) def end_program(self) -> None: """ @@ -730,7 +737,9 @@ def wait_till_axes_motion_complete( query_timeout_ms = min(query_timeout_ms, original_timeout) try: self.timeout(query_timeout_ms) - response = self.ask(f"MG _BG{axis}") + # poll once per query; an empty reply is treated as + # "not yet finished" and polled again until the deadline + response = self._ask_once(f"MG _BG{axis}") except gclib.GclibError as e: if time.monotonic() >= deadline: raise TimeoutError( diff --git a/tests/drivers/test_galil_dmc_41x3.py b/tests/drivers/test_galil_dmc_41x3.py new file mode 100644 index 00000000000..39965c3ada1 --- /dev/null +++ b/tests/drivers/test_galil_dmc_41x3.py @@ -0,0 +1,201 @@ +""" +Tests for the Galil DMC-41x3 driver. + +The driver needs the ``gclib`` package from Galil, which is not available on +PyPI, so a minimal stand-in for ``gclib`` is installed for these tests. +""" + +import importlib +import sys +from types import ModuleType +from typing import TYPE_CHECKING + +import pytest + +from qcodes import instrument_drivers +from qcodes.instrument import Instrument + +if TYPE_CHECKING: + from collections.abc import Generator + + from qcodes.instrument_drivers.Galil.dmc_41x3 import GalilDMC4133Controller + +_DRIVER_MODULES = ( + "qcodes.instrument_drivers.Galil.dmc_41x3", + "qcodes.instrument_drivers.Galil", +) + + +class FakeGclibError(Exception): + pass + + +class FakeGclibConnection: + """ + Stand-in for ``gclib.py``. Queries are answered from ``replies``, a + list of replies per command that are returned in turn. + """ + + def __init__(self) -> None: + self.replies: dict[str, list[str]] = {} + self.commands: list[str] = [] + self.closed = False + self.timeout = 5000 + + def GOpen(self, address: str) -> None: + pass + + def GInfo(self) -> str: + return "192.168.1.30, DMC4133 Rev 1.2c, 12345" + + def GCommand(self, command: str) -> str: + command = command.removesuffix("\r") + self.commands.append(command) + replies = self.replies.get(command) + return replies.pop(0) if replies else "" + + def GTimeout(self, timeout: int) -> None: + self.timeout = timeout + + def GClose(self) -> None: + self.closed = True + + +def _make_fake_gclib() -> ModuleType: + gclib = ModuleType("gclib") + gclib.py = FakeGclibConnection # type: ignore[attr-defined] + gclib.GclibError = FakeGclibError # type: ignore[attr-defined] + return gclib + + +@pytest.fixture(name="galil") +def _make_galil( + monkeypatch: pytest.MonkeyPatch, +) -> "Generator[GalilDMC4133Controller, None, None]": + monkeypatch.setitem(sys.modules, "gclib", _make_fake_gclib()) + for name in _DRIVER_MODULES: + monkeypatch.delitem(sys.modules, name, raising=False) + # importing the driver also binds ``Galil`` on the parent package + original_galil = instrument_drivers.__dict__.get("Galil") + dmc_41x3 = importlib.import_module("qcodes.instrument_drivers.Galil.dmc_41x3") + + driver = dmc_41x3.GalilDMC4133Controller("galil", address="192.168.1.30") + try: + yield driver + finally: + driver.close() + # do not leave the driver imported with the stand-in gclib + for name in _DRIVER_MODULES: + sys.modules.pop(name, None) + if original_galil is None: + instrument_drivers.__dict__.pop("Galil", None) + else: + instrument_drivers.__dict__["Galil"] = original_galil + + +def _connection(galil: "GalilDMC4133Controller") -> FakeGclibConnection: + assert isinstance(galil.g, FakeGclibConnection) + return galil.g + + +def test_idn(galil: "GalilDMC4133Controller") -> None: + assert galil.IDN() == { + "vendor": "Galil Motion Control, Inc.", + "model": "DMC4133", + "serial": "12345", + "firmware": "1.2c", + } + + +def test_absolute_position(galil: "GalilDMC4133Controller") -> None: + _connection(galil).replies["PA ?,?,?"] = [":", "100, 200, 300:"] + + assert galil.absolute_position() == {"A": 100, "B": 200, "C": 300} + + +def test_absolute_position_is_read_only(galil: "GalilDMC4133Controller") -> None: + # setting the absolute position used to only update the cached value + # without moving the motors + assert not galil.absolute_position.settable + with pytest.raises(NotImplementedError): + galil.absolute_position({"A": 1, "B": 2, "C": 3}) + + +@pytest.mark.parametrize( + ("reply", "expected"), + [ + ("1.0000", "off"), + ("0.0000", "on"), + # with LZ0 the controller does not strip leading zeros + ("0000000001.0000", "off"), + ], +) +def test_on_off_status( + galil: "GalilDMC4133Controller", reply: str, expected: str +) -> None: + # gclib can return an empty reply or the bare ':' prompt under rapid + # polling, which is retried + _connection(galil).replies["MG _MOA"] = ["", ":", reply] + + assert galil.motor_a.on_off_status() == expected + + +def test_off_when_error_occurs(galil: "GalilDMC4133Controller") -> None: + _connection(galil).replies["MG _OEA"] = [":", "3.0000"] + + assert galil.motor_a.off_when_error_occurs() == "enable for all" + + +def test_unparsable_reply_raises(galil: "GalilDMC4133Controller") -> None: + _connection(galil).replies["MG _MOA"] = [":"] * 20 + + with pytest.raises(RuntimeError, match="Got only empty replies to 'MG _MOA'"): + galil.motor_a.on_off_status() + + +def test_parameters_retry_empty_replies(galil: "GalilDMC4133Controller") -> None: + # every query retries empty replies, not only the ones that used to + # have their own retry loop + _connection(galil).replies["MG _SPA"] = ["", ":", "2000.0000"] + _connection(galil).replies["VS ?"] = [":", "4000.0000:"] + + assert galil.motor_a.speed() == 2000 + assert galil.plane_ab.vector_speed() == 4000 + + +def test_malformed_position_raises(galil: "GalilDMC4133Controller") -> None: + _connection(galil).replies["PA ?,?,?"] = ["100, 200"] + + with pytest.raises(ValueError, match="Expected three positions"): + galil.absolute_position() + + +def test_wait_till_motion_complete(galil: "GalilDMC4133Controller") -> None: + # an empty reply while waiting counts as "not yet finished" and is polled + # again; the wait ends when every axis reports 0 + connection = _connection(galil) + connection.replies["MG _BGA"] = ["1.0000", "", "0.0000"] + connection.replies["MG _BGB"] = ["0.0000"] * 3 + connection.replies["MG _BGC"] = [":", "0.0000", "0.0000"] + + galil.wait_till_motion_complete(timeout_s=5) + + assert connection.commands.count("MG _BGA") == 3 + # the I/O timeout is restored after polling + assert connection.timeout == 5000 + + +def test_close(galil: "GalilDMC4133Controller") -> None: + connection = _connection(galil) + driver_class = type(galil) + + galil.close() + + assert connection.closed + # the closed controller is no longer registered, so the name can be reused + assert not Instrument.exist("galil") + reopened = driver_class("galil", address="192.168.1.30") + reopened.close() + + # closing again does nothing + galil.close()