From f6dc1a89b54274c730a344aa4ab87db7dd6cf1ae Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 07:28:56 +0100 Subject: [PATCH 1/8] Fix Galil setter and absolute position types --- src/qcodes/instrument_drivers/Galil/dmc_41x3.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qcodes/instrument_drivers/Galil/dmc_41x3.py b/src/qcodes/instrument_drivers/Galil/dmc_41x3.py index 758a06424be..5b73c3eb2d7 100644 --- a/src/qcodes/instrument_drivers/Galil/dmc_41x3.py +++ b/src/qcodes/instrument_drivers/Galil/dmc_41x3.py @@ -456,25 +456,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 """ @@ -577,7 +577,7 @@ 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, From b3bc7eb1f1d6a9c86696300d55fd6d389400e364 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 07:35:32 +0100 Subject: [PATCH 2/8] Make Galil absolute position read-only Setting it only updated the cached value without moving the motors. Also add tests for the driver with a stand-in for gclib. --- docs/changes/newsfragments/xxxx.breaking | 2 + .../instrument_drivers/Galil/dmc_41x3.py | 2 +- tests/drivers/test_galil_dmc_41x3.py | 115 ++++++++++++++++++ 3 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 docs/changes/newsfragments/xxxx.breaking create mode 100644 tests/drivers/test_galil_dmc_41x3.py diff --git a/docs/changes/newsfragments/xxxx.breaking b/docs/changes/newsfragments/xxxx.breaking new file mode 100644 index 00000000000..de0fcb58e05 --- /dev/null +++ b/docs/changes/newsfragments/xxxx.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/src/qcodes/instrument_drivers/Galil/dmc_41x3.py b/src/qcodes/instrument_drivers/Galil/dmc_41x3.py index 5b73c3eb2d7..6cd629c446b 100644 --- a/src/qcodes/instrument_drivers/Galil/dmc_41x3.py +++ b/src/qcodes/instrument_drivers/Galil/dmc_41x3.py @@ -580,7 +580,7 @@ def __init__( 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", ) diff --git a/tests/drivers/test_galil_dmc_41x3.py b/tests/drivers/test_galil_dmc_41x3.py new file mode 100644 index 00000000000..717ba083a5a --- /dev/null +++ b/tests/drivers/test_galil_dmc_41x3.py @@ -0,0 +1,115 @@ +""" +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.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] = [] + + 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: + pass + + def GClose(self) -> None: + pass + + +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) + 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() + # GalilMotionController.close does not call Instrument.close, so the + # instrument would stay registered under its name + Instrument.close(driver) + # do not leave the driver imported with the stand-in gclib + for name in _DRIVER_MODULES: + sys.modules.pop(name, None) + + +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}) From 180942705ea7860adc2975d4c3a5a9672b70ef24 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 07:35:51 +0100 Subject: [PATCH 3/8] Parse whole Galil motor status replies on_off_status and off_when_error_occurs only looked at the first character of the reply, so the empty or bare ':' replies that gclib can return under rapid polling raised or gave the wrong state. Retry them like is_in_motion does. --- .../newsfragments/xxxx.improved_driver | 2 ++ .../instrument_drivers/Galil/dmc_41x3.py | 26 ++++++++++++++----- tests/drivers/test_galil_dmc_41x3.py | 24 +++++++++++++++++ 3 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 docs/changes/newsfragments/xxxx.improved_driver diff --git a/docs/changes/newsfragments/xxxx.improved_driver b/docs/changes/newsfragments/xxxx.improved_driver new file mode 100644 index 00000000000..97589ad3288 --- /dev/null +++ b/docs/changes/newsfragments/xxxx.improved_driver @@ -0,0 +1,2 @@ +``GalilDMC4133Motor.on_off_status`` and ``off_when_error_occurs`` now parse the whole reply and +retry empty replies, like ``is_in_motion``, instead of reading only its first character. diff --git a/src/qcodes/instrument_drivers/Galil/dmc_41x3.py b/src/qcodes/instrument_drivers/Galil/dmc_41x3.py index 6cd629c446b..fdf4b0f30ac 100644 --- a/src/qcodes/instrument_drivers/Galil/dmc_41x3.py +++ b/src/qcodes/instrument_drivers/Galil/dmc_41x3.py @@ -38,7 +38,7 @@ def _parse_motion_flag(response: str) -> int | None: """ - Parse the reply to ``MG _BG``. + Parse an integer reply to an ``MG`` query, e.g. ``MG _BG``. Under rapid polling gclib can return the bare ``:`` prompt or an empty string instead of a number. Return ``None`` for such transient or @@ -446,9 +446,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 self._ask_int(f"MG _OE{self._axis}") def _set_off_when_error_occurs(self, val: int) -> None: """ @@ -490,12 +488,28 @@ def on_off_status(self) -> str: """ Tells motor on off status """ - val = self.ask(f"MG _MO{self._axis}") - if val[0] == "1": + if self._ask_int(f"MG _MO{self._axis}") == 1: return "off" else: return "on" + def _ask_int(self, cmd: str) -> int: + """ + Ask for an integer reply, e.g. to ``MG _MO``. + + Transient empty replies of the controller are retried. + """ + response = "" + for _ in range(_REPLY_RETRIES): + response = self.ask(cmd) + value = _parse_motion_flag(response) + if value is not None: + return value + time.sleep(_POLL_INTERVAL_S) + raise RuntimeError( + f"Could not parse reply to {cmd!r} after retries: {response!r}" + ) + def servo_here(self) -> None: """ Servo at the motor diff --git a/tests/drivers/test_galil_dmc_41x3.py b/tests/drivers/test_galil_dmc_41x3.py index 717ba083a5a..8d03d442820 100644 --- a/tests/drivers/test_galil_dmc_41x3.py +++ b/tests/drivers/test_galil_dmc_41x3.py @@ -113,3 +113,27 @@ def test_absolute_position_is_read_only(galil: "GalilDMC4133Controller") -> None 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")]) +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="Could not parse reply to 'MG _MOA'"): + galil.motor_a.on_off_status() From 7f02aa5f034571e29d1bf48e7bbbd1f15a14fdc1 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 07:39:11 +0100 Subject: [PATCH 4/8] Call Instrument.close when closing Galil controller close() only closed the gclib connection, so the controller stayed registered and could not be created again with the same name. --- .../newsfragments/xxxx.improved_driver | 2 ++ .../instrument_drivers/Galil/dmc_41x3.py | 8 ++++++- tests/drivers/test_galil_dmc_41x3.py | 22 +++++++++++++++---- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/docs/changes/newsfragments/xxxx.improved_driver b/docs/changes/newsfragments/xxxx.improved_driver index 97589ad3288..c6ab321cb6f 100644 --- a/docs/changes/newsfragments/xxxx.improved_driver +++ b/docs/changes/newsfragments/xxxx.improved_driver @@ -1,2 +1,4 @@ ``GalilDMC4133Motor.on_off_status`` and ``off_when_error_occurs`` now parse the whole reply and retry empty replies, like ``is_in_motion``, instead of reading 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 fdf4b0f30ac..64aca58cb62 100644 --- a/src/qcodes/instrument_drivers/Galil/dmc_41x3.py +++ b/src/qcodes/instrument_drivers/Galil/dmc_41x3.py @@ -178,7 +178,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: """ diff --git a/tests/drivers/test_galil_dmc_41x3.py b/tests/drivers/test_galil_dmc_41x3.py index 8d03d442820..29235c667c4 100644 --- a/tests/drivers/test_galil_dmc_41x3.py +++ b/tests/drivers/test_galil_dmc_41x3.py @@ -38,6 +38,7 @@ class FakeGclibConnection: def __init__(self) -> None: self.replies: dict[str, list[str]] = {} self.commands: list[str] = [] + self.closed = False def GOpen(self, address: str) -> None: pass @@ -55,7 +56,7 @@ def GTimeout(self, timeout: int) -> None: pass def GClose(self) -> None: - pass + self.closed = True def _make_fake_gclib() -> ModuleType: @@ -79,9 +80,6 @@ def _make_galil( yield driver finally: driver.close() - # GalilMotionController.close does not call Instrument.close, so the - # instrument would stay registered under its name - Instrument.close(driver) # do not leave the driver imported with the stand-in gclib for name in _DRIVER_MODULES: sys.modules.pop(name, None) @@ -137,3 +135,19 @@ def test_unparsable_reply_raises(galil: "GalilDMC4133Controller") -> None: with pytest.raises(RuntimeError, match="Could not parse reply to 'MG _MOA'"): galil.motor_a.on_off_status() + + +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() From 01ee2101995b275b6923dcbe35f9ea432fa257b7 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 07:43:39 +0100 Subject: [PATCH 5/8] Retry empty Galil replies for every query Move the retry of empty or bare ':' replies into ask_raw instead of separate loops in a few methods, so all getters get it. The motion wait loop polls once per query and keeps its own deadline handling. Malformed non-empty replies now raise instead of being retried. --- .../newsfragments/xxxx.improved_driver | 7 +- .../instrument_drivers/Galil/dmc_41x3.py | 111 ++++++++---------- tests/drivers/test_galil_dmc_41x3.py | 37 +++++- 3 files changed, 90 insertions(+), 65 deletions(-) diff --git a/docs/changes/newsfragments/xxxx.improved_driver b/docs/changes/newsfragments/xxxx.improved_driver index c6ab321cb6f..58712f32d8a 100644 --- a/docs/changes/newsfragments/xxxx.improved_driver +++ b/docs/changes/newsfragments/xxxx.improved_driver @@ -1,4 +1,7 @@ -``GalilDMC4133Motor.on_off_status`` and ``off_when_error_occurs`` now parse the whole reply and -retry empty replies, like ``is_in_motion``, instead of reading only its first character. +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 64aca58cb62..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 an integer reply to an ``MG`` query, e.g. ``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: """ @@ -452,7 +479,7 @@ def _get_off_when_error_occurs(self) -> int: Gets the status if motor is automatically set to turn off when error occurs. """ - return self._ask_int(f"MG _OE{self._axis}") + return int(float(self.ask(f"MG _OE{self._axis}"))) def _set_off_when_error_occurs(self, val: int) -> None: """ @@ -494,28 +521,11 @@ def on_off_status(self) -> str: """ Tells motor on off status """ - if self._ask_int(f"MG _MO{self._axis}") == 1: + if int(float(self.ask(f"MG _MO{self._axis}"))) == 1: return "off" else: return "on" - def _ask_int(self, cmd: str) -> int: - """ - Ask for an integer reply, e.g. to ``MG _MO``. - - Transient empty replies of the controller are retried. - """ - response = "" - for _ in range(_REPLY_RETRIES): - response = self.ask(cmd) - value = _parse_motion_flag(response) - if value is not None: - return value - time.sleep(_POLL_INTERVAL_S) - raise RuntimeError( - f"Could not parse reply to {cmd!r} after retries: {response!r}" - ) - def servo_here(self) -> None: """ Servo at the motor @@ -532,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 @@ -655,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: """ @@ -750,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 index 29235c667c4..a642db0e2c1 100644 --- a/tests/drivers/test_galil_dmc_41x3.py +++ b/tests/drivers/test_galil_dmc_41x3.py @@ -39,6 +39,7 @@ 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 @@ -53,7 +54,7 @@ def GCommand(self, command: str) -> str: return replies.pop(0) if replies else "" def GTimeout(self, timeout: int) -> None: - pass + self.timeout = timeout def GClose(self) -> None: self.closed = True @@ -133,10 +134,42 @@ def test_off_when_error_occurs(galil: "GalilDMC4133Controller") -> None: def test_unparsable_reply_raises(galil: "GalilDMC4133Controller") -> None: _connection(galil).replies["MG _MOA"] = [":"] * 20 - with pytest.raises(RuntimeError, match="Could not parse reply to 'MG _MOA'"): + 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) From 026dcdb3f4de4c2e3ef40ce10aac929df5b9bf61 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 27 Oct 2026 08:23:53 +0100 Subject: [PATCH 6/8] Rename newsfragments to PR 8677 --- docs/changes/newsfragments/{xxxx.breaking => 8677.breaking} | 0 .../newsfragments/{xxxx.improved_driver => 8677.improved_driver} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename docs/changes/newsfragments/{xxxx.breaking => 8677.breaking} (100%) rename docs/changes/newsfragments/{xxxx.improved_driver => 8677.improved_driver} (100%) diff --git a/docs/changes/newsfragments/xxxx.breaking b/docs/changes/newsfragments/8677.breaking similarity index 100% rename from docs/changes/newsfragments/xxxx.breaking rename to docs/changes/newsfragments/8677.breaking diff --git a/docs/changes/newsfragments/xxxx.improved_driver b/docs/changes/newsfragments/8677.improved_driver similarity index 100% rename from docs/changes/newsfragments/xxxx.improved_driver rename to docs/changes/newsfragments/8677.improved_driver From cb7bf602cf4308007a07910c7d5c0bfdcfaf35ec Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Thu, 29 Oct 2026 07:31:21 +0100 Subject: [PATCH 7/8] Restore Galil package attribute after tests Importing the driver binds Galil on qcodes.instrument_drivers, so removing the sys.modules entries alone left the driver built on the stand-in gclib importable after the tests. --- tests/drivers/test_galil_dmc_41x3.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/drivers/test_galil_dmc_41x3.py b/tests/drivers/test_galil_dmc_41x3.py index a642db0e2c1..f5a4ca1de4c 100644 --- a/tests/drivers/test_galil_dmc_41x3.py +++ b/tests/drivers/test_galil_dmc_41x3.py @@ -12,6 +12,7 @@ import pytest +from qcodes import instrument_drivers from qcodes.instrument import Instrument if TYPE_CHECKING: @@ -74,6 +75,8 @@ def _make_galil( 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") @@ -84,6 +87,10 @@ def _make_galil( # 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: From 65142a44e97da4491af574df784d5ec8cb5f06b6 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Thu, 29 Oct 2026 07:31:22 +0100 Subject: [PATCH 8/8] Test Galil on/off status with leading zeros --- tests/drivers/test_galil_dmc_41x3.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/drivers/test_galil_dmc_41x3.py b/tests/drivers/test_galil_dmc_41x3.py index f5a4ca1de4c..39965c3ada1 100644 --- a/tests/drivers/test_galil_dmc_41x3.py +++ b/tests/drivers/test_galil_dmc_41x3.py @@ -121,7 +121,15 @@ def test_absolute_position_is_read_only(galil: "GalilDMC4133Controller") -> None galil.absolute_position({"A": 1, "B": 2, "C": 3}) -@pytest.mark.parametrize(("reply", "expected"), [("1.0000", "off"), ("0.0000", "on")]) +@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: