diff --git a/broker/JFJochStateMachine.cpp b/broker/JFJochStateMachine.cpp index 5189335c..a701fdc8 100644 --- a/broker/JFJochStateMachine.cpp +++ b/broker/JFJochStateMachine.cpp @@ -64,7 +64,7 @@ bool JFJochStateMachine::ImportPedestalG1G2(const JFJochReceiverOutput &receiver return true; } -void JFJochStateMachine::CalibrateJUNGFRAU(std::unique_lock &ul) { +bool JFJochStateMachine::CalibrateJUNGFRAU(std::unique_lock &ul) { if (!gain_calibration.empty()) { if (gain_calibration.size() != experiment.GetModulesNum()) throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, @@ -73,14 +73,20 @@ void JFJochStateMachine::CalibrateJUNGFRAU(std::unique_lock &ul) { calibration->GainCalibration(i) = gain_calibration[i]; } - TakePedestalInternalG0(ul); + // Abandon the sequence on the first failure. Collecting G1 on top of a G0 that was never + // measured only produces a calibration that looks complete. + if (!TakePedestalInternalG0(ul)) + return false; if (!experiment.IsFixedGainG1()) { for (int i = 0; i < experiment.GetStorageCellNumber(); i++) { - TakePedestalInternalG1(ul, i); - TakePedestalInternalG2(ul, i); + if (!TakePedestalInternalG1(ul, i)) + return false; + if (!TakePedestalInternalG2(ul, i)) + return false; } } pixel_mask.LoadDetectorBadPixelMask(experiment, calibration.get()); + return true; } void JFJochStateMachine::CalibrateDetector(std::unique_lock ul) { @@ -90,46 +96,56 @@ void JFJochStateMachine::CalibrateDetector(std::unique_lock ul) { UpdatePixelMaskStatistics(pixel_mask.GetStatistics()); logger.Info("Calibration sequence started"); + bool calibrated; try { if (experiment.GetDetectorType() == DetectorType::EIGER) { // PSI EIGER - only reset calibration calibration.reset(); + calibrated = true; } else if (experiment.GetDetectorType() == DetectorType::DECTRIS) { // DECTRIS - take dark data for mask calibration.reset(); - TakeDarkMaskInternal(ul); + calibrated = TakeDarkMaskInternal(ul); } else { // PSI JUNGFRAU - take pedestal calibration = std::make_unique(experiment); - CalibrateJUNGFRAU(ul); + calibrated = CalibrateJUNGFRAU(ul); } // Update pixel mask statistics UpdatePixelMaskStatistics(pixel_mask.GetStatistics()); - // configure detector for standard operation - services.ConfigureDetector(experiment); + // configure detector for standard operation - only worth doing if there is a calibration to + // operate with, and a cancelled sequence has left the detector mid-abort anyway + if (calibrated) + services.ConfigureDetector(experiment); } catch (const std::exception &e) { logger.Error("Calibration sequence error {}", e.what()); - SetState(JFJochState::Error, e.what(), BrokerStatus::MessageSeverity::Error); + // The calibration is in an undefined state, so the detector has to be initialized again. + SetState(JFJochState::Inactive, e.what(), BrokerStatus::MessageSeverity::Error); + c.notify_all(); // ul unlocks on the way out throw; } - SetState(JFJochState::Idle, "Calibration sequence done", BrokerStatus::MessageSeverity::Success); - logger.Info("Calibration sequence done"); + // The steps above report a cancellation or a failure through SetState and return false; that + // must not be overwritten with success here. + if (calibrated) { + SetState(JFJochState::Idle, "Calibration sequence done", BrokerStatus::MessageSeverity::Success); + logger.Info("Calibration sequence done"); + } ul.unlock(); // Notify all outside of mutex c.notify_all(); } -void JFJochStateMachine::TakeDarkMaskInternal(std::unique_lock &ul) { +bool JFJochStateMachine::TakeDarkMaskInternal(std::unique_lock &ul) { if (cancel_sequence) { SetState(JFJochState::Inactive, "Mask sequence cancelled", - BrokerStatus::MessageSeverity::Warning); - return; + BrokerStatus::MessageSeverity::Error); + return false; } services.LoadDetectorPixelMask(pixel_mask); if (experiment.GetDarkMaskNumberOfFrames() == 0) - return; + return true; DiffractionExperiment local_experiment(experiment); local_experiment.Mode(DetectorMode::DarkMask); @@ -145,14 +161,16 @@ void JFJochStateMachine::TakeDarkMaskInternal(std::unique_lock &ul) auto mask_output = services.Stop(); ul.lock(); - if (mask_output.receiver_output.dark_mask_result.size() == local_experiment.GetPixelsNum()) { - pixel_mask.LoadDarkBadPixelMask(local_experiment, mask_output.receiver_output.dark_mask_result); - SetState(JFJochState::Idle); - } else - SetState(JFJochState::Error, "Mask not collected properly", BrokerStatus::MessageSeverity::Error); + if (mask_output.receiver_output.dark_mask_result.size() != local_experiment.GetPixelsNum()) { + SetState(JFJochState::Inactive, "Mask not collected properly", BrokerStatus::MessageSeverity::Error); + return false; + } + pixel_mask.LoadDarkBadPixelMask(local_experiment, mask_output.receiver_output.dark_mask_result); + SetState(JFJochState::Idle); + return true; } -void JFJochStateMachine::TakePedestalInternalG0(std::unique_lock &ul) { +bool JFJochStateMachine::TakePedestalInternalG0(std::unique_lock &ul) { DiffractionExperiment local_experiment(experiment); std::string message; if (local_experiment.IsFixedGainG1()) { @@ -171,12 +189,12 @@ void JFJochStateMachine::TakePedestalInternalG0(std::unique_lock &ul if (cancel_sequence) { SetState(JFJochState::Inactive, "Pedestal sequence cancelled", - BrokerStatus::MessageSeverity::Warning); - return; + BrokerStatus::MessageSeverity::Error); + return false; } if (local_experiment.GetPedestalG0Frames() == 0) - return; + return true; SetState(JFJochState::Calibration, message, BrokerStatus::MessageSeverity::Info); services.ConfigureDetector(local_experiment); @@ -190,15 +208,17 @@ void JFJochStateMachine::TakePedestalInternalG0(std::unique_lock &ul auto pedestal_output = services.Stop(); ul.lock(); - if (ImportPedestalG0(pedestal_output.receiver_output)) - SetState(JFJochState::Idle); - else - SetState(JFJochState::Error, + if (!ImportPedestalG0(pedestal_output.receiver_output)) { + SetState(JFJochState::Inactive, "Pedestal not collected properly", BrokerStatus::MessageSeverity::Error); + return false; + } + SetState(JFJochState::Idle); + return true; } -void JFJochStateMachine::TakePedestalInternalG1(std::unique_lock &ul, int32_t storage_cell) { +bool JFJochStateMachine::TakePedestalInternalG1(std::unique_lock &ul, int32_t storage_cell) { DiffractionExperiment local_experiment(experiment); local_experiment.Mode(DetectorMode::PedestalG1); @@ -211,12 +231,12 @@ void JFJochStateMachine::TakePedestalInternalG1(std::unique_lock &ul if (cancel_sequence) { SetState(JFJochState::Inactive, "Pedestal sequence cancelled", - BrokerStatus::MessageSeverity::Warning); - return; + BrokerStatus::MessageSeverity::Error); + return false; } if (local_experiment.GetPedestalG1Frames() == 0) - return; + return true; SetState(JFJochState::Calibration, @@ -233,13 +253,16 @@ void JFJochStateMachine::TakePedestalInternalG1(std::unique_lock &ul auto pedestal_output = services.Stop(); ul.lock(); - if (!ImportPedestalG1G2(pedestal_output.receiver_output, 1, storage_cell)) - SetState(JFJochState::Error, + if (!ImportPedestalG1G2(pedestal_output.receiver_output, 1, storage_cell)) { + SetState(JFJochState::Inactive, "Pedestal not collected properly", BrokerStatus::MessageSeverity::Error); + return false; + } + return true; } -void JFJochStateMachine::TakePedestalInternalG2(std::unique_lock &ul, int32_t storage_cell) { +bool JFJochStateMachine::TakePedestalInternalG2(std::unique_lock &ul, int32_t storage_cell) { DiffractionExperiment local_experiment(experiment); local_experiment.Mode(DetectorMode::PedestalG2); @@ -251,12 +274,12 @@ void JFJochStateMachine::TakePedestalInternalG2(std::unique_lock &ul if (cancel_sequence) { SetState(JFJochState::Inactive, "Pedestal sequence cancelled", - BrokerStatus::MessageSeverity::Warning); - return; + BrokerStatus::MessageSeverity::Error); + return false; } if (local_experiment.GetPedestalG2Frames() == 0) - return; + return true; SetState(JFJochState::Calibration, @@ -273,10 +296,13 @@ void JFJochStateMachine::TakePedestalInternalG2(std::unique_lock &ul auto pedestal_output = services.Stop(); ul.lock(); - if (!ImportPedestalG1G2(pedestal_output.receiver_output, 2, storage_cell)) - SetState(JFJochState::Error, + if (!ImportPedestalG1G2(pedestal_output.receiver_output, 2, storage_cell)) { + SetState(JFJochState::Inactive, "Pedestal not collected properly", BrokerStatus::MessageSeverity::Error); + return false; + } + return true; } void JFJochStateMachine::Initialize() { @@ -336,6 +362,9 @@ void JFJochStateMachine::InitializeThread(std::unique_lock ul) { } catch (const std::exception &e) { logger.Error("Initialize error {}", e.what()); SetState(JFJochState::Error, e.what(), BrokerStatus::MessageSeverity::Error); + // Wake anyone in WaitTillNotBusy/WaitTillMeasurementDone - the state has left Busy, and + // without this they sleep out their whole timeout before noticing. + c.notify_all(); // ul unlocks on the way out throw; } CalibrateDetector(std::move(ul)); diff --git a/broker/JFJochStateMachine.h b/broker/JFJochStateMachine.h index 17ad3e58..6ebea093 100644 --- a/broker/JFJochStateMachine.h +++ b/broker/JFJochStateMachine.h @@ -137,12 +137,15 @@ class JFJochStateMachine { bool ImportPedestalG0(const JFJochReceiverOutput &receiver_output); bool IsRunning() const; // Is state Busy/Pedestal/Measure void ResetError() noexcept; - void TakeDarkMaskInternal(std::unique_lock &ul); + // The calibration steps report their own outcome through SetState and return false if the + // sequence was cancelled or the data was not collected properly, so the caller does not + // overwrite that with success. + bool TakeDarkMaskInternal(std::unique_lock &ul); void CalibrateDetector(std::unique_lock ul); - void CalibrateJUNGFRAU(std::unique_lock &ul); - void TakePedestalInternalG0(std::unique_lock &ul); - void TakePedestalInternalG1(std::unique_lock &ul, int32_t storage_cell = 0); - void TakePedestalInternalG2(std::unique_lock &ul, int32_t storage_cell = 0); + bool CalibrateJUNGFRAU(std::unique_lock &ul); + bool TakePedestalInternalG0(std::unique_lock &ul); + bool TakePedestalInternalG1(std::unique_lock &ul, int32_t storage_cell = 0); + bool TakePedestalInternalG2(std::unique_lock &ul, int32_t storage_cell = 0); bool ImportDetectorSettings(const DetectorSettings& input); void UpdateROIDefinition(); diff --git a/broker/jfjoch_api.yaml b/broker/jfjoch_api.yaml index 6a936f41..66daeedd 100644 --- a/broker/jfjoch_api.yaml +++ b/broker/jfjoch_api.yaml @@ -442,7 +442,7 @@ components: type: integer format: int64 minimum: 1 - maximum: 194 + maximum: 230 description: Number of space group for the crystal. Currently used solely as metadata, not relevant for image processing done in Jungfraujoch. sample_name: type: string diff --git a/tests/JFJochStateMachineTest.cpp b/tests/JFJochStateMachineTest.cpp index 884a39e3..3c43e8ee 100644 --- a/tests/JFJochStateMachineTest.cpp +++ b/tests/JFJochStateMachineTest.cpp @@ -315,3 +315,43 @@ TEST_CASE("JFJochStateMachine_AsyncStartFailure") { REQUIRE_NOTHROW(state_machine.WaitTillMeasurementDone()); REQUIRE(state_machine.GetStatus().state == JFJochState::Idle); } + +// A calibration that does not complete must not be reported as a good one. Every step signals +// failure by calling SetState and returning normally, so nothing threw and the sequence used to +// finish with an unconditional "Calibration sequence done"/Success on top of it - leaving the +// broker Idle and apparently ready while holding partial pedestals. +TEST_CASE("JFJochStateMachine_CalibrationFailure") { + Logger logger("JFJochStateMachine_CalibrationFailure"); + JFJochServices services(logger); + DiffractionExperiment experiment; + JFJochStateMachine state_machine(experiment, services, logger); + state_machine.AddDetectorSetup(DetJF4M()); + + // Ask for a G0 pedestal. There is no receiver, so no frames come back and the import fails - + // the same outcome as a pedestal that collected nothing on real hardware. + DetectorSettings settings = state_machine.GetDetectorSettings(); + settings.PedestalG0Frames(100); + REQUIRE_NOTHROW(state_machine.LoadDetectorSettings(settings)); + + REQUIRE_NOTHROW(state_machine.Initialize()); + REQUIRE_NOTHROW(state_machine.WaitTillMeasurementDone()); + + // Inactive, not Idle and not Error: the calibration is undefined, so the detector has to be + // initialized again rather than looking ready to measure. + auto status = state_machine.GetStatus(); + REQUIRE(status.state == JFJochState::Inactive); + REQUIRE(status.message_severity == BrokerStatus::MessageSeverity::Error); + REQUIRE(status.message == "Pedestal not collected properly"); + + // ... and a data collection is refused on it, instead of producing mis-converted images. + DatasetSettings setup; + REQUIRE_THROWS(state_machine.Start(setup)); + + // The success path is unchanged - with no pedestal to take, the sequence still reports success. + JFJochStateMachine calibrated(experiment, services, logger); + calibrated.AddDetectorSetup(DetJF4M()); + REQUIRE_NOTHROW(calibrated.Initialize()); + REQUIRE_NOTHROW(calibrated.WaitTillMeasurementDone()); + REQUIRE(calibrated.GetStatus().state == JFJochState::Idle); + REQUIRE(calibrated.GetStatus().message_severity == BrokerStatus::MessageSeverity::Success); +}