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.

Sign up for GitHub

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

Add aperture position readbacks #385 #395

Merged
merged 6 commits into from
Mar 20, 2024
Merged

Add aperture position readbacks #385 #395

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
00b2c29
Add aperture position readbacks #385
katesmith280 Mar 19, 2024
a45a6f2
385 add robot load position and tolerance
katesmith280 Mar 20, 2024
6a9ed0b
#385 abstracted robot load to config and added tolerance test
katesmith280 Mar 20, 2024
39357f2
Use abstraction instead of hard coded 31.4 for robot load tests
katesmith280 Mar 20, 2024
67b7152
Remove hard coded values from robot load tests
katesmith280 Mar 20, 2024
2b45300
Update src/dodal/devices/aperturescatterguard.py
katesmith280 Mar 20, 2024
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
5 changes: 4 additions & 1 deletion src/dodal/devices/aperture.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from ophyd import Component, Device
from ophyd import Component, Device, EpicsSignalRO

from dodal.devices.util.motor_utils import ExtendedEpicsMotor

Expand All @@ -7,3 +7,6 @@ class Aperture(Device):
x = Component(ExtendedEpicsMotor, "X")
y = Component(ExtendedEpicsMotor, "Y")
z = Component(ExtendedEpicsMotor, "Z")
small = Component(EpicsSignalRO, "Y:SMALL_CALC")
medium = Component(EpicsSignalRO, "Y:MEDIUM_CALC")
large = Component(EpicsSignalRO, "Y:LARGE_CALC")
30 changes: 16 additions & 14 deletions src/dodal/devices/aperturescatterguard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from functools import reduce
from typing import List, Optional, Sequence

import numpy as np
from ophyd import Component as Cpt
from ophyd import SignalRO
from ophyd.epics_motor import EpicsMotor
Expand Down Expand Up @@ -93,7 +92,7 @@ class ApertureScatterguard(InfoLoggingDevice):
class SelectedAperture(SignalRO):
def get(self):
assert isinstance(self.parent, ApertureScatterguard)
return self.parent._get_closest_position_to_current()
return self.parent._get_current_aperture_position()

selected_aperture = Cpt(SelectedAperture)

Expand Down Expand Up @@ -123,22 +122,25 @@ def _set_raw_unsafe(self, positions: ApertureFiveDimensionalLocation) -> AndStat
operator.and_, [motor.set(pos) for motor, pos in zip(motors, positions)]
)

def _get_closest_position_to_current(self) -> SingleAperturePosition:
def _get_current_aperture_position(self) -> SingleAperturePosition:
"""
Returns the closest valid position to current position within {TOLERANCE_STEPS}.
Returns the current aperture position using readback values
for SMALL, MEDIUM, LARGE. ROBOT_LOAD position defined when
mini aperture y <= ROBOT_LOAD.location.aperture_y + tolerance.
If no position is found then raises InvalidApertureMove.
"""
assert isinstance(self.aperture_positions, AperturePositions)
for aperture in self.aperture_positions.as_list():
aperture_in_tolerence = []
motors = self._get_motor_list()
for motor, test_position in zip(motors, list(aperture.location)):
current_position = motor.user_readback.get()
tolerance = self.TOLERANCE_STEPS * motor.motor_resolution.get()
diff = abs(current_position - test_position)
aperture_in_tolerence.append(diff <= tolerance)
if np.all(aperture_in_tolerence):
return aperture
current_ap_y = float(self.aperture.y.user_readback.get())
robot_load_ap_y = self.aperture_positions.ROBOT_LOAD.location.aperture_y
tolerance = self.TOLERANCE_STEPS * self.aperture.y.motor_resolution.get()
if int(self.aperture.large.get()) == 1:
return self.aperture_positions.LARGE
elif int(self.aperture.medium.get()) == 1:
return self.aperture_positions.MEDIUM
elif int(self.aperture.small.get()) == 1:
return self.aperture_positions.SMALL
elif current_ap_y <= +robot_load_ap_y + tolerance:
katesmith280 marked this conversation as resolved.
Show resolved Hide resolved
return self.aperture_positions.ROBOT_LOAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: I feel that the robot load position should still have some tolerance on it - probably the same MRES * some n steps as before - otherwise we'll still end up with similar issues where the state is unrecognised when the current_ap_y is just a hair short of the canonical ROBOT_LOAD position. Since this position is well out of the way of collisions we can probably increase the default number of steps to 5 or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I suggested this logic to @katesmith280. I'm happy with a tolerance or without given that it's just a less than at the moment (so has some tolerance built in). If you think tolerance improves it then that's fine by me

Copy link
Collaborator Author

@katesmith280 katesmith280 Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I pushed a new change that decoupled the the robot load position (31.4mm) and the permitted tolerance (5 mm). So the aperture position value ROBOT_LOAD now has a bit more meaning, i.e. position plus tolerance.

I may not have done this in the most elegant way so any improvemens are greatly appreciated

Copy link
Contributor

@d-perl d-perl Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@DominicOram Oh, sorry. Yeah it's not critical, we could always address it later if it causes problems - I did see the <= but was just worried that that tolerance was only one-sided and won't work if the move to robot load stopped slightly before the position, so I do think it's more robust with that in.

@katesmith280 great, I'll have a look now


raise InvalidApertureMove("Current aperture/scatterguard state unrecognised")

Expand Down
72 changes: 56 additions & 16 deletions tests/devices/unit_tests/test_aperture_scatterguard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def aperture_in_medium_pos(
ap_sg.aperture.z.set(medium.aperture_z)
ap_sg.scatterguard.x.set(medium.scatterguard_x)
ap_sg.scatterguard.y.set(medium.scatterguard_y)
ap_sg.aperture.medium.sim_put(1) # type: ignore
yield ap_sg


Expand Down Expand Up @@ -175,32 +176,71 @@ def set_underlying_motors(
ap_sg.scatterguard.y.set(position.scatterguard_y)


def test_aperture_positions_get_close_position_truthy_exact(
def test_aperture_positions_large(
ap_sg: ApertureScatterguard, aperture_positions: AperturePositions
):
should_be_large = ApertureFiveDimensionalLocation(2.389, 40.986, 15.8, 5.25, 4.43)
set_underlying_motors(ap_sg, should_be_large)
ap_sg.aperture.large.sim_put(1) # type: ignore
assert ap_sg._get_current_aperture_position() == aperture_positions.LARGE

assert ap_sg._get_closest_position_to_current() == aperture_positions.LARGE

def test_aperture_positions_medium(
ap_sg: ApertureScatterguard, aperture_positions: AperturePositions
):
ap_sg.aperture.medium.sim_put(1) # type: ignore
assert ap_sg._get_current_aperture_position() == aperture_positions.MEDIUM


def test_aperture_positions_get_close_position_truthy_inside_tolerance(
def test_aperture_positions_small(
ap_sg: ApertureScatterguard, aperture_positions: AperturePositions
):
should_be_large = ApertureFiveDimensionalLocation(2.389, 40.9865, 15.8, 5.25, 4.43)
set_underlying_motors(ap_sg, should_be_large)
assert ap_sg._get_closest_position_to_current() == aperture_positions.LARGE
ap_sg.aperture.small.sim_put(1) # type: ignore
assert ap_sg._get_current_aperture_position() == aperture_positions.SMALL


def test_aperture_positions_get_close_position_falsy(
def test_aperture_positions_robot_load(
ap_sg: ApertureScatterguard, aperture_positions: AperturePositions
):
large_missed_by_2_at_y = ApertureFiveDimensionalLocation(
2.389, 42, 15.8, 5.25, 4.43
)
set_underlying_motors(ap_sg, large_missed_by_2_at_y)
ap_sg.aperture.large.sim_put(0) # type: ignore
ap_sg.aperture.medium.sim_put(0) # type: ignore
ap_sg.aperture.small.sim_put(0) # type: ignore
ap_sg.aperture.y.set(aperture_positions.ROBOT_LOAD.location.aperture_y) # type: ignore
assert ap_sg._get_current_aperture_position() == aperture_positions.ROBOT_LOAD


def test_aperture_positions_robot_load_within_tolerance(
ap_sg: ApertureScatterguard, aperture_positions: AperturePositions
):
robot_load_ap_y = aperture_positions.ROBOT_LOAD.location.aperture_y
tolerance = ap_sg.TOLERANCE_STEPS * ap_sg.aperture.y.motor_resolution.get()
ap_sg.aperture.large.sim_put(0) # type: ignore
ap_sg.aperture.medium.sim_put(0) # type: ignore
ap_sg.aperture.small.sim_put(0) # type: ignore
ap_sg.aperture.y.set(robot_load_ap_y + tolerance) # type: ignore
assert ap_sg._get_current_aperture_position() == aperture_positions.ROBOT_LOAD


def test_aperture_positions_robot_load_outside_tolerance(
ap_sg: ApertureScatterguard, aperture_positions: AperturePositions
):
robot_load_ap_y = aperture_positions.ROBOT_LOAD.location.aperture_y
tolerance = (ap_sg.TOLERANCE_STEPS + 1) * ap_sg.aperture.y.motor_resolution.get()
ap_sg.aperture.large.sim_put(0) # type: ignore
ap_sg.aperture.medium.sim_put(0) # type: ignore
ap_sg.aperture.small.sim_put(0) # type: ignore
ap_sg.aperture.y.set(robot_load_ap_y + tolerance) # type: ignore
with pytest.raises(InvalidApertureMove):
ap_sg._get_closest_position_to_current()
ap_sg._get_current_aperture_position()


def test_aperture_positions_robot_load_unsafe(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: a test similar to these two for the above tolerance would be good

ap_sg: ApertureScatterguard, aperture_positions: AperturePositions
):
ap_sg.aperture.large.sim_put(0) # type: ignore
ap_sg.aperture.medium.sim_put(0) # type: ignore
ap_sg.aperture.small.sim_put(0) # type: ignore
ap_sg.aperture.y.set(50.0) # type: ignore
with pytest.raises(InvalidApertureMove):
ap_sg._get_current_aperture_position()


def test_given_aperture_not_set_through_device_but_motors_in_position_when_device_read_then_position_returned(
Expand All @@ -216,12 +256,12 @@ def test_given_aperture_not_set_through_device_but_motors_in_position_when_devic
def test_when_aperture_set_and_device_read_then_position_returned(
aperture_in_medium_pos: ApertureScatterguard, aperture_positions: AperturePositions
):
set_status = aperture_in_medium_pos.set(aperture_positions.SMALL)
set_status = aperture_in_medium_pos.set(aperture_positions.MEDIUM)
set_status.wait()
selected_aperture = aperture_in_medium_pos.read()
assert (
selected_aperture["test_ap_sg_selected_aperture"]["value"]
== aperture_positions.SMALL
== aperture_positions.MEDIUM
)


Expand Down
Loading