Harden Galil DMC-41x3 motion polling and connection handling

docs/changes/newsfragments/8647.improved_driver

@@ -0,0 +1 @@
+The Galil DMC-41x3 driver now retries empty or malformed controller replies when polling motion status and axis positions, and waiting for motion to complete is bounded by a timeout (300 s by default) that raises ``TimeoutError``. Each motion-status query is limited by the remaining motion timeout, with millisecond resolution, and restores the original I/O timeout afterward. Connecting now falls back through the ``--direct -s ALL`` and ``-s ALL`` options before using the bare address. Pressing Ctrl-C while waiting for motion now calls ``abort()`` and then turns the axes off with ``MO``.

src/qcodes/instrument_drivers/Galil/dmc_41x3.py

@@ -4,6 +4,8 @@
 Colloquially known as the "stepper motors".
 """
 
+import time
+from math import ceil
 from typing import TYPE_CHECKING
 
 import numpy as np
@@ -29,6 +31,48 @@
         "environment."
     ) from e
 
+_POLL_INTERVAL_S = 0.01
+_DEFAULT_MOTION_TIMEOUT_S = 300.0
+_REPLY_RETRIES = 10
+
+
+def _parse_motion_flag(response: str) -> int | None:
+    """
+    Parse the reply to ``MG _BG<axis>``.
+
+    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.
+    """
+    cleaned = response.strip().strip(":").strip()
+    if not cleaned:
+        return None
+    try:
+        return int(float(cleaned))
+    except (ValueError, OverflowError):
+        return None
+
+
+def _parse_position_reply(response: str) -> dict[str, int] | None:
+    """
+    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.
+    """
+    cleaned = response.strip().removesuffix(":").strip()
+    parts = cleaned.split(",")
+    if len(parts) != 3:
+        return None
+    try:
+        a, b, c = (int(float(part)) for part in parts)
+    except (ValueError, OverflowError):
+        return None
+    return {"A": a, "B": b, "C": c}
+
 
 class GalilMotionController(Instrument):
     """
@@ -61,8 +105,35 @@ def open(self) -> None:
         This applies that Motion controller no more requests for an IP address
         and a connection to the Motion controller can be done by the IP
         address burned in.
+
+        If the address already contains gclib switches (e.g.
+        ``"<address> --direct -s ALL"``) it is used as is. Otherwise the
+        address forms ``"<address> --direct -s ALL"``, ``"<address> -s ALL"``
+        and ``"<address>"`` are tried in turn.
+
+        Raises:
+            RuntimeError: if the connection fails for all address forms.
+
         """
-        self.g.GOpen(self._address + " --direct -s ALL")
+        address = self._address
+        if "--" in address or " -" in address:
+            candidates = [address]
+        else:
+            candidates = [f"{address} --direct -s ALL", f"{address} -s ALL", address]
+
+        errors: list[str] = []
+        for candidate in candidates:
+            try:
+                self.g.GOpen(candidate)
+                return
+            except gclib.GclibError as e:
+                errors.append(f"{candidate!r}: {e}")
+
+        raise RuntimeError(
+            f"Failed to connect to Galil controller. Tried address strings: "
+            f"{candidates}. Confirm controller address/subnet reachability and "
+            f"power/link status. Errors: {' | '.join(errors)}"
+        )
 
     def get_idn(self) -> dict[str, str | None]:
         """
@@ -95,7 +166,7 @@ def timeout(self, val: int) -> None:
         Sets timeout for the instrument
 
         Args:
-            val: time in milliseconds. -1 disables the timeout
+            val: time in milliseconds. -1 restores the initial connection timeout.
 
         """
         if val < -1:
@@ -441,19 +512,33 @@ def is_in_motion(self) -> int:
         """
         Checks if the motor is in motion or not. return 1, if motor is in
         motion otherwise 0
-        """
-        return int(float(self.ask(f"MG _BG{self._axis}")))
 
-    def wait_till_motor_motion_complete(self) -> None:
+        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}"
+        )
+
+    def wait_till_motor_motion_complete(
+        self, timeout_s: float = _DEFAULT_MOTION_TIMEOUT_S
+    ) -> None:
         """
         Wait for motion on the motor to complete
+
+        Args:
+            timeout_s: raise ``TimeoutError`` if the motor is still moving, or
+                keeps returning unparsable replies, after this many seconds.
+
         """
-        try:
-            while self.is_in_motion():
-                pass
-        except KeyboardInterrupt:
-            self.parent.abort()
-            self.off()
+        self.parent.wait_till_axes_motion_complete(self._axis, timeout_s)
 
     def error_magnitude(self) -> float:
         """
@@ -550,14 +635,19 @@ 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
-        """
-        result = {}
-        data = self.ask("PA ?,?,?").split(" ")
-        result["A"] = int(data[0][:-1])
-        result["B"] = int(data[1][:-1])
-        result["C"] = int(data[2])
 
-        return result
+        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}"
+        )
 
     def end_program(self) -> None:
         """
@@ -601,20 +691,82 @@ def begin_motors(self) -> None:
         """
         self.write("BG")
 
-    def wait_till_motion_complete(self) -> None:
+    def wait_till_axes_motion_complete(
+        self, axes: str = "ABC", timeout_s: float = _DEFAULT_MOTION_TIMEOUT_S
+    ) -> None:
         """
-        This method waits for the motion on all motors to complete
+        Wait until none of the given axes is moving
+
+        The controller is polled at a fixed interval. Transient empty replies
+        are treated as "not yet finished". Each query's I/O timeout is capped
+        by the remaining motion timeout (rounded up to milliseconds), and the
+        original I/O timeout is restored afterward. I/O errors before the
+        motion deadline are propagated. If the I/O timeout is -1, the initial
+        connection timeout is not exposed by gclib, so only the remaining
+        motion timeout and the 32767 ms API limit bound the query.
+
+        Args:
+            axes: axis letters to wait for, e.g. ``"AB"``.
+            timeout_s: raise ``TimeoutError`` if the axes are still moving, or
+                keep returning unparsable replies or fail to respond, after
+                this many seconds.
+
         """
+        deadline = time.monotonic() + timeout_s
         try:
-            while (
-                int(float(self.ask("MG _BGA")))
-                or int(float(self.ask("MG _BGB")))
-                or int(float(self.ask("MG _BGC")))
-            ):
-                pass
+            while True:
+                flags: dict[str, int | None] = {}
+                for axis in axes:
+                    remaining = deadline - time.monotonic()
+                    if remaining <= 0:
+                        raise TimeoutError(
+                            f"Axes {axes} did not finish moving within "
+                            f"{timeout_s:g}s (motion flags: {flags})."
+                        )
+                    original_timeout = self.g.timeout
+                    # GTimeout takes a signed short in the C API.
+                    query_timeout_ms = min(ceil(remaining * 1000), 32767)
+                    if original_timeout >= 0:
+                        query_timeout_ms = min(query_timeout_ms, original_timeout)
+                    try:
+                        self.timeout(query_timeout_ms)
+                        response = self.ask(f"MG _BG{axis}")
+                    except gclib.GclibError as e:
+                        if time.monotonic() >= deadline:
+                            raise TimeoutError(
+                                f"Axes {axes} did not finish moving within "
+                                f"{timeout_s:g}s (querying axis {axis})."
+                            ) from e
+                        raise
+                    finally:
+                        self.timeout(original_timeout)
+                    flags[axis] = _parse_motion_flag(response)
+                remaining = deadline - time.monotonic()
+                if remaining <= 0:
+                    raise TimeoutError(
+                        f"Axes {axes} did not finish moving within "
+                        f"{timeout_s:g}s (motion flags: {flags})."
+                    )
+                # Done only when every axis reported a definite 0.
+                if all(flag == 0 for flag in flags.values()):
+                    return
+                time.sleep(min(_POLL_INTERVAL_S, remaining))
         except KeyboardInterrupt:
             self.abort()
-            self.motors_off()
+            self.write(f"MO {axes}")
+
+    def wait_till_motion_complete(
+        self, timeout_s: float = _DEFAULT_MOTION_TIMEOUT_S
+    ) -> None:
+        """
+        This method waits for the motion on all motors to complete
+
+        Args:
+            timeout_s: raise ``TimeoutError`` if the motors are still moving
+                after this many seconds.
+
+        """
+        self.wait_till_axes_motion_complete("ABC", timeout_s)
 
 
 class GalilDMC4133Arm: