Skip to content
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

Rationalise Slits Implementations #462

Open
callumforrester opened this issue Apr 24, 2024 · 8 comments · May be fixed by #557 or DiamondLightSource/hyperion#1394
Open

Rationalise Slits Implementations #462

callumforrester opened this issue Apr 24, 2024 · 8 comments · May be fixed by #557 or DiamondLightSource/hyperion#1394
Assignees

Comments

@callumforrester
Copy link
Contributor

Dodal now has three attempts at a device to control 4-blade slits sets. One from #431, one from #315 and one originally imported from Hyperion: https://github.com/DiamondLightSource/dodal/blob/main/src/dodal/devices/s4_slit_gaps.py

As a developer I would like to rationalise them into a single device so I can reduce technical debt. See additional discussion in #384. Current work for the Hypersion slits and #431 requires underlying EPICS implementations to be rationalised first. From @DominicOram: this is being tracked internally at https://jira.diamond.ac.uk/browse/I03-956. Currently unsure if the #315 slits can also operate on the same of PVs, @dan-fernandes may be able to comment.

Acceptance Criteria

  • There is one class for 4-bladed slits
  • It is an ophyd-async device
  • It has tests
  • All beamline configs that use slits now use it
@callumforrester callumforrester mentioned this issue Apr 24, 2024
2 tasks
callumforrester added a commit that referenced this issue Apr 25, 2024
Create ophyd-async version of `S4SlitGaps`. To be rationalised in #462 

* Port S4SlitGaps to ophyd-async and add tests
* Add I22 and P38 config for slit gaps
* Add docstrings to slit gaps
@DominicOram
Copy link
Contributor

The new PVs are now on i03, we can switch over to using them

@DominicOram
Copy link
Contributor

To do the i03 + i22 merge:

  • We need to change i03 over to use the i22 ones and see what breaks
  • If there is specific logic in the i03 ones, move this over (but I don't think there is)
  • Run i03 system tests to confirm
  • Run hyperion unit tests
  • Small job

@DominicOram
Copy link
Contributor

Blocked on the PVs not being as we expected. Working with controls on this

@dan-fernandes
Copy link

Sorry, late to the thread.

Currently unsure if the #315 slits can also operate on the same of PVs, @dan-fernandes may be able to comment.

These slits implement a different Ophyd class for each set of slits that have different PV names, American vs British English, etc. (it was to just get stuff working, not very well thought out.)
Then my beamline-agnostic Bluesky plan had a factory method pattern to get the correct ophyd class from a config file.

There are other problems with my slit implementation (such as relying on a class I wrote that is essentially just a Motor, very rudimentary simulator, etc.) so I would be happy to edit my Bluesky plan to use whichever slit implementation you choose.

Happy to talk in person if this doesn't help!

@olliesilvester
Copy link
Collaborator

mx-controls say they can make the PV naming consistent during next shutdown (CENTRE over CENTER)

@stan-dot
Copy link
Contributor

stan-dot commented Aug 5, 2024

this reappered at i18

@stan-dot
Copy link
Contributor

stan-dot commented Aug 5, 2024

also this is relevant
#587

@gilesknap
Copy link

I'd like to comment that in future I would expect all containerised motor IOCs with gap and centre slits to be using the template in the pmac module. So this is what we should be aiming for, or if not then we need to change it to what we want:
https://github.com/DiamondLightSource/pmac/blob/dls-master/pmacApp/Db/gap_and_centre_slits.template

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Blocked
Development

Successfully merging a pull request may close this issue.

6 participants