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

Convert mirror voltage devices to use ophyd async #636

Merged
merged 7 commits into from
Jul 11, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 62 additions & 62 deletions src/dodal/devices/focusing_mirror.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
from enum import Enum, IntEnum
from typing import Any

from ophyd import Component, Device, EpicsSignal
from ophyd.status import Status, StatusBase
from ophyd_async.core import StandardReadable
from enum import Enum

from ophyd_async.core import (
AsyncStatus,
Device,
DeviceVector,
StandardReadable,
observe_value,
)
from ophyd_async.core.signal import soft_signal_r_and_setter
from ophyd_async.epics.motion import Motor
from ophyd_async.epics.signal import (
epics_signal_r,
epics_signal_rw,
epics_signal_x,
)
@@ -32,93 +36,89 @@ class MirrorStripe(str, Enum):
PLATINUM = "Platinum"


class MirrorVoltageDemand(IntEnum):
N_A = 0
OK = 1
FAIL = 2
SLEW = 3
class MirrorVoltageDemand(str, Enum):
N_A = "N/A"
OK = "OK"
FAIL = "FAIL"
SLEW = "SLEW"


class MirrorVoltageDevice(Device):
"""Abstract the bimorph mirror voltage PVs into a single device that can be set asynchronously and returns when
the demanded voltage setpoint is accepted, without blocking the caller as this process can take significant time.
"""

_actual_v: EpicsSignal = Component(EpicsSignal, "R")
_setpoint_v: EpicsSignal = Component(EpicsSignal, "D")
_demand_accepted: EpicsSignal = Component(EpicsSignal, "DSEV")
def __init__(self, name: str = "", prefix: str = ""):
self._actual_v = epics_signal_r(int, prefix + "R")
self._setpoint_v = epics_signal_rw(int, prefix + "D")
self._demand_accepted = epics_signal_r(MirrorVoltageDemand, prefix + "DSEV")
super().__init__(name=name)

def set(self, value, *args, **kwargs) -> StatusBase:
@AsyncStatus.wrap
async def set(self, value, *args, **kwargs):
"""Combine the following operations into a single set:
1. apply the value to the setpoint PV
2. Return to the caller with a Status future
3. Wait until demand is accepted
4. When either demand is accepted or DEFAULT_SETTLE_TIME expires, signal the result on the Status
"""

setpoint_v = self._setpoint_v
demand_accepted = self._demand_accepted

if demand_accepted.get() != MirrorVoltageDemand.OK:
if await demand_accepted.get_value() != MirrorVoltageDemand.OK:
raise AssertionError(
f"Attempted to set {setpoint_v.name} when demand is not accepted."
)

if setpoint_v.get() == value:
if await setpoint_v.get_value() == value:
LOGGER.debug(f"{setpoint_v.name} already at {value} - skipping set")
return Status(success=True, done=True)
return

LOGGER.debug(f"setting {setpoint_v.name} to {value}")
demand_accepted_status = Status(self, DEFAULT_SETTLE_TIME_S)

subscription: dict[str, Any] = {"handle": None}

def demand_check_callback(old_value, value, **kwargs):
LOGGER.debug(f"Got event old={old_value} new={value} for {setpoint_v.name}")
if old_value != MirrorVoltageDemand.OK and value == MirrorVoltageDemand.OK:
LOGGER.debug(f"Demand accepted for {setpoint_v.name}")
subs_handle = subscription.pop("handle", None)
if subs_handle is None:
raise AssertionError("Demand accepted before set attempted")
demand_accepted.unsubscribe(subs_handle)

demand_accepted_status.set_finished()
# else timeout handled by parent demand_accepted_status
# Register an observer up front to ensure we don't miss events after we
# perform the set
demand_accepted_iterator = observe_value(
demand_accepted, timeout=DEFAULT_SETTLE_TIME_S
)
# discard the current value (OK) so we can await a subsequent change
await anext(demand_accepted_iterator)
await setpoint_v.set(value)

# The set should always change to SLEW regardless of whether we are
# already at the set point, then change back to OK/FAIL depending on
# success
accepted_value = await anext(demand_accepted_iterator)
assert accepted_value == MirrorVoltageDemand.SLEW
Comment on lines +85 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: Sorry, I'm wondering if there is another race condition here where if we get an update straight after the set we may still see an OK? I think a better solution might be not to discard the value before the set but to keep reading until the updates show slew?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did look at the code in ophyd_async before to confirm that we will always get the current value in the first event, so I don't think that there is any possibility of this race condition and we should be safe.

observe_value's implementation calls signal.subscribe_value(q.put_nowait)
before awaiting new values.
signal.subscribe() calls signal_notify() which calls our q.put_nowait() - this posts the event on to the queue which is then collected by the subsequent await.

If this implementation ever changes it will probably break calling code as anything that follows this model under this assumption will end up blocking waiting for an event that never comes.

I'm also slightly confused where you think the extra event will come from - we've already validated that the demand will be OK prior to entering the function and we should be the only ones applying changes. The discard from the first anext() is before the set, so there is no way that the discarded value can be "SLEW" or a subsequent "OK", even if the current value wasn't pushed by the call to observe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tom's comment #636 (comment) implies that it is possible we get multiple updates with the same value. It depends quite a bit on how the IOC is written, for example it's possible that the PV will "send out" an update on a periodic basis regardless of if the value has changed or not. I think you're right that I'm being overly cautious though, looking at the PV it seems that it should be stable.

LOGGER.debug(
f"Demand not accepted for {setpoint_v.name}, waiting for acceptance..."
)
while MirrorVoltageDemand.SLEW == (
accepted_value := await anext(demand_accepted_iterator)
):
pass

subscription["handle"] = demand_accepted.subscribe(demand_check_callback)
setpoint_status = setpoint_v.set(value)
status = setpoint_status & demand_accepted_status
return status
if accepted_value != MirrorVoltageDemand.OK:
raise AssertionError(
f"Voltage slew failed for {setpoint_v.name}, new state={accepted_value}"
)


class VFMMirrorVoltages(Device):
def __init__(self, *args, daq_configuration_path: str, **kwargs):
super().__init__(*args, **kwargs)
class VFMMirrorVoltages(StandardReadable):
def __init__(
self, name: str, prefix: str, *args, daq_configuration_path: str, **kwargs
):
self.voltage_lookup_table_path = (
daq_configuration_path + "/json/mirrorFocus.json"
)

_channel14_voltage_device = Component(MirrorVoltageDevice, "BM:V14")
_channel15_voltage_device = Component(MirrorVoltageDevice, "BM:V15")
_channel16_voltage_device = Component(MirrorVoltageDevice, "BM:V16")
_channel17_voltage_device = Component(MirrorVoltageDevice, "BM:V17")
_channel18_voltage_device = Component(MirrorVoltageDevice, "BM:V18")
_channel19_voltage_device = Component(MirrorVoltageDevice, "BM:V19")
_channel20_voltage_device = Component(MirrorVoltageDevice, "BM:V20")
_channel21_voltage_device = Component(MirrorVoltageDevice, "BM:V21")

@property
def voltage_channels(self) -> list[MirrorVoltageDevice]:
return [
self._channel14_voltage_device,
self._channel15_voltage_device,
self._channel16_voltage_device,
self._channel17_voltage_device,
self._channel18_voltage_device,
self._channel19_voltage_device,
self._channel20_voltage_device,
self._channel21_voltage_device,
]
with self.add_children_as_readables():
self.voltage_channels = DeviceVector(
{
i - 14: MirrorVoltageDevice(prefix=f"{prefix}BM:V{i}")
for i in range(14, 22)
}
)
super().__init__(*args, name=name, **kwargs)


class FocusingMirror(StandardReadable):
17 changes: 5 additions & 12 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -6,12 +6,11 @@
import time
from os import environ, getenv
from pathlib import Path
from typing import Mapping, cast
from typing import Mapping
from unittest.mock import MagicMock, patch

import pytest
from bluesky.run_engine import RunEngine
from ophyd.sim import make_fake_device

from dodal.beamlines import i03
from dodal.common.beamlines import beamline_utils
@@ -73,17 +72,11 @@ def pytest_runtest_teardown():


@pytest.fixture
def vfm_mirror_voltages() -> VFMMirrorVoltages:
voltages = cast(
VFMMirrorVoltages,
make_fake_device(VFMMirrorVoltages)(
name="vfm_mirror_voltages",
prefix="BL-I03-MO-PSU-01:",
daq_configuration_path=i03.DAQ_CONFIGURATION_PATH,
),
)
def vfm_mirror_voltages(RE: RunEngine) -> VFMMirrorVoltages:
voltages = i03.vfm_mirror_voltages(fake_with_ophyd_sim=True)
voltages.voltage_lookup_table_path = "tests/test_data/test_mirror_focus.json"
return voltages
yield voltages
beamline_utils.clear_devices()


s03_epics_server_port = getenv("S03_EPICS_CA_SERVER_PORT")
249 changes: 188 additions & 61 deletions tests/devices/unit_tests/test_focusing_mirror.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
from threading import Timer
from unittest.mock import DEFAULT, MagicMock, patch
import asyncio

# prevent python 3.10 exception doppelganger stupidity
Copy link
Contributor

Choose a reason for hiding this comment

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

Could: A link to somewhere that explains this issue would be good. It's not clear what this means

# see https://docs.python.org/3.10/library/asyncio-exceptions.html
# https://github.com/python/cpython/issues?q=is%3Aissue+timeouterror++alias+
from asyncio import TimeoutError
from unittest.mock import DEFAULT, patch

import pytest
from ophyd.sim import NullStatus
from ophyd.status import Status, StatusBase
from bluesky import FailedStatus, RunEngine
from bluesky import plan_stubs as bps
from ophyd_async.core import get_mock_put, set_mock_value

from dodal.devices.focusing_mirror import (
FocusingMirrorWithStripes,
@@ -12,121 +18,242 @@
MirrorVoltageDevice,
VFMMirrorVoltages,
)
from dodal.log import LOGGER


@pytest.fixture
def vfm_mirror_voltages_not_ok(vfm_mirror_voltages) -> VFMMirrorVoltages:
vfm_mirror_voltages._channel14_voltage_device._demand_accepted.sim_put(
MirrorVoltageDemand.FAIL
set_mock_value(
vfm_mirror_voltages.voltage_channels[0]._demand_accepted,
MirrorVoltageDemand.FAIL,
)
return vfm_mirror_voltages


@pytest.fixture
def vfm_mirror_voltages_with_set(vfm_mirror_voltages) -> VFMMirrorVoltages:
def not_ok_then_ok(_):
vfm_mirror_voltages._channel14_voltage_device._demand_accepted.sim_put(
MirrorVoltageDemand.SLEW
return vfm_mirror_voltages_with_set_to_value(
vfm_mirror_voltages, MirrorVoltageDemand.OK
)


@pytest.fixture
def vfm_mirror_voltages_with_set_multiple_spins(
vfm_mirror_voltages,
) -> VFMMirrorVoltages:
return vfm_mirror_voltages_with_set_to_value(
vfm_mirror_voltages, MirrorVoltageDemand.OK, 3
)


@pytest.fixture
def vfm_mirror_voltages_with_set_accepted_fail(
vfm_mirror_voltages,
) -> VFMMirrorVoltages:
return vfm_mirror_voltages_with_set_to_value(
vfm_mirror_voltages, MirrorVoltageDemand.FAIL
)


def vfm_mirror_voltages_with_set_to_value(
vfm_mirror_voltages, new_value: MirrorVoltageDemand, spins: int = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could: spins isn't that obvious a name. Maybe number_of_slew_updates?

) -> VFMMirrorVoltages:
async def set_demand_accepted_after_delay():
await asyncio.sleep(0.1)
nonlocal spins
if spins > 0:
set_mock_value(
vfm_mirror_voltages.voltage_channels[0]._demand_accepted,
MirrorVoltageDemand.SLEW,
)
spins -= 1
asyncio.create_task(set_demand_accepted_after_delay())
else:
set_mock_value(
vfm_mirror_voltages.voltage_channels[0]._demand_accepted,
new_value,
)
LOGGER.debug("DEMAND ACCEPTED OK")

def not_ok_then_other_value(*args, **kwargs):
set_mock_value(
vfm_mirror_voltages.voltage_channels[0]._demand_accepted,
MirrorVoltageDemand.SLEW,
)
Timer(
0.1,
lambda: vfm_mirror_voltages._channel14_voltage_device._demand_accepted.sim_put(
MirrorVoltageDemand.OK
),
).start()
asyncio.create_task(set_demand_accepted_after_delay())
return DEFAULT

vfm_mirror_voltages._channel14_voltage_device._setpoint_v.set = MagicMock(
side_effect=not_ok_then_ok
)
vfm_mirror_voltages._channel14_voltage_device._demand_accepted.sim_put(
MirrorVoltageDemand.OK
get_mock_put(
vfm_mirror_voltages.voltage_channels[0]._setpoint_v
).side_effect = not_ok_then_other_value
set_mock_value(
vfm_mirror_voltages.voltage_channels[0]._demand_accepted,
MirrorVoltageDemand.OK,
)
return vfm_mirror_voltages


@pytest.fixture
def vfm_mirror_voltages_with_set_timing_out(vfm_mirror_voltages) -> VFMMirrorVoltages:
def not_ok(_):
vfm_mirror_voltages._channel14_voltage_device._demand_accepted.sim_put(
MirrorVoltageDemand.SLEW
def not_ok(*args, **kwargs):
set_mock_value(
vfm_mirror_voltages.voltage_channels[0]._demand_accepted,
MirrorVoltageDemand.SLEW,
)
return DEFAULT

vfm_mirror_voltages._channel14_voltage_device._setpoint_v.set = MagicMock(
side_effect=not_ok
)
vfm_mirror_voltages._channel14_voltage_device._demand_accepted.sim_put(
MirrorVoltageDemand.OK
get_mock_put(
vfm_mirror_voltages.voltage_channels[0]._setpoint_v
).side_effect = not_ok
set_mock_value(
vfm_mirror_voltages.voltage_channels[0]._demand_accepted,
MirrorVoltageDemand.OK,
)
return vfm_mirror_voltages


def test_mirror_set_voltage_sets_and_waits_happy_path(
RE: RunEngine,
vfm_mirror_voltages_with_set: VFMMirrorVoltages,
):
vfm_mirror_voltages_with_set._channel14_voltage_device._setpoint_v.set.return_value = NullStatus()
vfm_mirror_voltages_with_set._channel14_voltage_device._demand_accepted.sim_put(
MirrorVoltageDemand.OK
async def completed():
pass

mock_put = get_mock_put(
vfm_mirror_voltages_with_set.voltage_channels[0]._setpoint_v
)
mock_put.return_value = completed()
set_mock_value(
vfm_mirror_voltages_with_set.voltage_channels[0]._demand_accepted,
MirrorVoltageDemand.OK,
)

status: StatusBase = vfm_mirror_voltages_with_set.voltage_channels[0].set(100)
status.wait()
vfm_mirror_voltages_with_set._channel14_voltage_device._setpoint_v.set.assert_called_with(
100
def plan():
yield from bps.abs_set(
vfm_mirror_voltages_with_set.voltage_channels[0], 100, wait=True
)

RE(plan())

mock_put.assert_called_with(100, wait=True, timeout=10.0)


def test_mirror_set_voltage_sets_and_waits_happy_path_spin_while_waiting_for_slew(
RE: RunEngine,
vfm_mirror_voltages_with_set_multiple_spins: VFMMirrorVoltages,
):
async def completed():
pass

mock_put = get_mock_put(
vfm_mirror_voltages_with_set_multiple_spins.voltage_channels[0]._setpoint_v
)
mock_put.return_value = completed()
set_mock_value(
vfm_mirror_voltages_with_set_multiple_spins.voltage_channels[
0
]._demand_accepted,
MirrorVoltageDemand.OK,
)
assert status.success

def plan():
yield from bps.abs_set(
vfm_mirror_voltages_with_set_multiple_spins.voltage_channels[0],
100,
wait=True,
)

RE(plan())

mock_put.assert_called_with(100, wait=True, timeout=10.0)


def test_mirror_set_voltage_set_rejected_when_not_ok(
RE: RunEngine,
vfm_mirror_voltages_not_ok: VFMMirrorVoltages,
):
with pytest.raises(AssertionError):
vfm_mirror_voltages_not_ok.voltage_channels[0].set(100)
def plan():
with pytest.raises(FailedStatus) as e:
yield from bps.abs_set(
vfm_mirror_voltages_not_ok.voltage_channels[0], 100, wait=True
)

assert isinstance(e.value.args[0].exception(), AssertionError)

RE(plan())


def test_mirror_set_voltage_sets_and_waits_set_fail(
RE: RunEngine,
vfm_mirror_voltages_with_set: VFMMirrorVoltages,
):
vfm_mirror_voltages_with_set._channel14_voltage_device._setpoint_v.set.return_value = Status(
success=False, done=True
)
def failed(*args, **kwargs):
raise AssertionError("Test Failure")

get_mock_put(
vfm_mirror_voltages_with_set.voltage_channels[0]._setpoint_v
).side_effect = failed

status: StatusBase = vfm_mirror_voltages_with_set.voltage_channels[0].set(100)
with pytest.raises(Exception):
status.wait()
def plan():
with pytest.raises(FailedStatus) as e:
yield from bps.abs_set(
vfm_mirror_voltages_with_set.voltage_channels[0], 100, wait=True
)

assert not status.success
assert isinstance(e.value.args[0].exception(), AssertionError)

RE(plan())


def test_mirror_set_voltage_sets_and_waits_demand_accepted_fail(
RE: RunEngine, vfm_mirror_voltages_with_set_accepted_fail
):
def plan():
with pytest.raises(FailedStatus) as e:
yield from bps.abs_set(
vfm_mirror_voltages_with_set_accepted_fail.voltage_channels[0],
100,
wait=True,
)

assert isinstance(e.value.args[0].exception(), AssertionError)

RE(plan())


@patch("dodal.devices.focusing_mirror.DEFAULT_SETTLE_TIME_S", 3)
def test_mirror_set_voltage_sets_and_waits_settle_timeout_expires(
RE: RunEngine,
vfm_mirror_voltages_with_set_timing_out: VFMMirrorVoltages,
):
vfm_mirror_voltages_with_set_timing_out._channel14_voltage_device._setpoint_v.set.return_value = NullStatus()

status: StatusBase = vfm_mirror_voltages_with_set_timing_out.voltage_channels[
0
].set(100)

with pytest.raises(Exception) as excinfo:
status.wait()
def plan():
with pytest.raises(Exception) as excinfo:
yield from bps.abs_set(
vfm_mirror_voltages_with_set_timing_out.voltage_channels[0],
100,
wait=True,
)
assert isinstance(excinfo.value.args[0].exception(), TimeoutError)

# Cannot assert because ophyd discards the original exception
# assert isinstance(excinfo.value, WaitTimeoutError)
assert excinfo.value
RE(plan())


def test_mirror_set_voltage_returns_immediately_if_voltage_already_demanded(
RE: RunEngine,
vfm_mirror_voltages_with_set: VFMMirrorVoltages,
):
vfm_mirror_voltages_with_set._channel14_voltage_device._setpoint_v.sim_put(100)
set_mock_value(vfm_mirror_voltages_with_set.voltage_channels[0]._setpoint_v, 100)

def plan():
yield from bps.abs_set(
vfm_mirror_voltages_with_set.voltage_channels[0], 100, wait=True
)

status: StatusBase = vfm_mirror_voltages_with_set.voltage_channels[0].set(100)
status.wait()
RE(plan())

assert status.success
vfm_mirror_voltages_with_set._channel14_voltage_device._setpoint_v.set.assert_not_called()
get_mock_put(
vfm_mirror_voltages_with_set.voltage_channels[0]._setpoint_v
).assert_not_called()


def test_mirror_populates_voltage_channels(