MMCore: (more) proper support for multi-channel cameras#936
Draft
marktsuchida wants to merge 17 commits into
Draft
MMCore: (more) proper support for multi-channel cameras#936marktsuchida wants to merge 17 commits into
marktsuchida wants to merge 17 commits into
Conversation
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.)
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.)
Finally. (Assisted by Claude Code; any errors are mine.)
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.)
(Assisted by Claude Code; any errors are mine.)
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.)
Another long-standing bug we can now easily fix. (Assisted by Claude Code; any errors are mine.)
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.)
It will fail anyway on InsertImage(), so fail fast. (Assisted by Claude Code; any errors are mine.)
marktsuchida
force-pushed
the
seq-acq-tracking
branch
from
1356217 to
dade037
Compare
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.)
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.)
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.)
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.)
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.)
DRY. (Assisted by Claude Code; any errors are mine.)
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.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a DIV-bumping change; it might become the main change for DIV 76 (#886).
SequenceAcquisitionobject to explicitly represent and track the lifetime of each sequence acquisition, replacing ad-hoc state scattered acrossCMMCoreDevice interface changes
Bumps the device interface version:
MM::Camera: addGetChannelCameraPtr()for MMCore to query participating physical camerasAddTag/RemoveTagmechanism (tags are now generated by MMCore duringInsertImage())Updated device adapters: MultiCamera, ArduinoCounter, CameraPulser, TwoPhoton. Regular (physical, single-channel) cameras don't need to change.
Notable behavioral changes
PrepareForAcq/InsertImage/AcqFinishedcallbacks now reject callers not participating in the active acquisitionSequenceAcquisitionStarted/Stoppednotifications fire once per composite camera, not per physical cameraCameraChannelIndex;CameraChannelNameis now added by MMCoreThings not covered here