Mbs analyser slits#2074
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## mbs_detector #2074 +/- ##
=============================================
Coverage 99.14% 99.14%
=============================================
Files 342 343 +1
Lines 13281 13338 +57
=============================================
+ Hits 13167 13224 +57
Misses 114 114 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| config_sigs = ( | ||
| analyser_slits.direction, | ||
| analyser_slits.size, | ||
| analyser_slits.shape, | ||
| analyser_slits.setting, |
There was a problem hiding this comment.
It looks good, but I worry about users who will be tempted to make changes like:
analyser_slits.size.set(300)
in their terminal, then because you have only one directional syncing in EntranceSlitInformationDevice layer this soft signal will fall back to Epics value.
Maybe those soft signals should not be exposed at all?
There was a problem hiding this comment.
These signals are read only. The write signals are all private. From blueapi side, I believe it doesn't expose private signals. They can only change it via epics (which is how it should be done). The read only config sigs are then given to the detector to record the data at the start of the scan whenever data collection is done. It is also impossible (very hard to do) to sync in the opposite direction as the valid combinations are defined by the epics enums.
| energy_source: SignalR[float], | ||
| shutter: GenericFastShutter | None = None, | ||
| source_selector: SourceSelector | None = None, | ||
| config_sigs: Sequence[SignalR] = (), |
There was a problem hiding this comment.
why allow here any arbitrary config signals, is there a use case for it or is it not a final class?
There was a problem hiding this comment.
This is reflecting how StandardDetector adds config_sigs https://github.com/bluesky/ophyd-async/blob/main/src/ophyd_async/core/_detector.py#L421C2-L421C3
and AreaDetector API https://github.com/bluesky/ophyd-async/blob/1e07267fb12ff511b248a77a5ed5b6ac50364ad9/src/ophyd_async/epics/adcore/_detector.py#L17
The electron analyser detectors currently use StandardDetector, but I am going to move to use AreaDetector soon. I was waiting for stable release of ophyd-async first to make it easier so was waiting for this to be merged first bluesky/ophyd-async#1268
There was a problem hiding this comment.
OK, but I mean you could specify here analyser_slit as input parameter and pass down all it's soft signals to
config_sigs = (
analyser_slit.direction,
analyser_slit.setting,
analyser_slit.size,
analyser_slit.shape,
self.driver.region_name,
self.driver.energy_mode,
There was a problem hiding this comment.
I could, but then it makes it less generic when I can reuse what it already does rather than adding it to the constructor to do the exact same thing
Fixes #2069
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}