-
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
706 make undulator gap writeable i18 #721
Conversation
needs to be added to all of the undulator device instantiations |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
==========================================
- Coverage 94.91% 94.90% -0.01%
==========================================
Files 115 115
Lines 4664 4675 +11
==========================================
+ Hits 4427 4437 +10
- Misses 237 238 +1 ☔ View full report in Codecov by Sentry. |
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.
The undulator is used in i04_1
, i22
and p38
so we would need to add lookup paths there too.
The undulator was originally like this but @callumforrester changed it at #461 (comment) to "make the read-only case simpler". Do we still have a requirement for this to be read-only in some cases? Is this to do Adjust undulator gap to using current lookup table for initial Bragg angle
in DiamondLightSource/i18-bluesky#12 (comment)? In which case you could leave it in the undulator_dcm
and just set the undulator_dcm device to the current energy and it should correct the undulator with the correct gap?
9d2b708
to
30cb170
Compare
072d4ff
to
df41871
Compare
that is the not covered line. while I thought that this test @DominicOram perhaps you see something I don't about the behavior of the device here? |
Odd, I agree it looks like it's tested. I think it's fine to not worry about it. You're still at high coverage |
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. Minor points, mostly take them or leave them other than @DiamondJoseph's point that i22
/p38
still need a lookup table
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.
Sorry, meant request changes based on the i22 issue
0eea568
to
55cbb0b
Compare
9d3f0d4
to
6a39c89
Compare
178786d
to
8634f40
Compare
e61e1c1
to
1216791
Compare
c6a9e35
to
a18a373
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.
Thanks, one minor change, otherwise looks good.
src/dodal/devices/undulator.py
Outdated
await asyncio.gather( | ||
self._set_undulator_gap(value), | ||
) |
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: Why are you doing a gather
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.
must have been by mistake, just fixed this
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, feel free to merge then
Fixes #706
needed for i18 #710 and DiamondLightSource/i18-bluesky#4
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}