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.

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

remove target_attr, keep target signal #535

Merged
merged 4 commits into from
Oct 7, 2021

Conversation

prjemian
Copy link
Contributor

@prjemian prjemian commented Sep 20, 2021

refactor (per #531) target_attr and _target into target, end user could override with named EpicsSignal if desired.

@prjemian prjemian added this to the 1.5.2 milestone Sep 20, 2021
@prjemian prjemian self-assigned this Sep 20, 2021
@prjemian
Copy link
Contributor Author

@gfabbris This is ready for your review.

@prjemian
Copy link
Contributor Author

@gfabbris This is the most important to review (for you) on this project at this time.

@prjemian prjemian modified the milestones: 1.5.2, 1.5.3 Sep 27, 2021
Copy link
Collaborator

@gfabbris gfabbris left a comment

Choose a reason for hiding this comment

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

Looks good to me, only one issue.

@@ -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?

@prjemian
Copy link
Contributor Author

prjemian commented Oct 4, 2021

Will need to resolve merge conflicts with upstream branch due to new content in main branch.

@prjemian
Copy link
Contributor Author

prjemian commented Oct 5, 2021

Upstream changes (from main branch) did not cause merge conflicts. Good.

@prjemian prjemian requested a review from gfabbris October 6, 2021 04:44
@prjemian
Copy link
Contributor Author

prjemian commented Oct 6, 2021

@gfabbris Ready for your review again.

@prjemian prjemian merged commit 0f43b5a into 530-PVPositionerSoftDone Oct 7, 2021
@prjemian prjemian deleted the 530-target branch October 7, 2021 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants