Add DualFastShutter #1795
Add DualFastShutter #1795
Conversation
…Source/dodal into add_dual_fast_shutter
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1795 +/- ##
=======================================
Coverage 99.12% 99.13%
=======================================
Files 286 287 +1
Lines 10786 10862 +76
=======================================
+ Hits 10692 10768 +76
Misses 94 94 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Relm-Arrowny
left a comment
Relm-Arrowny
left a comment
There was a problem hiding this comment.
looks good to me, I have some comments/questions consider them all nits. Going through the code, I have a feeling the the shutter are not really fast shutter as they are open for a prolong period of time, for me fast shutter are those open only when the detector is ready and close as soon as detector is finish, but it probably a dissuasion for an other issue.
| # Should this be moved to a VGScientController only? | ||
| if self.shutter is not None: | ||
| await self.shutter.set(self.shutter.close_state) |
There was a problem hiding this comment.
I vote for yes, consider not every sub class has it than it is not base.
| if self.shutter is not None: | ||
| await self.shutter.set(self.shutter.open_state) | ||
|
|
There was a problem hiding this comment.
Question, is this want you want? Do you want to open the shutter during arm?
| def _validate_shutter_states(self, state1: EnumTypesT, state2: EnumTypesT) -> None: | ||
| if state1 is not state2: | ||
| raise ValueError( | ||
| f"{state1} is not same value as {state2}. They must be the same to be compatible." | ||
| ) |
There was a problem hiding this comment.
Is this really necessary, as long as they are GenericFastShutter They can only have two state open or close, does the underlining enum really needed to match?
Fixes #1615
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}