Fix misc eiger arming issues#2052
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2052 +/- ##
=======================================
Coverage 99.11% 99.12%
=======================================
Files 327 331 +4
Lines 12810 12895 +85
=======================================
+ Hits 12697 12782 +85
Misses 113 113 ☔ 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.
Couple of suggestions. Now we're less parallel this presumably will affect the time it takes to arm/disarm the detector. Have you investigated how much it changes throughput and agreed that the stability is preferred?
| status = self.cam.roi_mode.set( | ||
| 1 if enable else 0, timeout=self.timeouts.general_status_timeout | ||
| ) | ||
| status.wait(timeout=self.timeouts.general_status_timeout) |
There was a problem hiding this comment.
Could: If we're going to wait on each individual status then we don't need to keep the status around, e.g.
| status = self.cam.roi_mode.set( | |
| 1 if enable else 0, timeout=self.timeouts.general_status_timeout | |
| ) | |
| status.wait(timeout=self.timeouts.general_status_timeout) | |
| self.cam.roi_mode.set(1 if enable else 0, timeout=self.timeouts.general_status_timeout).wait(timeout=self.timeouts.general_status_timeout) |
There was a problem hiding this comment.
Sorry for my question if it misses the point here. Is it possibl to improve IOC PV so the set call can rely on callback instead of wait on status, or the status already embedded callback inside?
There was a problem hiding this comment.
Yes, the status is the callback from the IOC
| ) | ||
| status.wait(timeout=self.timeouts.general_status_timeout) | ||
| LOGGER.debug(f"Setting image_height to {detector_dimensions.height}") | ||
| status &= self.odin.file_writer.image_height.set( |
There was a problem hiding this comment.
Should: The last status has already been waited on so no need to do &= here, = will do (or just don't keep it around as above)
| timeout=self.timeouts.general_status_timeout, | ||
| ) | ||
| status.wait(timeout=self.timeouts.general_status_timeout) | ||
| return status |
There was a problem hiding this comment.
Should: The only reason this returns the status is so that we can wait on it, if we've already waited on it then it seems a bit pointless
There was a problem hiding this comment.
this all goes into the list of run_functions_without_blocking() so to follow convention we return the status.
| def _wait_for_odin_status(self) -> StatusBase: | ||
| self.forward_bit_depth_to_filewriter() | ||
| await_value(self.odin.meta.active, 1).wait(self.timeouts.general_status_timeout) | ||
| await_value(self.odin.meta.active, 1).wait(60) |
There was a problem hiding this comment.
Should: Can we pull this into a const?
| fake_eiger.arming_status.wait(1) # This should complete long before 1s | ||
| # One log message kicking off arming, then one for each of the 13 arming stages | ||
| assert len(caplog.messages) == 14 | ||
| assert len(caplog.messages) == 18 |
There was a problem hiding this comment.
Should: This no longer matches the comment above
Yes, we ran it with this hotfixed, it doesn't slow things down that much. I don't have specific numbers but we think the delay is probably of the order of ~1s, judging from the archiver logs for signals. however most of that is while waiting for the robot load. |
Fair enough, it goes away with the fastCS Eiger anyway. Other comments still stand I think |
3 participants
Fixes
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}