AMI430 improvments

docs/changes/newsfragments/7923.improved_driver

@@ -0,0 +1,8 @@
+Converted ``get_error``, ``ramp``, ``pause``, ``zero``, ``reset_quench``, ``set_quenched``,
+and ``reset`` on ``AMIModel430`` from QCoDeS functions (``add_function``) to proper methods.
+The ``zero`` method now invalidates the ``field`` parameter cache and updates the parent
+``AMIModel4303D`` driver's internal setpoint tracking when called on a child instrument.
+Fixed ``AMIModel4303D`` to properly set ``_parent_instrument`` on child ``AMIModel430``
+instruments during initialization, enabling the safety-check delegation path in
+``set_field``. Also fixed ``_request_field_change`` to correctly call ``_set_setpoints``
+instead of non-existent ``_set_x``/``_set_y``/``_set_z`` methods.

src/qcodes/instrument/sims/AMI430.yaml

@@ -1,7 +1,6 @@
 spec: "1.0"
 
 devices:
-
   device 1:
     eom:
       GPIB INSTR:
@@ -11,9 +10,12 @@ devices:
     dialogues:
       - q: "*IDN?"
         r: "QCoDeS, AMI430_simulation, 1337, 0.0.01"
+      - q: "SYST:ERR?"
+        r: "0, No Error"
+      - q: "*RST"
+        r: ""
 
     properties:
-
       ramp rate units:
         default: 0
         getter:
@@ -39,41 +41,41 @@ devices:
           q: "CONF:CURR:LIMIT {}"
 
       ramp rate current first segment:
-        default: '50, 50, 50'
+        default: "50, 50, 50"
         getter:
-          q: 'RAMP:RATE:CURRENT:1?'
-          r: '{}'
+          q: "RAMP:RATE:CURRENT:1?"
+          r: "{}"
         setter:
-          q: 'CONF:RAMP:RATE:CURRENT:1 {}'
+          q: "CONF:RAMP:RATE:CURRENT:1 {}"
 
       segment for ramp rate:
         # This is some segment value that gets set in the driver code
         # when ramp rate field parameter is set
         default: 1
         setter:
-          q: 'CONF:RAMP:RATE:SEG {}'
+          q: "CONF:RAMP:RATE:SEG {}"
 
       ramp rate field first segment:
-        default: '0.11, 0.11, 0.11'
+        default: "0.11, 0.11, 0.11"
         getter:
-          q: 'RAMP:RATE:FIELD:1?'
-          r: '{}'
+          q: "RAMP:RATE:FIELD:1?"
+          r: "{}"
         setter:
-          q: 'CONF:RAMP:RATE:FIELD 1,{},0'
+          q: "CONF:RAMP:RATE:FIELD 1,{},0"
 
       ramp target:
-        default: 0  # or what?
+        default: 0 # or what?
         getter:
           q: "FIELD:TARG?"
           r: "{}"
         #setter:  # this is commented out because two properties can not share a setter
-         # q: "CONF:FIELD:TARG {}"
+        # q: "CONF:FIELD:TARG {}"
 
       coil constant:
         default: 2.0
         getter:
-          q: 'COIL?'
-          r: '{}'
+          q: "COIL?"
+          r: "{}"
         setter:
           q: "CONF:COIL {}"
 
@@ -83,13 +85,15 @@ devices:
           q: "FIELD:MAG?"
           r: "{}"
         setter:
-          q: "CONF:FIELD:TARG {}"  # in the simulated instrument, the target is reached
+          q: "CONF:FIELD:TARG {}" # in the simulated instrument, the target is reached
 
       ramping state:
-        default: 2  # always in holding state, always ready
+        default: 2 # always in holding state, always ready
         getter:
           q: "STATE?"
           r: "{}"
+        setter:
+          q: "CONF:STATE {}"
 
       quench state:
         default: 0
@@ -104,6 +108,8 @@ devices:
         getter:
           q: "PERS?"
           r: "{}"
+        setter:
+          q: "CONF:PERS {}"
 
       persistent heater state:
         default: 0
@@ -137,7 +143,6 @@ devices:
         setter:
           q: "CONF:PS:CTIME {}"
 
-
       pause:
         setter:
           q: "PAUSE"
@@ -146,6 +151,10 @@ devices:
         setter:
           q: "RAMP"
 
+      zero:
+        setter:
+          q: "ZERO"
+
       current rating:
         default: 3
         getter:
@@ -162,7 +171,6 @@ devices:
         setter:
           q: "CONF:PS {}"
 
-
 # we always need three power supplies, one for each axis.
 # For the testing we add a few more.
 resources:

src/qcodes/instrument_drivers/american_magnetics/AMI430_visa.py

@@ -45,7 +45,7 @@ class AMI430Warning(UserWarning):
     pass
 
 
-class AMI430SwitchHeater(InstrumentChannel):
+class AMI430SwitchHeater(InstrumentChannel["AMIModel430"]):
     class _Decorators:
         @classmethod
         def check_enabled(
@@ -138,14 +138,14 @@ def _check_enabled(self) -> bool:
     @_Decorators.check_enabled
     def _on(self) -> None:
         self.write("PS 1")
-        while self._parent.ramping_state() == "heating switch":
-            self._parent._sleep(0.5)
+        while self.parent.ramping_state() == "heating switch":
+            self.parent._sleep(0.5)
 
     @_Decorators.check_enabled
     def _off(self) -> None:
         self.write("PS 0")
-        while self._parent.ramping_state() == "cooling switch":
-            self._parent._sleep(0.5)
+        while self.parent.ramping_state() == "cooling switch":
+            self.parent._sleep(0.5)
 
     def _check_state(self) -> bool:
         if self.enabled() is False:
@@ -218,10 +218,8 @@ def __init__(
                 # read the hello part of the welcome message
                 self.visa_handle.read()
 
-        self._parent_instrument = None
+        self._parent_instrument: AMIModel4303D | None = None
 
-        # Add reset function
-        self.add_function("reset", call_cmd="*RST")
         if reset:
             self.reset()
 
@@ -251,8 +249,8 @@ def __init__(
         """Parameter current_ramp_limit"""
         self.field_ramp_limit: Parameter = self.add_parameter(
             "field_ramp_limit",
-            get_cmd=self.current_ramp_limit,
-            set_cmd=self.current_ramp_limit,
+            get_cmd=self.current_ramp_limit.get,
+            set_cmd=self.current_ramp_limit.set,
             scale=1 / float(self.ask("COIL?")),
             unit="T/s",
         )
@@ -309,8 +307,7 @@ def __init__(
             "is_quenched", get_cmd="QU?", val_mapping={True: 1, False: 0}
         )
         """Parameter is_quenched"""
-        self.add_function("reset_quench", call_cmd="QU 0")
-        self.add_function("set_quenched", call_cmd="QU 1")
+
         self.ramping_state: Parameter = self.add_parameter(
             "ramping_state",
             get_cmd="STATE?",
@@ -345,17 +342,42 @@ def __init__(
         )
         """Submodule the switch heater submodule."""
 
-        # Add interaction functions
-        self.add_function("get_error", call_cmd="SYST:ERR?")
-        self.add_function("ramp", call_cmd="RAMP")
-        self.add_function("pause", call_cmd="PAUSE")
-        self.add_function("zero", call_cmd="ZERO")
-
         # Correctly assign all units
         self._update_units()
 
         self.connect_message()
 
+    def get_error(self) -> str:
+        """Get the last error from the instrument"""
+        return self.ask("SYST:ERR?")
+
+    def ramp(self) -> None:
+        """Start ramping to the setpoint"""
+        self.write("RAMP")
+
+    def pause(self) -> None:
+        """Pause ramping"""
+        self.write("PAUSE")
+
+    def zero(self) -> None:
+        """Ramp to zero current"""
+        self.write("ZERO")
+        self.field.cache.invalidate()
+        if self._parent_instrument is not None:
+            self._parent_instrument._update_setpoint_on_child_zero(self)
+
+    def reset_quench(self) -> None:
+        """Reset a quench condition on the instrument"""
+        self.write("QU 0")
+
+    def set_quenched(self) -> None:
+        """Set a quench condition on the instrument"""
+        self.write("QU 1")
+
+    def reset(self) -> None:
+        """Reset the instrument to default settings"""
+        self.write("*RST")
+
     def _sleep(self, t: float) -> None:
         """
         Sleep for a number of seconds t. If we are or using
@@ -662,6 +684,10 @@ def find_ami430_with_name(ami430_name: str) -> AMIModel430:
             else find_ami430_with_name(instrument_z)
         )
 
+        self._instrument_x._parent_instrument = self
+        self._instrument_y._parent_instrument = self
+        self._instrument_z._parent_instrument = self
+
         self._field_limit: float | Iterable[CartesianFieldLimitFunction]
         if isinstance(field_limit, Iterable):
             self._field_limit = field_limit
@@ -1034,8 +1060,12 @@ def _adjust_child_instruments(self, values: tuple[float, float, float]) -> None:
             raise ValueError("_set_fields aborted; field would exceed limit")
 
         # Check if the individual instruments are ready
-        for name in ("x", "y", "z"):
-            instrument = getattr(self, f"_instrument_{name}")
+        Instruments_to_check = (
+            self._instrument_x,
+            self._instrument_y,
+            self._instrument_z,
+        )
+        for instrument in Instruments_to_check:
             if instrument.ramping_state() == "ramping":
                 msg = f"_set_fields aborted; magnet {instrument} is already ramping"
                 raise AMI430Exception(msg)
@@ -1175,18 +1205,31 @@ def pause(self) -> None:
         ):
             axis_instrument.pause()
 
+    def _update_setpoint_on_child_zero(self, instrument: AMIModel430) -> None:
+        """
+        Update the internal ``_set_point`` when a child instrument's
+        ``zero()`` method is called directly, so that the 3D driver's
+        setpoint tracking remains consistent.
+        """
+        if instrument is self._instrument_x:
+            self._set_point.set_component(x=0.0)
+        elif instrument is self._instrument_y:
+            self._set_point.set_component(y=0.0)
+        elif instrument is self._instrument_z:
+            self._set_point.set_component(z=0.0)
+
     def _request_field_change(self, instrument: AMIModel430, value: NumberType) -> None:
         """
         This method is called by the child x/y/z magnets if they are set
         individually. It results in additional safety checks being
         performed by this 3D driver.
         """
         if instrument is self._instrument_x:
-            self._set_x(value)
+            self._set_setpoints(("x",), (float(value),))
         elif instrument is self._instrument_y:
-            self._set_y(value)
+            self._set_setpoints(("y",), (float(value),))
         elif instrument is self._instrument_z:
-            self._set_z(value)
+            self._set_setpoints(("z",), (float(value),))
         else:
             msg = "This magnet doesnt belong to its specified parent {}"
             raise NameError(msg.format(self))