From a3f2113fbca270862217de77e8774c9c95e16112 Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 10:25:48 -0700 Subject: [PATCH 01/12] fix: restore legacy RecordingWidget Record button 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) --- software/control/widgets.py | 15 +---- software/tests/control/test_widgets.py | 93 +++++++++++++++++++++++++- 2 files changed, 95 insertions(+), 13 deletions(-) diff --git a/software/control/widgets.py b/software/control/widgets.py index 52323c74e..0783a116a 100644 --- a/software/control/widgets.py +++ b/software/control/widgets.py @@ -4414,7 +4414,6 @@ def __init__(self, streamHandler, imageSaver, main=None, *args, **kwargs): super().__init__(*args, **kwargs) self.imageSaver = imageSaver # for saving path control self.streamHandler = streamHandler - self.base_path_is_set = False self.add_components() self.setFrameStyle(QFrame.Panel | QFrame.Raised) @@ -4425,8 +4424,6 @@ def add_components(self): self.lineEdit_savingDir = QLineEdit() self.lineEdit_savingDir.setReadOnly(True) - self.lineEdit_savingDir.setText("Choose a base saving directory") - self.lineEdit_savingDir.setText(DEFAULT_SAVING_PATH) self.imageSaver.set_base_path(DEFAULT_SAVING_PATH) @@ -4485,19 +4482,13 @@ def add_components(self): self.imageSaver.stop_recording.connect(self.stop_recording) def set_saving_dir(self): - dialog = QFileDialog() - save_dir_base = dialog.getExistingDirectory(None, "Select Folder") + save_dir_base = QFileDialog.getExistingDirectory(None, "Select Folder") + if not save_dir_base: + return self.imageSaver.set_base_path(save_dir_base) self.lineEdit_savingDir.setText(save_dir_base) - self.base_path_is_set = True def toggle_recording(self, pressed): - if self.base_path_is_set == False: - self.btn_record.setChecked(False) - msg = QMessageBox() - msg.setText("Please choose base saving directory first") - msg.exec_() - return if pressed: self.lineEdit_experimentID.setEnabled(False) self.btn_setSavingDir.setEnabled(False) diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index 7fc5ae99f..0a8a6468d 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -1,13 +1,15 @@ import logging import os import tempfile +import time from unittest.mock import MagicMock, Mock, patch +import numpy as np import pytest import control._def import control.microscope -from control.widgets import check_ram_available_with_error_dialog, NDViewerTab, SurfacePlotWidget +from control.widgets import check_ram_available_with_error_dialog, NDViewerTab, RecordingWidget, SurfacePlotWidget import tests.control.test_stubs as ts @@ -2356,6 +2358,95 @@ def test_expand_button_shows_only_dropped_when_single_message(self, warning_widg assert widget.btn_expand.isVisible() +# ============================================================================ +# RecordingWidget Tests +# ============================================================================ + + +@pytest.fixture +def recording_widget(qtbot, tmp_path): + """Create a RecordingWidget wired to real StreamHandler + ImageSaver, saving into tmp_path.""" + from control.core import core as _core + + is_live = {"v": True} + stream_handler = _core.QtStreamHandler(accept_new_frame_fn=lambda: is_live["v"]) + image_saver = _core.ImageSaver() + stream_handler.packet_image_to_write.connect(image_saver.enqueue) + + original_default = control._def.DEFAULT_SAVING_PATH + control._def.DEFAULT_SAVING_PATH = str(tmp_path) + # widgets.py imported DEFAULT_SAVING_PATH via `from control._def import *`, so patch its binding too. + import control.widgets as _widgets + + original_widgets_default = _widgets.DEFAULT_SAVING_PATH + _widgets.DEFAULT_SAVING_PATH = str(tmp_path) + + widget = RecordingWidget(stream_handler, image_saver) + qtbot.addWidget(widget) + + yield widget, stream_handler, image_saver, is_live + + control._def.DEFAULT_SAVING_PATH = original_default + _widgets.DEFAULT_SAVING_PATH = original_widgets_default + image_saver.close() + + +class TestRecordingWidget: + """Regression tests for the legacy single-camera RecordingWidget.""" + + def test_default_saving_path_applied_on_construction(self, recording_widget, tmp_path): + widget, _, image_saver, _ = recording_widget + assert widget.lineEdit_savingDir.text() == str(tmp_path) + assert image_saver.base_path == str(tmp_path) + + def test_record_without_browse_starts_recording(self, recording_widget): + """Pressing Record with the default path should start recording — no Browse required.""" + widget, stream_handler, image_saver, _ = recording_widget + widget.lineEdit_experimentID.setText("exp") + + widget.btn_record.setChecked(True) + widget.toggle_recording(True) + + assert stream_handler._handler.save_image_flag is True + assert image_saver.experiment_ID.startswith("exp_") + assert not widget.lineEdit_experimentID.isEnabled() + assert not widget.btn_setSavingDir.isEnabled() + + def test_browse_cancel_preserves_existing_path(self, recording_widget, tmp_path): + """Cancelling the file dialog must not clobber the previously valid path.""" + widget, _, image_saver, _ = recording_widget + with patch("qtpy.QtWidgets.QFileDialog.getExistingDirectory", return_value=""): + widget.set_saving_dir() + assert image_saver.base_path == str(tmp_path) + assert widget.lineEdit_savingDir.text() == str(tmp_path) + + def test_frame_is_persisted_to_disk(self, recording_widget, tmp_path): + """Pushing a frame through the StreamHandler while recording writes a file.""" + widget, stream_handler, image_saver, _ = recording_widget + from squid.abc import CameraFrame + + widget.lineEdit_experimentID.setText("exp") + widget.entry_saveFPS.setValue(1000) + widget.toggle_recording(True) + + img = np.zeros((32, 32), dtype=np.uint8) + frame = CameraFrame(frame_id=0, timestamp=time.time(), frame=img, frame_format=0, frame_pixel_format=0) + stream_handler.get_frame_callback()(frame) + + deadline = time.time() + 5.0 + while time.time() < deadline and image_saver.counter == 0: + time.sleep(0.05) + + exp_dir = os.path.join(image_saver.base_path, image_saver.experiment_ID) + files = [] + for root, _, fs in os.walk(exp_dir): + files.extend(fs) + assert files, f"No image saved under {exp_dir}" + + widget.toggle_recording(False) + assert stream_handler._handler.save_image_flag is False + + class TestWarningErrorWidgetErrorExemptionWithDroppedCount: """Tests for error exemption interacting with dropped count.""" From dddfef15c67153dd58562cb314531444cecd3ce4 Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 10:33:42 -0700 Subject: [PATCH 02/12] test: simplify RecordingWidget fixture and frame-persistence test 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) --- software/tests/control/test_widgets.py | 89 +++++++++++--------------- 1 file changed, 38 insertions(+), 51 deletions(-) diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index 0a8a6468d..ab8670cf2 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -1,15 +1,20 @@ import logging import os import tempfile -import time +from pathlib import Path from unittest.mock import MagicMock, Mock, patch import numpy as np import pytest +from qtpy.QtCore import Qt import control._def import control.microscope +import control.widgets +from control.core import core as core_module from control.widgets import check_ram_available_with_error_dialog, NDViewerTab, RecordingWidget, SurfacePlotWidget +from squid.abc import CameraFrame, CameraFrameFormat +from squid.config import CameraPixelFormat import tests.control.test_stubs as ts @@ -2364,87 +2369,69 @@ def test_expand_button_shows_only_dropped_when_single_message(self, warning_widg @pytest.fixture -def recording_widget(qtbot, tmp_path): - """Create a RecordingWidget wired to real StreamHandler + ImageSaver, saving into tmp_path.""" - from control.core import core as _core - - is_live = {"v": True} - stream_handler = _core.QtStreamHandler(accept_new_frame_fn=lambda: is_live["v"]) - image_saver = _core.ImageSaver() +def recording_widget(qtbot, tmp_path, monkeypatch): + # `widgets.py` imports DEFAULT_SAVING_PATH via `from control._def import *`, which binds a + # separate name in the widgets module — both bindings must be patched. + monkeypatch.setattr(control._def, "DEFAULT_SAVING_PATH", str(tmp_path)) + monkeypatch.setattr(control.widgets, "DEFAULT_SAVING_PATH", str(tmp_path)) + + stream_handler = core_module.QtStreamHandler(accept_new_frame_fn=lambda: True) + image_saver = core_module.ImageSaver() stream_handler.packet_image_to_write.connect(image_saver.enqueue) - original_default = control._def.DEFAULT_SAVING_PATH - control._def.DEFAULT_SAVING_PATH = str(tmp_path) - # widgets.py imported DEFAULT_SAVING_PATH via `from control._def import *`, so patch its binding too. - import control.widgets as _widgets - - original_widgets_default = _widgets.DEFAULT_SAVING_PATH - _widgets.DEFAULT_SAVING_PATH = str(tmp_path) - widget = RecordingWidget(stream_handler, image_saver) qtbot.addWidget(widget) - - yield widget, stream_handler, image_saver, is_live - - control._def.DEFAULT_SAVING_PATH = original_default - _widgets.DEFAULT_SAVING_PATH = original_widgets_default - image_saver.close() + try: + yield widget, stream_handler, image_saver + finally: + image_saver.close() class TestRecordingWidget: """Regression tests for the legacy single-camera RecordingWidget.""" def test_default_saving_path_applied_on_construction(self, recording_widget, tmp_path): - widget, _, image_saver, _ = recording_widget + widget, _, image_saver = recording_widget assert widget.lineEdit_savingDir.text() == str(tmp_path) assert image_saver.base_path == str(tmp_path) - def test_record_without_browse_starts_recording(self, recording_widget): - """Pressing Record with the default path should start recording — no Browse required.""" - widget, stream_handler, image_saver, _ = recording_widget + def test_record_without_browse_starts_recording(self, qtbot, recording_widget): + widget, _, image_saver = recording_widget widget.lineEdit_experimentID.setText("exp") - widget.btn_record.setChecked(True) - widget.toggle_recording(True) + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) - assert stream_handler._handler.save_image_flag is True + assert widget.btn_record.isChecked() assert image_saver.experiment_ID.startswith("exp_") assert not widget.lineEdit_experimentID.isEnabled() assert not widget.btn_setSavingDir.isEnabled() def test_browse_cancel_preserves_existing_path(self, recording_widget, tmp_path): - """Cancelling the file dialog must not clobber the previously valid path.""" - widget, _, image_saver, _ = recording_widget + widget, _, image_saver = recording_widget with patch("qtpy.QtWidgets.QFileDialog.getExistingDirectory", return_value=""): widget.set_saving_dir() assert image_saver.base_path == str(tmp_path) assert widget.lineEdit_savingDir.text() == str(tmp_path) - def test_frame_is_persisted_to_disk(self, recording_widget, tmp_path): - """Pushing a frame through the StreamHandler while recording writes a file.""" - widget, stream_handler, image_saver, _ = recording_widget - from squid.abc import CameraFrame - + def test_frame_is_persisted_to_disk(self, qtbot, recording_widget): + widget, stream_handler, image_saver = recording_widget widget.lineEdit_experimentID.setText("exp") widget.entry_saveFPS.setValue(1000) - widget.toggle_recording(True) - - img = np.zeros((32, 32), dtype=np.uint8) - frame = CameraFrame(frame_id=0, timestamp=time.time(), frame=img, frame_format=0, frame_pixel_format=0) + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + + frame = CameraFrame( + frame_id=0, + timestamp=0.0, + frame=np.zeros((32, 32), dtype=np.uint8), + frame_format=CameraFrameFormat.RAW, + frame_pixel_format=CameraPixelFormat.MONO8, + ) stream_handler.get_frame_callback()(frame) - deadline = time.time() + 5.0 - while time.time() < deadline and image_saver.counter == 0: - time.sleep(0.05) - - exp_dir = os.path.join(image_saver.base_path, image_saver.experiment_ID) - files = [] - for root, _, fs in os.walk(exp_dir): - files.extend(fs) - assert files, f"No image saved under {exp_dir}" + qtbot.waitUntil(lambda: image_saver.counter > 0, timeout=2000) - widget.toggle_recording(False) - assert stream_handler._handler.save_image_flag is False + exp_dir = Path(image_saver.base_path) / image_saver.experiment_ID + assert any(p.is_file() for p in exp_dir.rglob("*")), f"No image saved under {exp_dir}" class TestWarningErrorWidgetErrorExemptionWithDroppedCount: From e80c1a41920faf86424d0f00919eb92f5b3163e5 Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 16:29:31 -0700 Subject: [PATCH 03/12] feat: default Acquisition.IMAGE_FORMAT to tiff (was bmp) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- software/control/_def.py | 2 +- software/tests/control/test_def.py | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/software/control/_def.py b/software/control/_def.py index 182a0683e..53dbbc736 100644 --- a/software/control/_def.py +++ b/software/control/_def.py @@ -116,7 +116,7 @@ def convert_to_var(option: Union[str, "TriggerMode"]) -> "TriggerMode": class Acquisition: NUMBER_OF_FOVS_PER_AF = 3 - IMAGE_FORMAT = "bmp" + IMAGE_FORMAT = "tiff" IMAGE_DISPLAY_SCALING_FACTOR = 0.3 DX = 0.9 DY = 0.9 diff --git a/software/tests/control/test_def.py b/software/tests/control/test_def.py index e658db6f9..b04945dd7 100644 --- a/software/tests/control/test_def.py +++ b/software/tests/control/test_def.py @@ -227,3 +227,16 @@ def test_unrecognized_values_default_to_false(self): assert self._parse_sim_setting("fasle") is False assert self._parse_sim_setting("simualte") is False assert self._parse_sim_setting("invalid") is False + + +def test_default_image_format_is_tiff(): + """The in-code default for Acquisition.IMAGE_FORMAT is 'tiff' (was 'bmp'). + + INI files with [ACQUISITION] image_format=... override this at module + load time, so we assert against the source rather than the runtime value. + """ + import inspect + import control._def + + source = inspect.getsource(control._def.Acquisition) + assert 'IMAGE_FORMAT = "tiff"' in source From d5eae97d74ba9346904689cf296912b3506daa6c Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 17:12:47 -0700 Subject: [PATCH 04/12] feat(ImageSaver): add set_channel_provider for per-frame channel tagging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- software/control/core/core.py | 10 ++++++++++ software/tests/control/test_widgets.py | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/software/control/core/core.py b/software/control/core/core.py index ad1fa2526..e120c6e81 100644 --- a/software/control/core/core.py +++ b/software/control/core/core.py @@ -110,6 +110,16 @@ def __init__(self, image_format=Acquisition.IMAGE_FORMAT): self.counter = 0 self.recording_start_time = 0 self.recording_time_limit = -1 + self._channel_provider: Optional[Callable[[], Optional[Any]]] = None + + def set_channel_provider(self, provider): + """Register a callable returning the active live channel (or None). + + Called per-frame from the saver thread. Cross-thread reads of the + channel reference are GIL-atomic, so no lock is needed; the GUI thread + just swaps the reference when the user toggles channels. + """ + self._channel_provider = provider def process_queue(self): while True: diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index ab8670cf2..66f137bbf 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -2395,6 +2395,14 @@ def test_default_saving_path_applied_on_construction(self, recording_widget, tmp assert widget.lineEdit_savingDir.text() == str(tmp_path) assert image_saver.base_path == str(tmp_path) + def test_channel_provider_can_be_set_and_cleared(self, recording_widget): + _, _, image_saver = recording_widget + provider = lambda: None + image_saver.set_channel_provider(provider) + assert image_saver._channel_provider is provider + image_saver.set_channel_provider(None) + assert image_saver._channel_provider is None + def test_record_without_browse_starts_recording(self, qtbot, recording_widget): widget, _, image_saver = recording_widget widget.lineEdit_experimentID.setText("exp") From b2ae490dbb79f9f6a589b2914495c40ad3872c9d Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 17:17:49 -0700 Subject: [PATCH 05/12] feat(ImageSaver): delegate frame writes to utils_acquisition.save_image 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) --- software/control/core/core.py | 64 +++++++++++++++----------- software/tests/control/test_widgets.py | 6 ++- 2 files changed, 41 insertions(+), 29 deletions(-) diff --git a/software/control/core/core.py b/software/control/core/core.py index e120c6e81..0967da375 100644 --- a/software/control/core/core.py +++ b/software/control/core/core.py @@ -32,10 +32,11 @@ from typing import List, Tuple, Optional, Dict, Any, Callable, TypeVar -from queue import Queue +from queue import Queue, Empty from threading import Thread, Lock from pathlib import Path from datetime import datetime +from dataclasses import dataclass from enum import Enum from control.models import AcquisitionChannel import time @@ -93,6 +94,20 @@ def set_display_resolution_scaling(self, display_resolution_scaling): self._handler.set_display_resolution_scaling(display_resolution_scaling) +@dataclass +class _DefaultRecordingChannel: + """Stand-in for an AcquisitionChannel when no live channel is set during recording. + + save_image() and return_pseudo_colored_image() only consult `.name`; the other + fields keep frames.csv rows uniform when no channel is available. + """ + + name: str = "live" + exposure_time: float = 0.0 + analog_gain: float = 0.0 + illumination_intensity: float = 0.0 + + class ImageSaver(QObject): stop_recording = Signal() @@ -123,39 +138,32 @@ def set_channel_provider(self, provider): def process_queue(self): while True: - # stop the thread if stop signal is received if self.stop_signal_received: return - # process the queue try: - [image, frame_ID, timestamp] = self.queue.get(timeout=0.1) - self.image_lock.acquire(True) - folder_ID = int(self.counter / self.max_num_image_per_folder) - file_ID = int(self.counter % self.max_num_image_per_folder) - # create a new folder - if file_ID == 0: - utils.ensure_directory_exists(os.path.join(self.base_path, self.experiment_ID, str(folder_ID))) - - if image.dtype == np.uint16: - # need to use tiff when saving 16 bit images - saving_path = os.path.join( - self.base_path, self.experiment_ID, str(folder_ID), str(file_ID) + "_" + str(frame_ID) + ".tiff" - ) - iio.imwrite(saving_path, image) - else: - saving_path = os.path.join( - self.base_path, - self.experiment_ID, - str(folder_ID), - str(file_ID) + "_" + str(frame_ID) + "." + self.image_format, + image, frame_id, timestamp = self.queue.get(timeout=0.1) + except Empty: + continue + try: + with self.image_lock: + folder_id = self.counter // self.max_num_image_per_folder + file_id = self.counter % self.max_num_image_per_folder + save_dir = os.path.join(self.base_path, self.experiment_ID, str(folder_id)) + if file_id == 0: + utils.ensure_directory_exists(save_dir) + + channel = (self._channel_provider() if self._channel_provider else None) or _DefaultRecordingChannel() + utils_acquisition.save_image( + image=image, + file_id=str(file_id), + save_directory=save_dir, + config=channel, + is_color=image.ndim == 3, ) - cv2.imwrite(saving_path, image) - - self.counter = self.counter + 1 + self.counter += 1 self.queue.task_done() - self.image_lock.release() except: - pass + pass # logging in Task 5 def enqueue(self, image, frame_ID, timestamp): try: diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index 66f137bbf..5ca412dd0 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -2439,7 +2439,11 @@ def test_frame_is_persisted_to_disk(self, qtbot, recording_widget): qtbot.waitUntil(lambda: image_saver.counter > 0, timeout=2000) exp_dir = Path(image_saver.base_path) / image_saver.experiment_ID - assert any(p.is_file() for p in exp_dir.rglob("*")), f"No image saved under {exp_dir}" + files = [p for p in exp_dir.rglob("*") if p.is_file()] + assert files, f"No image saved under {exp_dir}" + # No channel provider was set, so save_image gets the default "live" sentinel. + # Filename comes from utils_acquisition.get_image_filepath: _. + assert any("live" in p.stem for p in files), f"Expected 'live' in filename; got {[p.name for p in files]}" class TestWarningErrorWidgetErrorExemptionWithDroppedCount: From d57233c0ed544c5379335671d181e9d5b92e5e7c Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 17:19:52 -0700 Subject: [PATCH 06/12] feat(ImageSaver): emit frames.csv sidecar with per-frame metadata 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) --- software/control/core/core.py | 52 ++++++++++++++++++++++---- software/tests/control/test_widgets.py | 36 ++++++++++++++++++ 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/software/control/core/core.py b/software/control/core/core.py index 0967da375..31db166f7 100644 --- a/software/control/core/core.py +++ b/software/control/core/core.py @@ -40,6 +40,7 @@ from enum import Enum from control.models import AcquisitionChannel import time +import csv import itertools import json import math @@ -126,6 +127,9 @@ def __init__(self, image_format=Acquisition.IMAGE_FORMAT): self.recording_start_time = 0 self.recording_time_limit = -1 self._channel_provider: Optional[Callable[[], Optional[Any]]] = None + self._csv_file = None + self._csv_writer = None + self._dropped_count = 0 def set_channel_provider(self, provider): """Register a callable returning the active live channel (or None). @@ -160,6 +164,27 @@ def process_queue(self): config=channel, is_color=image.ndim == 3, ) + + if self._csv_writer is not None: + rel_path = os.path.join( + str(folder_id), + os.path.basename( + utils_acquisition.get_image_filepath("", str(file_id), channel.name, image.dtype) + ), + ) + self._csv_writer.writerow( + [ + frame_id, + datetime.fromtimestamp(timestamp).isoformat(), + getattr(channel, "name", "live"), + getattr(channel, "exposure_time", 0.0), + getattr(channel, "analog_gain", 0.0), + getattr(channel, "illumination_intensity", 0.0), + rel_path, + ] + ) + self._csv_file.flush() + self.counter += 1 self.queue.task_done() except: @@ -184,24 +209,37 @@ def set_recording_time_limit(self, time_limit): def start_new_experiment(self, experiment_ID, add_timestamp=True): if add_timestamp: - # generate unique experiment ID self.experiment_ID = experiment_ID + "_" + datetime.now().strftime("%Y-%m-%d_%H-%M-%S.%f") else: self.experiment_ID = experiment_ID self.recording_start_time = time.time() - # create a new folder + self.counter = 0 + self._dropped_count = 0 + + experiment_dir = os.path.join(self.base_path, self.experiment_ID) try: - utils.ensure_directory_exists(os.path.join(self.base_path, self.experiment_ID)) - # to do: save configuration + utils.ensure_directory_exists(experiment_dir) except: - pass - # reset the counter - self.counter = 0 + pass # logging in Task 5 + + 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( + ["frame_id", "timestamp_iso", "channel", "exposure_ms", "gain", "illumination_intensity", "file"] + ) + self._csv_file.flush() def close(self): self.queue.join() self.stop_signal_received = True self.thread.join() + if self._csv_file is not None: + try: + self._csv_file.close() + finally: + self._csv_file = None + self._csv_writer = None class ImageSaver_Tracking(QObject): diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index 5ca412dd0..7e20f0fb8 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -2403,6 +2403,42 @@ def test_channel_provider_can_be_set_and_cleared(self, recording_widget): image_saver.set_channel_provider(None) assert image_saver._channel_provider is None + def test_frames_csv_written_with_metadata(self, qtbot, recording_widget): + widget, _, image_saver = recording_widget + + class _Stub: + name = "488 nm" + exposure_time = 25.0 + analog_gain = 1.5 + illumination_intensity = 80.0 + + image_saver.set_channel_provider(lambda: _Stub()) + + widget.lineEdit_experimentID.setText("exp") + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + + # Enqueue directly so streamHandler's save-FPS throttle doesn't drop frames. + img = np.zeros((16, 16), dtype=np.uint8) + for i in range(3): + image_saver.enqueue(img, i * 30, 1000.0 + i) + + qtbot.waitUntil(lambda: image_saver.counter == 3, timeout=2000) + + csv_path = Path(image_saver.base_path) / image_saver.experiment_ID / "frames.csv" + assert csv_path.is_file() + + import csv as _csv + + with csv_path.open() as f: + rows = list(_csv.DictReader(f)) + assert len(rows) == 3 + assert [r["frame_id"] for r in rows] == ["0", "30", "60"] + assert rows[0]["channel"] == "488 nm" + assert float(rows[0]["exposure_ms"]) == 25.0 + assert float(rows[0]["gain"]) == 1.5 + assert float(rows[0]["illumination_intensity"]) == 80.0 + assert rows[0]["file"].endswith(".tiff") or rows[0]["file"].endswith(".bmp") + def test_record_without_browse_starts_recording(self, qtbot, recording_widget): widget, _, image_saver = recording_widget widget.lineEdit_experimentID.setText("exp") From 2e0fd6df654445bde3d6f3e7ec1d0072f239c52b Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 17:25:16 -0700 Subject: [PATCH 07/12] feat(ImageSaver): replace bare-except/print with squid.logging - 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) --- software/control/core/core.py | 48 +++++++++++++++++++------- software/tests/control/test_widgets.py | 23 ++++++++++++ 2 files changed, 58 insertions(+), 13 deletions(-) diff --git a/software/control/core/core.py b/software/control/core/core.py index 31db166f7..5720f1eb3 100644 --- a/software/control/core/core.py +++ b/software/control/core/core.py @@ -95,6 +95,9 @@ def set_display_resolution_scaling(self, display_resolution_scaling): self._handler.set_display_resolution_scaling(display_resolution_scaling) +log = squid.logging.get_logger(__name__) + + @dataclass class _DefaultRecordingChannel: """Stand-in for an AcquisitionChannel when no live channel is set during recording. @@ -130,6 +133,7 @@ def __init__(self, image_format=Acquisition.IMAGE_FORMAT): self._csv_file = None self._csv_writer = None self._dropped_count = 0 + self._last_queue_full_warning_ts = 0.0 def set_channel_provider(self, provider): """Register a callable returning the active live channel (or None). @@ -187,19 +191,26 @@ def process_queue(self): self.counter += 1 self.queue.task_done() - except: - pass # logging in Task 5 + 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}") - def enqueue(self, image, frame_ID, timestamp): + def enqueue(self, image, frame_id, timestamp): 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 + + if self.recording_time_limit > 0 and time.time() - self.recording_start_time >= self.recording_time_limit: + log.info(f"Auto-stopping: time limit reached ({self.recording_time_limit}s)") + self.stop_recording.emit() def set_base_path(self, path): self.base_path = path @@ -219,8 +230,9 @@ def start_new_experiment(self, experiment_ID, add_timestamp=True): experiment_dir = os.path.join(self.base_path, self.experiment_ID) try: utils.ensure_directory_exists(experiment_dir) - except: - pass # logging in Task 5 + except Exception as e: + log.error(f"Failed to create experiment directory {experiment_dir}: {e}") + raise csv_path = os.path.join(experiment_dir, "frames.csv") self._csv_file = open(csv_path, "w", newline="") @@ -230,6 +242,10 @@ def start_new_experiment(self, experiment_ID, add_timestamp=True): ) self._csv_file.flush() + log.info(f"Recording started: id={self.experiment_ID}, dir={experiment_dir}") + if self._channel_provider is None: + log.warning("channel_provider not set; frames tagged with default 'live' channel") + def close(self): self.queue.join() self.stop_signal_received = True @@ -240,6 +256,12 @@ def close(self): finally: self._csv_file = None self._csv_writer = None + if self.experiment_ID: + duration = time.time() - self.recording_start_time + log.info( + f"Recording stopped: frames_saved={self.counter}, " + f"dropped={self._dropped_count}, duration={duration:.1f}s" + ) class ImageSaver_Tracking(QObject): diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index 7e20f0fb8..9a79d5f04 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -2403,6 +2403,29 @@ def test_channel_provider_can_be_set_and_cleared(self, recording_widget): image_saver.set_channel_provider(None) assert image_saver._channel_provider is None + def test_directory_creation_failure_raises_and_logs(self, qtbot, recording_widget, caplog): + _, _, image_saver = recording_widget + with patch("control.utils.ensure_directory_exists", side_effect=OSError("mock disk error")): + with caplog.at_level(logging.ERROR): + with pytest.raises(OSError): + image_saver.start_new_experiment("doomed") + assert any("Failed to create experiment directory" in rec.message for rec in caplog.records) + + def test_queue_full_logs_warning_and_increments_dropped_count(self, qtbot, recording_widget, caplog): + widget, _, image_saver = recording_widget + widget.lineEdit_experimentID.setText("exp") + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + + # Block the saver thread by holding image_lock so the queue can't drain. + img = np.zeros((8, 8), dtype=np.uint8) + with image_saver.image_lock: + with caplog.at_level(logging.WARNING): + for frame_id in range(40): + image_saver.enqueue(img, frame_id, 0.0) + + assert image_saver._dropped_count > 0 + assert any("queue full" in rec.message.lower() for rec in caplog.records) + def test_frames_csv_written_with_metadata(self, qtbot, recording_widget): widget, _, image_saver = recording_widget From de09b42e2df12ba8423b963c0df16602ec54199b Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 17:29:44 -0700 Subject: [PATCH 08/12] feat(RecordingWidget): wire channel_provider from liveController 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) --- software/control/gui_hcs.py | 6 ++- software/control/widgets.py | 9 ++++- software/tests/control/test_widgets.py | 52 ++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/software/control/gui_hcs.py b/software/control/gui_hcs.py index 07414e56e..a7cd22b04 100644 --- a/software/control/gui_hcs.py +++ b/software/control/gui_hcs.py @@ -888,7 +888,11 @@ def load_widgets(self): self.emission_filter_wheel, self.liveController, config_repo=self.microscope.config_repo ) - self.recordingControlWidget = widgets.RecordingWidget(self.streamHandler, self.imageSaver) + self.recordingControlWidget = widgets.RecordingWidget( + self.streamHandler, + self.imageSaver, + channel_provider=lambda: self.liveController.currentConfiguration, + ) self.wellplateFormatWidget = widgets.WellplateFormatWidget( self.stage, self.navigationViewer, self.streamHandler, self.liveController ) diff --git a/software/control/widgets.py b/software/control/widgets.py index 0783a116a..9b2344a31 100644 --- a/software/control/widgets.py +++ b/software/control/widgets.py @@ -4410,10 +4410,11 @@ def update_displacement_um_display(self, displacement=None): class RecordingWidget(QFrame): - def __init__(self, streamHandler, imageSaver, main=None, *args, **kwargs): + def __init__(self, streamHandler, imageSaver, main=None, channel_provider=None, *args, **kwargs): super().__init__(*args, **kwargs) - self.imageSaver = imageSaver # for saving path control + self.imageSaver = imageSaver self.streamHandler = streamHandler + self._channel_provider = channel_provider self.add_components() self.setFrameStyle(QFrame.Panel | QFrame.Raised) @@ -4492,10 +4493,13 @@ def toggle_recording(self, pressed): if pressed: 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()) self.streamHandler.start_recording() else: self.streamHandler.stop_recording() + self.imageSaver.set_channel_provider(None) self.lineEdit_experimentID.setEnabled(True) self.btn_setSavingDir.setEnabled(True) @@ -4504,6 +4508,7 @@ def stop_recording(self): self.lineEdit_experimentID.setEnabled(True) self.btn_record.setChecked(False) self.streamHandler.stop_recording() + self.imageSaver.set_channel_provider(None) self.btn_setSavingDir.setEnabled(True) diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index 9a79d5f04..ecd0b3608 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -2403,6 +2403,58 @@ def test_channel_provider_can_be_set_and_cleared(self, recording_widget): image_saver.set_channel_provider(None) assert image_saver._channel_provider is None + def test_channel_change_mid_recording_reflected_in_filenames(self, qtbot, recording_widget): + widget, _, image_saver = recording_widget + + class _Stub: + def __init__(self, name): + self.name = name + self.exposure_time = 10.0 + self.analog_gain = 1.0 + self.illumination_intensity = 50.0 + + state = {"channel": _Stub("BF LED matrix full")} + image_saver.set_channel_provider(lambda: state["channel"]) + + widget.lineEdit_experimentID.setText("exp") + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + + img = np.zeros((8, 8), dtype=np.uint8) + image_saver.enqueue(img, 0, 0.0) + qtbot.waitUntil(lambda: image_saver.counter == 1, timeout=2000) + + state["channel"] = _Stub("488 nm") + image_saver.enqueue(img, 1, 1.0) + qtbot.waitUntil(lambda: image_saver.counter == 2, timeout=2000) + + exp_dir = Path(image_saver.base_path) / image_saver.experiment_ID + names = sorted(p.name for p in exp_dir.rglob("*") if p.suffix in (".tiff", ".bmp")) + assert any("BF_LED_matrix_full" in n for n in names), f"No BF file: {names}" + assert any("488_nm" in n for n in names), f"No 488 file: {names}" + + def test_widget_constructed_with_channel_provider(self, qtbot, tmp_path, monkeypatch): + """RecordingWidget accepts channel_provider kwarg and registers it on Record-press.""" + monkeypatch.setattr(control._def, "DEFAULT_SAVING_PATH", str(tmp_path)) + monkeypatch.setattr(control.widgets, "DEFAULT_SAVING_PATH", str(tmp_path)) + + stream_handler = core_module.QtStreamHandler(accept_new_frame_fn=lambda: True) + image_saver = core_module.ImageSaver() + stream_handler.packet_image_to_write.connect(image_saver.enqueue) + + sentinel = object() + provider = lambda: sentinel # noqa: E731 + + widget = RecordingWidget(stream_handler, image_saver, channel_provider=provider) + qtbot.addWidget(widget) + try: + widget.lineEdit_experimentID.setText("exp") + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + assert image_saver._channel_provider is provider + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + assert image_saver._channel_provider is None + finally: + image_saver.close() + def test_directory_creation_failure_raises_and_logs(self, qtbot, recording_widget, caplog): _, _, image_saver = recording_widget with patch("control.utils.ensure_directory_exists", side_effect=OSError("mock disk error")): From 83edde762d8fe814de0bc4e7c4d9bb994e5878f5 Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 17:30:34 -0700 Subject: [PATCH 09/12] style: black formatting on core/core.py --- software/control/core/core.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/software/control/core/core.py b/software/control/core/core.py index 5720f1eb3..20304405c 100644 --- a/software/control/core/core.py +++ b/software/control/core/core.py @@ -160,7 +160,9 @@ def process_queue(self): if file_id == 0: utils.ensure_directory_exists(save_dir) - channel = (self._channel_provider() if self._channel_provider else None) or _DefaultRecordingChannel() + channel = ( + self._channel_provider() if self._channel_provider else None + ) or _DefaultRecordingChannel() utils_acquisition.save_image( image=image, file_id=str(file_id), From de9db195f57c326a4c1ca0f1950e79e9f04858cd Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 18:08:25 -0700 Subject: [PATCH 10/12] =?UTF-8?q?fix(ImageSaver):=20address=20Copilot=20re?= =?UTF-8?q?view=20=E2=80=94=20lifecycle=20hygiene?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- software/control/core/core.py | 27 ++++++++++++----- software/control/widgets.py | 2 ++ software/tests/control/test_widgets.py | 41 ++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 7 deletions(-) diff --git a/software/control/core/core.py b/software/control/core/core.py index 20304405c..3323cf9f3 100644 --- a/software/control/core/core.py +++ b/software/control/core/core.py @@ -32,7 +32,7 @@ from typing import List, Tuple, Optional, Dict, Any, Callable, TypeVar -from queue import Queue, Empty +from queue import Queue, Empty, Full from threading import Thread, Lock from pathlib import Path from datetime import datetime @@ -192,17 +192,18 @@ def process_queue(self): self._csv_file.flush() self.counter += 1 - self.queue.task_done() 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}") + finally: + self.queue.task_done() def enqueue(self, image, frame_id, timestamp): try: self.queue.put_nowait([image, frame_id, timestamp]) - except Exception: + except Full: self._dropped_count += 1 now = time.time() if now - self._last_queue_full_warning_ts >= 1.0: @@ -221,6 +222,9 @@ def set_recording_time_limit(self, time_limit): self.recording_time_limit = time_limit def start_new_experiment(self, experiment_ID, add_timestamp=True): + # Defensively close any prior recording in case the caller didn't. + self.stop_experiment() + if add_timestamp: self.experiment_ID = experiment_ID + "_" + datetime.now().strftime("%Y-%m-%d_%H-%M-%S.%f") else: @@ -248,10 +252,12 @@ def start_new_experiment(self, experiment_ID, add_timestamp=True): if self._channel_provider is None: log.warning("channel_provider not set; frames tagged with default 'live' channel") - def close(self): - self.queue.join() - self.stop_signal_received = True - self.thread.join() + def stop_experiment(self): + """Finalize the current recording: close frames.csv and log a summary. + + Called by the widget on Stop and on time-limit auto-stop. Idempotent + so close() can call it again on app shutdown without double-logging. + """ if self._csv_file is not None: try: self._csv_file.close() @@ -264,6 +270,13 @@ def close(self): f"Recording stopped: frames_saved={self.counter}, " f"dropped={self._dropped_count}, duration={duration:.1f}s" ) + self.experiment_ID = "" + + def close(self): + self.queue.join() + self.stop_signal_received = True + self.thread.join() + self.stop_experiment() class ImageSaver_Tracking(QObject): diff --git a/software/control/widgets.py b/software/control/widgets.py index 9b2344a31..4877da185 100644 --- a/software/control/widgets.py +++ b/software/control/widgets.py @@ -4499,6 +4499,7 @@ def toggle_recording(self, pressed): self.streamHandler.start_recording() else: self.streamHandler.stop_recording() + self.imageSaver.stop_experiment() self.imageSaver.set_channel_provider(None) self.lineEdit_experimentID.setEnabled(True) self.btn_setSavingDir.setEnabled(True) @@ -4508,6 +4509,7 @@ def stop_recording(self): self.lineEdit_experimentID.setEnabled(True) self.btn_record.setChecked(False) self.streamHandler.stop_recording() + self.imageSaver.stop_experiment() self.imageSaver.set_channel_provider(None) self.btn_setSavingDir.setEnabled(True) diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index ecd0b3608..93bf50f26 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -2455,6 +2455,47 @@ def test_widget_constructed_with_channel_provider(self, qtbot, tmp_path, monkeyp finally: image_saver.close() + def test_close_does_not_hang_when_write_raises(self, qtbot, recording_widget): + """A write exception must still mark the queue item done; otherwise close() hangs on join().""" + widget, _, image_saver = recording_widget + widget.lineEdit_experimentID.setText("exp") + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + + with patch("control.utils_acquisition.save_image", side_effect=RuntimeError("boom")): + image_saver.enqueue(np.zeros((4, 4), dtype=np.uint8), 0, 0.0) + qtbot.waitUntil(lambda: image_saver.queue.empty(), timeout=2000) + + def test_multiple_recordings_close_previous_csv(self, qtbot, recording_widget): + """Calling start_new_experiment twice without close() must not leak the previous CSV handle.""" + widget, _, image_saver = recording_widget + widget.lineEdit_experimentID.setText("first") + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + first_csv = image_saver._csv_file + assert first_csv is not None and not first_csv.closed + + # Stop the first recording and start a second one + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + assert first_csv.closed, "Stopping the recording should close frames.csv" + + widget.lineEdit_experimentID.setText("second") + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + second_csv = image_saver._csv_file + assert second_csv is not None and second_csv is not first_csv + assert not second_csv.closed + + def test_stop_experiment_finalizes_csv_and_logs_summary(self, qtbot, recording_widget, caplog): + widget, _, image_saver = recording_widget + widget.lineEdit_experimentID.setText("exp") + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + image_saver.enqueue(np.zeros((4, 4), dtype=np.uint8), 0, 0.0) + qtbot.waitUntil(lambda: image_saver.counter == 1, timeout=2000) + + with caplog.at_level(logging.INFO): + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + + assert image_saver._csv_file is None + assert any("Recording stopped" in rec.message and "frames_saved=1" in rec.message for rec in caplog.records) + def test_directory_creation_failure_raises_and_logs(self, qtbot, recording_widget, caplog): _, _, image_saver = recording_widget with patch("control.utils.ensure_directory_exists", side_effect=OSError("mock disk error")): From ce4266008e07162e98481f4ae59b7e3c315034e0 Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 18:14:40 -0700 Subject: [PATCH 11/12] test: consolidate redundant RecordingWidget tests (13 -> 11) - 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) --- software/tests/control/test_widgets.py | 35 ++++++++------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index 93bf50f26..dd2fe224b 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -2395,14 +2395,6 @@ def test_default_saving_path_applied_on_construction(self, recording_widget, tmp assert widget.lineEdit_savingDir.text() == str(tmp_path) assert image_saver.base_path == str(tmp_path) - def test_channel_provider_can_be_set_and_cleared(self, recording_widget): - _, _, image_saver = recording_widget - provider = lambda: None - image_saver.set_channel_provider(provider) - assert image_saver._channel_provider is provider - image_saver.set_channel_provider(None) - assert image_saver._channel_provider is None - def test_channel_change_mid_recording_reflected_in_filenames(self, qtbot, recording_widget): widget, _, image_saver = recording_widget @@ -2465,37 +2457,32 @@ def test_close_does_not_hang_when_write_raises(self, qtbot, recording_widget): image_saver.enqueue(np.zeros((4, 4), dtype=np.uint8), 0, 0.0) qtbot.waitUntil(lambda: image_saver.queue.empty(), timeout=2000) - def test_multiple_recordings_close_previous_csv(self, qtbot, recording_widget): - """Calling start_new_experiment twice without close() must not leak the previous CSV handle.""" + def test_recording_lifecycle_finalizes_csv_and_starts_fresh(self, qtbot, recording_widget, caplog): + """Stopping a recording closes frames.csv, logs the summary, and a subsequent + recording opens a brand-new CSV (no file-descriptor leak across sessions).""" widget, _, image_saver = recording_widget + widget.lineEdit_experimentID.setText("first") qtbot.mouseClick(widget.btn_record, Qt.LeftButton) first_csv = image_saver._csv_file assert first_csv is not None and not first_csv.closed - # Stop the first recording and start a second one - qtbot.mouseClick(widget.btn_record, Qt.LeftButton) - assert first_csv.closed, "Stopping the recording should close frames.csv" - - widget.lineEdit_experimentID.setText("second") - qtbot.mouseClick(widget.btn_record, Qt.LeftButton) - second_csv = image_saver._csv_file - assert second_csv is not None and second_csv is not first_csv - assert not second_csv.closed - - def test_stop_experiment_finalizes_csv_and_logs_summary(self, qtbot, recording_widget, caplog): - widget, _, image_saver = recording_widget - widget.lineEdit_experimentID.setText("exp") - qtbot.mouseClick(widget.btn_record, Qt.LeftButton) image_saver.enqueue(np.zeros((4, 4), dtype=np.uint8), 0, 0.0) qtbot.waitUntil(lambda: image_saver.counter == 1, timeout=2000) with caplog.at_level(logging.INFO): qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + assert first_csv.closed, "Stopping should close frames.csv" assert image_saver._csv_file is None assert any("Recording stopped" in rec.message and "frames_saved=1" in rec.message for rec in caplog.records) + widget.lineEdit_experimentID.setText("second") + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + second_csv = image_saver._csv_file + assert second_csv is not None and second_csv is not first_csv + assert not second_csv.closed + def test_directory_creation_failure_raises_and_logs(self, qtbot, recording_widget, caplog): _, _, image_saver = recording_widget with patch("control.utils.ensure_directory_exists", side_effect=OSError("mock disk error")): From f8d44c5d9ea8bd6ccb9cd8e629b2617072ebfcd2 Mon Sep 17 00:00:00 2001 From: You Yan Date: Thu, 7 May 2026 20:19:46 -0700 Subject: [PATCH 12/12] fix(ImageSaver): restore fast write path + drain queue on stop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 //, 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) --- software/control/core/core.py | 43 +++++++++++++++----------- software/tests/control/test_widgets.py | 5 ++- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/software/control/core/core.py b/software/control/core/core.py index 3323cf9f3..0fd46cf3d 100644 --- a/software/control/core/core.py +++ b/software/control/core/core.py @@ -163,21 +163,20 @@ def process_queue(self): channel = ( self._channel_provider() if self._channel_provider else None ) or _DefaultRecordingChannel() - utils_acquisition.save_image( - image=image, - file_id=str(file_id), - save_directory=save_dir, - config=channel, - is_color=image.ndim == 3, + # Use get_image_filepath for channel-aware naming consistency with the + # multipoint pipeline, but write directly with cv2/iio for speed — + # save_image's imageio.imwrite path is ~10x slower than cv2.imwrite for + # uint8 BMPs and turns the saver into the FPS bottleneck. + saving_path = utils_acquisition.get_image_filepath( + save_dir, str(file_id), channel.name, image.dtype ) + if image.dtype == np.uint16: + iio.imwrite(saving_path, image) + else: + cv2.imwrite(saving_path, image) if self._csv_writer is not None: - rel_path = os.path.join( - str(folder_id), - os.path.basename( - utils_acquisition.get_image_filepath("", str(file_id), channel.name, image.dtype) - ), - ) + rel_path = os.path.relpath(saving_path, os.path.join(self.base_path, self.experiment_ID)) self._csv_writer.writerow( [ frame_id, @@ -189,7 +188,9 @@ def process_queue(self): rel_path, ] ) - self._csv_file.flush() + # No per-row flush: it's an fsync per frame on most filesystems + # and was the second-largest contributor to the FPS regression. + # stop_experiment() flushes via close(). self.counter += 1 except OSError as e: @@ -253,11 +254,18 @@ def start_new_experiment(self, experiment_ID, add_timestamp=True): log.warning("channel_provider not set; frames tagged with default 'live' channel") def stop_experiment(self): - """Finalize the current recording: close frames.csv and log a summary. + """Finalize the current recording: drain the queue, close frames.csv, log summary. - Called by the widget on Stop and on time-limit auto-stop. Idempotent - so close() can call it again on app shutdown without double-logging. + Called by the widget on Stop and on time-limit auto-stop. Idempotent. + Drains the queue first so any buffered frames land in the current + experiment dir before experiment_ID is cleared — without that, the + saver thread races and writes to a non-existent directory. """ + # Block until the saver thread has processed every buffered frame. + # Safe because the caller has already stopped the streamHandler, so no + # new items can be enqueued; queue.task_done() runs in finally so a + # write exception cannot leave us hanging here. + self.queue.join() if self._csv_file is not None: try: self._csv_file.close() @@ -273,10 +281,9 @@ def stop_experiment(self): self.experiment_ID = "" def close(self): - self.queue.join() + self.stop_experiment() self.stop_signal_received = True self.thread.join() - self.stop_experiment() class ImageSaver_Tracking(QObject): diff --git a/software/tests/control/test_widgets.py b/software/tests/control/test_widgets.py index dd2fe224b..9c8c0c29a 100644 --- a/software/tests/control/test_widgets.py +++ b/software/tests/control/test_widgets.py @@ -2519,6 +2519,7 @@ class _Stub: widget.lineEdit_experimentID.setText("exp") qtbot.mouseClick(widget.btn_record, Qt.LeftButton) + exp_id = image_saver.experiment_ID # stop_experiment will clear it # Enqueue directly so streamHandler's save-FPS throttle doesn't drop frames. img = np.zeros((16, 16), dtype=np.uint8) @@ -2526,8 +2527,10 @@ class _Stub: image_saver.enqueue(img, i * 30, 1000.0 + i) qtbot.waitUntil(lambda: image_saver.counter == 3, timeout=2000) + # Stop the recording so frames.csv is flushed and closed. + qtbot.mouseClick(widget.btn_record, Qt.LeftButton) - csv_path = Path(image_saver.base_path) / image_saver.experiment_ID / "frames.csv" + csv_path = Path(image_saver.base_path) / exp_id / "frames.csv" assert csv_path.is_file() import csv as _csv