Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove target_attr, keep target signal #535

Merged
merged 4 commits into from
Oct 7, 2021
Merged

remove target_attr, keep target signal #535

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e58ebdf
MNT #530 remove target_attr, keep target signal
prjemian Sep 20, 2021
0241ba8
DOC #535
prjemian Sep 20, 2021
c760d6f
MNT #530 Should this class update target value?
prjemian Oct 5, 2021
a84b0ea
DOC #535
prjemian Oct 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions apstools/_devices/positioner_soft_done.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from ophyd import Signal
from ophyd import EpicsSignal
from ophyd import EpicsSignalRO
from ophyd.signal import EpicsSignalBase
import logging


Expand Down Expand Up @@ -37,10 +38,6 @@ class PVPositionerSoftDone(PVPositioner):

Defaults to ``10^(-1*precision)``,
where ``precision = setpoint.precision``.
target_attr : str, optional
Used if the setpoint is controlled incrementally by EPICS
(like with a ramp). Then a target attribute signal must be
defined, and its name passed in this variable.
kwargs :
Passed to `ophyd.PVPositioner`

Expand All @@ -59,6 +56,18 @@ class PVPositionerSoftDone(PVPositioner):
The stop PV to set when motion should be stopped
stop_value : any, optional
The value sent to stop_signal when a stop is requested
target : Signal
The target value of a move request.

Override (in subclass) with `EpicsSignal` to connect with a PV.

In some controllers (such as temperature controllers),
the setpoint may be changed incrementally
towards this target value (such as a ramp or controlled trajectory).
In such cases, the ``target`` will be final value while ``setpoint``
will be the current desired position.

Otherwise, both ``setpoint`` and ``target`` will be set to the same value.

(new in apstools 1.5.2)
"""
Expand All @@ -76,6 +85,8 @@ 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")

@property
def precision(self):
return self.setpoint.precision
Expand All @@ -84,7 +95,7 @@ def cb_readback(self, *args, **kwargs):
"""
Called when readback changes (EPICS CA monitor event).
"""
diff = self.readback.get() - self._target.get()
diff = self.readback.get() - self.setpoint.get()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work well for some devices in which the setpoint changes when a ramp is used. For example, this is the case for the lakeshore 340. So, it should be diff = self.readback.get() - self.target.get()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setpoint is the ultimate temperature where the controller is headed. The _target is the interim required value for the temperature control loop. (For example, a controller's PID loop will ramp _target as the immediate objective to calculate the following error: readback - _target.) This _target value may be changed incrementally (ramped) by the controller for a controlled transition from current to setpoint. We do not need to examine the controller's interim _target to determine if the controller has reached the requested temperature.

Definitely: done = |readback - setpoint| <= tolerance

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the Lakeshore 340 EPICS supports ramps the setpoint PV. Thus I think it's not great to call it setpoint in EPICS and target in Bluesky.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're saying the EPICS Lakeshore support uses setpoint for the interim setpoint of the ramp?
Doesn't the Lakeshore have a PV that reports where it is headed? That's the PV to be called setpoint in Bluesky to be consistent with how it is used in other positioners, such as EpicsMotor.

The PTC10 has both: a setpoint where the controller is headed (that is setpoint in bluesky) and a setpoint for the PID loop (that would target in bluesky but this interim goal is not interesting to the user).

An interesting case is when |target - setpoint| <= tolerance, that's when we expect to be close enough that |readback- setpoint| <= tolerance could become True.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would use this for more controllers than the Lakeshore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gfabbris Can you review again?

_tolerance = (
self.tolerance.get()
if self.tolerance.get() >= 0
Expand Down Expand Up @@ -112,7 +123,6 @@ def __init__(
readback_pv="",
setpoint_pv="",
tolerance=None,
target_attr="setpoint",
**kwargs,
):

Expand All @@ -121,8 +131,6 @@ def __init__(

super().__init__(prefix=prefix, **kwargs)

self._target = getattr(self, target_attr)

# Make the default alias for the readback the name of the
# positioner itself as in EpicsMotor.
self.readback.name = self.name
Expand All @@ -135,9 +143,11 @@ def __init__(
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._target.put(position, wait=True)
if self._target != self.setpoint:
self.setpoint.put(position, wait=True)
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)
Expand Down