-
Notifications
You must be signed in to change notification settings - Fork 8
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
658 create device for pressure jump cell #673
base: main
Are you sure you want to change the base?
Conversation
92f4c0a
to
0d07c25
Compare
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.
At the moment it's implemented as one large object hard to debug, should be split into components
0d07c25
to
803b90a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #673 +/- ##
==========================================
+ Coverage 94.46% 94.47% +0.01%
==========================================
Files 115 116 +1
Lines 4604 4726 +122
==========================================
+ Hits 4349 4465 +116
- Misses 255 261 +6 ☔ View full report in Codecov by Sentry. |
07522bb
to
431887c
Compare
self.pump_forward_limit = epics_signal_r( | ||
LimitSwitchState, prefix + "D74IN1" | ||
) | ||
self.pump_backward_limit = epics_signal_r( | ||
LimitSwitchState, prefix + "D74IN0" | ||
) |
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.
Are the limits settable and required to be Read signals or are they read-once Config signals?
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.
how to check that?
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.
They're epics_signal_r
read only from the ophyd layer, which means they're read-only from the ophyd layer. They could be derived from other signals, and therefore changing on the rate of every point of a scan, but I would infer from the name that they are hardware limits that don't change over the lifetime of the device, let alone within a scan. I would make them config signals.
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.
These signals are the state of limit switches for the pump so they will change during a scan if the pump is driven for a some length of time (approx 50s) in one direction. The name could be improved to pump_..._limit_state
7af5a66
to
a6b8c74
Compare
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.
some small bits
36c2344
to
a91272f
Compare
self.pump_forward_limit = epics_signal_r( | ||
LimitSwitchState, prefix + "D74IN1" | ||
) | ||
self.pump_backward_limit = epics_signal_r( | ||
LimitSwitchState, prefix + "D74IN0" | ||
) |
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.
They're epics_signal_r
read only from the ophyd layer, which means they're read-only from the ophyd layer. They could be derived from other signals, and therefore changing on the rate of every point of a scan, but I would infer from the name that they are hardware limits that don't change over the lifetime of the device, let alone within a scan. I would make them config signals.
1ce82ad
to
7567b94
Compare
15996f2
to
4a7b253
Compare
… and pressure jumps.
…s and formatting.
… added unit test for readback values.
4a7b253
to
cadd4d2
Compare
closes #658
Ophyd device for pressure jump cell #658, required for pressure jump experiment.
Initial control PVs, TODO fast adc capture.
Checks for reviewer
dodal connect ${BEAMLINE}