refactor(galil-rio): fix socket-signal cached readings

csaxs_bec/devices/omny/galil/galil_rio.py

@@ -26,7 +26,6 @@ from ophyd_devices.utils.socket import SocketIO
 from csaxs_bec.devices.omny.galil.galil_ophyd import (
     GalilCommunicationError,
     GalilSignalBase,
-    GalilSignalRO,
     retry_once,
 )
 
@@ -67,9 +66,10 @@ class GalilRIOController(Controller):
             )
 
 
-class GalilRIOSignal(GalilSignalBase):
+class GalilRIOAnalogSignalRO(GalilSignalBase):
     """
     Read-only Signal for reading a single analog input channel from the Galil RIO controller.
+    To make the readback more efficient, we will read all analog channels
     It always read all 8 analog channels at once, and updates the reabacks of all channels.
     New readbacks are only fetched from the controller if the last readback is older than
     _READ_TIMEOUT seconds, otherwise the last cached readback is returned to reduce network traffic.
@@ -80,74 +80,12 @@ class GalilRIOSignal(GalilSignalBase):
         parent (GalilRIO): Parent GalilRIO device.
     """
 
-    _READ_TIMEOUT = 0.1  # seconds
-
-    def __init__(self, signal_name: str, channel: int, parent: GalilRIO, **kwargs):
-        super().__init__(signal_name, parent=parent, **kwargs)
-        self._readback_metadata = (
-            self.root._readback_metadata
-            if hasattr(self.root, "_readback_metadata")
-            else {"last_readback": 0.0}
-        )
-        self._channel = channel
-        self._metadata["connected"] = False
-
-    def get(self):
-        """Get current analog channel values from the Galil RIO controller."""
-        # If the last readback has happend more than _READ_TIMEOUT seconds ago, read all channels again
-        if time.monotonic() - self._readback_metadata["last_readback"] > self._READ_TIMEOUT:
-            self._readback = self._socket_get()
-        return self._readback
-
-    # pylint: disable=protected-access
-    def _update_all_channels(self, values: list[float], signal_cls: Type[GalilRIOSignal]) -> None:
-        """
-        Update all analog channel readbacks based on the provided list of values.
-        List of values must be in order from an_ch0 to an_ch7.
-
-        We first have to update the timestamp of the GalilRIO _readback_metadata device.
-        Then we update all readbacks of all an_ch channels, before we run any subscriptions.
-        This ensures that all readbacks are updated before any subscriptions are run, which
-        may themselves read other channels.
-
-        Args:
-            values (list[float]): List of 8 float values corresponding to the analog channels.
-                                  They must be in order from an_ch0 to an_ch7.
-            signal_cls (Type[GalilRIOSignal]): The class of the signal to update, used to identify which signals to update.
-        """
-        timestamp = time.time()
-        # Update parent's last readback before running subscriptions!!
-        self._readback_metadata["last_readback"] = time.monotonic()
-        updates: dict[str, tuple[float, float]] = {}  # attr_name -> (new_val, old_val)
-        # Update all readbacks first
-        for walk in self.parent.walk_signals():
-            if isinstance(walk.item, signal_cls):
-                idx = int(walk.item.attr_name[-1])
-                if 0 <= idx < len(values):
-                    old_val = walk.item._readback
-                    new_val = values[idx]
-                    walk.item._metadata["timestamp"] = timestamp
-                    walk.item._readback = new_val
-                    updates[walk.item.attr_name] = (new_val, old_val)
-
-        # Run subscriptions after all readbacks have been updated
-        for walk in self.parent.walk_signals():
-            if walk.item.attr_name in updates:
-                new_val, old_val = updates[walk.item.attr_name]
-                walk.item._run_subs(
-                    sub_type=walk.item.SUB_VALUE,
-                    old_value=old_val,
-                    value=new_val,
-                    timestamp=timestamp,
-                )
-
-
-class GalilRIOSignalRO(GalilRIOSignal):
-
     _NUM_ANALOG_CHANNELS = 8
 
     def __init__(self, signal_name: str, channel: int, parent: GalilRIO, **kwargs):
         super().__init__(signal_name=signal_name, channel=channel, parent=parent, **kwargs)
+        self._channel = channel
+        self._metadata["connected"] = False
         self._metadata["write_access"] = False
 
     def _socket_set(self, val):
@@ -158,17 +96,65 @@ class GalilRIOSignalRO(GalilRIOSignal):
         cmd = "MG@" + ", @".join([f"AN[{ii}]" for ii in range(self._NUM_ANALOG_CHANNELS)])
         ret = self.controller.socket_put_and_receive(cmd)
         values = [float(val) for val in ret.strip().split(" ")]
-        # This updates all channels' readbacks, including self._readback
-        self._update_all_channels(values, signal_cls=GalilRIOSignalRO)
+        # This updates all channels' readbacks, including self._readback. Channels must be named following
+        # the convention ch0, ch1, ..., ch7 for this to work correctly.
+        self._update_all_channels(values)
         return self._readback
 
+    # pylint: disable=protected-access
+    def _update_all_channels(self, values: list[float]) -> None:
+        """
+        This method updates the readback values of all analog channels based on the list of values provided.
+        It also runs the subscriptions for each channel after updating the readbacks. It relies on
+        _last_readback being updated before calling _socket_get to ensure that timestamps are properly updated.
+        This convention is implemented in the SocketIO class. We futher rely on the convention that the channels
+        are named ch0, ch1, ..., ch7 for this to work correctly. Only GalilRIOAnalogSignalRO signals will be considered
+        in this update logic.
 
-class GalilRIODigitalOutSignal(GalilRIOSignal):  # We reuse the logic implemented for Galil
+        Args:
+            values (list[float]): List of new readback values for all channels, where the
+                index corresponds to the channel number (0-7).
+        """
+        updates: dict[str, tuple[float, float]] = {}  # attr_name -> (new_val, old_val)
+        # Update all readbacks first
+        for walk in self.parent.walk_signals():
+            if isinstance(walk.item, GalilRIOAnalogSignalRO):
+                idx = int(walk.item.attr_name[-1])
+                if 0 <= idx < len(values):
+                    old_val = walk.item._readback
+                    new_val = values[idx]
+                    walk.item._metadata["timestamp"] = self._last_readback
+                    walk.item._last_readback = self._last_readback
+                    walk.item._readback = new_val
+                    updates[walk.item.attr_name] = (new_val, old_val)
+                else:
+                    logger.warning(
+                        f"Received {len(values)} values but found channel index {idx} in signal {walk.item.name}. Skipping update for this signal."
+                    )
+
+        # Run subscriptions after all readbacks have been updated
+        for walk in self.parent.walk_signals():
+            if walk.item.attr_name in updates:
+                new_val, old_val = updates[walk.item.attr_name]
+                walk.item._run_subs(
+                    sub_type=walk.item.SUB_VALUE,
+                    old_value=old_val,
+                    value=new_val,
+                    timestamp=self._last_readback,
+                )
+
+
+class GalilRIODigitalOutSignal(GalilSignalBase):  # We reuse the logic implemented for Galil
     """
     Signal for controlling digital outputs of the Galil RIO controller.
     """
 
-    _NUM_DIGITAL_OUTPUT_CHANNELS = 24
+    _NUM_DIGITAL_OUTPUT_CHANNELS = 16
+
+    def __init__(self, signal_name: str, channel: int, parent: GalilRIO, **kwargs):
+        super().__init__(signal_name, parent=parent, **kwargs)
+        self._channel = channel
+        self._metadata["connected"] = False
 
     def _socket_get(self) -> float:
         """Get command for the readback signal"""
@@ -197,7 +183,7 @@ def _create_analog_channels(num_channels: int) -> dict[str, tuple]:
     an_channels = {}
     for i in range(0, num_channels):
         an_channels[f"ch{i}"] = (
-            GalilRIOSignalRO,
+            GalilRIOAnalogSignalRO,
             f"ch{i}",
             {
                 "kind": Kind.normal,
@@ -248,7 +234,7 @@ class GalilRIO(PSIDeviceBase):
     #############################
 
     analog_in = DDC(
-        _create_analog_channels(GalilRIOSignalRO._NUM_ANALOG_CHANNELS)
+        _create_analog_channels(GalilRIOAnalogSignalRO._NUM_ANALOG_CHANNELS)
     )  # Creates ch0 to ch7
     digital_out = DDC(
         _create_digital_output_channels(GalilRIODigitalOutSignal._NUM_DIGITAL_OUTPUT_CHANNELS)