Convert mirror voltage devices to use ophyd async #636

rtuck99 · 2024-06-18T13:43:36Z

This converts the VFMMirrorVoltages device to use ophyd-async
Fixes #604

See also corresponding hyperion change TBD

Instructions to reviewer on how to test:

Tests pass
mirror voltage devices connect and experiment plans continue to work

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}

DominicOram · 2024-06-18T16:55:59Z

src/dodal/devices/focusing_mirror.py

+        async for accepted_value in observe_value(
+            demand_accepted, timeout=DEFAULT_SETTLE_TIME_S
+        ):
+            LOGGER.debug(
+                f"Current demand accepted = {accepted_value} for {setpoint_v.name}"
+            )
+            if not set_applied:
+                if accepted_value == MirrorVoltageDemand.OK:
+                    await setpoint_v.set(value)
+                    set_applied = True
+                else:
                    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
+            else:
+                if accepted_value != MirrorVoltageDemand.OK:
+                    LOGGER.debug(
+                        f"Demand not accepted for {setpoint_v.name}, waiting for acceptance..."
+                    )
+                else:
+                    LOGGER.debug(f"Demand accepted for {setpoint_v.name}")
+                    break


Why are we doing the set inside the loop? To try and avoid the race condition of missing the update on demand_accepted where it goes to OK? If so I think we can probably think of something slightly cleaner

DominicOram · 2024-06-18T16:59:56Z

tests/conftest.py

@@ -74,16 +73,10 @@ def pytest_runtest_teardown():

 @pytest.fixture
 def vfm_mirror_voltages() -> VFMMirrorVoltages:


Should: Given this is an ophyd_async device it needs a RE before being instantiated

codecov · 2024-06-19T14:38:54Z

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.43%. Comparing base (76be228) to head (e306c61).
Report is 35 commits behind head on main.

❗ Current head e306c61 differs from pull request most recent head e3af2a7

Please upload reports for the commit e3af2a7 to get more accurate results.

Files	Patch %	Lines
src/dodal/devices/focusing_mirror.py	94.11%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #636      +/-   ##
==========================================
+ Coverage   93.03%   93.43%   +0.40%     
==========================================
  Files         103      105       +2     
  Lines        3903     4083     +180     
==========================================
+ Hits         3631     3815     +184     
+ Misses        272      268       -4

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

DominicOram

Thank you, I think this is a lot cleaner. Some comments in code

DominicOram · 2024-06-21T10:36:41Z

src/dodal/devices/focusing_mirror.py

-                if subs_handle is None:
-                    raise AssertionError("Demand accepted before set attempted")
-                demand_accepted.unsubscribe(subs_handle)
+        it_values = observe_value(demand_accepted, timeout=DEFAULT_SETTLE_TIME_S)


Could: I think it_values could be better named maybe demand_accepted_iterator?

Should: I think it would be good to have a comment on why we're starting the observe early e.g. the race condition stuff

DominicOram · 2024-06-21T10:37:39Z

src/dodal/devices/focusing_mirror.py

-                demand_accepted.unsubscribe(subs_handle)
+        it_values = observe_value(demand_accepted, timeout=DEFAULT_SETTLE_TIME_S)
+        accepted_value = await anext(it_values)
+        if accepted_value == MirrorVoltageDemand.OK:


Should: We do this check at the start of this function, I think it's unlikely it will have changed so no need to check again

DominicOram · 2024-06-21T10:40:47Z

src/dodal/devices/focusing_mirror.py

+        while MirrorVoltageDemand.SLEW == (accepted_value := await anext(it_values)):
+            pass


Could: I'm 90% sure it's we won't get multiple of these so just the check above is fine. @coretl does observe_values guarantee you wont get multiple updates if the value is the same?

It passes the camonitor updates straight through, so as long as EPICS isn't squashing the updates (via MDEL) then you will get multiple updates

Hmm... Then we will need to always check it in a loop

DominicOram · 2024-06-21T10:42:42Z

tests/devices/unit_tests/test_focusing_mirror.py

-from unittest.mock import DEFAULT, MagicMock, patch
+import asyncio
+
+# prevent python 3.10 exception doppelganger stupidity


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

DominicOram · 2024-06-21T11:00:36Z

tests/devices/unit_tests/test_focusing_mirror.py

        )
        return DEFAULT

-    vfm_mirror_voltages._channel14_voltage_device._setpoint_v.set = MagicMock(
+    vfm_mirror_voltages.voltage_channels[0]._setpoint_v.set = AsyncMock(


Should: So set is already backed by a mock. you can do:

from ophyd_async.core import get_mock_put mock = get_mock_put(vfm_mirror_voltages.voltage_channels[0]._setpoint_v)

mock will already be a mock that you can play with and it should fix your type hinting later on

DominicOram

Thank you! Apologies, I think there is still a potential race, maybe I'm getting paranoid now but I feel like if this is the first time we do something like this it will be used as a model for other devices so we should be as accurate as possible.

DominicOram · 2024-06-27T11:41:08Z

src/dodal/devices/focusing_mirror.py

+        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)


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?

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.

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.

DominicOram · 2024-06-27T11:45:01Z

tests/devices/unit_tests/test_focusing_mirror.py

@@ -45,27 +56,36 @@ def vfm_mirror_voltages_with_set_accepted_fail(


 def vfm_mirror_voltages_with_set_to_value(
-    vfm_mirror_voltages, new_value: MirrorVoltageDemand
+    vfm_mirror_voltages, new_value: MirrorVoltageDemand, spins: int = 0


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

DominicOram

Great, thank you! Sorry for the back and forth, took a while to get my head round all the potential races.

DominicOram · 2024-07-11T09:47:42Z

src/dodal/devices/focusing_mirror.py

+        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)


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.

rtuck99 added 2 commits June 18, 2024 14:32

(#604) Convert focusing_mirror mirror voltages to ophyd_async

8792cff

(#604) Refactor VFMMirrorVoltages to use DeviceVector

908c409

rtuck99 changed the title ~~604 ophyd async device for mirror voltages~~ 604 convert mirror voltage devices to use ophyd async Jun 18, 2024

rtuck99 mentioned this pull request Jun 18, 2024

Changes to support ophyd_async for mirror voltages DiamondLightSource/hyperion#1453

Merged

Fix python 3.10 test failure

5532a53

rtuck99 marked this pull request as ready for review June 18, 2024 14:42

rtuck99 requested review from dperl-dls and DominicOram June 18, 2024 14:42

Make ruff happy

89fe883

DominicOram reviewed Jun 18, 2024

View reviewed changes

(#604) response to PR comments

e306c61

rtuck99 requested a review from DominicOram June 20, 2024 11:57

DominicOram requested changes Jun 21, 2024

View reviewed changes

(#604) Response to PR review comments

e3af2a7

rtuck99 requested a review from DominicOram June 27, 2024 09:43

DominicOram requested changes Jun 27, 2024

View reviewed changes

rtuck99 requested a review from DominicOram July 2, 2024 14:56

DominicOram approved these changes Jul 11, 2024

View reviewed changes

Merge branch 'main' into 604_ophyd_async_device_for_mirror_voltages

0a0a749

DominicOram changed the title ~~604 convert mirror voltage devices to use ophyd async~~ Convert mirror voltage devices to use ophyd async Jul 11, 2024

DominicOram merged commit 555f696 into main Jul 11, 2024
11 checks passed

DominicOram deleted the 604_ophyd_async_device_for_mirror_voltages branch July 11, 2024 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert mirror voltage devices to use ophyd async #636

Convert mirror voltage devices to use ophyd async #636

rtuck99 commented Jun 18, 2024

DominicOram Jun 18, 2024

DominicOram Jun 18, 2024

codecov bot commented Jun 19, 2024 •

edited

Loading

DominicOram left a comment

DominicOram Jun 21, 2024

DominicOram Jun 21, 2024

DominicOram Jun 21, 2024

DominicOram Jun 21, 2024

coretl Jun 21, 2024

DominicOram Jun 21, 2024

DominicOram Jun 21, 2024

DominicOram Jun 21, 2024

DominicOram left a comment

DominicOram Jun 27, 2024

rtuck99 Jul 2, 2024

DominicOram Jul 11, 2024

DominicOram Jun 27, 2024

DominicOram left a comment

DominicOram Jul 11, 2024

		@@ -74,16 +73,10 @@ def pytest_runtest_teardown():

		@pytest.fixture
		def vfm_mirror_voltages() -> VFMMirrorVoltages:

		while MirrorVoltageDemand.SLEW == (accepted_value := await anext(it_values)):
		pass

Convert mirror voltage devices to use ophyd async #636

Convert mirror voltage devices to use ophyd async #636

Conversation

rtuck99 commented Jun 18, 2024

Instructions to reviewer on how to test:

Checks for reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 19, 2024 • edited Loading

Codecov Report

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 19, 2024 •

edited

Loading