mx/Jungfraujoch
2
1
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases 71 Wiki Activity

Broker: stop reporting a failed calibration as a successful one

Browse Source 
Every calibration step signals failure by calling SetState and returning
normally - none of them throws, so none reached the catch in
CalibrateDetector. The unconditional SetState(Idle, "Calibration sequence
done", Success) after the try block then overwrote all of them.

/cancel during a JUNGFRAU pedestal therefore left the broker Idle and
apparently ready to measure while holding a truncated G0 and
default-constructed zeros for G1/G2, and every subsequent run was
silently mis-converted with nothing in /status to show it. The genuine
failures - "Pedestal not collected properly", "Mask not collected
properly" - were hidden the same way.

The steps now return whether they succeeded, and the sequence reports
success only if they all did. A cancellation or a failure leaves the
state Inactive with Error severity rather than Idle or Error: the
calibration is undefined, so the detector has to be initialized again,
which is what Inactive means everywhere else in the machine. The
exception path joins them, since a throw mid-sequence leaves the
calibration no better defined. Cancelled pedestals were already Inactive
but carried Warning severity, which reads as an advisory.

CalibrateJUNGFRAU now abandons the sequence at the first failure instead
of collecting G1 and G2 on top of a G0 that was never measured - the
cancel path already behaved that way - and ConfigureDetector is skipped
when there is no calibration to operate with, a cancelled sequence having
left the detector mid-abort.

Both error paths that end an Initialize now notify the condition
variable. The state has left Busy, but without the notification a client
in /wait_until_running slept out its whole timeout - up to an hour, if it
asked for one - before noticing a failure that had already happened.

Separately, dataset_settings.space_group_number allowed 1..194 in the
OpenAPI schema while the broker accepts 1..230, so every generated
client's validate() rejected all 36 cubic space groups before the request
left. The regenerated clients follow in the version bump.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uwv9ScHtDH6g8tYgfSuApo
This commit is contained in:
leonarski_f
2026-08-26 16:58:03 +02:00
co-authored by Claude Opus 5
parent a6a715703f
commit c29dd67b5f
4 changed files with 118 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
broker/JFJochStateMachine.cpp
+69 -40
View File
@@ -64,7 +64,7 @@ bool JFJochStateMachine::ImportPedestalG1G2(const JFJochReceiverOutput &receiver
return true;
}
void JFJochStateMachine::CalibrateJUNGFRAU(std::unique_lock<std::mutex> &ul) {
bool JFJochStateMachine::CalibrateJUNGFRAU(std::unique_lock<std::mutex> &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<std::mutex> &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<std::mutex> ul) {
@@ -90,46 +96,56 @@ void JFJochStateMachine::CalibrateDetector(std::unique_lock<std::mutex> 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<JFCalibration>(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<std::mutex> &ul) {
bool JFJochStateMachine::TakeDarkMaskInternal(std::unique_lock<std::mutex> &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<std::mutex> &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<std::mutex> &ul) {
bool JFJochStateMachine::TakePedestalInternalG0(std::unique_lock<std::mutex> &ul) {
DiffractionExperiment local_experiment(experiment);
std::string message;
if (local_experiment.IsFixedGainG1()) {
@@ -171,12 +189,12 @@ void JFJochStateMachine::TakePedestalInternalG0(std::unique_lock<std::mutex> &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<std::mutex> &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<std::mutex> &ul, int32_t storage_cell) {
bool JFJochStateMachine::TakePedestalInternalG1(std::unique_lock<std::mutex> &ul, int32_t storage_cell) {
DiffractionExperiment local_experiment(experiment);
local_experiment.Mode(DetectorMode::PedestalG1);
@@ -211,12 +231,12 @@ void JFJochStateMachine::TakePedestalInternalG1(std::unique_lock<std::mutex> &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<std::mutex> &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<std::mutex> &ul, int32_t storage_cell) {
bool JFJochStateMachine::TakePedestalInternalG2(std::unique_lock<std::mutex> &ul, int32_t storage_cell) {
DiffractionExperiment local_experiment(experiment);
local_experiment.Mode(DetectorMode::PedestalG2);
@@ -251,12 +274,12 @@ void JFJochStateMachine::TakePedestalInternalG2(std::unique_lock<std::mutex> &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<std::mutex> &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<std::mutex> 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));
broker/JFJochStateMachine.h
+8 -5
View File
@@ -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<std::mutex> &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<std::mutex> &ul);
void CalibrateDetector(std::unique_lock<std::mutex> ul);
void CalibrateJUNGFRAU(std::unique_lock<std::mutex> &ul);
void TakePedestalInternalG0(std::unique_lock<std::mutex> &ul);
void TakePedestalInternalG1(std::unique_lock<std::mutex> &ul, int32_t storage_cell = 0);
void TakePedestalInternalG2(std::unique_lock<std::mutex> &ul, int32_t storage_cell = 0);
bool CalibrateJUNGFRAU(std::unique_lock<std::mutex> &ul);
bool TakePedestalInternalG0(std::unique_lock<std::mutex> &ul);
bool TakePedestalInternalG1(std::unique_lock<std::mutex> &ul, int32_t storage_cell = 0);
bool TakePedestalInternalG2(std::unique_lock<std::mutex> &ul, int32_t storage_cell = 0);
bool ImportDetectorSettings(const DetectorSettings& input);
void UpdateROIDefinition();
broker/jfjoch_api.yaml
+1 -1
View File
@@ -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
tests/JFJochStateMachineTest.cpp
+40
View File
@@ -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);
}