Make stage synchronous if arming not started #92
Conversation
olliesilvester
changed the title
src/dodal/devices/utils.py
Outdated
There was a problem hiding this comment.
This isn't related to #76 but I noticed the the status in the final function in the callback chain was never being checked
Codecov Report
@@ Coverage Diff @@
## main #92 +/- ##
==========================================
+ Coverage 89.07% 89.13% +0.06%
==========================================
Files 57 57
Lines 1949 1960 +11
==========================================
+ Hits 1736 1747 +11
Misses 213 213
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thanks, I think suggested change cleans things up a bit. Can you try and write a test to prove if you've fixed DiamondLightSource/hyperion#686? Basically add somewhere in the fast_grid_scan_plan tests confirming the eiger gets armed
src/dodal/devices/eiger.py
Outdated
| def stage(self): | ||
| if self.armed_state.value == "unarmed": | ||
| self.async_stage().wait(60) | ||
|
|
||
| return self.do_arming_chain() | ||
| elif self.armed_state.value == "arming": | ||
| self.arming_status.wait(60) |
There was a problem hiding this comment.
Should: I think we can do this slightly more cleanly:
- If the
arming_statusis not complete then wait on it to finish - If it is and
self.cam.acquireis high assume you're armed, otherwise assume not and do an async arm
I think this means you can then remove armed_state all together.
There was a problem hiding this comment.
I think this also means we can remove the None return
There was a problem hiding this comment.
The last part of staging is awaiting for self.odin.fan.ready to be set to 1, shouldn't we check this value rather than self.cam.acquire?
There was a problem hiding this comment.
Good spot, I think we should confirm acquire and fan.ready. Can you make a new function called something like is_armed that we use to check both of these?
| @@ -79,8 +86,19 @@ def async_stage(self): | |||
| status_ok, error_message = self.odin.check_odin_initialised() | |||
| if not status_ok: | |||
| raise Exception(f"Odin not initialised: {error_message}") | |||
| self.armed_state = self.ArmedState.ARMING | |||
| self.arming_status = self.do_arming_chain() | |||
There was a problem hiding this comment.
Should: You should create a placeholder class variable for this that's initialised to a complete status, this helps autocomplete etc.
…' of github.com:DiamondLightSource/dodal into 76_make_stage_synchronous_if_arming_not_started_rebased
| @@ -290,7 +306,7 @@ def do_arming_chain(self) -> Status: | |||
| self.set_odin_pvs, | |||
| self.set_mx_settings_pvs, | |||
| self.set_num_triggers_and_captures, | |||
| lambda: await_value(self.STALE_PARAMS_TIMEOUT, 0, 60), | |||
| lambda: await_value(self.stale_params, 0, 60), | |||
There was a problem hiding this comment.
This typo looks like it should have broken the arming, not sure how it was working before if this was in there
There was a problem hiding this comment.
Yh, it's probably just not covered well in the tests
| @@ -290,7 +306,7 @@ def do_arming_chain(self) -> Status: | |||
| self.set_odin_pvs, | |||
| self.set_mx_settings_pvs, | |||
| self.set_num_triggers_and_captures, | |||
| lambda: await_value(self.STALE_PARAMS_TIMEOUT, 0, 60), | |||
| lambda: await_value(self.stale_params, 0, 60), | |||
There was a problem hiding this comment.
Yh, it's probably just not covered well in the tests
src/dodal/devices/eiger.py
Outdated
| def stage(self): | ||
| if self.armed_state.value == "unarmed": | ||
| self.async_stage().wait(60) | ||
|
|
||
| return self.do_arming_chain() | ||
| elif self.armed_state.value == "arming": | ||
| self.arming_status.wait(60) |
There was a problem hiding this comment.
Good spot, I think we should confirm acquire and fan.ready. Can you make a new function called something like is_armed that we use to check both of these?
src/dodal/devices/eiger.py
Outdated
| else: | ||
| if self.odin.fan.ready.get() != 1: | ||
| # Arming hasn't started, do it asynchronously | ||
| self.async_stage() |
There was a problem hiding this comment.
Must: I think in this case we need to be synchronous so we should have a wait here
src/dodal/devices/eiger.py
Outdated
| else: | ||
| if self.odin.fan.ready.get() != 1: |
There was a problem hiding this comment.
Nit: This can be an elif
Co-authored-by: Dominic Oram <[email protected]>
Co-authored-by: Dominic Oram <[email protected]>
DominicOram
deleted the
76_make_stage_synchronous_if_arming_not_started_rebased
branch
Fixes #76
This should also fix the very slow unit tests introduced in #43
To test:
Run new tests and check they are sensible
Check changes satisfy the criteria
See if this fixes DiamondLightSource/hyperion#686