-
Notifications
You must be signed in to change notification settings - Fork 12
Mbs analyser slits #2074
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
base: mbs_detector
Are you sure you want to change the base?
Mbs analyser slits #2074
Changes from 7 commits
6a0f533
f2f1b7f
eeff228
6c4a648
14fa5dd
ae3db99
ec824b3
239e45a
c31af66
d2c26db
685f269
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| from bluesky.protocols import Reading | ||
| from ophyd_async.core import ( | ||
| DEFAULT_TIMEOUT, | ||
| AsyncStatus, | ||
| DeviceMock, | ||
| StandardReadable, | ||
| StrictEnum, | ||
| soft_signal_r_and_setter, | ||
| ) | ||
| from ophyd_async.epics.core import epics_signal_rw | ||
| from pydantic import BaseModel | ||
|
|
||
|
|
||
| class SlitPosition(StrictEnum): | ||
| P100_0_1_CURVED = "100 0.1 curved" | ||
| P200_0_1_STRAIGHT = "200 0.1 straight" | ||
| P300_0_2_CURVED = "300 0.2 curved" | ||
| P400_0_2_STRAIGHT = "400 0.2 straight" | ||
| P500_0_2_STRAIGHT = "500 0.2 straight" | ||
| P600_0_3_STRAIGHT = "600 0.3 straight" | ||
| P700_0_5_STRAIGHT = "700 0.5 straight" | ||
| P800_0_8_STRAIGHT = "800 0.8 straight" | ||
| P850_3_HOLE = "850 3 hole" | ||
| P900_1_5_STRAIGHT = "900 1.5 straight" | ||
|
|
||
|
|
||
| class EntranceSlitInformation(BaseModel): | ||
| direction: str = "vertical" | ||
| setting: int = 100 | ||
| size: float = 0.1 | ||
| shape: str = "curved" | ||
|
|
||
| @classmethod | ||
| def from_slit_positions(cls, pos: SlitPosition): | ||
|
oliwenmandiamond marked this conversation as resolved.
Outdated
|
||
| setting, size, shape = str(pos).split() | ||
| return cls(setting=int(setting), size=float(size), shape=shape) | ||
|
|
||
| def to_slit_position(self) -> SlitPosition: | ||
| return SlitPosition(f"{self.setting} {self.size:g} {self.shape}") | ||
|
|
||
|
|
||
| class EntranceSlitInformationDevice(StandardReadable): | ||
| """Device that connects to epics signal containing slit information from an enum | ||
| value. This is synced with soft signals as individual signals which can be added as | ||
| config_signals to give to detectors to save as nicely formatted data. | ||
| """ | ||
|
|
||
| def __init__(self, pv: str, name: str = ""): | ||
| self.slit_pos = epics_signal_rw(SlitPosition, pv) | ||
| # Formatted slit info as individual soft signals for metadata | ||
| with self.add_children_as_readables(): | ||
| self.direction, self._direction_w = soft_signal_r_and_setter(str) | ||
| self.setting, self._setting_w = soft_signal_r_and_setter(int) | ||
| self.size, self._size_w = soft_signal_r_and_setter(float) | ||
| self.shape, self._shape_w = soft_signal_r_and_setter(str) | ||
| super().__init__(name) | ||
|
|
||
| @AsyncStatus.wrap | ||
| async def set(self, value: SlitPosition): | ||
| await self.slit_pos.set(value) | ||
|
|
||
| async def connect( | ||
| self, | ||
| mock: bool | DeviceMock = False, | ||
| timeout: float = DEFAULT_TIMEOUT, | ||
| force_reconnect: bool = False, | ||
| ) -> None: | ||
| await super().connect(mock, timeout, force_reconnect) | ||
|
|
||
| def _sync_soft_signals_with_epics( | ||
| value: dict[str, Reading[SlitPosition]], | ||
| ) -> None: | ||
| val = value[self.slit_pos.name]["value"] | ||
| new_slit_info = EntranceSlitInformation.from_slit_positions(val) | ||
| self._direction_w(new_slit_info.direction) | ||
| self._setting_w(new_slit_info.setting) | ||
| self._size_w(new_slit_info.size) | ||
| self._shape_w(new_slit_info.shape) | ||
|
|
||
| self.slit_pos.subscribe(_sync_soft_signals_with_epics) | ||
|
oliwenmandiamond marked this conversation as resolved.
Outdated
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| from collections.abc import Sequence | ||
| from typing import Generic | ||
|
|
||
| from ophyd_async.core import SignalR, soft_signal_rw | ||
|
|
@@ -32,6 +33,7 @@ def __init__( | |
| energy_source: SignalR[float], | ||
| shutter: GenericFastShutter | None = None, | ||
| source_selector: SourceSelector | None = None, | ||
| config_sigs: Sequence[SignalR] = (), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why allow here any arbitrary config signals, is there a use case for it or is it not a final class?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is reflecting how and The electron analyser detectors currently use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, but I mean you could specify here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| name: str = "", | ||
| ): | ||
| # Make attribute of class so connect applies to driver and populates parent. | ||
|
|
@@ -49,6 +51,7 @@ def __init__( | |
| ) | ||
| trigger_logic = ElectronAnalayserTriggerLogic(self.driver, set()) | ||
| config_sigs = ( | ||
| *config_sigs, | ||
| self.driver.region_name, | ||
| self.driver.energy_mode, | ||
| self.driver.acquisition_mode, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| import pytest | ||
| from ophyd_async.core import init_devices | ||
| from ophyd_async.testing import assert_reading, partial_reading | ||
|
|
||
| from dodal.devices.electron_analyser.mbs import ( | ||
| EntranceSlitInformation, | ||
| EntranceSlitInformationDevice, | ||
| SlitPosition, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("slit_pos", [pos.value for pos in SlitPosition]) | ||
| def test_entrance_slit_info_to_slit_position(slit_pos: SlitPosition): | ||
| slit_info = EntranceSlitInformation.from_slit_positions(slit_pos) | ||
| assert slit_info.to_slit_position() == slit_pos | ||
|
|
||
|
|
||
| def test_entrance_slit_info_from_slit_position(): | ||
| slit_info = EntranceSlitInformation.from_slit_positions(SlitPosition.P850_3_HOLE) | ||
| assert slit_info.setting == 850 | ||
| assert slit_info.size == 3.0 | ||
| assert slit_info.shape == "hole" | ||
| assert slit_info.direction == "vertical" | ||
|
|
||
| slit_info = EntranceSlitInformation.from_slit_positions( | ||
| SlitPosition.P300_0_2_CURVED | ||
| ) | ||
| assert slit_info.setting == 300 | ||
| assert slit_info.size == 0.2 | ||
| assert slit_info.shape == "curved" | ||
| assert slit_info.direction == "vertical" | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def slit_info_device() -> EntranceSlitInformationDevice: | ||
| with init_devices(mock=True): | ||
| slit_info_device = EntranceSlitInformationDevice("TEST:") | ||
| return slit_info_device | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("slit_pos", [pos.value for pos in SlitPosition]) | ||
| async def test_slit_info_device_soft_signals_sync_with_epics( | ||
| slit_info_device: EntranceSlitInformationDevice, slit_pos: SlitPosition | ||
| ) -> None: | ||
| await slit_info_device.set(slit_pos) | ||
|
|
||
| slit_info = EntranceSlitInformation.from_slit_positions(slit_pos) | ||
| assert await slit_info_device.setting.get_value() == slit_info.setting | ||
| assert await slit_info_device.shape.get_value() == slit_info.shape | ||
| assert await slit_info_device.size.get_value() == slit_info.size | ||
| assert await slit_info_device.direction.get_value() == slit_info.direction | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("slit_pos", [pos.value for pos in SlitPosition]) | ||
| async def test_slit_info_device_read_and_soft_signals_sync_with_epics( | ||
| slit_info_device: EntranceSlitInformationDevice, slit_pos: SlitPosition | ||
| ) -> None: | ||
| await slit_info_device.set(slit_pos) | ||
| slit_info = EntranceSlitInformation.from_slit_positions(slit_pos) | ||
|
|
||
| await assert_reading( | ||
| slit_info_device, | ||
| { | ||
| "slit_info_device-size": partial_reading(slit_info.size), | ||
| "slit_info_device-shape": partial_reading(slit_info.shape), | ||
| "slit_info_device-setting": partial_reading(slit_info.setting), | ||
| "slit_info_device-direction": partial_reading(slit_info.direction), | ||
| }, | ||
| ) |
Uh oh!
There was an error while loading. Please reload this page.
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.
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?
Uh oh!
There was an error while loading. Please reload this page.
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.
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.