651 energy change fixes #676

rtuck99 · 2024-11-26T11:07:02Z

This fixes energy change issues discovered during beamline testing #651

Instructions to reviewer on how to test:

Energy changes work on beamline
Unit tests pass

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

codecov · 2024-11-26T11:08:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.44%. Comparing base (06a635a) to head (63b4d6f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #676   +/-   ##
=======================================
  Coverage   86.44%   86.44%           
=======================================
  Files         102      102           
  Lines        6935     6935           
=======================================
  Hits         5995     5995           
  Misses        940      940

Components	Coverage Δ
i24 SSX	`71.94% <ø> (ø)`
hyperion	`96.84% <100.00%> (ø)`
other	`94.15% <ø> (ø)`

DominicOram

Great, thanks A few comments.

DominicOram · 2024-11-27T12:33:24Z

src/mx_bluesky/hyperion/device_setup_plans/dcm_pitch_roll_mirror_adjuster.py

    offset_mm = undulator_dcm.dcm_fixed_offset_mm
    LOGGER.info(f"Adjusting DCM offset to {offset_mm} mm")
-    yield from bps.abs_set(dcm.offset_in_mm, offset_mm, group=DCM_GROUP)
+    yield from bps.abs_set(dcm.offset_in_mm, offset_mm, wait=True)


Should: The offset is now being set in the undulator_dcm, why are we doing it again here?

That was something that got merged in from the beamline changes which I forgot to remove. I think it should be done here

Must: But, from memory, we needed that on the beamline because we needed the DCM energy to be set before we could set the DCM offset? So if we do this here we will need a wait for UNDULATOR_GROUP (which is poorly named as it is moving the DCM too) to finish first. I think that then costs us some time as we'll be ending up waiting on the undulator here too.

Should: Additionally, if we're going to do it here the wait=True is frustrating because AFAIK we don't need to wait on this to have finished before we start changing things on the mirrors as they're entirely separate devices.

General point: This is why I think we should push this further into the device, if we push it all the way into the DCM itself it's more obvious about what is the minimum thing we're waiting on.

DominicOram · 2024-11-27T12:40:29Z

src/mx_bluesky/hyperion/device_setup_plans/dcm_pitch_roll_mirror_adjuster.py

    yield from bps.trigger(mirror.apply_stripe)

+    # yaw, lat cannot be done simultaneously


Why can't yaw and lat be done simultaneously? They look like two separate motor PVs?

Yaw and lat are compound axes of the front and back motors, so moving one will cancel any move of the other

Great, thank, I've added this to the comment

DominicOram · 2024-11-27T12:43:26Z

src/mx_bluesky/hyperion/device_setup_plans/dcm_pitch_roll_mirror_adjuster.py

+    yield from bps.abs_set(
+        mirror.x_mm, mirror_config["lat_mm"], wait=True, timeout=YAW_LAT_TIMEOUT_S
+    )
+
+    LOGGER.info(f"Adjusting {mirror.name} yaw to {mirror_config['yaw_mrad']}")
+    yield from bps.abs_set(
+        mirror.yaw_mrad, mirror_config["yaw_mrad"], wait=True, timeout=YAW_LAT_TIMEOUT_S
+    )


Could: Rather than ask the device for it's settings based on the energy and then apply them in the plan could we just put it all into the device and allow us to do mirror.set(energy_kev) with the device handling the rest? This seems like it could be cleaner?

The yaw and lat are part of the dcm, they change because of the mirror stripe change which is the VFM and a separate device, unless we want to roll up the VFM into the undulator_dcm to make it vfm_undulator_dcm?

DominicOram · 2024-11-27T12:45:25Z

src/mx_bluesky/hyperion/device_setup_plans/dcm_pitch_roll_mirror_adjuster.py

-    # DCM Perp pitch
+    # DCM offset
+    # wait for other DCM motions and energy change to complete before changing offset
+    yield from bps.wait(DCM_GROUP, timeout=30)


Could: Similar to above, given the offest_mm is now being done in the undulator_dcm should we do all these moves in the device side to avoid the logic being split over so many places?

I don't think it should be done in the undulator_dcm, I'm beginning to think the opposite, that the undulator_dcm should just be a dumb device and everything be orchestrated in this plan.

I think it's cleaner to put all the logic for a particular device into the device so a plan just needs to do yield from bps.set(dcm, energy) and the dcm handles all of it's rolls/pitches etc. Especially as IMHO the asyncio syntax for ordering operations is cleaner than the wait=GROUP syntax. But I think either way that's outside of the scope here, sorry for bringing it up in the first place

DominicOram · 2024-11-27T12:46:39Z

src/mx_bluesky/hyperion/device_setup_plans/dcm_pitch_roll_mirror_adjuster.py

+
+    LOGGER.info(f"Adjusting {mirror.name} yaw to {mirror_config['yaw_mrad']}")
+    yield from bps.abs_set(
+        mirror.yaw_mrad, mirror_config["yaw_mrad"], wait=True, timeout=YAW_LAT_TIMEOUT_S


Should: It's misleading that the LAT_TIMEOUT is also being used on yaw

That's why is called YAW_LAT_TIMEOUT and not LAT_TIMEOUT

D'oh, sorry

DominicOram · 2024-11-27T12:49:49Z

src/mx_bluesky/hyperion/experiment_plans/robot_load_then_centre_plan.py

@@ -178,12 +182,23 @@ def robot_load_then_xray_centre(
            parameters,
        )
        LOGGER.info("Pin not loaded, loading and centring")


Nit: Unrelated to this PR, I know but it's odd this message is logged after the fact

DominicOram · 2024-11-27T12:53:12Z

src/mx_bluesky/hyperion/experiment_plans/robot_load_then_centre_plan.py

+            yield from set_energy_plan(
+                demand_energy_ev / 1000,


Could: Given we're now doing this same logic in the robot load (the only other place this plan is called) maybe we could just change set_energy_plan to take an optional in kev and move the if and /1000 into there?

Yes, I should have done this - fewer tests and simpler code!

DominicOram · 2024-11-27T12:56:06Z

src/mx_bluesky/hyperion/experiment_plans/load_centre_collect_full_plan.py

+    assert (
+        multi_rotation.demand_energy_ev
+        == parameters.robot_load_then_centre.demand_energy_ev
+    ), "Setting a different energy for gridscan and rotation is not supported"


Should: We should be able to support this quite easily. Can you make an issue for it?

DominicOram

Thanks. Still some comments, sorry.

DominicOram · 2024-11-29T15:39:01Z

src/mx_bluesky/hyperion/device_setup_plans/dcm_pitch_roll_mirror_adjuster.py

    yield from bps.trigger(mirror.apply_stripe)

+    # yaw, lat cannot be done simultaneously


Great, thank, I've added this to the comment

DominicOram · 2024-11-29T15:42:32Z

src/mx_bluesky/hyperion/device_setup_plans/dcm_pitch_roll_mirror_adjuster.py

    offset_mm = undulator_dcm.dcm_fixed_offset_mm
    LOGGER.info(f"Adjusting DCM offset to {offset_mm} mm")
-    yield from bps.abs_set(dcm.offset_in_mm, offset_mm, group=DCM_GROUP)
+    yield from bps.abs_set(dcm.offset_in_mm, offset_mm, wait=True)


Must: But, from memory, we needed that on the beamline because we needed the DCM energy to be set before we could set the DCM offset? So if we do this here we will need a wait for UNDULATOR_GROUP (which is poorly named as it is moving the DCM too) to finish first. I think that then costs us some time as we'll be ending up waiting on the undulator here too.

Should: Additionally, if we're going to do it here the wait=True is frustrating because AFAIK we don't need to wait on this to have finished before we start changing things on the mirrors as they're entirely separate devices.

General point: This is why I think we should push this further into the device, if we push it all the way into the DCM itself it's more obvious about what is the minimum thing we're waiting on.

DominicOram · 2024-11-29T15:49:04Z

src/mx_bluesky/hyperion/device_setup_plans/dcm_pitch_roll_mirror_adjuster.py

-    # DCM Perp pitch
+    # DCM offset
+    # wait for other DCM motions and energy change to complete before changing offset
+    yield from bps.wait(DCM_GROUP, timeout=30)


I think it's cleaner to put all the logic for a particular device into the device so a plan just needs to do yield from bps.set(dcm, energy) and the dcm handles all of it's rolls/pitches etc. Especially as IMHO the asyncio syntax for ordering operations is cleaner than the wait=GROUP syntax. But I think either way that's outside of the scope here, sorry for bringing it up in the first place

noemifrisina

lgtm, changes were addressed, please resolve the conflicts

…that were always constant

DO is currently on leave. Changes addressed

rtuck99 force-pushed the 651_energy_change_fixes branch from f1583b8 to 7f77e6b Compare November 26, 2024 11:21

rtuck99 mentioned this pull request Nov 26, 2024

MX Bluesky 651 fix energy changes DiamondLightSource/dodal#918

Merged

4 tasks

rtuck99 requested review from DominicOram and olliesilvester November 27, 2024 08:46

DominicOram requested changes Nov 27, 2024

View reviewed changes

rtuck99 mentioned this pull request Nov 28, 2024

Add support to load_centre_collect for doing gridscan at different energy to the rotation #686

Open

rtuck99 requested a review from DominicOram November 28, 2024 10:23

DominicOram previously requested changes Nov 29, 2024

View reviewed changes

noemifrisina requested changes Dec 2, 2024

View reviewed changes

rtuck99 added 8 commits December 2, 2024 15:30

(#651) Energy change fixes

bfbc0f1

Set vfm yaw, lat on energy change, remove changes from lookup tables …

cc20564

…that were always constant

Pin dodal hash

1394da3

Add unit test for voltage serial set, tidy up logging

8bb3eb4

Add set energy call when sample already loaded

d108eec

Tidy up test_robot_load_then_centre fixtures

d0f9634

Changes from PR review comments

4aae2c5

Move the dcm_offset set to the undulator_dcm

63b4d6f

rtuck99 force-pushed the 651_energy_change_fixes branch from a15adef to 63b4d6f Compare December 2, 2024 15:38

noemifrisina approved these changes Dec 2, 2024

View reviewed changes

rtuck99 merged commit 396940c into main Dec 2, 2024
22 checks passed

rtuck99 deleted the 651_energy_change_fixes branch December 2, 2024 15:52

rtuck99 mentioned this pull request Dec 3, 2024

Fix system tests for oav zoom controller and load_centre_collect params #695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

651 energy change fixes #676

651 energy change fixes #676

rtuck99 commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

DominicOram left a comment

DominicOram Nov 27, 2024

rtuck99 Nov 28, 2024

DominicOram Nov 29, 2024

DominicOram Nov 27, 2024

rtuck99 Nov 28, 2024

DominicOram Nov 29, 2024

DominicOram Nov 27, 2024

rtuck99 Nov 28, 2024

DominicOram Nov 27, 2024

rtuck99 Nov 28, 2024

DominicOram Nov 29, 2024

DominicOram Nov 27, 2024

rtuck99 Nov 28, 2024

DominicOram Nov 29, 2024

DominicOram Nov 27, 2024

DominicOram Nov 27, 2024

rtuck99 Nov 28, 2024

DominicOram Nov 27, 2024

rtuck99 Nov 28, 2024

DominicOram left a comment

DominicOram Nov 29, 2024

DominicOram Nov 29, 2024

DominicOram Nov 29, 2024

noemifrisina left a comment

		yield from bps.trigger(mirror.apply_stripe)

		# yaw, lat cannot be done simultaneously

651 energy change fixes #676

651 energy change fixes #676

Conversation

rtuck99 commented Nov 26, 2024 • edited Loading

Instructions to reviewer on how to test:

Checks for reviewer

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

DominicOram left a 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

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

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

DominicOram left a 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

noemifrisina left a comment

Choose a reason for hiding this comment

rtuck99 commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading