Tidy apple2 undulator #1002

DominicOram · 2025-01-23T17:18:13Z

Fixes #999

This didn't end up removing as much code as I expected but I do think it's more maintainable as it contains less repetition.

Instructions to reviewer on how to test:

Confirm tests still pass and behaviour unchanged

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 · 2025-01-23T17:19:46Z

src/dodal/devices/apple2_undulator.py

        with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL):
-            self.user_setpoint_readback = epics_signal_r(float, fullPV + ".RBV")
+            self.user_setpoint_readback = epics_signal_r(float, motor_PV + ".RBV")


@Relm-Arrowny I could instead make everything use user_readback but this changes the name of the signal when it is read() I wasn't sure if the name was important so left it.

I think it should be safe to change it to user_readback in fact it is better if we inherit from motor but I think super need to goes before the signal definition otherwise when we call the super class constructor it will override the user_setpoint we define.

Now that I am typing away I remember why I did not use motor, The main reason is I do not want them to be stoppable as we are not meant to try to stop the id once they are moving. I was also worry that leaving movable/locatable in there where they would not function as intended, as we have this strange relationship of user_setpoint return done immediately and nothing happen until we say go.
May be we should call this UndulatorPhaseIO/signals instead?

Ok, if the motor bit is the controversial one I'll revert that and make a follow up issue

codecov · 2025-01-23T17:19:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.66%. Comparing base (f370749) to head (5812239).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1002      +/-   ##
==========================================
- Coverage   97.67%   97.66%   -0.02%     
==========================================
  Files         159      159              
  Lines        6590     6560      -30     
==========================================
- Hits         6437     6407      -30     
  Misses        153      153

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

Relm-Arrowny

Thanks for doing this, it looks good, there are only one part I am not sure about.

Relm-Arrowny · 2025-01-24T10:27:36Z

src/dodal/devices/apple2_undulator.py

        with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL):
-            self.user_setpoint_readback = epics_signal_r(float, fullPV + ".RBV")
+            self.user_setpoint_readback = epics_signal_r(float, motor_PV + ".RBV")


I think it should be safe to change it to user_readback in fact it is better if we inherit from motor but I think super need to goes before the signal definition otherwise when we call the super class constructor it will override the user_setpoint we define.

Now that I am typing away I remember why I did not use motor, The main reason is I do not want them to be stoppable as we are not meant to try to stop the id once they are moving. I was also worry that leaving movable/locatable in there where they would not function as intended, as we have this strange relationship of user_setpoint return done immediately and nothing happen until we say go.
May be we should call this UndulatorPhaseIO/signals instead?

Relm-Arrowny · 2025-01-30T16:50:18Z

src/dodal/devices/apple2_undulator.py



-class UndulatorPhaseMotor(StandardReadable):
+class UndulatorPhaseMotor(Motor):


Could: Inherit from motor here mean we will have stopable which is something we are told not to do with our id. Furthermore, movable/locatable will not function correctly, So it probably better if we not make this motor but instead change the name and call it UndulatorPhaseIO(Device)?

Relm-Arrowny · 2025-01-30T16:51:05Z

src/dodal/devices/apple2_undulator.py

        self.user_setpoint = epics_signal_w(str, fullPV + "SET")
        self.user_setpoint_demand_readback = epics_signal_r(float, fullPV + "DMD")

-        fullPV = fullPV + "MTR"
+        motor_PV = fullPV + "MTR"
        with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL):
-            self.user_setpoint_readback = epics_signal_r(float, fullPV + ".RBV")
+            self.user_setpoint_readback = epics_signal_r(float, motor_PV + ".RBV")


Must: If we were to use motor, I think this

super().__init__(motor_PV, name)

need to goes before:

self.user_setpoint = epics_signal_w(str, fullPV + "SET") self.user_setpoint_demand_readback = epics_signal_r(float, fullPV + "DMD")

Otherwise it get override by Motor.

Isn't super().init() what automatically names the children after connecting: would the children still get named in this order?

Isn't super().init() what automatically names the children after connecting: would the children still get named in this order?

I am not sure now that you question this...... but last time I try inherit motor and change the signal inside Motor something like this would happens, I definite

self.user_setpoint = epics_signal_w(str, fullPV + "SET")

and call super().init() and user_setpoint get replace by:

Motor.user_setpoint = epics_signal_rw(float, prefix + ".VAL")

Yes, it would make sense that the call to super replaces it. I will work out the exact behaviour and may make a ophyd-async issue about it.

Relm-Arrowny

looks good thanks.

DominicOram · 2025-02-03T17:35:54Z

Sorry @DiamondJoseph, need a Bluesky Reviewer and you already commented on here...

DiamondJoseph

Some nits around typing, I need to know about how the jaw_phase set signal works before I'll approve, and I'm not completely happy about passing both strings and signals around.

Basically looks there though.

src/dodal/devices/apple2_undulator.py

DiamondJoseph · 2025-02-03T17:53:25Z

src/dodal/devices/apple2_undulator.py

+async def estimate_motor_timeout(
+    setpoint: SignalR, curr_pos: SignalR, velocity: SignalR
+):
+    vel = await velocity.get_value()
+    cur_pos = await curr_pos.get_value()
+    target_pos = float(await setpoint.get_value())
+    return abs((target_pos - cur_pos) * 2.0 / vel) + 1


I presume this is duplicated for motor- can we have a ticket for the behaviour that we want out of Motor to be isolated in ophyd-async, to at least document why this has ended up here for the future, with the ticket linked as a comment here?

I think this comes back to the discussion of whether we inherit from Motor etc. I will make a new issue

src/dodal/devices/apple2_undulator.py

Co-authored-by: Joseph Ware <53935796+DiamondJoseph@users.noreply.github.com>

DominicOram added 4 commits January 22, 2025 19:04

Use a common base class for checking undulator status

5f0c3a2

Harmonise undulator timeout calculations

09f5672

Move more common things to SafeUndulatorMover

34fc792

Reuse the ophyd_async motor for undulator weird motor

9aeb5e5

DominicOram requested a review from Relm-Arrowny January 23, 2025 17:18

DominicOram requested a review from a team as a code owner January 23, 2025 17:18

DominicOram commented Jan 23, 2025

View reviewed changes

Remove pass statements that upset codecov

9e9701c

Relm-Arrowny mentioned this pull request Jan 29, 2025

Removes/reduces refenece use on i10 IDs #1032

Draft

4 tasks

Relm-Arrowny requested changes Jan 30, 2025

View reviewed changes

Revert UndulatorPhaseMotor to not be based on Motor

03d8367

DominicOram requested a review from Relm-Arrowny February 3, 2025 12:15

Relm-Arrowny approved these changes Feb 3, 2025

View reviewed changes

DominicOram requested a review from DiamondJoseph February 3, 2025 17:35

DiamondJoseph requested changes Feb 3, 2025

View reviewed changes

Add default name to SafeUndulatorMover

a72815b

Co-authored-by: Joseph Ware <53935796+DiamondJoseph@users.noreply.github.com>

DominicOram mentioned this pull request Feb 6, 2025

Update bluesky version #1041

Open

Add a bit more typing

fdd1789

DiamondJoseph approved these changes Feb 6, 2025

View reviewed changes

DominicOram added 2 commits February 6, 2025 15:04

Merge branch 'main' into 999_tidy_apple2_undulator

aeded8b

Add cast back in

b971704

DominicOram requested a review from DiamondJoseph February 6, 2025 15:32

Merge branch 'main' into 999_tidy_apple2_undulator

5812239

DiamondJoseph approved these changes Feb 6, 2025

View reviewed changes

DominicOram merged commit 3f6af7a into main Feb 6, 2025
19 checks passed

DominicOram deleted the 999_tidy_apple2_undulator branch February 6, 2025 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy apple2 undulator #1002

Tidy apple2 undulator #1002

DominicOram commented Jan 23, 2025 •

edited by Relm-Arrowny

Loading

DominicOram Jan 23, 2025

Relm-Arrowny Jan 24, 2025

DominicOram Feb 3, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

Relm-Arrowny left a comment

Relm-Arrowny Jan 24, 2025

Relm-Arrowny Jan 30, 2025

Relm-Arrowny Jan 30, 2025

DiamondJoseph Jan 30, 2025

Relm-Arrowny Jan 30, 2025 •

edited

Loading

DominicOram Feb 3, 2025

Relm-Arrowny left a comment

DominicOram commented Feb 3, 2025

DiamondJoseph left a comment

DiamondJoseph Feb 3, 2025

DominicOram Feb 6, 2025



		class UndulatorPhaseMotor(StandardReadable):
		class UndulatorPhaseMotor(Motor):

Tidy apple2 undulator #1002

Tidy apple2 undulator #1002

Conversation

DominicOram commented Jan 23, 2025 • edited by Relm-Arrowny Loading

Instructions to reviewer on how to test:

Checks for reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

Relm-Arrowny 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

Relm-Arrowny Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Relm-Arrowny left a comment

Choose a reason for hiding this comment

DominicOram commented Feb 3, 2025

DiamondJoseph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram commented Jan 23, 2025 •

edited by Relm-Arrowny

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading

Relm-Arrowny Jan 30, 2025 •

edited

Loading