Reconcile I22 and I03 DCMs

[callumforrester]

As a developer I would like to work with a minimal number of DCM classes.

Currently the PV naming conventions vary across beamlines, even when the hardware configuration is relatively similar. In the case of I22 and I03, the DCMs are quite similar. According to @JamesOHeaDLS:

The main difference is that I03 only has roll and pitch on one of their crystals, whereas I22 has roll and pitch on both their first and second crystals

Which suggests that the PVs could at least adopt the same naming conventions, even if one DCM has extra motors, and we could have a shared base class.

Acceptance Criteria

• One DCM base class
• I03 and I22 implementations with only the extra I22 motor added

[JamesOHeaDLS]

Hi @callumforrester

I have a broader question - is the expectation, with the current developments to dodal, ophyd, and blusky, etc, that PV names across Diamond will adopt common naming conventions for devices?

I'm not criticising, but just highlighting that there is currently no action plan on the controls side that I'm aware of to make this happen.

Including @coretl on this thread.

Thanks,

James

[callumforrester]

Hi James, currently we've been doing it ad-hoc. When we encounter a discrepancy between two beamlines, we try asking the relevant controls engineer to change the PV names. This is quite nice and agile, and it means we don't need a giant work package to rename all PVs across Diamond simultaneously, just that the level of standardisation that is needed for DAQ purposes should creep in over time. I'll defer to @coretl though as I know he has opinions.

[coretl]

I'm not a big fan of the current approach of "make som PV names, and rely on DAQ to tell us if they're right", so I'm keen to find a better solution.

One possibility discussed with @gilesknap was that we make some DLS record alias templates that live in a single support module that we can include for existing beamlines. This could point to dodal as the canonical description of what PV names are required for which type of device.

[olliesilvester]

As of https://github.com/DiamondLightSource/dodal/pull/652, we also have an i24 DCM. So the DCM base class now should contain common PV's between i03, i22 and i24. And I guess any PV's which only differ in syntax should be renamed to a common name

[callumforrester]

@olliesilvester Somewhere, I can't find it, you outlined a nice compositional idea where you build a DCM object out of a bunch of modules, so we can cope with design differences etc.

@coretl RE: finding a better solution than relying on DAQ to tell you if the PVs are correct. I think you should be relying on CI rather than DAQ, I think any solution that relies on CI to tell DAQ and/or controls if the PVs are wrong is a huge improvement on the status quo. We were hoping https://github.com/DiamondLightSource/dodal/issues/507 would provide at least some of that, but open to other ideas.

[olliesilvester]

@olliesilvester Somewhere, I can't find it, you outlined a nice compositional idea where you build a DCM object out of a bunch of modules, so we can cope with design differences etc.

I think I found the email you were referring to 'we have decided to decompose our current bluesky plans into common building blocks for any MX beamline to use. A beamline can then construct its specific implementation by building up these smaller generic plans. These generic plans will be stored in the mx_bluesky repo. We will also look into using bluesky's 'prepare' command for device setup.'

So yeah, same idea works for devices

[JamesOHeaDLS]

Hello, I'm not familiar with dodal and friends at all so the following suggestion is with full ignorance:

Could the code not hold variables which are populated with the particular naming implementation?

eg for a DCM:

base_pv = BL03I-MO-DCM-01 bragg_suffix = :BRAGG first_xtal_roll = :ROLL second_xtal_roll = None first_crystal_pitch = :PITCH second_crystal_pitch = None etc

[callumforrester]

@JamesOHeaDLS Yes, good suggestion, I think this covers at least some of the use cases. It requires there to be a consistent superstate of all DCMs, i.e. something that could hold all of the variables without conflicts. That does make it harder to tell how the DCM works just from reading the code though. For instance, if I03's DCM has a second_crystal_pitch = Motor("BL03I-MO-ETC") and I22's has second_crystal_pitch = None, you have to look it up somewhere. And every piece of code that access it has to check if it's None or not.

It would be nicer if I22's DCM simply didn't have a second_crystal_pitch attribute in the first place. Then it's immediately obvious and the code that uses it gets simpler.

Maybe something more like this?

# i22.py
def dcm() -> DCMBase:
    return DCMBase(
        "BL22I-MO-DCM-01", 
        crystals=[
            DCMCrystalRollOnly("BL22I-MO-DCM-01:XTSAL1"),
            DCMCrystalRollAndPitch("BL22I-MO-DCM-01:XTSAL2"),
        ]
    )

# i03.py
def dcm() -> DCMBase:
    return DCMBase(
        "BL03I-MO-DCM-01", 
        crystals=[
            DCMCrystalRollAndPitch("BL03I-MO-DCM-01:XTSAL1"),
            DCMCrystalRollAndPitch("BL03I-MO-DCM-01:XTSAL2"),
        ]
    )

[DominicOram]

Additionally, the i03 DCM has a PV for wavelength and d-spacing. We should get these added to the i22 one.

[callumforrester]

@DominicOram is there a summary of the PV changes we need anywhere? I can pass it to controls.

[DominicOram]

Attached are the EDM screens of (from left to right), I22, I24 and I03. There is the obvious difference that i22/i24 have control of both crystals whereas i03 only the one, I think we can deal with this fine in ophyd as discussed above. However, there are other differences that I think we can standardise on:

• I24 and I03 both have a wavelength calculation. However, in i03 this is a motor record and the PV is called WAVELENGTH, in i24 it's an ai and called LAMBDA. There is no such record on i22. Propose we standardise on the PV name/type and put the same PV on i22
• I03 has a DSPACING:RBV PV, presumably the other beamlines have this somewhere in the IOC as it's needed to calculate bragg angle but if it is exposed I can't see where Propose we expose this on all beamlines with a common PV name
• I22/I03 has a :PERP and I24 has a :GAP - from discussion in the office we think these may physically mean the same thing if they do then I propose we standardise the name on these
• I22 uses urad for pitch/roll etc. whereas I24/I03 uses mRad I'm not too sure how we best fix this and if we want to standardise at the EPICS layer but if we're not careful it will burn us

I will start a discussion with controls on the above

[JamesOHeaDLS]

Hello,

• I agree we should standardise the record type and name for the wavelength calculation. For a bit more detail, I03 uses the 'translated_motor' template from the pmac module to present the wavelength as a motor (the template is a motor record with a macro where you can provide the mathematical relationship between it and another record). Whereas I24 does the same calculation, but via a handspun calc record. I think the pmac template is the way to go as it is a common method provided by a standard module
• The DSPACING records and friends all come from the accel_dcm template also within the pmac module. This module holds EPICS interfaces for our standard coordinate systems. It would be straightforward for other beamlines to implement this template. All DCM beamlines use the same coordinate system so it will work with all. The name is hardcoded within the template so it would be standarised by default
• Yes, GAP and PERP are the same thing. Happy to standardise. I would get input from the scientists on preference
• For the units, checking the implementation, the difference is just down to the MRES value. It would be easy to tweak in either direction. The tricky bit would be to get one or other beamline to change their units, and to do so without breaking something else in the stack