From e363fc51cc601b75bc215002c9f64687d34cf0dd Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Wed, 31 May 2023 16:19:45 +0100 Subject: [PATCH 01/13] Reinstate stage and add tests --- src/dodal/devices/eiger.py | 27 ++++++++++++++-- tests/devices/unit_tests/test_eiger.py | 45 ++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index 78d284ef2f..fb3e6255ec 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -26,6 +26,13 @@ class ArmingSignal(Signal): def set(self, value, *, timeout=None, settle_time=None, **kwargs): return self.parent.async_stage() + class ArmedState(Enum): + UNARMED = "unarmed" + ARMING = "arming" + ARMED = "armed" + + armed_state = ArmedState.UNARMED + do_arm: ArmingSignal = Component(ArmingSignal) cam: EigerDetectorCam = Component(EigerDetectorCam, "CAM:") odin: EigerOdin = Component(EigerOdin, "") @@ -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() + return self.arming_status - return self.do_arming_chain() + def stage(self): + if self.armed_state.value == "unarmed": + self.async_stage().wait(60) + + elif self.armed_state.value == "arming": + self.arming_status.wait(60) + + else: + return None def unstage(self) -> bool: assert self.detector_params is not None @@ -99,11 +117,15 @@ def unstage(self) -> bool: LOGGER.info("Disarming detector") self.disarm_detector() - + self.armed_state = self.ArmedState.UNARMED status_ok = self.odin.check_odin_state() self.disable_roi_mode() return status_ok + # This is no longer used by staging + def enable_roi_mode(self): + self.change_roi_mode(True) + def disable_roi_mode(self): self.change_roi_mode(False) @@ -265,6 +287,7 @@ def _wait_fan_ready(self) -> Status: def _finish_arm(self) -> Status: LOGGER.info("Eiger staging: Finishing arming") + self.armed_state = self.ArmedState.ARMED return Status(done=True, success=True) def forward_bit_depth_to_filewriter(self): diff --git a/tests/devices/unit_tests/test_eiger.py b/tests/devices/unit_tests/test_eiger.py index 1c130fecf9..d0920081f6 100644 --- a/tests/devices/unit_tests/test_eiger.py +++ b/tests/devices/unit_tests/test_eiger.py @@ -499,3 +499,48 @@ def test_given_in_free_run_mode_when_unstaged_then_waiting_on_file_writer_to_fin assert fake_eiger.odin.meta.stop_writing.get() == 1 assert fake_eiger.odin.file_writer.capture.get() == 0 + + +def test_if_arming_in_progress_then_stage_waits_for_completion( + fake_eiger: EigerDetector, mock_set_odin_filewriter +): + assert fake_eiger.armed_state.value == "unarmed" + fake_eiger.do_async_staging = MagicMock(return_value=Status(timeout=0)) + fake_eiger.async_stage() + assert fake_eiger.armed_state.value == "arming" + + # Should do .wait and error on timeout + with pytest.raises(Exception): + fake_eiger.stage() + + # State still in arming, but now wait should be done successfully + fake_eiger.arming_status = Status(done=True, success=True) + assert fake_eiger.stage() is None + + +def test_if_eiger_isnt_armed_then_stage_calls_async_stage(fake_eiger: EigerDetector): + assert fake_eiger.armed_state.value == "unarmed" + fake_eiger.async_stage = MagicMock() + fake_eiger.stage() + fake_eiger.async_stage.assert_called_once() + + +def test_if_eiger_is_armed_then_stage_does_nothing(fake_eiger: EigerDetector): + fake_eiger.armed_state = fake_eiger.ArmedState.ARMED + fake_eiger.async_stage = MagicMock() + assert fake_eiger.stage() is None + fake_eiger.async_stage.assert_not_called() + + +def test_armed_state_goes_to_armed_upon_stage_completion(fake_eiger: EigerDetector): + fake_eiger._finish_arm() + assert fake_eiger.armed_state.value == "armed" + + +def test_armed_state_goes_to_unarmed_after_unstage( + fake_eiger: EigerDetector, mock_set_odin_filewriter +): + fake_eiger.armed_state = fake_eiger.ArmedState.ARMED + fake_eiger.filewriters_finished = MagicMock() + fake_eiger.unstage() + assert fake_eiger.armed_state.value == "unarmed" From e05571dacdd7adb17dc22e1721dad3fcd11c135b Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Wed, 14 Jun 2023 10:11:10 +0100 Subject: [PATCH 02/13] Improvements to unit tests and run_functions_without_blocking --- src/dodal/devices/utils.py | 3 ++- tests/devices/unit_tests/test_eiger.py | 23 ++++++++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/dodal/devices/utils.py b/src/dodal/devices/utils.py index ade997313b..49d894f355 100644 --- a/src/dodal/devices/utils.py +++ b/src/dodal/devices/utils.py @@ -41,7 +41,8 @@ def run_functions_without_blocking( # intermediate statuses have an exception, the full_status will timeout. full_status = Status(timeout=timeout) - def closing_func(): + def closing_func(old_status): + check_callback_error(old_status) full_status.set_finished() # Wrap each function by first checking the previous status and attaching a callback to the next diff --git a/tests/devices/unit_tests/test_eiger.py b/tests/devices/unit_tests/test_eiger.py index d0920081f6..5797f45831 100644 --- a/tests/devices/unit_tests/test_eiger.py +++ b/tests/devices/unit_tests/test_eiger.py @@ -29,6 +29,10 @@ TEST_DET_DIST_TO_BEAM_CONVERTER_PATH = "tests/devices/unit_tests/test_lookup_table.txt" +class StatusException(Exception): + pass + + def create_new_params() -> DetectorParams: return DetectorParams( current_energy=TEST_CURRENT_ENERGY, @@ -443,11 +447,24 @@ def test_when_stage_called_then_finish_arm_on_fan_ready( def test_check_callback_error(fake_eiger: EigerDetector, iteration): def get_bad_status(): status = Status() - status.set_exception(Exception) + status.set_exception(StatusException) + return status + + def get_good_status(): + status = Status() + status.set_finished() return status LOGGER.error = MagicMock() + # These functions timeout without extra tweaking rather than give us the specific status error for the test + fake_eiger.set_odin_pvs = MagicMock() + fake_eiger.set_odin_pvs.return_value = get_good_status() + fake_eiger._wait_for_odin_status = MagicMock() + fake_eiger._wait_for_odin_status.return_value = get_good_status() + fake_eiger._wait_fan_ready = MagicMock() + fake_eiger._wait_fan_ready.return_value = get_good_status() + unwrapped_funcs = [ ( lambda: fake_eiger.set_detector_threshold( @@ -467,8 +484,8 @@ def get_bad_status(): unwrapped_funcs[iteration] = get_bad_status - with pytest.raises(Exception): - run_functions_without_blocking(unwrapped_funcs).wait() + with pytest.raises(StatusException): + run_functions_without_blocking(unwrapped_funcs).wait(timeout=10) LOGGER.error.assert_called_once() From 4a617751013a87f857c59c0209ed2cfadb4ea56f Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Wed, 14 Jun 2023 10:19:08 +0100 Subject: [PATCH 03/13] remove unused function --- src/dodal/devices/eiger.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index fb3e6255ec..85e00725f9 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -122,10 +122,6 @@ def unstage(self) -> bool: self.disable_roi_mode() return status_ok - # This is no longer used by staging - def enable_roi_mode(self): - self.change_roi_mode(True) - def disable_roi_mode(self): self.change_roi_mode(False) From 53bd9c9e8d74ba38474ecba24ce123abed621119 Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Tue, 20 Jun 2023 16:01:16 +0100 Subject: [PATCH 04/13] Remove armed state, adjust unit tests --- src/dodal/devices/eiger.py | 19 ++++++------ tests/devices/unit_tests/test_eiger.py | 40 +++++++++----------------- 2 files changed, 22 insertions(+), 37 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index 85e00725f9..f3921b2c33 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -31,8 +31,6 @@ class ArmedState(Enum): ARMING = "arming" ARMED = "armed" - armed_state = ArmedState.UNARMED - do_arm: ArmingSignal = Component(ArmingSignal) cam: EigerDetectorCam = Component(EigerDetectorCam, "CAM:") odin: EigerOdin = Component(EigerOdin, "") @@ -48,6 +46,9 @@ class ArmedState(Enum): detector_params: Optional[DetectorParams] = None + arming_status = Status() + arming_status.set_finished() + @classmethod def with_params( cls, @@ -86,19 +87,17 @@ 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() return self.arming_status def stage(self): - if self.armed_state.value == "unarmed": - self.async_stage().wait(60) - - elif self.armed_state.value == "arming": + if self.arming_status.done is False: + # Arming has started so wait for it to finish self.arming_status.wait(60) - else: - return None + if self.odin.fan.ready.get() != 1: + # Arming hasn't started, do it asynchronously + self.async_stage().wait(60) def unstage(self) -> bool: assert self.detector_params is not None @@ -117,7 +116,6 @@ def unstage(self) -> bool: LOGGER.info("Disarming detector") self.disarm_detector() - self.armed_state = self.ArmedState.UNARMED status_ok = self.odin.check_odin_state() self.disable_roi_mode() return status_ok @@ -283,7 +281,6 @@ def _wait_fan_ready(self) -> Status: def _finish_arm(self) -> Status: LOGGER.info("Eiger staging: Finishing arming") - self.armed_state = self.ArmedState.ARMED return Status(done=True, success=True) def forward_bit_depth_to_filewriter(self): diff --git a/tests/devices/unit_tests/test_eiger.py b/tests/devices/unit_tests/test_eiger.py index 5797f45831..0ccb83a19e 100644 --- a/tests/devices/unit_tests/test_eiger.py +++ b/tests/devices/unit_tests/test_eiger.py @@ -66,6 +66,12 @@ def finished_status(): return status +def get_bad_status(exception=Exception): + status = Status() + status.set_exception(Exception) + return status + + @pytest.fixture def mock_set_odin_filewriter(fake_eiger: EigerDetector): fake_eiger.odin.nodes.clear_odin_errors = MagicMock() @@ -521,43 +527,25 @@ def test_given_in_free_run_mode_when_unstaged_then_waiting_on_file_writer_to_fin def test_if_arming_in_progress_then_stage_waits_for_completion( fake_eiger: EigerDetector, mock_set_odin_filewriter ): - assert fake_eiger.armed_state.value == "unarmed" - fake_eiger.do_async_staging = MagicMock(return_value=Status(timeout=0)) - fake_eiger.async_stage() - assert fake_eiger.armed_state.value == "arming" + def set_bad_arming_status(): + fake_eiger.arming_status = get_bad_status() - # Should do .wait and error on timeout + assert fake_eiger.arming_status.done is True + fake_eiger.arming_status = get_bad_status() + # Should do .wait and error with pytest.raises(Exception): fake_eiger.stage() - # State still in arming, but now wait should be done successfully - fake_eiger.arming_status = Status(done=True, success=True) - assert fake_eiger.stage() is None - def test_if_eiger_isnt_armed_then_stage_calls_async_stage(fake_eiger: EigerDetector): - assert fake_eiger.armed_state.value == "unarmed" fake_eiger.async_stage = MagicMock() + fake_eiger.odin.fan.ready.sim_put(0) fake_eiger.stage() fake_eiger.async_stage.assert_called_once() def test_if_eiger_is_armed_then_stage_does_nothing(fake_eiger: EigerDetector): - fake_eiger.armed_state = fake_eiger.ArmedState.ARMED + fake_eiger.odin.fan.ready.sim_put(1) fake_eiger.async_stage = MagicMock() - assert fake_eiger.stage() is None + fake_eiger.stage() fake_eiger.async_stage.assert_not_called() - - -def test_armed_state_goes_to_armed_upon_stage_completion(fake_eiger: EigerDetector): - fake_eiger._finish_arm() - assert fake_eiger.armed_state.value == "armed" - - -def test_armed_state_goes_to_unarmed_after_unstage( - fake_eiger: EigerDetector, mock_set_odin_filewriter -): - fake_eiger.armed_state = fake_eiger.ArmedState.ARMED - fake_eiger.filewriters_finished = MagicMock() - fake_eiger.unstage() - assert fake_eiger.armed_state.value == "unarmed" From 7da8905345c4fef0cf576e4d6036fc00bc3c3643 Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Wed, 31 May 2023 16:19:45 +0100 Subject: [PATCH 05/13] Reinstate stage and add tests --- src/dodal/devices/eiger.py | 27 ++++++++++++++-- tests/devices/unit_tests/test_eiger.py | 45 ++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index 78d284ef2f..fb3e6255ec 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -26,6 +26,13 @@ class ArmingSignal(Signal): def set(self, value, *, timeout=None, settle_time=None, **kwargs): return self.parent.async_stage() + class ArmedState(Enum): + UNARMED = "unarmed" + ARMING = "arming" + ARMED = "armed" + + armed_state = ArmedState.UNARMED + do_arm: ArmingSignal = Component(ArmingSignal) cam: EigerDetectorCam = Component(EigerDetectorCam, "CAM:") odin: EigerOdin = Component(EigerOdin, "") @@ -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() + return self.arming_status - return self.do_arming_chain() + def stage(self): + if self.armed_state.value == "unarmed": + self.async_stage().wait(60) + + elif self.armed_state.value == "arming": + self.arming_status.wait(60) + + else: + return None def unstage(self) -> bool: assert self.detector_params is not None @@ -99,11 +117,15 @@ def unstage(self) -> bool: LOGGER.info("Disarming detector") self.disarm_detector() - + self.armed_state = self.ArmedState.UNARMED status_ok = self.odin.check_odin_state() self.disable_roi_mode() return status_ok + # This is no longer used by staging + def enable_roi_mode(self): + self.change_roi_mode(True) + def disable_roi_mode(self): self.change_roi_mode(False) @@ -265,6 +287,7 @@ def _wait_fan_ready(self) -> Status: def _finish_arm(self) -> Status: LOGGER.info("Eiger staging: Finishing arming") + self.armed_state = self.ArmedState.ARMED return Status(done=True, success=True) def forward_bit_depth_to_filewriter(self): diff --git a/tests/devices/unit_tests/test_eiger.py b/tests/devices/unit_tests/test_eiger.py index 1c130fecf9..d0920081f6 100644 --- a/tests/devices/unit_tests/test_eiger.py +++ b/tests/devices/unit_tests/test_eiger.py @@ -499,3 +499,48 @@ def test_given_in_free_run_mode_when_unstaged_then_waiting_on_file_writer_to_fin assert fake_eiger.odin.meta.stop_writing.get() == 1 assert fake_eiger.odin.file_writer.capture.get() == 0 + + +def test_if_arming_in_progress_then_stage_waits_for_completion( + fake_eiger: EigerDetector, mock_set_odin_filewriter +): + assert fake_eiger.armed_state.value == "unarmed" + fake_eiger.do_async_staging = MagicMock(return_value=Status(timeout=0)) + fake_eiger.async_stage() + assert fake_eiger.armed_state.value == "arming" + + # Should do .wait and error on timeout + with pytest.raises(Exception): + fake_eiger.stage() + + # State still in arming, but now wait should be done successfully + fake_eiger.arming_status = Status(done=True, success=True) + assert fake_eiger.stage() is None + + +def test_if_eiger_isnt_armed_then_stage_calls_async_stage(fake_eiger: EigerDetector): + assert fake_eiger.armed_state.value == "unarmed" + fake_eiger.async_stage = MagicMock() + fake_eiger.stage() + fake_eiger.async_stage.assert_called_once() + + +def test_if_eiger_is_armed_then_stage_does_nothing(fake_eiger: EigerDetector): + fake_eiger.armed_state = fake_eiger.ArmedState.ARMED + fake_eiger.async_stage = MagicMock() + assert fake_eiger.stage() is None + fake_eiger.async_stage.assert_not_called() + + +def test_armed_state_goes_to_armed_upon_stage_completion(fake_eiger: EigerDetector): + fake_eiger._finish_arm() + assert fake_eiger.armed_state.value == "armed" + + +def test_armed_state_goes_to_unarmed_after_unstage( + fake_eiger: EigerDetector, mock_set_odin_filewriter +): + fake_eiger.armed_state = fake_eiger.ArmedState.ARMED + fake_eiger.filewriters_finished = MagicMock() + fake_eiger.unstage() + assert fake_eiger.armed_state.value == "unarmed" From 3686bb62a3288f9d3540a952740cfbff0cae387a Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Wed, 14 Jun 2023 10:11:10 +0100 Subject: [PATCH 06/13] Improvements to unit tests and run_functions_without_blocking --- src/dodal/devices/utils.py | 3 ++- tests/devices/unit_tests/test_eiger.py | 23 ++++++++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/dodal/devices/utils.py b/src/dodal/devices/utils.py index ade997313b..49d894f355 100644 --- a/src/dodal/devices/utils.py +++ b/src/dodal/devices/utils.py @@ -41,7 +41,8 @@ def run_functions_without_blocking( # intermediate statuses have an exception, the full_status will timeout. full_status = Status(timeout=timeout) - def closing_func(): + def closing_func(old_status): + check_callback_error(old_status) full_status.set_finished() # Wrap each function by first checking the previous status and attaching a callback to the next diff --git a/tests/devices/unit_tests/test_eiger.py b/tests/devices/unit_tests/test_eiger.py index d0920081f6..5797f45831 100644 --- a/tests/devices/unit_tests/test_eiger.py +++ b/tests/devices/unit_tests/test_eiger.py @@ -29,6 +29,10 @@ TEST_DET_DIST_TO_BEAM_CONVERTER_PATH = "tests/devices/unit_tests/test_lookup_table.txt" +class StatusException(Exception): + pass + + def create_new_params() -> DetectorParams: return DetectorParams( current_energy=TEST_CURRENT_ENERGY, @@ -443,11 +447,24 @@ def test_when_stage_called_then_finish_arm_on_fan_ready( def test_check_callback_error(fake_eiger: EigerDetector, iteration): def get_bad_status(): status = Status() - status.set_exception(Exception) + status.set_exception(StatusException) + return status + + def get_good_status(): + status = Status() + status.set_finished() return status LOGGER.error = MagicMock() + # These functions timeout without extra tweaking rather than give us the specific status error for the test + fake_eiger.set_odin_pvs = MagicMock() + fake_eiger.set_odin_pvs.return_value = get_good_status() + fake_eiger._wait_for_odin_status = MagicMock() + fake_eiger._wait_for_odin_status.return_value = get_good_status() + fake_eiger._wait_fan_ready = MagicMock() + fake_eiger._wait_fan_ready.return_value = get_good_status() + unwrapped_funcs = [ ( lambda: fake_eiger.set_detector_threshold( @@ -467,8 +484,8 @@ def get_bad_status(): unwrapped_funcs[iteration] = get_bad_status - with pytest.raises(Exception): - run_functions_without_blocking(unwrapped_funcs).wait() + with pytest.raises(StatusException): + run_functions_without_blocking(unwrapped_funcs).wait(timeout=10) LOGGER.error.assert_called_once() From ea77f4240a92329c09a0a5911548bf01443b6d8b Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Wed, 14 Jun 2023 10:19:08 +0100 Subject: [PATCH 07/13] remove unused function --- src/dodal/devices/eiger.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index fb3e6255ec..85e00725f9 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -122,10 +122,6 @@ def unstage(self) -> bool: self.disable_roi_mode() return status_ok - # This is no longer used by staging - def enable_roi_mode(self): - self.change_roi_mode(True) - def disable_roi_mode(self): self.change_roi_mode(False) From e8e75ce67dbb7514dc71b749a7ce061a7ff66763 Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Tue, 20 Jun 2023 16:01:16 +0100 Subject: [PATCH 08/13] Remove armed state, adjust unit tests --- src/dodal/devices/eiger.py | 19 ++++++------ tests/devices/unit_tests/test_eiger.py | 40 +++++++++----------------- 2 files changed, 22 insertions(+), 37 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index 85e00725f9..f3921b2c33 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -31,8 +31,6 @@ class ArmedState(Enum): ARMING = "arming" ARMED = "armed" - armed_state = ArmedState.UNARMED - do_arm: ArmingSignal = Component(ArmingSignal) cam: EigerDetectorCam = Component(EigerDetectorCam, "CAM:") odin: EigerOdin = Component(EigerOdin, "") @@ -48,6 +46,9 @@ class ArmedState(Enum): detector_params: Optional[DetectorParams] = None + arming_status = Status() + arming_status.set_finished() + @classmethod def with_params( cls, @@ -86,19 +87,17 @@ 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() return self.arming_status def stage(self): - if self.armed_state.value == "unarmed": - self.async_stage().wait(60) - - elif self.armed_state.value == "arming": + if self.arming_status.done is False: + # Arming has started so wait for it to finish self.arming_status.wait(60) - else: - return None + if self.odin.fan.ready.get() != 1: + # Arming hasn't started, do it asynchronously + self.async_stage().wait(60) def unstage(self) -> bool: assert self.detector_params is not None @@ -117,7 +116,6 @@ def unstage(self) -> bool: LOGGER.info("Disarming detector") self.disarm_detector() - self.armed_state = self.ArmedState.UNARMED status_ok = self.odin.check_odin_state() self.disable_roi_mode() return status_ok @@ -283,7 +281,6 @@ def _wait_fan_ready(self) -> Status: def _finish_arm(self) -> Status: LOGGER.info("Eiger staging: Finishing arming") - self.armed_state = self.ArmedState.ARMED return Status(done=True, success=True) def forward_bit_depth_to_filewriter(self): diff --git a/tests/devices/unit_tests/test_eiger.py b/tests/devices/unit_tests/test_eiger.py index 5797f45831..0ccb83a19e 100644 --- a/tests/devices/unit_tests/test_eiger.py +++ b/tests/devices/unit_tests/test_eiger.py @@ -66,6 +66,12 @@ def finished_status(): return status +def get_bad_status(exception=Exception): + status = Status() + status.set_exception(Exception) + return status + + @pytest.fixture def mock_set_odin_filewriter(fake_eiger: EigerDetector): fake_eiger.odin.nodes.clear_odin_errors = MagicMock() @@ -521,43 +527,25 @@ def test_given_in_free_run_mode_when_unstaged_then_waiting_on_file_writer_to_fin def test_if_arming_in_progress_then_stage_waits_for_completion( fake_eiger: EigerDetector, mock_set_odin_filewriter ): - assert fake_eiger.armed_state.value == "unarmed" - fake_eiger.do_async_staging = MagicMock(return_value=Status(timeout=0)) - fake_eiger.async_stage() - assert fake_eiger.armed_state.value == "arming" + def set_bad_arming_status(): + fake_eiger.arming_status = get_bad_status() - # Should do .wait and error on timeout + assert fake_eiger.arming_status.done is True + fake_eiger.arming_status = get_bad_status() + # Should do .wait and error with pytest.raises(Exception): fake_eiger.stage() - # State still in arming, but now wait should be done successfully - fake_eiger.arming_status = Status(done=True, success=True) - assert fake_eiger.stage() is None - def test_if_eiger_isnt_armed_then_stage_calls_async_stage(fake_eiger: EigerDetector): - assert fake_eiger.armed_state.value == "unarmed" fake_eiger.async_stage = MagicMock() + fake_eiger.odin.fan.ready.sim_put(0) fake_eiger.stage() fake_eiger.async_stage.assert_called_once() def test_if_eiger_is_armed_then_stage_does_nothing(fake_eiger: EigerDetector): - fake_eiger.armed_state = fake_eiger.ArmedState.ARMED + fake_eiger.odin.fan.ready.sim_put(1) fake_eiger.async_stage = MagicMock() - assert fake_eiger.stage() is None + fake_eiger.stage() fake_eiger.async_stage.assert_not_called() - - -def test_armed_state_goes_to_armed_upon_stage_completion(fake_eiger: EigerDetector): - fake_eiger._finish_arm() - assert fake_eiger.armed_state.value == "armed" - - -def test_armed_state_goes_to_unarmed_after_unstage( - fake_eiger: EigerDetector, mock_set_odin_filewriter -): - fake_eiger.armed_state = fake_eiger.ArmedState.ARMED - fake_eiger.filewriters_finished = MagicMock() - fake_eiger.unstage() - assert fake_eiger.armed_state.value == "unarmed" From 09d4013938803282284150251a164999144bce1a Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Wed, 21 Jun 2023 10:39:13 +0100 Subject: [PATCH 09/13] Fix typo that should have broken arming --- src/dodal/devices/eiger.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index f3921b2c33..36a5ad4820 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -26,11 +26,6 @@ class ArmingSignal(Signal): def set(self, value, *, timeout=None, settle_time=None, **kwargs): return self.parent.async_stage() - class ArmedState(Enum): - UNARMED = "unarmed" - ARMING = "arming" - ARMED = "armed" - do_arm: ArmingSignal = Component(ArmingSignal) cam: EigerDetectorCam = Component(EigerDetectorCam, "CAM:") odin: EigerOdin = Component(EigerOdin, "") @@ -97,7 +92,7 @@ def stage(self): else: if self.odin.fan.ready.get() != 1: # Arming hasn't started, do it asynchronously - self.async_stage().wait(60) + self.async_stage() def unstage(self) -> bool: assert self.detector_params is not None @@ -306,7 +301,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), self._wait_for_odin_status, lambda: self.cam.acquire.set(1, timeout=self.GENERAL_STATUS_TIMEOUT), self._wait_fan_ready, From c6f0e617b0675323b4ea2bddfd9652700dc767ce Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Wed, 21 Jun 2023 10:48:20 +0100 Subject: [PATCH 10/13] Remove armedstate after merge --- src/dodal/devices/eiger.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index f3b6521d01..36a5ad4820 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -26,11 +26,6 @@ class ArmingSignal(Signal): def set(self, value, *, timeout=None, settle_time=None, **kwargs): return self.parent.async_stage() - class ArmedState(Enum): - UNARMED = "unarmed" - ARMING = "arming" - ARMED = "armed" - do_arm: ArmingSignal = Component(ArmingSignal) cam: EigerDetectorCam = Component(EigerDetectorCam, "CAM:") odin: EigerOdin = Component(EigerOdin, "") From 3dbdef5b29d662fcae5b5c7fb5221d12e4fcb13d Mon Sep 17 00:00:00 2001 From: olliesilvester <122091460+olliesilvester@users.noreply.github.com> Date: Tue, 27 Jun 2023 10:50:15 +0100 Subject: [PATCH 11/13] Commit nit suggestion Co-authored-by: Dominic Oram --- src/dodal/devices/eiger.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index 36a5ad4820..9ed5b9b1ca 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -86,7 +86,7 @@ def async_stage(self): return self.arming_status def stage(self): - if self.arming_status.done is False: + if not self.arming_status.done: # Arming has started so wait for it to finish self.arming_status.wait(60) else: From ce4f67628e9a1434d9930974e9fb42becf55fc0b Mon Sep 17 00:00:00 2001 From: Oliver Silvester Date: Tue, 27 Jun 2023 11:04:55 +0100 Subject: [PATCH 12/13] Add is_armed function --- src/dodal/devices/eiger.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index 36a5ad4820..c750c2d215 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -85,14 +85,19 @@ def async_stage(self): self.arming_status = self.do_arming_chain() return self.arming_status + def is_armed(self): + if self.odin.fan.ready.get() == 1 and self.cam.acquire.get() == 1: + return True + else: + return False + def stage(self): if self.arming_status.done is False: # Arming has started so wait for it to finish self.arming_status.wait(60) - else: - if self.odin.fan.ready.get() != 1: - # Arming hasn't started, do it asynchronously - self.async_stage() + elif not self.is_armed(): + # Arming hasn't started, do it asynchronously + self.async_stage().wait(timeout=self.GENERAL_STATUS_TIMEOUT) def unstage(self) -> bool: assert self.detector_params is not None From 126cf2710510d8ac7885a904ada247f2454ac5fc Mon Sep 17 00:00:00 2001 From: olliesilvester <122091460+olliesilvester@users.noreply.github.com> Date: Tue, 27 Jun 2023 12:59:02 +0100 Subject: [PATCH 13/13] Minor nit change Co-authored-by: Dominic Oram --- src/dodal/devices/eiger.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index 3407bd0d52..72a9835d04 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -86,10 +86,7 @@ def async_stage(self): return self.arming_status def is_armed(self): - if self.odin.fan.ready.get() == 1 and self.cam.acquire.get() == 1: - return True - else: - return False + return self.odin.fan.ready.get() == 1 and self.cam.acquire.get() == 1 def stage(self): if not self.arming_status.done: