-
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
[#207] Add VMXm fast grid scan devices #211
base: main
Are you sure you want to change the base?
Conversation
src/dodal/beamlines/vmxm.py
Outdated
return device_instantiation( | ||
device_factory=FastGridScan, | ||
name="fast_grid_scan", | ||
prefix="-MO-SAMP-11:FGS:", # TODO: currently needs Pxxxx prefixes in EPICS on VMXm, ask controls if we can alias to versions without these prefixes? |
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.
Have asked controls (Andy) about this, await response before deciding what to do with 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.
We also have an issue where the i03 FGS uses DWELL_TIME
and VMXm uses EXPOSURE_TIME
. This could warrant two different ophyd devices, but they should both have a standard exposure_time
signal, as that's more user friendly. i.e. if you do make two devices rename the signal on the i03 one
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.
I'll ask Andy whether this interface can be made the same as i03. It would be nice to avoid needing two separate ophyd devices for what is conceptually very similar/the same I think?
Or do you think long term we'll go towards separate ophyd devices anyway?
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.
Maybe we can create an "alias" in ophyd so that both work?
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.
I agree having one device is better. If we have to have two then they should be interchangeable at the plan level. Long term we'll end up getting rid of the FGS on i03 anyway
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main DiamondLightSource/hyperion#211 +/- ##
==========================================
- Coverage 88.13% 87.29% -0.84%
==========================================
Files 72 77 +5
Lines 2494 2637 +143
==========================================
+ Hits 2198 2302 +104
- Misses 296 335 +39 ☔ View full report in Codecov by Sentry. |
Does a system test run against the new devices? I think from a discussion we had in the office a while ago we decided on using the most user friendly name for the dodal module so this should be |
Not yet but in process of writing some
That's what I started with, but we've made assumptions in a few places in dodal that we can translate |
Need to add an |
93923f9
to
3908472
Compare
@@ -35,7 +36,9 @@ def set(self, value, *, timeout=None, settle_time=None, **kwargs): | |||
|
|||
STALE_PARAMS_TIMEOUT = 60 | |||
GENERAL_STATUS_TIMEOUT = 10 | |||
ALL_FRAMES_TIMEOUT = 120 | |||
ALL_FRAMES_TIMEOUT = ( | |||
600 # FIXME: VMXm hack - we need to get this passed in a a param probably? |
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.
Per code 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.
Alternatively/in addition, we could make this variable default to a certain value based on the current beamline
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.
It's really based on the experiment. It's how long do we expect the detector to be exposing for in total
sleep( | ||
2 | ||
) # FIXME - VMXm hack - on VMXm ODIN will *occasionally* fail to start without this sleep. Need a better solution. |
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.
We might be able to do some kind of cleverer wait for ODIN to be ready to start capturing. Needs careful testing on VMXm if we adjust this logic as it was only failing to start ~10% of gridscans before this change was added.
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.
Any idea what was causing it to fail when the sleep wasn't there?
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.
Not really. Some kind of race condition at the ODIN level, I think. 2 seconds is a number plucked from thin air and we might be able to lower it or put in some better logic, but it'll need testing on VMXm if we do that. Should be possible on a Tuesday or in a shutdown if you ask the scientists - we shouldn't need beam to test this.
For this issue, I'd maybe suggest doing some variant of:
if beamline == "vmxm":
sleep(2)
and write an issue to investigate and fix this properly.
from ophyd.epics_motor import MotorBundle | ||
|
||
|
||
class VmxmSampleMotors(MotorBundle): |
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.
We needed this to work around a bug/feature(?) of the VMXm motion system, we needed to manually resend omega in order to get the expected motion.
Add a comment along these lines?
status &= self.toggle.set("Off") | ||
else: | ||
status &= self.toggle.set("On") | ||
return status |
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.
This device is the same as I03's backlight other than the PV names. I think we should make a base Backlight
device where the PV names are parameters
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.
I agree, but in Ophyd V1 it's actually surprisingly tricky to do that due to some of the Component
magic - I know because I had the exact same thought as you and then tried it...
Suggest spinning that out into an issue if there isn't already one?
@@ -306,16 +316,20 @@ def _finish_arm(self) -> Status: | |||
|
|||
def forward_bit_depth_to_filewriter(self): | |||
bit_depth = self.bit_depth.get() | |||
self.odin.file_writer.data_type.put(f"UInt{bit_depth}") | |||
self.odin.file_writer.data_type.set(f"UInt{bit_depth}").wait( |
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.
Was this changed in attempt to fix the _wait_for_odin_status error?
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.
Yes, but I believe from a discussion with Dom on the beamline this is actually the "more correct" option anyway as it waits for a completion callback.
It wasn't sufficient to fix the issue on VMXm but I still think this is a good change to make.
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.
Yes, I agree this is more correct
These changes look good to me, it looks like there is only one thing which will break how things currently work if we merged. We need to:
And there are a few things I can make separate issues for which aren't as essential:
There's also the issue we already made about having variations of a device within a plan |
@DominicOram found this while scouring the issues for i22-related stuff |
Yes, I'm aware this is still open. The issue is on hold whilst we're working on the best solution that will work for VMXm and i03. Did you have a specific question about it? |
nothing specific, I just thought that Tom-Willemsen 's PR might have been lost |
Fixes #207
ixx
.