Mx bluesky 1189 scan invalid #1540
Conversation
rtuck99
force-pushed
the
mx-bluesky_1189_scan_invalid
branch
from
74d0a46 to
4fde8a1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1540 +/- ##
=======================================
Coverage 98.88% 98.88%
=======================================
Files 260 260
Lines 9472 9493 +21
=======================================
+ Hits 9366 9387 +21
Misses 106 106 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rtuck99
force-pushed
the
mx-bluesky_1189_scan_invalid
branch
from
1ad901c to
8171d36
Compare
There was a problem hiding this comment.
This is so much cleaner. A few comments in code
Possibly we could raise a more specific exception here; SampleException is currently in mx-bluesky, but we could define our own
Yh, I think we should. I think it's a bit more readable, plus I don't think it's currently possible to get a TimeoutError from the prepare for any reason other than the check tasks
src/dodal/devices/fast_grid_scan.py
Outdated
| self.x_scan_valid = epics_signal_r(float, f"{prefix}X_SCAN_VALID") | ||
| self.y_scan_valid = epics_signal_r(float, f"{prefix}Y_SCAN_VALID") | ||
| self.z_scan_valid = epics_signal_r(float, f"{prefix}Z_SCAN_VALID") | ||
| self.scan_invalid = epics_signal_r(float, f"{prefix}SCAN_INVALID") |
There was a problem hiding this comment.
Must: FastGridScanCommon is also used by i02_1, where these PVs don't exist. I.e. if you do a dodal connect i02_1 this fails
| ) -> MotionProgram: ... | ||
|
|
||
| @AsyncStatus.wrap | ||
| async def prepare(self, value: ParamType): |
There was a problem hiding this comment.
Thanks, this is a lot nicer than doing it plan side!
src/dodal/devices/fast_grid_scan.py
Outdated
|
|
||
| LOGGER.info("Applying gridscan parameters...") | ||
| # Create arguments for bps.mv | ||
| for key, signal in self.movable_params.items(): |
There was a problem hiding this comment.
Could: Can we make movable_params private now?
src/dodal/devices/fast_grid_scan.py
Outdated
| # wait for parameter readbacks to update | ||
| await asyncio.gather(*set_statuses) | ||
|
|
||
| LOGGER.info("Readbacks confirmed, waiting for validity checks to pass...") |
There was a problem hiding this comment.
Should: This isn't reading the RBVs. The set logic on epics_signal_rw_rbv is to return when we get a put callback on the PV. See bluesky/ophyd-async#996 for some discussion on how this might change. Solution is to either:
- Explicitly wait on the read being as we expect here (ideally by using 996 create a helper to easily allow tying a setpoint and readback together bluesky/ophyd-async#1011)
- Ask controls to get the put callback logic to include this check (I think this is not widely done outside of motors though)
There was a problem hiding this comment.
A bit confused about this - these statuses are from set_and_wait_for_value() which does actually do an observe_value() on the signal. I appreciate these aren't motors and so don't have a separate RBV PV, but surely as they are rw signals it does actually read the value back from the device?
| # XXX Can we use x/y/z scan valid to distinguish between SampleException/pin invalid | ||
| # and other non-sample-related errors? |
There was a problem hiding this comment.
Should: I'm not sure if there are cases where the scan is invalid but it's not a sample error but maybe I'm missing something?
src/dodal/devices/fast_grid_scan.py
Outdated
| check_tasks = [] | ||
| for signal, expected_value in { | ||
| self.x_scan_valid: 1, | ||
| self.y_scan_valid: 1, | ||
| self.z_scan_valid: 1, | ||
| self.scan_invalid: 0, | ||
| }.items(): | ||
| check_tasks.append( | ||
| wait_for_value( | ||
| signal, expected_value, timeout=self.VALIDITY_CHECK_TIMEOUT | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Nit:
| check_tasks = [] | |
| for signal, expected_value in { | |
| self.x_scan_valid: 1, | |
| self.y_scan_valid: 1, | |
| self.z_scan_valid: 1, | |
| self.scan_invalid: 0, | |
| }.items(): | |
| check_tasks.append( | |
| wait_for_value( | |
| signal, expected_value, timeout=self.VALIDITY_CHECK_TIMEOUT | |
| ) | |
| ) | |
| check_tasks = [ | |
| wait_for_value(signal, expected_value, timeout=self.VALIDITY_CHECK_TIMEOUT) | |
| for signal, expected_value in { | |
| self.x_scan_valid: 1, | |
| self.y_scan_valid: 1, | |
| self.z_scan_valid: 1, | |
| self.scan_invalid: 0, | |
| }.items() | |
| ] |
Much of a muchness though
| return epics_signal_rw(int, f"{prefix}Y_COUNTER") | ||
|
|
||
|
|
||
| def set_fast_grid_scan_params(scan: FastGridScanCommon[ParamType], params: ParamType): |
There was a problem hiding this comment.
Could: Maybe a docstring in here on what to expect if it's invalid
rtuck99
force-pushed
the
mx-bluesky_1189_scan_invalid
branch
from
fa5cc9f to
b3c0ce0
Compare
Fixes
See mx-bluesky PR
This changes
fast_grid_scan.set_fast_grid_scan_params()plan to delegate to theFastGridScanCommon.prepare()method.prepare()combines setting the parameters with checking that they are valid, if the parameters are invalid, a TimeoutError will occur.SampleExceptionis currently in mx-bluesky, but we could define our ownInstructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}