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

Pressure jump fast adc based on area detector #745

Draft
wants to merge 1 commit into
base: 658-pressure-jump-cell
Choose a base branch
from

Conversation

barnettwilliam
Copy link
Contributor

Closes #727
Area detector for pressure jump cell fast capture.

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}

Comment on lines +12 to +26
from ophyd import ADComponent as ADC
from ophyd import (
AreaDetector,
CamBase,
Component,
Device,
EpicsSignal,
HDF5Plugin,
OverlayPlugin,
ProcessPlugin,
ROIPlugin,
StatsPlugin,
Signal,
StatusBase,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use ophyd-async instead of ophyd implementation of things.

This then is an implementation of StandardDetector

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, in general all new devices should be exclusively ophyd-async. The aim is to eventually remove ophyd as a dependency of dodal.

Comment on lines +28 to +63
class PressureJumpAdcConfigParams:
pass

class PressureJumpCellADC(AreaDetector):
"""
High pressure X-ray cell fast ADC, used to capture pressure jumps.
"""
cam = ADC(CamBase, "-EA-HPXC-01:CAM:")
scale = ADC(ROIPlugin, "-EA-HPXC-01:SCALE:")
# trig = # TODO support module adUtil NDPluginReframe?
# arr = # TODO support module ADCore NDPluginStdArrays? required?
hdf5 = ADC(HDF5Plugin, "-DI-OAV-01:HDF5:")

p1Roi = ADC(ROIPlugin, "-EA-HPXC-01:ROIP1:")
p1Proc = ADC(ProcessPlugin, "-EA-HPXC-01:PROCP1:")
p1Stats = ADC(StatsPlugin, "-EA-HPXC-01:STATP1:")

p2Roi = ADC(ROIPlugin, "-EA-HPXC-01:ROIP2:")
p2Proc = ADC(ProcessPlugin, "-EA-HPXC-01:PROCP2:")
p2Stats = ADC(StatsPlugin, "-EA-HPXC-01:STATP2:")

p3Roi = ADC(ROIPlugin, "-EA-HPXC-01:ROIP3:")
p3Proc = ADC(ProcessPlugin, "-EA-HPXC-01:PROCP3:")
p3Stats = ADC(StatsPlugin, "-EA-HPXC-01:STATP3:")

def __init__(self, *args, params: PressureJumpAdcConfigParams, **kwargs):
super().__init__(*args, **kwargs)
self.parameters = params
self.snapshot.oav_params = params
self.subscription_id = None
self._snapshot_trigger_subscription_id = None

def wait_for_connection(self, all_signals=False, timeout=2):
connected = super().wait_for_connection(all_signals, timeout)
return connected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compare the implementation of e.g. the AdAravis in ophyd-async
https://github.com/bluesky/ophyd-async/tree/e4fb8492bfad29dff9f539f6adc63c8152f68493/src/ophyd_async/epics/adaravis

This should be EthercatADC(StandardDetector) with an EthercatController, EthercatDriver if appropriate (from the looks of this can just use AdBase though, as no special signals?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Joseph I'll have a look. At the moment I'm not sure what would make a signal 'special'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any signals for the pressure exposed on the camera itself? From the ophyd definition I don't see any signals that are unique to function of the device class or particularly sought out for the experimental procedure, which is what I mean by "special".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, looking at the ioc configuration for the pressure jump cell BL38P-EA-IOC-12 it uses ADEthercat, ADCore, and adUtil standard support modules where there are 4 ethercat channels likely for the pressures. I wasn't sure if this will be picked up from the IOC by the ophyd modules or whether this needs to be specified in the ophyd device?

There is an additional support module specific to the pressure cell where pressureCell.scale is used in the IOC configuration which I presume adjusts the scale for the visualisation.

@stan-dot stan-dot force-pushed the 658-pressure-jump-cell branch 4 times, most recently from 15996f2 to 4a7b253 Compare August 23, 2024 12:07
@stan-dot stan-dot force-pushed the 658-pressure-jump-cell branch 2 times, most recently from 26e46cd to 1c77f3c Compare August 30, 2024 09:36
@barnettwilliam barnettwilliam force-pushed the 727-pressure-jump-cell-area-detector branch from 94497c4 to 4456dd2 Compare September 17, 2024 14:53
Copy link

codecov bot commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.

Project coverage is 94.49%. Comparing base (1c77f3c) to head (4456dd2).

Files with missing lines Patch % Lines
src/dodal/devices/pressure_jump_cell_adc.py 86.66% 4 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           658-pressure-jump-cell     #745      +/-   ##
==========================================================
- Coverage                   94.53%   94.49%   -0.05%     
==========================================================
  Files                         116      117       +1     
  Lines                        4743     4776      +33     
==========================================================
+ Hits                         4484     4513      +29     
- Misses                        259      263       +4     

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

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 this pull request may close these issues.

3 participants