-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add fswitch device for i22 #469
Conversation
This is an initial implementation thought to achieve the desired result however it needs to be checked with beamline staff. There is some scope in future for combining with the i04 fswitch (currently transfocator.py) but the epics interfaces are currently incompatible. |
|
||
NUM_FILTERS = 128 | ||
|
||
def __init__(self, prefix: str, name: str = "") -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd prefer for top-level devices like this to require a name, but it looks like the Slits made it in with a default so I can't point to precedence =(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coretl thinks the opposite so we should probably resolve this at some point. Large point of diverging effort otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be recorded in a ticket or is it already noted somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #415 (comment) I suggest using the name of the device factory function to name the device. This would mean you could do:
# i22.py
@device_factory(lazy=True)
def fswitch():
return FSwitch(f"{BL}-MO-FSWT-01:")
assert fswitch().name == "fswitch"
If @DominicOram is happy with this approach I will write this up as an ADR for Dodal along with the connection strategy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe that's the default we've tried to go with. Thank you @coretl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add to the i22 beamline module too
OUT = "OUT" | ||
|
||
|
||
class FSwitch(StandardReadable): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there signals for the desired config fields: lens geometry, cylindrical, lens_material?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be one for the scientists. I imagine it'll be constant (for i22), but I can't see it mentioned in the beamline paper or https://www.diamond.ac.uk/default/Instruments/Soft-Condensed-Matter/small-angle/I22/Ops-modes/I22-TR-SAXS-WAXS.html. I can tag it onto some other questions I've fired at Andy if that's helpful...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For i22:
F-switch only contains one type of lens which are paraboloid, cylindrical, beryllium lenses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest just making these soft signals as per
dodal/src/dodal/devices/focusing_mirror.py
Line 136 in a3e68ef
self.type, _ = soft_signal_r_and_backend(str, "single") |
""
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest just making these....
Note: I think we will need to be on the latest ophyd-async
first. I'm working on it, should be done this morning (#475)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion @Tom-Willemsen I think we would like to have some further thoughts on the soft signals/metadata but may add it in a follow up PR.
Eventually this should be combined with the transfocator device in the i04 | ||
module but is currently incompatible as the Epics interfaces are different. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: I would prefer this as a Github issue rather than a docstring, could just link to the issue here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, as discussed at #399 (comment) I think the first thing to do to combine them would be to reach out to i22 and see if they want the logic for beamsize calculation. @aragaod (i04 beamline scientist) has said he'd be happy to chat to them about how i04 use it if we arrange the discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've requested for the new version of the support module to be tested soon. This will also simplify the device as the new version contains a PV that provides this value already.
We discussed earlier that we still would like to get this merged to aid with testing on Tuesday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new version contains a PV that provides this value already
I presume controls will check the maths with i22 scientists directly? Very much wouldn't assume that i04's maths necessarily just works for i22...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that for the number of lenses in the beam it is just counting up the filter PVs that are "IN_BEAM" as this device is doing? Or is there some complexity I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along with I22 config, could also do with P38 config for a fake fswitch, see other optics components already there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, wrong button!
This is my first comment in dodal or Bluesky, so I don't know the class naming convention please clarify for me. So my question is why this class use "FSwitch", not "FiltersSwitch" which will be more readable and easier for later joined developer to find what is already available so it can reduce potential duplication and make resue easier. |
The device is called an "F-Switch" see: Do people think a better name should be used? I'm not really familiar with the device so open to suggestions. |
On i22, it seems to be mostly referred to as an p.s. I've also asked Andy for his opinion in case there's subtle naming distinctions I'm missing. |
0613e1a
to
0381677
Compare
Thank @joeshannon. I thought using abbreviation is 'sole right' of software engineers, did not realise hardware enginners also prefer to use it. now I understand it is the name of the Device! |
Fixes #399
Instructions to reviewer on how to test:
Checks for reviewer