diff --git a/common/DetectorSetup.cpp b/common/DetectorSetup.cpp index f6b2b3cc..c00ef508 100644 --- a/common/DetectorSetup.cpp +++ b/common/DetectorSetup.cpp @@ -74,8 +74,10 @@ DetectorSetup::DetectorSetup(std::shared_ptr in_geometry, break; case DetectorType::DECTRIS: high_voltage = 0; - bit_depth_readout = 16; - bit_depth_image = 16; + // bit_depth_readout stays unset: a DECTRIS detector's electronic readout depth is a + // constant we never need, and what is reported downstream is the image depth (see + // DiffractionExperiment::FillMessage). + bit_depth_image = 16; // placeholder, replaced from the stream2 image_dtype when armed read_out_time = std::chrono::microseconds(0); if (!det_modules_hostname.empty() && ( det_modules_hostname.size() != 1)) throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, @@ -324,23 +326,6 @@ DetectorSetup &DetectorSetup::BitDepthImage(int64_t input) { } } -DetectorSetup &DetectorSetup::BitDepthReadout(int64_t input) { - if (GetDetectorType() != DetectorType::DECTRIS) - throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, - "Bit depth readout can be only changed for DECTRIS detector"); - switch (input) { - case 8: - case 12: - case 16: - case 32: - bit_depth_readout = input; - return *this; - default: - throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, - "Bit depth readout can be only 8, 12, 16 or 32"); - } -} - std::optional DetectorSetup::GetBitDepthReadout() const { return bit_depth_readout; } diff --git a/common/DetectorSetup.h b/common/DetectorSetup.h index 4a2472b2..e0c84e54 100644 --- a/common/DetectorSetup.h +++ b/common/DetectorSetup.h @@ -76,7 +76,6 @@ public: DetectorSetup& ModuleSync(bool input); DetectorSetup& ReadOutTime(std::chrono::nanoseconds input); DetectorSetup& Geometry(const DetectorGeometryFixed& input); - DetectorSetup& BitDepthReadout(int64_t input); DetectorSetup& BitDepthImage(int64_t input); DetectorSetup& MinFrameTime(std::chrono::nanoseconds input); DetectorSetup& MinCountTime(std::chrono::nanoseconds input); diff --git a/common/DiffractionExperiment.cpp b/common/DiffractionExperiment.cpp index 1015bd0b..30e37503 100644 --- a/common/DiffractionExperiment.cpp +++ b/common/DiffractionExperiment.cpp @@ -669,7 +669,17 @@ void DiffractionExperiment::FillMessage(StartMessage &message) const { message.sensor_material = detector.GetSensorMaterial(); message.sensor_thickness = detector.GetSensorThickness_um() * 1e-6f; message.bit_depth_image = GetByteDepthImage() * 8; - message.bit_depth_readout = GetBitDepthReadout(); + // Both fields carry the depth of the STORED image, deliberately. NXmx has no field for it - + // only bit_depth_readout, defined as the depth of the detector electronics - so every NXmx + // reader takes bit_depth_readout as the width of the stored pixel (DIALS uses it to decide + // which values are masking markers, and cannot read a 32-bit image without it). The electronic + // readout depth is a constant of the detector and of no use to a data consumer, so reporting it + // here would only mislead: it differs from the image depth exactly when summation widens the + // image, which is when getting it wrong does damage. + message.bit_depth_readout = message.bit_depth_image; + // Lowest valid value, so a reader can tell the error marker from data. Unsigned images use 0; + // signed ones reserve INTx_MIN as the marker (GetUnderflow), so the lowest valid is one above. + message.underload_value = IsPixelSigned() ? GetUnderflow() + 1 : 0; message.indexing_algorithm = GetIndexingAlgorithm(); message.images_per_trigger = dataset.GetImageNumPerTrigger(); diff --git a/common/JFJochMessages.h b/common/JFJochMessages.h index 9a8989e1..ffbeecc6 100644 --- a/common/JFJochMessages.h +++ b/common/JFJochMessages.h @@ -230,6 +230,7 @@ struct StartMessage { int64_t saturation_value; std::optional error_value; + std::optional underload_value; // NXmx: lowest valid value float pixel_size_x; float pixel_size_y; diff --git a/detector_control/DectrisSimplonClient.cpp b/detector_control/DectrisSimplonClient.cpp index 1170fc51..0da7b7c9 100644 --- a/detector_control/DectrisSimplonClient.cpp +++ b/detector_control/DectrisSimplonClient.cpp @@ -198,7 +198,9 @@ void DectrisSimplonClient::ReadDetectorConfig(DetectorSetup &setup) { setup.SensorMaterial(GetDetCfg( "sensor_material").val); setup.SensorThickness_um(std::round(GetDetCfg( "sensor_thickness").val.get() * 1e6f)); - setup.BitDepthReadout(GetDetCfg("bit_depth_readout").val.get()); + // Only the image depth is read: it sizes the image buffer before the first frame arrives, and + // is replaced from the stream2 image_dtype when the detector arms. The electronic readout depth + // is deliberately not read - nothing consumes it. setup.BitDepthImage(GetDetCfg("bit_depth_image").val.get()); setup.MinFrameTime(float2time(GetDetCfg("frame_time").min.get())); setup.MinCountTime(float2time(GetDetCfg("count_time").min.get())); diff --git a/docs/CBOR.md b/docs/CBOR.md index 93f6ae24..fa411393 100644 --- a/docs/CBOR.md +++ b/docs/CBOR.md @@ -69,7 +69,7 @@ There are minor differences at the moment: | storage_cell_number | uint64 (optional) | Number of storage cells used by JUNGFRAU | | | storage_cell_delay | Rational | Delay of storage cells in JUNGFRAU | | | threshold_energy | Map(string -> float) | Per-channel threshold energy \[eV\] (map of channel name to value) | | -| image_dtype | string | Pixel bit type (e.g. uint16) | X | +| image_dtype | string | Pixel type of the image data: `uint8`, `uint16`, `uint32` (DECTRIS), plus `int8`, `int16`, `int32` as a Jungfraujoch extension. Sole wire encoding of both the bit depth and the sign, and must agree with the per-image typed-array tag | X | | unit_cell | object (optional) | Unit cell of the system: a, b, c \[angstrom\] and alpha, beta, gamma \[degree\] | | | az_int_q_bin_count | uint64 | Number of azimuthal integration bins in the radial direction | | | az_int_phi_bin_count | uint64 | Number of azimuthal integration bins in the phi angle direction | | @@ -104,7 +104,8 @@ There are minor differences at the moment: | - experiment_group | string | ID of instrument user, e.g., p-group (SLS/SwissFEL) or proposal number | | | - jfjoch_release | string | Jungfraujoch release number | | | - socket_number | uint64 | Number of ZeroMQ socket (on `jfjoch_broker` side) used for transmission | | -| - bit_depth_readout | uint64 | Bit depth of the detector readout | | +| - bit_depth_readout | uint64 | Bit depth of the **stored image** (see note below), copied to NXmx `bit_depth_readout` | | +| - underload_value | int64 | Lowest valid value; copied to NXmx `underload_value`. `0` for an unsigned image, `INTx_MIN + 1` for a signed one | | | - writer_notification_zmq_addr | string | ZeroMQ address to inform `jfjoch_broker` about writers that finished operation | | | - xfel_pulse_id | uint64 | Pulse IDs are recorded for images | | | - ring_current_mA | float | Ring current at the start of the measurement | | diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 56260509..68e417c1 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -5,6 +5,9 @@ This is an UNSTABLE release. It includes many experimental features, as well as * CUDA builds made with a CUDA 12 toolkit now also contain device code for Volta, so the RHEL 8 packages and the portable Linux `.tgz` run on a V100; CUDA 13 dropped Volta, so the RHEL 9, Ubuntu and Windows artefacts remain Turing and newer. * Documentation: the supported GPU generations and the minimum NVIDIA driver version of every released artefact. +* `bit_depth_readout` now reports the bit depth of the stored image rather than the detector's electronic readout depth, which is what NXmx readers expect - DIALS derives its masking markers from that field and cannot read a 32-bit image without it. +* NXmx `underload_value` (lowest valid value) is now written, so a reader masks the error-pixel marker instead of treating it as an intensity. +* A DECTRIS detector sending signed images is no longer declared unsigned in the outgoing stream and in HDF5. ### 1.0.0-rc.161 This is an UNSTABLE release. It includes many experimental features, as well as many AI generated fixes. We recommend using rc.152 for production use. diff --git a/docs/HDF5.md b/docs/HDF5.md index fad40276..20941532 100644 --- a/docs/HDF5.md +++ b/docs/HDF5.md @@ -160,13 +160,31 @@ File-level HDF5 attributes `file_name`, `file_time`, `HDF5_Version` are also set | `threshold_energy` | NXmx | eV (EIGER; written only for a single channel) | | `x_pixel_size`, `y_pixel_size` | base | m | | `serial_number` | base | | -| `bit_depth_readout` | NXmx | | -| `saturation_value` | NXmx | | +| `bit_depth_readout` | NXmx | bit depth of the **stored image**, not of the detector electronics - see below | +| `saturation_value` | NXmx | highest valid value | +| `underload_value` | NXmx | lowest valid value: `0` for an unsigned image, `INTx_MIN + 1` for a signed one | | `flatfield_applied` | NXmx | | | `pixel_mask`, `pixel_mask_applied` | NXmx | `pixel_mask` is `[y, x]`, hard-linked from `detectorSpecific/pixel_mask` | | `countrate_correction_applied` | NXmx | | | `number_of_cycles` | base | frame-summation factor | +#### Why `bit_depth_readout` is the image depth + +NXmx defines only `bit_depth_readout`, "how many bits the electronics record per pixel", and has no +field for the depth of the image actually stored. The two differ whenever summation is used: the +readout stays at the detector's native width while the summed image must be wider to hold the sum. + +Jungfraujoch writes the **stored image depth** into `bit_depth_readout` (and the identical value +into the non-standard `bit_depth_image`). The electronic value is a constant of the detector and +tells a data consumer nothing, whereas readers do use `bit_depth_readout` as the width of the +stored pixel — DIALS, for instance, derives its masking markers from it and cannot read a 32-bit +image without it. Writing the electronic value there would therefore mislead exactly in the case +where the two differ. + +Note that `bit_depth_readout` gives the width only. The **sign** is carried solely by the HDF5 +element type of `/entry/data/data` (and, on the wire, by `image_dtype`); there is no NXmx field for +it. + ### `/entry/instrument/detector/transformations` (NXtransformations) The NXtransformations *mechanism* (the `depends_on` chain, `transformation_type`, `vector`, @@ -425,8 +443,8 @@ group for compatibility with existing tooling: |-------|-------|---------| | `detector_distance` | m | duplicate of `distance` (Dectris/Neggia compatibility) | | `detector_number` | | detector identifier (Dectris convention) | -| `error_value` | | masked/error pixel sentinel (NXmx standard would be `underload_value`) | -| `bit_depth_image` | | stored image bit depth (NXmx standard is `bit_depth_readout`) | +| `error_value` | | masked/error pixel sentinel: `UINTx_MAX` unsigned, `INTx_MIN` signed (NXmx has no equivalent; `underload_value` is written too, and is one above it) | +| `bit_depth_image` | | stored image bit depth; equal to `bit_depth_readout` (DECTRIS convention, not NXmx) | | `acquisition_type` | | always `triggered` (Dectris convention) | | `jungfrau_conversion_applied` | | JUNGFRAU photon/keV conversion applied | | `jungfrau_conversion_factor` | eV | conversion factor | diff --git a/frame_serialize/CBORStream2Deserializer.cpp b/frame_serialize/CBORStream2Deserializer.cpp index 6ba60651..d62460a7 100644 --- a/frame_serialize/CBORStream2Deserializer.cpp +++ b/frame_serialize/CBORStream2Deserializer.cpp @@ -1128,6 +1128,8 @@ namespace { message.writer_notification_zmq_addr = j["writer_notification_zmq_addr"]; if (j.contains("bit_depth_readout")) message.bit_depth_readout = j["bit_depth_readout"]; + if (j.contains("underload_value")) + message.underload_value = j["underload_value"]; if (j.contains("summation_mode")) message.summation_mode = j["summation_mode"]; if (j.contains("overwrite")) diff --git a/frame_serialize/CBORStream2Serializer.cpp b/frame_serialize/CBORStream2Serializer.cpp index 21056641..e2b8d549 100644 --- a/frame_serialize/CBORStream2Serializer.cpp +++ b/frame_serialize/CBORStream2Serializer.cpp @@ -569,6 +569,8 @@ inline void CBOR_ENC_START_USER_DATA(CborEncoder& encoder, const char* key, j["socket_number"] = message.socket_number.value(); if (message.bit_depth_readout) j["bit_depth_readout"] = message.bit_depth_readout.value(); + if (message.underload_value) + j["underload_value"] = message.underload_value.value(); if (!message.writer_notification_zmq_addr.empty()) j["writer_notification_zmq_addr"] = message.writer_notification_zmq_addr; if (message.summation_mode.has_value()) diff --git a/receiver/JFJochReceiverLite.cpp b/receiver/JFJochReceiverLite.cpp index 9b5dde20..06b8ab7a 100644 --- a/receiver/JFJochReceiverLite.cpp +++ b/receiver/JFJochReceiverLite.cpp @@ -205,10 +205,11 @@ void JFJochReceiverLite::Configure(const StartMessage &msg) { experiment.Detector().SensorMaterial(msg.sensor_material); experiment.Detector().SensorThickness_um(msg.sensor_thickness * 1e6); experiment.Detector().SaturationLimit(msg.saturation_value); + // Images are forwarded byte-for-byte, so the stream's own image_dtype - not anything configured + // locally - decides both the width and the sign the outgoing metadata must declare. Taking only + // the width used to leave a signed stream declared unsigned in NXmx. experiment.Detector().BitDepthImage(msg.bit_depth_image); - - if (msg.bit_depth_readout) - experiment.Detector().BitDepthReadout(msg.bit_depth_readout.value()); + experiment.PixelSigned(msg.pixel_signed); } void JFJochReceiverLite::MaskThread(uint32_t id) { diff --git a/tests/DetectorSetupTest.cpp b/tests/DetectorSetupTest.cpp index 5e81b8c5..9f1d316e 100644 --- a/tests/DetectorSetupTest.cpp +++ b/tests/DetectorSetupTest.cpp @@ -32,26 +32,17 @@ TEST_CASE("DetectorSetup_MismatchInGeometry") { } TEST_CASE("DetectorSetup_ReadoutDepth") { + // The electronic readout depth is only carried where the acquisition needs it: JUNGFRAU is + // fixed at 16, PSI EIGER takes it from the detector settings, and a DECTRIS detector does not + // carry one at all - what it reports downstream is the image depth. auto setup = DetDECTRIS(123,123, "zzz", "a"); - REQUIRE_NOTHROW(setup.BitDepthReadout(16)); - REQUIRE(setup.GetBitDepthReadout() == 16); - REQUIRE_NOTHROW(setup.BitDepthReadout(12)); - REQUIRE(setup.GetBitDepthReadout() == 12); - REQUIRE_NOTHROW(setup.BitDepthReadout(32)); - REQUIRE(setup.GetBitDepthReadout() == 32); - REQUIRE_NOTHROW(setup.BitDepthReadout(8)); - REQUIRE(setup.GetBitDepthReadout() == 8); - REQUIRE_THROWS(setup.BitDepthReadout(0)); - REQUIRE_THROWS(setup.BitDepthReadout(15)); - REQUIRE_THROWS(setup.BitDepthReadout(-1)); + REQUIRE(!setup.GetBitDepthReadout()); auto setup2 = DetJF(1); REQUIRE(setup2.GetBitDepthReadout() == 16); - REQUIRE_THROWS(setup2.BitDepthReadout(32)); auto setup3 = DetEIGER(1); REQUIRE(!setup3.GetBitDepthReadout()); - REQUIRE_THROWS(setup3.BitDepthReadout(32)); } TEST_CASE("DetectorSetup_ImageDepth") { diff --git a/tools/jfjoch_simplon_test.cpp b/tools/jfjoch_simplon_test.cpp index 1da1a3a4..cb917e0c 100644 --- a/tools/jfjoch_simplon_test.cpp +++ b/tools/jfjoch_simplon_test.cpp @@ -50,8 +50,6 @@ int main(int argc, char **argv) { setup.GetGeometry().GetHeight(true)); WriteTIFFToFile("det_mask.tiff", image); - if (setup.GetBitDepthReadout()) - logger.Info("Bit depth readout {:8d}", setup.GetBitDepthReadout().value()); if (setup.GetBitDepthImage()) logger.Info("Bit depth image {:8d}", setup.GetBitDepthImage().value()); diff --git a/writer/HDF5NXmx.cpp b/writer/HDF5NXmx.cpp index 5ab2a398..0bcb9e5f 100644 --- a/writer/HDF5NXmx.cpp +++ b/writer/HDF5NXmx.cpp @@ -365,6 +365,8 @@ void NXmx::Detector(const StartMessage &start) { if (start.bit_depth_readout) SaveScalar(group, "bit_depth_readout", start.bit_depth_readout.value()); SaveScalar(group, "saturation_value", start.saturation_value); + if (start.underload_value) + SaveScalar(group, "underload_value", start.underload_value.value()); if (start.error_value) SaveScalar(group, "error_value", start.error_value.value()); // this is not NXmx SaveScalar(group, "flatfield_applied", start.flatfield_enabled);