Merge pull request #954 from BCDA-APS/916-PVPositionerSoftDone-update

PVPositionerSoftDone should set done False at start of move

@MDecarabas, @gfabbris -- Thanks for the reviews and contributions!

CHANGES.rst

@@ -73,6 +73,7 @@ Fixes
 * lineup2() should work with low intensity peaks.
 * lineup2() would raise ZeroDivideError in some cases.
 * Increase minimum aps-dm-api version to 8.
+* PVPositionerSoftDone should set 'done' to False at start of a move.
 * Race condition with SR570 pre-amp.
 
 Maintenance

apstools/devices/eurotherm_2216e.py

@@ -83,7 +83,7 @@ def __init__(self, prefix="", *, tolerance=1, **kwargs):
             readback_pv="ignoreRBV",
             setpoint_pv="ignore",
             tolerance=tolerance,
-            update_target=False,
+            use_target=False,
             **kwargs,
         )
         self.sensor.subscribe(self.cb_sensor)

apstools/devices/lakeshore_controllers.py

@@ -68,7 +68,7 @@ class LakeShore336_LoopControl(PVPositionerSoftDoneWithStop):
 
     def __init__(self, *args, loop_number=None, timeout=10 * HOUR, **kwargs):
         self.loop_number = loop_number
-        super().__init__(*args, timeout=timeout, tolerance=0.1, readback_pv=f"IN{loop_number}", **kwargs)
+        super().__init__(*args, timeout=timeout, tolerance=0.1, use_target=True, readback_pv=f"IN{loop_number}", **kwargs)
         self._settle_time = 0
 
     @property
@@ -171,7 +171,7 @@ class LS340_LoopBase(PVPositionerSoftDoneWithStop):
 
     def __init__(self, *args, loop_number=None, timeout=10 * HOUR, **kwargs):
         self.loop_number = loop_number
-        super().__init__(*args, readback_pv="ignore", timeout=timeout, tolerance=0.1, **kwargs)
+        super().__init__(*args, readback_pv="ignore", timeout=timeout, use_target=True, tolerance=0.1, **kwargs)
         self._settle_time = 0
 
     @property

apstools/devices/positioner_soft_done.py

@@ -18,12 +18,17 @@
 from ophyd import FormattedComponent
 from ophyd import PVPositioner
 from ophyd import Signal
-from ophyd.signal import EpicsSignalBase
 
 # from ..tests import timed_pause
 
 logger = logging.getLogger(__name__)
 
+# Must use a data type that can be serialized as json (Python's None cannot be serialized)
+# This ValueError: Cannot determine the appropriate bluesky-friendly data type for value
+# None of Python type <class 'NoneType'>. Supported types include: int, float, str, and
+# iterables such as list, tuple, np.ndarray, and so on.
+TARGET_UNDEFINED = "undefined"
+
 
 class PVPositionerSoftDone(PVPositioner):
     """
@@ -48,11 +53,11 @@ class PVPositionerSoftDone(PVPositioner):
 
         Defaults to ``10^(-1*precision)``,
         where ``precision = setpoint.precision``.
-    update_target : bool
+    use_target : bool
         ``True`` when this object update the ``target`` Component directly.
         Use ``False`` if the ``target`` Component will be updated externally,
         such as by the controller when ``target`` is an ``EpicsSignal``.
-        Defaults to ``True``.
+        Defaults to ``False``.
     kwargs :
         Passed to `ophyd.PVPositioner`
 
@@ -101,10 +106,7 @@ class PVPositionerSoftDone(PVPositioner):
     tolerance = Component(Signal, value=-1, kind="config")
     report_dmov_changes = Component(Signal, value=False, kind="omitted")
 
-    target = Component(Signal, value="None", kind="config")
-
-    _rb_count = 0
-    _sp_count = 0
+    target = Component(Signal, value=TARGET_UNDEFINED, kind="config")
 
     def __init__(
         self,
@@ -113,7 +115,7 @@ def __init__(
         readback_pv="",
         setpoint_pv="",
         tolerance=None,
-        update_target=True,
+        use_target=False,
         **kwargs,
     ):
         # fmt: off
@@ -132,10 +134,12 @@ def __init__(
         # Make the default alias for the readback the name of the
         # positioner itself as in EpicsMotor.
         self.readback.name = self.name
-        self.update_target = update_target
+        self.use_target = use_target
 
         self.readback.subscribe(self.cb_readback)
         self.setpoint.subscribe(self.cb_setpoint)
+        self.setpoint.subscribe(self.cb_update_target, event_type="setpoint")
+
         # cancel subscriptions before object is garbage collected
         weakref.finalize(self.readback, self.readback.unsubscribe_all)
         weakref.finalize(self.setpoint, self.setpoint.unsubscribe_all)
@@ -159,6 +163,9 @@ def actual_tolerance(self):
         )
     # fmt: on
 
+    def cb_update_target(self, value, *args, **kwargs):
+        self.target.put(value)
+
     def cb_readback(self, *args, **kwargs):
         """
         Called when readback changes (EPICS CA monitor event) or on-demand.
@@ -171,8 +178,6 @@ def cb_readback(self, *args, **kwargs):
         if idle:
             return
 
-        self._rb_count += 1
-
         if self.inposition:
             self.done.put(self.done_value)
             if self.report_dmov_changes.get():
@@ -192,7 +197,6 @@ def cb_setpoint(self, *args, **kwargs):
         and do not react to the "wrong" signature.
         """
         if "value" in kwargs and "status" not in kwargs:
-            self._sp_count += 1
             self.done.put(not self.done_value)
         logger.debug("cb_setpoint: done=%s, setpoint=%s", self.done.get(), self.setpoint.get())
 
@@ -208,7 +212,7 @@ def inposition(self):
         # Since this method must execute quickly, do NOT force
         # EPICS CA gets using `use_monitor=False`.
         rb = self.readback.get()
-        sp = self.setpoint.get()
+        sp = self.setpoint.get() if self.use_target is False else self.target.get()
         tol = self.actual_tolerance
         inpos = math.isclose(rb, sp, abs_tol=tol)
         logger.debug("inposition: inpos=%s rb=%s sp=%s tol=%s", inpos, rb, sp, tol)
@@ -221,18 +225,11 @@ def precision(self):
     def _setup_move(self, position):
         """Move and do not wait until motion is complete (asynchronous)"""
         self.log.debug("%s.setpoint = %s", self.name, position)
-        if self.update_target:
-            kwargs = {}
-            if issubclass(self.target.__class__, EpicsSignalBase):
-                kwargs["wait"] = True  # Signal.put() warns if kwargs are given
-            self.target.put(position, **kwargs)
         self.setpoint.put(position, wait=True)
+        self.done.put(False)
         if self.actuate is not None:
             self.log.debug("%s.actuate = %s", self.name, self.actuate_value)
             self.actuate.put(self.actuate_value, wait=False)
-        # This is needed because in a special case the setpoint.put does not
-        # run the "sub_value" subscriptions.
-        self.cb_setpoint()
         self.cb_readback()  # This is needed to force the first check.
 
 
@@ -255,14 +252,3 @@ def stop(self, *, success=False):
             self.setpoint.put(self.position)
             time.sleep(2.0 / 60)  # two clock ticks, allow for EPICS record processing
             self.cb_readback()  # re-evaluate soft done Signal
-
-
-# -----------------------------------------------------------------------------
-# :author:    Pete R. Jemian
-# :email:     [email protected]
-# :copyright: (c) 2017-2024, UChicago Argonne, LLC
-#
-# Distributed under the terms of the Argonne National Laboratory Open Source License.
-#
-# The full license is in the file LICENSE.txt, distributed with this software.
-# -----------------------------------------------------------------------------

apstools/devices/tests/test_eurotherm_2216e.py

@@ -15,7 +15,7 @@ def test_device():
     assert not euro.connected
 
     assert euro.tolerance.get() == 1
-    assert euro.update_target is False
+    assert euro.use_target is False
     assert euro.target is None
 
     cns = """

apstools/devices/tests/test_lakeshores.py

@@ -7,6 +7,7 @@
 from ...tests import IOC_GP
 from ..lakeshore_controllers import LakeShore336Device
 from ..lakeshore_controllers import LakeShore340Device
+from ..positioner_soft_done import TARGET_UNDEFINED
 
 PV_PREFIX = f"phony:{IOC_GP}lakeshore:"
 
@@ -16,8 +17,8 @@ def test_lakeshore_336():
     assert not t336.connected
 
     # Signal components
-    assert t336.loop1.target.get() == "None"
-    assert t336.loop2.target.get() == "None"
+    assert t336.loop1.target.get() == TARGET_UNDEFINED
+    assert t336.loop2.target.get() == TARGET_UNDEFINED
 
     assert t336.loop1.tolerance.get() == 0.1
     assert t336.loop2.tolerance.get() == 0.1
@@ -68,8 +69,8 @@ def test_lakeshore_340():
     assert not t340.connected
 
     # Signal components
-    assert t340.control.target.get() == "None"
-    assert t340.sample.target.get() == "None"
+    assert t340.control.target.get() == TARGET_UNDEFINED
+    assert t340.sample.target.get() == TARGET_UNDEFINED
 
     assert t340.control.tolerance.get() == 0.1
     assert t340.sample.tolerance.get() == 0.1

apstools/devices/tests/test_positioner_soft_done.py

@@ -13,6 +13,7 @@
 from ...utils import run_in_thread
 from ..positioner_soft_done import PVPositionerSoftDone
 from ..positioner_soft_done import PVPositionerSoftDoneWithStop
+from ..positioner_soft_done import TARGET_UNDEFINED
 
 PV_PREFIX = f"{IOC_GP}gp:"
 delay_active = False
@@ -35,7 +36,7 @@ def pos():
     """Test Positioner based on two analogout PVs."""
     # fmt: off
     pos = PVPositionerSoftDoneWithStop(
-        PV_PREFIX, readback_pv="float1", setpoint_pv="float2", name="pos"
+        PV_PREFIX, readback_pv="float1", setpoint_pv="float2", use_target=True, name="pos"
     )
     # fmt: on
     pos.wait_for_connection()
@@ -93,9 +94,8 @@ def confirm_in_position(p, dt):
     p.readback.get(use_monitor=False)  # force a read from the IOC
     p.cb_readback()  # update self.done
 
-    # collect these values at one instant (as close as possible).
-    c_rb = p._rb_count  # do not expect any new callbacks during this method
-    c_sp = p._sp_count
+    # Collect these values at one instant (as close in time as possible).
+    # Do not expect any new callbacks during this function.
     dmov = p.done.get()
     rb = p.readback.get()
     sp = p.setpoint.get()
@@ -106,18 +106,13 @@ def confirm_in_position(p, dt):
         f"{p.name=}"
         f"  {rb=:.5f} {sp=:.5f} {tol=}"
         f"  {dt=:.4f}s"
-        f"  {p._sp_count=}"
-        f"  {p._rb_count=}"
         f"  {p.done=}"
         f"  {p.done_value=}"
         f"  {time.time()=:.4f}"
     )
     # fmt: on
 
-    assert p._rb_count == c_rb, diagnostics
-    assert p._sp_count == c_sp, diagnostics
     assert dmov == p.done_value, diagnostics
-    # assert math.isclose(rb, sp, abs_tol=tol), diagnostics
 
 
 @run_in_thread
@@ -202,20 +197,17 @@ def test_structure(device, has_inposition):
     assert pos.setpoint.pvname == "v"
     assert pos.done.get() is True
     assert pos.done_value is True
-    assert pos.target.get() == "None"
+    assert pos.target.get() == TARGET_UNDEFINED
     assert pos.tolerance.get() == -1
 
 
-@pytest.mark.local
 def test_put_and_stop(rbv, prec, pos):
     assert pos.tolerance.get() == -1
     assert pos.precision == prec.get()
 
     def motion(rb_initial, target, rb_mid=None):
         rbv.put(rb_initial)  # make the readback to different
-        c_sp = pos._sp_count
         pos.setpoint.put(target)
-        assert pos._sp_count == c_sp + 1
         assert math.isclose(pos.readback.get(use_monitor=False), rb_initial, abs_tol=0.02)
         assert math.isclose(pos.setpoint.get(use_monitor=False), target, abs_tol=0.02)
         assert pos.done.get() != pos.done_value
@@ -248,7 +240,6 @@ def motion(rb_initial, target, rb_mid=None):
     motion(1, 0, 0.5)  # interrupted move
 
 
-@pytest.mark.local
 def test_move_and_stop_nonzero(rbv, pos):
     timed_pause()
 
@@ -271,9 +262,14 @@ def test_move_and_stop_nonzero(rbv, pos):
     assert pos.inposition
 
 
-@pytest.mark.local
 def test_move_and_stopped_early(rbv, pos):
     def motion(target, delay, interrupt=False):
+        """
+        Test moving pos to target.  Update rbv after delay.
+
+        If interrupt is True, stop the move before it is done
+        (at a time that is less than the 'delay' value).
+        """
         timed_pause(0.1)  # allow previous activities to settle down
 
         t0 = time.time()
@@ -286,8 +282,7 @@ def motion(target, delay, interrupt=False):
         rb_new = pos.readback.get(use_monitor=False)
         arrived = math.isclose(rb_new, target, abs_tol=pos.actual_tolerance)
         # fmt: on
-        if interrupt:
-            assert not status.done
+        if interrupt and not status.done:
             assert not status.success
             assert not arrived, f"{dt=:.3f}"
             pos.stop()
@@ -350,11 +345,9 @@ def test_position_sequence_calcpos(target, calcpos):
     def motion(p, goal):
         timed_pause(0.1)  # allow previous activities to settle down
 
-        c_sp = p._sp_count
         t0 = time.time()
         status = p.move(goal)
         dt = time.time() - t0
-        assert p._sp_count == c_sp + 1
         assert status.elapsed > 0, str(status)
         assert status.done, str(status)