-
Notifications
You must be signed in to change notification settings - Fork 12
Add devices required for Hyperion beamstop check #1695
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1695 +/- ##
=======================================
Coverage 99.08% 99.09%
=======================================
Files 274 275 +1
Lines 10057 10132 +75
=======================================
+ Hits 9965 10040 +75
Misses 92 92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
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.
Thanks, mostly minor comments
src/dodal/beamlines/i03.py
Outdated
| """Get the i04 ipin device, instantiate it if it hasn't already been. | ||
| If this is called when already instantiated in i04, it will return the existing object. |
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.
Should: I know these docstrings are rubbish but this is i03, not i04
| """ | ||
|
|
||
| DATA_COLLECTION = "Data Collection" | ||
| OUT = "Out" |
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.
Nit: For the aperture/scatterguard we've used OUT_OF_BEAM might be good to use the same convention here
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.
Seems like i24 also has its own set of position enums, perhaps we should standardise them.
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.
Yh, I'd be up for a common set if we can think of ones that work.
| await self.z_mm.set(pos[2]) | ||
| await asyncio.gather( | ||
| self.x_mm.set(pos[0]), | ||
| self.y_mm.set(pos[1]), | ||
| ) |
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.
Should: Can we have a test that the order is obeyed. Should be able to do it by using the parent mock, something like test_aperture_scatterguard_select_bottom_moves_sg_down_then_assembly_up
02e7c86 to
bbe89ce
Compare
Add gain control to IPin device
Add
OUTposition to the beamstopRequired for
Instructions to reviewer on how to test:
dodal connect i03shows devices as connectedChecks for reviewer
dodal connect ${BEAMLINE}