Merge pull request #1293 from ZLLentz/enh_tmo_mpod_apalis_hotfix

ENH/FIX: apply TMO's mpod apalis hotfixes, adding features and fixing a major bug

docs/source/upcoming_release_notes/1293-enh_tmo_mpod_apalis_hotfix.rst

@@ -0,0 +1,40 @@
+1293 enh_tmo_mpod_apalis_hotfix
+###############################
+
+API Breaks
+----------
+- N/A
+
+Library Features
+----------------
+- N/A
+
+Device Features
+---------------
+- Add the ``desc``, ``last_voltage_set``, and ``is_trip`` component signals to
+  `MPODApalisChannel`. These have been helpful during operations at TMO.
+  ``last_voltage_set`` will also get a ``voltage_setpoint`` alias, which is the
+  original name as used in TMO's scripts.
+- Add proper control limits to `MPODApalisChannel.voltage` and `MPODApalisChannel.current`.
+  This will give useful errors when someone tries to put values outside of the
+  channel's supported range.
+
+New Devices
+-----------
+- N/A
+
+Bugfixes
+--------
+- Fix an issue where arbitrarily large negative values were permitted to be
+  passed during the `MPODApalisChannel.set_voltage` method, and where
+  small values passed to a negative-polarity channel would jump to the
+  most negative value. Now, this function will clamp all values between
+  zero and the maximum channel voltage.
+
+Maintenance
+-----------
+- Added unit tests to cover the `MPODApalisChannel` changes.
+
+Contributors
+------------
+- zllentz

pcdsdevices/mpod_apalis.py

@@ -6,6 +6,7 @@
 from ophyd.signal import EpicsSignal, EpicsSignalRO
 
 from pcdsdevices.interface import BaseInterface
+from pcdsdevices.signal import EpicsSignalEditMD
 
 from .device import GroupDevice
 
@@ -18,22 +19,35 @@ class MPODApalisChannel(BaseInterface, Device):
 
     Parameters
     ----------
-    channel_prefix : str
+    prefix : str
         The EPICS base of the MPOD Channel, e.g. 'TMO:MPOD:01:M6:C6'.
-    name : str
+    name : str, keyword-only
         A name to refer to the device.
-
     """
-    voltage = Cpt(EpicsSignal, ':VoltageMeasure',
-                  write_pv=':VoltageSet', kind='normal',
-                  doc='MPOD Channel Voltage Measurement [V]')
+    voltage = Cpt(
+        EpicsSignalEditMD, ':VoltageMeasure',
+        write_pv=':VoltageSet', kind='normal', limits=True,
+        doc=(
+            "MPOD Channel Voltage Measurement [V]. "
+            "The value of this signal is the actual measured voltage "
+            "of the channel. If you put to this signal it will change "
+            "the channel's voltage setpoint."
+        ),
+    )
 
     max_voltage = Cpt(EpicsSignalRO, ':VoltageNominal', kind='normal',
                       doc='MPOD Channel Maximum Voltage [V]')
 
-    current = Cpt(EpicsSignal, ':CurrentMeasure',
-                  write_pv=':CurrentSet', kind='normal',
-                  doc='MPOD Channel Current Measure')
+    current = Cpt(
+        EpicsSignalEditMD, ':CurrentMeasure',
+        write_pv=':CurrentSet', kind='normal', limits=True,
+        doc=(
+            "MPOD Channel Current Measurement [A]. "
+            "The value of this signal is the actual measured current "
+            "of the channel. If you put to this signal it will change "
+            "the channel's current setpoint."
+        ),
+    )
 
     max_current = Cpt(EpicsSignalRO, ':CurrentNominal', kind='normal',
                       doc='MPOD Channel Current Maximum')
@@ -42,10 +56,32 @@ class MPODApalisChannel(BaseInterface, Device):
                 kind='normal', string=True,
                 doc='MPOD Channel State [Off/On]')
 
+    desc = Cpt(EpicsSignal, ':VoltageMeasure.DESC', kind='normal',
+               doc='MPOD Channel Description')
+
+    last_voltage_set = Cpt(
+        EpicsSignalRO, ':VoltageSet', kind='normal',
+        doc=(
+            'Readback to verify the MPOD Channel Voltage Setpoint [V]. '
+            'This is used to compare the measured readback voltage with '
+            'the last value we set to the channel. '
+            'To change the voltage, use the voltage signal or the '
+            'set_voltage helper method.'
+        )
+    )
+
+    is_trip = Cpt(EpicsSignalRO, ':isTrip', kind='omitted',
+                  doc='True if MPOD channel is tripped.')
+
     tab_component_names = True
     tab_whitelist = ['on', 'off',
                      'set_voltage', 'set_current']
 
+    @property
+    def voltage_setpoint(self) -> EpicsSignalRO:
+        """Name alias for backwards compatibility."""
+        return self.last_voltage_set
+
     def on(self):
         """Set mpod channel On."""
         self.state.put(1)
@@ -54,40 +90,76 @@ def off(self):
         """Set mpod channel Off."""
         self.state.put(0)
 
-    def set_voltage(self, voltage_number):
+    def set_voltage(self, voltage_number: float) -> None:
         """
         Set mpod channel voltage in V.
+
+        Values above or below the channel's range will be clamped
+        to the range.
+
         Parameters
         ----------
         voltage_number : number
             Voltage in V.
         """
-        max_voltage = self.max_voltage.get()
-
-        if voltage_number <= max_voltage:
-            self.voltage.put(voltage_number)
-        else:
-            self.voltage.put(max_voltage)
-            logger.warning('The maximal voltage is %g will set voltage to %g'
-                           % (max_voltage, max_voltage))
+        return _put_clamped(signal=self.voltage, value=voltage_number)
 
-    def set_current(self, current_number):
+    def set_current(self, current_number: float) -> None:
         """
         Set mpod channel current in A.
+
+        Values above or below the channel's range will be clamped
+        to the range.
+
         Parameters
         ----------
         current_number : number
             Current in A.
         """
+        return _put_clamped(signal=self.current, value=current_number)
+
+    @max_voltage.sub_value
+    def _new_max_voltage(self, value: float, **kwargs) -> None:
+        """Set explicit limits on voltage for better error reporting."""
+        bounds = (0, value)
+        self.voltage._override_metadata(
+            lower_ctrl_limit=min(bounds),
+            upper_ctrl_limit=max(bounds),
+        )
+
+    @max_current.sub_value
+    def _new_max_current(self, value: float, **kwargs) -> None:
+        """Set explicit limits on current for better error reporting."""
+        bounds = (0, value)
+        self.current._override_metadata(
+            lower_ctrl_limit=min(bounds),
+            upper_ctrl_limit=max(bounds),
+        )
+
+
+def _put_clamped(signal: EpicsSignal, value: float) -> None:
+    """
+    Force put value to be within the limits of the signal.
 
-        max_current = self.max_current.get()
-
-        if current_number <= max_current:
-            self.current.put(current_number)
-        else:
-            self.current.put(max_current)
-            logger.warning('The maximal current is %g will set current to %g'
-                           % (max_current, max_current))
+    Warn if the value is outside the range and needed to be clamped
+    """
+    low_val = min(signal.limits)
+    high_val = max(signal.limits)
+
+    def local_warn(alt_value: float):
+        logger.warning(
+            "Cannot put %g. The limits are %s, will set %s to %g",
+            value, signal.limits, signal.attr_name, alt_value,
+        )
+
+    if value < low_val:
+        local_warn(alt_value=low_val)
+        value = low_val
+    elif value > high_val:
+        local_warn(alt_value=high_val)
+        value = high_val
+
+    signal.put(value)
 
 
 class MPODApalisModule(BaseInterface, GroupDevice):

pcdsdevices/tests/test_mpod_apalis.py

@@ -2,6 +2,7 @@
 
 import pytest
 from ophyd.sim import make_fake_device
+from ophyd.utils import LimitError
 
 from ..device_types import MPODApalisModule4Channel
 from ..mpod_apalis import MPODApalisChannel, MPODApalisCrate
@@ -12,27 +13,40 @@
 @pytest.fixture(scope='function')
 def fake_mpod_channel1():
     FakeMPODChannel = make_fake_device(MPODApalisChannel)
-    C1 = FakeMPODChannel('TEST:MPOD:CHANNEL1', name='TestC1')
+    c1 = FakeMPODChannel('TEST:MPOD:CHANNEL1', name='TestC1')
     # switch off
-    C1.state.sim_put(0)
-    C1.voltage.sim_put(50)
-    C1.current.sim_put(0.02)
-    C1.max_voltage.sim_put(100)
-    C1.max_current.sim_put(0.1)
-    return C1
+    c1.state.sim_put(0)
+    c1.voltage.sim_put(50)
+    c1.current.sim_put(0.02)
+    c1.max_voltage.sim_put(100)
+    c1.max_current.sim_put(0.1)
+    return c1
 
 
 @pytest.fixture(scope='function')
 def fake_mpod_channel2():
     FakeMPODChannel = make_fake_device(MPODApalisChannel)
-    C2 = FakeMPODChannel('TEST:MPOD:CHANNEL2', name='TestC2')
+    c2 = FakeMPODChannel('TEST:MPOD:CHANNEL2', name='TestC2')
     # switch off
-    C2.state.sim_put(0)
-    C2.voltage.sim_put(90)
-    C2.current.sim_put(0.01)
-    C2.max_voltage.sim_put(100)
-    C2.max_current.sim_put(0.1)
-    return C2
+    c2.state.sim_put(0)
+    c2.voltage.sim_put(90)
+    c2.current.sim_put(0.01)
+    c2.max_voltage.sim_put(100)
+    c2.max_current.sim_put(0.1)
+    return c2
+
+
+@pytest.fixture(scope='function')
+def fake_mpod_channel_neg():
+    FakeMPODChannel = make_fake_device(MPODApalisChannel)
+    cn = FakeMPODChannel('TEST:MPOD:CHANNEL2', name='TestC2')
+    # switch off
+    cn.state.sim_put(0)
+    cn.voltage.sim_put(-90)
+    cn.current.sim_put(0.01)
+    cn.max_voltage.sim_put(-100)
+    cn.max_current.sim_put(0.1)
+    return cn
 
 
 @pytest.fixture(scope='function')
@@ -61,14 +75,42 @@ def test_switch_on_off(fake_mpod_channel1):
     assert fake_mpod_channel1.state.get() == '1'
 
 
-def test_set_voltage(fake_mpod_channel1, fake_mpod_channel2):
+def test_channel_limits(fake_mpod_channel1, fake_mpod_channel2, fake_mpod_channel_neg):
+    logger.debug('Testing MPOD Channel control limits')
+    assert fake_mpod_channel1.voltage.limits == (0, 100)
+    assert fake_mpod_channel1.current.limits == (0, 0.1)
+    with pytest.raises(LimitError):
+        fake_mpod_channel1.voltage.put(-1)
+    with pytest.raises(LimitError):
+        fake_mpod_channel1.voltage.put(150)
+    with pytest.raises(LimitError):
+        fake_mpod_channel1.current.put(-1)
+    with pytest.raises(LimitError):
+        fake_mpod_channel1.current.put(2)
+
+    assert fake_mpod_channel_neg.voltage.limits == (-100, 0)
+    with pytest.raises(LimitError):
+        fake_mpod_channel_neg.voltage.put(1)
+    with pytest.raises(LimitError):
+        fake_mpod_channel_neg.voltage.put(-150)
+
+
+def test_set_voltage(fake_mpod_channel1, fake_mpod_channel2, fake_mpod_channel_neg):
     logger.debug('Testing MPOD Channel Setting Voltage')
     assert fake_mpod_channel1.voltage.get() == 50
     fake_mpod_channel1.set_voltage(0)
     assert fake_mpod_channel1.voltage.get() == 0
     assert fake_mpod_channel2.voltage.get() == 90
     fake_mpod_channel2.set_voltage(140)
     assert fake_mpod_channel2.voltage.get() == 100
+    fake_mpod_channel2.set_voltage(-140)
+    assert fake_mpod_channel1.voltage.get() == 0
+    fake_mpod_channel_neg.set_voltage(-20)
+    assert fake_mpod_channel_neg.voltage.get() == -20
+    fake_mpod_channel_neg.set_voltage(20)
+    assert fake_mpod_channel_neg.voltage.get() == 0
+    fake_mpod_channel_neg.set_voltage(-2000000)
+    assert fake_mpod_channel_neg.voltage.get() == -100
 
 
 def test_set_current(fake_mpod_channel1, fake_mpod_channel2):
@@ -79,6 +121,8 @@ def test_set_current(fake_mpod_channel1, fake_mpod_channel2):
     assert fake_mpod_channel1.current.get() == 0.02
     fake_mpod_channel1.set_current(0.4)
     assert fake_mpod_channel1.current.get() == 0.1
+    fake_mpod_channel1.set_current(-0.4)
+    assert fake_mpod_channel1.current.get() == 0
 
 
 def test_clear_faults(fake_mpod_module4Channel):