-
Notifications
You must be signed in to change notification settings - Fork 9
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.
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
Add PVPositionerSoftDone #531
Conversation
I really want unit testing for this support but this project does not have an EPICS IOC available until #235. |
One way to apply a functional test (to evaluate this PR independently) is to configure one of the synApps userCalcs ( swait configuration details
In [6]: tsim = PVPositionerSoftDone("gp:userCalc8", name="tsim", readback_pv=".VAL", setpoint_pv=".B", tolerance=1.0)
In [7]: tsim.read()
Out[7]:
OrderedDict([('tsim',
{'value': 29.90983443961242, 'timestamp': 1632159680.426064}),
('tsim_setpoint',
{'value': 29.537445639734493, 'timestamp': 1632159674.42613})])
In [8]: tsim.move(25)
Out[8]: MoveStatus(done=True, pos=tsim, elapsed=2.1, success=True, settle_time=0.0)
In [9]: tsim.move(35)
Out[9]: MoveStatus(done=True, pos=tsim, elapsed=9.8, success=True, settle_time=0.0)
In [10]: tsim.move(25)
Out[10]: MoveStatus(done=True, pos=tsim, elapsed=9.4, success=True, settle_time=0.0) |
The functional test seems about right. Device reported comparable times to raise and lower the (simulated) temperature by the same amount to a fixed tolerance of 1 degree. Given the noise on this signal, a lower tolerance could invoke an indefinite wait. |
apstools/devices.py
Outdated
Motion tolerance. The motion is considered done if | ||
`abs(readback-setpoint) <= tolerance`. Defaults to `10^(-1*precision)`, | ||
where `precision = setpoint.precision` | ||
target_attr : str, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the target_attr
kwarg be refactored into an always-present attribute named target
which is initially set to None
? Means more logic changes below but less likely to be misconfigured (or means more easily understood) by end-user of this device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the 530-target
branch
Also, complexity of |
This pull request introduces 2 alerts when merging 486e7bd into 40180d1 - view on LGTM.com new alerts:
|
This is a candidate for unit testing with an EPICS IOC (#235). |
Needs to resolve merge conflicts with main branch. |
remove target_attr, keep target signal @gfabbris Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gfabbris Thanks for the contribution!
The
PVPositionerSoftDone
is basically the same asophyd.PVPositioner
that uses a soft (instead of EPICS) done signal. A few other (current) differences:where, for instance, the readback PV is "prefixPV:myreadbackPV". The advantage of using this is that the readback/setpoint signals are already marked as hinted/normal and forces auto monitor and PV completion. Alternatively, you can still do something like:
or
If not set, it will use
10^(-1*precision)
, where the setpoint precision is used.PVPositionerSoftDone
will recognize as a "done" motion before it's truly done. To fix this, you can define a dummytarget
signal:Closes #530.