-
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
Settling times for the SR-570 Preamplifier #910
Conversation
By the way, this PR does add a lot of tests (~72), but they're parameterized and run in under a second. Hopefully that's okay. They basically test all the possible combinations of gain values. |
8b1c708
to
b9f6b87
Compare
Co-authored-by: Pete R Jemian <prjemian@users.noreply.github.com>
return super().set(value, timeout=timeout, settle_time=_settle_time) | ||
|
||
|
||
class GainSignal(GainMixin, EpicsSignal): |
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.
Is it necessary to separate the code of GainMixin from GainSignal? In addition to the use here, will the mixin be used somewhere else?
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.
It was originally going to be used in a pcdsdevices MultiDerivedSignal class. I re-factored it to no longer need it that way. Happy to re-combine the mixin with the signal.
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.
Better to recombine if you do not see an additional use.
All tests pass locally. Noting that $ git grep GainMixin
apstools/devices/srs570_preamplifier.py:class GainMixin:
apstools/devices/srs570_preamplifier.py:class GainSignal(GainMixin, EpicsSignal): |
|
||
Used to introduce a specific settle time when setting to account | ||
for the amp's R–C relaxation time when changing gain. | ||
|
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.
this blank line can be deleted
-------- | ||
Signal.set | ||
EpicsSignal.set | ||
|
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.
Is this blank line needed?
|
||
Used to introduce a specific settle time when setting to account | ||
for the amp's R–C relaxation time when changing gain. | ||
|
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.
Is this blank line needed?
8e82a97
to
dc5bdc2
Compare
I got rid of the GainMixin class. And also the _settle_time method since it's a static method and works just as well as a function. Assuming the tests pass, I think this is good? |
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.
LGTM! @canismarko - Please press the big green Merge button.
When changing gain on the SR-570 pre-amp, the voltage output spikes momentarily, and then settles down. Lower gain values settle faster. This PR adds a settling time to gain-related signals that depends on the target gain, based on measurements of the pre-amps at sector 25-ID-C. Presumably, other SR-570 preamps should be similar.
Fixes #906