fix: legacy RecordingWidget + Toupcam continuous-mode FPS + Live illumination#535
Merged
Conversation
The Record button always short-circuited with a "Please choose base saving directory first" modal even though DEFAULT_SAVING_PATH was already populated, because base_path_is_set was never flipped after the default path was applied. Also, cancelling the Browse dialog clobbered the existing path with an empty string. Drop the dead flag and gating modal (DEFAULT_SAVING_PATH is normalized in _def.py and always valid), and make set_saving_dir a no-op on cancel. Add regression tests covering construction, Record-without- Browse, Browse-cancel, and end-to-end frame persistence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch the RecordingWidget fixture to monkeypatch for DEFAULT_SAVING_PATH (automatic restore on test failure), drop the dead `is_live` cell, hoist imports, and rely on observable widget/saver state instead of reaching through `stream_handler._handler.save_image_flag`. Replace the manual `time.sleep` polling with `qtbot.waitUntil` so it pumps the Qt event loop, and use `qtbot.mouseClick` to also exercise the Record button's clicked signal wiring. Tighten file-search to `Path.rglob`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The in-code default for individual-image saves becomes TIFF for both multipoint and the recording widget. Users with an [ACQUISITION] image_format= override in their INI keep getting whatever they set — this only changes the source default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cross-thread callable returning the active AcquisitionChannel; saver thread calls it per frame. Plumbing only — process_queue still uses the inline imwrite path until the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
process_queue stops doing inline iio.imwrite / cv2.imwrite and instead calls utils_acquisition.save_image(), inheriting dtype-based extension (uint16 -> tiff, else Acquisition.IMAGE_FORMAT), channel-aware filenames, BF-LED RGB->GRAY conversion, and pseudo-color from the multipoint pipeline. A small _DefaultRecordingChannel sentinel handles the case where no channel provider is registered. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
One row per saved frame: frame_id, timestamp_iso, channel, exposure_ms, gain, illumination_intensity, file (relative path). Lets users identify recordings by metadata long after capture and provides bidirectional lookup between filename and frame_id. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- start_new_experiment: ERROR + raise on dir-create failure (was silent) - enqueue: throttled WARNING with dropped-count tracking (was print) - process_queue: ERROR + stop on OSError; WARNING per per-frame failure - close(): INFO summary line with frames_saved, dropped, duration - start: INFO line plus a one-time WARNING if no channel_provider is set WARNING/ERROR records surface via the existing WarningErrorWidget panel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RecordingWidget gains a channel_provider constructor kwarg; gui_hcs passes a lambda returning liveController.currentConfiguration. On Record-press the widget registers the provider on the saver; on stop (button or auto via stop_recording signal) it clears it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores the legacy “Simple Recording” widget behavior so the Record button works out-of-the-box with DEFAULT_SAVING_PATH, and modernizes single-camera recording output to match the multipoint acquisition saving path (TIFF-by-default, channel-aware filenames, and a per-recording frames.csv sidecar).
Changes:
- Fixes
RecordingWidgetdirectory selection logic so recording can start without an explicit browse, and canceling the browse dialog no longer clobbers the current saving directory. - Updates
ImageSaverto write frames viautils_acquisition.save_image()and emit per-frame metadata toframes.csv, with improved logging for write/queue issues. - Flips the in-code default
Acquisition.IMAGE_FORMATfrom"bmp"to"tiff"and adds regression tests for recording + default format.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
software/control/widgets.py |
Restores legacy RecordingWidget behavior (no blocking modal), adds optional channel provider wiring, and fixes browse-cancel behavior. |
software/control/gui_hcs.py |
Passes a channel provider from the live controller into RecordingWidget for per-frame metadata. |
software/control/core/core.py |
Refactors ImageSaver to use save_image(), writes frames.csv, and adds logging/backpressure tracking. |
software/control/_def.py |
Switches default acquisition image format to TIFF. |
software/tests/control/test_widgets.py |
Adds focused regression tests for RecordingWidget + ImageSaver behavior, filenames, CSV, and logging paths. |
software/tests/control/test_def.py |
Adds a test asserting the source default for IMAGE_FORMAT is TIFF. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+195
to
+200
| self.queue.task_done() | ||
| self.image_lock.release() | ||
| except: | ||
| pass | ||
| except OSError as e: | ||
| log.error(f"Writer fatal error: {e}; stopping recording") | ||
| self.stop_recording.emit() | ||
| except Exception as e: | ||
| log.warning(f"Failed to write frame {frame_id}: {e}") |
Comment on lines
+239
to
+242
| csv_path = os.path.join(experiment_dir, "frames.csv") | ||
| self._csv_file = open(csv_path, "w", newline="") | ||
| self._csv_writer = csv.writer(self._csv_file) | ||
| self._csv_writer.writerow( |
Comment on lines
203
to
+211
| try: | ||
| self.queue.put_nowait([image, frame_ID, timestamp]) | ||
| if (self.recording_time_limit > 0) and ( | ||
| time.time() - self.recording_start_time >= self.recording_time_limit | ||
| ): | ||
| self.stop_recording.emit() | ||
| # when using self.queue.put(str_), program can be slowed down despite multithreading because of the block and the GIL | ||
| except: | ||
| print("imageSaver queue is full, image discarded") | ||
| self.queue.put_nowait([image, frame_id, timestamp]) | ||
| except Exception: | ||
| self._dropped_count += 1 | ||
| now = time.time() | ||
| if now - self._last_queue_full_warning_ts >= 1.0: | ||
| log.warning(f"Image queue full; frame {frame_id} dropped") | ||
| self._last_queue_full_warning_ts = now | ||
| return |
Comment on lines
4499
to
4503
| self.streamHandler.start_recording() | ||
| else: | ||
| self.streamHandler.stop_recording() | ||
| self.imageSaver.set_channel_provider(None) | ||
| self.lineEdit_experimentID.setEnabled(True) |
Four related fixes from PR #535 review: 1. queue.task_done() now runs in finally; a write exception no longer leaves the item un-acked, which would have hung close()'s queue.join() indefinitely. 2. Extract stop_experiment(): closes frames.csv and logs the per-recording summary. Makes the Stop button (and time-limit auto-stop) finalize the CSV instead of leaving it open until app shutdown. 3. enqueue() now catches queue.Full specifically (was blanket Exception), so unrelated errors aren't silently misreported as queue saturation. 4. start_new_experiment() defensively calls stop_experiment() to close any prior CSV — protects against multi-record sessions leaking handles if a caller forgets to stop. The widget's toggle_recording() and stop_recording() now both call imageSaver.stop_experiment() so a single Record/Stop cycle produces a finalized CSV plus a summary log line, instead of one CSV per session. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop test_channel_provider_can_be_set_and_cleared (trivial slot getter/setter; the same code path is exercised through the widget by test_widget_constructed_with_channel_provider). - Merge test_multiple_recordings_close_previous_csv and test_stop_experiment_finalizes_csv_and_logs_summary into a single test_recording_lifecycle_finalizes_csv_and_starts_fresh that walks start -> stop (assert closed + summary log) -> start (assert new CSV). No coverage loss; less ceremony. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two bugs reported on PR #535: 1. 'Writer fatal error: directory doesn't exist' — Stop clears experiment_ID="" but the saver thread can still have buffered frames to process. Subsequent writes resolve to <base>/<file_id>/, a path that was never created. stop_experiment() now drains the queue via queue.join() before tearing down state, so buffered frames land in the right experiment dir. 2. Recording FPS dropped to 2-3. utils_acquisition.save_image() routes every frame through imageio.imwrite, which is ~10x slower than cv2.imwrite for uint8 frames. Plus csv_file.flush() per row added an fsync per frame. Now we use get_image_filepath() for channel-aware naming consistency with the multipoint pipeline but write directly with cv2.imwrite/iio.imwrite (legacy fast path); CSV is flushed only on stop/close. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop the uint16 -> iio.imwrite branch. cv2.imwrite handles 16-bit TIFF (and PNG) natively and is 2-3x faster than imageio/tifffile for the same data. get_image_filepath already forces uint16 -> .tiff so cv2 sees a TIFF path either way. - Add lightweight per-recording diagnostics logged in the "Recording stopped" summary: imwrite_avg/max, queue_wait_avg, input fps, save fps, drop count. Tells you at a glance whether the saver is the bottleneck (high imwrite_avg + low queue_wait), starved (low imwrite_avg + high queue_wait), or dropping at enqueue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six bugs surfaced in the code review (Phase 2 verifications): 1. Move counter increment to immediately after cv2.imwrite (was after CSV writerow). If writerow raised — closed file, UnicodeEncodeError, etc. — the counter stayed put and the next frame's file_id would reuse the same path, silently overwriting the just-saved image. 2. Discard frames that arrive after stop_experiment clears state. A Qt- queued enqueue slot can fire after the GUI thread has run stop_experiment (queue.join drains the Python queue but not the Qt event queue). Without a guard, save_dir becomes <base>/0/ and the frame lands in the wrong directory. 3. Open frames.csv with encoding="utf-8". Without it, Windows defaults to cp1252 and channel names with µ/°/Å raise UnicodeEncodeError on the first writerow. 4. Use getattr(channel, "name", "live") in path construction too (consistent with the CSV row writes); a provider returning a non-None object missing .name no longer drops the frame silently. 5. Move Thread.start() to the end of __init__ so the saver thread can't observe uninitialized diagnostic counters. 6. Reset _last_queue_full_warning_ts in start_new_experiment so a stop-restart immediately after a queue-saturation event in the previous recording doesn't suppress the first drop warning of the new one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Picks up the remaining cheap, reasonable fixes from PR #535 review: #1 cv2.imwrite return value: cv2 returns False (without raising) on missing dir, missing codec, or unsupported dtype. We now check the return value, log a throttled WARNING per failure, increment a _silent_write_failures counter (surfaced in the diagnostics summary), and skip counter increment + CSV row write so no row references a file that isn't on disk. #4 channel-name path sanitization: get_image_filepath previously only replaced spaces; now also strips '/', '\\', ':', '*', '?', '"', '<', '>', '|'. Strict improvement for both the recording widget and the multipoint pipeline. #7 frames.csv open guarded: a PermissionError on the CSV file no longer leaves the widget in a half-started state. On failure, experiment_ID is cleared and the exception is re-raised so the toggle_recording slot doesn't run streamHandler.start_recording(). #8 drop the dead image_format constructor parameter: process_queue hasn't read it since the cv2.imwrite refactor — it derives the extension from control._def.Acquisition.IMAGE_FORMAT via get_image_filepath. No in-tree callers passed it. #9 OSError auto-stop emits only once per recording: a saturated queue hitting a persistent OSError (e.g., full disk) no longer fires 10 redundant stop_recording signals. _stop_requested_from_writer is reset in start_new_experiment; subsequent errors during shutdown log as WARNING instead of re-emitting. #18 channel_provider returning None at start_new_experiment now logs a distinct WARNING. Before, a user who launched the GUI and clicked Record without first selecting a live channel got every frame tagged 'live' with no warning (because the provider lambda itself was non-None, just its return value was). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
4494
to
4498
| self.lineEdit_experimentID.setEnabled(False) | ||
| self.btn_setSavingDir.setEnabled(False) | ||
| if self._channel_provider is not None: | ||
| self.imageSaver.set_channel_provider(self._channel_provider) | ||
| self.imageSaver.start_new_experiment(self.lineEdit_experimentID.text()) |
Comment on lines
+185
to
+190
| saving_path = utils_acquisition.get_image_filepath( | ||
| save_dir, str(file_id), channel_name, image.dtype | ||
| ) | ||
| iio.imwrite(saving_path, image) | ||
| imwrite_started_ns = time.perf_counter_ns() | ||
| ok = cv2.imwrite(saving_path, image) | ||
| imwrite_ns = time.perf_counter_ns() - imwrite_started_ns |
| ) | ||
| self._csv_file.flush() | ||
| except OSError as e: | ||
| log.error(f"Failed to open {csv_path}: {e}") |
Two more Copilot-flagged issues: 1. RecordingWidget.toggle_recording disabled the experiment-ID and browse controls, and (via the checkable button) checked btn_record BEFORE calling start_new_experiment. After this PR raises on dir-create / CSV-open failures, that left the UI half-stuck: button checked, fields disabled, no recording in progress. Now: wrap the start call, restore UI state on any exception, and re-uncheck the button so the user can retry. 2. start_new_experiment's CSV exception path nulled out self._csv_file without closing it first. If open() succeeded but the header writerow or flush raised (rare — typically disk-full mid-write), that leaked the file descriptor. Now close the handle in the exception path before nulling the refs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…FRAMERATE on continuous
Reported: continuous-mode FPS sits at ~4 fps for ITR3CMOS26000KMA at
MONO16 / 2x2 binning, while software and hardware trigger both hit
~10 fps. Trigger mode bypasses PRECISE_FRAMERATE so the camera reads
at sensor-readout speed per trigger; continuous mode is paced by
PRECISE_FRAMERATE.
Three changes in _set_acquisition_mode_imp:
1. Wrap the TRIGGER option flip (and HW GPIO setup) in
`with self._pause_streaming():`. Every other state-mutating method
in this file (set_pixel_format, set_binning, set_frame_format)
already does this — the trigger-mode path was the lone outlier.
Camera options that need a fresh stream to take effect should now
actually take effect.
2. When switching to continuous, explicitly read
MAX_PRECISE_FRAMERATE and write it back to PRECISE_FRAMERATE
inside the paused window. Previously this only happened as a side
effect of `set_exposure_time(...)` running afterwards on a live
stream — exactly the path the comment in `_calculate_strobe_info`
already calls out as needing a re-set ("default value is only 90%
of setting value").
3. Add an INFO log line after the switch reporting
PRECISE_FRAMERATE, MAX_PRECISE_FRAMERATE, BANDWIDTH, and
FR_LIMIT. Lets us distinguish a hardware cap (MAX is itself low
in video mode at this config) from the SDK silently ignoring our
PRECISE_FRAMERATE put.
No test changes — TestRecordingWidget and existing tests aren't on
this code path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three fixes from the post-commit review of 3ece218: 1. Move set_exposure_time(self.get_exposure_time()) INSIDE the _pause_streaming with-block. It chains into _update_internal_settings → _calculate_strobe_info which itself writes PRECISE_FRAMERATE (camera_toupcam.py:130). With the call after the with-block, that write happened on a *live* stream — exactly the path the diff's own comment says is unreliable. Now strobe-info, ExpoTime, and the PRECISE_FRAMERATE write all run while paused. 2. Drop the now-redundant CONTINUOUS-specific MAX→PRECISE_FRAMERATE put. _calculate_strobe_info already does the same write, and with #1 it now does it inside the pause window. 3. Reset self._trigger_sent = False after the pause. stop_streaming() cancels any in-flight frame callback that would have cleared the flag, so a mode switch initiated shortly after send_trigger() would leave _trigger_sent=True until ~1.5×exposure+4s timeout — silently blocking the next trigger. Also broadened the diagnostic log's except from HRESULTException to Exception so a non-numeric get_Option return (TypeError on the format spec) doesn't propagate out and break the mode switch at the last step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reverts the structural simplification from bf42d30. Moving set_exposure_time inside _pause_streaming caused _calculate_strobe_info to run on a stopped stream, where get_Option(MAX_PRECISE_FRAMERATE) fails — the post-rebuild log line "get max_framerate fail" is the strobe-info chain re-raising and propagating out of the mode switch. Some Toupcam options can only be queried while the stream is running. Restore the previous structure: explicit MAX→PRECISE_FRAMERATE put for CONTINUOUS inside the with-block (which doesn't need to read MAX_.. on a stopped stream because we cached it on the live stream just before via a different read), and set_exposure_time after. Keep the two unrelated fixes from bf42d30: _trigger_sent reset and the broadened diagnostic-log except clause. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Building on de37d87 — the in-pause CONTINUOUS branch reads MAX_PRECISE_FRAMERATE and writes PRECISE_FRAMERATE. But the user's "get max_framerate fail" log shows the camera rejects that get_Option while the stream is stopped. The previous structure swallowed the HRESULT but then the put_Option was skipped, defeating the whole point of the fix (PRECISE_FRAMERATE never gets driven to max). Cache MAX_PRECISE_FRAMERATE before _pause_streaming (while the stream is still live, where the read works), then write PRECISE_FRAMERATE inside the with-block (the only state where the write sticks). If the pre-pause read fails on some SKU, log a warning and skip the in-pause write — set_exposure_time after the pause will still try via _calculate_strobe_info on the running stream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reverts the four toupcam commits on this branch: - 3ece218 (pause/restart + explicit PRECISE_FRAMERATE) - bf42d30 (/simplify: move set_exposure_time into pause; broke get max_framerate) - de37d87 (move set_exposure_time back out) - ccdaadc (cache MAX_PRECISE_FRAMERATE before pause) User reports continuous mode FPS got slower with the latest attempt than it was at baseline. The root cause is not the trigger-option mid-stream flip — it's that we lack a clean way to read the camera's MAX_PRECISE_FRAMERATE in the destination mode and apply it as a put_Option while the stream is in the state where the option actually sticks. Restoring control/camera_toupcam.py to master keeps PR #535 scoped to the recording widget. The Toupcam continuous-FPS investigation should move to a separate branch where it doesn't block the recording-widget review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pure diagnostic, no behavior changes. After the user's revert showed
MAX_PRECISE_FRAMERATE is identical in continuous and trigger modes but
the actual delivered FPS is still lower in continuous, we need to
find where the per-frame time goes. Likely suspects:
- OPTION_DDR_DEPTH + auto-exposure: when auto-exposure is enabled in
video mode, DDR caches only 1 frame, so the camera blocks on host
PullImageV2 + processing time (no pipelining).
- Host-side per-frame work (PullImageV2 USB transfer ~50ms for 13MB
MONO16, rotate_and_flip image.copy(), display crop) hides behind
the trigger timer in SW/HW modes but directly subtracts from FPS
in continuous.
Two diagnostics:
1. _log_startup_option_dump() runs once after _update_internal_settings
in __init__. Logs current TRIGGER, PRECISE_FRAMERATE,
MAX_PRECISE_FRAMERATE, FRAMERATE_LIMIT, BANDWIDTH, DDR_DEPTH, RAW,
LINEAR, CURVE, MULTITHREAD, LOW_NOISE, and AUTOEXP. Tells us at a
glance what state the camera defaults to.
2. _on_frame_callback now times each stage (pull, process, propagate)
and the inter-frame interval, logging every Nth frame (default 30)
so we can compare continuous vs trigger without log spam. The log
line tags the mode so trigger-vs-continuous comparison is direct.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… fails Pre-existing bug exposed by switching from continuous to software trigger: _calculate_strobe_info re-raises on get_Option( MAX_PRECISE_FRAMERATE) failure, which propagates out through set_exposure_time → _set_acquisition_mode_imp, leaving the mode switch half-applied and the camera in an unusable state. The option isn't actually required for trigger modes (PRECISE_FRAMERATE only paces video mode). When the read fails — typically in a transient state right after the TRIGGER option flips — log a warning and fall back to a sensor-floor vheight calculation. mode-switch keeps going, and continuous mode's PRECISE_FRAMERATE still gets driven to max on the (more reliable) startup path through _update_internal_settings where the camera does accept the read. Same treatment for the immediately-following put_Option( PRECISE_FRAMERATE) — log and skip rather than raise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Empirically, the pre-existing put_Option(PRECISE_FRAMERATE, MAX_PRECISE_FRAMERATE) in _calculate_strobe_info was throttling continuous mode to ~4 fps for ITR3CMOS26000KMA at MONO16 / 2x2 binning, while software / hardware trigger modes ran at the expected ~10 fps. The asymmetry is by design — PRECISE_FRAMERATE only paces continuous mode, so a "bad" write only affects that path. The legacy comment "default value is only 90% of setting value" motivated explicitly setting MAX, but on this camera (and likely others) the SDK's ~90%-of-MAX default is what the firmware can actually sustain. Writing the burst MAX makes the camera fall back to a fail-safe rate that's *slower* than the default — the opposite of the intended effect. Remove the put entirely. Keep the get so the strobe-time calc still has a max_framerate_fps value for vheight (in practice vheight floors at roi_height + 56 anyway). When the get fails, fall back to a high value so the floor still applies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This reverts commit 4482eb8.
Root cause for slow continuous mode, confirmed by the user's three
empirical findings:
1. Start Live in software trigger first, then switch to continuous
without stopping Live: ~10 fps continuous.
2. Stop Live, switch to continuous, then start Live: drops to ~2 fps.
3. ~18 fps continuous was briefly reached in 82d01d1 but couldn't
be reproduced — order-dependent on camera state at Start().
Pattern: continuous-mode FPS depends on the camera's state at
StartPullModeWithCallback time, not on subsequent option flips.
The Toupcam SDK doc for OPTION_DDR_DEPTH explicitly says default is
"auto: one frame for video mode when auto-exposure is enabled, full
capacity for others." Stream starts in video mode (TRIGGER=0 default)
+ default auto-exposure on => DDR caches one frame => host
PullImage path serializes with sensor readout => slow. Stream started
in trigger mode doesn't apply the auto-exposure DDR cap, and the
allocation isn't re-evaluated on trigger flip, so that path is fast.
Fix: put_AutoExpoEnable(False) in _configure_camera, which runs BEFORE
_start_raw_camera_stream in __init__. Squid sets exposure explicitly
per channel, so disabling auto-exposure has no user-visible effect
besides unlocking full DDR buffering.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
start_live() only ran the illumination-on path indirectly through _start_triggerred_acquisition() -> timer -> trigger_acquisition(), which only fires for software trigger (and hardware trigger with internal timer). Continuous trigger mode skips that entire chain — the camera just autonomously streams frames — so illumination was never turned on when Live started in continuous mode. stop_live() already calls turn_off_illumination() for all modes, so only the "on" side was broken. Add a symmetric turn_on_illumination() call in the continuous-mode branch of start_live. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Alpaca233
changed the title
Resolved conflict in software/control/gui_hcs.py: master added a new LaserEngineWidget construction block immediately before the RecordingWidget instantiation, while this branch added the channel_provider kwarg to RecordingWidget. Kept both: laser-engine widget block from master plus channel_provider kwarg from this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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 PR landed as a tightly-coupled chain of fixes that surfaced while bench-testing the recording widget. Three groups:
1. Legacy RecordingWidget — make it work + scientifically usable
The original bug. A
base_path_is_setflag was never flipped afterDEFAULT_SAVING_PATHwas applied, so clicking Record always short-circuited with a "Please choose base saving directory first" modal. Also:set_saving_dirblindly clobbered the existing path on dialog cancel.The follow-on work. Once the widget actually recorded, the saved files were unidentifiable two weeks later — BMPs in numbered folders with no metadata. Rebuilt
core.ImageSaverto:cv2.imwrite(uint8 + uint16) viautils_acquisition.get_image_filepath()for naming consistency with the multipoint pipeline (<file_id>_<channel>.<ext>). cv2 was ~10× faster than the previousimageiopath on uint8 and ~2-3× on uint16.frames.csvsidecar per recording:frame_id, timestamp_iso, channel, exposure_ms, gain, illumination_intensity, file. UTF-8 encoded; flushed only on stop so per-frame fsync doesn't gate FPS.channel_providercallable, registered byRecordingWidgetfromgui_hcs.py(lambda: self.liveController.currentConfiguration).except: pass/printpaths withsquid.logging: ERROR on dir-create / CSV-open failure (re-raised so the widget can restore UI state); throttled WARNING on queue saturation; single-emitOSErrorauto-stop; INFO start/stop summary + per-recording diagnostics (imwrite_avg/max,queue_wait_avg,input/saved FPS, drops, silent-write-failures).stop_experiment()lifecycle: drains the queue, closes CSV, logs the summary. Called from the Stop button and fromstop_recordingauto-stop slot.start_new_experimentclears stale state;process_queuediscards frames that arrive in the Qt event queue afterexperiment_IDwas cleared (race that previously wrote to<base>/0/);counterincrements immediately aftercv2.imwritesucceeds, before the CSV row write (so a writerow exception doesn't cause the next frame to overwrite).Default format flip.
Acquisition.IMAGE_FORMATdefault changes frombmptotiff(_def.py:119). BMP path stays available via[ACQUISITION] image_format = bmpin INI. Also extended channel-name sanitization inget_image_filepathto strip/ \ : * ? \" < > |in addition to spaces.2. Toupcam continuous-mode FPS regression
Independent issue noticed while bench-testing: continuous mode delivered ~2 fps for ITR3CMOS26000KMA at MONO16 / 2×2 binning, while software / hardware trigger ran at ~10 fps. Root cause confirmed via three empirical findings:
StartPullModeWithCallbacktime, not on subsequent option flips.Fix:
put_AutoExpoEnable(False)in_configure_camerabefore_start_raw_camera_streamruns. The Toupcam SDK doc explicitly saysOPTION_DDR_DEPTHdefault is "auto: one frame for video mode when auto-exposure is enabled, full capacity for others." Stream starting in video + auto-exposure-on locks DDR to one frame → hostPullImageserializes with sensor readout → 2 fps. Disabling auto-exposure unlocks full-capacity DDR buffering. Squid sets exposure explicitly per channel, so disabling auto-exposure has no user-visible effect besides the FPS win.Plus a defensive cleanup:
_calculate_strobe_infono longer re-raises onget_Option(MAX_PRECISE_FRAMERATE)failure — it warns and falls back to the sensor-floorvheight. The option is only meaningful for continuous-mode pacing; failing the read shouldn't break mode switches.3. Continuous-mode Live illumination
LiveController.start_livepreviously turned on illumination only via_start_triggerred_acquisition→ timer →trigger_acquisition, which fires for software trigger (and hardware trigger with internal timer). Continuous mode skipped that chain entirely, so starting Live in continuous mode left illumination off.stop_livealready turned it off symmetrically. Addedturn_on_illumination()in the continuous branch ofstart_live.Test plan
QT_QPA_PLATFORM=offscreen pytest tests/control/test_widgets.py tests/control/test_def.py— 168 passedblack --config pyproject.toml --check— cleanNotes
camera_toupcam.py) for future debugging; both at INFO level. Could be moved to DEBUG in a follow-up once we've verified the fix holds across configurations.🤖 Generated with Claude Code