From e9ac34093c2ded13e3a1f3c621495b3c92b59b5b Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Sun, 26 Apr 2026 18:55:35 +0900 Subject: [PATCH 01/17] Explicit object to represent sequence acquisitions SequenceAcquisition is created for each new sequence acquisition and is cleaned up (for now) lazily. Better cleanup will be possible once we start tracking cameras participating in composite cameras (like Multi Camera). This is a start toward more directly managing sequence acquisitions by the Core, so that multiple cameras/acquisitions can be better supported and thread safety can be improved. For now, it does little more than just create the SequenceAcquisition object. (Assisted by Claude Code; any errors are mine.) --- MMCore/MMCore.cpp | 43 ++++++- MMCore/MMCore.h | 18 +++ MMCore/MMCore.vcxproj | 2 + MMCore/MMCore.vcxproj.filters | 6 + MMCore/Makefile.am | 2 + MMCore/SequenceAcquisition.cpp | 44 +++++++ MMCore/SequenceAcquisition.h | 59 +++++++++ MMCore/meson.build | 1 + MMCore/unittest/UnloadDevice-Tests.cpp | 166 +++++++++++++++++++++++++ 9 files changed, 338 insertions(+), 3 deletions(-) create mode 100644 MMCore/SequenceAcquisition.cpp create mode 100644 MMCore/SequenceAcquisition.h diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index c873141f6..4a7c23a98 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -51,6 +51,7 @@ #include "MMEventCallback.h" #include "NotificationQueue.h" #include "PluginManager.h" +#include "SequenceAcquisition.h" #include "SynchronizedConfiguration.h" #include "DeviceUtils.h" @@ -873,6 +874,18 @@ void CMMCore::removeAllDeviceRoles() { setSLMDevice(""); } +void CMMCore::stopAllSequenceAcquisitionsForUnload() +{ + for (auto& kv : acquisitions_) { + auto& cam = kv.second->Camera(); + mmi::DeviceModuleLockGuard guard(cam); + if (cam->IsCapturing()) + cam->StopSequenceAcquisition(); + } + acquisitions_.clear(); +} + + /** * Unloads the device from the core and adjusts all configuration data. */ @@ -887,6 +900,8 @@ void CMMCore::unloadDevice(const char* label///< the name of the device to unloa std::shared_ptr pDevice = deviceManager_->GetDevice(label); + stopAllSequenceAcquisitionsForUnload(); + try { removeDeviceRole(pDevice); @@ -909,6 +924,8 @@ void CMMCore::unloadDevice(const char* label///< the name of the device to unloa */ void CMMCore::unloadAllDevices() MMCORE_LEGACY_THROW(CMMError) { + stopAllSequenceAcquisitionsForUnload(); + try { removeAllDeviceRoles(); @@ -3060,13 +3077,17 @@ void CMMCore::startSequenceAcquisition(long numImages, double unused, bool stopO mmi::DeviceModuleLockGuard guard(camera); LOG_DEBUG(coreLogger_) << "Will start sequence acquisition from default camera"; + acquisitions_[camera->GetLabel()] = + mmcore::internal::SequenceAcquisition::Create(camera); // Forward `unused` to the device rather than substituting 0.0: // a small number of camera adapters (Andor) did implement this // parameter, and passing 0.0 unconditionally could regress them. // The MM::Camera contract now says new adapters must ignore it. int nRet = camera->StartSequenceAcquisition(numImages, unused, stopOnOverflow); - if (nRet != DEVICE_OK) + if (nRet != DEVICE_OK) { + acquisitions_.erase(camera->GetLabel()); throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); + } } catch (std::bad_alloc& ex) { @@ -3121,13 +3142,17 @@ void CMMCore::startSequenceAcquisition(const char* label, long numImages, double cbuf_->SetOverwriteData(!stopOnOverflow); LOG_DEBUG(coreLogger_) << "Will start sequence acquisition from camera " << label; + acquisitions_[pCam->GetLabel()] = + mmcore::internal::SequenceAcquisition::Create(pCam); // Forward `unused` to the device rather than substituting 0.0: a small // number of camera adapters (Andor) did implement this parameter, and // passing 0.0 unconditionally could regress them. The MM::Camera // contract now says new adapters must ignore it. int nRet = pCam->StartSequenceAcquisition(numImages, unused, stopOnOverflow); - if (nRet != DEVICE_OK) + if (nRet != DEVICE_OK) { + acquisitions_.erase(pCam->GetLabel()); throw CMMError(getDeviceErrorText(nRet, pCam).c_str(), MMERR_DEVICE_GENERIC); + } LOG_DEBUG(coreLogger_) << "Did start sequence acquisition from camera " << label; @@ -3198,6 +3223,11 @@ void CMMCore::stopSequenceAcquisition(const char* label) MMCORE_LEGACY_THROW(CMM logError(label, getDeviceErrorText(nRet, pCam).c_str()); throw CMMError(getDeviceErrorText(nRet, pCam).c_str(), MMERR_DEVICE_GENERIC); } + { + auto it = acquisitions_.find(pCam->GetLabel()); + if (it != acquisitions_.end()) + it->second->MarkStopRequested(); + } LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from camera " << label; // onSequenceAcquisitionStopped will be called by CoreCallback::AcqFinished @@ -3235,13 +3265,17 @@ void CMMCore::startContinuousSequenceAcquisition(double unused) MMCORE_LEGACY_TH callback_->ResetImageInsertionState(); cbuf_->SetOverwriteData(true); LOG_DEBUG(coreLogger_) << "Will start continuous sequence acquisition from current camera"; + acquisitions_[camera->GetLabel()] = + mmcore::internal::SequenceAcquisition::Create(camera); // Forward `unused` to the device rather than substituting 0.0: a small // number of camera adapters (Andor) did implement this parameter, and // passing 0.0 unconditionally could regress them. The MM::Camera // contract now says new adapters must ignore it. int nRet = camera->StartSequenceAcquisition(unused); - if (nRet != DEVICE_OK) + if (nRet != DEVICE_OK) { + acquisitions_.erase(camera->GetLabel()); throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); + } } else { @@ -3268,6 +3302,9 @@ void CMMCore::stopSequenceAcquisition() MMCORE_LEGACY_THROW(CMMError) logError(getDeviceName(camera).c_str(), getDeviceErrorText(nRet, camera).c_str()); throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); } + auto it = acquisitions_.find(camera->GetLabel()); + if (it != acquisitions_.end()) + it->second->MarkStopRequested(); } else { diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index 5b3a2e3d5..2d3ff17cf 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -95,6 +95,7 @@ namespace internal { class DeviceManager; class LogManager; class NotificationQueue; + class SequenceAcquisition; } // namespace internal } // namespace mmcore @@ -732,6 +733,14 @@ class CMMCore std::unique_ptr cbuf_; std::unique_ptr callback_; + // Active sequence acquisitions keyed by logical-camera label. At most + // one entry per camera, but multiple cameras may have concurrent + // acquisitions because the labelled start/stop API is per-camera (sort of). + // Mutated only from the user thread. + std::map> + acquisitions_; + std::shared_ptr pluginManager_; std::shared_ptr deviceManager_; std::map errorText_; @@ -773,6 +782,15 @@ class CMMCore void removeAllDeviceRoles(); void loadSystemConfigurationImpl(const char* fileName) MMCORE_LEGACY_THROW(CMMError); + // Stop all in-flight sequence acquisitions and clear the acquisitions_ + // map. Called before unloading any device: acquisitions_ holds strong + // references to CameraInstance, which would otherwise outlive the + // unload and leave a zombie wrapper around a shut-down adapter. + // Stops every acquisition (not just one for the unloaded camera) + // because Core (for now) does not know which logical cameras a given + // physical device participates in (e.g. via Multi Camera). + void stopAllSequenceAcquisitionsForUnload(); + void setCameraInternal(const std::string& label); void setShutterInternal(const std::string& label); void setFocusInternal(const std::string& label); diff --git a/MMCore/MMCore.vcxproj b/MMCore/MMCore.vcxproj index ec3486963..e4a67db6e 100644 --- a/MMCore/MMCore.vcxproj +++ b/MMCore/MMCore.vcxproj @@ -113,6 +113,7 @@ + @@ -183,6 +184,7 @@ + diff --git a/MMCore/MMCore.vcxproj.filters b/MMCore/MMCore.vcxproj.filters index 67a3cf0b8..806769852 100644 --- a/MMCore/MMCore.vcxproj.filters +++ b/MMCore/MMCore.vcxproj.filters @@ -129,6 +129,9 @@ Source Files + + Source Files + Source Files @@ -329,6 +332,9 @@ Header Files + + Header Files + Header Files diff --git a/MMCore/Makefile.am b/MMCore/Makefile.am index 7f94e6d4f..554c8590e 100644 --- a/MMCore/Makefile.am +++ b/MMCore/Makefile.am @@ -109,6 +109,8 @@ libMMCore_la_SOURCES = \ SerializedMetadata.h \ Semaphore.cpp \ Semaphore.h \ + SequenceAcquisition.cpp \ + SequenceAcquisition.h \ SynchronizedConfiguration.h \ Task.cpp \ Task.h \ diff --git a/MMCore/SequenceAcquisition.cpp b/MMCore/SequenceAcquisition.cpp new file mode 100644 index 000000000..90747b230 --- /dev/null +++ b/MMCore/SequenceAcquisition.cpp @@ -0,0 +1,44 @@ +// PROJECT: Micro-Manager +// SUBSYSTEM: MMCore +// +// COPYRIGHT: University of California, San Francisco, 2026, +// All Rights reserved +// +// LICENSE: This file is distributed under the "Lesser GPL" (LGPL) license. +// License text is included with the source distribution. +// +// This file is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty +// of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. +// +// IN NO EVENT SHALL THE COPYRIGHT OWNER OR +// CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, +// INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES. + +#include "SequenceAcquisition.h" + +#include "Devices/CameraInstance.h" + +#include + +namespace mmcore { +namespace internal { + +std::shared_ptr +SequenceAcquisition::Create(std::shared_ptr camera) +{ + return std::shared_ptr( + new SequenceAcquisition(std::move(camera))); +} + +SequenceAcquisition::SequenceAcquisition( + std::shared_ptr camera) : + cameraLabel_(camera->GetLabel()), + camera_(std::move(camera)) +{ +} + +SequenceAcquisition::~SequenceAcquisition() = default; + +} // namespace internal +} // namespace mmcore diff --git a/MMCore/SequenceAcquisition.h b/MMCore/SequenceAcquisition.h new file mode 100644 index 000000000..0bee7f178 --- /dev/null +++ b/MMCore/SequenceAcquisition.h @@ -0,0 +1,59 @@ +// PROJECT: Micro-Manager +// SUBSYSTEM: MMCore +// +// COPYRIGHT: University of California, San Francisco, 2026, +// All Rights reserved +// +// LICENSE: This file is distributed under the "Lesser GPL" (LGPL) license. +// License text is included with the source distribution. +// +// This file is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty +// of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. +// +// IN NO EVENT SHALL THE COPYRIGHT OWNER OR +// CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, +// INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES. + +#pragma once + +#include +#include +#include + +namespace mmcore { +namespace internal { + +class CameraInstance; + +class SequenceAcquisition { +public: + static std::shared_ptr Create( + std::shared_ptr camera); + ~SequenceAcquisition(); + + SequenceAcquisition(const SequenceAcquisition&) = delete; + SequenceAcquisition& operator=(const SequenceAcquisition&) = delete; + + const std::string& CameraLabel() const noexcept { return cameraLabel_; } + const std::shared_ptr& Camera() const noexcept { + return camera_; + } + + bool WasStopRequested() const noexcept { + return stopRequested_.load(std::memory_order_acquire); + } + void MarkStopRequested() noexcept { + stopRequested_.store(true, std::memory_order_release); + } + +private: + explicit SequenceAcquisition(std::shared_ptr camera); + + const std::string cameraLabel_; + const std::shared_ptr camera_; + std::atomic stopRequested_{false}; +}; + +} // namespace internal +} // namespace mmcore diff --git a/MMCore/meson.build b/MMCore/meson.build index 71a828d70..59767779c 100644 --- a/MMCore/meson.build +++ b/MMCore/meson.build @@ -64,6 +64,7 @@ mmcore_sources = files( 'MMCore.cpp', 'PluginManager.cpp', 'Semaphore.cpp', + 'SequenceAcquisition.cpp', 'Task.cpp', 'TaskSet.cpp', 'TaskSet_CopyMemory.cpp', diff --git a/MMCore/unittest/UnloadDevice-Tests.cpp b/MMCore/unittest/UnloadDevice-Tests.cpp index 8f6623a5b..34b336013 100644 --- a/MMCore/unittest/UnloadDevice-Tests.cpp +++ b/MMCore/unittest/UnloadDevice-Tests.cpp @@ -4,6 +4,150 @@ #include "MockDeviceUtils.h" #include "StubDevices.h" +#include +#include +#include +#include +#include + +namespace { + +// Camera that produces images on its own thread between Start and Stop. +struct AsyncCamera : CCameraBase { + std::string name = "AsyncCamera"; + unsigned width = 64; + unsigned height = 64; + unsigned bytesPerPixel = 1; + unsigned nComponents = 1; + unsigned bitDepth = 8; + int binning = 1; + double exposure = 10.0; + + int Initialize() override { return DEVICE_OK; } + int Shutdown() override { return DEVICE_OK; } + bool Busy() override { return false; } + void GetName(char* buf) const override { + CDeviceUtils::CopyLimitedString(buf, name.c_str()); + } + + int SnapImage() override { + imgBuf_.assign( + static_cast(width) * height * bytesPerPixel, 0); + return DEVICE_OK; + } + const unsigned char* GetImageBuffer() override { return imgBuf_.data(); } + long GetImageBufferSize() const override { + return static_cast(width) * height * bytesPerPixel; + } + unsigned GetImageWidth() const override { return width; } + unsigned GetImageHeight() const override { return height; } + unsigned GetImageBytesPerPixel() const override { return bytesPerPixel; } + unsigned GetNumberOfComponents() const override { return nComponents; } + unsigned GetBitDepth() const override { return bitDepth; } + int GetBinning() const override { return binning; } + int SetBinning(int b) override { binning = b; return DEVICE_OK; } + void SetExposure(double e) override { exposure = e; } + double GetExposure() const override { return exposure; } + int SetROI(unsigned, unsigned, unsigned, unsigned) override { + return DEVICE_OK; + } + int GetROI(unsigned& x, unsigned& y, unsigned& w, unsigned& h) override { + x = 0; y = 0; w = width; h = height; + return DEVICE_OK; + } + int ClearROI() override { return DEVICE_OK; } + int IsExposureSequenceable(bool& seq) const override { + seq = false; + return DEVICE_OK; + } + + int StartSequenceAcquisition(long numImages, double, bool) override { + GetCoreCallback()->PrepareForAcq(this); + { + std::lock_guard lk(mu_); + running_ = true; + stopRequested_ = false; + } + thread_ = std::thread([this, numImages] { AcqThread(numImages); }); + return DEVICE_OK; + } + + int StartSequenceAcquisition(double) override { + GetCoreCallback()->PrepareForAcq(this); + { + std::lock_guard lk(mu_); + running_ = true; + stopRequested_ = false; + } + thread_ = std::thread([this] { AcqThread(-1); }); + return DEVICE_OK; + } + + ~AsyncCamera() { + { + std::lock_guard lk(mu_); + stopRequested_ = true; + } + cv_.notify_one(); + if (thread_.joinable()) + thread_.join(); + } + + int StopSequenceAcquisition() override { + { + std::lock_guard lk(mu_); + stopRequested_ = true; + } + cv_.notify_one(); + if (thread_.joinable()) + thread_.join(); + return DEVICE_OK; + } + + bool IsCapturing() override { + std::lock_guard lk(mu_); + return running_; + } + +private: + void AcqThread(long numImages) { + std::vector buf( + static_cast(width) * height * bytesPerPixel, 0); + long count = 0; + while (numImages < 0 || count < numImages) { + { + std::lock_guard lk(mu_); + if (stopRequested_) + break; + } + GetCoreCallback()->InsertImage(this, buf.data(), + width, height, bytesPerPixel, nComponents, "{}"); + ++count; + { + std::unique_lock lk(mu_); + cv_.wait_for(lk, std::chrono::microseconds(100), + [this] { return stopRequested_; }); + if (stopRequested_) + break; + } + } + GetCoreCallback()->AcqFinished(this, 0); + { + std::lock_guard lk(mu_); + running_ = false; + } + } + + bool running_ = false; + bool stopRequested_ = false; + std::mutex mu_; + std::condition_variable cv_; + std::thread thread_; + std::vector imgBuf_; +}; + +} // namespace + TEST_CASE("Unload the current camera", "[regression]") { StubCamera cam; MockAdapterWithDevices adapter{{"cam", &cam}}; @@ -119,3 +263,25 @@ TEST_CASE("Unload all devices with a magnifier loaded", "[regression]") { c.unloadAllDevices(); } + +TEST_CASE("unloadAllDevices during sequence acquisition does not crash", + "[Unload]") { + AsyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.startContinuousSequenceAcquisition(0.0); + CHECK_NOTHROW(c.unloadAllDevices()); +} + +TEST_CASE("unloadDevice during sequence acquisition does not crash", + "[Unload]") { + AsyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.startContinuousSequenceAcquisition(0.0); + CHECK_NOTHROW(c.unloadDevice("cam")); +} From 0437c6d4a7b643dd2020f179097f6a164a11e8d9 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Thu, 30 Apr 2026 13:44:31 +0900 Subject: [PATCH 02/17] Let MMCore support multi-channel camera seq acq Simplify the way in which frame tags are generated for multi-channel cameras. Especially for composite cameras (e.g. Multi Camera), avoid mutating the physical camera objects (which could not be cleanly undone at the right timing) and instead map physical cameras to tags in the Core. Known behavioral changes: 1. Physical cameras associated with Multi Camera (or similar composite cameras) no longer incorrectly generate camera channel name/index tags when running sequence acquisition as an individual camera. 2. When autoshutter is enabled, composite cameras no longer cause the shutter to be opened/closed multiple times (even though it was idempotent). None of the physical cameras start acquiring until the shutter finishes opening, and the shutter is not closed until all physical cameras finish/stop acquiring. 3. If opening the shutter fails, `PrepareForAcq()` now returns an error. 4. `PrepareForAcq/InsertImage/AcqFinished` callbacks will fail if not issued from a camera currently participating in a sequence acquisition (previously any device could theoretically call these at any time and send images to the buffer). 5. The `SequenceAcquisitionStarted/Stopped` notifications now use the primary camera label and are issued once, instead of for each physical camera, in the case of a composite multi-channel camera. 6. Intrinsic multi-channel cameras must tag images with a valid CameraChannelIndex that is in range; this is now verified. CameraChannelName is now added by MMCore so does not need to be added by the camera. 7. Nested multi-channel cameras are explicitly detected and disallowed. Bumps the device interface version due to the following changes: - In `MM::Camera`, add `GetChannelCameraPtr()` so that MMCore can query the primary camera for all participating (physical) cameras. - Remove the `AddTag/RemoveTag` mechanism; instead, let the `SequenceAcquisition` object manage all participating cameras in a composite multi-channel camera acquisition, and generate tags (`CameraChannelIndex`, `CameraChannelName`) during `InsertImage()` in MMCore, where possible (`CameraChannelIndex` is still required from intrinsic multi-channel cameras, because they come from the same source). Note: the CameraChannelIndex and CameraChannelName tags are still prefixed with `primariCameraLabel-` for now, to preserve existing behavior. Composite multi-channel cameras MultiCamera, ArduinoCounter, and CameraPulser are updated to remove the physical camera mutations (`AddTag`/`RemoveTag`) and implement `GetChannelCameraPtr()`. The only intrinsic multi-channel camera, TwoPhoton, is updated to remove the tags that are no longer necessary. (Assisted by Claude Code; any errors are mine.) --- .../ArduinoCounter/ArduinoCounter.cpp | 27 ++- .../ArduinoCounter/ArduinoCounter.h | 1 + .../TeensyPulseGenerator/CameraPulser.cpp | 27 ++- .../TeensyPulseGenerator/CameraPulser.h | 1 + DeviceAdapters/TwoPhoton/TwoPhoton.cpp | 11 +- DeviceAdapters/Utilities/MultiCamera.cpp | 27 ++- DeviceAdapters/Utilities/Utilities.h | 1 + MMCore/CoreCallback.cpp | 140 +++++++++++-- MMCore/Devices/CameraInstance.cpp | 15 +- MMCore/Devices/CameraInstance.h | 4 +- MMCore/MMCore.cpp | 156 ++++++++++++-- MMCore/MMCore.h | 27 ++- MMCore/SequenceAcquisition.cpp | 148 ++++++++++++- MMCore/SequenceAcquisition.h | 104 +++++++++- MMCore/unittest/CircularBuffer-Tests.cpp | 75 +++---- MMCore/unittest/EventCallback-Tests.cpp | 20 +- MMCore/unittest/ImageMetadataTags-Tests.cpp | 73 +++---- .../MultiChannelSequenceAcquisition-Tests.cpp | 196 ++++++++++++------ MMCore/unittest/SequenceAcquisition-Tests.cpp | 11 +- MMCore/unittest/StubDevices.h | 26 +-- MMDevice/DeviceBase.h | 49 ++--- MMDevice/MMDevice.h | 50 ++--- 22 files changed, 835 insertions(+), 354 deletions(-) diff --git a/DeviceAdapters/ArduinoCounter/ArduinoCounter.cpp b/DeviceAdapters/ArduinoCounter/ArduinoCounter.cpp index 43e69a4de..cc5faf4a7 100644 --- a/DeviceAdapters/ArduinoCounter/ArduinoCounter.cpp +++ b/DeviceAdapters/ArduinoCounter/ArduinoCounter.cpp @@ -626,6 +626,18 @@ int ArduinoCounterCamera::Logical2Physical(int logical) return -1; } +MM::Camera* ArduinoCounterCamera::GetChannelCameraPtr(unsigned n) +{ + if (n >= nrCamerasInUse_) + return nullptr; + int ch = Logical2Physical(static_cast(n)); + if (ch < 0 || static_cast(ch) >= usedCameras_.size()) + return nullptr; + if (usedCameras_[ch] == g_Undefined) + return nullptr; + return (MM::Camera*)GetDevice(usedCameras_[ch].c_str()); +} + int ArduinoCounterCamera::OnPhysicalCamera(MM::PropertyBase* pProp, MM::ActionType eAct, long i) { @@ -636,13 +648,6 @@ int ArduinoCounterCamera::OnPhysicalCamera(MM::PropertyBase* pProp, MM::ActionTy else if (eAct == MM::AfterSet) { - MM::Camera* camera = (MM::Camera*)GetDevice(usedCameras_[i].c_str()); - if (camera != 0) - { - camera->RemoveTag(MM::g_Keyword_CameraChannelName); - camera->RemoveTag(MM::g_Keyword_CameraChannelIndex); - } - std::string cameraName; pProp->Get(cameraName); @@ -650,15 +655,9 @@ int ArduinoCounterCamera::OnPhysicalCamera(MM::PropertyBase* pProp, MM::ActionTy usedCameras_[i] = g_Undefined; } else { - camera = (MM::Camera*)GetDevice(cameraName.c_str()); + MM::Camera* camera = (MM::Camera*)GetDevice(cameraName.c_str()); if (camera != 0) { usedCameras_[i] = cameraName; - std::ostringstream os; - os << i; - char myName[MM::MaxStrLength]; - GetLabel(myName); - camera->AddTag(MM::g_Keyword_CameraChannelName, myName, usedCameras_[i].c_str()); - camera->AddTag(MM::g_Keyword_CameraChannelIndex, myName, os.str().c_str()); } else return ERR_INVALID_DEVICE_NAME; diff --git a/DeviceAdapters/ArduinoCounter/ArduinoCounter.h b/DeviceAdapters/ArduinoCounter/ArduinoCounter.h index 1b4664d1a..f41b03a13 100644 --- a/DeviceAdapters/ArduinoCounter/ArduinoCounter.h +++ b/DeviceAdapters/ArduinoCounter/ArduinoCounter.h @@ -107,6 +107,7 @@ class ArduinoCounterCamera : public CCameraBase unsigned GetNumberOfComponents() const; unsigned GetNumberOfChannels() const; int GetChannelName(unsigned channel, char* name); + MM::Camera* GetChannelCameraPtr(unsigned n); bool IsCapturing(); // action interface diff --git a/DeviceAdapters/TeensyPulseGenerator/CameraPulser.cpp b/DeviceAdapters/TeensyPulseGenerator/CameraPulser.cpp index 69698f423..c67991b6f 100644 --- a/DeviceAdapters/TeensyPulseGenerator/CameraPulser.cpp +++ b/DeviceAdapters/TeensyPulseGenerator/CameraPulser.cpp @@ -646,6 +646,18 @@ int CameraPulser::Logical2Physical(int logical) return -1; } +MM::Camera* CameraPulser::GetChannelCameraPtr(unsigned n) +{ + if (n >= nrCamerasInUse_) + return nullptr; + int ch = Logical2Physical(static_cast(n)); + if (ch < 0 || static_cast(ch) >= usedCameras_.size()) + return nullptr; + if (usedCameras_[ch] == g_Undefined) + return nullptr; + return (MM::Camera*)GetDevice(usedCameras_[ch].c_str()); +} + int CameraPulser::OnPhysicalCamera(MM::PropertyBase* pProp, MM::ActionType eAct, long i) { @@ -657,13 +669,6 @@ int CameraPulser::OnPhysicalCamera(MM::PropertyBase* pProp, MM::ActionType eAct, else if (eAct == MM::AfterSet) { - MM::Camera* camera = (MM::Camera*)GetDevice(usedCameras_[i].c_str()); - if (camera != 0) - { - camera->RemoveTag(MM::g_Keyword_CameraChannelName); - camera->RemoveTag(MM::g_Keyword_CameraChannelIndex); - } - std::string cameraName; pProp->Get(cameraName); @@ -671,15 +676,9 @@ int CameraPulser::OnPhysicalCamera(MM::PropertyBase* pProp, MM::ActionType eAct, usedCameras_[i] = g_Undefined; } else { - camera = (MM::Camera*)GetDevice(cameraName.c_str()); + MM::Camera* camera = (MM::Camera*)GetDevice(cameraName.c_str()); if (camera != 0) { usedCameras_[i] = cameraName; - std::ostringstream os; - os << i; - char myName[MM::MaxStrLength]; - GetLabel(myName); - camera->AddTag(MM::g_Keyword_CameraChannelName, myName, usedCameras_[i].c_str()); - camera->AddTag(MM::g_Keyword_CameraChannelIndex, myName, os.str().c_str()); } else return ERR_INVALID_DEVICE_NAME; diff --git a/DeviceAdapters/TeensyPulseGenerator/CameraPulser.h b/DeviceAdapters/TeensyPulseGenerator/CameraPulser.h index bfe5f4493..e924131e3 100644 --- a/DeviceAdapters/TeensyPulseGenerator/CameraPulser.h +++ b/DeviceAdapters/TeensyPulseGenerator/CameraPulser.h @@ -54,6 +54,7 @@ class CameraPulser : public CCameraBase unsigned GetNumberOfComponents() const; unsigned GetNumberOfChannels() const; int GetChannelName(unsigned channel, char* name); + MM::Camera* GetChannelCameraPtr(unsigned n); bool IsCapturing(); // action interface diff --git a/DeviceAdapters/TwoPhoton/TwoPhoton.cpp b/DeviceAdapters/TwoPhoton/TwoPhoton.cpp index a4ab5e113..75fa1b6a4 100644 --- a/DeviceAdapters/TwoPhoton/TwoPhoton.cpp +++ b/DeviceAdapters/TwoPhoton/TwoPhoton.cpp @@ -995,10 +995,6 @@ int BitFlowCamera::LiveThread::svc() break; } - char label[MM::MaxStrLength]; - cam_->GetLabel(label); - std::string labelStr = label; - MM::MMTime timestamp = cam_->GetCurrentMMTime(); MM::CameraImageMetadata md; std::string prefix = "TwoPhoton-"; @@ -1010,17 +1006,12 @@ int BitFlowCamera::LiveThread::svc() md.AddTag(prefix + MM::g_Keyword_Metadata_ImageNumber, CDeviceUtils::ConvertToString(imageCounter_)); - // insert all channels + // MMCore stamps CameraChannelName based on CameraChannelIndex. for (unsigned i=0; iGetNumberOfChannels(); i++) { char buf[MM::MaxStrLength]; snprintf(buf, MM::MaxStrLength, "%d", i); md.AddTag(MM::g_Keyword_CameraChannelIndex, buf); - md.AddTag(labelStr + "-" + MM::g_Keyword_CameraChannelIndex, buf); // compat - - cam_->GetChannelName(i, buf); - md.AddTag(MM::g_Keyword_CameraChannelName, buf); - md.AddTag(labelStr + "-" + MM::g_Keyword_CameraChannelName, buf); // compat ret = cam_->GetCoreCallback()->InsertImage(cam_, cam_->GetImageBuffer(i), cam_->GetImageWidth(), diff --git a/DeviceAdapters/Utilities/MultiCamera.cpp b/DeviceAdapters/Utilities/MultiCamera.cpp index db9d52410..4fa6bac8d 100644 --- a/DeviceAdapters/Utilities/MultiCamera.cpp +++ b/DeviceAdapters/Utilities/MultiCamera.cpp @@ -560,6 +560,18 @@ int MultiCamera::Logical2Physical(int logical) return -1; } +MM::Camera* MultiCamera::GetChannelCameraPtr(unsigned n) +{ + if (n >= nrCamerasInUse_) + return nullptr; + int ch = Logical2Physical(static_cast(n)); + if (ch < 0 || static_cast(ch) >= usedCameras_.size()) + return nullptr; + if (usedCameras_[ch] == g_Undefined) + return nullptr; + return (MM::Camera*)GetDevice(usedCameras_[ch].c_str()); +} + int MultiCamera::OnPhysicalCamera(MM::PropertyBase* pProp, MM::ActionType eAct, long i) { @@ -570,13 +582,6 @@ int MultiCamera::OnPhysicalCamera(MM::PropertyBase* pProp, MM::ActionType eAct, else if (eAct == MM::AfterSet) { - MM::Camera* camera = (MM::Camera*)GetDevice(usedCameras_[i].c_str()); - if (camera != 0) - { - camera->RemoveTag(MM::g_Keyword_CameraChannelName); - camera->RemoveTag(MM::g_Keyword_CameraChannelIndex); - } - std::string cameraName; pProp->Get(cameraName); @@ -584,15 +589,9 @@ int MultiCamera::OnPhysicalCamera(MM::PropertyBase* pProp, MM::ActionType eAct, usedCameras_[i] = g_Undefined; } else { - camera = (MM::Camera*)GetDevice(cameraName.c_str()); + MM::Camera* camera = (MM::Camera*)GetDevice(cameraName.c_str()); if (camera != 0) { usedCameras_[i] = cameraName; - std::ostringstream os; - os << i; - char myName[MM::MaxStrLength]; - GetLabel(myName); - camera->AddTag(MM::g_Keyword_CameraChannelName, myName, usedCameras_[i].c_str()); - camera->AddTag(MM::g_Keyword_CameraChannelIndex, myName, os.str().c_str()); } else return ERR_INVALID_DEVICE_NAME; diff --git a/DeviceAdapters/Utilities/Utilities.h b/DeviceAdapters/Utilities/Utilities.h index 525d49f0e..f3b29e584 100644 --- a/DeviceAdapters/Utilities/Utilities.h +++ b/DeviceAdapters/Utilities/Utilities.h @@ -173,6 +173,7 @@ class MultiCamera : public CCameraBase unsigned GetNumberOfComponents() const; unsigned GetNumberOfChannels() const; int GetChannelName(unsigned channel, char* name); + MM::Camera* GetChannelCameraPtr(unsigned n); bool IsCapturing(); // action interface diff --git a/MMCore/CoreCallback.cpp b/MMCore/CoreCallback.cpp index ba19d1062..daaaad5e3 100644 --- a/MMCore/CoreCallback.cpp +++ b/MMCore/CoreCallback.cpp @@ -29,6 +29,7 @@ #include "CoreCallback.h" #include "DeviceManager.h" #include "Notification.h" +#include "SequenceAcquisition.h" #include "SerializedMetadata.h" #include "SynchronizedConfiguration.h" @@ -219,7 +220,7 @@ CoreCallback::Sleep(const MM::Device*, double intervalMs) /** - * Append the metadata tags attached to device caller to md. + * Append the camera-label metadata tag to md. */ void CoreCallback::AddCameraMetadata(const MM::Device* caller, @@ -231,18 +232,6 @@ CoreCallback::AddCameraMetadata(const MM::Device* caller, std::string label = camera->GetLabel(); md.AddTag(MM::g_Keyword_Metadata_CameraLabel, label); - - std::string serializedMD; - try - { - serializedMD = camera->GetTags(); - } - catch (const CMMError&) - { - return; - } - - md.AppendSerialized(serializedMD.c_str()); } int CoreCallback::InsertImage(const MM::Device* caller, const unsigned char* buf, @@ -307,11 +296,68 @@ int CoreCallback::InsertImage(const MM::Device* caller, const unsigned char* buf unsigned width, unsigned height, unsigned bytesPerPixel, unsigned nComponents, const char* serializedMetadata) { + auto sa = core_->findAcquisitionByCaller(caller); + if (!sa) { + LOG_ERROR(core_->coreLogger_) << + "InsertImage() from device that is not a participant of any " + "active sequence acquisition; if this device is a physical " + "sub-camera of a multi-channel camera, the multi-channel camera " + "must override GetChannelCameraPtr()"; + return DEVICE_ERR; + } + const auto pi = sa->LookupParticipant(caller); + try { SerializedMetadata md = BuildSequenceImageMetadata( caller, width, height, bytesPerPixel, nComponents, serializedMetadata); + using Kind = SequenceAcquisition::ParticipantInfo::Kind; + const std::string prefix = sa->CameraLabel() + '-'; + const std::string prefixedIndexKey = + prefix + MM::g_Keyword_CameraChannelIndex; + const std::string prefixedNameKey = + prefix + MM::g_Keyword_CameraChannelName; + + if (pi.kind == Kind::CompositeChannel) { + md.AddTag(prefixedIndexKey, std::to_string(pi.index)); + md.AddTag(prefixedNameKey, sa->Channel(pi.index).channelName); + } else if (pi.kind == Kind::IntrinsicPrimary) { + auto idxTag = md.GetTag(prefixedIndexKey.c_str()); + if (!idxTag) + idxTag = md.GetTag(MM::g_Keyword_CameraChannelIndex); + + if (!idxTag) { + if (sa->NumChannels() != 1) { + LOG_ERROR(core_->coreLogger_) << + "Intrinsic multi-channel camera image missing required " + "CameraChannelIndex tag from " << sa->CameraLabel(); + return DEVICE_INCOMPATIBLE_IMAGE; + } + // Plain single-channel camera, no opt-in: no stamping. + } else { + long parsedIdx = -1; + try { + parsedIdx = std::stol(std::string(*idxTag)); + } catch (...) { + LOG_ERROR(core_->coreLogger_) << + "CameraChannelIndex not parseable in image from " + << sa->CameraLabel(); + return DEVICE_INCOMPATIBLE_IMAGE; + } + if (parsedIdx < 0 || + static_cast(parsedIdx) >= sa->NumChannels()) { + LOG_ERROR(core_->coreLogger_) << + "CameraChannelIndex out of range (" << parsedIdx + << ") in image from " << sa->CameraLabel(); + return DEVICE_INCOMPATIBLE_IMAGE; + } + md.AddTag(prefixedIndexKey, std::to_string(parsedIdx)); + md.AddTag(prefixedNameKey, + sa->Channel(static_cast(parsedIdx)).channelName); + } + } + MM::ImageProcessor* ip = GetImageProcessor(caller); if (ip != nullptr) { @@ -358,6 +404,21 @@ int CoreCallback::AcqFinished(const MM::Device* caller, int /*statusCode*/) return DEVICE_ERR; } + auto sa = core_->findAcquisitionByCaller(caller); + if (!sa) { + LOG_ERROR(core_->coreLogger_) << + "AcqFinished() from device '" << camera->GetLabel() << + "' that is not a participant of any active sequence acquisition" + "; if this device is a physical sub-camera of a multi-channel " + "camera, the multi-channel camera must override " + "GetChannelCameraPtr()"; + return DEVICE_ERR; + } + + const bool isLast = sa->RecordFinish(caller); + if (!isLast) + return DEVICE_OK; + std::shared_ptr currentCamera = core_->currentCameraDevice_.lock(); @@ -410,13 +471,48 @@ int CoreCallback::AcqFinished(const MM::Device* caller, int /*statusCode*/) } core_->postNotification( - notif::SequenceAcquisitionStopped{camera->GetLabel()}); + notif::SequenceAcquisitionStopped{sa->CameraLabel()}); + + if (sa->IsComplete()) + core_->eraseCompletedAcquisition(sa->CameraLabel()); return DEVICE_OK; } int CoreCallback::PrepareForAcq(const MM::Device* caller) { + auto sa = core_->findAcquisitionByCaller(caller); + if (!sa) { + char label[MM::MaxStrLength]; + caller->GetLabel(label); + LOG_ERROR(core_->coreLogger_) << + "PrepareForAcq() from device '" << label << + "' that is not a participant of any active sequence acquisition" + "; if this device is a physical sub-camera of a multi-channel " + "camera, the multi-channel camera must override " + "GetChannelCameraPtr()"; + return DEVICE_ERR; + } + + const auto disp = sa->BeginPrepare(caller); + using PD = SequenceAcquisition::PrepareDisposition; + switch (disp) { + case PD::NotParticipant: + // Should not happen — findAcquisitionByCaller already verified. + return DEVICE_ERR; + case PD::AlreadyPrepared: + return DEVICE_OK; + case PD::AlreadyOpened: + return DEVICE_OK; + case PD::OpenFailed: + return DEVICE_ERR; + case PD::WaitForOpener: + return sa->WaitForShutterOpened() ? DEVICE_OK : DEVICE_ERR; + case PD::FirstOpener: + break; + } + + bool openOk = true; if (core_->autoShutter_) { std::shared_ptr shutter = @@ -428,18 +524,22 @@ int CoreCallback::PrepareForAcq(const MM::Device* caller) DeviceModuleLockGuard g(shutter); sret = shutter->SetOpen(true); } - if (sret == DEVICE_OK) + if (sret == DEVICE_OK) { core_->postNotification(notif::ShutterOpenChanged{ shutter->GetLabel(), true}); - core_->waitForDevice(shutter); + core_->waitForDevice(shutter); + } else { + openOk = false; + } } } - char label[MM::MaxStrLength]; - caller->GetLabel(label); - core_->postNotification(notif::SequenceAcquisitionStarted{label}); + if (openOk) + core_->postNotification(notif::SequenceAcquisitionStarted{sa->CameraLabel()}); - return DEVICE_OK; + sa->FinishShutterOpen(openOk); + + return openOk ? DEVICE_OK : DEVICE_ERR; } /** diff --git a/MMCore/Devices/CameraInstance.cpp b/MMCore/Devices/CameraInstance.cpp index cb452ba37..e3c4cb9d5 100644 --- a/MMCore/Devices/CameraInstance.cpp +++ b/MMCore/Devices/CameraInstance.cpp @@ -122,20 +122,7 @@ int CameraInstance::StartSequenceAcquisition(long numImages, double interval_ms, int CameraInstance::StartSequenceAcquisition(double interval_ms) { RequireInitialized(__func__); return GetImpl()->StartSequenceAcquisition(interval_ms); } int CameraInstance::StopSequenceAcquisition() { RequireInitialized(__func__); return GetImpl()->StopSequenceAcquisition(); } bool CameraInstance::IsCapturing() { RequireInitialized(__func__); return GetImpl()->IsCapturing(); } - -std::string CameraInstance::GetTags() -{ - RequireInitialized(__func__); - // TODO Note the danger of limiting serialized metadata to MM::MaxStrLength - // (CCameraBase takes no precaution to limit string length; it is an - // interface bug). - DeviceStringBuffer serializedMetadataBuf(this, "GetTags"); - GetImpl()->GetTags(serializedMetadataBuf.GetBuffer()); - return serializedMetadataBuf.Get(); -} - -void CameraInstance::AddTag(const char* key, const char* deviceLabel, const char* value) { RequireInitialized(__func__); return GetImpl()->AddTag(key, deviceLabel, value); } -void CameraInstance::RemoveTag(const char* key) { RequireInitialized(__func__); return GetImpl()->RemoveTag(key); } +MM::Camera* CameraInstance::GetChannelCameraPtr(unsigned n) { RequireInitialized(__func__); return GetImpl()->GetChannelCameraPtr(n); } int CameraInstance::IsExposureSequenceable(bool& isSequenceable) const { RequireInitialized(__func__); return GetImpl()->IsExposureSequenceable(isSequenceable); } int CameraInstance::GetExposureSequenceMaxLength(long& nrEvents) const { RequireInitialized(__func__); return GetImpl()->GetExposureSequenceMaxLength(nrEvents); } int CameraInstance::StartExposureSequence() { RequireInitialized(__func__); return GetImpl()->StartExposureSequence(); } diff --git a/MMCore/Devices/CameraInstance.h b/MMCore/Devices/CameraInstance.h index 920fef413..ec9e7b459 100644 --- a/MMCore/Devices/CameraInstance.h +++ b/MMCore/Devices/CameraInstance.h @@ -72,9 +72,7 @@ class CameraInstance : public DeviceInstanceBase int StartSequenceAcquisition(double interval_ms); int StopSequenceAcquisition(); bool IsCapturing(); - std::string GetTags(); - void AddTag(const char* key, const char* deviceLabel, const char* value); - void RemoveTag(const char* key); + MM::Camera* GetChannelCameraPtr(unsigned n); int IsExposureSequenceable(bool& isSequenceable) const; int GetExposureSequenceMaxLength(long& nrEvents) const; int StartExposureSequence(); diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index 4a7c23a98..f0257f109 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -874,15 +874,89 @@ void CMMCore::removeAllDeviceRoles() { setSLMDevice(""); } -void CMMCore::stopAllSequenceAcquisitionsForUnload() +std::vector +CMMCore::buildSequenceChannelSnapshot( + std::shared_ptr camera) MMCORE_LEGACY_THROW(CMMError) +{ + const unsigned n = camera->GetNumberOfChannels(); + std::vector channels; + channels.reserve(n); + for (unsigned i = 0; i < n; ++i) { + mmi::SequenceAcquisition::ChannelInfo info; + info.channelName = camera->GetChannelName(i); + MM::Camera* physCam = camera->GetChannelCameraPtr(i); + info.physCamDevice = physCam; + if (physCam) { + std::shared_ptr physInstance; + try { + physInstance = std::dynamic_pointer_cast( + deviceManager_->GetDevice(physCam)); + } catch (const CMMError&) { + throw CMMError( + "Composite camera '" + camera->GetLabel() + "' channel " + + std::to_string(i) + + " references unregistered physical camera", + MMERR_DEVICE_GENERIC); + } + if (!physInstance) { + throw CMMError( + "Composite camera '" + camera->GetLabel() + "' channel " + + std::to_string(i) + + " physical pointer is not a Camera", + MMERR_DEVICE_GENERIC); + } + { + mmi::DeviceModuleLockGuard pg(physInstance); + if (physInstance->GetNumberOfChannels() > 1) { + throw CMMError( + "Composite camera '" + camera->GetLabel() + "' channel " + + std::to_string(i) + + " is itself a multi-channel camera ('" + + physInstance->GetLabel() + + "'); nested multi-channel cameras are not supported", + MMERR_DEVICE_GENERIC); + } + } + info.physCamLabel = physInstance->GetLabel(); + } + channels.push_back(std::move(info)); + } + return channels; +} + +std::shared_ptr +CMMCore::findAcquisitionByCaller(const MM::Device* caller) { - for (auto& kv : acquisitions_) { + if (!caller) + return nullptr; + std::lock_guard g(acquisitionsMutex_); + for (const auto& kv : acquisitions_) { + if (kv.second->HasParticipant(caller)) + return kv.second; + } + return nullptr; +} + +void CMMCore::eraseCompletedAcquisition(const std::string& cameraLabel) +{ + std::lock_guard g(acquisitionsMutex_); + acquisitions_.erase(cameraLabel); +} + +void CMMCore::stopAndClearAllSequenceAcquisitions() +{ + std::map> toStop; + { + std::lock_guard g(acquisitionsMutex_); + toStop.swap(acquisitions_); + } + for (auto& kv : toStop) { auto& cam = kv.second->Camera(); mmi::DeviceModuleLockGuard guard(cam); if (cam->IsCapturing()) cam->StopSequenceAcquisition(); } - acquisitions_.clear(); } @@ -900,7 +974,7 @@ void CMMCore::unloadDevice(const char* label///< the name of the device to unloa std::shared_ptr pDevice = deviceManager_->GetDevice(label); - stopAllSequenceAcquisitionsForUnload(); + stopAndClearAllSequenceAcquisitions(); try { removeDeviceRole(pDevice); @@ -924,7 +998,7 @@ void CMMCore::unloadDevice(const char* label///< the name of the device to unloa */ void CMMCore::unloadAllDevices() MMCORE_LEGACY_THROW(CMMError) { - stopAllSequenceAcquisitionsForUnload(); + stopAndClearAllSequenceAcquisitions(); try { removeAllDeviceRoles(); @@ -3077,15 +3151,23 @@ void CMMCore::startSequenceAcquisition(long numImages, double unused, bool stopO mmi::DeviceModuleLockGuard guard(camera); LOG_DEBUG(coreLogger_) << "Will start sequence acquisition from default camera"; - acquisitions_[camera->GetLabel()] = - mmcore::internal::SequenceAcquisition::Create(camera); + auto channels = buildSequenceChannelSnapshot(camera); + { + std::lock_guard aqg(acquisitionsMutex_); + acquisitions_[camera->GetLabel()] = + mmcore::internal::SequenceAcquisition::Create( + camera, std::move(channels)); + } // Forward `unused` to the device rather than substituting 0.0: // a small number of camera adapters (Andor) did implement this // parameter, and passing 0.0 unconditionally could regress them. // The MM::Camera contract now says new adapters must ignore it. int nRet = camera->StartSequenceAcquisition(numImages, unused, stopOnOverflow); if (nRet != DEVICE_OK) { - acquisitions_.erase(camera->GetLabel()); + { + std::lock_guard aqg(acquisitionsMutex_); + acquisitions_.erase(camera->GetLabel()); + } throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); } } @@ -3142,15 +3224,23 @@ void CMMCore::startSequenceAcquisition(const char* label, long numImages, double cbuf_->SetOverwriteData(!stopOnOverflow); LOG_DEBUG(coreLogger_) << "Will start sequence acquisition from camera " << label; - acquisitions_[pCam->GetLabel()] = - mmcore::internal::SequenceAcquisition::Create(pCam); + auto channels = buildSequenceChannelSnapshot(pCam); + { + std::lock_guard aqg(acquisitionsMutex_); + acquisitions_[pCam->GetLabel()] = + mmcore::internal::SequenceAcquisition::Create( + pCam, std::move(channels)); + } // Forward `unused` to the device rather than substituting 0.0: a small // number of camera adapters (Andor) did implement this parameter, and // passing 0.0 unconditionally could regress them. The MM::Camera // contract now says new adapters must ignore it. int nRet = pCam->StartSequenceAcquisition(numImages, unused, stopOnOverflow); if (nRet != DEVICE_OK) { - acquisitions_.erase(pCam->GetLabel()); + { + std::lock_guard aqg(acquisitionsMutex_); + acquisitions_.erase(pCam->GetLabel()); + } throw CMMError(getDeviceErrorText(nRet, pCam).c_str(), MMERR_DEVICE_GENERIC); } @@ -3224,9 +3314,18 @@ void CMMCore::stopSequenceAcquisition(const char* label) MMCORE_LEGACY_THROW(CMM throw CMMError(getDeviceErrorText(nRet, pCam).c_str(), MMERR_DEVICE_GENERIC); } { - auto it = acquisitions_.find(pCam->GetLabel()); - if (it != acquisitions_.end()) - it->second->MarkStopRequested(); + std::shared_ptr sa; + { + std::lock_guard aqg(acquisitionsMutex_); + auto it = acquisitions_.find(pCam->GetLabel()); + if (it != acquisitions_.end()) + sa = it->second; + } + if (sa) { + const bool causedComplete = sa->MarkStopRequested(); + if (causedComplete) + eraseCompletedAcquisition(sa->CameraLabel()); + } } LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from camera " << label; @@ -3265,15 +3364,23 @@ void CMMCore::startContinuousSequenceAcquisition(double unused) MMCORE_LEGACY_TH callback_->ResetImageInsertionState(); cbuf_->SetOverwriteData(true); LOG_DEBUG(coreLogger_) << "Will start continuous sequence acquisition from current camera"; - acquisitions_[camera->GetLabel()] = - mmcore::internal::SequenceAcquisition::Create(camera); + auto channels = buildSequenceChannelSnapshot(camera); + { + std::lock_guard aqg(acquisitionsMutex_); + acquisitions_[camera->GetLabel()] = + mmcore::internal::SequenceAcquisition::Create( + camera, std::move(channels)); + } // Forward `unused` to the device rather than substituting 0.0: a small // number of camera adapters (Andor) did implement this parameter, and // passing 0.0 unconditionally could regress them. The MM::Camera // contract now says new adapters must ignore it. int nRet = camera->StartSequenceAcquisition(unused); if (nRet != DEVICE_OK) { - acquisitions_.erase(camera->GetLabel()); + { + std::lock_guard aqg(acquisitionsMutex_); + acquisitions_.erase(camera->GetLabel()); + } throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); } } @@ -3302,9 +3409,18 @@ void CMMCore::stopSequenceAcquisition() MMCORE_LEGACY_THROW(CMMError) logError(getDeviceName(camera).c_str(), getDeviceErrorText(nRet, camera).c_str()); throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); } - auto it = acquisitions_.find(camera->GetLabel()); - if (it != acquisitions_.end()) - it->second->MarkStopRequested(); + std::shared_ptr sa; + { + std::lock_guard aqg(acquisitionsMutex_); + auto it = acquisitions_.find(camera->GetLabel()); + if (it != acquisitions_.end()) + sa = it->second; + } + if (sa) { + const bool causedComplete = sa->MarkStopRequested(); + if (causedComplete) + eraseCompletedAcquisition(sa->CameraLabel()); + } } else { diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index 2d3ff17cf..ec8ff9e87 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -53,6 +53,7 @@ #include "Logging/Logger.h" #include "MockDeviceAdapter.h" #include "Notification.h" +#include "SequenceAcquisition.h" #include "MMDevice.h" #include "MMDeviceConstants.h" @@ -736,11 +737,14 @@ class CMMCore // Active sequence acquisitions keyed by logical-camera label. At most // one entry per camera, but multiple cameras may have concurrent // acquisitions because the labelled start/stop API is per-camera (sort of). - // Mutated only from the user thread. + // Both the user thread and device-callback threads access this map; all + // access is protected by acquisitionsMutex_. std::map> acquisitions_; + mutable std::mutex acquisitionsMutex_; + std::shared_ptr pluginManager_; std::shared_ptr deviceManager_; std::map errorText_; @@ -789,7 +793,26 @@ class CMMCore // Stops every acquisition (not just one for the unloaded camera) // because Core (for now) does not know which logical cameras a given // physical device participates in (e.g. via Multi Camera). - void stopAllSequenceAcquisitionsForUnload(); + void stopAndClearAllSequenceAcquisitions(); + + // Build the per-channel snapshot for `camera`, throwing CMMError if any + // composite channel references a multi-channel phys cam (nested + // multi-channel cameras are unsupported). Caller must hold the camera's + // DeviceModuleLockGuard. + std::vector< + mmcore::internal::SequenceAcquisition::ChannelInfo> + buildSequenceChannelSnapshot( + std::shared_ptr camera) + MMCORE_LEGACY_THROW(CMMError); + + // Look up the SA whose participant set contains `caller`. Returns nullptr + // if none. Takes acquisitionsMutex_. + std::shared_ptr + findAcquisitionByCaller(const MM::Device* caller); + + // Erase a completed SA from `acquisitions_` (if present, by label). + // Idempotent. Takes acquisitionsMutex_. + void eraseCompletedAcquisition(const std::string& cameraLabel); void setCameraInternal(const std::string& label); void setShutterInternal(const std::string& label); diff --git a/MMCore/SequenceAcquisition.cpp b/MMCore/SequenceAcquisition.cpp index 90747b230..6593b88c4 100644 --- a/MMCore/SequenceAcquisition.cpp +++ b/MMCore/SequenceAcquisition.cpp @@ -24,21 +24,161 @@ namespace mmcore { namespace internal { +namespace { + +bool ComputeHasIntrinsic( + const std::vector& channels) +{ + for (const auto& ch : channels) + if (ch.physCamDevice == nullptr) + return true; + return false; +} + +std::set ComputeExpectedParticipants( + const std::shared_ptr& camera, + const std::vector& channels, + bool hasIntrinsic) +{ + std::set result; + for (const auto& ch : channels) + if (ch.physCamDevice) + result.insert(ch.physCamDevice); + if (hasIntrinsic) + result.insert(camera->GetRawPtr()); + return result; +} + +} // namespace + std::shared_ptr -SequenceAcquisition::Create(std::shared_ptr camera) +SequenceAcquisition::Create(std::shared_ptr camera, + std::vector channels) { return std::shared_ptr( - new SequenceAcquisition(std::move(camera))); + new SequenceAcquisition(std::move(camera), std::move(channels))); } SequenceAcquisition::SequenceAcquisition( - std::shared_ptr camera) : + std::shared_ptr camera, + std::vector channels) : cameraLabel_(camera->GetLabel()), - camera_(std::move(camera)) + camera_(std::move(camera)), + channels_(std::move(channels)), + hasIntrinsic_(ComputeHasIntrinsic(channels_)), + expectedParticipants_( + ComputeExpectedParticipants(camera_, channels_, hasIntrinsic_)) { } SequenceAcquisition::~SequenceAcquisition() = default; +SequenceAcquisition::ParticipantInfo +SequenceAcquisition::LookupParticipant(const MM::Device* caller) const noexcept +{ + ParticipantInfo info; + if (caller == nullptr) + return info; + for (unsigned i = 0; i < channels_.size(); ++i) { + if (channels_[i].physCamDevice == caller) { + info.kind = ParticipantInfo::Kind::CompositeChannel; + info.index = i; + return info; + } + } + if (hasIntrinsic_ && caller == camera_->GetRawPtr()) { + info.kind = ParticipantInfo::Kind::IntrinsicPrimary; + return info; + } + return info; +} + +bool SequenceAcquisition::HasParticipant( + const MM::Device* caller) const noexcept { + return caller && expectedParticipants_.count(caller); +} + +bool SequenceAcquisition::WasStopRequested() const noexcept +{ + std::lock_guard g(mu_); + return stopRequested_; +} + +bool SequenceAcquisition::MarkStopRequested() noexcept +{ + std::lock_guard g(mu_); + if (stopRequested_) + return false; + stopRequested_ = true; + return finishedParticipants_.size() == expectedParticipants_.size(); +} + +SequenceAcquisition::PrepareDisposition +SequenceAcquisition::BeginPrepare(const MM::Device* caller) +{ + if (caller == nullptr) + return PrepareDisposition::NotParticipant; + if (expectedParticipants_.find(caller) == expectedParticipants_.end()) + return PrepareDisposition::NotParticipant; + + std::lock_guard g(mu_); + const bool first = readyParticipants_.insert(caller).second; + if (!first) + return PrepareDisposition::AlreadyPrepared; + + switch (shutterState_) { + case ShutterState::NotOpened: + shutterState_ = ShutterState::Opening; + return PrepareDisposition::FirstOpener; + case ShutterState::Opening: + return PrepareDisposition::WaitForOpener; + case ShutterState::Opened: + return PrepareDisposition::AlreadyOpened; + case ShutterState::OpenFailed: + return PrepareDisposition::OpenFailed; + } + return PrepareDisposition::NotParticipant; +} + +void SequenceAcquisition::FinishShutterOpen(bool success) +{ + { + std::lock_guard g(mu_); + shutterState_ = success ? ShutterState::Opened : ShutterState::OpenFailed; + } + shutterOpenedCv_.notify_all(); +} + +bool SequenceAcquisition::WaitForShutterOpened() +{ + std::unique_lock g(mu_); + shutterOpenedCv_.wait(g, [this] { + return shutterState_ == ShutterState::Opened || + shutterState_ == ShutterState::OpenFailed; + }); + return shutterState_ == ShutterState::Opened; +} + +bool SequenceAcquisition::RecordFinish(const MM::Device* caller) +{ + if (caller == nullptr) + return false; + if (expectedParticipants_.find(caller) == expectedParticipants_.end()) + return false; + + std::lock_guard g(mu_); + const bool first = finishedParticipants_.insert(caller).second; + if (!first) + return false; + return finishedParticipants_.size() == expectedParticipants_.size(); +} + +bool SequenceAcquisition::IsComplete() const noexcept +{ + std::lock_guard g(mu_); + return stopRequested_ && + finishedParticipants_.size() == expectedParticipants_.size(); +} + } // namespace internal } // namespace mmcore diff --git a/MMCore/SequenceAcquisition.h b/MMCore/SequenceAcquisition.h index 0bee7f178..3986a16a1 100644 --- a/MMCore/SequenceAcquisition.h +++ b/MMCore/SequenceAcquisition.h @@ -17,9 +17,14 @@ #pragma once -#include +#include "MMDevice.h" + +#include #include +#include +#include #include +#include namespace mmcore { namespace internal { @@ -28,10 +33,42 @@ class CameraInstance; class SequenceAcquisition { public: + struct ChannelInfo { + // null for intrinsic; non-null = phys cam pointer used for caller- + // identity matching in callbacks. + MM::Camera* physCamDevice = nullptr; + // Snapshot of primary->GetChannelName(idx) at start time. + std::string channelName; + // Label of the phys cam (for composite channels). Empty for intrinsic. + std::string physCamLabel; + }; + + struct ParticipantInfo { + enum class Kind { + NotParticipant, // caller is not in the SA's participant set + CompositeChannel, // caller is a phys cam at `index` + IntrinsicPrimary, // caller is the primary, which has at least one + // intrinsic channel (or is a degenerate single- + // channel non-composite) + }; + Kind kind = Kind::NotParticipant; + unsigned index = 0; // valid only when kind == CompositeChannel + }; + + enum class PrepareDisposition { + FirstOpener, // caller must open shutter, then call FinishShutterOpen + WaitForOpener, // another caller is opening; call WaitForShutterOpened + AlreadyOpened, // shutter already open; return immediately + OpenFailed, // a previous opener failed; caller should also fail + NotParticipant, // caller isn't an expected participant + AlreadyPrepared,// caller already called PrepareForAcq before + }; + static std::shared_ptr Create( - std::shared_ptr camera); - ~SequenceAcquisition(); + std::shared_ptr camera, + std::vector channels); + ~SequenceAcquisition(); SequenceAcquisition(const SequenceAcquisition&) = delete; SequenceAcquisition& operator=(const SequenceAcquisition&) = delete; @@ -40,19 +77,68 @@ class SequenceAcquisition { return camera_; } - bool WasStopRequested() const noexcept { - return stopRequested_.load(std::memory_order_acquire); + // Lookup by caller MM::Device* (== MM::Camera*) → participant info. + // Immutable after construction. + ParticipantInfo LookupParticipant(const MM::Device* caller) const noexcept; + bool HasParticipant(const MM::Device* caller) const noexcept; + + bool HasIntrinsicChannel() const noexcept { return hasIntrinsic_; } + const std::vector& Channels() const noexcept { + return channels_; } - void MarkStopRequested() noexcept { - stopRequested_.store(true, std::memory_order_release); + unsigned NumChannels() const noexcept { + return static_cast(channels_.size()); } + const ChannelInfo& Channel(unsigned n) const { return channels_.at(n); } + + // Mutable state (mutex-protected): + bool WasStopRequested() const noexcept; + + // Mark stop requested. Returns true iff this call caused a transition to + // "complete" (i.e., stopRequested && all participants have finished). + bool MarkStopRequested() noexcept; + + // Returns disposition; see enum. On FirstOpener, caller must invoke + // FinishShutterOpen exactly once after attempting to open the shutter + // (success or failure). On WaitForOpener, caller must invoke + // WaitForShutterOpened to block until the opener completes. + PrepareDisposition BeginPrepare(const MM::Device* caller); + + // Called by the FirstOpener exactly once, regardless of success. + void FinishShutterOpen(bool success); + + // Blocks until shutter state is terminal (Opened or OpenFailed). Returns + // true if Opened, false if OpenFailed. + bool WaitForShutterOpened(); + + // Records that `caller` finished the acquisition. Returns true iff this is + // the boundary call (last expected participant to finish), in which case + // the caller is responsible for the auto-shutter close + notification. + // `caller` not in expectedParticipants_ → returns false. + // Repeat call from same caller → returns false. + bool RecordFinish(const MM::Device* caller); + + // True iff stopRequested AND all expected participants have called + // RecordFinish. + bool IsComplete() const noexcept; private: - explicit SequenceAcquisition(std::shared_ptr camera); + SequenceAcquisition(std::shared_ptr camera, + std::vector channels); const std::string cameraLabel_; const std::shared_ptr camera_; - std::atomic stopRequested_{false}; + const std::vector channels_; + const bool hasIntrinsic_; + const std::set expectedParticipants_; + + mutable std::mutex mu_; + std::condition_variable shutterOpenedCv_; + enum class ShutterState { NotOpened, Opening, Opened, OpenFailed }; + ShutterState shutterState_ = ShutterState::NotOpened; + bool stopRequested_ = false; + std::set readyParticipants_; + std::set finishedParticipants_; }; } // namespace internal diff --git a/MMCore/unittest/CircularBuffer-Tests.cpp b/MMCore/unittest/CircularBuffer-Tests.cpp index 8044bfb0a..b7c1fb056 100644 --- a/MMCore/unittest/CircularBuffer-Tests.cpp +++ b/MMCore/unittest/CircularBuffer-Tests.cpp @@ -39,34 +39,21 @@ TEST_CASE("Buffer is empty after init", "[CircularBuffer]") { CHECK(c.isBufferOverflowed() == false); } -TEST_CASE("initializeCircularBuffer clears existing images", +TEST_CASE("Starting a fresh sequence acquisition clears existing images", "[CircularBuffer]") { StubCamera cam; MockAdapterWithDevices adapter{{"cam", &cam}}; CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); REQUIRE(cam.InsertTestImage() == DEVICE_OK); - c.initializeCircularBuffer(); + c.stopSequenceAcquisition(); + c.startSequenceAcquisition(1'000'000, 0.0, true); CHECK(c.getRemainingImageCount() == 0); CHECK(c.isBufferOverflowed() == false); -} - -TEST_CASE("initializeCircularBuffer clears images even with same dimensions", - "[CircularBuffer]") { - StubCamera cam; - MockAdapterWithDevices adapter{{"cam", &cam}}; - CMMCore c; - adapter.LoadIntoCore(c); - c.setCameraDevice("cam"); - c.initializeCircularBuffer(); - REQUIRE(cam.InsertTestImage() == DEVICE_OK); - REQUIRE(cam.InsertTestImage() == DEVICE_OK); - REQUIRE(c.getRemainingImageCount() == 2); - c.initializeCircularBuffer(); - CHECK(c.getRemainingImageCount() == 0); + c.stopSequenceAcquisition(); } // Insert @@ -78,9 +65,10 @@ TEST_CASE("getRemainingImageCount is 1 after one insert", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); CHECK(cam.InsertTestImage() == DEVICE_OK); CHECK(c.getRemainingImageCount() == 1); + c.stopSequenceAcquisition(); } TEST_CASE("Insert with mismatched width returns DEVICE_INCOMPATIBLE_IMAGE", @@ -90,10 +78,11 @@ TEST_CASE("Insert with mismatched width returns DEVICE_INCOMPATIBLE_IMAGE", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); cam.width = 256; CHECK(cam.InsertTestImage() == DEVICE_INCOMPATIBLE_IMAGE); CHECK(c.getRemainingImageCount() == 0); + c.stopSequenceAcquisition(); } TEST_CASE("Insert with mismatched height returns DEVICE_INCOMPATIBLE_IMAGE", @@ -103,10 +92,11 @@ TEST_CASE("Insert with mismatched height returns DEVICE_INCOMPATIBLE_IMAGE", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); cam.height = 256; CHECK(cam.InsertTestImage() == DEVICE_INCOMPATIBLE_IMAGE); CHECK(c.getRemainingImageCount() == 0); + c.stopSequenceAcquisition(); } TEST_CASE( @@ -117,10 +107,11 @@ TEST_CASE( CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); cam.bytesPerPixel = 2; CHECK(cam.InsertTestImage() == DEVICE_INCOMPATIBLE_IMAGE); CHECK(c.getRemainingImageCount() == 0); + c.stopSequenceAcquisition(); } TEST_CASE("Insert with mismatched nComponents succeeds", @@ -130,10 +121,11 @@ TEST_CASE("Insert with mismatched nComponents succeeds", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); cam.nComponents = 4; CHECK(cam.InsertTestImage() == DEVICE_OK); CHECK(c.getRemainingImageCount() == 1); + c.stopSequenceAcquisition(); } // getLastImage @@ -144,9 +136,10 @@ TEST_CASE("getLastImage returns non-null after insert", "[CircularBuffer]") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); CHECK(c.getLastImage() != nullptr); + c.stopSequenceAcquisition(); } TEST_CASE("getLastImage on empty buffer throws", "[CircularBuffer]") { @@ -166,7 +159,7 @@ TEST_CASE("getLastImage returns most recently inserted image", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); const std::size_t imgSize = static_cast(cam.width) * cam.height * cam.bytesPerPixel; @@ -178,6 +171,7 @@ TEST_CASE("getLastImage returns most recently inserted image", auto* img = static_cast(c.getLastImage()); REQUIRE(img != nullptr); CHECK(img[0] == 30); + c.stopSequenceAcquisition(); } // popNextImage @@ -189,10 +183,11 @@ TEST_CASE("popNextImage returns non-null and decrements count", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); CHECK(c.popNextImage() != nullptr); CHECK(c.getRemainingImageCount() == 0); + c.stopSequenceAcquisition(); } TEST_CASE("popNextImage on empty buffer throws", "[CircularBuffer]") { @@ -212,7 +207,7 @@ TEST_CASE("popNextImage returns images in insertion order", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); const std::size_t imgSize = static_cast(cam.width) * cam.height * cam.bytesPerPixel; @@ -226,6 +221,7 @@ TEST_CASE("popNextImage returns images in insertion order", REQUIRE(img != nullptr); CHECK(img[0] == expected); } + c.stopSequenceAcquisition(); } TEST_CASE("FIFO ordering is maintained across interleaved pops and inserts", @@ -235,7 +231,7 @@ TEST_CASE("FIFO ordering is maintained across interleaved pops and inserts", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); const std::size_t imgSize = static_cast(cam.width) * cam.height * cam.bytesPerPixel; @@ -257,6 +253,7 @@ TEST_CASE("FIFO ordering is maintained across interleaved pops and inserts", REQUIRE(img != nullptr); CHECK(img[0] == expected); } + c.stopSequenceAcquisition(); } // getNBeforeLastImageMD @@ -268,7 +265,7 @@ TEST_CASE("getNBeforeLastImageMD returns images by reverse offset", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); for (int i = 0; i < 3; ++i) REQUIRE(cam.InsertTestImage() == DEVICE_OK); @@ -281,6 +278,7 @@ TEST_CASE("getNBeforeLastImageMD returns images by reverse offset", c.getNBeforeLastImageMD(2, md); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_ImageNumber).GetValue() == "0"); + c.stopSequenceAcquisition(); } TEST_CASE("getNBeforeLastImageMD throws when offset exceeds available images", @@ -290,7 +288,7 @@ TEST_CASE("getNBeforeLastImageMD throws when offset exceeds available images", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); REQUIRE(cam.InsertTestImage() == DEVICE_OK); @@ -298,6 +296,7 @@ TEST_CASE("getNBeforeLastImageMD throws when offset exceeds available images", CHECK_NOTHROW(c.getNBeforeLastImageMD(0, md)); CHECK_NOTHROW(c.getNBeforeLastImageMD(1, md)); CHECK_THROWS_AS(c.getNBeforeLastImageMD(2, md), CMMError); + c.stopSequenceAcquisition(); } TEST_CASE("getNBeforeLastImageMD on empty buffer throws", @@ -323,7 +322,7 @@ TEST_CASE("Free plus remaining equals total after each insert", adapter.LoadIntoCore(c); c.setCameraDevice("cam"); c.setCircularBufferMemoryFootprint(1); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); long total = c.getBufferTotalCapacity(); REQUIRE(total == 4); @@ -332,6 +331,7 @@ TEST_CASE("Free plus remaining equals total after each insert", REQUIRE(cam.InsertTestImage() == DEVICE_OK); CHECK(c.getBufferFreeCapacity() + c.getRemainingImageCount() == total); } + c.stopSequenceAcquisition(); } TEST_CASE("setCircularBufferMemoryFootprint round-trips", "[CircularBuffer]") { @@ -368,7 +368,7 @@ TEST_CASE("Overflow with overwrite disabled", "[CircularBuffer]") { adapter.LoadIntoCore(c); c.setCameraDevice("cam"); c.setCircularBufferMemoryFootprint(1); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); long total = c.getBufferTotalCapacity(); REQUIRE(total == 4); @@ -380,6 +380,7 @@ TEST_CASE("Overflow with overwrite disabled", "[CircularBuffer]") { CHECK(cam.InsertTestImage() == DEVICE_BUFFER_OVERFLOW); CHECK(c.isBufferOverflowed() == true); CHECK(c.getRemainingImageCount() == total); + c.stopSequenceAcquisition(); } TEST_CASE("Overflow with overwrite enabled", "[CircularBuffer]") { @@ -402,6 +403,7 @@ TEST_CASE("Overflow with overwrite enabled", "[CircularBuffer]") { CHECK(cam.InsertTestImage() == DEVICE_OK); CHECK(c.isBufferOverflowed() == false); CHECK(c.getRemainingImageCount() == 1); + c.stopSequenceAcquisition(); } // Clear @@ -412,11 +414,12 @@ TEST_CASE("clearCircularBuffer resets remaining count", "[CircularBuffer]") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); REQUIRE(cam.InsertTestImage() == DEVICE_OK); c.clearCircularBuffer(); CHECK(c.getRemainingImageCount() == 0); + c.stopSequenceAcquisition(); } TEST_CASE("Overflow is sticky until buffer is cleared", "[CircularBuffer]") { @@ -426,7 +429,7 @@ TEST_CASE("Overflow is sticky until buffer is cleared", "[CircularBuffer]") { adapter.LoadIntoCore(c); c.setCameraDevice("cam"); c.setCircularBufferMemoryFootprint(1); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); long total = c.getBufferTotalCapacity(); REQUIRE(total == 4); @@ -440,6 +443,7 @@ TEST_CASE("Overflow is sticky until buffer is cleared", "[CircularBuffer]") { c.clearCircularBuffer(); CHECK(cam.InsertTestImage() == DEVICE_OK); + c.stopSequenceAcquisition(); } TEST_CASE("clearCircularBuffer resets overflow flag", "[CircularBuffer]") { @@ -449,7 +453,7 @@ TEST_CASE("clearCircularBuffer resets overflow flag", "[CircularBuffer]") { adapter.LoadIntoCore(c); c.setCameraDevice("cam"); c.setCircularBufferMemoryFootprint(1); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(1'000'000, 0.0, true); long total = c.getBufferTotalCapacity(); for (long i = 0; i < total; ++i) @@ -459,4 +463,5 @@ TEST_CASE("clearCircularBuffer resets overflow flag", "[CircularBuffer]") { c.clearCircularBuffer(); CHECK(c.isBufferOverflowed() == false); + c.stopSequenceAcquisition(); } diff --git a/MMCore/unittest/EventCallback-Tests.cpp b/MMCore/unittest/EventCallback-Tests.cpp index e86263d5f..b68a2306a 100644 --- a/MMCore/unittest/EventCallback-Tests.cpp +++ b/MMCore/unittest/EventCallback-Tests.cpp @@ -363,12 +363,13 @@ TEST_CASE("onSequenceAcquisitionStarted from device", c.setCameraDevice("cam"); c.registerCallback(&cb); - cam.PrepareForAcq(); + c.startSequenceAcquisition(10, 0.0, true); REQUIRE(cb.waitFor(CBType::SequenceAcquisitionStarted)); auto recs = cb.records(CBType::SequenceAcquisitionStarted); REQUIRE(recs.size() >= 1); CHECK(recs[0].s1 == "cam"); + c.stopSequenceAcquisition(); } TEST_CASE("onSequenceAcquisitionStopped from device", @@ -381,7 +382,8 @@ TEST_CASE("onSequenceAcquisitionStopped from device", c.setCameraDevice("cam"); c.registerCallback(&cb); - cam.AcqFinished(); + c.startSequenceAcquisition(10, 0.0, true); + c.stopSequenceAcquisition(); REQUIRE(cb.waitFor(CBType::SequenceAcquisitionStopped)); auto recs = cb.records(CBType::SequenceAcquisitionStopped); @@ -534,13 +536,14 @@ TEST_CASE("onShutterOpenChanged from AcqFinished", "[EventCallback]") { c.setAutoShutter(true); c.registerCallback(&cb); - cam.AcqFinished(); + c.startSequenceAcquisition(10, 0.0, true); + c.stopSequenceAcquisition(); - REQUIRE(cb.waitFor(CBType::ShutterOpenChanged)); + REQUIRE(cb.waitForCount(CBType::ShutterOpenChanged, 2)); auto recs = cb.records(CBType::ShutterOpenChanged); - REQUIRE(recs.size() >= 1); - CHECK(recs[0].s1 == "shutter"); - CHECK(recs[0].b1 == false); + REQUIRE(recs.size() >= 2); + CHECK(recs[1].s1 == "shutter"); + CHECK(recs[1].b1 == false); } TEST_CASE("onShutterOpenChanged from PrepareForAcq", "[EventCallback]") { @@ -555,13 +558,14 @@ TEST_CASE("onShutterOpenChanged from PrepareForAcq", "[EventCallback]") { c.setAutoShutter(true); c.registerCallback(&cb); - cam.PrepareForAcq(); + c.startSequenceAcquisition(10, 0.0, true); REQUIRE(cb.waitFor(CBType::ShutterOpenChanged)); auto recs = cb.records(CBType::ShutterOpenChanged); REQUIRE(recs.size() >= 1); CHECK(recs[0].s1 == "shutter"); CHECK(recs[0].b1 == true); + c.stopSequenceAcquisition(); } TEST_CASE("onImageSnapped from snapImage", "[EventCallback]") { diff --git a/MMCore/unittest/ImageMetadataTags-Tests.cpp b/MMCore/unittest/ImageMetadataTags-Tests.cpp index c27bb628e..adb921f94 100644 --- a/MMCore/unittest/ImageMetadataTags-Tests.cpp +++ b/MMCore/unittest/ImageMetadataTags-Tests.cpp @@ -15,12 +15,13 @@ TEST_CASE("All core metadata fields present for GRAY8 image") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); cam.InsertTestImage(); Metadata md; c.getLastImageMD(md); + c.stopSequenceAcquisition(); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_CameraLabel).GetValue() == "cam"); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_ImageNumber).GetValue() == @@ -84,7 +85,7 @@ TEST_CASE("PixelType metadata for all pixel formats") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); cam.InsertTestImage(); @@ -92,6 +93,7 @@ TEST_CASE("PixelType metadata for all pixel formats") { c.getLastImageMD(md); CHECK(md.GetSingleTag(MM::g_Keyword_PixelType).GetValue() == expectedPixelType); + c.stopSequenceAcquisition(); } TEST_CASE("Camera label matches device label") { @@ -100,13 +102,14 @@ TEST_CASE("Camera label matches device label") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("myCam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); cam.InsertTestImage(); Metadata md; c.getLastImageMD(md); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_CameraLabel).GetValue() == "myCam"); + c.stopSequenceAcquisition(); } TEST_CASE("Width and Height reflect camera dimensions") { @@ -117,7 +120,7 @@ TEST_CASE("Width and Height reflect camera dimensions") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); cam.InsertTestImage(); @@ -125,6 +128,7 @@ TEST_CASE("Width and Height reflect camera dimensions") { c.getLastImageMD(md); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_Width).GetValue() == "256"); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_Height).GetValue() == "128"); + c.stopSequenceAcquisition(); } TEST_CASE("ImageNumber increments across inserted images") { @@ -133,7 +137,7 @@ TEST_CASE("ImageNumber increments across inserted images") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); cam.InsertTestImage(); cam.InsertTestImage(); @@ -149,6 +153,7 @@ TEST_CASE("ImageNumber increments across inserted images") { c.popNextImageMD(md); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_ImageNumber).GetValue() == "2"); + c.stopSequenceAcquisition(); } TEST_CASE("Unconditionally-added fields overwrite camera-provided values") { @@ -157,7 +162,7 @@ TEST_CASE("Unconditionally-added fields overwrite camera-provided values") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); MM::CameraImageMetadata camMd; camMd.AddTag(MM::g_Keyword_Metadata_CameraLabel, "WRONG"); @@ -181,6 +186,7 @@ TEST_CASE("Unconditionally-added fields overwrite camera-provided values") { CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_TimeInCore).GetValue() != "wrong"); CHECK(md.GetKeys().size() == 7); + c.stopSequenceAcquisition(); } TEST_CASE("ElapsedTime-ms preserved when camera provides it") { @@ -189,7 +195,7 @@ TEST_CASE("ElapsedTime-ms preserved when camera provides it") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); MM::CameraImageMetadata camMd; camMd.AddTag(MM::g_Keyword_Elapsed_Time_ms, "42.0"); @@ -198,40 +204,7 @@ TEST_CASE("ElapsedTime-ms preserved when camera provides it") { Metadata md; c.getLastImageMD(md); CHECK(md.GetSingleTag(MM::g_Keyword_Elapsed_Time_ms).GetValue() == "42.0"); -} - -TEST_CASE("Custom device tag is transmitted") { - StubCamera cam; - MockAdapterWithDevices adapter{{"cam", &cam}}; - CMMCore c; - adapter.LoadIntoCore(c); - c.setCameraDevice("cam"); - c.initializeCircularBuffer(); - - cam.AddTag("MyCustomTag", "cam", "hello"); - cam.InsertTestImage(); - - Metadata md; - c.getLastImageMD(md); - CHECK(md.GetSingleTag("cam-MyCustomTag").GetValue() == "hello"); - CHECK(md.GetKeys().size() == 8); -} - -TEST_CASE("RemoveTag removes a previously added device tag") { - StubCamera cam; - MockAdapterWithDevices adapter{{"cam", &cam}}; - CMMCore c; - adapter.LoadIntoCore(c); - c.setCameraDevice("cam"); - c.initializeCircularBuffer(); - - cam.AddTag("MyCustomTag", "cam", "hello"); - cam.RemoveTag("cam-MyCustomTag"); - cam.InsertTestImage(); - - Metadata md; - c.getLastImageMD(md); - CHECK(md.GetKeys().size() == 7); + c.stopSequenceAcquisition(); } TEST_CASE("ImageNumber is tracked per camera across interleaved inserts") { @@ -241,7 +214,8 @@ TEST_CASE("ImageNumber is tracked per camera across interleaved inserts") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("camA"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition("camA", 10, 0.0, true); + c.startSequenceAcquisition("camB", 10, 0.0, true); REQUIRE(camA.InsertTestImage() == DEVICE_OK); REQUIRE(camB.InsertTestImage() == DEVICE_OK); @@ -260,6 +234,9 @@ TEST_CASE("ImageNumber is tracked per camera across interleaved inserts") { CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_ImageNumber).GetValue() == e.number); } + + c.stopSequenceAcquisition("camA"); + c.stopSequenceAcquisition("camB"); } TEST_CASE("ImageNumber resets after clearCircularBuffer") { @@ -268,7 +245,7 @@ TEST_CASE("ImageNumber resets after clearCircularBuffer") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); REQUIRE(cam.InsertTestImage() == DEVICE_OK); @@ -280,6 +257,7 @@ TEST_CASE("ImageNumber resets after clearCircularBuffer") { Metadata md; c.popNextImageMD(md); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_ImageNumber).GetValue() == "0"); + c.stopSequenceAcquisition(); } TEST_CASE("ImageNumber resets after re-initializeCircularBuffer") { @@ -288,17 +266,19 @@ TEST_CASE("ImageNumber resets after re-initializeCircularBuffer") { CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); REQUIRE(cam.InsertTestImage() == DEVICE_OK); + c.stopSequenceAcquisition(); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(10, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); Metadata md; c.popNextImageMD(md); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_ImageNumber).GetValue() == "0"); + c.stopSequenceAcquisition(); } TEST_CASE("ImageNumber is monotonic across overwrite-on-overflow wrap") { @@ -335,7 +315,7 @@ TEST_CASE("ImageNumbers are contiguous under stop-on-overflow") { adapter.LoadIntoCore(c); c.setCameraDevice("cam"); c.setCircularBufferMemoryFootprint(1); - c.initializeCircularBuffer(); + c.startSequenceAcquisition(100, 0.0, true); long total = c.getBufferTotalCapacity(); REQUIRE(total == 4); @@ -351,4 +331,5 @@ TEST_CASE("ImageNumbers are contiguous under stop-on-overflow") { CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_ImageNumber).GetValue() == std::to_string(i)); } + c.stopSequenceAcquisition(); } diff --git a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp index 7635b910e..880524d8c 100644 --- a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp @@ -21,27 +21,17 @@ namespace { // construction. // Mirrors the real adapter's behaviors that matter for MMCore-side testing: // - Reports GetNumberOfChannels() == number of physicals. -// - Calls AddTag on each physical with its own label as deviceLabel. +// - Exposes its physicals via GetChannelCameraPtr. // - Forwards Start/StopSequenceAcquisition to each physical. // - IsCapturing() reflects whether any physical is capturing. struct MockCompositeCamera : CCameraBase { std::string name = "MockCompositeCamera"; - std::vector physicals; + std::vector physicals; - explicit MockCompositeCamera(std::vector p) + explicit MockCompositeCamera(std::vector p) : physicals(std::move(p)) {} - int Initialize() override { - char myLabel[MM::MaxStrLength]; - GetLabel(myLabel); - for (size_t i = 0; i < physicals.size(); ++i) { - physicals[i]->AddTag(MM::g_Keyword_CameraChannelName, - myLabel, physicals[i]->name.c_str()); - physicals[i]->AddTag(MM::g_Keyword_CameraChannelIndex, - myLabel, std::to_string(i).c_str()); - } - return DEVICE_OK; - } + int Initialize() override { return DEVICE_OK; } int Shutdown() override { return DEVICE_OK; } bool Busy() override { return false; } void GetName(char* buf) const override { @@ -70,13 +60,15 @@ struct MockCompositeCamera : CCameraBase { } int GetChannelName(unsigned channel, char* chName) override { if (channel < physicals.size()) { - CDeviceUtils::CopyLimitedString(chName, - physicals[channel]->name.c_str()); + physicals[channel]->GetLabel(chName); } else { CDeviceUtils::CopyLimitedString(chName, ""); } return DEVICE_OK; } + MM::Camera* GetChannelCameraPtr(unsigned n) override { + return n < physicals.size() ? physicals[n] : nullptr; + } unsigned GetBitDepth() const override { return physicals.empty() ? 8 : physicals[0]->GetBitDepth(); } @@ -138,11 +130,12 @@ struct MockCompositeCamera : CCameraBase { }; // Minimal mock of an intrinsic multi-channel camera (in the style of the -// TwoPhoton adapter): a single device that itself emits N channels by calling -// InsertImage once per channel per frame. Channel-identifying tags are -// embedded in the serialized metadata passed to InsertImage, not stored on -// the device via AddTag (which is the composite pattern). Tests drive -// InsertTestFrame() manually after StartSequenceAcquisition. +// TwoPhoton adapter): a single device that itself emits N channels by +// calling InsertImage once per channel per frame, embedding only +// CameraChannelIndex in the serialized metadata. (CameraChannelName is +// stamped by MMCore from its start-time snapshot of GetChannelName, so +// intrinsic devices do not — and should not — emit it.) Tests drive +// InsertTestImage() manually after StartSequenceAcquisition. struct MockIntrinsicMultiChannelCamera : CCameraBase { std::string name = "MockIntrinsicMultiChannelCamera"; @@ -227,28 +220,17 @@ struct MockIntrinsicMultiChannelCamera : // completion or error path). void TriggerSelfFinish() { Finish(); } - // Inserts a single channel of a multi-channel frame, with channel- - // identifying tags in the serialized metadata. Tests call this once per - // channel per frame; the order is up to the test (real intrinsic adapters - // may interleave channels). - // - // The dual unprefixed + label-prefixed tag format mirrors the current - // behavior of the only in-tree intrinsic multi-chan camera (TwoPhoton, - // which is unmaintained) and may need updating if/when the tag rules are - // clarified. + // Inserts a single channel of a multi-channel frame, with the channel + // index embedded in the serialized metadata. (No channel name: MMCore + // stamps it from its start-time snapshot.) Tests call this once per + // channel per frame; the order is up to the test (real intrinsic + // adapters may interleave channels). int InsertTestImage(unsigned channel) { - char label[MM::MaxStrLength]; - GetLabel(label); - const std::string labelStr = label; std::vector buf( static_cast(width) * height * bytesPerPixel, 0); MM::CameraImageMetadata md; - const std::string idx = std::to_string(channel); - md.AddTag(MM::g_Keyword_CameraChannelIndex, idx); - md.AddTag(labelStr + '-' + MM::g_Keyword_CameraChannelIndex, idx); - md.AddTag(MM::g_Keyword_CameraChannelName, channelNames[channel]); - md.AddTag(labelStr + '-' + MM::g_Keyword_CameraChannelName, - channelNames[channel]); + md.AddTag(MM::g_Keyword_CameraChannelIndex, + std::to_string(channel).c_str()); return GetCoreCallback()->InsertImage(this, buf.data(), width, height, bytesPerPixel, md.Serialize()); } @@ -335,10 +317,10 @@ TEST_CASE("isSequenceRunning is true while composite camera is acquiring " CHECK_FALSE(c.isSequenceRunning()); } -// --- Tag attachment --- +// --- Tag scoping --- -TEST_CASE("Composite camera Initialize attaches CameraChannelName/Index tags " - "to physicals", +TEST_CASE("Composite phys cam used standalone after composite acquisition has " + "no composite-prefixed channel tags", "[MultiChannelSequenceAcquisition]") { SyncCamera p0("p0"); SyncCamera p1("p1"); @@ -347,24 +329,29 @@ TEST_CASE("Composite camera Initialize attaches CameraChannelName/Index tags " {"p0", &p0}, {"p1", &p1}, {"composite", &composite}}; CMMCore c; adapter.LoadIntoCore(c); - c.setCameraDevice("composite"); - c.initializeCircularBuffer(); + c.setCameraDevice("composite"); + c.startSequenceAcquisition(2, 0.0, true); REQUIRE(p0.InsertTestImage() == DEVICE_OK); - { - Metadata md; - c.popNextImageMD(md); - CHECK(md.GetSingleTag("composite-CameraChannelName").GetValue() == "p0"); - CHECK(md.GetSingleTag("composite-CameraChannelIndex").GetValue() == "0"); - } - REQUIRE(p1.InsertTestImage() == DEVICE_OK); - { + c.stopSequenceAcquisition(); + // Drain composite frames. + while (c.getRemainingImageCount() > 0) { Metadata md; c.popNextImageMD(md); - CHECK(md.GetSingleTag("composite-CameraChannelName").GetValue() == "p1"); - CHECK(md.GetSingleTag("composite-CameraChannelIndex").GetValue() == "1"); } + + // Now use p0 standalone — frames should not carry composite tags. + c.setCameraDevice("p0"); + c.startSequenceAcquisition(1, 0.0, true); + REQUIRE(p0.InsertTestImage() == DEVICE_OK); + c.stopSequenceAcquisition(); + + REQUIRE(c.getRemainingImageCount() == 1); + Metadata md; + c.popNextImageMD(md); + CHECK_THROWS(md.GetSingleTag("composite-CameraChannelIndex")); + CHECK_THROWS(md.GetSingleTag("composite-CameraChannelName")); } TEST_CASE("Composite circular buffer holds frames-times-channels", @@ -465,7 +452,8 @@ TEST_CASE("Named-camera startSequenceAcquisition on composite camera " // --- Cleanup --- -TEST_CASE("Composite: each physical's AcqFinished re-closes the auto-shutter", +TEST_CASE("Composite: only the last physical's AcqFinished closes the " + "auto-shutter", "[MultiChannelSequenceAcquisition]") { SyncCamera p0("p0"); SyncCamera p1("p1"); @@ -484,14 +472,11 @@ TEST_CASE("Composite: each physical's AcqFinished re-closes the auto-shutter", REQUIRE(shutter.open == true); p0.TriggerSelfFinish(); - CHECK(shutter.open == false); + // First physical to finish should not close the shutter. + CHECK(shutter.open == true); - // This might be a little over-constraining; the important thing is that the - // shutter gets closed at least once. But we can't easily write the correct - // test until we have the correct behavior (close shutter when the _last_ - // phys cam finishes). - shutter.open = true; p1.TriggerSelfFinish(); + // Last physical to finish closes it. CHECK(shutter.open == false); c.stopSequenceAcquisition(); @@ -585,14 +570,12 @@ TEST_CASE("Sequence acquisition with 2-channel intrinsic camera tags each " c.popNextImageMD(md); CHECK(md.GetSingleTag(MM::g_Keyword_Metadata_CameraLabel).GetValue() == "intrinsic"); - CHECK(md.GetSingleTag(MM::g_Keyword_CameraChannelName).GetValue() == - e.channelName); - CHECK(md.GetSingleTag(MM::g_Keyword_CameraChannelIndex).GetValue() == + CHECK(md.GetSingleTag("intrinsic-CameraChannelIndex").GetValue() == e.channelIndex); + // CameraChannelName is stamped by MMCore from its start-time snapshot + // even though the device only emits CameraChannelIndex. CHECK(md.GetSingleTag("intrinsic-CameraChannelName").GetValue() == e.channelName); - CHECK(md.GetSingleTag("intrinsic-CameraChannelIndex").GetValue() == - e.channelIndex); } } @@ -1034,3 +1017,84 @@ TEST_CASE("Mid-frame buffer overflow with stopOnOverflow=false overwrites " c.stopSequenceAcquisition(); } } + +// --- Nested-multi-channel rejection --- + +TEST_CASE("Composite camera whose phys cam is itself multi-channel is " + "rejected at start time", + "[MultiChannelSequenceAcquisition]") { + SyncCamera p0("p0"); + SyncCamera p1("p1"); + MockCompositeCamera inner({&p0, &p1}); + MockCompositeCamera outer({&inner}); + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"inner", &inner}, {"outer", &outer}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("outer"); + + CHECK_THROWS_AS(c.startSequenceAcquisition(10, 0.0, true), CMMError); + CHECK_FALSE(inner.IsCapturing()); +} + +// --- Single-channel composite --- + +TEST_CASE("Single-channel composite (one phys cam) tags its frames with " + "channel index 0 and the phys cam's channel name", + "[MultiChannelSequenceAcquisition]") { + SyncCamera p0("p0"); + MockCompositeCamera composite({&p0}); + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + + c.startSequenceAcquisition(1, 0.0, true); + REQUIRE(p0.InsertTestImage() == DEVICE_OK); + c.stopSequenceAcquisition(); + + REQUIRE(c.getRemainingImageCount() == 1); + Metadata md; + c.popNextImageMD(md); + CHECK(md.GetSingleTag("composite-CameraChannelIndex").GetValue() == "0"); + CHECK(md.GetSingleTag("composite-CameraChannelName").GetValue() == "p0"); +} + +// --- Plain single-channel camera, no opt-in --- + +TEST_CASE("Plain single-channel camera without CameraChannelIndex tag in " + "device metadata gets no composite-prefixed tags", + "[MultiChannelSequenceAcquisition]") { + SyncCamera cam("cam"); + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + + c.startSequenceAcquisition(1, 0.0, true); + REQUIRE(cam.InsertTestImage() == DEVICE_OK); + c.stopSequenceAcquisition(); + + REQUIRE(c.getRemainingImageCount() == 1); + Metadata md; + c.popNextImageMD(md); + CHECK_THROWS(md.GetSingleTag("cam-CameraChannelIndex")); + CHECK_THROWS(md.GetSingleTag("cam-CameraChannelName")); +} + +// --- Intrinsic emits invalid index --- + +TEST_CASE("Intrinsic multi-channel device emitting out-of-range " + "CameraChannelIndex has its image rejected", + "[MultiChannelSequenceAcquisition]") { + MockIntrinsicMultiChannelCamera cam({"chA", "chB"}); + MockAdapterWithDevices adapter{{"intrinsic", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("intrinsic"); + + c.startSequenceAcquisition(10, 0.0, true); + CHECK(cam.InsertTestImage(99) == DEVICE_INCOMPATIBLE_IMAGE); + c.stopSequenceAcquisition(); +} diff --git a/MMCore/unittest/SequenceAcquisition-Tests.cpp b/MMCore/unittest/SequenceAcquisition-Tests.cpp index 301145729..2721c684c 100644 --- a/MMCore/unittest/SequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/SequenceAcquisition-Tests.cpp @@ -246,10 +246,14 @@ TEST_CASE("startSequenceAcquisition clears pre-existing images from buffer", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + + c.startSequenceAcquisition(10, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); REQUIRE(cam.InsertTestImage() == DEVICE_OK); REQUIRE(c.getRemainingImageCount() == 2); + c.stopSequenceAcquisition(); + REQUIRE(c.getRemainingImageCount() == 2); + c.startSequenceAcquisition(10, 0.0, true); CHECK(c.getRemainingImageCount() == 0); c.stopSequenceAcquisition(); @@ -313,9 +317,12 @@ TEST_CASE("Named-camera startSequenceAcquisition initializes and clears buffer", CMMCore c; adapter.LoadIntoCore(c); c.setCameraDevice("cam"); - c.initializeCircularBuffer(); + + c.startSequenceAcquisition("cam", 10, 0.0, true); REQUIRE(cam.InsertTestImage() == DEVICE_OK); REQUIRE(c.getRemainingImageCount() == 1); + c.stopSequenceAcquisition("cam"); + c.startSequenceAcquisition("cam", 10, 0.0, true); CHECK(c.getRemainingImageCount() == 0); c.stopSequenceAcquisition("cam"); diff --git a/MMCore/unittest/StubDevices.h b/MMCore/unittest/StubDevices.h index b24eb53e4..c67b0439d 100644 --- a/MMCore/unittest/StubDevices.h +++ b/MMCore/unittest/StubDevices.h @@ -74,19 +74,21 @@ struct StubCamera : CCameraBase { return DEVICE_OK; } int StartSequenceAcquisition(long, double, bool) override { - return DEVICE_OK; + capturing = true; + return GetCoreCallback()->PrepareForAcq(this); } - int StartSequenceAcquisition(double) override { return DEVICE_OK; } - int StopSequenceAcquisition() override { return DEVICE_OK; } - bool IsCapturing() override { return capturing; } - - int PrepareForAcq() { + int StartSequenceAcquisition(double) override { + capturing = true; return GetCoreCallback()->PrepareForAcq(this); } - - int AcqFinished(int statusCode = 0) { - return GetCoreCallback()->AcqFinished(this, statusCode); + int StopSequenceAcquisition() override { + if (capturing) { + capturing = false; + GetCoreCallback()->AcqFinished(this, 0); + } + return DEVICE_OK; } + bool IsCapturing() override { return capturing; } int InsertTestImage( const MM::CameraImageMetadata& md = MM::CameraImageMetadata{}, @@ -167,13 +169,11 @@ struct SyncCamera : CCameraBase { int StartSequenceAcquisition(long, double, bool) override { capturing = true; - GetCoreCallback()->PrepareForAcq(this); - return DEVICE_OK; + return GetCoreCallback()->PrepareForAcq(this); } int StartSequenceAcquisition(double) override { capturing = true; - GetCoreCallback()->PrepareForAcq(this); - return DEVICE_OK; + return GetCoreCallback()->PrepareForAcq(this); } int StopSequenceAcquisition() override { Finish(); diff --git a/MMDevice/DeviceBase.h b/MMDevice/DeviceBase.h index fdc85cf9a..b9de8b010 100644 --- a/MMDevice/DeviceBase.h +++ b/MMDevice/DeviceBase.h @@ -1451,6 +1451,22 @@ class CCameraBase : public CDeviceBase return DEVICE_OK; } + /** + * @brief Return the physical camera responsible for channel `n`, or + * nullptr. + * + * Regular cameras should not override this function. + * + * Default implementation: returns nullptr (i.e., the camera is intrinsic + * or single-channel). Composite multi-channel cameras (such as + * MultiCamera) must override this to return the physical camera assigned + * to channel `n`. + */ + virtual MM::Camera* GetChannelCameraPtr(unsigned /* n */) + { + return nullptr; + } + /** * @brief Return the image buffer for a specific channel. * @@ -1469,19 +1485,6 @@ class CCameraBase : public CDeviceBase return 0; } - /** - * @brief Fill serializedMetadata with the device's metadata tags. - */ - virtual void GetTags(char* serializedMetadata) - { - MM::CameraImageMetadata md; - for (const auto& p : addedTags_) - { - md.AddTag(p.first.c_str(), p.second.c_str()); - } - CDeviceUtils::CopyLimitedString(serializedMetadata, md.Serialize()); - } - /** * @brief Start sequence acquisition. * @@ -1522,23 +1525,6 @@ class CCameraBase : public CDeviceBase virtual bool IsCapturing() = 0; - virtual void AddTag(const char* key, const char* deviceLabel, const char* value) - { - std::string k; - if (deviceLabel != std::string("_")) - { - k += deviceLabel; - k += '-'; - } - k += key; - addedTags_[k] = value; - } - - virtual void RemoveTag(const char* key) - { - addedTags_.erase(key); - } - virtual bool SupportsMultiROI() { return false; @@ -1566,9 +1552,6 @@ class CCameraBase : public CDeviceBase { return DEVICE_UNSUPPORTED_COMMAND; } - -private: - std::map addedTags_; }; diff --git a/MMDevice/MMDevice.h b/MMDevice/MMDevice.h index e79784ffc..74206cd3f 100644 --- a/MMDevice/MMDevice.h +++ b/MMDevice/MMDevice.h @@ -26,7 +26,7 @@ // Device Interface Version — see README.md for the full versioning policy. // Must be incremented for any binary-incompatible change. -#define DEVICE_INTERFACE_VERSION 75 +#define DEVICE_INTERFACE_VERSION 76 // N.B. Method parameters and return values in Device and its derived // classes must be POD types or pointers (no std::string, etc.) to @@ -379,6 +379,28 @@ namespace MM { * An implementation of this function is provided in DeviceBase.h. It will return an empty string */ virtual int GetChannelName(unsigned channel, char* name) = 0; + + /** + * @brief Return the physical camera responsible for channel `n`, or nullptr. + * + * For a composite multi-channel camera (such as Multi Camera), each + * channel is backed by a distinct physical Camera; this returns the + * MM::Camera* of the device assigned to channel `n`. For an intrinsic + * multi-channel camera (a single device emitting multiple channels) and + * for ordinary single-channel cameras, this returns nullptr. + * + * Mixed devices (some channels composite, others intrinsic) are + * permitted: channels with non-null pointers are treated as composite, + * channels with null pointers are treated as intrinsic and are + * MMCore-tagged accordingly via verification of CameraChannelIndex / + * CameraChannelName in the device-supplied image metadata. + * + * Required: 0 <= n < GetNumberOfChannels(); behavior outside that range + * is undefined except that the default CCameraBase implementation + * returns nullptr for any in-range n and logs+returns nullptr for + * out-of-range n. + */ + virtual MM::Camera* GetChannelCameraPtr(unsigned n) = 0; /** * @brief Return the size in bytes of the image buffer. * @@ -494,32 +516,6 @@ namespace MM { */ virtual bool IsCapturing() = 0; - /** - * @brief Get the metadata tags stored in this device. - * - * These tags will automatically be add to the metadata of an image inserted - * into the circular buffer. - */ - virtual void GetTags(char* serializedMetadata) = 0; - - /** - * @brief Add new tag or modify the value of an existing one. - * - * These will automatically be added to images inserted into the circular buffer. - * Use this mechanism for tags that do not change often. For metadata that - * change often, create an instance of metadata yourself and add to one of - * the versions of the InsertImage function. - */ - virtual void AddTag(const char* key, const char* deviceLabel, const char* value) = 0; - - /** - * @brief Remove an existing tag from the metadata associated with this device. - * - * These tags will automatically be add to the metadata of an image inserted - * into the circular buffer. - */ - virtual void RemoveTag(const char* key) = 0; - /** * @brief Return whether a camera's exposure time can be sequenced. * From 2173d662414579a1549bf93e122357dc26d69a30 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Fri, 1 May 2026 19:48:29 +0900 Subject: [PATCH 03/17] Fix indentation --- MMCore/MMCore.cpp | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index f0257f109..1daa56274 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -3158,25 +3158,25 @@ void CMMCore::startSequenceAcquisition(long numImages, double unused, bool stopO mmcore::internal::SequenceAcquisition::Create( camera, std::move(channels)); } - // Forward `unused` to the device rather than substituting 0.0: - // a small number of camera adapters (Andor) did implement this - // parameter, and passing 0.0 unconditionally could regress them. - // The MM::Camera contract now says new adapters must ignore it. - int nRet = camera->StartSequenceAcquisition(numImages, unused, stopOnOverflow); - if (nRet != DEVICE_OK) { - { - std::lock_guard aqg(acquisitionsMutex_); - acquisitions_.erase(camera->GetLabel()); - } - throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); - } - } - catch (std::bad_alloc& ex) - { - std::ostringstream messs; - messs << getCoreErrorText(MMERR_OutOfMemory).c_str() << " " << ex.what() << '\n'; - throw CMMError(messs.str().c_str() , MMERR_OutOfMemory); - } + // Forward `unused` to the device rather than substituting 0.0: + // a small number of camera adapters (Andor) did implement this + // parameter, and passing 0.0 unconditionally could regress them. + // The MM::Camera contract now says new adapters must ignore it. + int nRet = camera->StartSequenceAcquisition(numImages, unused, stopOnOverflow); + if (nRet != DEVICE_OK) { + { + std::lock_guard aqg(acquisitionsMutex_); + acquisitions_.erase(camera->GetLabel()); + } + throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); + } + } + catch (std::bad_alloc& ex) + { + std::ostringstream messs; + messs << getCoreErrorText(MMERR_OutOfMemory).c_str() << " " << ex.what() << '\n'; + throw CMMError(messs.str().c_str() , MMERR_OutOfMemory); + } } else { From b882e215b5245b041e2f1a43837b9cc7a688aa8b Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Fri, 1 May 2026 20:18:01 +0900 Subject: [PATCH 04/17] MMDevice: document seq acq contract better Finally. (Assisted by Claude Code; any errors are mine.) --- MMDevice/DeviceBase.h | 12 ++++++++-- MMDevice/MMDevice.h | 51 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/MMDevice/DeviceBase.h b/MMDevice/DeviceBase.h index b9de8b010..38d3841ed 100644 --- a/MMDevice/DeviceBase.h +++ b/MMDevice/DeviceBase.h @@ -1418,7 +1418,10 @@ class CCameraBase : public CDeviceBase virtual int StartSequenceAcquisition(double unused) = 0; /** - * @brief Stop and wait for the thread to finish. + * @brief Stop and wait for the acquisition thread to finish. + * + * See `MM::Camera::StopSequenceAcquisition` for the behavioral + * contract. */ virtual int StopSequenceAcquisition() = 0; @@ -1665,7 +1668,12 @@ class CLegacyCameraBase : public CCameraBase virtual long GetImageCounter() {return thd_->GetImageCounter();} virtual long GetNumberOfImages() {return thd_->GetNumberOfImages();} - // called from the thread function before exit + /** + * @brief Called from the acquisition thread before it exits. + * + * The default calls `Core::AcqFinished()`. Overrides must also call + * `AcqFinished()` exactly once. + */ virtual void OnThreadExiting() { try diff --git a/MMDevice/MMDevice.h b/MMDevice/MMDevice.h index 74206cd3f..c69be62dd 100644 --- a/MMDevice/MMDevice.h +++ b/MMDevice/MMDevice.h @@ -485,6 +485,9 @@ namespace MM { /** * @brief Start sequence acquisition. * + * Implementations must call `Core::PrepareForAcq()` before inserting + * any images. + * * @param numImages Number of images to acquire. * @param unused Has no effect. Implementations **must** ignore * this parameter. Previously named `intervalMs` / @@ -507,12 +510,22 @@ namespace MM { virtual int StartSequenceAcquisition(double unused) = 0; /** * @brief Stop an ongoing sequence acquisition. + * + * Must be synchronous: must not return until the acquisition has + * actually stopped (acquisition thread joined or equivalent). After + * return, `IsCapturing()` must return false. + * + * Must be a no-op when no acquisition is running. + * + * Should not call `Core::AcqFinished()` directly — see + * `Core::AcqFinished()` for the rationale. */ virtual int StopSequenceAcquisition() = 0; /** * @brief Indicate whether sequence acquisition is currently running. * - * Returns true when sequence acquisition is active, false otherwise. + * Must return false after `StopSequenceAcquisition()` returns and + * after an acquisition finishes on its own. Safe to call at any time. */ virtual bool IsCapturing() = 0; @@ -1685,8 +1698,42 @@ namespace MM { // Prefer std::chrono::steady_clock::now() in new code. virtual MM::MMTime GetCurrentMMTime() = 0; - // sequence acquisition + /** + * @brief Notify the Core that a sequence acquisition has finished. + * + * Must be called exactly once per acquisition, when the acquisition + * actually stops — whether it completed all requested images, + * encountered an error, or was told to stop via + * `Camera::StopSequenceAcquisition()`. + * + * Call from the acquisition thread as it exits (e.g., in an + * `OnThreadExiting()` override), not from + * `StopSequenceAcquisition()` itself. This ensures the callback + * fires both when the acquisition finishes on its own and when it is + * stopped externally. When `StopSequenceAcquisition()` joins the + * acquisition thread, `AcqFinished()` will be called during the + * join, before `StopSequenceAcquisition()` returns. + * + * For multi-channel cameras: each physical sub-camera calls + * `AcqFinished()` independently. The primary camera calls only if + * it has at least one intrinsic channel. + * + * @param caller The calling device (pass `this`). + * @param statusCode 0 on success, or an error code. + */ virtual int AcqFinished(const Device* caller, int statusCode) = 0; + /** + * @brief Prepare the Core for a sequence acquisition. + * + * Must be called during `Camera::StartSequenceAcquisition()`, before + * inserting any images. + * + * For multi-channel cameras: each physical sub-camera calls + * `PrepareForAcq()` independently. The primary camera calls only if + * it has at least one intrinsic channel. + * + * @param caller The calling device (pass `this`). + */ virtual int PrepareForAcq(const Device* caller) = 0; /** From 35f7f350ea95a88a8e8977029c353e9b38a4438e Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Fri, 1 May 2026 21:00:25 +0900 Subject: [PATCH 05/17] MMCore: DRY start/stopSequenceAcquisition() Factor out the common code and remove minor differences (which were presumably not intentional). Differences resolved: - Camera adapter lock is acquired _before_ initializing the sequence buffer (because the latter accesses camera parameters). - Don't catch std::bad_alloc (CircularBuffer::Initialize() already does for the main buffer memory). (Assisted by Claude Code; any errors are mine.) --- MMCore/MMCore.cpp | 303 +++++++++++++++++----------------------------- MMCore/MMCore.h | 8 ++ 2 files changed, 120 insertions(+), 191 deletions(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index 1daa56274..ed509fa78 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -943,6 +943,22 @@ void CMMCore::eraseCompletedAcquisition(const std::string& cameraLabel) acquisitions_.erase(cameraLabel); } +void CMMCore::markAcquisitionStopRequested(const std::string& cameraLabel) +{ + std::shared_ptr sa; + { + std::lock_guard aqg(acquisitionsMutex_); + auto it = acquisitions_.find(cameraLabel); + if (it != acquisitions_.end()) + sa = it->second; + } + if (sa) { + const bool causedComplete = sa->MarkStopRequested(); + if (causedComplete) + eraseCompletedAcquisition(sa->CameraLabel()); + } +} + void CMMCore::stopAndClearAllSequenceAcquisitions() { std::map camera, + bool overwriteData, + std::function startDevice) +{ + mmi::DeviceModuleLockGuard guard(camera); + if (camera->IsCapturing()) { + throw CMMError( + getCoreErrorText(MMERR_NotAllowedDuringSequenceAcquisition).c_str(), + MMERR_NotAllowedDuringSequenceAcquisition); + } + + if (!cbuf_->Initialize( + static_cast(camera->GetImageWidth()) * + camera->GetImageHeight() * + camera->GetImageBytesPerPixel())) + { + logError(getDeviceName(camera).c_str(), + getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str()); + throw CMMError( + getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str(), + MMERR_CircularBufferFailedToInitialize); + } + cbuf_->Clear(); + callback_->ResetImageInsertionState(); + cbuf_->SetOverwriteData(overwriteData); + + auto channels = buildSequenceChannelSnapshot(camera); + { + std::lock_guard aqg(acquisitionsMutex_); + acquisitions_[camera->GetLabel()] = + mmcore::internal::SequenceAcquisition::Create( + camera, std::move(channels)); + } + + int nRet = startDevice(); + if (nRet != DEVICE_OK) { + { + std::lock_guard aqg(acquisitionsMutex_); + acquisitions_.erase(camera->GetLabel()); + } + throw CMMError(getDeviceErrorText(nRet, camera).c_str(), + MMERR_DEVICE_GENERIC); + } +} + /** * Starts streaming camera sequence acquisition. * This command does not block the calling thread for the duration of the acquisition. @@ -3126,72 +3188,28 @@ long CMMCore::getImageBufferSize() void CMMCore::startSequenceAcquisition(long numImages, double unused, bool stopOnOverflow) MMCORE_LEGACY_THROW(CMMError) { std::shared_ptr camera = currentCameraDevice_.lock(); - if (camera) - { - if(camera->IsCapturing()) - { - throw CMMError(getCoreErrorText( - MMERR_NotAllowedDuringSequenceAcquisition).c_str() - ,MMERR_NotAllowedDuringSequenceAcquisition); - } - - try - { - if (!cbuf_->Initialize( - static_cast(camera->GetImageWidth()) * - camera->GetImageHeight() * - camera->GetImageBytesPerPixel())) - { - logError(getDeviceName(camera).c_str(), getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str()); - throw CMMError(getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str(), MMERR_CircularBufferFailedToInitialize); - } - cbuf_->Clear(); - callback_->ResetImageInsertionState(); - cbuf_->SetOverwriteData(!stopOnOverflow); - mmi::DeviceModuleLockGuard guard(camera); - - LOG_DEBUG(coreLogger_) << "Will start sequence acquisition from default camera"; - auto channels = buildSequenceChannelSnapshot(camera); - { - std::lock_guard aqg(acquisitionsMutex_); - acquisitions_[camera->GetLabel()] = - mmcore::internal::SequenceAcquisition::Create( - camera, std::move(channels)); - } - // Forward `unused` to the device rather than substituting 0.0: - // a small number of camera adapters (Andor) did implement this - // parameter, and passing 0.0 unconditionally could regress them. - // The MM::Camera contract now says new adapters must ignore it. - int nRet = camera->StartSequenceAcquisition(numImages, unused, stopOnOverflow); - if (nRet != DEVICE_OK) { - { - std::lock_guard aqg(acquisitionsMutex_); - acquisitions_.erase(camera->GetLabel()); - } - throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); - } - } - catch (std::bad_alloc& ex) - { - std::ostringstream messs; - messs << getCoreErrorText(MMERR_OutOfMemory).c_str() << " " << ex.what() << '\n'; - throw CMMError(messs.str().c_str() , MMERR_OutOfMemory); - } + if (!camera) { + logError("no camera available", + getCoreErrorText(MMERR_CameraNotAvailable).c_str()); + throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), + MMERR_CameraNotAvailable); } - else - { - logError("no camera available", getCoreErrorText(MMERR_CameraNotAvailable).c_str()); - throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), MMERR_CameraNotAvailable); - } - LOG_DEBUG(coreLogger_) << "Did start sequence acquisition from default camera"; - // onSequenceAcquisitionStarted will be called by CoreCallback::PrepareForAcq + LOG_DEBUG(coreLogger_) << + "Will start sequence acquisition from default camera"; + // Forward `unused` to the device rather than substituting 0.0: a small + // number of camera adapters (Andor) did implement this parameter, and + // passing 0.0 unconditionally could regress them. The MM::Camera + // contract now says new adapters must ignore it. + startSequenceAcquisitionImpl(camera, !stopOnOverflow, + [&] { return camera->StartSequenceAcquisition( + numImages, unused, stopOnOverflow); }); + LOG_DEBUG(coreLogger_) << + "Did start sequence acquisition from default camera"; } /** * Starts streaming camera sequence acquisition for a specified camera. * This command does not block the calling thread for the duration of the acquisition. - * The difference between this method and the one with the same name but operating on the "default" - * camera is that it does not automatically initialize the circular buffer. * * @param label Label of the camera device. * @param numImages Number of images requested from the camera. @@ -3205,48 +3223,13 @@ void CMMCore::startSequenceAcquisition(const char* label, long numImages, double { std::shared_ptr pCam = deviceManager_->GetDeviceOfType(label); - - mmi::DeviceModuleLockGuard guard(pCam); - if(pCam->IsCapturing()) - throw CMMError(getCoreErrorText(MMERR_NotAllowedDuringSequenceAcquisition).c_str(), - MMERR_NotAllowedDuringSequenceAcquisition); - - if (!cbuf_->Initialize( - static_cast(pCam->GetImageWidth()) * - pCam->GetImageHeight() * - pCam->GetImageBytesPerPixel())) - { - logError(getDeviceName(pCam).c_str(), getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str()); - throw CMMError(getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str(), MMERR_CircularBufferFailedToInitialize); - } - cbuf_->Clear(); - callback_->ResetImageInsertionState(); - cbuf_->SetOverwriteData(!stopOnOverflow); LOG_DEBUG(coreLogger_) << "Will start sequence acquisition from camera " << label; - auto channels = buildSequenceChannelSnapshot(pCam); - { - std::lock_guard aqg(acquisitionsMutex_); - acquisitions_[pCam->GetLabel()] = - mmcore::internal::SequenceAcquisition::Create( - pCam, std::move(channels)); - } - // Forward `unused` to the device rather than substituting 0.0: a small - // number of camera adapters (Andor) did implement this parameter, and - // passing 0.0 unconditionally could regress them. The MM::Camera - // contract now says new adapters must ignore it. - int nRet = pCam->StartSequenceAcquisition(numImages, unused, stopOnOverflow); - if (nRet != DEVICE_OK) { - { - std::lock_guard aqg(acquisitionsMutex_); - acquisitions_.erase(pCam->GetLabel()); - } - throw CMMError(getDeviceErrorText(nRet, pCam).c_str(), MMERR_DEVICE_GENERIC); - } - + startSequenceAcquisitionImpl(pCam, !stopOnOverflow, + [&] { return pCam->StartSequenceAcquisition( + numImages, unused, stopOnOverflow); }); LOG_DEBUG(coreLogger_) << "Did start sequence acquisition from camera " << label; - // onSequenceAcquisitionStarted will be called by CoreCallback::PrepareForAcq } /** @@ -3313,21 +3296,7 @@ void CMMCore::stopSequenceAcquisition(const char* label) MMCORE_LEGACY_THROW(CMM logError(label, getDeviceErrorText(nRet, pCam).c_str()); throw CMMError(getDeviceErrorText(nRet, pCam).c_str(), MMERR_DEVICE_GENERIC); } - { - std::shared_ptr sa; - { - std::lock_guard aqg(acquisitionsMutex_); - auto it = acquisitions_.find(pCam->GetLabel()); - if (it != acquisitions_.end()) - sa = it->second; - } - if (sa) { - const bool causedComplete = sa->MarkStopRequested(); - if (causedComplete) - eraseCompletedAcquisition(sa->CameraLabel()); - } - } - + markAcquisitionStopRequested(pCam->GetLabel()); LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from camera " << label; // onSequenceAcquisitionStopped will be called by CoreCallback::AcqFinished } @@ -3339,58 +3308,22 @@ void CMMCore::stopSequenceAcquisition(const char* label) MMCORE_LEGACY_THROW(CMM * @param unused Has no effect. Pass 0.0. See `startSequenceAcquisition` for * the history of this parameter. */ -void CMMCore::startContinuousSequenceAcquisition(double unused) MMCORE_LEGACY_THROW(CMMError) +void CMMCore::startContinuousSequenceAcquisition(double unused) + MMCORE_LEGACY_THROW(CMMError) { std::shared_ptr camera = currentCameraDevice_.lock(); - if (camera) - { - mmi::DeviceModuleLockGuard guard(camera); - if(camera->IsCapturing()) - { - throw CMMError(getCoreErrorText( - MMERR_NotAllowedDuringSequenceAcquisition).c_str() - ,MMERR_NotAllowedDuringSequenceAcquisition); - } - - if (!cbuf_->Initialize( - static_cast(camera->GetImageWidth()) * - camera->GetImageHeight() * - camera->GetImageBytesPerPixel())) - { - logError(getDeviceName(camera).c_str(), getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str()); - throw CMMError(getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str(), MMERR_CircularBufferFailedToInitialize); - } - cbuf_->Clear(); - callback_->ResetImageInsertionState(); - cbuf_->SetOverwriteData(true); - LOG_DEBUG(coreLogger_) << "Will start continuous sequence acquisition from current camera"; - auto channels = buildSequenceChannelSnapshot(camera); - { - std::lock_guard aqg(acquisitionsMutex_); - acquisitions_[camera->GetLabel()] = - mmcore::internal::SequenceAcquisition::Create( - camera, std::move(channels)); - } - // Forward `unused` to the device rather than substituting 0.0: a small - // number of camera adapters (Andor) did implement this parameter, and - // passing 0.0 unconditionally could regress them. The MM::Camera - // contract now says new adapters must ignore it. - int nRet = camera->StartSequenceAcquisition(unused); - if (nRet != DEVICE_OK) { - { - std::lock_guard aqg(acquisitionsMutex_); - acquisitions_.erase(camera->GetLabel()); - } - throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); - } + if (!camera) { + logError("no camera available", + getCoreErrorText(MMERR_CameraNotAvailable).c_str()); + throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), + MMERR_CameraNotAvailable); } - else - { - logError("no camera available", getCoreErrorText(MMERR_CameraNotAvailable).c_str()); - throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), MMERR_CameraNotAvailable); - } - LOG_DEBUG(coreLogger_) << "Did start continuous sequence acquisition from current camera"; - // onSequenceAcquisitionStarted will be called by CoreCallback::PrepareForAcq + LOG_DEBUG(coreLogger_) << + "Will start continuous sequence acquisition from current camera"; + startSequenceAcquisitionImpl(camera, true, + [&] { return camera->StartSequenceAcquisition(unused); }); + LOG_DEBUG(coreLogger_) << + "Did start continuous sequence acquisition from current camera"; } /** @@ -3399,37 +3332,25 @@ void CMMCore::startContinuousSequenceAcquisition(double unused) MMCORE_LEGACY_TH void CMMCore::stopSequenceAcquisition() MMCORE_LEGACY_THROW(CMMError) { std::shared_ptr camera = currentCameraDevice_.lock(); - if (camera) - { - mmi::DeviceModuleLockGuard guard(camera); - LOG_DEBUG(coreLogger_) << "Will stop sequence acquisition from current camera"; - int nRet = camera->StopSequenceAcquisition(); - if (nRet != DEVICE_OK) - { - logError(getDeviceName(camera).c_str(), getDeviceErrorText(nRet, camera).c_str()); - throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); - } - std::shared_ptr sa; - { - std::lock_guard aqg(acquisitionsMutex_); - auto it = acquisitions_.find(camera->GetLabel()); - if (it != acquisitions_.end()) - sa = it->second; - } - if (sa) { - const bool causedComplete = sa->MarkStopRequested(); - if (causedComplete) - eraseCompletedAcquisition(sa->CameraLabel()); - } + if (!camera) { + logError("no camera available", + getCoreErrorText(MMERR_CameraNotAvailable).c_str()); + throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), + MMERR_CameraNotAvailable); } - else - { - logError("no camera available", getCoreErrorText(MMERR_CameraNotAvailable).c_str()); - throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), MMERR_CameraNotAvailable); + mmi::DeviceModuleLockGuard guard(camera); + LOG_DEBUG(coreLogger_) << + "Will stop sequence acquisition from current camera"; + int nRet = camera->StopSequenceAcquisition(); + if (nRet != DEVICE_OK) { + logError(getDeviceName(camera).c_str(), + getDeviceErrorText(nRet, camera).c_str()); + throw CMMError(getDeviceErrorText(nRet, camera).c_str(), + MMERR_DEVICE_GENERIC); } - - LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from current camera"; - // onSequenceAcquisitionStopped will be called by CoreCallback::AcqFinished + markAcquisitionStopRequested(camera->GetLabel()); + LOG_DEBUG(coreLogger_) << + "Did stop sequence acquisition from current camera"; } /** diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index ec8ff9e87..91df1b387 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -60,6 +60,7 @@ #include #include +#include #include #include #include @@ -814,6 +815,13 @@ class CMMCore // Idempotent. Takes acquisitionsMutex_. void eraseCompletedAcquisition(const std::string& cameraLabel); + void startSequenceAcquisitionImpl( + std::shared_ptr camera, + bool overwriteData, + std::function startDevice); + + void markAcquisitionStopRequested(const std::string& cameraLabel); + void setCameraInternal(const std::string& label); void setShutterInternal(const std::string& label); void setFocusInternal(const std::string& label); From 9fb8fead78f8b969a44244599b8e7e7a22cf1726 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Sat, 2 May 2026 10:43:52 +0900 Subject: [PATCH 06/17] Deduplicate mock AsyncCamera (Assisted by Claude Code; any errors are mine.) --- MMCore/unittest/SequenceAcquisition-Tests.cpp | 142 ----------------- MMCore/unittest/StubDevices.h | 143 ++++++++++++++++++ MMCore/unittest/UnloadDevice-Tests.cpp | 140 ----------------- 3 files changed, 143 insertions(+), 282 deletions(-) diff --git a/MMCore/unittest/SequenceAcquisition-Tests.cpp b/MMCore/unittest/SequenceAcquisition-Tests.cpp index 2721c684c..8350f7df6 100644 --- a/MMCore/unittest/SequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/SequenceAcquisition-Tests.cpp @@ -6,151 +6,9 @@ #include "StubDevices.h" #include -#include -#include #include #include -// A camera that produces images asynchronously on its own thread. -struct AsyncCamera : CCameraBase { - std::string name = "AsyncCamera"; - unsigned width = 64; - unsigned height = 64; - unsigned bytesPerPixel = 1; - unsigned nComponents = 1; - unsigned bitDepth = 8; - int binning = 1; - double exposure = 10.0; - - int Initialize() override { return DEVICE_OK; } - int Shutdown() override { return DEVICE_OK; } - bool Busy() override { return false; } - void GetName(char* buf) const override { - CDeviceUtils::CopyLimitedString(buf, name.c_str()); - } - - int SnapImage() override { - imgBuf_.assign( - static_cast(width) * height * bytesPerPixel, 0); - return DEVICE_OK; - } - const unsigned char* GetImageBuffer() override { - return imgBuf_.data(); - } - long GetImageBufferSize() const override { - return static_cast(width) * height * bytesPerPixel; - } - unsigned GetImageWidth() const override { return width; } - unsigned GetImageHeight() const override { return height; } - unsigned GetImageBytesPerPixel() const override { return bytesPerPixel; } - unsigned GetNumberOfComponents() const override { return nComponents; } - unsigned GetBitDepth() const override { return bitDepth; } - int GetBinning() const override { return binning; } - int SetBinning(int b) override { binning = b; return DEVICE_OK; } - void SetExposure(double e) override { exposure = e; } - double GetExposure() const override { return exposure; } - int SetROI(unsigned, unsigned, unsigned, unsigned) override { - return DEVICE_OK; - } - int GetROI(unsigned& x, unsigned& y, unsigned& w, unsigned& h) override { - x = 0; y = 0; w = width; h = height; - return DEVICE_OK; - } - int ClearROI() override { return DEVICE_OK; } - int IsExposureSequenceable(bool& seq) const override { - seq = false; - return DEVICE_OK; - } - - int StartSequenceAcquisition(long numImages, double /*unused*/, bool /*stopOnOverflow*/) override { - GetCoreCallback()->PrepareForAcq(this); - { - std::lock_guard lk(mu_); - running_ = true; - stopRequested_ = false; - } - thread_ = std::thread([this, numImages] { - AcqThread(numImages); - }); - return DEVICE_OK; - } - - int StartSequenceAcquisition(double /*unused*/) override { - GetCoreCallback()->PrepareForAcq(this); - { - std::lock_guard lk(mu_); - running_ = true; - stopRequested_ = false; - } - thread_ = std::thread([this] { - AcqThread(-1); - }); - return DEVICE_OK; - } - - ~AsyncCamera() { - { - std::lock_guard lk(mu_); - stopRequested_ = true; - } - cv_.notify_one(); - if (thread_.joinable()) - thread_.join(); - } - - int StopSequenceAcquisition() override { - { - std::lock_guard lk(mu_); - stopRequested_ = true; - } - cv_.notify_one(); - if (thread_.joinable()) - thread_.join(); - return DEVICE_OK; - } - - bool IsCapturing() override { - std::lock_guard lk(mu_); - return running_; - } - -private: - void AcqThread(long numImages) { - std::vector buf( - static_cast(width) * height * bytesPerPixel, 0); - long count = 0; - while (numImages < 0 || count < numImages) { - { - std::lock_guard lk(mu_); - if (stopRequested_) - break; - } - GetCoreCallback()->InsertImage(this, buf.data(), - width, height, bytesPerPixel, nComponents, "{}"); - ++count; - { - std::unique_lock lk(mu_); - cv_.wait_for(lk, std::chrono::microseconds(100), - [this] { return stopRequested_; }); - if (stopRequested_) - break; - } - } - GetCoreCallback()->AcqFinished(this, 0); - { - std::lock_guard lk(mu_); - running_ = false; - } - } - - bool running_ = false; - bool stopRequested_ = false; - std::mutex mu_; - std::condition_variable cv_; - std::thread thread_; - std::vector imgBuf_; -}; - // --- Lifecycle error handling --- TEST_CASE("startSequenceAcquisition throws when no camera set", diff --git a/MMCore/unittest/StubDevices.h b/MMCore/unittest/StubDevices.h index c67b0439d..baf99628d 100644 --- a/MMCore/unittest/StubDevices.h +++ b/MMCore/unittest/StubDevices.h @@ -7,7 +7,10 @@ #include "CameraImageMetadata.h" #include "DeviceBase.h" +#include +#include #include +#include #include struct StubGeneric : CGenericBase { @@ -206,6 +209,146 @@ struct SyncCamera : CCameraBase { std::vector imgBuf_; }; +// A camera that produces images asynchronously on its own thread. +struct AsyncCamera : CCameraBase { + std::string name = "AsyncCamera"; + unsigned width = 64; + unsigned height = 64; + unsigned bytesPerPixel = 1; + unsigned nComponents = 1; + unsigned bitDepth = 8; + int binning = 1; + double exposure = 10.0; + + int Initialize() override { return DEVICE_OK; } + int Shutdown() override { return DEVICE_OK; } + bool Busy() override { return false; } + void GetName(char* buf) const override { + CDeviceUtils::CopyLimitedString(buf, name.c_str()); + } + + int SnapImage() override { + imgBuf_.assign( + static_cast(width) * height * bytesPerPixel, 0); + return DEVICE_OK; + } + const unsigned char* GetImageBuffer() override { + return imgBuf_.data(); + } + long GetImageBufferSize() const override { + return static_cast(width) * height * bytesPerPixel; + } + unsigned GetImageWidth() const override { return width; } + unsigned GetImageHeight() const override { return height; } + unsigned GetImageBytesPerPixel() const override { return bytesPerPixel; } + unsigned GetNumberOfComponents() const override { return nComponents; } + unsigned GetBitDepth() const override { return bitDepth; } + int GetBinning() const override { return binning; } + int SetBinning(int b) override { binning = b; return DEVICE_OK; } + void SetExposure(double e) override { exposure = e; } + double GetExposure() const override { return exposure; } + int SetROI(unsigned, unsigned, unsigned, unsigned) override { + return DEVICE_OK; + } + int GetROI(unsigned& x, unsigned& y, unsigned& w, unsigned& h) override { + x = 0; y = 0; w = width; h = height; + return DEVICE_OK; + } + int ClearROI() override { return DEVICE_OK; } + int IsExposureSequenceable(bool& seq) const override { + seq = false; + return DEVICE_OK; + } + + int StartSequenceAcquisition(long numImages, double /*unused*/, bool /*stopOnOverflow*/) override { + GetCoreCallback()->PrepareForAcq(this); + { + std::lock_guard lk(mu_); + running_ = true; + stopRequested_ = false; + } + thread_ = std::thread([this, numImages] { + AcqThread(numImages); + }); + return DEVICE_OK; + } + + int StartSequenceAcquisition(double /*unused*/) override { + GetCoreCallback()->PrepareForAcq(this); + { + std::lock_guard lk(mu_); + running_ = true; + stopRequested_ = false; + } + thread_ = std::thread([this] { + AcqThread(-1); + }); + return DEVICE_OK; + } + + ~AsyncCamera() { + { + std::lock_guard lk(mu_); + stopRequested_ = true; + } + cv_.notify_one(); + if (thread_.joinable()) + thread_.join(); + } + + int StopSequenceAcquisition() override { + { + std::lock_guard lk(mu_); + stopRequested_ = true; + } + cv_.notify_one(); + if (thread_.joinable()) + thread_.join(); + return DEVICE_OK; + } + + bool IsCapturing() override { + std::lock_guard lk(mu_); + return running_; + } + +private: + void AcqThread(long numImages) { + std::vector buf( + static_cast(width) * height * bytesPerPixel, 0); + long count = 0; + while (numImages < 0 || count < numImages) { + { + std::lock_guard lk(mu_); + if (stopRequested_) + break; + } + GetCoreCallback()->InsertImage(this, buf.data(), + width, height, bytesPerPixel, nComponents, "{}"); + ++count; + { + std::unique_lock lk(mu_); + cv_.wait_for(lk, std::chrono::microseconds(100), + [this] { return stopRequested_; }); + if (stopRequested_) + break; + } + } + GetCoreCallback()->AcqFinished(this, 0); + { + std::lock_guard lk(mu_); + running_ = false; + } + } + + bool running_ = false; + bool stopRequested_ = false; + std::mutex mu_; + std::condition_variable cv_; + std::thread thread_; + std::vector imgBuf_; +}; + struct StubStage : CStageBase { std::string name = "StubStage"; using CStageBase::OnStagePositionChanged; diff --git a/MMCore/unittest/UnloadDevice-Tests.cpp b/MMCore/unittest/UnloadDevice-Tests.cpp index 34b336013..a581bbbe0 100644 --- a/MMCore/unittest/UnloadDevice-Tests.cpp +++ b/MMCore/unittest/UnloadDevice-Tests.cpp @@ -5,149 +5,9 @@ #include "StubDevices.h" #include -#include -#include #include #include -namespace { - -// Camera that produces images on its own thread between Start and Stop. -struct AsyncCamera : CCameraBase { - std::string name = "AsyncCamera"; - unsigned width = 64; - unsigned height = 64; - unsigned bytesPerPixel = 1; - unsigned nComponents = 1; - unsigned bitDepth = 8; - int binning = 1; - double exposure = 10.0; - - int Initialize() override { return DEVICE_OK; } - int Shutdown() override { return DEVICE_OK; } - bool Busy() override { return false; } - void GetName(char* buf) const override { - CDeviceUtils::CopyLimitedString(buf, name.c_str()); - } - - int SnapImage() override { - imgBuf_.assign( - static_cast(width) * height * bytesPerPixel, 0); - return DEVICE_OK; - } - const unsigned char* GetImageBuffer() override { return imgBuf_.data(); } - long GetImageBufferSize() const override { - return static_cast(width) * height * bytesPerPixel; - } - unsigned GetImageWidth() const override { return width; } - unsigned GetImageHeight() const override { return height; } - unsigned GetImageBytesPerPixel() const override { return bytesPerPixel; } - unsigned GetNumberOfComponents() const override { return nComponents; } - unsigned GetBitDepth() const override { return bitDepth; } - int GetBinning() const override { return binning; } - int SetBinning(int b) override { binning = b; return DEVICE_OK; } - void SetExposure(double e) override { exposure = e; } - double GetExposure() const override { return exposure; } - int SetROI(unsigned, unsigned, unsigned, unsigned) override { - return DEVICE_OK; - } - int GetROI(unsigned& x, unsigned& y, unsigned& w, unsigned& h) override { - x = 0; y = 0; w = width; h = height; - return DEVICE_OK; - } - int ClearROI() override { return DEVICE_OK; } - int IsExposureSequenceable(bool& seq) const override { - seq = false; - return DEVICE_OK; - } - - int StartSequenceAcquisition(long numImages, double, bool) override { - GetCoreCallback()->PrepareForAcq(this); - { - std::lock_guard lk(mu_); - running_ = true; - stopRequested_ = false; - } - thread_ = std::thread([this, numImages] { AcqThread(numImages); }); - return DEVICE_OK; - } - - int StartSequenceAcquisition(double) override { - GetCoreCallback()->PrepareForAcq(this); - { - std::lock_guard lk(mu_); - running_ = true; - stopRequested_ = false; - } - thread_ = std::thread([this] { AcqThread(-1); }); - return DEVICE_OK; - } - - ~AsyncCamera() { - { - std::lock_guard lk(mu_); - stopRequested_ = true; - } - cv_.notify_one(); - if (thread_.joinable()) - thread_.join(); - } - - int StopSequenceAcquisition() override { - { - std::lock_guard lk(mu_); - stopRequested_ = true; - } - cv_.notify_one(); - if (thread_.joinable()) - thread_.join(); - return DEVICE_OK; - } - - bool IsCapturing() override { - std::lock_guard lk(mu_); - return running_; - } - -private: - void AcqThread(long numImages) { - std::vector buf( - static_cast(width) * height * bytesPerPixel, 0); - long count = 0; - while (numImages < 0 || count < numImages) { - { - std::lock_guard lk(mu_); - if (stopRequested_) - break; - } - GetCoreCallback()->InsertImage(this, buf.data(), - width, height, bytesPerPixel, nComponents, "{}"); - ++count; - { - std::unique_lock lk(mu_); - cv_.wait_for(lk, std::chrono::microseconds(100), - [this] { return stopRequested_; }); - if (stopRequested_) - break; - } - } - GetCoreCallback()->AcqFinished(this, 0); - { - std::lock_guard lk(mu_); - running_ = false; - } - } - - bool running_ = false; - bool stopRequested_ = false; - std::mutex mu_; - std::condition_variable cv_; - std::thread thread_; - std::vector imgBuf_; -}; - -} // namespace - TEST_CASE("Unload the current camera", "[regression]") { StubCamera cam; MockAdapterWithDevices adapter{{"cam", &cam}}; From 49caa1c67cf73d38c9613af33e2551bdf814ec9f Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Sat, 2 May 2026 10:44:53 +0900 Subject: [PATCH 07/17] Thread-safe autoshutter closing for seq acq Remove the unsafe hacks; when shutter is in the same adapter as the _primary_ camera (whose adapter we _may_ hold the lock for, in the same or different thread), fall back to closing the shutter afterwards on the thread calling stopSequenceAcquisition(). But always use try_lock() so that we start closing the shutter asap when we are able to -- so that the specimen is not exposed to illumination while the camera tears down the sequence acquisition (which for some cameras might be slow). Real (physical) cameras don't typically have a shutter in the same adapter. Demo/simulated cameras often do, but then we don't care if the shutter closure is ordered after the return from StopSequenceAcquisition(). Multi Camera (and similar) does not call AcqFinished() (only its physical cameras do), so even if the shutter is from the same adapter as the composite camera, we don't need to defer shutter closure. In no case do we call the shutter without holding the device module lock any more. (Assisted by Claude Code; any errors are mine.) --- MMCore/CoreCallback.cpp | 47 ++++------- MMCore/MMCore.cpp | 79 ++++++++++++++---- MMCore/MMCore.h | 6 ++ MMCore/SequenceAcquisition.cpp | 12 +++ MMCore/SequenceAcquisition.h | 4 + MMCore/unittest/MockDeviceUtils.h | 5 ++ .../MultiChannelSequenceAcquisition-Tests.cpp | 74 +++++++++++++++++ MMCore/unittest/SequenceAcquisition-Tests.cpp | 82 +++++++++++++++++++ 8 files changed, 261 insertions(+), 48 deletions(-) diff --git a/MMCore/CoreCallback.cpp b/MMCore/CoreCallback.cpp index daaaad5e3..fb1a9527d 100644 --- a/MMCore/CoreCallback.cpp +++ b/MMCore/CoreCallback.cpp @@ -419,50 +419,35 @@ int CoreCallback::AcqFinished(const MM::Device* caller, int /*statusCode*/) if (!isLast) return DEVICE_OK; - std::shared_ptr currentCamera = - core_->currentCameraDevice_.lock(); - if (core_->autoShutter_) { std::shared_ptr shutter = core_->currentShutterDevice_.lock(); if (shutter) { - // We need to lock the shutter's module for thread safety, but there's - // a case where deadlock would result. int sret = DEVICE_ERR; - if (camera->GetAdapterModule() == shutter->GetAdapterModule()) - { - // This is a nasty hack to allow the case where the shutter and - // camera live in the same module. It is not safe, but this is how - // _all_ cases used to be implemented, and I can't immediately - // think of a fully safe fix that is reasonably simple. - sret = shutter->SetOpen(false); - } - else if (currentCamera && currentCamera->GetAdapterModule() == - shutter->GetAdapterModule()) + if (shutter->GetAdapterModule() == + sa->Camera()->GetAdapterModule()) { - // Likewise, we might be called as a result of a call to - // StopSequenceAcquisition() on a virtual wrapper camera device - // (such as Multi Camera), in which case we would get a deadlock if - // the shutter is in the same module as the virtual camera. - // This is an even nastier hack in that it ignores the possibility - // of StopSequenceAcquisition() being called on a camera other than - // currentCamera, but such cases are rare. - sret = shutter->SetOpen(false); + // Shutter is in the same adapter module as the primary camera. + // stopSequenceAcquisition() may be holding that module's lock + // (on a different thread, waiting for this thread to exit via + // join), so a blocking lock would deadlock. Use try_lock: if + // the lock is free, close immediately; otherwise defer to + // stopSequenceAcquisition(), which will close after releasing + // the module lock. + auto& mtx = shutter->GetAdapterModule()->GetLock(); + if (mtx.try_lock()) { + std::lock_guard g(mtx, std::adopt_lock); + sret = shutter->SetOpen(false); + } else { + sa->DeferShutterClose(); + } } else { - // If the shutter is in a different device adapter, it is safe to - // lock that adapter. DeviceModuleLockGuard g(shutter); sret = shutter->SetOpen(false); - - // We could wait for the shutter to close here, but the - // implementation has always returned without waiting. The camera - // doesn't care, so let's keep the behavior. Thus, - // stopSequenceAcquisition() does not wait for the shutter before - // returning. } if (sret == DEVICE_OK) core_->postNotification(notif::ShutterOpenChanged{ diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index ed509fa78..55dcc9306 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -937,6 +937,14 @@ CMMCore::findAcquisitionByCaller(const MM::Device* caller) return nullptr; } +std::shared_ptr +CMMCore::findAcquisitionByCamera(const std::string& cameraLabel) +{ + std::lock_guard g(acquisitionsMutex_); + auto it = acquisitions_.find(cameraLabel); + return it != acquisitions_.end() ? it->second : nullptr; +} + void CMMCore::eraseCompletedAcquisition(const std::string& cameraLabel) { std::lock_guard g(acquisitionsMutex_); @@ -959,6 +967,24 @@ void CMMCore::markAcquisitionStopRequested(const std::string& cameraLabel) } } +void CMMCore::closeDeferredAutoShutter() +{ + if (!autoShutter_) + return; + std::shared_ptr shutter = + currentShutterDevice_.lock(); + if (!shutter) + return; + { + mmi::DeviceModuleLockGuard g(shutter); + int sret = shutter->SetOpen(false); + if (sret != DEVICE_OK) + return; + } + postNotification(notif::ShutterOpenChanged{ + shutter->GetLabel(), false}); +} + void CMMCore::stopAndClearAllSequenceAcquisitions() { std::map pCam = deviceManager_->GetDeviceOfType(label); - mmi::DeviceModuleLockGuard guard(pCam); - LOG_DEBUG(coreLogger_) << "Will stop sequence acquisition from camera " << label; - int nRet = pCam->StopSequenceAcquisition(); - if (nRet != DEVICE_OK) + auto sa = findAcquisitionByCamera(pCam->GetLabel()); + { - logError(label, getDeviceErrorText(nRet, pCam).c_str()); - throw CMMError(getDeviceErrorText(nRet, pCam).c_str(), MMERR_DEVICE_GENERIC); + mmi::DeviceModuleLockGuard guard(pCam); + LOG_DEBUG(coreLogger_) << + "Will stop sequence acquisition from camera " << label; + int nRet = pCam->StopSequenceAcquisition(); + if (nRet != DEVICE_OK) + { + logError(label, getDeviceErrorText(nRet, pCam).c_str()); + throw CMMError(getDeviceErrorText(nRet, pCam).c_str(), + MMERR_DEVICE_GENERIC); + } } + + if (sa && sa->TakeDeferredShutterClose()) + closeDeferredAutoShutter(); + markAcquisitionStopRequested(pCam->GetLabel()); - LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from camera " << label; - // onSequenceAcquisitionStopped will be called by CoreCallback::AcqFinished + LOG_DEBUG(coreLogger_) << + "Did stop sequence acquisition from camera " << label; } /** @@ -3338,16 +3374,25 @@ void CMMCore::stopSequenceAcquisition() MMCORE_LEGACY_THROW(CMMError) throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), MMERR_CameraNotAvailable); } - mmi::DeviceModuleLockGuard guard(camera); - LOG_DEBUG(coreLogger_) << - "Will stop sequence acquisition from current camera"; - int nRet = camera->StopSequenceAcquisition(); - if (nRet != DEVICE_OK) { - logError(getDeviceName(camera).c_str(), - getDeviceErrorText(nRet, camera).c_str()); - throw CMMError(getDeviceErrorText(nRet, camera).c_str(), - MMERR_DEVICE_GENERIC); + + auto sa = findAcquisitionByCamera(camera->GetLabel()); + + { + mmi::DeviceModuleLockGuard guard(camera); + LOG_DEBUG(coreLogger_) << + "Will stop sequence acquisition from current camera"; + int nRet = camera->StopSequenceAcquisition(); + if (nRet != DEVICE_OK) { + logError(getDeviceName(camera).c_str(), + getDeviceErrorText(nRet, camera).c_str()); + throw CMMError(getDeviceErrorText(nRet, camera).c_str(), + MMERR_DEVICE_GENERIC); + } } + + if (sa && sa->TakeDeferredShutterClose()) + closeDeferredAutoShutter(); + markAcquisitionStopRequested(camera->GetLabel()); LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from current camera"; diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index 91df1b387..b8df6f852 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -811,6 +811,10 @@ class CMMCore std::shared_ptr findAcquisitionByCaller(const MM::Device* caller); + // Look up SA by primary camera label. Returns nullptr if none. + std::shared_ptr + findAcquisitionByCamera(const std::string& cameraLabel); + // Erase a completed SA from `acquisitions_` (if present, by label). // Idempotent. Takes acquisitionsMutex_. void eraseCompletedAcquisition(const std::string& cameraLabel); @@ -822,6 +826,8 @@ class CMMCore void markAcquisitionStopRequested(const std::string& cameraLabel); + void closeDeferredAutoShutter(); + void setCameraInternal(const std::string& label); void setShutterInternal(const std::string& label); void setFocusInternal(const std::string& label); diff --git a/MMCore/SequenceAcquisition.cpp b/MMCore/SequenceAcquisition.cpp index 6593b88c4..7469f0593 100644 --- a/MMCore/SequenceAcquisition.cpp +++ b/MMCore/SequenceAcquisition.cpp @@ -180,5 +180,17 @@ bool SequenceAcquisition::IsComplete() const noexcept finishedParticipants_.size() == expectedParticipants_.size(); } +void SequenceAcquisition::DeferShutterClose() +{ + std::lock_guard g(mu_); + shutterCloseDeferred_ = true; +} + +bool SequenceAcquisition::TakeDeferredShutterClose() +{ + std::lock_guard g(mu_); + return std::exchange(shutterCloseDeferred_, false); +} + } // namespace internal } // namespace mmcore diff --git a/MMCore/SequenceAcquisition.h b/MMCore/SequenceAcquisition.h index 3986a16a1..1c5ebfbef 100644 --- a/MMCore/SequenceAcquisition.h +++ b/MMCore/SequenceAcquisition.h @@ -122,6 +122,9 @@ class SequenceAcquisition { // RecordFinish. bool IsComplete() const noexcept; + void DeferShutterClose(); + bool TakeDeferredShutterClose(); + private: SequenceAcquisition(std::shared_ptr camera, std::vector channels); @@ -137,6 +140,7 @@ class SequenceAcquisition { enum class ShutterState { NotOpened, Opening, Opened, OpenFailed }; ShutterState shutterState_ = ShutterState::NotOpened; bool stopRequested_ = false; + bool shutterCloseDeferred_ = false; std::set readyParticipants_; std::set finishedParticipants_; }; diff --git a/MMCore/unittest/MockDeviceUtils.h b/MMCore/unittest/MockDeviceUtils.h index 6a8438fa5..4d44fc859 100644 --- a/MMCore/unittest/MockDeviceUtils.h +++ b/MMCore/unittest/MockDeviceUtils.h @@ -18,6 +18,11 @@ class MockAdapterWithDevices : public MockDeviceAdapter { std::initializer_list> il) : devices(il) {} + explicit MockAdapterWithDevices( + std::string name, + std::initializer_list> il) + : adapter_name(std::move(name)), devices(il) {} + void InitializeModuleData(RegisterDeviceFunc registerDevice) override { for (auto name_device : devices) { const auto name = name_device.first; diff --git a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp index 880524d8c..4d9022e45 100644 --- a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp @@ -7,7 +7,9 @@ #include "MockDeviceUtils.h" #include "StubDevices.h" +#include #include +#include #include #include @@ -508,6 +510,78 @@ TEST_CASE("Composite: physical's AcqFinished does not touch shutter " c.stopSequenceAcquisition(); } +TEST_CASE("Composite with async physicals: same-module shutter closes on stop " + "without deadlock", + "[MultiChannelSequenceAcquisition]") { + AsyncCamera p0; + p0.name = "p0"; + AsyncCamera p1; + p1.name = "p1"; + MockCompositeCamera composite({&p0, &p1}); + StubShutter shutter; + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"composite", &composite}, + {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(1000000, 0.0, true); + REQUIRE(shutter.open == true); + c.stopSequenceAcquisition(); + CHECK(shutter.open == false); +} + +TEST_CASE("Composite in separate module from phys cam and shutter: shutter " + "closes via blocking lock without deadlock", + "[MultiChannelSequenceAcquisition]") { + AsyncCamera p0; + p0.name = "p0"; + MockCompositeCamera composite({&p0}); + StubShutter shutter; + MockAdapterWithDevices compositeAdapter{"composite_adapter", + {{"composite", &composite}}}; + MockAdapterWithDevices physAdapter{"phys_adapter", + {{"p0", &p0}, {"shutter", &shutter}}}; + CMMCore c; + compositeAdapter.LoadIntoCore(c); + physAdapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(1000000, 0.0, true); + REQUIRE(shutter.open == true); + c.stopSequenceAcquisition(); + CHECK(shutter.open == false); +} + +TEST_CASE("Composite with async physicals: same-module shutter not touched " + "when autoShutter is off", + "[MultiChannelSequenceAcquisition]") { + AsyncCamera p0; + p0.name = "p0"; + AsyncCamera p1; + p1.name = "p1"; + MockCompositeCamera composite({&p0, &p1}); + StubShutter shutter; + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"composite", &composite}, + {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + c.setShutterDevice("shutter"); + c.setAutoShutter(false); + + c.startSequenceAcquisition(1000000, 0.0, true); + shutter.open = true; + c.stopSequenceAcquisition(); + CHECK(shutter.open == true); +} + TEST_CASE("Composite: startSequenceAcquisition after all physicals self-finish " "without intervening stop succeeds", "[MultiChannelSequenceAcquisition]") { diff --git a/MMCore/unittest/SequenceAcquisition-Tests.cpp b/MMCore/unittest/SequenceAcquisition-Tests.cpp index 8350f7df6..bb3ddf2db 100644 --- a/MMCore/unittest/SequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/SequenceAcquisition-Tests.cpp @@ -396,6 +396,88 @@ TEST_CASE("Camera self-finish does not touch shutter when autoShutter is off", c.stopSequenceAcquisition(); } +// --- Async shutter close paths --- + +TEST_CASE("Async same-module shutter closes on stopSequenceAcquisition without " + "deadlock", + "[SequenceAcquisition]") { + AsyncCamera cam; + StubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(1000000, 0.0, true); + REQUIRE(shutter.open == true); + c.stopSequenceAcquisition(); + CHECK(shutter.open == false); +} + +TEST_CASE("Async same-module shutter closes on camera self-finish", + "[SequenceAcquisition]") { + AsyncCamera cam; + StubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(3, 0.0, true); + REQUIRE(shutter.open == true); + + auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (c.isSequenceRunning() && + std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + REQUIRE_FALSE(c.isSequenceRunning()); + CHECK(shutter.open == false); + c.stopSequenceAcquisition(); +} + +TEST_CASE("Async same-module shutter not touched when autoShutter is off", + "[SequenceAcquisition]") { + AsyncCamera cam; + StubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(false); + + c.startSequenceAcquisition(1000000, 0.0, true); + shutter.open = true; + c.stopSequenceAcquisition(); + CHECK(shutter.open == true); +} + +TEST_CASE("Async different-module shutter closes on stopSequenceAcquisition " + "without deadlock", + "[SequenceAcquisition]") { + AsyncCamera cam; + StubShutter shutter; + MockAdapterWithDevices camAdapter{"cam_adapter", {{"cam", &cam}}}; + MockAdapterWithDevices shutterAdapter{"shutter_adapter", + {{"shutter", &shutter}}}; + CMMCore c; + camAdapter.LoadIntoCore(c); + shutterAdapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(1000000, 0.0, true); + REQUIRE(shutter.open == true); + c.stopSequenceAcquisition(); + CHECK(shutter.open == false); +} + TEST_CASE("startSequenceAcquisition after camera self-finish without " "intervening stop succeeds", "[SequenceAcquisition]") { From d481fb728da5ec7921eb45465fab29dec1e0f4d0 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Sat, 2 May 2026 14:04:24 +0900 Subject: [PATCH 08/17] Roll back autoshutter on failed seq acq start Another long-standing bug we can now easily fix. (Assisted by Claude Code; any errors are mine.) --- MMCore/MMCore.cpp | 12 +++++++++++- MMCore/SequenceAcquisition.cpp | 6 ++++++ MMCore/SequenceAcquisition.h | 2 ++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index 55dcc9306..f34b72d03 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -3188,9 +3188,19 @@ void CMMCore::startSequenceAcquisitionImpl( int nRet = startDevice(); if (nRet != DEVICE_OK) { + std::shared_ptr sa; { std::lock_guard aqg(acquisitionsMutex_); - acquisitions_.erase(camera->GetLabel()); + auto it = acquisitions_.find(camera->GetLabel()); + if (it != acquisitions_.end()) { + sa = std::move(it->second); + acquisitions_.erase(it); + } + } + if (sa && sa->NeedsStartRollback()) { + closeDeferredAutoShutter(); + postNotification( + notif::SequenceAcquisitionStopped{sa->CameraLabel()}); } throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC); diff --git a/MMCore/SequenceAcquisition.cpp b/MMCore/SequenceAcquisition.cpp index 7469f0593..a9faf5db1 100644 --- a/MMCore/SequenceAcquisition.cpp +++ b/MMCore/SequenceAcquisition.cpp @@ -159,6 +159,12 @@ bool SequenceAcquisition::WaitForShutterOpened() return shutterState_ == ShutterState::Opened; } +bool SequenceAcquisition::NeedsStartRollback() const noexcept +{ + std::lock_guard g(mu_); + return shutterState_ == ShutterState::Opened; +} + bool SequenceAcquisition::RecordFinish(const MM::Device* caller) { if (caller == nullptr) diff --git a/MMCore/SequenceAcquisition.h b/MMCore/SequenceAcquisition.h index 1c5ebfbef..04ece3f36 100644 --- a/MMCore/SequenceAcquisition.h +++ b/MMCore/SequenceAcquisition.h @@ -111,6 +111,8 @@ class SequenceAcquisition { // true if Opened, false if OpenFailed. bool WaitForShutterOpened(); + bool NeedsStartRollback() const noexcept; + // Records that `caller` finished the acquisition. Returns true iff this is // the boundary call (last expected participant to finish), in which case // the caller is responsible for the auto-shutter close + notification. From e48e9e8b38273e0a394f8bdfc377a9558ffbe2b2 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Sat, 2 May 2026 17:41:49 +0900 Subject: [PATCH 09/17] Prevent concurrent seq acq with same cameras That is, disallow starting a sequence acquisition where any of the participants (primary + physical) overlap with an ongoing acquisition. (Assisted by Claude Code; any errors are mine.) --- MMCore/MMCore.cpp | 40 +++++- MMCore/SequenceAcquisition.cpp | 6 + MMCore/SequenceAcquisition.h | 4 + .../MultiChannelSequenceAcquisition-Tests.cpp | 116 ------------------ MMCore/unittest/SequenceAcquisition-Tests.cpp | 90 ++++++++++++++ MMCore/unittest/StubDevices.h | 111 ++++++++++++++++- 6 files changed, 247 insertions(+), 120 deletions(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index f34b72d03..a81b5dd2b 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -3179,11 +3179,45 @@ void CMMCore::startSequenceAcquisitionImpl( cbuf_->SetOverwriteData(overwriteData); auto channels = buildSequenceChannelSnapshot(camera); + auto newSa = mmcore::internal::SequenceAcquisition::Create( + camera, std::move(channels)); { std::lock_guard aqg(acquisitionsMutex_); - acquisitions_[camera->GetLabel()] = - mmcore::internal::SequenceAcquisition::Create( - camera, std::move(channels)); + + { + auto it = acquisitions_.find(camera->GetLabel()); + if (it != acquisitions_.end() && + !it->second->AllParticipantsFinished()) { + throw CMMError( + "Sequence acquisition on '" + camera->GetLabel() + + "' is already running", + MMERR_NotAllowedDuringSequenceAcquisition); + } + } + + for (const auto& [existingLabel, existingSa] : acquisitions_) { + if (existingSa->AllParticipantsFinished()) + continue; + if (existingSa->HasParticipant(camera->GetRawPtr())) { + throw CMMError( + "Camera '" + camera->GetLabel() + + "' is already participating in a sequence acquisition on '" + + existingLabel + "'", + MMERR_NotAllowedDuringSequenceAcquisition); + } + for (const auto& ch : newSa->Channels()) { + if (ch.physCamDevice && + existingSa->HasParticipant(ch.physCamDevice)) { + throw CMMError( + "Camera '" + ch.physCamLabel + + "' is already participating in a sequence acquisition on '" + + existingLabel + "'", + MMERR_NotAllowedDuringSequenceAcquisition); + } + } + } + + acquisitions_[camera->GetLabel()] = std::move(newSa); } int nRet = startDevice(); diff --git a/MMCore/SequenceAcquisition.cpp b/MMCore/SequenceAcquisition.cpp index a9faf5db1..cba27bcee 100644 --- a/MMCore/SequenceAcquisition.cpp +++ b/MMCore/SequenceAcquisition.cpp @@ -186,6 +186,12 @@ bool SequenceAcquisition::IsComplete() const noexcept finishedParticipants_.size() == expectedParticipants_.size(); } +bool SequenceAcquisition::AllParticipantsFinished() const noexcept +{ + std::lock_guard g(mu_); + return finishedParticipants_.size() == expectedParticipants_.size(); +} + void SequenceAcquisition::DeferShutterClose() { std::lock_guard g(mu_); diff --git a/MMCore/SequenceAcquisition.h b/MMCore/SequenceAcquisition.h index 04ece3f36..5d6bfe64c 100644 --- a/MMCore/SequenceAcquisition.h +++ b/MMCore/SequenceAcquisition.h @@ -124,6 +124,10 @@ class SequenceAcquisition { // RecordFinish. bool IsComplete() const noexcept; + // True iff all expected participants have called RecordFinish + // (regardless of whether stop was requested). + bool AllParticipantsFinished() const noexcept; + void DeferShutterClose(); bool TakeDeferredShutterClose(); diff --git a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp index 4d9022e45..674df7a01 100644 --- a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp @@ -15,122 +15,6 @@ namespace { -// Minimal mock of a composite multi-channel camera (in the style of the -// Multi Camera (Utilities) adapter, where multiple physical cameras are -// presented as channels of a single device — as opposed to an intrinsic -// multi-channel camera, which is a single device that itself emits multiple -// channels). Holds its physical cameras as direct pointers given at -// construction. -// Mirrors the real adapter's behaviors that matter for MMCore-side testing: -// - Reports GetNumberOfChannels() == number of physicals. -// - Exposes its physicals via GetChannelCameraPtr. -// - Forwards Start/StopSequenceAcquisition to each physical. -// - IsCapturing() reflects whether any physical is capturing. -struct MockCompositeCamera : CCameraBase { - std::string name = "MockCompositeCamera"; - std::vector physicals; - - explicit MockCompositeCamera(std::vector p) - : physicals(std::move(p)) {} - - int Initialize() override { return DEVICE_OK; } - int Shutdown() override { return DEVICE_OK; } - bool Busy() override { return false; } - void GetName(char* buf) const override { - CDeviceUtils::CopyLimitedString(buf, name.c_str()); - } - - int SnapImage() override { return DEVICE_OK; } - const unsigned char* GetImageBuffer() override { - return physicals.empty() ? nullptr : physicals[0]->GetImageBuffer(); - } - long GetImageBufferSize() const override { - return physicals.empty() ? 0 : physicals[0]->GetImageBufferSize(); - } - unsigned GetImageWidth() const override { - return physicals.empty() ? 0 : physicals[0]->GetImageWidth(); - } - unsigned GetImageHeight() const override { - return physicals.empty() ? 0 : physicals[0]->GetImageHeight(); - } - unsigned GetImageBytesPerPixel() const override { - return physicals.empty() ? 0 : physicals[0]->GetImageBytesPerPixel(); - } - unsigned GetNumberOfComponents() const override { return 1; } - unsigned GetNumberOfChannels() const override { - return static_cast(physicals.size()); - } - int GetChannelName(unsigned channel, char* chName) override { - if (channel < physicals.size()) { - physicals[channel]->GetLabel(chName); - } else { - CDeviceUtils::CopyLimitedString(chName, ""); - } - return DEVICE_OK; - } - MM::Camera* GetChannelCameraPtr(unsigned n) override { - return n < physicals.size() ? physicals[n] : nullptr; - } - unsigned GetBitDepth() const override { - return physicals.empty() ? 8 : physicals[0]->GetBitDepth(); - } - int GetBinning() const override { - return physicals.empty() ? 1 : physicals[0]->GetBinning(); - } - int SetBinning(int b) override { - for (auto* p : physicals) p->SetBinning(b); - return DEVICE_OK; - } - void SetExposure(double e) override { - for (auto* p : physicals) p->SetExposure(e); - } - double GetExposure() const override { - return physicals.empty() ? 0.0 : physicals[0]->GetExposure(); - } - int SetROI(unsigned, unsigned, unsigned, unsigned) override { - return DEVICE_OK; - } - int GetROI(unsigned& x, unsigned& y, unsigned& w, unsigned& h) override { - x = 0; y = 0; - w = GetImageWidth(); - h = GetImageHeight(); - return DEVICE_OK; - } - int ClearROI() override { return DEVICE_OK; } - int IsExposureSequenceable(bool& seq) const override { - seq = false; - return DEVICE_OK; - } - - int StartSequenceAcquisition(long n, double i, bool s) override { - for (auto* p : physicals) { - int ret = p->StartSequenceAcquisition(n, i, s); - if (ret != DEVICE_OK) return ret; - } - return DEVICE_OK; - } - int StartSequenceAcquisition(double i) override { - for (auto* p : physicals) { - int ret = p->StartSequenceAcquisition(i); - if (ret != DEVICE_OK) return ret; - } - return DEVICE_OK; - } - int StopSequenceAcquisition() override { - for (auto* p : physicals) { - int ret = p->StopSequenceAcquisition(); - if (ret != DEVICE_OK) return ret; - } - return DEVICE_OK; - } - bool IsCapturing() override { - for (auto* p : physicals) { - if (p->IsCapturing()) return true; - } - return false; - } -}; - // Minimal mock of an intrinsic multi-channel camera (in the style of the // TwoPhoton adapter): a single device that itself emits N channels by // calling InsertImage once per channel per frame, embedding only diff --git a/MMCore/unittest/SequenceAcquisition-Tests.cpp b/MMCore/unittest/SequenceAcquisition-Tests.cpp index bb3ddf2db..2fb3db89b 100644 --- a/MMCore/unittest/SequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/SequenceAcquisition-Tests.cpp @@ -30,6 +30,96 @@ TEST_CASE("startSequenceAcquisition throws when already capturing", c.stopSequenceAcquisition(); } +// --- Participant-level conflict detection --- + +TEST_CASE("Standalone camera conflicts with running composite that uses it", + "[SequenceAcquisition]") { + SyncCamera p1("p1"); + SyncCamera p2("p2"); + MockCompositeCamera composite({&p1, &p2}); + MockAdapterWithDevices adapter{ + {"p1", &p1}, {"p2", &p2}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + + c.startSequenceAcquisition("composite", 10, 0.0, true); + REQUIRE(c.isSequenceRunning("composite")); + + // Hide IsCapturing() so the acquisitions_ check is the one that fires. + p1.reportCapturing = false; + CHECK_THROWS_AS( + c.startSequenceAcquisition("p1", 10, 0.0, true), CMMError); + + CHECK(c.isSequenceRunning("composite")); + c.stopSequenceAcquisition("composite"); +} + +TEST_CASE("Composite conflicts with running standalone that shares a physical", + "[SequenceAcquisition]") { + SyncCamera p1("p1"); + SyncCamera p2("p2"); + MockCompositeCamera composite({&p1, &p2}); + MockAdapterWithDevices adapter{ + {"p1", &p1}, {"p2", &p2}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + + c.startSequenceAcquisition("p1", 10, 0.0, true); + REQUIRE(c.isSequenceRunning("p1")); + + CHECK_THROWS_AS( + c.startSequenceAcquisition("composite", 10, 0.0, true), CMMError); + + CHECK(c.isSequenceRunning("p1")); + c.stopSequenceAcquisition("p1"); +} + +TEST_CASE("Two composites sharing a physical camera conflict", + "[SequenceAcquisition]") { + SyncCamera p1("p1"); + SyncCamera p2("p2"); + SyncCamera p3("p3"); + MockCompositeCamera compositeA({&p1, &p2}); + MockCompositeCamera compositeB({&p1, &p3}); + compositeB.name = "compositeB"; + MockAdapterWithDevices adapter{ + {"p1", &p1}, {"p2", &p2}, {"p3", &p3}, + {"compositeA", &compositeA}, {"compositeB", &compositeB}}; + CMMCore c; + adapter.LoadIntoCore(c); + + c.startSequenceAcquisition("compositeA", 10, 0.0, true); + REQUIRE(c.isSequenceRunning("compositeA")); + + // Hide IsCapturing() on the physicals that compositeB would check. + p1.reportCapturing = false; + p3.reportCapturing = false; + CHECK_THROWS_AS( + c.startSequenceAcquisition("compositeB", 10, 0.0, true), CMMError); + + CHECK(c.isSequenceRunning("compositeA")); + c.stopSequenceAcquisition("compositeA"); +} + +TEST_CASE("Independent cameras do not conflict", + "[SequenceAcquisition]") { + SyncCamera p1("p1"); + SyncCamera p2("p2"); + MockAdapterWithDevices adapter{{"p1", &p1}, {"p2", &p2}}; + CMMCore c; + adapter.LoadIntoCore(c); + + c.startSequenceAcquisition("p1", 10, 0.0, true); + REQUIRE(c.isSequenceRunning("p1")); + + c.startSequenceAcquisition("p2", 10, 0.0, true); + CHECK(c.isSequenceRunning("p1")); + CHECK(c.isSequenceRunning("p2")); + + c.stopSequenceAcquisition("p1"); + c.stopSequenceAcquisition("p2"); +} + TEST_CASE("startContinuousSequenceAcquisition throws when no camera set", "[SequenceAcquisition]") { CMMCore c; diff --git a/MMCore/unittest/StubDevices.h b/MMCore/unittest/StubDevices.h index baf99628d..655a6e467 100644 --- a/MMCore/unittest/StubDevices.h +++ b/MMCore/unittest/StubDevices.h @@ -11,6 +11,7 @@ #include #include #include +#include #include struct StubGeneric : CGenericBase { @@ -129,6 +130,7 @@ struct SyncCamera : CCameraBase { int binning = 1; double exposure = 10.0; bool capturing = false; + bool reportCapturing = true; explicit SyncCamera(std::string n = "SyncCamera") : name(std::move(n)) {} @@ -182,7 +184,7 @@ struct SyncCamera : CCameraBase { Finish(); return DEVICE_OK; } - bool IsCapturing() override { return capturing; } + bool IsCapturing() override { return capturing && reportCapturing; } void TriggerSelfFinish() { Finish(); } @@ -617,3 +619,110 @@ struct StubGalvo : CGalvoBase { return DEVICE_OK; } }; + +// Minimal mock of a composite multi-channel camera (in the style of the +// Multi Camera adapter). Holds its physical cameras as direct pointers. +struct MockCompositeCamera : CCameraBase { + std::string name = "MockCompositeCamera"; + std::vector physicals; + + explicit MockCompositeCamera(std::vector p) + : physicals(std::move(p)) {} + + int Initialize() override { return DEVICE_OK; } + int Shutdown() override { return DEVICE_OK; } + bool Busy() override { return false; } + void GetName(char* buf) const override { + CDeviceUtils::CopyLimitedString(buf, name.c_str()); + } + + int SnapImage() override { return DEVICE_OK; } + const unsigned char* GetImageBuffer() override { + return physicals.empty() ? nullptr : physicals[0]->GetImageBuffer(); + } + long GetImageBufferSize() const override { + return physicals.empty() ? 0 : physicals[0]->GetImageBufferSize(); + } + unsigned GetImageWidth() const override { + return physicals.empty() ? 0 : physicals[0]->GetImageWidth(); + } + unsigned GetImageHeight() const override { + return physicals.empty() ? 0 : physicals[0]->GetImageHeight(); + } + unsigned GetImageBytesPerPixel() const override { + return physicals.empty() ? 0 : physicals[0]->GetImageBytesPerPixel(); + } + unsigned GetNumberOfComponents() const override { return 1; } + unsigned GetNumberOfChannels() const override { + return static_cast(physicals.size()); + } + int GetChannelName(unsigned channel, char* chName) override { + if (channel < physicals.size()) { + physicals[channel]->GetLabel(chName); + } else { + CDeviceUtils::CopyLimitedString(chName, ""); + } + return DEVICE_OK; + } + MM::Camera* GetChannelCameraPtr(unsigned n) override { + return n < physicals.size() ? physicals[n] : nullptr; + } + unsigned GetBitDepth() const override { + return physicals.empty() ? 8 : physicals[0]->GetBitDepth(); + } + int GetBinning() const override { + return physicals.empty() ? 1 : physicals[0]->GetBinning(); + } + int SetBinning(int b) override { + for (auto* p : physicals) p->SetBinning(b); + return DEVICE_OK; + } + void SetExposure(double e) override { + for (auto* p : physicals) p->SetExposure(e); + } + double GetExposure() const override { + return physicals.empty() ? 0.0 : physicals[0]->GetExposure(); + } + int SetROI(unsigned, unsigned, unsigned, unsigned) override { + return DEVICE_OK; + } + int GetROI(unsigned& x, unsigned& y, unsigned& w, unsigned& h) override { + x = 0; y = 0; + w = GetImageWidth(); + h = GetImageHeight(); + return DEVICE_OK; + } + int ClearROI() override { return DEVICE_OK; } + int IsExposureSequenceable(bool& seq) const override { + seq = false; + return DEVICE_OK; + } + + int StartSequenceAcquisition(long n, double i, bool s) override { + for (auto* p : physicals) { + int ret = p->StartSequenceAcquisition(n, i, s); + if (ret != DEVICE_OK) return ret; + } + return DEVICE_OK; + } + int StartSequenceAcquisition(double i) override { + for (auto* p : physicals) { + int ret = p->StartSequenceAcquisition(i); + if (ret != DEVICE_OK) return ret; + } + return DEVICE_OK; + } + int StopSequenceAcquisition() override { + for (auto* p : physicals) { + int ret = p->StopSequenceAcquisition(); + if (ret != DEVICE_OK) return ret; + } + return DEVICE_OK; + } + bool IsCapturing() override { + for (auto* p : physicals) { + if (p->IsCapturing()) return true; + } + return false; + } +}; From dade0376526b49409f231fc64bdaf92ffa57d886 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Sat, 2 May 2026 18:41:32 +0900 Subject: [PATCH 10/17] Enfoce equal w/h/bpp/ncomp for composite cameras It will fail anyway on InsertImage(), so fail fast. (Assisted by Claude Code; any errors are mine.) --- MMCore/MMCore.cpp | 26 ++++++++ .../MultiChannelSequenceAcquisition-Tests.cpp | 62 +++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index a81b5dd2b..b954642db 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -879,6 +879,11 @@ CMMCore::buildSequenceChannelSnapshot( std::shared_ptr camera) MMCORE_LEGACY_THROW(CMMError) { const unsigned n = camera->GetNumberOfChannels(); + const unsigned expectedW = camera->GetImageWidth(); + const unsigned expectedH = camera->GetImageHeight(); + const unsigned expectedBPP = camera->GetImageBytesPerPixel(); + const unsigned expectedNC = camera->GetNumberOfComponents(); + std::vector channels; channels.reserve(n); for (unsigned i = 0; i < n; ++i) { @@ -916,6 +921,27 @@ CMMCore::buildSequenceChannelSnapshot( "'); nested multi-channel cameras are not supported", MMERR_DEVICE_GENERIC); } + const unsigned pw = physInstance->GetImageWidth(); + const unsigned ph = physInstance->GetImageHeight(); + const unsigned pbpp = physInstance->GetImageBytesPerPixel(); + const unsigned pnc = physInstance->GetNumberOfComponents(); + if (pw != expectedW || ph != expectedH || + pbpp != expectedBPP || pnc != expectedNC) { + throw CMMError( + "Physical camera '" + physInstance->GetLabel() + + "' has image geometry " + + std::to_string(pw) + " x " + std::to_string(ph) + + ", " + std::to_string(pbpp) + " byte(s)/pixel, " + + std::to_string(pnc) + " component(s)" + + ", which differs from composite camera '" + + camera->GetLabel() + "' (" + + std::to_string(expectedW) + " x " + + std::to_string(expectedH) + ", " + + std::to_string(expectedBPP) + " byte(s)/pixel, " + + std::to_string(expectedNC) + " component(s))" + + "; all channels must have identical image geometry", + MMERR_DEVICE_GENERIC); + } } info.physCamLabel = physInstance->GetLabel(); } diff --git a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp index 674df7a01..26e1b740e 100644 --- a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp @@ -1056,3 +1056,65 @@ TEST_CASE("Intrinsic multi-channel device emitting out-of-range " CHECK(cam.InsertTestImage(99) == DEVICE_INCOMPATIBLE_IMAGE); c.stopSequenceAcquisition(); } + +// --- Image geometry validation --- + +TEST_CASE("Composite seq acq rejects mismatched width", + "[MultiChannelSequenceAcquisition]") { + SyncCamera p0("p0"); + SyncCamera p1("p1"); + p1.width = 128; + MockCompositeCamera composite({&p0, &p1}); + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + + CHECK_THROWS_AS(c.startSequenceAcquisition(1, 0.0, true), CMMError); +} + +TEST_CASE("Composite seq acq rejects mismatched height", + "[MultiChannelSequenceAcquisition]") { + SyncCamera p0("p0"); + SyncCamera p1("p1"); + p1.height = 128; + MockCompositeCamera composite({&p0, &p1}); + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + + CHECK_THROWS_AS(c.startSequenceAcquisition(1, 0.0, true), CMMError); +} + +TEST_CASE("Composite seq acq rejects mismatched bytesPerPixel", + "[MultiChannelSequenceAcquisition]") { + SyncCamera p0("p0"); + SyncCamera p1("p1"); + p1.bytesPerPixel = 2; + MockCompositeCamera composite({&p0, &p1}); + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + + CHECK_THROWS_AS(c.startSequenceAcquisition(1, 0.0, true), CMMError); +} + +TEST_CASE("Composite seq acq rejects mismatched numberOfComponents", + "[MultiChannelSequenceAcquisition]") { + SyncCamera p0("p0"); + SyncCamera p1("p1"); + p1.nComponents = 4; + MockCompositeCamera composite({&p0, &p1}); + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + + CHECK_THROWS_AS(c.startSequenceAcquisition(1, 0.0, true), CMMError); +} From e38731dfe4809e599c67d0676ab2da89b98f0f34 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Sun, 3 May 2026 12:26:57 +0900 Subject: [PATCH 11/17] Prohibit sequence buffer init/resize during acq This would always cause problems or errors; now catch upfront. For now, no acquisition can be running because we use a single, global buffer. (Assisted by Claude Code; any errors are mine.) --- MMCore/MMCore.cpp | 22 ++++++ MMCore/MMCore.h | 4 + MMCore/unittest/CircularBuffer-Tests.cpp | 97 ++++++++++++++++++++++++ 3 files changed, 123 insertions(+) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index b954642db..bf063d609 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -977,6 +977,15 @@ void CMMCore::eraseCompletedAcquisition(const std::string& cameraLabel) acquisitions_.erase(cameraLabel); } +bool CMMCore::hasInFlightAcquisitionLocked() const +{ + for (const auto& kv : acquisitions_) { + if (!kv.second->AllParticipantsFinished()) + return true; + } + return false; +} + void CMMCore::markAcquisitionStopRequested(const std::string& cameraLabel) { std::shared_ptr sa; @@ -3353,6 +3362,12 @@ void CMMCore::prepareSequenceAcquisition(const char* label) MMCORE_LEGACY_THROW( */ void CMMCore::initializeCircularBuffer() MMCORE_LEGACY_THROW(CMMError) { + std::lock_guard aqg(acquisitionsMutex_); + if (hasInFlightAcquisitionLocked()) + throw CMMError( + getCoreErrorText(MMERR_NotAllowedDuringSequenceAcquisition).c_str(), + MMERR_NotAllowedDuringSequenceAcquisition); + std::shared_ptr camera = currentCameraDevice_.lock(); if (camera) { @@ -3651,6 +3666,13 @@ void CMMCore::setCircularBufferMemoryFootprint(unsigned sizeMB ///< n megabytes { LOG_DEBUG(coreLogger_) << "Will set circular buffer size to " << sizeMB << " MB"; + + std::lock_guard aqg(acquisitionsMutex_); + if (hasInFlightAcquisitionLocked()) + throw CMMError( + getCoreErrorText(MMERR_NotAllowedDuringSequenceAcquisition).c_str(), + MMERR_NotAllowedDuringSequenceAcquisition); + try { cbuf_ = std::make_unique(sizeMB); diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index b8df6f852..1a94367aa 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -819,6 +819,10 @@ class CMMCore // Idempotent. Takes acquisitionsMutex_. void eraseCompletedAcquisition(const std::string& cameraLabel); + // True iff any entry in acquisitions_ still has at least one participant + // that has not called RecordFinish. Caller must hold acquisitionsMutex_. + bool hasInFlightAcquisitionLocked() const; + void startSequenceAcquisitionImpl( std::shared_ptr camera, bool overwriteData, diff --git a/MMCore/unittest/CircularBuffer-Tests.cpp b/MMCore/unittest/CircularBuffer-Tests.cpp index b7c1fb056..650d5b64d 100644 --- a/MMCore/unittest/CircularBuffer-Tests.cpp +++ b/MMCore/unittest/CircularBuffer-Tests.cpp @@ -465,3 +465,100 @@ TEST_CASE("clearCircularBuffer resets overflow flag", "[CircularBuffer]") { CHECK(c.isBufferOverflowed() == false); c.stopSequenceAcquisition(); } + +// Reinit guard: setCircularBufferMemoryFootprint and initializeCircularBuffer +// must not race with a producer that may call CoreCallback::InsertImage. + +TEST_CASE("setCircularBufferMemoryFootprint throws while sequence running", + "[CircularBuffer]") { + SyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.startSequenceAcquisition(10, 0.0, true); + REQUIRE(c.isSequenceRunning()); + + try { + c.setCircularBufferMemoryFootprint(64); + FAIL("expected CMMError"); + } catch (CMMError& e) { + CHECK(e.getCode() == MMERR_NotAllowedDuringSequenceAcquisition); + } + + c.stopSequenceAcquisition(); +} + +TEST_CASE("initializeCircularBuffer throws while sequence running", + "[CircularBuffer]") { + SyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.startSequenceAcquisition(10, 0.0, true); + REQUIRE(c.isSequenceRunning()); + + try { + c.initializeCircularBuffer(); + FAIL("expected CMMError"); + } catch (CMMError& e) { + CHECK(e.getCode() == MMERR_NotAllowedDuringSequenceAcquisition); + } + + c.stopSequenceAcquisition(); +} + +TEST_CASE("setCircularBufferMemoryFootprint succeeds after explicit stop", + "[CircularBuffer]") { + SyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.startSequenceAcquisition(10, 0.0, true); + c.stopSequenceAcquisition(); + CHECK_NOTHROW(c.setCircularBufferMemoryFootprint(64)); +} + +TEST_CASE("initializeCircularBuffer succeeds after explicit stop", + "[CircularBuffer]") { + SyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.startSequenceAcquisition(10, 0.0, true); + c.stopSequenceAcquisition(); + CHECK_NOTHROW(c.initializeCircularBuffer()); +} + +TEST_CASE("setCircularBufferMemoryFootprint succeeds after camera self-finish " + "without explicit stop", + "[CircularBuffer]") { + SyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.startSequenceAcquisition(10, 0.0, true); + cam.TriggerSelfFinish(); + REQUIRE(c.isSequenceRunning() == false); + // Acquisition entry lingers in acquisitions_ (no stopSequenceAcquisition + // call), but all participants have finished, so reinit must succeed. + CHECK_NOTHROW(c.setCircularBufferMemoryFootprint(64)); +} + +TEST_CASE("initializeCircularBuffer succeeds after camera self-finish " + "without explicit stop", + "[CircularBuffer]") { + SyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.startSequenceAcquisition(10, 0.0, true); + cam.TriggerSelfFinish(); + REQUIRE(c.isSequenceRunning() == false); + CHECK_NOTHROW(c.initializeCircularBuffer()); +} From acd28e67be5cccd768b6f05edd6f09bb646ad6fa Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Mon, 4 May 2026 11:17:47 +0900 Subject: [PATCH 12/17] Prevent deadlock from background PrepareForAcq() If the shutter is in the same adapter as the camera _and_ the camera calls PrepareForAcq() from a thread other than the one calling into StartSequenceAcquisition(), defer the shutter opening so that we don't deadlock. This is probably not an issue today, but would become one if, say, a MultiCamera-like device decides to start physical cameras in parallel (which has its own set of problems if they come from the same adapter, so perhaps is unlikely to happen). Even with this, there will be a deadlock if the camera's StartSequenceAcquisition() waits for PrepareForAcq() to return; that is forbidden and is now documented (there's no reasonable way to work around that situation, and fortunately we don't face it in any known existing camera). (Assisted by Claude Code; any errors are mine.) --- MMCore/CoreCallback.cpp | 27 +- MMCore/MMCore.cpp | 180 +++++++----- MMCore/MMCore.h | 3 + MMCore/SequenceAcquisition.cpp | 12 + MMCore/SequenceAcquisition.h | 4 + MMCore/unittest/SequenceAcquisition-Tests.cpp | 258 ++++++++++++++++++ MMCore/unittest/StubDevices.h | 189 ++++++++++++- MMDevice/MMDevice.h | 13 + 8 files changed, 618 insertions(+), 68 deletions(-) diff --git a/MMCore/CoreCallback.cpp b/MMCore/CoreCallback.cpp index fb1a9527d..e52b04ad0 100644 --- a/MMCore/CoreCallback.cpp +++ b/MMCore/CoreCallback.cpp @@ -504,11 +504,36 @@ int CoreCallback::PrepareForAcq(const MM::Device* caller) core_->currentShutterDevice_.lock(); if (shutter) { - int sret; + int sret = DEVICE_ERR; + bool deferred = false; + if (shutter->GetAdapterModule() == + sa->Camera()->GetAdapterModule()) + { + // Shutter is in the same adapter module as the primary camera. + // startSequenceAcquisitionImpl() holds that module's lock; if + // PrepareForAcq is being invoked from a thread other than that + // caller, a blocking lock here would deadlock or contend + // unnecessarily. Use try_lock: same-thread re-entry on the + // recursive_mutex succeeds and we open inline; otherwise defer + // to startSequenceAcquisitionImpl(), which will open after + // releasing the module lock. + auto& mtx = shutter->GetAdapterModule()->GetLock(); + if (mtx.try_lock()) { + std::lock_guard g(mtx, std::adopt_lock); + sret = shutter->SetOpen(true); + } else { + sa->DeferShutterOpen(); + deferred = true; + } + } + else { DeviceModuleLockGuard g(shutter); sret = shutter->SetOpen(true); } + if (deferred) { + return sa->WaitForShutterOpened() ? DEVICE_OK : DEVICE_ERR; + } if (sret == DEVICE_OK) { core_->postNotification(notif::ShutterOpenChanged{ shutter->GetLabel(), true}); diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index bf063d609..e505e7d26 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -1020,6 +1020,36 @@ void CMMCore::closeDeferredAutoShutter() shutter->GetLabel(), false}); } +void CMMCore::openDeferredAutoShutter( + const std::shared_ptr& sa) +{ + bool openOk = true; + if (autoShutter_) + { + std::shared_ptr shutter = + currentShutterDevice_.lock(); + if (shutter) + { + int sret; + { + mmi::DeviceModuleLockGuard g(shutter); + sret = shutter->SetOpen(true); + } + if (sret == DEVICE_OK) { + postNotification(notif::ShutterOpenChanged{ + shutter->GetLabel(), true}); + waitForDevice(shutter); + } else { + openOk = false; + } + } + } + if (openOk) + postNotification( + notif::SequenceAcquisitionStarted{sa->CameraLabel()}); + sa->FinishShutterOpen(openOk); +} + void CMMCore::stopAndClearAllSequenceAcquisitions() { std::map startDevice) { - mmi::DeviceModuleLockGuard guard(camera); - if (camera->IsCapturing()) { - throw CMMError( - getCoreErrorText(MMERR_NotAllowedDuringSequenceAcquisition).c_str(), - MMERR_NotAllowedDuringSequenceAcquisition); - } - - if (!cbuf_->Initialize( - static_cast(camera->GetImageWidth()) * - camera->GetImageHeight() * - camera->GetImageBytesPerPixel())) - { - logError(getDeviceName(camera).c_str(), - getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str()); - throw CMMError( - getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str(), - MMERR_CircularBufferFailedToInitialize); - } - cbuf_->Clear(); - callback_->ResetImageInsertionState(); - cbuf_->SetOverwriteData(overwriteData); - - auto channels = buildSequenceChannelSnapshot(camera); - auto newSa = mmcore::internal::SequenceAcquisition::Create( - camera, std::move(channels)); + std::shared_ptr sa; { - std::lock_guard aqg(acquisitionsMutex_); + mmi::DeviceModuleLockGuard guard(camera); + if (camera->IsCapturing()) { + throw CMMError( + getCoreErrorText(MMERR_NotAllowedDuringSequenceAcquisition).c_str(), + MMERR_NotAllowedDuringSequenceAcquisition); + } + if (!cbuf_->Initialize( + static_cast(camera->GetImageWidth()) * + camera->GetImageHeight() * + camera->GetImageBytesPerPixel())) { - auto it = acquisitions_.find(camera->GetLabel()); - if (it != acquisitions_.end() && - !it->second->AllParticipantsFinished()) { - throw CMMError( - "Sequence acquisition on '" + camera->GetLabel() + - "' is already running", - MMERR_NotAllowedDuringSequenceAcquisition); - } + logError(getDeviceName(camera).c_str(), + getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str()); + throw CMMError( + getCoreErrorText(MMERR_CircularBufferFailedToInitialize).c_str(), + MMERR_CircularBufferFailedToInitialize); } + cbuf_->Clear(); + callback_->ResetImageInsertionState(); + cbuf_->SetOverwriteData(overwriteData); - for (const auto& [existingLabel, existingSa] : acquisitions_) { - if (existingSa->AllParticipantsFinished()) - continue; - if (existingSa->HasParticipant(camera->GetRawPtr())) { - throw CMMError( - "Camera '" + camera->GetLabel() + - "' is already participating in a sequence acquisition on '" + - existingLabel + "'", - MMERR_NotAllowedDuringSequenceAcquisition); + auto channels = buildSequenceChannelSnapshot(camera); + auto newSa = mmcore::internal::SequenceAcquisition::Create( + camera, std::move(channels)); + { + std::lock_guard aqg(acquisitionsMutex_); + + { + auto it = acquisitions_.find(camera->GetLabel()); + if (it != acquisitions_.end() && + !it->second->AllParticipantsFinished()) { + throw CMMError( + "Sequence acquisition on '" + camera->GetLabel() + + "' is already running", + MMERR_NotAllowedDuringSequenceAcquisition); + } } - for (const auto& ch : newSa->Channels()) { - if (ch.physCamDevice && - existingSa->HasParticipant(ch.physCamDevice)) { + + for (const auto& [existingLabel, existingSa] : acquisitions_) { + if (existingSa->AllParticipantsFinished()) + continue; + if (existingSa->HasParticipant(camera->GetRawPtr())) { throw CMMError( - "Camera '" + ch.physCamLabel + + "Camera '" + camera->GetLabel() + "' is already participating in a sequence acquisition on '" + existingLabel + "'", MMERR_NotAllowedDuringSequenceAcquisition); } + for (const auto& ch : newSa->Channels()) { + if (ch.physCamDevice && + existingSa->HasParticipant(ch.physCamDevice)) { + throw CMMError( + "Camera '" + ch.physCamLabel + + "' is already participating in a sequence acquisition on '" + + existingLabel + "'", + MMERR_NotAllowedDuringSequenceAcquisition); + } + } } - } - acquisitions_[camera->GetLabel()] = std::move(newSa); - } + sa = newSa; + acquisitions_[camera->GetLabel()] = std::move(newSa); + } - int nRet = startDevice(); - if (nRet != DEVICE_OK) { - std::shared_ptr sa; - { - std::lock_guard aqg(acquisitionsMutex_); - auto it = acquisitions_.find(camera->GetLabel()); - if (it != acquisitions_.end()) { - sa = std::move(it->second); - acquisitions_.erase(it); + int nRet = startDevice(); + if (nRet != DEVICE_OK) { + std::shared_ptr failedSa; + { + std::lock_guard aqg(acquisitionsMutex_); + auto it = acquisitions_.find(camera->GetLabel()); + if (it != acquisitions_.end()) { + failedSa = std::move(it->second); + acquisitions_.erase(it); + } } + if (failedSa) { + // If a participant deferred the shutter open and parked on the + // CV, the deferred open will not be executed; release the + // parked waiters with OpenFailed. + if (failedSa->TakeDeferredShutterOpen()) + failedSa->FinishShutterOpen(false); + if (failedSa->NeedsStartRollback()) { + closeDeferredAutoShutter(); + postNotification( + notif::SequenceAcquisitionStopped{failedSa->CameraLabel()}); + } + } + throw CMMError(getDeviceErrorText(nRet, camera).c_str(), + MMERR_DEVICE_GENERIC); } - if (sa && sa->NeedsStartRollback()) { - closeDeferredAutoShutter(); - postNotification( - notif::SequenceAcquisitionStopped{sa->CameraLabel()}); - } - throw CMMError(getDeviceErrorText(nRet, camera).c_str(), - MMERR_DEVICE_GENERIC); } + + // Camera adapter module lock is released here. If a participant's + // PrepareForAcq deferred the shutter open (because it ran on a thread + // other than this one and could not acquire the shared module lock), + // execute it now. + if (sa && sa->TakeDeferredShutterOpen()) + openDeferredAutoShutter(sa); } /** diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index 1a94367aa..6241ee49d 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -832,6 +832,9 @@ class CMMCore void closeDeferredAutoShutter(); + void openDeferredAutoShutter( + const std::shared_ptr& sa); + void setCameraInternal(const std::string& label); void setShutterInternal(const std::string& label); void setFocusInternal(const std::string& label); diff --git a/MMCore/SequenceAcquisition.cpp b/MMCore/SequenceAcquisition.cpp index cba27bcee..b10a24bcb 100644 --- a/MMCore/SequenceAcquisition.cpp +++ b/MMCore/SequenceAcquisition.cpp @@ -204,5 +204,17 @@ bool SequenceAcquisition::TakeDeferredShutterClose() return std::exchange(shutterCloseDeferred_, false); } +void SequenceAcquisition::DeferShutterOpen() +{ + std::lock_guard g(mu_); + shutterOpenDeferred_ = true; +} + +bool SequenceAcquisition::TakeDeferredShutterOpen() +{ + std::lock_guard g(mu_); + return std::exchange(shutterOpenDeferred_, false); +} + } // namespace internal } // namespace mmcore diff --git a/MMCore/SequenceAcquisition.h b/MMCore/SequenceAcquisition.h index 5d6bfe64c..2703bc1f4 100644 --- a/MMCore/SequenceAcquisition.h +++ b/MMCore/SequenceAcquisition.h @@ -131,6 +131,9 @@ class SequenceAcquisition { void DeferShutterClose(); bool TakeDeferredShutterClose(); + void DeferShutterOpen(); + bool TakeDeferredShutterOpen(); + private: SequenceAcquisition(std::shared_ptr camera, std::vector channels); @@ -147,6 +150,7 @@ class SequenceAcquisition { ShutterState shutterState_ = ShutterState::NotOpened; bool stopRequested_ = false; bool shutterCloseDeferred_ = false; + bool shutterOpenDeferred_ = false; std::set readyParticipants_; std::set finishedParticipants_; }; diff --git a/MMCore/unittest/SequenceAcquisition-Tests.cpp b/MMCore/unittest/SequenceAcquisition-Tests.cpp index 2fb3db89b..f4c799fdc 100644 --- a/MMCore/unittest/SequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/SequenceAcquisition-Tests.cpp @@ -588,3 +588,261 @@ TEST_CASE("startSequenceAcquisition after camera self-finish without " CHECK(c.popNextImage() != nullptr); c.stopSequenceAcquisition(); } + +// --- Open-side autoshutter: inline (synchronous) PrepareForAcq --- + +TEST_CASE("Inline open: shutter opens before startSequenceAcquisition returns " + "(same adapter as camera)", + "[SequenceAcquisition][Autoshutter]") { + SyncCamera cam; + StubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + REQUIRE(shutter.open == false); + + c.startSequenceAcquisition(10, 0.0, true); + CHECK(shutter.open == true); + CHECK(shutter.setOpenTrueCount == 1); + + c.stopSequenceAcquisition(); + CHECK(shutter.open == false); +} + +TEST_CASE("Inline open: shutter opens when shutter is in a different adapter", + "[SequenceAcquisition][Autoshutter]") { + SyncCamera cam; + StubShutter shutter; + MockAdapterWithDevices camAdapter{"cam_adapter", {{"cam", &cam}}}; + MockAdapterWithDevices shutterAdapter{"shutter_adapter", + {{"shutter", &shutter}}}; + CMMCore c; + camAdapter.LoadIntoCore(c); + shutterAdapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(10, 0.0, true); + CHECK(shutter.open == true); + CHECK(shutter.setOpenTrueCount == 1); + c.stopSequenceAcquisition(); + CHECK(shutter.open == false); +} + +// --- Open-side autoshutter: deferred (async) PrepareForAcq --- + +TEST_CASE("Deferred open: shutter opens before worker's PrepareForAcq returns " + "(same adapter as camera)", + "[SequenceAcquisition][Autoshutter]") { + WorkerThreadCamera cam; + StubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + REQUIRE(shutter.open == false); + + c.startSequenceAcquisition(10, 0.0, true); + // After startSequenceAcquisition returns, the deferred open must have + // executed and the shutter is open. + CHECK(shutter.open == true); + CHECK(shutter.setOpenTrueCount == 1); + + // The worker thread's PrepareForAcq returned with DEVICE_OK after the + // shutter was opened. + REQUIRE(cam.WaitForPrepareReturned()); + CHECK(cam.PrepareReturnValue() == DEVICE_OK); + + c.stopSequenceAcquisition(); +} + +TEST_CASE("Deferred open: ShutterOpenChanged + SequenceAcquisitionStarted " + "fire exactly once", + "[SequenceAcquisition][Autoshutter]") { + WorkerThreadCamera cam; + StubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(10, 0.0, true); + REQUIRE(cam.WaitForPrepareReturned()); + CHECK(shutter.setOpenTrueCount == 1); + c.stopSequenceAcquisition(); + CHECK(shutter.setOpenTrueCount == 1); +} + +// --- Composite autoshutter --- + +TEST_CASE("Composite (sync physicals): only the FirstOpener calls SetOpen(true)", + "[SequenceAcquisition][Autoshutter]") { + SyncCamera p1("p1"); + SyncCamera p2("p2"); + MockCompositeCamera composite({&p1, &p2}); + StubShutter shutter; + MockAdapterWithDevices adapter{ + {"p1", &p1}, {"p2", &p2}, {"composite", &composite}, + {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(10, 0.0, true); + CHECK(shutter.open == true); + CHECK(shutter.setOpenTrueCount == 1); + c.stopSequenceAcquisition("composite"); + CHECK(shutter.open == false); +} + +TEST_CASE("Composite (async physicals): exactly one SetOpen(true), all " + "participants released", + "[SequenceAcquisition][Autoshutter]") { + WorkerThreadCamera p1("p1"); + WorkerThreadCamera p2("p2"); + MockCompositeCamera composite({&p1, &p2}); + StubShutter shutter; + MockAdapterWithDevices adapter{ + {"p1", &p1}, {"p2", &p2}, {"composite", &composite}, + {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(10, 0.0, true); + REQUIRE(p1.WaitForPrepareReturned()); + REQUIRE(p2.WaitForPrepareReturned()); + CHECK(p1.PrepareReturnValue() == DEVICE_OK); + CHECK(p2.PrepareReturnValue() == DEVICE_OK); + CHECK(shutter.open == true); + CHECK(shutter.setOpenTrueCount == 1); + + c.stopSequenceAcquisition("composite"); +} + +TEST_CASE("Composite (mixed sync + async): inline FirstOpener wins, async " + "sibling sees AlreadyOpened", + "[SequenceAcquisition][Autoshutter]") { + SyncCamera pSync("pSync"); + WorkerThreadCamera pAsync("pAsync"); + // Composite starts physicals in order; pSync first ensures inline + // FirstOpener opens the shutter on the calling thread before pAsync's + // worker reaches PrepareForAcq. + MockCompositeCamera composite({&pSync, &pAsync}); + StubShutter shutter; + MockAdapterWithDevices adapter{ + {"pSync", &pSync}, {"pAsync", &pAsync}, + {"composite", &composite}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(10, 0.0, true); + REQUIRE(pAsync.WaitForPrepareReturned()); + CHECK(pAsync.PrepareReturnValue() == DEVICE_OK); + CHECK(shutter.open == true); + CHECK(shutter.setOpenTrueCount == 1); + + c.stopSequenceAcquisition("composite"); +} + +// --- Failure paths --- + +TEST_CASE("Inline open: SetOpen failure propagates as start error", + "[SequenceAcquisition][Autoshutter]") { + SyncCamera cam; + StubShutter shutter; + shutter.setOpenTrueReturnValue = DEVICE_ERR; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + CHECK_THROWS_AS(c.startSequenceAcquisition(10, 0.0, true), CMMError); + CHECK(shutter.open == false); + CHECK(shutter.setOpenTrueCount == 1); + CHECK(shutter.setOpenFalseCount == 0); +} + +TEST_CASE("Deferred open: startDevice failure releases parked worker without " + "opening shutter", + "[SequenceAcquisition][Autoshutter]") { + WorkerThreadCamera cam; + cam.startReturnValue = DEVICE_ERR; + cam.waitForPrepareCalledBeforeStartReturns = true; + // Give the worker time to enter PrepareForAcq, defer, and park on the CV + // before StartSequenceAcquisition returns failure. + cam.extraSleepBeforeStartReturns = std::chrono::milliseconds(50); + StubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + CHECK_THROWS_AS(c.startSequenceAcquisition(10, 0.0, true), CMMError); + // Worker must have been released; PrepareForAcq returned DEVICE_ERR. + REQUIRE(cam.WaitForPrepareReturned()); + CHECK(cam.PrepareReturnValue() == DEVICE_ERR); + // Shutter never opened, so neither SetOpen(true) nor SetOpen(false) ran. + CHECK(shutter.open == false); + CHECK(shutter.setOpenTrueCount == 0); + CHECK(shutter.setOpenFalseCount == 0); + + // Drive the worker thread to exit. cam dtor would also do this, but be + // explicit. + cam.StopSequenceAcquisition(); +} + +// --- Negative cases (no autoshutter, no shutter) --- + +TEST_CASE("Deferred-open machinery is inert when autoShutter is off", + "[SequenceAcquisition][Autoshutter]") { + WorkerThreadCamera cam; + StubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(false); + + c.startSequenceAcquisition(10, 0.0, true); + REQUIRE(cam.WaitForPrepareReturned()); + CHECK(cam.PrepareReturnValue() == DEVICE_OK); + CHECK(shutter.open == false); + CHECK(shutter.setOpenTrueCount == 0); + c.stopSequenceAcquisition(); +} + +TEST_CASE("Deferred-open machinery is inert when no shutter is set", + "[SequenceAcquisition][Autoshutter]") { + WorkerThreadCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(10, 0.0, true); + REQUIRE(cam.WaitForPrepareReturned()); + CHECK(cam.PrepareReturnValue() == DEVICE_OK); + c.stopSequenceAcquisition(); +} diff --git a/MMCore/unittest/StubDevices.h b/MMCore/unittest/StubDevices.h index 655a6e467..e3bb87eb3 100644 --- a/MMCore/unittest/StubDevices.h +++ b/MMCore/unittest/StubDevices.h @@ -7,7 +7,9 @@ #include "CameraImageMetadata.h" #include "DeviceBase.h" +#include #include +#include #include #include #include @@ -351,6 +353,178 @@ struct AsyncCamera : CCameraBase { std::vector imgBuf_; }; +// A camera whose StartSequenceAcquisition spawns a worker thread that calls +// PrepareForAcq on that thread (rather than synchronously on the calling +// thread), then waits for stop. Worker calls AcqFinished before exiting. +// +// Test knobs let the StartSequenceAcquisition caller wait until the worker +// has reached the PrepareForAcq call site (and optionally sleep a small +// extra interval to let it enter the deferred-open wait), and let it return +// a configurable status code instead of DEVICE_OK. This is used to exercise +// the open-side deferred-open + rollback paths. +struct WorkerThreadCamera : CCameraBase { + std::string name; + unsigned width = 64; + unsigned height = 64; + unsigned bytesPerPixel = 1; + unsigned nComponents = 1; + unsigned bitDepth = 8; + int binning = 1; + double exposure = 10.0; + + // Test knobs (set before startSequenceAcquisition). Defaults cause the + // calling thread to wait until the worker has called PrepareForAcq, plus + // a small extra delay so the worker reliably reaches the deferred-open + // wait inside PrepareForAcq before StartSequenceAcquisition returns. + // (The "promise set" signal fires just *before* the call into + // PrepareForAcq, not after the deferred state is materialized; the + // extra sleep covers the inherent gap.) + int startReturnValue = DEVICE_OK; + bool waitForPrepareCalledBeforeStartReturns = true; + std::chrono::milliseconds extraSleepBeforeStartReturns{50}; + + explicit WorkerThreadCamera(std::string n = "WorkerThreadCamera") + : name(std::move(n)) {} + + ~WorkerThreadCamera() override { + { + std::lock_guard lk(mu_); + stopRequested_ = true; + } + cv_.notify_all(); + if (thread_.joinable()) + thread_.join(); + } + + int Initialize() override { return DEVICE_OK; } + int Shutdown() override { return DEVICE_OK; } + bool Busy() override { return false; } + void GetName(char* buf) const override { + CDeviceUtils::CopyLimitedString(buf, name.c_str()); + } + + int SnapImage() override { + imgBuf_.assign( + static_cast(width) * height * bytesPerPixel, 0); + return DEVICE_OK; + } + const unsigned char* GetImageBuffer() override { return imgBuf_.data(); } + long GetImageBufferSize() const override { + return static_cast(width) * height * bytesPerPixel; + } + unsigned GetImageWidth() const override { return width; } + unsigned GetImageHeight() const override { return height; } + unsigned GetImageBytesPerPixel() const override { return bytesPerPixel; } + unsigned GetNumberOfComponents() const override { return nComponents; } + unsigned GetBitDepth() const override { return bitDepth; } + int GetBinning() const override { return binning; } + int SetBinning(int b) override { binning = b; return DEVICE_OK; } + void SetExposure(double e) override { exposure = e; } + double GetExposure() const override { return exposure; } + int SetROI(unsigned, unsigned, unsigned, unsigned) override { + return DEVICE_OK; + } + int GetROI(unsigned& x, unsigned& y, unsigned& w, unsigned& h) override { + x = 0; y = 0; w = width; h = height; + return DEVICE_OK; + } + int ClearROI() override { return DEVICE_OK; } + int IsExposureSequenceable(bool& seq) const override { + seq = false; + return DEVICE_OK; + } + + int StartSequenceAcquisition(long, double, bool) override { + return SpawnWorker(); + } + int StartSequenceAcquisition(double) override { + return SpawnWorker(); + } + + int StopSequenceAcquisition() override { + { + std::lock_guard lk(mu_); + stopRequested_ = true; + } + cv_.notify_all(); + if (thread_.joinable()) + thread_.join(); + return DEVICE_OK; + } + + bool IsCapturing() override { + std::lock_guard lk(mu_); + return running_; + } + + // Returns once the worker thread has returned from PrepareForAcq (i.e., + // the shutter is open or the open path has terminated with failure). + bool WaitForPrepareReturned( + std::chrono::milliseconds timeout = + std::chrono::milliseconds(5000)) { + std::unique_lock lk(mu_); + return prepareReturnedCv_.wait_for(lk, timeout, + [this] { return prepareReturned_; }); + } + int PrepareReturnValue() { + std::lock_guard lk(mu_); + return prepareReturnValue_; + } + +private: + int SpawnWorker() { + std::promise reachedPromise; + auto reachedFut = reachedPromise.get_future(); + { + std::lock_guard lk(mu_); + running_ = true; + stopRequested_ = false; + prepareReturned_ = false; + prepareReturnValue_ = DEVICE_OK; + prepareCalledPromise_ = std::move(reachedPromise); + } + thread_ = std::thread([this] { WorkerLoop(); }); + if (waitForPrepareCalledBeforeStartReturns) + reachedFut.wait(); + if (extraSleepBeforeStartReturns.count() > 0) + std::this_thread::sleep_for(extraSleepBeforeStartReturns); + return startReturnValue; + } + + void WorkerLoop() { + // Signal that the worker is about to enter PrepareForAcq. + prepareCalledPromise_.set_value(); + int ret = GetCoreCallback()->PrepareForAcq(this); + { + std::lock_guard lk(mu_); + prepareReturnValue_ = ret; + prepareReturned_ = true; + } + prepareReturnedCv_.notify_all(); + // Wait for stop request (do not produce images by default). + { + std::unique_lock lk(mu_); + cv_.wait(lk, [this] { return stopRequested_; }); + } + GetCoreCallback()->AcqFinished(this, 0); + { + std::lock_guard lk(mu_); + running_ = false; + } + } + + bool running_ = false; + bool stopRequested_ = false; + bool prepareReturned_ = false; + int prepareReturnValue_ = DEVICE_OK; + std::mutex mu_; + std::condition_variable cv_; + std::condition_variable prepareReturnedCv_; + std::promise prepareCalledPromise_; + std::thread thread_; + std::vector imgBuf_; +}; + struct StubStage : CStageBase { std::string name = "StubStage"; using CStageBase::OnStagePositionChanged; @@ -467,6 +641,9 @@ struct StubShutter : CShutterBase { std::string name = "StubShutter"; using CShutterBase::GetCoreCallback; // No OnShutterOpenChanged on device side bool open = false; + int setOpenTrueCount = 0; + int setOpenFalseCount = 0; + int setOpenTrueReturnValue = DEVICE_OK; int Initialize() override { return DEVICE_OK; } int Shutdown() override { return DEVICE_OK; } @@ -475,7 +652,17 @@ struct StubShutter : CShutterBase { CDeviceUtils::CopyLimitedString(buf, name.c_str()); } - int SetOpen(bool o) override { open = o; return DEVICE_OK; } + int SetOpen(bool o) override { + if (o) { + ++setOpenTrueCount; + if (setOpenTrueReturnValue != DEVICE_OK) + return setOpenTrueReturnValue; + } else { + ++setOpenFalseCount; + } + open = o; + return DEVICE_OK; + } int GetOpen(bool& o) override { o = open; return DEVICE_OK; } int Fire(double) override { return DEVICE_OK; } }; diff --git a/MMDevice/MMDevice.h b/MMDevice/MMDevice.h index c69be62dd..b53ec4cef 100644 --- a/MMDevice/MMDevice.h +++ b/MMDevice/MMDevice.h @@ -1732,6 +1732,19 @@ namespace MM { * `PrepareForAcq()` independently. The primary camera calls only if * it has at least one intrinsic channel. * + * Threading: `PrepareForAcq()` may be called either synchronously on + * the thread that called `StartSequenceAcquisition()`, or + * asynchronously from a worker thread spawned by the camera adapter. + * If it is called from a worker thread, then + * `StartSequenceAcquisition()` must not block synchronously waiting + * for that thread's `PrepareForAcq()` to return — otherwise deadlock + * is possible (the Core may need the calling thread of + * `StartSequenceAcquisition()` to release its adapter module lock + * before the asynchronous `PrepareForAcq()` can complete). This is + * the open-side analogue of the requirement that `AcqFinished()` be + * called from the worker thread, not from `StopSequenceAcquisition()` + * itself. + * * @param caller The calling device (pass `this`). */ virtual int PrepareForAcq(const Device* caller) = 0; From 9eee9930e068d7390c8940ede2fcbedfe27f3638 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Mon, 4 May 2026 12:06:44 +0900 Subject: [PATCH 13/17] Fix exception paths for autoshutter opening Avoid waiting forever when shutter opening/waiting failed. Also, avoid leaking exceptions back to caller of PrepareForAcq(). (Assisted by Claude Code; any errors are mine.) --- MMCore/CoreCallback.cpp | 103 +++++++++++++++++++++++----------------- MMCore/MMCore.cpp | 10 ++++ 2 files changed, 70 insertions(+), 43 deletions(-) diff --git a/MMCore/CoreCallback.cpp b/MMCore/CoreCallback.cpp index e52b04ad0..d3457c376 100644 --- a/MMCore/CoreCallback.cpp +++ b/MMCore/CoreCallback.cpp @@ -497,59 +497,76 @@ int CoreCallback::PrepareForAcq(const MM::Device* caller) break; } - bool openOk = true; - if (core_->autoShutter_) - { - std::shared_ptr shutter = - core_->currentShutterDevice_.lock(); - if (shutter) + // Exceptions must not escape back into the adapter that called this + // callback. The OpenerGuard releases parked CV waiters on the throw path. + try { + struct OpenerGuard { + SequenceAcquisition* sa; + bool finished = false; + ~OpenerGuard() { if (!finished) sa->FinishShutterOpen(false); } + } guard{ sa.get() }; + + bool openOk = true; + if (core_->autoShutter_) { - int sret = DEVICE_ERR; - bool deferred = false; - if (shutter->GetAdapterModule() == - sa->Camera()->GetAdapterModule()) + std::shared_ptr shutter = + core_->currentShutterDevice_.lock(); + if (shutter) { - // Shutter is in the same adapter module as the primary camera. - // startSequenceAcquisitionImpl() holds that module's lock; if - // PrepareForAcq is being invoked from a thread other than that - // caller, a blocking lock here would deadlock or contend - // unnecessarily. Use try_lock: same-thread re-entry on the - // recursive_mutex succeeds and we open inline; otherwise defer - // to startSequenceAcquisitionImpl(), which will open after - // releasing the module lock. - auto& mtx = shutter->GetAdapterModule()->GetLock(); - if (mtx.try_lock()) { - std::lock_guard g(mtx, std::adopt_lock); + int sret = DEVICE_ERR; + bool deferred = false; + if (shutter->GetAdapterModule() == + sa->Camera()->GetAdapterModule()) + { + // Shutter is in the same adapter module as the primary camera. + // startSequenceAcquisitionImpl() holds that module's lock; if + // PrepareForAcq is being invoked from a thread other than that + // caller, a blocking lock here would deadlock or contend + // unnecessarily. Use try_lock: same-thread re-entry on the + // recursive_mutex succeeds and we open inline; otherwise defer + // to startSequenceAcquisitionImpl(), which will open after + // releasing the module lock. + auto& mtx = shutter->GetAdapterModule()->GetLock(); + if (mtx.try_lock()) { + std::lock_guard g(mtx, std::adopt_lock); + sret = shutter->SetOpen(true); + } else { + sa->DeferShutterOpen(); + deferred = true; + } + } + else + { + DeviceModuleLockGuard g(shutter); sret = shutter->SetOpen(true); + } + if (deferred) { + // Opener role transferred to the deferred opener; that thread + // now owns the FinishShutterOpen contract. + guard.finished = true; + return sa->WaitForShutterOpened() ? DEVICE_OK : DEVICE_ERR; + } + if (sret == DEVICE_OK) { + core_->postNotification(notif::ShutterOpenChanged{ + shutter->GetLabel(), true}); + core_->waitForDevice(shutter); } else { - sa->DeferShutterOpen(); - deferred = true; + openOk = false; } } - else - { - DeviceModuleLockGuard g(shutter); - sret = shutter->SetOpen(true); - } - if (deferred) { - return sa->WaitForShutterOpened() ? DEVICE_OK : DEVICE_ERR; - } - if (sret == DEVICE_OK) { - core_->postNotification(notif::ShutterOpenChanged{ - shutter->GetLabel(), true}); - core_->waitForDevice(shutter); - } else { - openOk = false; - } } - } - if (openOk) - core_->postNotification(notif::SequenceAcquisitionStarted{sa->CameraLabel()}); + if (openOk) + core_->postNotification(notif::SequenceAcquisitionStarted{sa->CameraLabel()}); - sa->FinishShutterOpen(openOk); + sa->FinishShutterOpen(openOk); + guard.finished = true; - return openOk ? DEVICE_OK : DEVICE_ERR; + return openOk ? DEVICE_OK : DEVICE_ERR; + } catch (CMMError& e) { + core_->logError("PrepareForAcq", e.getMsg().c_str()); + return DEVICE_ERR; + } } /** diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index e505e7d26..7b9bc75ea 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -1023,6 +1023,15 @@ void CMMCore::closeDeferredAutoShutter() void CMMCore::openDeferredAutoShutter( const std::shared_ptr& sa) { + // Release CV waiters with OpenFailed if we exit via exception before + // reaching the explicit FinishShutterOpen below. WaitForShutterOpened + // is a bare cv.wait by design, so the opener must always finish. + struct OpenerGuard { + mmi::SequenceAcquisition* sa; + bool finished = false; + ~OpenerGuard() { if (!finished) sa->FinishShutterOpen(false); } + } guard{ sa.get() }; + bool openOk = true; if (autoShutter_) { @@ -1048,6 +1057,7 @@ void CMMCore::openDeferredAutoShutter( postNotification( notif::SequenceAcquisitionStarted{sa->CameraLabel()}); sa->FinishShutterOpen(openOk); + guard.finished = true; } void CMMCore::stopAndClearAllSequenceAcquisitions() From 6a5b3d867879daaebce0c7d65c66c15cecdb587d Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Wed, 6 May 2026 14:58:03 +0900 Subject: [PATCH 14/17] Fix deferred shutter closing on camera self-finish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the autoshutter is in the same adapter module as the camera and AcqFinished's try_lock on the module lock fails, the old code unconditionally set shutterCloseDeferred_ and relied on stopSequenceAcquisition to drain it. That broke natural completion — finite-frame acquisitions that complete without anyone calling stopSequenceAcquisition would leave the autoshutter open forever, because the deferred-close was only drained from the stop path. The new design splits that path atomically (under SequenceAcquisition::mu_): - If stopRequested_ is set → defer (stopper will drain via TakeDeferredShutterClose). - Otherwise → spawn a worker thread that takes the module lock blockingly and closes the shutter itself, then posts ShutterOpenChanged. The thread is owned by the SA (shutterCloseWorker_) and joined either by stopSequenceAcquisition or by ~SequenceAcquisition. To make the atomicity work, CMMCore::stopSequenceAcquisition now calls MarkStopRequested() before taking the module lock. That ordering is important: it guarantees that any AcqFinished racing with the stopper observes stopRequested_=true and takes the defer branch instead of spawning a worker the stopper would have to chase. (Assisted by Claude Code; any errors are mine.) --- MMCore/CoreCallback.cpp | 45 ++++- MMCore/MMCore.cpp | 67 +++++--- MMCore/MMCore.h | 2 - MMCore/SequenceAcquisition.cpp | 49 ++++-- MMCore/SequenceAcquisition.h | 41 ++++- MMCore/unittest/SequenceAcquisition-Tests.cpp | 92 +++++++++++ MMCore/unittest/StubDevices.h | 156 +++++++++++++++++- 7 files changed, 403 insertions(+), 49 deletions(-) diff --git a/MMCore/CoreCallback.cpp b/MMCore/CoreCallback.cpp index d3457c376..2693ee140 100644 --- a/MMCore/CoreCallback.cpp +++ b/MMCore/CoreCallback.cpp @@ -40,6 +40,9 @@ #include #include #include +#include +#include +#include #include #include @@ -433,15 +436,49 @@ int CoreCallback::AcqFinished(const MM::Device* caller, int /*statusCode*/) // stopSequenceAcquisition() may be holding that module's lock // (on a different thread, waiting for this thread to exit via // join), so a blocking lock would deadlock. Use try_lock: if - // the lock is free, close immediately; otherwise defer to - // stopSequenceAcquisition(), which will close after releasing - // the module lock. + // the lock is free, close immediately; otherwise atomically + // either defer to an in-flight stopSequenceAcquisition or + // spawn a worker that takes the lock blockingly. auto& mtx = shutter->GetAdapterModule()->GetLock(); if (mtx.try_lock()) { std::lock_guard g(mtx, std::adopt_lock); sret = shutter->SetOpen(false); } else { - sa->DeferShutterClose(); + // Same-module camera adapter lock is held by another thread + // (typically stopSequenceAcquisition mid-execution, or an + // unrelated caller such as isSequenceRunning). Atomically: + // if stop has been requested, record a deferred-close for + // the in-flight stop to pick up via TakeDeferredShutterClose; + // otherwise spawn a worker that takes the module lock + // blockingly and closes the shutter. + try { + CMMCore* core = core_; + sa->SpawnOrDeferShutterClose([core, shutter]() { + return std::thread([core, shutter] { + int wret; + { + DeviceModuleLockGuard g(shutter); + wret = shutter->SetOpen(false); + } + if (wret == DEVICE_OK) { + core->postNotification(notif::ShutterOpenChanged{ + shutter->GetLabel(), false}); + } else { + LOG_ERROR(core->coreLogger_) << + "Deferred autoshutter close worker: " + "SetOpen(false) on '" << shutter->GetLabel() + << "' returned " << wret; + } + }); + }); + } catch (const std::system_error&) { + LOG_ERROR(core_->coreLogger_) << + "Failed to spawn deferred autoshutter close worker for '" + << sa->CameraLabel() << + "'; shutter close deferred to next stop"; + // SpawnOrDeferShutterClose has already set + // shutterCloseDeferred_. + } } } else diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index 7b9bc75ea..c9d133d44 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -986,22 +986,6 @@ bool CMMCore::hasInFlightAcquisitionLocked() const return false; } -void CMMCore::markAcquisitionStopRequested(const std::string& cameraLabel) -{ - std::shared_ptr sa; - { - std::lock_guard aqg(acquisitionsMutex_); - auto it = acquisitions_.find(cameraLabel); - if (it != acquisitions_.end()) - sa = it->second; - } - if (sa) { - const bool causedComplete = sa->MarkStopRequested(); - if (causedComplete) - eraseCompletedAcquisition(sa->CameraLabel()); - } -} - void CMMCore::closeDeferredAutoShutter() { if (!autoShutter_) @@ -1069,10 +1053,21 @@ void CMMCore::stopAndClearAllSequenceAcquisitions() toStop.swap(acquisitions_); } for (auto& kv : toStop) { - auto& cam = kv.second->Camera(); - mmi::DeviceModuleLockGuard guard(cam); - if (cam->IsCapturing()) - cam->StopSequenceAcquisition(); + auto& sa = kv.second; + // Same ordering rationale as stopSequenceAcquisition: set + // stopRequested_ before taking the camera adapter module lock so a + // concurrent AcqFinished takes the deferred-close path (drained + // below) rather than spawning a worker we would race to join. + (void)sa->MarkStopRequested(); + auto& cam = sa->Camera(); + { + mmi::DeviceModuleLockGuard guard(cam); + if (cam->IsCapturing()) + cam->StopSequenceAcquisition(); + } + if (sa->TakeDeferredShutterClose()) + closeDeferredAutoShutter(); + sa->JoinDeferredShutterCloseWorker(); } } @@ -3458,6 +3453,14 @@ void CMMCore::stopSequenceAcquisition(const char* label) MMCORE_LEGACY_THROW(CMM deviceManager_->GetDeviceOfType(label); auto sa = findAcquisitionByCamera(pCam->GetLabel()); + if (sa) { + // Set stopRequested_ before taking the camera adapter module lock + // so that any AcqFinished that fires from the camera's thread while + // we hold the lock observes it under SequenceAcquisition::mu_ and + // takes the deferred-close path (which we drain below) rather than + // racing the spawn-and-adopt path with our join. + (void)sa->MarkStopRequested(); + } { mmi::DeviceModuleLockGuard guard(pCam); @@ -3472,10 +3475,14 @@ void CMMCore::stopSequenceAcquisition(const char* label) MMCORE_LEGACY_THROW(CMM } } - if (sa && sa->TakeDeferredShutterClose()) - closeDeferredAutoShutter(); + if (sa) { + if (sa->TakeDeferredShutterClose()) + closeDeferredAutoShutter(); + sa->JoinDeferredShutterCloseWorker(); + if (sa->IsComplete()) + eraseCompletedAcquisition(sa->CameraLabel()); + } - markAcquisitionStopRequested(pCam->GetLabel()); LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from camera " << label; } @@ -3519,6 +3526,10 @@ void CMMCore::stopSequenceAcquisition() MMCORE_LEGACY_THROW(CMMError) } auto sa = findAcquisitionByCamera(camera->GetLabel()); + if (sa) { + // See same-named overload above for ordering rationale. + (void)sa->MarkStopRequested(); + } { mmi::DeviceModuleLockGuard guard(camera); @@ -3533,10 +3544,14 @@ void CMMCore::stopSequenceAcquisition() MMCORE_LEGACY_THROW(CMMError) } } - if (sa && sa->TakeDeferredShutterClose()) - closeDeferredAutoShutter(); + if (sa) { + if (sa->TakeDeferredShutterClose()) + closeDeferredAutoShutter(); + sa->JoinDeferredShutterCloseWorker(); + if (sa->IsComplete()) + eraseCompletedAcquisition(sa->CameraLabel()); + } - markAcquisitionStopRequested(camera->GetLabel()); LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from current camera"; } diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index 6241ee49d..968f9dfcf 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -828,8 +828,6 @@ class CMMCore bool overwriteData, std::function startDevice); - void markAcquisitionStopRequested(const std::string& cameraLabel); - void closeDeferredAutoShutter(); void openDeferredAutoShutter( diff --git a/MMCore/SequenceAcquisition.cpp b/MMCore/SequenceAcquisition.cpp index b10a24bcb..0db92582d 100644 --- a/MMCore/SequenceAcquisition.cpp +++ b/MMCore/SequenceAcquisition.cpp @@ -19,6 +19,8 @@ #include "Devices/CameraInstance.h" +#include +#include #include namespace mmcore { @@ -71,7 +73,10 @@ SequenceAcquisition::SequenceAcquisition( { } -SequenceAcquisition::~SequenceAcquisition() = default; +SequenceAcquisition::~SequenceAcquisition() +{ + JoinDeferredShutterCloseWorker(); +} SequenceAcquisition::ParticipantInfo SequenceAcquisition::LookupParticipant(const MM::Device* caller) const noexcept @@ -98,12 +103,6 @@ bool SequenceAcquisition::HasParticipant( return caller && expectedParticipants_.count(caller); } -bool SequenceAcquisition::WasStopRequested() const noexcept -{ - std::lock_guard g(mu_); - return stopRequested_; -} - bool SequenceAcquisition::MarkStopRequested() noexcept { std::lock_guard g(mu_); @@ -192,12 +191,6 @@ bool SequenceAcquisition::AllParticipantsFinished() const noexcept return finishedParticipants_.size() == expectedParticipants_.size(); } -void SequenceAcquisition::DeferShutterClose() -{ - std::lock_guard g(mu_); - shutterCloseDeferred_ = true; -} - bool SequenceAcquisition::TakeDeferredShutterClose() { std::lock_guard g(mu_); @@ -216,5 +209,35 @@ bool SequenceAcquisition::TakeDeferredShutterOpen() return std::exchange(shutterOpenDeferred_, false); } +SequenceAcquisition::ShutterCloseSpawnResult +SequenceAcquisition::SpawnOrDeferShutterClose( + std::function factory) +{ + std::lock_guard g(mu_); + if (stopRequested_) { + shutterCloseDeferred_ = true; + return ShutterCloseSpawnResult::Deferred; + } + assert(!shutterCloseWorker_.joinable()); + try { + shutterCloseWorker_ = factory(); + } catch (...) { + shutterCloseDeferred_ = true; + throw; + } + return ShutterCloseSpawnResult::Spawned; +} + +void SequenceAcquisition::JoinDeferredShutterCloseWorker() +{ + std::thread w; + { + std::lock_guard g(mu_); + w = std::move(shutterCloseWorker_); + } + if (w.joinable()) + w.join(); +} + } // namespace internal } // namespace mmcore diff --git a/MMCore/SequenceAcquisition.h b/MMCore/SequenceAcquisition.h index 2703bc1f4..e330912a8 100644 --- a/MMCore/SequenceAcquisition.h +++ b/MMCore/SequenceAcquisition.h @@ -20,10 +20,12 @@ #include "MMDevice.h" #include +#include #include #include #include #include +#include #include namespace mmcore { @@ -92,10 +94,17 @@ class SequenceAcquisition { const ChannelInfo& Channel(unsigned n) const { return channels_.at(n); } // Mutable state (mutex-protected): - bool WasStopRequested() const noexcept; - // Mark stop requested. Returns true iff this call caused a transition to // "complete" (i.e., stopRequested && all participants have finished). + // + // Load-bearing ordering: stopSequenceAcquisition() calls this BEFORE + // taking the camera adapter module lock. AcqFinished checks + // stopRequested_ under SequenceAcquisition::mu_ inside + // SpawnOrDeferShutterClose; setting the flag first ensures that an + // AcqFinished arriving while the stop path holds the module lock takes + // the deferred-close path (drained by the stopper) rather than spawning + // a worker that the stopper would race to join. Do not move + // MarkStopRequested inside the module-lock scope. bool MarkStopRequested() noexcept; // Returns disposition; see enum. On FirstOpener, caller must invoke @@ -128,12 +137,37 @@ class SequenceAcquisition { // (regardless of whether stop was requested). bool AllParticipantsFinished() const noexcept; - void DeferShutterClose(); bool TakeDeferredShutterClose(); void DeferShutterOpen(); bool TakeDeferredShutterOpen(); + enum class ShutterCloseSpawnResult { + Spawned, // factory was invoked; worker stored as deferred-close worker + Deferred, // stopRequested already set; shutterCloseDeferred_ raised + }; + + // Atomic spawn-or-defer for the deferred autoshutter close on the + // same-module path of CoreCallback::AcqFinished. Held under mu_: + // + // - If stopRequested_ is true, sets shutterCloseDeferred_=true and + // returns Deferred without invoking factory(). + // - Otherwise invokes factory() and stores the resulting std::thread + // as shutterCloseWorker_; returns Spawned. + // + // If factory() throws (e.g. std::system_error from std::thread + // construction), shutterCloseDeferred_ is set to true and the exception + // propagates to the caller. Must be called at most once per + // SequenceAcquisition; the caller is responsible for any logging on the + // exception path. + ShutterCloseSpawnResult SpawnOrDeferShutterClose( + std::function factory); + + // Join the deferred-shutter-close worker if one was adopted; no-op + // otherwise. Idempotent. Safe to call from any thread other than the + // worker itself. + void JoinDeferredShutterCloseWorker(); + private: SequenceAcquisition(std::shared_ptr camera, std::vector channels); @@ -153,6 +187,7 @@ class SequenceAcquisition { bool shutterOpenDeferred_ = false; std::set readyParticipants_; std::set finishedParticipants_; + std::thread shutterCloseWorker_; }; } // namespace internal diff --git a/MMCore/unittest/SequenceAcquisition-Tests.cpp b/MMCore/unittest/SequenceAcquisition-Tests.cpp index f4c799fdc..7952c5173 100644 --- a/MMCore/unittest/SequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/SequenceAcquisition-Tests.cpp @@ -568,6 +568,98 @@ TEST_CASE("Async different-module shutter closes on stopSequenceAcquisition " CHECK(shutter.open == false); } +TEST_CASE("Natural completion with contended same-module lock closes shutter " + "via worker (no stop ever called)", + "[SequenceAcquisition][Autoshutter]") { + SyncCamera cam; + ConcurrentStubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(10, 0.0, true); + REQUIRE(shutter.GetOpenSync() == true); + + // Hold the camera adapter module lock from another thread, by parking + // it inside IsCapturing() (called via c.isSequenceRunning() under the + // module lock). Once WaitForIsCapturingBlocked returns, the holder + // provably holds the module lock until UnblockIsCapturing. + cam.blockIsCapturing = true; + std::thread holder([&] { (void)c.isSequenceRunning(); }); + REQUIRE(cam.WaitForIsCapturingBlocked()); + + // Drive natural completion. AcqFinished's try_lock fails (holder owns + // the module lock); stop was not requested, so AcqFinished spawns the + // worker, which then blocks waiting for the module lock. + cam.TriggerSelfFinish(); + + cam.UnblockIsCapturing(); + holder.join(); + + // Worker may now acquire the lock and close the shutter. Synchronize on + // the SetOpen(false) call; this provides happens-before for the + // subsequent open/count reads. + REQUIRE(shutter.WaitForSetOpenFalse(1)); + CHECK(shutter.GetOpenSync() == false); + CHECK(shutter.GetSetOpenFalseCount() == 1); +} + +TEST_CASE("stopSequenceAcquisition holds module lock; concurrent AcqFinished " + "defers and stopper drains the deferred shutter close", + "[SequenceAcquisition][Autoshutter]") { + SyncCamera cam; + ConcurrentStubShutter shutter; + MockAdapterWithDevices adapter{{"cam", &cam}, {"shutter", &shutter}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + c.setShutterDevice("shutter"); + c.setAutoShutter(true); + + c.startSequenceAcquisition(1000000, 0.0, true); + REQUIRE(shutter.GetOpenSync() == true); + + // Force the stopper to park inside camera->StopSequenceAcquisition + // while holding the camera adapter module lock. MarkStopRequested + // has already run on the stopper (it precedes the module-lock scope + // in CMMCore::stopSequenceAcquisition), so stopRequested_ is true + // and observable to a concurrent AcqFinished. + cam.blockStopSeqAcq = true; + std::thread stopper([&] { c.stopSequenceAcquisition(); }); + const auto stopperTid = stopper.get_id(); + REQUIRE(cam.WaitForStopBlocked()); + + // Fire AcqFinished from a different thread. try_lock on the module + // lock fails (stopper holds it). SpawnOrDeferShutterClose observes + // stopRequested_ == true and takes the defer branch: it sets + // shutterCloseDeferred_ and returns Deferred without spawning a + // worker. AcqFinished returns; the shutter is not yet closed. + std::thread acqFin([&] { cam.TriggerSelfFinish(); }); + acqFin.join(); + + // Stopper is still parked; the deferred close has not yet been + // drained. + CHECK(shutter.GetSetOpenFalseCount() == 0); + CHECK(shutter.GetOpenSync() == true); + + // Release the stopper. It exits the module-lock scope, calls + // TakeDeferredShutterClose (returns true), and closes the shutter + // on its own thread. + cam.ReleaseStopBlocked(); + stopper.join(); + + CHECK(shutter.GetOpenSync() == false); + CHECK(shutter.GetSetOpenFalseCount() == 1); + // The close ran on the stopper thread. This is what proves the defer + // branch was actually taken: a spawn-branch close would happen on + // the worker thread spawned inside SpawnOrDeferShutterClose, not on + // the stopper. + CHECK(shutter.GetSetOpenFalseThreadId() == stopperTid); +} + TEST_CASE("startSequenceAcquisition after camera self-finish without " "intervening stop succeeds", "[SequenceAcquisition]") { diff --git a/MMCore/unittest/StubDevices.h b/MMCore/unittest/StubDevices.h index e3bb87eb3..485d1df8d 100644 --- a/MMCore/unittest/StubDevices.h +++ b/MMCore/unittest/StubDevices.h @@ -134,6 +134,17 @@ struct SyncCamera : CCameraBase { bool capturing = false; bool reportCapturing = true; + // When true, IsCapturing() blocks until UnblockIsCapturing() is called. + // Tests use this to hold the camera adapter module lock from a thread + // that calls c.isSequenceRunning(). + bool blockIsCapturing = false; + + // When true, StopSequenceAcquisition() blocks until ReleaseStopBlocked() + // is called. While blocked, the calling thread typically holds the + // camera adapter module lock (since CMMCore::stopSequenceAcquisition + // invokes camera->StopSequenceAcquisition under that lock). + bool blockStopSeqAcq = false; + explicit SyncCamera(std::string n = "SyncCamera") : name(std::move(n)) {} int Initialize() override { return DEVICE_OK; } @@ -183,13 +194,66 @@ struct SyncCamera : CCameraBase { return GetCoreCallback()->PrepareForAcq(this); } int StopSequenceAcquisition() override { + if (blockStopSeqAcq) { + std::unique_lock lk(stopBlockMu_); + inStopBlock_ = true; + stopBlockCv_.notify_all(); + stopBlockCv_.wait(lk, [this] { return stopReleased_; }); + inStopBlock_ = false; + // Note: do NOT call Finish() here. The test fires AcqFinished + // from a separate thread while we are parked, so that AcqFinished + // races against this thread's module-lock hold (the desired + // scenario). Calling Finish() would fire AcqFinished on this + // thread, which holds the module lock recursively — try_lock + // would succeed and SpawnOrDeferShutterClose would never run. + return DEVICE_OK; + } Finish(); return DEVICE_OK; } - bool IsCapturing() override { return capturing && reportCapturing; } + bool IsCapturing() override { + if (blockIsCapturing) { + std::unique_lock lk(blockMu_); + inIsCapturingBlock_ = true; + blockCv_.notify_all(); + blockCv_.wait(lk, [this] { return isCapturingReleased_; }); + inIsCapturingBlock_ = false; + } + return capturing && reportCapturing; + } void TriggerSelfFinish() { Finish(); } + bool WaitForIsCapturingBlocked( + std::chrono::milliseconds timeout = + std::chrono::milliseconds(5000)) { + std::unique_lock lk(blockMu_); + return blockCv_.wait_for(lk, timeout, + [this] { return inIsCapturingBlock_; }); + } + void UnblockIsCapturing() { + { + std::lock_guard lk(blockMu_); + isCapturingReleased_ = true; + } + blockCv_.notify_all(); + } + + bool WaitForStopBlocked( + std::chrono::milliseconds timeout = + std::chrono::milliseconds(5000)) { + std::unique_lock lk(stopBlockMu_); + return stopBlockCv_.wait_for(lk, timeout, + [this] { return inStopBlock_; }); + } + void ReleaseStopBlocked() { + { + std::lock_guard lk(stopBlockMu_); + stopReleased_ = true; + } + stopBlockCv_.notify_all(); + } + int InsertTestImage(const unsigned char* pixels = nullptr) { std::vector defaultBuf; const unsigned char* buf = pixels; @@ -211,6 +275,16 @@ struct SyncCamera : CCameraBase { } std::vector imgBuf_; + + std::mutex blockMu_; + std::condition_variable blockCv_; + bool inIsCapturingBlock_ = false; + bool isCapturingReleased_ = false; + + std::mutex stopBlockMu_; + std::condition_variable stopBlockCv_; + bool inStopBlock_ = false; + bool stopReleased_ = false; }; // A camera that produces images asynchronously on its own thread. @@ -667,6 +741,86 @@ struct StubShutter : CShutterBase { int Fire(double) override { return DEVICE_OK; } }; +// Mock shutter for multi-threaded tests. State changes are mutex- +// synchronized; reads must go through the GetOpenSync / GetSet*Count +// accessors. WaitForSetOpenTrue / WaitForSetOpenFalse provide an +// observable synchronization point for tests that need to wait for a +// worker thread to reach SetOpen. +struct ConcurrentStubShutter : CShutterBase { + std::string name = "ConcurrentStubShutter"; + using CShutterBase::GetCoreCallback; + + int Initialize() override { return DEVICE_OK; } + int Shutdown() override { return DEVICE_OK; } + bool Busy() override { return false; } + void GetName(char* buf) const override { + CDeviceUtils::CopyLimitedString(buf, name.c_str()); + } + + int SetOpen(bool o) override { + { + std::lock_guard lk(stateMu_); + if (o) { + ++setOpenTrueCount_; + } else { + ++setOpenFalseCount_; + setOpenFalseThreadId_ = std::this_thread::get_id(); + } + open_ = o; + } + stateCv_.notify_all(); + return DEVICE_OK; + } + int GetOpen(bool& o) override { + std::lock_guard lk(stateMu_); + o = open_; + return DEVICE_OK; + } + int Fire(double) override { return DEVICE_OK; } + + bool WaitForSetOpenTrue( + int count = 1, + std::chrono::milliseconds timeout = + std::chrono::milliseconds(5000)) { + std::unique_lock lk(stateMu_); + return stateCv_.wait_for(lk, timeout, + [this, count] { return setOpenTrueCount_ >= count; }); + } + bool WaitForSetOpenFalse( + int count = 1, + std::chrono::milliseconds timeout = + std::chrono::milliseconds(5000)) { + std::unique_lock lk(stateMu_); + return stateCv_.wait_for(lk, timeout, + [this, count] { return setOpenFalseCount_ >= count; }); + } + + bool GetOpenSync() { + std::lock_guard lk(stateMu_); + return open_; + } + int GetSetOpenTrueCount() { + std::lock_guard lk(stateMu_); + return setOpenTrueCount_; + } + int GetSetOpenFalseCount() { + std::lock_guard lk(stateMu_); + return setOpenFalseCount_; + } + std::thread::id GetSetOpenFalseThreadId() { + std::lock_guard lk(stateMu_); + return setOpenFalseThreadId_; + } + +private: + std::mutex stateMu_; + std::condition_variable stateCv_; + bool open_ = false; + int setOpenTrueCount_ = 0; + int setOpenFalseCount_ = 0; + std::thread::id setOpenFalseThreadId_; +}; + struct StubMagnifier : CMagnifierBase { std::string name = "StubMagnifier"; using CMagnifierBase::OnMagnifierChanged; From 6492ff47372ff4725c09f6487aa59098aaa6a9c9 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Thu, 7 May 2026 13:01:17 +0900 Subject: [PATCH 15/17] Make isSequenceRunning() return per-acq status If the camera is a participant in a composite camera acquisition, it remains "running" until that acquisition as a whole finishes. We keep the camera IsCapturing() check, too, because some cameras may briefly return IsCapturing() == true after calling AcqFinished(). (Assisted by Claude Code; any errors are mine.) --- MMCore/MMCore.cpp | 42 ++++++-- MMCore/MMCore.h | 9 ++ MMCore/unittest/SequenceAcquisition-Tests.cpp | 100 ++++++++++++++++-- MMCore/unittest/StubDevices.h | 68 ++++++------ 4 files changed, 168 insertions(+), 51 deletions(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index c9d133d44..34af33e93 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -971,6 +971,23 @@ CMMCore::findAcquisitionByCamera(const std::string& cameraLabel) return it != acquisitions_.end() ? it->second : nullptr; } +std::shared_ptr +CMMCore::findAcquisitionInvolvingCamera( + const std::string& cameraLabel, const MM::Device* device) +{ + std::lock_guard g(acquisitionsMutex_); + for (const auto& kv : acquisitions_) { + const auto& sa = kv.second; + if (sa->AllParticipantsFinished()) + continue; + if (sa->CameraLabel() == cameraLabel || + (device && sa->HasParticipant(device))) { + return sa; + } + } + return nullptr; +} + void CMMCore::eraseCompletedAcquisition(const std::string& cameraLabel) { std::lock_guard g(acquisitionsMutex_); @@ -3563,17 +3580,19 @@ void CMMCore::stopSequenceAcquisition() MMCORE_LEGACY_THROW(CMMError) bool CMMCore::isSequenceRunning() MMCORE_NOEXCEPT { std::shared_ptr camera = currentCameraDevice_.lock(); - if (camera) + if (!camera) + return false; + try { - try - { - mmi::DeviceModuleLockGuard guard(camera); - return camera->IsCapturing(); - } - catch (const CMMError&) // Possibly uninitialized camera - { - // Fall through - } + if (findAcquisitionInvolvingCamera(camera->GetLabel(), + camera->GetRawPtr())) + return true; + mmi::DeviceModuleLockGuard guard(camera); + return camera->IsCapturing(); + } + catch (const CMMError&) // Possibly uninitialized camera + { + // Fall through } return false; }; @@ -3587,6 +3606,9 @@ bool CMMCore::isSequenceRunning(const char* label) MMCORE_LEGACY_THROW(CMMError) std::shared_ptr pCam = deviceManager_->GetDeviceOfType(label); + if (findAcquisitionInvolvingCamera(label, pCam->GetRawPtr())) + return true; + mmi::DeviceModuleLockGuard guard(pCam); return pCam->IsCapturing(); }; diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index 968f9dfcf..baa40c154 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -815,6 +815,15 @@ class CMMCore std::shared_ptr findAcquisitionByCamera(const std::string& cameraLabel); + // Look up an in-flight SA in which the named camera is the primary OR + // `device` is a participant. Returns nullptr if no such SA exists or if + // every match has AllParticipantsFinished() == true. Used by + // isSequenceRunning. Takes acquisitionsMutex_. + std::shared_ptr + findAcquisitionInvolvingCamera( + const std::string& cameraLabel, + const MM::Device* device); + // Erase a completed SA from `acquisitions_` (if present, by label). // Idempotent. Takes acquisitionsMutex_. void eraseCompletedAcquisition(const std::string& cameraLabel); diff --git a/MMCore/unittest/SequenceAcquisition-Tests.cpp b/MMCore/unittest/SequenceAcquisition-Tests.cpp index 7952c5173..629c3d12d 100644 --- a/MMCore/unittest/SequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/SequenceAcquisition-Tests.cpp @@ -185,6 +185,92 @@ TEST_CASE("isSequenceRunning (by label) tracks acquisition lifecycle", CHECK(c.isSequenceRunning("cam") == false); } +TEST_CASE("isSequenceRunning (by label) is true for a composite participant", + "[SequenceAcquisition]") { + SyncCamera p1("p1"); + SyncCamera p2("p2"); + MockCompositeCamera composite({&p1, &p2}); + MockAdapterWithDevices adapter{ + {"p1", &p1}, {"p2", &p2}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + + c.startSequenceAcquisition("composite", 10, 0.0, true); + CHECK(c.isSequenceRunning("composite")); + CHECK(c.isSequenceRunning("p1")); + CHECK(c.isSequenceRunning("p2")); + c.stopSequenceAcquisition("composite"); + CHECK_FALSE(c.isSequenceRunning("p1")); + CHECK_FALSE(c.isSequenceRunning("p2")); +} + +TEST_CASE("isSequenceRunning is true when tracking finished but device still " + "reports capturing", + "[SequenceAcquisition]") { + SyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + + c.startSequenceAcquisition(10, 0.0, true); + cam.TriggerSelfFinish(); // RecordFinish runs; AllParticipantsFinished() true. + // Simulate a device that has not yet returned to !IsCapturing (e.g. + // post-AcqFinished cleanup window or out-of-band activity). + cam.capturing = true; + CHECK(c.isSequenceRunning("cam")); + CHECK(c.isSequenceRunning()); + cam.capturing = false; + c.stopSequenceAcquisition(); +} + +TEST_CASE("isSequenceRunning is true while in-flight even if device IsCapturing " + "transiently reports false", + "[SequenceAcquisition]") { + SyncCamera cam; + MockAdapterWithDevices adapter{{"cam", &cam}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("cam"); + + c.startSequenceAcquisition(10, 0.0, true); + // Tracking entry exists and has unfinished participant; tracking is + // authoritative even if the device reports !IsCapturing. + cam.reportCapturing = false; + CHECK(c.isSequenceRunning("cam")); + CHECK(c.isSequenceRunning()); + cam.reportCapturing = true; + c.stopSequenceAcquisition(); +} + +TEST_CASE("isSequenceRunning (default) is true when current camera is a " + "composite participant of another acquisition", + "[SequenceAcquisition]") { + SyncCamera p1("p1"); + SyncCamera p2("p2"); + MockCompositeCamera composite({&p1, &p2}); + MockAdapterWithDevices adapter{ + {"p1", &p1}, {"p2", &p2}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + + // Set current camera to a physical, then start the composite acquisition + // by named label (so the current camera does not change). + c.setCameraDevice("p1"); + c.startSequenceAcquisition("composite", 10, 0.0, true); + // Hide device-level capturing so only tracking can answer affirmatively. + p1.reportCapturing = false; + CHECK(c.isSequenceRunning()); + p1.reportCapturing = true; + c.stopSequenceAcquisition("composite"); +} + +TEST_CASE("isSequenceRunning (by label) throws on unknown camera label", + "[SequenceAcquisition]") { + CMMCore c; + CHECK_THROWS_AS(c.isSequenceRunning("noSuchCamera"), CMMError); +} + // --- Buffer initialization side effects --- TEST_CASE("startSequenceAcquisition clears pre-existing images from buffer", @@ -584,19 +670,19 @@ TEST_CASE("Natural completion with contended same-module lock closes shutter " REQUIRE(shutter.GetOpenSync() == true); // Hold the camera adapter module lock from another thread, by parking - // it inside IsCapturing() (called via c.isSequenceRunning() under the - // module lock). Once WaitForIsCapturingBlocked returns, the holder - // provably holds the module lock until UnblockIsCapturing. - cam.blockIsCapturing = true; - std::thread holder([&] { (void)c.isSequenceRunning(); }); - REQUIRE(cam.WaitForIsCapturingBlocked()); + // it inside SetExposure() (called via c.setExposure under the module + // lock). Once WaitForSetExposureBlocked returns, the holder provably + // holds the module lock until UnblockSetExposure. + cam.blockSetExposure = true; + std::thread holder([&] { c.setExposure("cam", 10.0); }); + REQUIRE(cam.WaitForSetExposureBlocked()); // Drive natural completion. AcqFinished's try_lock fails (holder owns // the module lock); stop was not requested, so AcqFinished spawns the // worker, which then blocks waiting for the module lock. cam.TriggerSelfFinish(); - cam.UnblockIsCapturing(); + cam.UnblockSetExposure(); holder.join(); // Worker may now acquire the lock and close the shutter. Synchronize on diff --git a/MMCore/unittest/StubDevices.h b/MMCore/unittest/StubDevices.h index 485d1df8d..b6c84ec48 100644 --- a/MMCore/unittest/StubDevices.h +++ b/MMCore/unittest/StubDevices.h @@ -134,10 +134,10 @@ struct SyncCamera : CCameraBase { bool capturing = false; bool reportCapturing = true; - // When true, IsCapturing() blocks until UnblockIsCapturing() is called. + // When true, SetExposure() blocks until UnblockSetExposure() is called. // Tests use this to hold the camera adapter module lock from a thread - // that calls c.isSequenceRunning(). - bool blockIsCapturing = false; + // that calls c.setExposure("cam", ...). + bool blockSetExposure = false; // When true, StopSequenceAcquisition() blocks until ReleaseStopBlocked() // is called. While blocked, the calling thread typically holds the @@ -170,7 +170,16 @@ struct SyncCamera : CCameraBase { unsigned GetBitDepth() const override { return bitDepth; } int GetBinning() const override { return binning; } int SetBinning(int b) override { binning = b; return DEVICE_OK; } - void SetExposure(double e) override { exposure = e; } + void SetExposure(double e) override { + if (blockSetExposure) { + std::unique_lock lk(setExpMu_); + inSetExposureBlock_ = true; + setExpCv_.notify_all(); + setExpCv_.wait(lk, [this] { return setExposureReleased_; }); + inSetExposureBlock_ = false; + } + exposure = e; + } double GetExposure() const override { return exposure; } int SetROI(unsigned, unsigned, unsigned, unsigned) override { return DEVICE_OK; @@ -211,34 +220,10 @@ struct SyncCamera : CCameraBase { Finish(); return DEVICE_OK; } - bool IsCapturing() override { - if (blockIsCapturing) { - std::unique_lock lk(blockMu_); - inIsCapturingBlock_ = true; - blockCv_.notify_all(); - blockCv_.wait(lk, [this] { return isCapturingReleased_; }); - inIsCapturingBlock_ = false; - } - return capturing && reportCapturing; - } + bool IsCapturing() override { return capturing && reportCapturing; } void TriggerSelfFinish() { Finish(); } - bool WaitForIsCapturingBlocked( - std::chrono::milliseconds timeout = - std::chrono::milliseconds(5000)) { - std::unique_lock lk(blockMu_); - return blockCv_.wait_for(lk, timeout, - [this] { return inIsCapturingBlock_; }); - } - void UnblockIsCapturing() { - { - std::lock_guard lk(blockMu_); - isCapturingReleased_ = true; - } - blockCv_.notify_all(); - } - bool WaitForStopBlocked( std::chrono::milliseconds timeout = std::chrono::milliseconds(5000)) { @@ -254,6 +239,21 @@ struct SyncCamera : CCameraBase { stopBlockCv_.notify_all(); } + bool WaitForSetExposureBlocked( + std::chrono::milliseconds timeout = + std::chrono::milliseconds(5000)) { + std::unique_lock lk(setExpMu_); + return setExpCv_.wait_for(lk, timeout, + [this] { return inSetExposureBlock_; }); + } + void UnblockSetExposure() { + { + std::lock_guard lk(setExpMu_); + setExposureReleased_ = true; + } + setExpCv_.notify_all(); + } + int InsertTestImage(const unsigned char* pixels = nullptr) { std::vector defaultBuf; const unsigned char* buf = pixels; @@ -276,15 +276,15 @@ struct SyncCamera : CCameraBase { std::vector imgBuf_; - std::mutex blockMu_; - std::condition_variable blockCv_; - bool inIsCapturingBlock_ = false; - bool isCapturingReleased_ = false; - std::mutex stopBlockMu_; std::condition_variable stopBlockCv_; bool inStopBlock_ = false; bool stopReleased_ = false; + + std::mutex setExpMu_; + std::condition_variable setExpCv_; + bool inSetExposureBlock_ = false; + bool setExposureReleased_ = false; }; // A camera that produces images asynchronously on its own thread. From f8d4cb9f9fde542306be5b4a3164bf8addcba8b1 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Thu, 7 May 2026 13:13:54 +0900 Subject: [PATCH 16/17] Factor out common stopSequenceAcquisitionImpl impl DRY. (Assisted by Claude Code; any errors are mine.) --- MMCore/MMCore.cpp | 99 +++++++++++++++++++---------------------------- MMCore/MMCore.h | 3 ++ 2 files changed, 42 insertions(+), 60 deletions(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index 34af33e93..4aba428b0 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -3346,6 +3346,39 @@ void CMMCore::startSequenceAcquisitionImpl( openDeferredAutoShutter(sa); } +void CMMCore::stopSequenceAcquisitionImpl( + std::shared_ptr camera) +{ + auto sa = findAcquisitionByCamera(camera->GetLabel()); + if (sa) { + // Set stopRequested_ before taking the camera adapter module lock + // so that any AcqFinished that fires from the camera's thread while + // we hold the lock observes it under SequenceAcquisition::mu_ and + // takes the deferred-close path (which we drain below) rather than + // racing the spawn-and-adopt path with our join. + (void)sa->MarkStopRequested(); + } + + { + mmi::DeviceModuleLockGuard guard(camera); + int nRet = camera->StopSequenceAcquisition(); + if (nRet != DEVICE_OK) { + logError(getDeviceName(camera).c_str(), + getDeviceErrorText(nRet, camera).c_str()); + throw CMMError(getDeviceErrorText(nRet, camera).c_str(), + MMERR_DEVICE_GENERIC); + } + } + + if (sa) { + if (sa->TakeDeferredShutterClose()) + closeDeferredAutoShutter(); + sa->JoinDeferredShutterCloseWorker(); + if (sa->IsComplete()) + eraseCompletedAcquisition(sa->CameraLabel()); + } +} + /** * Starts streaming camera sequence acquisition. * This command does not block the calling thread for the duration of the acquisition. @@ -3468,38 +3501,9 @@ void CMMCore::stopSequenceAcquisition(const char* label) MMCORE_LEGACY_THROW(CMM { std::shared_ptr pCam = deviceManager_->GetDeviceOfType(label); - - auto sa = findAcquisitionByCamera(pCam->GetLabel()); - if (sa) { - // Set stopRequested_ before taking the camera adapter module lock - // so that any AcqFinished that fires from the camera's thread while - // we hold the lock observes it under SequenceAcquisition::mu_ and - // takes the deferred-close path (which we drain below) rather than - // racing the spawn-and-adopt path with our join. - (void)sa->MarkStopRequested(); - } - - { - mmi::DeviceModuleLockGuard guard(pCam); - LOG_DEBUG(coreLogger_) << - "Will stop sequence acquisition from camera " << label; - int nRet = pCam->StopSequenceAcquisition(); - if (nRet != DEVICE_OK) - { - logError(label, getDeviceErrorText(nRet, pCam).c_str()); - throw CMMError(getDeviceErrorText(nRet, pCam).c_str(), - MMERR_DEVICE_GENERIC); - } - } - - if (sa) { - if (sa->TakeDeferredShutterClose()) - closeDeferredAutoShutter(); - sa->JoinDeferredShutterCloseWorker(); - if (sa->IsComplete()) - eraseCompletedAcquisition(sa->CameraLabel()); - } - + LOG_DEBUG(coreLogger_) << + "Will stop sequence acquisition from camera " << label; + stopSequenceAcquisitionImpl(pCam); LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from camera " << label; } @@ -3541,34 +3545,9 @@ void CMMCore::stopSequenceAcquisition() MMCORE_LEGACY_THROW(CMMError) throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), MMERR_CameraNotAvailable); } - - auto sa = findAcquisitionByCamera(camera->GetLabel()); - if (sa) { - // See same-named overload above for ordering rationale. - (void)sa->MarkStopRequested(); - } - - { - mmi::DeviceModuleLockGuard guard(camera); - LOG_DEBUG(coreLogger_) << - "Will stop sequence acquisition from current camera"; - int nRet = camera->StopSequenceAcquisition(); - if (nRet != DEVICE_OK) { - logError(getDeviceName(camera).c_str(), - getDeviceErrorText(nRet, camera).c_str()); - throw CMMError(getDeviceErrorText(nRet, camera).c_str(), - MMERR_DEVICE_GENERIC); - } - } - - if (sa) { - if (sa->TakeDeferredShutterClose()) - closeDeferredAutoShutter(); - sa->JoinDeferredShutterCloseWorker(); - if (sa->IsComplete()) - eraseCompletedAcquisition(sa->CameraLabel()); - } - + LOG_DEBUG(coreLogger_) << + "Will stop sequence acquisition from current camera"; + stopSequenceAcquisitionImpl(camera); LOG_DEBUG(coreLogger_) << "Did stop sequence acquisition from current camera"; } diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index baa40c154..46d24ed51 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -837,6 +837,9 @@ class CMMCore bool overwriteData, std::function startDevice); + void stopSequenceAcquisitionImpl( + std::shared_ptr camera); + void closeDeferredAutoShutter(); void openDeferredAutoShutter( From 62293a974230ee28ea3615caa2d6572dd4247313 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Thu, 7 May 2026 13:32:22 +0900 Subject: [PATCH 17/17] stopSequenceAcquisition() requires primary camera Reject attempts to stop a non-primary participant in a composite camera acquisition. (Previous behavior was to roguely stop just that physical camera, which doesn't really make sense.) (Assisted by Claude Code; any errors are mine.) --- MMCore/MMCore.cpp | 19 ++++++ .../MultiChannelSequenceAcquisition-Tests.cpp | 62 +++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index 4aba428b0..67cae868d 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -3349,6 +3349,25 @@ void CMMCore::startSequenceAcquisitionImpl( void CMMCore::stopSequenceAcquisitionImpl( std::shared_ptr camera) { + // Reject if the camera is a non-primary participant of an active SA. + // Stop must go through the primary so that MarkStopRequested is + // observed before the camera adapter module lock is taken (see + // SequenceAcquisition.h ordering note), the deferred shutter-close + // is drained, and eraseCompletedAcquisition runs coherently — and + // so the composite adapter can cascade-stop its physicals. + if (auto involving = findAcquisitionInvolvingCamera( + camera->GetLabel(), camera->GetRawPtr())) { + if (involving->CameraLabel() != camera->GetLabel()) { + throw CMMError( + "Camera '" + camera->GetLabel() + + "' is a participant of the sequence acquisition on '" + + involving->CameraLabel() + + "'; stop the primary camera ('" + involving->CameraLabel() + + "') instead", + MMERR_NotAllowedDuringSequenceAcquisition); + } + } + auto sa = findAcquisitionByCamera(camera->GetLabel()); if (sa) { // Set stopRequested_ before taking the camera adapter module lock diff --git a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp index 26e1b740e..215000024 100644 --- a/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp +++ b/MMCore/unittest/MultiChannelSequenceAcquisition-Tests.cpp @@ -203,6 +203,68 @@ TEST_CASE("isSequenceRunning is true while composite camera is acquiring " CHECK_FALSE(c.isSequenceRunning()); } +TEST_CASE("Composite: stopSequenceAcquisition by phys-cam label is rejected " + "while composite SA is active", + "[MultiChannelSequenceAcquisition]") { + SyncCamera p0("p0"); + SyncCamera p1("p1"); + MockCompositeCamera composite({&p0, &p1}); + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + + c.startSequenceAcquisition(10, 0.0, true); + CHECK_THROWS_AS(c.stopSequenceAcquisition("p0"), CMMError); + CHECK(c.isSequenceRunning("composite")); + c.stopSequenceAcquisition("composite"); + CHECK_FALSE(c.isSequenceRunning()); +} + +TEST_CASE("Composite: no-arg stopSequenceAcquisition is rejected when current " + "camera is a phys-cam participant", + "[MultiChannelSequenceAcquisition]") { + SyncCamera p0("p0"); + SyncCamera p1("p1"); + MockCompositeCamera composite({&p0, &p1}); + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + // Set current camera to p0 *before* the SA starts; setCameraDevice + // is rejected during an active SA, so this models the case where the + // user kicked off the SA on the composite by label and the current + // camera happens to be a participant. + c.setCameraDevice("p0"); + + c.startSequenceAcquisition("composite", 10, 0.0, true); + CHECK_THROWS_AS(c.stopSequenceAcquisition(), CMMError); + CHECK(c.isSequenceRunning("composite")); + c.stopSequenceAcquisition("composite"); + CHECK_FALSE(c.isSequenceRunning()); +} + +TEST_CASE("Composite: stopSequenceAcquisition by phys-cam label is allowed " + "after all physicals self-finish", + "[MultiChannelSequenceAcquisition]") { + SyncCamera p0("p0"); + SyncCamera p1("p1"); + MockCompositeCamera composite({&p0, &p1}); + MockAdapterWithDevices adapter{ + {"p0", &p0}, {"p1", &p1}, {"composite", &composite}}; + CMMCore c; + adapter.LoadIntoCore(c); + c.setCameraDevice("composite"); + + c.startSequenceAcquisition(10, 0.0, true); + p0.TriggerSelfFinish(); + p1.TriggerSelfFinish(); + REQUIRE_FALSE(c.isSequenceRunning()); + + CHECK_NOTHROW(c.stopSequenceAcquisition("p0")); +} + // --- Tag scoping --- TEST_CASE("Composite phys cam used standalone after composite acquisition has "