add support for i16 DCM - there are PVs still missing in EPICS #1580

fajinyuan · 2025-09-25T14:50:40Z

Fixes #1568

Instructions to reviewer on how to test:

added additional motors for each crystal
waiting for missing PVs in EPICS IOC for i16 DCM

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}

fajinyuan · 2025-09-25T14:52:15Z

waiting for https://jira.diamond.ac.uk/browse/I16-960 to pass 'dodal connect i16' test

codecov · 2025-09-25T14:54:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.80%. Comparing base (2585faf) to head (6eedc1d).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1580      +/-   ##
==========================================
- Coverage   98.82%   98.80%   -0.03%     
==========================================
  Files         250      262      +12     
  Lines        8992     9501     +509     
==========================================
+ Hits         8886     9387     +501     
- Misses        106      114       +8

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

DominicOram · 2025-09-25T18:36:12Z

Big fan of pushing for standardisation on the controls side. See #592 where we went through this look for i22/i03

oliwenmandiamond

This is very similar to I22 DCM with some slight differences. That class also doesn't have tests associated with it and uses the same method of overriding the read and describe methods.

As we are waiting on controls to standardise the PV's, I am inclined to approve of this so it's not held in limbo. I do however think once more beamlines have created more custom DCM's, we re-evaluate this in near future so that:

We can create another class that can form a base for more beamline DCMs to eliminate more duplicate code.
We add some tests if they have to override the read and describe methods with custom logic

Until all the beamlines DCM's are added, I don't think it is worth doing yet until we have a clear picture of what all the beamline requirements are. I will make an issue to track this.

src/dodal/beamlines/i16.py

oliwenmandiamond

I22 do have tests for their DCM device. Please add some tests for I16 DCM device and then we create a new issue to consolidate the two devices (+ any others) in future once all DCM's are added and know all beamline requirements.

src/dodal/beamlines/i16.py

Relm-Arrowny · 2025-09-30T11:30:18Z

src/dodal/devices/i16/dcm.py

+    async def describe(self) -> dict[str, DataKey]:
+        default_describe = await super().describe()
+        return {
+            f"{self.name}-wavelength_in_a": DataKey(
+                dtype="number",
+                shape=[],
+                source=self.name,
+                units="angstrom",
+            ),
+            **default_describe,
+        }
+
+    async def read(self) -> dict[str, Reading]:
+        default_reading = await super().read()
+        energy: float = default_reading[f"{self.name}-energy_in_kev"]["value"]
+        if energy > 0.0:
+            wavelength = _CONVERSION_CONSTANT / energy
+        else:
+            wavelength = 0.0
+
+        return {
+            **default_reading,
+            f"{self.name}-wavelength_in_a": Reading(
+                value=wavelength,
+                timestamp=time.time(),
+            ),
+        }


Should: I think this is the perfect place for derived signal.
This probably overcome the issue they discussing here

Then you only need to add it as readable

Something like this:

inside __init__ with self.add_children_as_readables(): self.wavelength = derived_signal_r( raw_to_derived=self._read_wavelength, derived_units="angstrom", energy=self.energy_in_kev, ) def _read_wavelength(self, energy: float) -> float: if energy > 0.0: wavelength = _CONVERSION_CONSTANT / energy else: wavelength = 0.0 return wavelength

Must: Add test for read.

Just saw #1584 may be it should be in the base class?

Yes agreed, I think there needs to be a different approach to this for I16 and I22 to get this to work

why we do the energy to wavelength conversion here, not just simply use EPICS wavelength PV ?

Some reading on bluesky verbs for read and describe. https://blueskyproject.io/bluesky/main/hardware.html

They are called at every point in a scan. read returns the data to record and save. describe tells us about the type, shape, PV used, etc.

Thanks for your comments @oliwenmandiamond and @Relm-Arrowny @DominicOram. It looked I choose a wrong dcm.py from i22 to use for i16 then.
I will remove describe and read method from i16/dcm.py, and use derived signal instead.

sorry @Relm-Arrowny I just realised the wavelength is already in BaseDCM, why I need to redo it using derived signal here, could you explain this to me please?

#1598 should help fix this as you will have two options on which class to inherit from for DCM. One of them won't include wavelength, so you can define it yourself as a derived signal then.

sorry @Relm-Arrowny I just realised the wavelength is already in BaseDCM, why I need to redo it using derived signal here, could you explain this to me please?

The only reason I suggested it is because I saw the conversion from energy, if they already exit then you can ignore me and you should not need to modify read or describe.

accept recommendation Co-authored-by: oliwenmandiamond <[email protected]>

DG-At-Diamond · 2025-11-13T16:05:46Z

The last I remember of this issue was we discovered that the 'DCM' on i16 was actually a CCM that had originally been designed as a DCM. It would still in theory be usable as a DCM if the crystals were replaced but this is not done in practice. We were going to discuss the requirements with the beamline but probably implement it as a CCM when we are ready to implement that. Can anyone confirm this is still as far as the conversation got @fajinyuan @oliwenmandiamond? If so it might be worth closing this request without merging it.

fajinyuan · 2025-11-13T16:53:16Z

Please implement the CCM, instead of DCM. Also please update the ticket replacing DCM with CCM.

add suprot for i16 DCM - there are PVs still missing in EPICS

08dbe98

fajinyuan requested a review from a team as a code owner September 25, 2025 14:50

fajinyuan linked an issue Sep 25, 2025 that may be closed by this pull request

Add DCM device for i16 #1568

Open

fajinyuan self-assigned this Sep 25, 2025

fajinyuan requested review from Relm-Arrowny and oliwenmandiamond September 25, 2025 14:53

Merge branch 'main' into 1568-add-dcm-device-for-i16

0f3d590

oliwenmandiamond approved these changes Sep 30, 2025

View reviewed changes

src/dodal/beamlines/i16.py Show resolved Hide resolved

oliwenmandiamond requested changes Sep 30, 2025

View reviewed changes

oliwenmandiamond changed the title ~~add suprot for i16 DCM - there are PVs still missing in EPICS~~ add support for i16 DCM - there are PVs still missing in EPICS Sep 30, 2025

Relm-Arrowny reviewed Sep 30, 2025

View reviewed changes

fajinyuan and others added 2 commits October 2, 2025 08:54

Update src/dodal/beamlines/i16.py

8149bec

accept recommendation Co-authored-by: oliwenmandiamond <[email protected]>

simplify impl to not use i22 dcm.py as example.

6eedc1d

add support for i16 DCM - there are PVs still missing in EPICS #1580

Are you sure you want to change the base?

add support for i16 DCM - there are PVs still missing in EPICS #1580

Uh oh!

Conversation

fajinyuan commented Sep 25, 2025

Instructions to reviewer on how to test:

Checks for reviewer

Uh oh!

fajinyuan commented Sep 25, 2025

Uh oh!

codecov bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DominicOram commented Sep 25, 2025

Uh oh!

oliwenmandiamond left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oliwenmandiamond left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Relm-Arrowny Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Relm-Arrowny Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Relm-Arrowny Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

oliwenmandiamond Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

fajinyuan Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliwenmandiamond Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fajinyuan Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

fajinyuan Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

oliwenmandiamond Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Relm-Arrowny Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

DG-At-Diamond commented Nov 13, 2025

Uh oh!

fajinyuan commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Sep 25, 2025 •

edited

Loading

Relm-Arrowny Sep 30, 2025 •

edited

Loading

fajinyuan Oct 2, 2025 •

edited

Loading

oliwenmandiamond Oct 2, 2025 •

edited

Loading

oliwenmandiamond Oct 3, 2025 •

edited

Loading