-
Notifications
You must be signed in to change notification settings - Fork 11
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
I19 hutch access device #1098
I19 hutch access device #1098
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1098 +/- ##
=======================================
Coverage 97.70% 97.71%
=======================================
Files 163 164 +1
Lines 6680 6690 +10
=======================================
+ Hits 6527 6537 +10
Misses 153 153 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Great, thanks. One comment, take it or leave it
class HutchAccessControl(StandardReadable): | ||
def __init__(self, prefix: str, name: str = "") -> None: | ||
self.active_hutch = epics_signal_r(str, f"{prefix}EHStatus.VALA") | ||
super().__init__(name) |
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.
Could: I think this is actually a good candidate for a declarative device (https://blueskyproject.io/ophyd-async/main/explanations/decisions/0009-procedural-vs-declarative-devices.html#decision)
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.
On a first try, doing this won't connect giving this error:
raise NotConnected(exceptions)
ophyd_async.core._utils.NotConnected:
access_control: NotConnected: pva://BL19I-OP-STAT-01:PVI
... which makes sense as BL19I-OP-STAT-01:PVI
doesn't exist.
I think I'm going to merge this PR as is and try to figure this out in a separate branch
Fixes #1097
Instructions to reviewer on how to test:
dodal connect i19-optics
Checks for reviewer
dodal connect ${BEAMLINE}