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.11%
=======================================
Files 327 327
Lines 12810 12830 +20
=======================================
+ Hits 12697 12717 +20
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) |
| ) | ||
| 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
| 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
2 participants
Fixes
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}