Wait for smargon enable in robot load and unload #1791
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1791 +/- ##
=======================================
Coverage 99.12% 99.12%
=======================================
Files 282 282
Lines 10683 10688 +5
=======================================
+ Hits 10589 10594 +5
Misses 94 94 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, some comments in the code
| async def smargon_status_or_error( | ||
| self, expected_state: SmargonStatus = SmargonStatus.ENABLED | ||
| ): |
There was a problem hiding this comment.
Could: I think this taking enable as default is no longer that clear, I would change it to just no default
| self.smargon_enabled = epics_signal_r( | ||
| SmargonStatus, prefix + "ROBOT_OP_16_BITS.B8" | ||
| ) |
There was a problem hiding this comment.
Should: I think it's going to confuse someone one day that we call this smargon_enabled but the synoptic calls it Cryojet Retract Request. Particularly as there's no reason this robot couldn't be used on a beamline with a different gonio to a smargon. I think a more generic name like beamline_enabled and a comment like "We assume that a robot action is finished, and that we are safe to proceed once the cryoject retract request is complete"
There was a problem hiding this comment.
Must: I think the logic is also inverted. When this is high the smargon is disabled. So the name should be something like beamline_disabled
There was a problem hiding this comment.
Must: If you run dodal connect i03 you will see this fails. This is because a StrictEnum only works when the PV is an enum type with the same enum names. In this case the PV is a binary 0/1 with no mapping:
(.venv) [ffv81422@ws455 dodal]$ caget BL03I-MO-ROBOT-01:ROBOT_OP_16_BITS.B8
BL03I-MO-ROBOT-01:ROBOT_OP_16_BITS.B8 0
(.venv) [ffv81422@ws455 dodal]$ caget -d31 BL03I-MO-ROBOT-01:ROBOT_OP_16_BITS.B8
BL03I-MO-ROBOT-01:ROBOT_OP_16_BITS.B8
Native data type: DBF_CHAR
Request type: DBR_CTRL_ENUM
Element count: 1
Value: Enum Index Overflow (0)
Status: NO_ALARM
Severity: NO_ALARM
Enums: ( 0)
There actually doesn't seem to be a great way to do this, see bluesky/ophyd-async#1168. So I think we will have to leave it as an int for now.
There was a problem hiding this comment.
Could: Also, I don't think this PV needs to be in the readables, its unlikely a user will need to read it
| await self.unload.trigger(timeout=self.LOAD_TIMEOUT) | ||
| await wait_for_value(self.program_running, False, self.NOT_BUSY_TIMEOUT) | ||
| await wait_for( | ||
| self.smargon_status_or_error(SmargonStatus.ENABLED), | ||
| timeout=self.LOAD_TIMEOUT + self.NOT_BUSY_TIMEOUT, | ||
| ) |
There was a problem hiding this comment.
Could: I actually think there might be a race condition in here from before where the trigger may not yet have started a program running. I think to be safer we can just use exactly the same code as the load e.g. wait for disable then enable. In fact we can probably then reuse some of the code between the two
Fixes DiamondLightSource/mx-bluesky#1525
Instead of waiting on the pin sensor, the robot waits on the smargon disable PV to be low. This makes the robot logic more robust as before the robot could block the sensor and cause false positives (thinking a pin is loaded when it is not). This should also help make the robot load plans in mx-bluesky more generic (see DiamondLightSource/mx-bluesky#1478).
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}