fix the hardware time point hang and refuse overlap until frames can be discarded

src/main/java/org/micromanager/lightsheetmanager/model/PLogicScape.java

@@ -10,6 +10,7 @@
 import org.micromanager.lightsheetmanager.api.data.ChannelMode;
 import org.micromanager.lightsheetmanager.api.internal.ScapeAcquisitionSettings;
 import org.micromanager.lightsheetmanager.model.acquisitions.AcquisitionEngineScape;
+import org.micromanager.lightsheetmanager.model.acquisitions.CameraTriggers;
 import org.micromanager.lightsheetmanager.model.channels.ChannelSpec;
 import org.micromanager.lightsheetmanager.model.devices.cameras.CameraBase;
 import org.micromanager.lightsheetmanager.model.devices.vendor.ASIPLogic;
@@ -377,11 +378,7 @@ public boolean prepareControllerForAcquisitionSide(
 
             // if we are changing color slice by slice then set controller to do multiple slices per piezo move
             // otherwise just set to 1 slice per piezo move
-            int numSlicesPerPiezo = 1;
-            if (settings.channels().enabled() && settings.channels().mode() == ChannelMode.SLICE_HW) {
-                numSlicesPerPiezo = settings.channels().count();
-            }
-            scanner_.setSPIMNumSlicesPerPiezo(numSlicesPerPiezo);
+            scanner_.setSPIMNumSlicesPerPiezo(CameraTriggers.channelsPerSlice(settings));
 
             // set controller to do multiple volumes per start trigger if we are doing
             //   multiple channels with  hardware switching of channel volume by volume
@@ -451,22 +448,30 @@ public boolean prepareControllerForAcquisitionSide(
             piezoAmplitude = (settings.volume().slicesPerView() - 1) * settings.volume().sliceStepSize();
         }
 
-        // use this instead of settings.numSlices from here on out because
-        // we modify it if we are taking "extra slice" for synchronous/overlap
-        int numSlicesHW = settings.volume().slicesPerView();
+        // how many slices we ask the hardware for, which is one more than the number of images
+        // wanted in synchronous/overlap mode.
+        // Only the hardware time point event factories are sized from this same count, because
+        // only they describe one continuous burst in which the extra trigger's frame reads out.
+        // The software time point factories stay sized to the images wanted, and must: each of
+        // their volumes is its own camera sequence that ends before the extra trigger's frame
+        // arrives, so arming for this count instead would wait for a frame per volume that never
+        // comes.
+        final int numSlicesHW = CameraTriggers.slicesPerVolume(settings);
 
         // tweak the piezo parameters if we are using synchronous/overlap mode
         // object is to get exact same piezo/scanner positions in first N frames (piezo/scanner will move to N+1st position but no image taken)
         // amplitude is (N-1)*stepSize and artificially make N*stepSize instead => multiply amplitude by (N)/(N-1)
         // offset shifts by half a step
         final CameraMode cameraMode = settings.cameraMode();
         if (cameraMode == CameraMode.OVERLAP) {
-            if (settings.volume().slicesPerView() > 1) {
-                piezoAmplitude *= numSlicesHW / (numSlicesHW - 1.0);
+            // the number of images wanted, not the number of triggers: the extra trigger moves the
+            // piezo to a position no wanted image is taken at
+            final int numSlices = settings.volume().slicesPerView();
+            if (numSlices > 1) {
+                piezoAmplitude *= numSlices / (numSlices - 1.0);
             }
             // was piezoCenter += piezoAmplitude/(2*numSlicesHW) which isn't quite the same but close enough that nobody probably noticed
             piezoCenter += settings.volume().sliceStepSize() / 2;
-            numSlicesHW += 1;
         }
 
         // HACK(Brandon): used this to get a single camera to work with 2 simultaneous cameras

src/main/java/org/micromanager/lightsheetmanager/model/acquisitions/AcquisitionEngineScape.java

@@ -219,6 +219,7 @@ boolean run() {
             }
         }
 
+
             // --- testing code below ---
 //            StrVector deviceNames = core_.getLoadedDevices();
 //            for (String deviceName : deviceNames) {
@@ -672,8 +673,13 @@ public void close() {
                             // carrying no per-channel presets. Stamping a preset per channel made
                             // AcqEngJ split the merge and start one more camera sequence than the
                             // controller was armed to deliver, which then waited for frames that
-                            // never arrived. VOLUME_HW reaches this branch too, but needs the
-                            // opposite channel axis order and is refused on SCAPE.
+                            // never arrived.
+                            // VOLUME_HW reaches this branch too and is filed wrong when it does:
+                            // it switches channel once per volume, so it needs the channel axis
+                            // outside the z axis rather than innermost. The channel panel steers
+                            // users away from it but does not write the corrected mode back to the
+                            // settings, so a stored profile or an API caller still arrives here.
+                            // Nothing refuses it on this path yet.
                             currentAcquisition_.submitEventIterator(
                                     LightSheetEventAdapter.createChannelPerSliceAcqEvents(
                                             baseEvent.copy(), acqSettings_, cameraNames,
@@ -896,6 +902,28 @@ private boolean doHardwareCalculations(PLogicScape plc) {
                     changeChannelPerVolumeDoneFirst = true;
                     break;
                 case VOLUME_HW:
+                    if (acqSettings_.channels().count() > 1) {
+                        // The controller switches channel once per volume here, so it delivers every
+                        // slice of one channel before starting the next. The event stream built for
+                        // this geometry puts the channel axis innermost, which is the interleaved
+                        // order, so the counts match, nothing fails, and nearly every frame is filed
+                        // against the wrong channel and slice. Refuse rather than record that.
+                        // Refusing here also covers the combination with hardware time points, which
+                        // 1.4 rejects separately: both drive the controller's repeat counter, and
+                        // hardware time points overwrite the repeat count this mode depends on.
+                        // The channel panel steers away from this mode but does not write the
+                        // correction back to the settings, so it still arrives here from a stored
+                        // profile or through the API.
+                        studio_.logs().showError("Channel mode \"" + ChannelMode.VOLUME_HW
+                                + "\" is not supported: images would be saved against the wrong "
+                                + "channel and slice. Use \"" + ChannelMode.SLICE_HW
+                                + "\" for hardware channel switching, or \"" + ChannelMode.VOLUME
+                                + "\" to switch channels in software.");
+                        return false; // early exit
+                    }
+                    // one channel needs no hardware switching at all, so it behaves like the
+                    // single channel case below and cannot be misordered
+                    break;
                 case SLICE_HW:
                     if (acqSettings_.channels().count() == 1) {
                         // only 1 channel selected so don't have to really use hardware switching
@@ -927,11 +955,11 @@ private boolean doHardwareCalculations(PLogicScape plc) {
 
         // 1.4 adjusts nrSlicesSoftware at this point when hardware timepoints are in use: one
         // controller trigger covers every timepoint, so the camera sequence has to be sized for
-        // the whole burst rather than one volume. There is no counterpart here, and
-        // nrSlicesSoftware is never read below because AcqEngJ sizes the sequence from however
-        // many events it merges. It will not merge across a timepoint boundary while the events
-        // carry per-timepoint start times, so under hardware timepoints the camera sequence
-        // covers one timepoint while the controller runs all of them.
+        // the whole burst rather than one volume. The counterpart here is the event stream itself,
+        // since AcqEngJ sizes the sequence from however many events it merges, and the hardware
+        // timepoint factories carry the whole burst for exactly that reason. CameraTriggers holds
+        // the arithmetic both that stream and the controller are built from. nrSlicesSoftware
+        // stays dead: it is assigned above and never read.
 
         // TODO: make this more robust, should this be the first imaging camera?
         String cameraName;
@@ -1054,6 +1082,18 @@ private boolean doHardwareCalculations(PLogicScape plc) {
                         + "stage scanning. Raise the time point interval to at least " + minIntervalSec + " s.");
                 return false;
             }
+            if (CameraTriggers.hasSurplusFrames(acqSettings_)) {
+                // Overlap mode delivers more images than the dataset wants, and the extra ones
+                // arrive at the end of every volume rather than at the end of the run. Keeping the
+                // events lined up with the frames means consuming those images and discarding them
+                // afterwards, and no mechanism for discarding one exists yet. Refuse here rather
+                // than file them as the next time point's first slices and shift everything after.
+                studio_.logs().showError("Time point interval is too short: intervals under about "
+                        + minIntervalSec + " s switch to hardware time points, which can't yet be combined "
+                        + "with the \"Overlap/Synchronous\" camera mode. Either raise the time point "
+                        + "interval to at least " + minIntervalSec + " s, or choose a different camera mode.");
+                return false;
+            }
         }
 
         final int numTimePoints = acqSettings_.isUsingTimePoints() ? acqSettings_.numTimePoints() : 1;

src/main/java/org/micromanager/lightsheetmanager/model/acquisitions/CameraTriggers.java

@@ -0,0 +1,75 @@
+package org.micromanager.lightsheetmanager.model.acquisitions;
+
+import org.micromanager.lightsheetmanager.api.AcquisitionSettings;
+import org.micromanager.lightsheetmanager.api.data.CameraMode;
+import org.micromanager.lightsheetmanager.api.data.ChannelMode;
+
+/**
+ * How many camera triggers the controller emits, and how many of those come back as images.
+ * <p>
+ * Two places need these numbers and they have to agree: the controller is programmed with them,
+ * and the acquisition event stream is sized from them. AcqEngJ arms each camera for however many
+ * events it merged, so a trigger the controller emits that no event expects leaves the camera
+ * armed for a frame nobody claims, and an event with no trigger behind it waits forever.
+ * Computing the numbers here rather than at either call site is what keeps the two in step.
+ * <p>
+ * Overlap camera mode is where the two counts come apart. The camera reads one frame out while
+ * the next exposes, so getting N images out of N slice positions needs an N+1st trigger. That
+ * extra trigger does return an image; the image is simply not wanted. It arrives once per
+ * channel rather than once per volume, because the controller is told a slice count and a
+ * channel count and multiplies them.
+ */
+public final class CameraTriggers {
+
+    /**
+     * This class should not be instantiated.
+     */
+    private CameraTriggers() {
+        throw new AssertionError("Utility class; do not instantiate.");
+    }
+
+    /**
+     * The slice count the controller is programmed with, which is one more than the number of
+     * images wanted in overlap mode.
+     */
+    public static int slicesPerVolume(final AcquisitionSettings settings) {
+        if (settings.cameraMode() == CameraMode.OVERLAP) {
+            return settings.volume().slicesPerView() + 1;
+        }
+        return settings.volume().slicesPerView();
+    }
+
+    /**
+     * How many triggers the controller emits per slice position.
+     * <p>
+     * Slice by slice channel switching moves the channel within a slice, so the controller fires
+     * once per channel at each position. Every other channel arrangement fires once.
+     */
+    public static int channelsPerSlice(final AcquisitionSettings settings) {
+        if (settings.channels().enabled() && settings.channels().mode() == ChannelMode.SLICE_HW) {
+            return settings.channels().count();
+        }
+        return 1;
+    }
+
+    /**
+     * Whether the controller emits more triggers than there are images wanted.
+     * <p>
+     * Only overlap mode does. The other camera modes return exactly one image per trigger, so
+     * the trigger count and the image count are the same number and nothing has to be discarded.
+     */
+    public static boolean hasSurplusFrames(final AcquisitionSettings settings) {
+        return settings.cameraMode() == CameraMode.OVERLAP;
+    }
+
+    /**
+     * Whether a slice index addresses the surplus position rather than a wanted image.
+     * <p>
+     * The surplus position is the one past the last wanted slice, so it sits at the end of each
+     * volume and carries one image per channel.
+     */
+    public static boolean isSurplusSlice(final AcquisitionSettings settings, final int sliceIndex) {
+        return sliceIndex >= settings.volume().slicesPerView();
+    }
+
+}

src/main/java/org/micromanager/lightsheetmanager/model/acquisitions/LightSheetEventAdapter.java

@@ -74,14 +74,22 @@ public static Iterator<AcquisitionEvent> createTimelapseMultiChannelVolumeAcqEve
             applySingleChannel(baseEvent, usedChannels[0]);
         }
 
+        // No start time per time point. The controller owns the pacing once it is running the
+        // whole burst from one trigger, and a start time that differs per time point is half of
+        // what makes AcqEngJ refuse to merge across the boundary, which arms the cameras for one
+        // time point while the controller runs them all. The time index still goes on, since the
+        // datastore needs the coordinate and the index alone does not split the merge.
         Function<AcquisitionEvent, Iterator<AcquisitionEvent>> timelapse =
-                timelapse(settings.numTimePoints(), settings.timePointIntervalSec());
+                timelapse(settings.numTimePoints(), null);
         // Base 0 so cameras() leaves the plain camera index on the axis; channelAxis() runs
         // innermost and folds it into the combined slot.
         Function<AcquisitionEvent, Iterator<AcquisitionEvent>> cameras =
                 cameras(cameraDeviceNames, 0);
+        // The slice count the controller was programmed with, not the number of images wanted, so
+        // there is one event per trigger. The wrapper below then drops the single trigger that
+        // reads out no frame, leaving one event per frame the camera actually delivers.
         Function<AcquisitionEvent, Iterator<AcquisitionEvent>> zStack =
-                zStack(0, settings.volume().slicesPerView());
+                zStack(0, CameraTriggers.slicesPerVolume(settings));
         Function<AcquisitionEvent, Iterator<AcquisitionEvent>> channels =
                 channelAxis(usedChannels.length);
 
@@ -90,9 +98,18 @@ public static Iterator<AcquisitionEvent> createTimelapseMultiChannelVolumeAcqEve
         acqFunctions.add(cameras);
         acqFunctions.add(zStack);
         acqFunctions.add(channels);
-        return new AcquisitionEventIterator(baseEvent, acqFunctions, eventMonitor);
+        return SurplusFrames.wrap(
+                new AcquisitionEventIterator(baseEvent, acqFunctions, eventMonitor), settings);
     }
 
+    /**
+     * Build events for a timelapse of volumes with no channels, which the controller free-runs.
+     * <p>
+     * The channel-carrying sibling above describes the same burst; the only difference here is
+     * that the controller fires once per slice position instead of once per channel at each
+     * position, so a volume is one event per slice. Everything about the surplus frames in overlap
+     * mode applies unchanged, one image per volume rather than one per channel per volume.
+     */
     public static Iterator<AcquisitionEvent> createTimelapseVolumeAcqEvents(
             AcquisitionEvent baseEvent, AcquisitionSettings settings,
             String[] cameraDeviceNames,
@@ -106,14 +123,18 @@ public static Iterator<AcquisitionEvent> createTimelapseVolumeAcqEvents(
 
         Function<AcquisitionEvent, Iterator<AcquisitionEvent>> cameras = cameras(cameraDeviceNames);
 
+        // The slice count the controller was programmed with, not the number of images wanted, so
+        // there is one event per trigger. The wrapper below then drops the single trigger that
+        // reads out no frame, leaving one event per frame the camera actually delivers.
         Function<AcquisitionEvent, Iterator<AcquisitionEvent>> zStack =
-                zStack(0, settings.volume().slicesPerView());
+                zStack(0, CameraTriggers.slicesPerVolume(settings));
 
         ArrayList<Function<AcquisitionEvent, Iterator<AcquisitionEvent>>> acqFunctions = new ArrayList<>();
         acqFunctions.add(timelapse);
         acqFunctions.add(cameras);
         acqFunctions.add(zStack);
-        return new AcquisitionEventIterator(baseEvent, acqFunctions, eventMonitor);
+        return SurplusFrames.wrap(
+                new AcquisitionEventIterator(baseEvent, acqFunctions, eventMonitor), settings);
     }
 
     public static Iterator<AcquisitionEvent> createAcqEvents(