[#207] Add VMXm fast grid scan devices #211

Tom-Willemsen · 2023-10-17T16:15:32Z

Fixes #207

Adds VMXm devices
Makes some of our infrastructure more generic for dealing with beamline names that don't look like ixx.

Tom-Willemsen · 2023-10-17T16:15:59Z

src/dodal/beamlines/vmxm.py

+    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?


Have asked controls (Andy) about this, await response before deciding what to do with this

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

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?

Maybe we can create an "alias" in ophyd so that both work?

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 · 2023-10-17T16:16:32Z

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (e159386) 88.13% compared to head (a869668) 87.29%.
Report is 43 commits behind head on main.

❗ Current head a869668 differs from pull request most recent head 76c6391. Consider uploading reports for the commit 76c6391 to get more accurate results

Files	Patch %	Lines
src/dodal/devices/fast_grid_scan_2d.py	64.81%	19 Missing ⚠️
src/dodal/devices/vmxm/vmxm_attenuator.py	61.53%	10 Missing ⚠️
src/dodal/beamlines/vmxm.py	87.80%	5 Missing ⚠️
src/dodal/devices/backlight.py	54.54%	5 Missing ⚠️
src/dodal/devices/fast_grid_scan_common.py	98.97%	1 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

DominicOram · 2023-10-18T11:14:29Z

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 vmxm (I appreciate this should be documented)

Tom-Willemsen · 2023-10-18T11:48:34Z

Does a system test run against the new devices

Not yet but in process of writing some

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 vmxm

That's what I started with, but we've made assumptions in a few places in dodal that we can translate BEAMLINE env var directly into prefixes (e.g. BeamlinePrefix class) and module names (this one is my fault; get_beamline_based_on_environment_variable). Have made BeamlinePrefix more generic to accomodate. ~~Still need to look at get_beamline.~~

Tom-Willemsen · 2023-10-23T13:42:42Z

Need to add an Attenuator device - but note that this looks very different from i03's attenuator (looking at the EDM screens).

src/dodal/beamlines/vmxm.py

src/dodal/devices/backlight.py

src/dodal/devices/detector.py

src/dodal/devices/eiger.py

src/dodal/devices/fast_grid_scan.py

src/dodal/devices/utils.py

src/dodal/devices/vmxm/vmxm_attenuator.py

src/dodal/utils.py

…an_devices

Tom-Willemsen · 2023-11-24T10:43:31Z

src/dodal/devices/eiger.py

@@ -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?


Per code comment.

Alternatively/in addition, we could make this variable default to a certain value based on the current beamline

It's really based on the experiment. It's how long do we expect the detector to be exposing for in total

Tom-Willemsen · 2023-11-24T10:45:02Z

src/dodal/devices/eiger.py

+        sleep(
+            2
+        )  # FIXME - VMXm hack - on VMXm ODIN will *occasionally* fail to start without this sleep. Need a better solution.


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.

Any idea what was causing it to fail when the sleep wasn't there?

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.

Tom-Willemsen · 2023-11-24T10:47:34Z

src/dodal/devices/vmxm/vmxm_sample_motors.py

+from ophyd.epics_motor import MotorBundle
+
+
+class VmxmSampleMotors(MotorBundle):


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?

olliesilvester · 2023-11-27T16:11:24Z

src/dodal/devices/backlight.py

+            status &= self.toggle.set("Off")
+        else:
+            status &= self.toggle.set("On")
+        return status


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

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?

olliesilvester · 2023-11-27T16:24:35Z

src/dodal/devices/eiger.py

@@ -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(


Was this changed in attempt to fix the _wait_for_odin_status error?

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.

Yes, I agree this is more correct

olliesilvester · 2023-12-04T12:52:05Z

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:

Move ALL_FRAMES_TIMEOUT in eiger.py into some parameter. If this is based off the total exposure time, this value could perhaps be calculated from other grid scan parameters? - Parameterise ALL_FRAMES_TIMEOUT in eiger #256

And there are a few things I can make separate issues for which aren't as essential:

Making a BaseBackLight rather than two very similar versions for i03 and vmxm - maybe this is easier with opyhd-async devices? I'll make an issue for this once this PR is merged.
Moving all the common bits of code to move into fast_grid_scan_common.py Again, I'll make an issue after this is merged.
Find out why we need to manually resend omega in order to get expected motion (in vmxm_sample_motors.py)

There's also the issue we already made about having variations of a device within a plan

Hyperion should allow for variations of a device to be used in a plan mx-bluesky#320

stan-dot · 2024-07-09T09:54:53Z

@DominicOram found this while scouring the issues for i22-related stuff

DominicOram · 2024-07-09T10:32:12Z

@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?

stan-dot · 2024-07-09T11:04:06Z

nothing specific, I just thought that Tom-Willemsen 's PR might have been lost

[#207] Add VMXm fast grid scan devices

a516675

Tom-Willemsen commented Oct 17, 2023

View reviewed changes

Move to i02-1 rather than VMXm

2380dfd

Tom Willemsen added 3 commits October 18, 2023 16:27

Fix unit tests; make set_beamline more generic; add VMXm system test

8cbc6e9

lint

335ef4a

Return correct beamline module on VMXm

900467a

Add VMXm attenuator

75e5928

Tom-Willemsen mentioned this pull request Oct 23, 2023

Initial VMXm FGS plan DiamondLightSource/hyperion#942

Draft

VMXM - changes on beamline

429eb1d

Tom-Willemsen commented Nov 3, 2023

View reviewed changes

Tom Willemsen added 3 commits November 7, 2023 14:51

VMXm fast grid scans: Dodal tidy-ups

9adc9ee

Merge remote-tracking branch 'origin/main' into 207_vmxm_fast_grid_sc…

d96e13d

…an_devices

Correct merge

3908472

Tom-Willemsen force-pushed the 207_vmxm_fast_grid_scan_devices branch from 93923f9 to 3908472 Compare November 10, 2023 15:13

Tom Willemsen added 2 commits November 17, 2023 09:30

Add VMXm samp motors

a869668

Changes from VMXm beamtime

76c6391

Tom-Willemsen commented Nov 24, 2023

View reviewed changes

olliesilvester reviewed Nov 27, 2023

View reviewed changes

stan-dot added the i22 label Aug 19, 2024

Tom-Willemsen mentioned this pull request Oct 24, 2023

VMXm: fast grid scans DiamondLightSource/mx-bluesky#323

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#207] Add VMXm fast grid scan devices #211

[#207] Add VMXm fast grid scan devices #211

Tom-Willemsen commented Oct 17, 2023 •

edited

Loading

Tom-Willemsen Oct 17, 2023

DominicOram Oct 18, 2023 •

edited

Loading

Tom-Willemsen Oct 18, 2023

Tom-Willemsen Oct 18, 2023

DominicOram Oct 23, 2023

codecov bot commented Oct 17, 2023 •

edited

Loading

DominicOram commented Oct 18, 2023

Tom-Willemsen commented Oct 18, 2023 •

edited

Loading

Tom-Willemsen commented Oct 23, 2023

Tom-Willemsen Nov 24, 2023

olliesilvester Nov 27, 2023

DominicOram Nov 27, 2023

Tom-Willemsen Nov 24, 2023

olliesilvester Nov 27, 2023

Tom-Willemsen Nov 28, 2023

Tom-Willemsen Nov 24, 2023

olliesilvester Nov 27, 2023 •

edited

Loading

Tom-Willemsen Nov 28, 2023 •

edited

Loading

olliesilvester Nov 27, 2023

Tom-Willemsen Nov 28, 2023

DominicOram Nov 28, 2023

olliesilvester commented Dec 4, 2023 •

edited

Loading

stan-dot commented Jul 9, 2024

DominicOram commented Jul 9, 2024

stan-dot commented Jul 9, 2024

		from ophyd.epics_motor import MotorBundle


		class VmxmSampleMotors(MotorBundle):

[#207] Add VMXm fast grid scan devices #211

Are you sure you want to change the base?

[#207] Add VMXm fast grid scan devices #211

Conversation

Tom-Willemsen commented Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

DominicOram Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

DominicOram commented Oct 18, 2023

Tom-Willemsen commented Oct 18, 2023 • edited Loading

Tom-Willemsen commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliesilvester Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Tom-Willemsen Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliesilvester commented Dec 4, 2023 • edited Loading

stan-dot commented Jul 9, 2024

DominicOram commented Jul 9, 2024

stan-dot commented Jul 9, 2024

Tom-Willemsen commented Oct 17, 2023 •

edited

Loading

DominicOram Oct 18, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

Tom-Willemsen commented Oct 18, 2023 •

edited

Loading

olliesilvester Nov 27, 2023 •

edited

Loading

Tom-Willemsen Nov 28, 2023 •

edited

Loading

olliesilvester commented Dec 4, 2023 •

edited

Loading