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

Automatically keep I22 and P38 in sync #502

Open
4 tasks
callumforrester opened this issue May 3, 2024 · 11 comments · May be fixed by #678
Open
4 tasks

Automatically keep I22 and P38 in sync #502

callumforrester opened this issue May 3, 2024 · 11 comments · May be fixed by #678
Assignees

Comments

@callumforrester
Copy link
Contributor

callumforrester commented May 3, 2024

As a developer I would like to keep P38, at least from an ophyd point of view, as a close representation of I22. Currently they are treated as different beamlines with some similar components.

Possible Solution

Could we somehow define different versions of particular devices?

Acceptance Criteria

  • There should be one module, i22.py, p38.py should be deleted
  • dodal connect i22 should work without errors when the I22 PVs are available
  • dodal connect i22 --profile p38 or BEAMLINE=p38 dodal connect i22 (syntax can differ) should work when the P38 PVs are available
  • There should be no errors in either case, dodal should handle the differences between the beamlines

from #502 (comment)

@DiamondJoseph
Copy link
Contributor

A possible solution is to use the same beamline module for both, with careful @skip_device usage and care for visit directories- p38 is only ever going to be a commissioning visit(?)

@DiamondJoseph
Copy link
Contributor

p38 is already analogous to s04 and could be treated the same way: i22.py gets BL = get_beamline_name("p38")

@callumforrester
Copy link
Contributor Author

callumforrester commented May 7, 2024

I would suggest more than just d11 and saxs in the same beamline module. I would suggest that we say d11 is saxs, or at least a testing representation of it. So the ophyd devices of P38 are a 1:1 representation of I22, even if we have to mock some devices.

@stan-dot
Copy link
Contributor

are the PV names the same? What is stopping us from using the file as-is?

should we have BEAMLINE_NAME derived from venv?

@DiamondJoseph
Copy link
Contributor

BL = get_beamline_name("p38")

Checks Environment Variables to see if BEAMLINE is exported (it will be on a beamline control machine).

Comparing p38.py and i22.py:

(best guess):
d11 ~ saxs: "-DI-DCAM-03:" vs. "-EA-PILAT-01:",
d12 ~ waxs: "-DI-DCAM-04:" vs. "-EA-PILAT-03:",
d3 ~ oav: "-DI-DCAM-01:" vs. "-DI-OAV-01:"

i0 ~ i0: "-EA-XBPM-01:" vs. "-EA-XBPM-02:"
linkam ~ linkam: "-EA-LINKM-02:" vs. "-EA-TEMPC-05",

panda1/2/3: match
panda4, it: no equivalent device on p38
all others: sim devices on p38

d11, d12 shouldn't be exposed as -EA-PILAT-01, as they aren't Pilatuses (? should we allow this? Hopefully all plan stubs are written for StandardDetectors?)
it should be added as a sim device or else repeating the values from i0 (?)

Definitely the Linkam and TetrAMM could be exposed on the same prefix

@stan-dot
Copy link
Contributor

then we have the manual option of just applying a ternary expression for the PVs / or an if block

or we could make some decorator or such.

we'd also need to make it crash for panda4 I guess if there is an attempt to use it in a plan???

@callumforrester
Copy link
Contributor Author

The specific case of a detector being a PilatusDetector if it's i22 and an AravisDetector if it's p38 is what's interesting here...

@stan-dot
Copy link
Contributor

ok why not just a ternary expression?

not sure how different are the params, will explore this in a branch

@stan-dot stan-dot linked a pull request Jul 16, 2024 that will close this issue
4 tasks
@stan-dot stan-dot linked a pull request Jul 16, 2024 that will close this issue
4 tasks
@callumforrester
Copy link
Contributor Author

To add some concrete acceptance criteria:

  • There should be one module, i22.py, p38.py should be deleted
  • dodal connect i22 should work without errors when the I22 PVs are available
  • dodal connect i22 --profile p38 or BEAMLINE=p38 dodal connect i22 (syntax can differ) should work when the P38 PVs are available
  • There should be no errors in either case, dodal should handle the differences between the beamlines

@stan-dot
Copy link
Contributor

I guessed some of those, but it's great to see this clearly. Thanks!

@stan-dot
Copy link
Contributor

BEAMLINE=p38 dodal connect i22 will test this tomorrow, together with this
DiamondLightSource/i22-bluesky#87

@DiamondJoseph please advise

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 a pull request may close this issue.

3 participants