Mx bluesky 1354 scintillator safe move by rtuck99 · Pull Request #1819 · DiamondLightSource/dodal

rtuck99 · 2026-01-15T12:12:49Z

Fixes

Instructions to reviewer on how to test:

The aperture scatterguard is now moved sideways to the corresponding SCIN_MOVE position for its current position and the position restored on successful scintillator move.
Beamstop position is checked that it is either in data collection or out-of-beam position, any other unrecognised or unsupported position prevents a scintillator move.

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 · 2026-01-15T12:16:31Z

Codecov Report

❌ Patch coverage is 97.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.12%. Comparing base (07c1416) to head (a2e9c3f).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
src/dodal/devices/mx_phase1/scintillator.py	95.83%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1819      +/-   ##
==========================================
- Coverage   99.14%   99.12%   -0.03%     
==========================================
  Files         304      305       +1     
  Lines       11509    11571      +62     
==========================================
+ Hits        11411    11470      +59     
- Misses         98      101       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

olliesilvester

Thanks, left some comments which are mostly trying to make it easier to read.

olliesilvester · 2026-01-20T13:59:40Z

docs/reference/aperture-scatterguard-collisions.md

+moved in / out (scintillator only has motion in the y, z axes).
+
+```{raw} html
+:file: ../images/scintillator-motion.svg


Nice diagram, but it's hard to see if using dark mode on browser. Could you put it over a white background instead of a transparent background?

Could: Add an 'x' annotation for the x axis and label the scintillator, aperture and scatterguard

Discussed in person: I think we should remove the parts of this diagram which aren't necessary to understand the collision risk, eg the block to the left of the scatterguard. Also I think the scintillator should be moved +1 in in the x direction

olliesilvester · 2026-01-20T14:11:05Z

src/dodal/devices/aperturescatterguard.py

+    scintillator_move_miniap_x: float
+    scintillator_move_sg_x: float


nit: sg_x may not mean scatterguard to a new person - maybe just add a comment?

src/dodal/devices/scintillator.py

olliesilvester · 2026-01-20T14:25:34Z

src/dodal/devices/mx_phase1/scintillator.py

+                return
+            case _:
+                raise ValueError(
+                    f"Scintillator cannot be moved due to beamstop position {position}, must be in either in DATA_COLLECTION or OUT_OF_BEAM position."


Our scintillator and aperture scatterguard devices are currently in the "common device" section of dodal. Are we sure that the collision checks here are valid for a generic beamline? Maybe @DominicOram has some idea?

olliesilvester · 2026-01-20T14:25:49Z

src/dodal/devices/mx_phase1/scintillator.py

+                return
+            case _:
+                raise ValueError(
+                    f"Scintillator cannot be moved due to beamstop position {position}, must be in either in DATA_COLLECTION or OUT_OF_BEAM position."


Our scintillator and aperture scatterguard devices are currently in the "common device" section of dodal. Are we sure that the collision checks here are valid for a generic beamline? Maybe @DominicOram has some idea?

They are probably specific to phase 1 beamlines but then I suspect 90% of this device is specific to phase 1 beamlines. I think this true for many of the devices in this common section and the plan was to generally keep them there until we were sure they were specific and then move them. If we want to do that now I think it probably makes sense to make a new issue that's goes through them all

OK, we can leave it in common with a comment that it's likely MX specific. Then when someone else wants to use a scintillator we can write down the differences and try and make a common scint. Same for other MX devices in common

olliesilvester · 2026-01-20T14:30:33Z

src/dodal/devices/aperturescatterguard.py

+    scintillator_move_miniap_x: float
+    scintillator_move_sg_x: float


nit: sg_x may not mean scatterguard to a new person - maybe just a comment?

nit: Names of these variables are a bit unclear to me. Something like miniap_x_pos_for_safe_scint_move maybe?

olliesilvester · 2026-01-20T14:32:05Z

src/dodal/devices/mx_phase1/aperturescatterguard.py

@@ -373,3 +389,9 @@ async def prepare(self, value: ApertureValue):
            )
        else:
            await self.selected_aperture.set(value)
+
+    def get_scin_move_position(self) -> dict[Motor, float]:


Can you add a comment to say these are the ap + sg positions which make it safe to move the scint out? Variable name changes in https://github.com/DiamondLightSource/dodal/pull/1819/changes#r2708606396 might do the same job

olliesilvester · 2026-01-20T14:41:50Z

src/dodal/devices/scintillator.py

            case _:
                raise ValueError(f"Cannot set scintillator to position {position}")
+
+    async def do_with_ap_sg_in_safe_pos(self, func):


Docstring would be good here, I think the function name is a bit confusing. We are:

Saving the current ap_sg positions

Moving the ap_sg to positions which make it safe to move the scint

Moving the scint

Moving ap_sg back

olliesilvester · 2026-01-20T14:44:24Z

src/dodal/devices/scintillator.py

+
+        await self._check_beamstop_position()
+
+        async def move_to_new_position():


I might have missed something but I can't see why this needs to be an inner function which is passed to do_with_ap_sg_in_safe_pos? I think this can just be an outer class method, which takes position as a parameter, so the code only ever defines it once

Not sure if it's really a saving in terms of number of times it's defined - I don't think it incurs much additional overhead.
I've moved it anyway, not sure if it's an improvement

olliesilvester · 2026-01-20T14:54:12Z

tests/devices/test_aperture_scatterguard.py

+        ApertureValue.PARKED,
+    ],
+)
+async def test_restore_ap_sg_from_scin_move_position(


Not sure I understand the asserts in this test. isn't it just checking that the "safe for scintillator" positions are unchanged after moving the ap_sg?

I will rename this test, originally there was more logic in the aperture-scatterguard, but I moved it

…configuration

olliesilvester

Please can you just adjust the diagram to as we discussed in person, other comments are optional

src/dodal/devices/scintillator.py

olliesilvester · 2026-02-02T15:32:34Z

src/dodal/devices/scintillator.py

+            await self.z_mm.set(self._scintillator_in_yz_mm[1])
+            await self.y_mm.set(self._scintillator_in_yz_mm[0])
+
+    async def do_with_aperture_scatterguard_in_safe_pos(self, func):


nit: Are there any other functions we'd ever supply to this other than _move_to_new_position? If no, might be better to be less general with the word func, eg scintillator_move_func

olliesilvester · 2026-02-02T15:51:05Z

tests/devices/test_aperture_scatterguard.py

    ],
 )
-async def test_restore_ap_sg_from_scin_move_position(
+async def test_get_scin_move_position_returns_expected(


This test is still a bit confusing. Unless I'm misreading, it looks like there's extra stuff that isn't needed. Canwe just have

async def test_get_scin_move_position_returns_expected( aperture_in_medium_pos: ApertureScatterguard, ap_sg_configuration: ApertureScatterguardConfiguration, run_engine: RunEngine, ): ap_sg = aperture_in_medium_pos positions = ap_sg.get_scin_move_position() assert ( positions[ap_sg.aperture.x] == ap_sg_configuration.scintillator_move_aperture_x ) assert ( positions[ap_sg.scatterguard.x] == ap_sg_configuration.scintillator_move_scatterguard_x )

?

olliesilvester · 2026-02-02T16:32:13Z

I've just had another thought about this. I thought in general we should avoid the case where doing a move at the device level causes another device to move, doing this at the plan level might be better. I won't block the PR on this decision, but this is what I'd opt for

Co-authored-by: olliesilvester <122091460+olliesilvester@users.noreply.github.com>

rtuck99 · 2026-02-03T15:45:40Z

I've just had another thought about this. I thought in general we should avoid the case where doing a move at the device level causes another device to move, doing this at the plan level might be better. I won't block the PR on this decision, but this is what I'd opt for

I think normally I'd agree with you, however since the scintillator safe move position isn't a single well-defined position, exposing it in the aperture-scatterguard device public interface so that the scintillator could check that when it was requested to move it was safe to do so would be awkward.

If someone adds the scintillator to a beamline later on, supplies the additional devices in the constructor without knowing what it does, is that any more likely than someone writing a plan that just calls an unguarded set() on the scintillator? Both would result in a collision.

…illator_safe_move

…view

…ets to still operate conditional on valid device positioning

Base automatically changed from 1807_update_copier_template to main January 15, 2026 15:02

rtuck99 force-pushed the mx-bluesky_1354_scintillator_safe_move branch 2 times, most recently from e249c69 to 0e63853 Compare January 19, 2026 10:01

rtuck99 mentioned this pull request Jan 19, 2026

Support scintillator safe move DiamondLightSource/mx-bluesky#1551

Draft

1 task

rtuck99 added the enhancement New feature or request label Jan 19, 2026

rtuck99 marked this pull request as ready for review January 19, 2026 10:53

rtuck99 requested a review from a team as a code owner January 19, 2026 10:53

olliesilvester self-assigned this Jan 20, 2026

olliesilvester requested changes Jan 20, 2026

View reviewed changes

rtuck99 added 9 commits January 26, 2026 17:22

Initial changes to aperture_scatterguard and scintillator to support …

caf0f29

…configuration

1540 Add docs for aperture-scatterguard

b72ee3e

Add beamstop check to scintillator move

a36a098

Implement aperture scatterguard move for reverse direction

e685f8e

Make type-checking happy

e74e11b

Add scintillator motion diagram

483ab94

Correct annotation of scintillator motion diagram

2a3a39c

Make type-checking happy

77cf52a

Response to PR comments

d09522c

rtuck99 force-pushed the mx-bluesky_1354_scintillator_safe_move branch from 3771af8 to d09522c Compare January 26, 2026 17:31

Make diagram background opaque but even more so

db02445

rtuck99 requested a review from olliesilvester January 27, 2026 17:17

Merge branch 'main' into mx-bluesky_1354_scintillator_safe_move

a8ea767

olliesilvester requested changes Feb 2, 2026

View reviewed changes

Clarify scintillator motion diagram

59867b3

rtuck99 requested a review from olliesilvester February 3, 2026 15:24

rtuck99 and others added 2 commits February 3, 2026 15:26

Update docstring in src/dodal/devices/scintillator.py

ce57190

Co-authored-by: olliesilvester <122091460+olliesilvester@users.noreply.github.com>

Simplify get_scin_move_position test

5f241d3

rtuck99 requested a review from DominicOram February 3, 2026 15:46

rtuck99 added 4 commits February 4, 2026 13:40

Merge remote-tracking branch 'origin/main' into mx-bluesky_1354_scint…

0dfd9f0

…illator_safe_move

Refactor of aperture-scatterguard, scintillator as suggested in PR re…

f05343f

…view

Refactor scintillator, aperture-scatterguard again for selected_pos s…

71941a4

…ets to still operate conditional on valid device positioning

Fixup unit tests

a2e9c3f

rtuck99 marked this pull request as draft February 5, 2026 16:46

		scintillator_move_miniap_x: float
		scintillator_move_sg_x: float


		await self._check_beamstop_position()

		async def move_to_new_position():

Conversation

rtuck99 commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions to reviewer on how to test:

Checks for reviewer

Uh oh!

codecov bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

olliesilvester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olliesilvester left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olliesilvester commented Feb 2, 2026

Uh oh!

rtuck99 commented Feb 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

rtuck99 commented Jan 15, 2026 •

edited

Loading

codecov bot commented Jan 15, 2026 •

edited

Loading