From 40acf1e7491a2fd6e2a02b909d476e1d93d4265b Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Thu, 11 Jul 2024 14:41:58 -0500 Subject: [PATCH 01/12] update positioner_soft_done --- apstools/devices/positioner_soft_done.py | 94 +++++++++--------------- 1 file changed, 35 insertions(+), 59 deletions(-) diff --git a/apstools/devices/positioner_soft_done.py b/apstools/devices/positioner_soft_done.py index e290b04f..e6f5b8bb 100644 --- a/apstools/devices/positioner_soft_done.py +++ b/apstools/devices/positioner_soft_done.py @@ -31,36 +31,6 @@ TARGET_UNDEFINED = "undefined" -class _EpicsPositionerSetpointSignal(EpicsSignal): - """ - Special handling when PVPositionerSoftDone setpoint is changed. - - When the setpoint is changed, force`` done=False``. For any move, ``done`` - **must** transition to ``!= done_value``, then back to ``done_value``. - Without this response, a small move (within tolerance) will not return. - The ``cb_readback()`` method will compute ``done``. - """ - - def put(self, value, *args, **kwargs): - """Make sure 'done' signal goes False when setpoint is changed by us.""" - super().put(value, *args, **kwargs) - - self.parent.done.put(not self.parent.done_value) - if self.parent.update_target: - kwargs = {} - if issubclass(self.parent.target.__class__, EpicsSignalBase): - kwargs["wait"] = True # Signal.put() warns if kwargs are given - self.parent.target.put(value, **kwargs) - - # def get(self, *args, **kwargs): - # value = super().get(*args, **kwargs) - # if self.parent.update_target: - # target = self.parent.target.get() - # if target != TARGET_UNDEFINED: - # value = target - # return value - - class PVPositionerSoftDone(PVPositioner): """ PVPositioner that computes ``done`` as a soft signal. @@ -85,7 +55,7 @@ class PVPositionerSoftDone(PVPositioner): Defaults to ``10^(-1*precision)``, where ``precision = setpoint.precision``. update_target : bool - ``True`` when this object updates the ``target`` Component directly. + ``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``. @@ -128,7 +98,7 @@ class PVPositionerSoftDone(PVPositioner): EpicsSignalRO, "{prefix}{_readback_pv}", kind="hinted", auto_monitor=True ) setpoint = FormattedComponent( - _EpicsPositionerSetpointSignal, "{prefix}{_setpoint_pv}", kind="normal", put_complete=True + EpicsSignal, "{prefix}{_setpoint_pv}", kind="normal", put_complete=True ) # fmt: on done = Component(Signal, value=True, kind="config") @@ -139,6 +109,9 @@ class PVPositionerSoftDone(PVPositioner): target = Component(Signal, value=TARGET_UNDEFINED, kind="config") + _rb_count = 0 + _sp_count = 0 + def __init__( self, prefix="", @@ -169,6 +142,7 @@ def __init__( self.readback.subscribe(self.cb_readback) self.setpoint.subscribe(self.cb_setpoint) + self.setpoint.subscribe(self.cb_update_target) # cancel subscriptions before object is garbage collected weakref.finalize(self.readback, self.readback.unsubscribe_all) weakref.finalize(self.setpoint, self.setpoint.unsubscribe_all) @@ -192,6 +166,9 @@ def actual_tolerance(self): ) # fmt: on + def cb_update_target(self, value, *args, **kwargs): + self.target.put(value, wait=True) + def cb_readback(self, *args, **kwargs): """ Called when readback changes (EPICS CA monitor event) or on-demand. @@ -204,22 +181,30 @@ 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(): - logger.debug(f"{self.name} reached: {True}") + # if self.report_dmov_changes.get(): + # logger.debug(f"{self.name} reached: {True}") def cb_setpoint(self, *args, **kwargs): """ Called when setpoint changes (EPICS CA monitor event). - This method is called when the setpoint is changed by this code or from - some other EPICS client. + When the setpoint is changed, force`` done=False``. For any move, ``done`` + **must** transition to ``!= done_value``, then back to ``done_value``. - The 'done' signal is set to False in the custom - _EpicsPositionerSetpointSignal class. + Without this response, a small move (within tolerance) will not return. + The ``cb_readback()`` method will compute ``done``. + + Since other code will also call this method, check the keys in kwargs + and do not react to the "wrong" signature. """ - logger.debug("cb_setpoint: done=%s, setpoint=%s", self.done.get(), self.setpoint.get()) + 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()) @property def inposition(self): @@ -236,7 +221,7 @@ def inposition(self): sp = self.setpoint.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) + # logger.debug("inposition: inpos=%s rb=%s sp=%s tol=%s", inpos, rb, sp, tol) return inpos @property @@ -246,18 +231,20 @@ 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) - - # Write the setpoint value. + # TODO: The stuff in this if statement might not be necessary anymore. + # 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) - # The 'done' and 'target' signals are handled by - # the custom '_EpicsPositionerSetpointSignal' class. - 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) - - # Force the first check for done. - self.cb_readback() + # 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. class PVPositionerSoftDoneWithStop(PVPositionerSoftDone): @@ -279,14 +266,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: jemian@anl.gov -# :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. -# ----------------------------------------------------------------------------- From c77c5038164fde3ac573fcb73f1d4c323d517a9a Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Thu, 11 Jul 2024 14:47:37 -0500 Subject: [PATCH 02/12] remove unused import --- apstools/devices/positioner_soft_done.py | 1 - 1 file changed, 1 deletion(-) diff --git a/apstools/devices/positioner_soft_done.py b/apstools/devices/positioner_soft_done.py index e6f5b8bb..c07d1773 100644 --- a/apstools/devices/positioner_soft_done.py +++ b/apstools/devices/positioner_soft_done.py @@ -18,7 +18,6 @@ from ophyd import FormattedComponent from ophyd import PVPositioner from ophyd import Signal -from ophyd.signal import EpicsSignalBase # from ..tests import timed_pause From 029d3efd3c86328bf5405cbc57669b19cea5294a Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Thu, 11 Jul 2024 15:34:06 -0500 Subject: [PATCH 03/12] re-add logger --- apstools/devices/positioner_soft_done.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apstools/devices/positioner_soft_done.py b/apstools/devices/positioner_soft_done.py index c07d1773..b5191c35 100644 --- a/apstools/devices/positioner_soft_done.py +++ b/apstools/devices/positioner_soft_done.py @@ -184,8 +184,8 @@ def cb_readback(self, *args, **kwargs): if self.inposition: self.done.put(self.done_value) - # if self.report_dmov_changes.get(): - # logger.debug(f"{self.name} reached: {True}") + if self.report_dmov_changes.get(): + logger.debug(f"{self.name} reached: {True}") def cb_setpoint(self, *args, **kwargs): """ @@ -203,7 +203,7 @@ def cb_setpoint(self, *args, **kwargs): 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()) + logger.debug("cb_setpoint: done=%s, setpoint=%s", self.done.get(), self.setpoint.get()) @property def inposition(self): @@ -220,7 +220,7 @@ def inposition(self): sp = self.setpoint.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) + logger.debug("inposition: inpos=%s rb=%s sp=%s tol=%s", inpos, rb, sp, tol) return inpos @property From d0d46af62ea59e2f493d308c2171d2f21da5d282 Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Thu, 11 Jul 2024 17:24:33 -0500 Subject: [PATCH 04/12] tweaks and cleanup --- apstools/devices/positioner_soft_done.py | 27 +++++++----------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/apstools/devices/positioner_soft_done.py b/apstools/devices/positioner_soft_done.py index b5191c35..5c7c3154 100644 --- a/apstools/devices/positioner_soft_done.py +++ b/apstools/devices/positioner_soft_done.py @@ -53,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` @@ -108,9 +108,6 @@ class PVPositionerSoftDone(PVPositioner): target = Component(Signal, value=TARGET_UNDEFINED, kind="config") - _rb_count = 0 - _sp_count = 0 - def __init__( self, prefix="", @@ -118,7 +115,7 @@ def __init__( readback_pv="", setpoint_pv="", tolerance=None, - update_target=True, + use_target=False, **kwargs, ): # fmt: off @@ -137,11 +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) + 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) @@ -166,7 +164,7 @@ def actual_tolerance(self): # fmt: on def cb_update_target(self, value, *args, **kwargs): - self.target.put(value, wait=True) + self.target.put(value) def cb_readback(self, *args, **kwargs): """ @@ -217,7 +215,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) @@ -230,19 +228,10 @@ 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) - # TODO: The stuff in this if statement might not be necessary anymore. - # 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) 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. From 633802cd1a4046233ea58e4d51f0bd232e64c2ef Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Thu, 11 Jul 2024 17:34:03 -0500 Subject: [PATCH 05/12] update eurotherm to `use_target` --- apstools/devices/eurotherm_2216e.py | 2 +- apstools/devices/tests/test_eurotherm_2216e.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apstools/devices/eurotherm_2216e.py b/apstools/devices/eurotherm_2216e.py index 8afa6c77..c20c4c1f 100644 --- a/apstools/devices/eurotherm_2216e.py +++ b/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) diff --git a/apstools/devices/tests/test_eurotherm_2216e.py b/apstools/devices/tests/test_eurotherm_2216e.py index 59ceaa17..0941fad5 100644 --- a/apstools/devices/tests/test_eurotherm_2216e.py +++ b/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 = """ From 2b3d5cec35024dcadc0a7b108c7f6cb5785ab1f7 Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Thu, 11 Jul 2024 17:49:30 -0500 Subject: [PATCH 06/12] tweak test to use_target=True --- apstools/devices/tests/test_positioner_soft_done.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apstools/devices/tests/test_positioner_soft_done.py b/apstools/devices/tests/test_positioner_soft_done.py index 376ea7a3..51e926ce 100644 --- a/apstools/devices/tests/test_positioner_soft_done.py +++ b/apstools/devices/tests/test_positioner_soft_done.py @@ -36,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() From a05c3fce19c2fcf4abec8e9c3aa0aa7cb6d87b1c Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Thu, 11 Jul 2024 18:04:23 -0500 Subject: [PATCH 07/12] tweaks --- apstools/devices/positioner_soft_done.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apstools/devices/positioner_soft_done.py b/apstools/devices/positioner_soft_done.py index 5c7c3154..652135cc 100644 --- a/apstools/devices/positioner_soft_done.py +++ b/apstools/devices/positioner_soft_done.py @@ -200,7 +200,8 @@ def cb_setpoint(self, *args, **kwargs): """ if "value" in kwargs and "status" not in kwargs: self._sp_count += 1 - self.done.put(not self.done_value) + # self.done.put(not self.done_value) + self.done.put(self.inposition) logger.debug("cb_setpoint: done=%s, setpoint=%s", self.done.get(), self.setpoint.get()) @property From 4dc5dc36ff1091a718ec96a3d524256831750b40 Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Thu, 11 Jul 2024 18:16:50 -0500 Subject: [PATCH 08/12] debug --- apstools/devices/positioner_soft_done.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/apstools/devices/positioner_soft_done.py b/apstools/devices/positioner_soft_done.py index 652135cc..743833f8 100644 --- a/apstools/devices/positioner_soft_done.py +++ b/apstools/devices/positioner_soft_done.py @@ -178,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(): @@ -199,9 +197,7 @@ 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) - self.done.put(self.inposition) + self.done.put(not self.done_value) logger.debug("cb_setpoint: done=%s, setpoint=%s", self.done.get(), self.setpoint.get()) @property From 8bce07ed12077749fa6bd5846bb2dc51f436c507 Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Mon, 22 Jul 2024 11:14:26 -0500 Subject: [PATCH 09/12] Force done to be False in the start of the motion --- apstools/devices/positioner_soft_done.py | 1 + 1 file changed, 1 insertion(+) diff --git a/apstools/devices/positioner_soft_done.py b/apstools/devices/positioner_soft_done.py index 743833f8..666a8639 100644 --- a/apstools/devices/positioner_soft_done.py +++ b/apstools/devices/positioner_soft_done.py @@ -226,6 +226,7 @@ def _setup_move(self, position): """Move and do not wait until motion is complete (asynchronous)""" self.log.debug("%s.setpoint = %s", self.name, position) 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) From 0e32b4a7aea31feaee379ce854f0549f0bac1329 Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Mon, 22 Jul 2024 11:23:56 -0500 Subject: [PATCH 10/12] python 3.8 breaking test --- .github/workflows/code.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/code.yml b/.github/workflows/code.yml index 05cd7c69..a8e9ba46 100644 --- a/.github/workflows/code.yml +++ b/.github/workflows/code.yml @@ -98,7 +98,7 @@ jobs: strategy: matrix: python-version: - - "3.8" + # - "3.8" # TODO: Breaking - "3.9" - "3.10" - "3.11" From 4ef2ab5d92d915cbd620dca836d246d89383796e Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Mon, 22 Jul 2024 13:56:29 -0500 Subject: [PATCH 11/12] add use_target=True to lakeshores --- apstools/devices/lakeshore_controllers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apstools/devices/lakeshore_controllers.py b/apstools/devices/lakeshore_controllers.py index 86e587ac..aa387ff7 100644 --- a/apstools/devices/lakeshore_controllers.py +++ b/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 From 791232a0f379d631b72765007bf87c1ba45d3709 Mon Sep 17 00:00:00 2001 From: Gilberto Fabbris Date: Mon, 26 Aug 2024 16:43:34 -0500 Subject: [PATCH 12/12] retrigger checks