Add aperture position readbacks #385 #395
Conversation
|
Adding @dperl-dls as a reviewer too as I don't have lots of time at the moment. We'll see who gets to it first though |
| elif int(self.aperture.small.get()) == 1: | ||
| return self.aperture_positions.SMALL | ||
| elif current_ap_y <= self.ROBOT_LOAD_Y: | ||
| return self.aperture_positions.ROBOT_LOAD |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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
| assert ap_sg._get_current_aperture_position() == aperture_positions.ROBOT_LOAD | ||
|
|
||
|
|
||
| def test_aperture_positions_robot_load_unsafe( |
There was a problem hiding this comment.
Should: a test similar to these two for the above tolerance would be good
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #395 +/- ##
=======================================
Coverage 93.45% 93.46%
=======================================
Files 90 90
Lines 3377 3380 +3
=======================================
+ Hits 3156 3159 +3
Misses 221 221 ☔ View full report in Codecov by Sentry. |
| @@ -89,11 +88,13 @@ class ApertureScatterguard(InfoLoggingDevice): | |||
| scatterguard = Cpt(Scatterguard, "-MO-SCAT-01:") | |||
| aperture_positions: Optional[AperturePositions] = None | |||
| TOLERANCE_STEPS = 3 # Number of MRES steps | |||
| ROBOT_LOAD_TOLERANCE = 5 # Number of mm from ROBOT_LOAD_Y | |||
There was a problem hiding this comment.
Oh, sorry, I didn't notice this before - we can't use a hardcoded position here because they need to be able to be adjusted in a configuration file (used by both GDA and Hyperion), so we load the positions in to self.aperture_positions on creating this device. 5 mm is also too big of a tolerance, I was suggesting 5 times the spatial resolution of the motor.
So then in _get_current_aperture_position you can compare current_ap_y to that dynamically loaded position value with the calculated tolerance loaded from the motor, something like:
def _get_current_aperture_position(self) -> SingleAperturePosition:
...
robot_load_ap_y = self.aperture_positions.ROBOT_LOAD.location.aperture_y
tolerance = self.TOLERANCE_STEPS * self.aperture.y.motor_resolution.get()
...
elif current_ap_y <= + robot_load_ap_y + tolerance:
...There was a problem hiding this comment.
Thanks @dperl-dls - I've added in these abstractions now and just updated the tests to also call them (instead of the hard coded within ant outside tolerance values)
Co-authored-by: David Perl <[email protected]>
Fixes #385
Instructions to reviewer on how to test:
Checks for reviewer