Skip to content

Move aperture scatterguard independently#932

Merged
DominicOram merged 20 commits intomainfrom
mx_bluesky_267_move_aperture_scatterguard_independently
Feb 4, 2025
Merged

Move aperture scatterguard independently#932
DominicOram merged 20 commits intomainfrom
mx_bluesky_267_move_aperture_scatterguard_independently

Conversation

@DominicOram
Copy link
Contributor

@DominicOram DominicOram commented Nov 27, 2024

Fixes DiamondLightSource/mx-bluesky#267

Instructions to reviewer on how to test:

  1. Confirm tests pass
  2. Check the added API for aperture/scatterguard makes sense if given the docstring

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
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.65%. Comparing base (80d3586) to head (a2b9d8c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #932      +/-   ##
==========================================
+ Coverage   97.63%   97.65%   +0.02%     
==========================================
  Files         159      159              
  Lines        6560     6578      +18     
==========================================
+ Hits         6405     6424      +19     
+ Misses        155      154       -1     

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

@DominicOram DominicOram changed the title Mx bluesky 267 move aperture scatterguard independently Move aperture scatterguard independently Nov 29, 2024
@DominicOram DominicOram marked this pull request as ready for review November 29, 2024 16:38
@DominicOram DominicOram requested a review from a team as a code owner November 29, 2024 16:38
the other axes for the aperture that's to follow:

await aperture_scatterguard.aperture_outside_beam.set(ApertureValue.LARGE)

Copy link
Contributor

@rtuck99 rtuck99 Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is confusingly named. "outside beam" suggests that the movement is always outside the beam (or even that moving it will put the aperture out), but we are in beam, then it will still move the aperture, but not move it out (unless you explicitly set it to ROBOT_LOAD).

I feel like something akin to prepare_aperture would be better. But supporting the in-beam movement as well using this method still makes me uncomfortable because the whole point of this is to be able to call it earlier when it's not clear to move it back in yet, so why would you want this method to be able to do that anyway?

Hmm, what I just wrote there isn't really true since the apsg may be in-beam already if we didn't do a robot load. But I think this just confirms that the name of this doesn't really capture the intent or usage. I think an important feature of this function is that until you do move_out.trigger() the aperture is in an undefined position which you can't make assumptions about.

The more I think about this, the more I feel that perhaps OutTrigger and ApertureSelector (or perhaps even the whole ApertureScatterguard) should be one device which implements Preparable and Triggerable, and that you should just call prepare() and trigger() on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is confusingly named. "outside beam" suggests that the movement is always outside the beam (or even that moving it will put the aperture out), but we are in beam, then it will still move the aperture, but not move it out (unless you explicitly set it to ROBOT_LOAD).

Happy to rename it if we can think of a better name.

I think an important feature of this function is that until you do move_out.trigger() the aperture is in an undefined position which you can't make assumptions about.

I'm unsure what you mean by this. There are 6 positions, which are implicitly defined:

  • In + Large (maps to LARGE_APERTURE in the file)
  • In + Medium (maps to MEDIUM_APERTURE in the file)
  • In + Small (maps to SMALL_APERTURE in the file)
  • Out + Large (maps to ROBOT_LOAD in the file)
  • Out + Medium
  • Out + Small

move_out.trigger() actually moves you to one of the Out positions, which are less explicitly defined.

The more I think about this, the more I feel that perhaps OutTrigger and ApertureSelector (or perhaps even the whole ApertureScatterguard) should be one device which implements Preparable and Triggerable, and that you should just call prepare() and trigger() on it.

So aperture_scatterguard.trigger() would move it out or move it in? I'm not sure I'm understanding. I like the idea of using prepare though, we could maybe replace aperture_outside_beam with just prepare

In general I'm very open to changing the structure but I have gone round this loop 3 or 4 times now and it's not easy. I'll put in some time to whiteboard it.

In general my thought I do like keeping the interface of await aperture_scatterguard.set(ApertureValue.LARGE) moving the aperture in though, this seems intuitively what you would expect

Copy link
Contributor Author

@DominicOram DominicOram Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in person. The proposed solution is to have:

  • aperture_scatterguard.set(value: ApertureValueWithOut) where ApertureValueWithOut can be S/M/L/Out. For S/M/L the selected aperture gets moved into the beam. For Out we move the assembly out of the beam without moving the scatterguard
  • aperture_scatterguard.prepare(value: ApertureValue) where ApertureValue can be S/M/L. This will move the scatterguard to the selected aperture without moving in

There are some gotchas with this:

  • What do we do when prepare is called and we're already in - in this case we just still move to the location and stay in i.e. we just call set
  • There is a potential problem with calling prepare then set in quick succession. In this case we should make the set wait for the prepare to finish or throw an error

@DominicOram DominicOram requested a review from rtuck99 January 24, 2025 10:04
Copy link
Contributor

@rtuck99 rtuck99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@DominicOram DominicOram merged commit 9e3ef3c into main Feb 4, 2025
19 checks passed
@DominicOram DominicOram deleted the mx_bluesky_267_move_aperture_scatterguard_independently branch February 4, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move scatterguard into correct position without moving aperture

2 participants