Skip to content

feat: Add hardware trigger mode setting to Settings > Advanced #503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hongquanli wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: Add hardware trigger mode setting to Settings > Advanced #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd2decf
feat: Add hardware trigger mode setting to Settings > Advanced
hongquanli Mar 1, 2026
6d0ab9d
fix: Clamp INI trigger mode value and use HardwareTriggerMode constants
hongquanli Mar 1, 2026
8ec8c60
refactor: Convert HardwareTriggerMode to IntEnum for proper validation
hongquanli Mar 1, 2026
3e14022
fix: Add logging for invalid INI values and build combo from enum
hongquanli Mar 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions software/control/_def.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import json
import csv
import squid.logging
from enum import Enum, auto
from enum import Enum, IntEnum, auto

log = squid.logging.get_logger(__name__)

Expand Down Expand Up @@ -1107,7 +1107,7 @@ class SOFTWARE_POS_LIMIT:
DISPLAY_TOUPCAMER_BLACKLEVEL_SETTINGS = False


class HardwareTriggerMode:
class HardwareTriggerMode(IntEnum):
EDGE = 0 # Fixed pulse width (TRIGGER_PULSE_LENGTH_us)
LEVEL = 1 # Variable pulse width (illumination_on_time)

Expand Down
3 changes: 2 additions & 1 deletion software/control/camera_toupcam.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
CameraFrame,
)
from squid.config import CameraConfig, ToupcamCameraModel
import control._def
from control._def import *

Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern as in live_controller.py: this file imports control._def for runtime access but also keeps from control._def import *, which pulls a stale HARDWARE_TRIGGER_MODE into the local namespace. Consider switching away from the wildcard import (or otherwise preventing accidental use of the local binding) to avoid reintroducing the original bug later.

Suggested change
# Avoid using a potentially stale HARDWARE_TRIGGER_MODE imported via wildcard.
# The authoritative, runtime-updated value should be accessed as control._def.HARDWARE_TRIGGER_MODE.
if "HARDWARE_TRIGGER_MODE" in globals():
del HARDWARE_TRIGGER_MODE

Copilot uses AI. Check for mistakes.
import threading
Expand Down Expand Up @@ -833,7 +834,7 @@ def _set_acquisition_mode_imp(self, acquisition_mode: CameraAcquisitionMode):
self._camera.put_Option(toupcam.TOUPCAM_OPTION_TRIGGER, trigger_option_value)

if acquisition_mode == CameraAcquisitionMode.HARDWARE_TRIGGER:
if HARDWARE_TRIGGER_MODE == HardwareTriggerMode.LEVEL:
if control._def.HARDWARE_TRIGGER_MODE == HardwareTriggerMode.LEVEL:
try:
self._camera.put_Option(toupcam.TOUPCAM_OPTION_TRIGGER, 2)
except toupcam.HRESULTException as ex:
Expand Down
3 changes: 2 additions & 1 deletion software/control/core/live_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from control.microcontroller import Microcontroller
from squid.abc import CameraAcquisitionMode, AbstractCamera

import control._def
from control._def import *
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file now relies on control._def.HARDWARE_TRIGGER_MODE for runtime updates, but still does from control._def import *, which also imports a stale HARDWARE_TRIGGER_MODE name into the local namespace. To reduce the chance of future regressions (someone accidentally using the local binding again), consider replacing the wildcard import with explicit imports (or relying solely on the module import) in this module.

Suggested change
from control._def import *
from control._def import TriggerMode

Copilot uses AI. Check for mistakes.
from control.core.config.utils import apply_confocal_override
from control.models import merge_channel_configs
Expand Down Expand Up @@ -428,7 +429,7 @@ def set_trigger_mode(self, mode):
if self.is_live and self.use_internal_timer_for_hardware_trigger:
self._start_triggerred_acquisition()

self.microscope.low_level_drivers.microcontroller.set_trigger_mode(HARDWARE_TRIGGER_MODE)
self.microscope.low_level_drivers.microcontroller.set_trigger_mode(control._def.HARDWARE_TRIGGER_MODE)

if mode == TriggerMode.CONTINUOUS:
if (self.trigger_mode == TriggerMode.SOFTWARE) or (
Expand Down
2 changes: 1 addition & 1 deletion software/control/gui_hcs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,7 @@ def setup_hardware(self, skip_init: bool = False):
if DEFAULT_TRIGGER_MODE == TriggerMode.HARDWARE:
print("Setting acquisition mode to HARDWARE_TRIGGER")
self.camera.set_acquisition_mode(squid.abc.CameraAcquisitionMode.HARDWARE_TRIGGER)
self.microcontroller.set_trigger_mode(HARDWARE_TRIGGER_MODE)
self.microcontroller.set_trigger_mode(control._def.HARDWARE_TRIGGER_MODE)
else:
self.camera.set_acquisition_mode(squid.abc.CameraAcquisitionMode.SOFTWARE_TRIGGER)
self.camera.add_frame_callback(self.streamHandler.get_frame_callback())
Expand Down
34 changes: 34 additions & 0 deletions software/control/widgets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1346,6 +1346,28 @@ def _create_advanced_tab(self):
self.illumination_factor.setValue(self._get_config_float("GENERAL", "illumination_intensity_factor", 0.6))
hw_layout.addRow("Illumination Intensity Factor:", self.illumination_factor)

self.hw_trigger_mode_combo = QComboBox()
for member in HardwareTriggerMode:
self.hw_trigger_mode_combo.addItem(member.name.title(), member.value)
self.hw_trigger_mode_combo.setToolTip(
"Edge: Fixed pulse width (TRIGGER_PULSE_LENGTH_us)\n" "Level: Variable pulse width (illumination_on_time)"
)
try:
hw_trigger_value = HardwareTriggerMode(self._get_config_int("GENERAL", "hardware_trigger_mode", 0))
except ValueError:
Comment on lines +1356 to +1357
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new Advanced > Hardware Trigger Mode control doesn’t appear to have widget-test coverage (visibility for Toupcam/Tucsen vs hidden otherwise, persistence to INI, and _def.HARDWARE_TRIGGER_MODE update). There is already a comprehensive PreferencesDialog test suite (e.g. software/tests/control/test_preferences_dialog.py)—adding a small set of tests for this new setting would help prevent regressions.

Suggested change
hw_trigger_value = HardwareTriggerMode(self._get_config_int("GENERAL", "hardware_trigger_mode", 0))
except ValueError:
hw_trigger_value = HardwareTriggerMode(
self._get_config_int("GENERAL", "hardware_trigger_mode", 0)
)
except (ValueError, TypeError):
logger.warning(
"Invalid 'hardware_trigger_mode' value in config; defaulting to EDGE.",
exc_info=True,
)

Copilot uses AI. Check for mistakes.
raw = self._get_config_int("GENERAL", "hardware_trigger_mode", 0)
logger.warning("Invalid hardware_trigger_mode=%d in INI, defaulting to EDGE", raw)
hw_trigger_value = HardwareTriggerMode.EDGE
self.hw_trigger_mode_combo.setCurrentIndex(hw_trigger_value)
self.hw_trigger_mode_label = QLabel("Hardware Trigger Mode:")
hw_layout.addRow(self.hw_trigger_mode_label, self.hw_trigger_mode_combo)

# Only show for cameras that support level trigger
camera_type = self._get_config_value("GENERAL", "camera_type", "Default")
supports_level_trigger = camera_type in ("Toupcam", "Tucsen")
self.hw_trigger_mode_label.setVisible(supports_level_trigger)
self.hw_trigger_mode_combo.setVisible(supports_level_trigger)

hw_group.content.addLayout(hw_layout)
layout.addWidget(hw_group)

Expand Down Expand Up @@ -1837,6 +1859,8 @@ def _apply_settings(self) -> bool:
self.config.set("GENERAL", "led_matrix_g_factor", str(self.led_g_factor.value()))
self.config.set("GENERAL", "led_matrix_b_factor", str(self.led_b_factor.value()))
self.config.set("GENERAL", "illumination_intensity_factor", str(self.illumination_factor.value()))
self.config.set("GENERAL", "hardware_trigger_mode", str(self.hw_trigger_mode_combo.currentIndex()))
control._def.HARDWARE_TRIGGER_MODE = HardwareTriggerMode(self.hw_trigger_mode_combo.currentIndex())

Comment on lines +1863 to 1864
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

control._def.HARDWARE_TRIGGER_MODE is updated before the config file write succeeds. If saving fails (caught later), the app's runtime trigger mode will still change even though the user was told the settings could not be saved. Consider moving this runtime update into _apply_live_settings() (which runs after a successful write) or otherwise only mutating _def after the file save completes.

Suggested change
control._def.HARDWARE_TRIGGER_MODE = HardwareTriggerMode(self.hw_trigger_mode_combo.currentIndex())

Copilot uses AI. Check for mistakes.
# Advanced - Development Settings
self.config.set(
Expand Down Expand Up @@ -2202,6 +2226,16 @@ def _get_changes(self):
if not self._floats_equal(old_val, new_val):
changes.append(("Illumination Intensity Factor", str(old_val), str(new_val), False))

try:
old_mode = HardwareTriggerMode(self._get_config_int("GENERAL", "hardware_trigger_mode", 0))
except ValueError:
raw = self._get_config_int("GENERAL", "hardware_trigger_mode", 0)
logger.warning("Invalid hardware_trigger_mode=%d in INI, defaulting to EDGE", raw)
old_mode = HardwareTriggerMode.EDGE
new_mode = HardwareTriggerMode(self.hw_trigger_mode_combo.currentIndex())
if old_mode != new_mode:
changes.append(("Hardware Trigger Mode", old_mode.name.title(), new_mode.name.title(), False))

# Advanced - Development Settings
# Enable/disable requires restart (for warning banner/dialog), but speed/compression
# take effect on next acquisition since each acquisition starts a fresh subprocess
Expand Down