fix(firmware): don't set illumination_is_on from the strobe ISR (next-channel bleed) — v1.5 - #619
Open
Alpaca233 wants to merge 7 commits into
Conversation
…-channel bleed) — v1.5 In hardware-triggered multi-channel acquisitions the next channel's laser could come on during the current frame's readout, so each image carried a faint copy of the following channel (gradient in the last-read rows, intermittent). Root cause: ISR_strobeTimer set illumination_is_on = true at strobe start. The host sleeps exactly exposure + strobe_delay after the trigger and then sends SET_ILLUMINATION for the next channel; if that lands while the strobe is still on, set_illumination() sees the flag and calls turn_on_illumination(), energizing the new port immediately. The v1.3 strobe end then turns off only the latched port and, because the source changed, leaves the flag true, so the new port stays on through readout until its own strobe end. Fix: the ISR no longer sets the flag. It still clears it at strobe end when the strobe extinguished the host's own source (v1.3/v1.4 behaviour), so HW-triggered live view stays strobe-only after the first frame and intensity changes in live remain DAC-only. The flag can now only become true through TURN_ON_ILLUMINATION, which the acquisition loop never sends in hardware-trigger mode, so a strobe can no longer be the reason a second port is lit. The Cephla-Lab#549 latch guarantee (the ISR always extinguishes the port it lit) is unchanged. Bump firmware 1.4 -> 1.5. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…omments
The strobe begin and end sequences were byte-identical across the <=30 ms
and >30 ms ISR branches, and the v1.5 invariant ("begin never writes the
host-owned illumination_is_on; end owns the guarded clear") was stated in
three comment blocks instead of in code. Extract static inline
strobe_begin()/strobe_end() so the rule has one home, and cut the
duplicated prose (ISR preamble, per-branch clear comments, globals.h and
constants.h entries) down to one statement next to the code and one next
to the declaration. Behavior-identical; builds clean, native tests 80/80.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e owns the light set_microscope_mode()'s live-restart path unconditionally did turn_off(old) / turn_on(new) around a channel switch — a leftover from 2020 software-triggered live view. In hardware-trigger mode with an MCU-gated source (TTL laser shutter, MCU LED matrix) the strobe owns the light, and this TURN_ON is the one remaining path that holds firmware illumination_is_on true across a switch: the light stays continuously on until the first strobe end, and SET_ILLUMINATION can re-light a port mid-strobe (the host half of the next-channel-bleed fix; firmware v1.5 closes the ISR half). Keep the toggle when the trigger mode is not HARDWARE, when the host is deliberately holding illumination on (manual toggle — captured before turn_off clears it), or when the strobe can't gate the light (software shutter, e.g. LDI PC mode; external SciMicroscopy LED array), where this call is the only thing that turns the lamp on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
/simplify pass: the set_microscope_mode comment restated the helper docstring (keep only the capture-ordering fact + pointer); two per-test docstrings restated the module docstring; the five tests shared one assert pair, now _assert_toggled(). No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes hardware-triggered channel bleed by separating host illumination state from ISR strobe state.
Changes:
- Refactors firmware strobe begin/end handling and bumps firmware to v1.5.
- Avoids unnecessary host illumination toggles in hardware-trigger mode.
- Adds host-side illumination-switching tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
software/control/core/live_controller.py |
Adjusts live channel-switch illumination behavior. |
software/tests/control/test_live_controller_hw_illumination.py |
Tests illumination toggling scenarios. |
firmware/controller/src/functions.cpp |
Refactors strobe handling and removes ISR flag writes. |
firmware/controller/src/globals.h |
Clarifies illumination flag ownership. |
firmware/controller/src/constants.h |
Bumps firmware version to 1.5. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+106
to
+107
| if self.trigger_mode != TriggerMode.HARDWARE: | ||
| return False |
Collaborator
Author
There was a problem hiding this comment.
[Claude Code] Fixed in 5f7ae36 - _strobe_owns_light() now requires firmware >= (1, 3) (older controllers keep the legacy toggle unchanged), and on firmware < (1, 5) set_microscope_mode() additionally waits out the last hardware trigger's strobe window before sending SET_ILLUMINATION, which removes the trigger condition of the <= 1.2 stuck-port race in both live view and acquisitions. Covered by new tests (firmware (1, 2) keeps the toggle; the wait engages on (1, 4) and not on (1, 5)).
…he strobe window on old firmware Copilot review: skipping the legacy TURN_OFF(old)/TURN_ON(new) on firmware <= 1.2 widens that firmware's stuck-port race (unlatched ISR: SET_ILLUMINATION mid-strobe makes the strobe end turn off the NEW port and strand the old one). Two changes: - _strobe_owns_light() now also requires firmware >= (1, 3): on older controllers set_microscope_mode() keeps today's toggle, byte-for-byte. - On firmware < (1, 5), set_microscope_mode() waits out the last hardware trigger's strobe window (strobe delay + exposure + STROBE_GUARD_MARGIN_MS) before changing the illumination source, so SET_ILLUMINATION can never land mid-strobe: that removes the trigger condition of both the <= 1.2 stuck-port race and the 1.3/1.4 next-channel re-light, in live view and acquisitions alike. Trigger times are recorded via note_hardware_trigger_sent() at every HW trigger site (live timer, snap, worker single/RGB captures). v1.5+ controllers skip the wait entirely; in acquisitions the wait is normally absorbed by the existing wait-for-frame anyway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y HW-trigger sites Adversarial review of the guard commit found the wait ran BEFORE the live timer was stopped and read _strobe_clear_at once, so a timer-thread trigger could still start a strobe behind its back; and it was gated on the CURRENT trigger mode, missing a strobe still in flight right after leaving HARDWARE mode. - set_microscope_mode now stops the timer first, then waits via _wait_for_strobe_clear(): gated on firmware (< 1.5) only, taking a new _hw_trigger_send_lock (also held around the trigger sends in trigger_acquisition and snap) to flush an in-flight send, and looping until the recorded window has actually passed. - note_hardware_trigger_sent() added at the two remaining HW-trigger sites: Microscope.acquire_image and the contrast-AF worker. - update_illumination() (intensity slider) stays unguarded on purpose: it never changes the source, and a same-source re-light mid-strobe is harmless on every firmware; documented on _wait_for_strobe_clear(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, comment dedup /simplify pass over the strobe-guard commits: - Microcontroller gains strobe_isr_latches_source() (>= 1.3) and illumination_change_safe_during_strobe() (>= 1.5) next to supports_multi_port(), replacing raw version tuples in LiveController. - note_hardware_trigger_sent() self-guards (HARDWARE mode + firmware), making it a no-op on v1.5+ so the per-trigger camera round trip (2 live DCAM reads on Hamamatsu) disappears on current firmware. - New LiveController.send_camera_trigger() is the one seam that locks, sends, and records; trigger_acquisition, snap and the worker's two sites use it, deleting four duplicated trigger+note blocks and their mode conditionals. (Reviewed alternative — recording inside Microcontroller.send_hardware_trigger — rejected: SimulatedCamera never reaches hw_trigger_fn, so the guard would go dead in simulation, and 2 of 6 sites bypass the camera funnel.) - Firmware rationale now lives once on STROBE_GUARD_MARGIN_MS (moved out of the middle of the watchdog constants); other comments point at it. update_illumination() documents its intensity-only exemption; the worker's NL5 branch documents why it records no window. - Tests: module-level time import, dead fake-camera stub dropped, new coverage for the v1.5 no-op and the send_camera_trigger seam. No behavior change on any firmware. Co-Authored-By: Claude Fable 5 <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.
Problem
In hardware-triggered multi-channel acquisitions, a frame can carry a faint copy of the next channel's signal: a gradient that is strongest in the rows read out last (top of the image on a vertically flipped camera), sometimes uniform, intermittent. Reported on a Squid+ with a Kinetix in level-trigger mode (same exposure on every channel), where it was reproducible enough to look like "channel mixing".
Root cause
ISR_strobeTimer()setillumination_is_on = trueat strobe start. The host's acquisition loop (MultiPointWorker.acquire_camera_image) sleeps exactlyexposure + strobe_delayafter writing the trigger and then immediately sendsSET_ILLUMINATIONfor the next channel. The strobe endsexposure + strobe_delayafter the trigger arrives, so the margin between the two is USB/loop jitter, on the order of a millisecond, in either direction. WhenSET_ILLUMINATIONlands while the strobe is still on,set_illumination()sees the flag and callsturn_on_illumination()→ the next channel's port is energized immediately. The v1.3 strobe end (#549) then turns off only the latched port and, seeing the source has changed, leaves the flag true — so the next channel's laser stays on through the current frame's readout, the inter-frame gap and the next frame's reset sweep, until its own strobe end.Before v1.3 the same race had the opposite outcome (the strobe end turned off the new source and stranded the old one); v1.3 fixed the stranding but preserved the accidental re-light.
Fix
The ISR no longer sets the flag. It still clears it at strobe end when the strobe extinguished the host's own source (unchanged from v1.3/v1.4), so:
SET_ILLUMINATIONduring a strobe is DAC/source-only, no re-light; the strobe end turns off the latched port. Bleed gone.TURN_OFF(old)→ strobe start →SET_ILLUMINATION(new)→TURN_ON(new)→ strobe end): identical to v1.4 — the latched old port is extinguished, the host-commanded new port stays on until its own strobe end.SET_ILLUMINATIONs are DAC-only. (A "never touch the flag" variant was considered and rejected: it would leave the flag true after a live channel switch, and every intensity-slider tick would then re-light the laser continuously until the next strobe end.)<= 30 msdelayMicrosecondsbranch: unchanged.Behavioural change is two deleted
illumination_is_on = true;writes (both strobe-start branches). A follow-up commit folds the byte-identical strobe begin/end sequences of the two ISR branches intostrobe_begin()/strobe_end()helpers so the invariant (begin never writes the host flag; end owns the guarded clear) is structural rather than comment-enforced — behavior-identical. Plus the version bump 1.4 → 1.5.Host side (same fix, second half)
LiveController.set_microscope_mode()'s live-restart path unconditionally toggled illumination around a channel switch — a leftover from 2020 software-triggered live view, and the one remaining path that holdsillumination_is_ontrue across a switch in hardware-trigger mode. It now skips the toggle when the strobe owns an MCU-gated light (_strobe_owns_light()), and keeps it for software trigger, for a host-held manual toggle, and for sources the strobe can't gate (software shutter / SciMicroscopy LED array, where this call is the only light switch). Covered bytests/control/test_live_controller_hw_illumination.py.Per Copilot's review, the skip is firmware-gated:
_strobe_owns_light()requires the MCU to report firmware >= (1, 3) (on an unlatched <= 1.2 ISR, dropping the legacyTURN_OFF(old)would widen that firmware's stuck-port race), so older controllers keep today's behaviour byte-for-byte. Additionally, on firmware < (1, 5)set_microscope_mode()waits out the last hardware trigger's strobe window (strobe delay + exposure + STROBE_GUARD_MARGIN_MS) before changing the illumination source —SET_ILLUMINATIONcan then never land mid-strobe, which removes the trigger condition of both old-firmware races in live view and acquisitions (trigger times recorded vianote_hardware_trigger_sent()at every HW trigger site). v1.5+ controllers skip the wait entirely; in acquisitions it is normally absorbed by the existing wait-for-frame. An adversarial review of the guard hardened it further: the live timer is stopped before the wait, trigger sends and the wait share_hw_trigger_send_lock(flushing an in-flight timer-thread send) and the wait loops until the window has actually passed; the wait is gated on firmware only, not the current trigger mode (a strobe can still be in flight right after leaving HARDWARE mode); andnote_hardware_trigger_sent()also coversMicroscope.acquire_imageand the contrast-AF worker. On pre-1.5 firmware a live channel switch may block the GUI thread for up to one frame time — accepted as the cost of correctness there. The intensity-onlyupdate_illumination()path is deliberately unguarded (it never changes the source; a same-source re-light mid-strobe is harmless).Testing
pio run -e teensy41— builds clean (warnings pre-existing inTMC4361A.cpp/ Teensy core),firmware.hexproducedpio test -e native— 80/80pytest tests/control/test_firmware_protocol.py tests/control/test_firmware_sim_serial.py tests/control/test_microcontroller.py— 47 passed, 1 pre-existing skippytest tests/control/test_live_controller_hw_illumination.py tests/control/test_intensity_cap.py tests/control/test_channel_sequence.py tests/control/test_LiveControlWidget_offset.py— 62 passed;black --checkcleanNot in this PR (follow-ups)
FirmwareSimSerialstill reports 1.3 (was not bumped for 1.4 either); nothing in Python is gated above 1.0.SET_STROBE_DELAYlanding mid-strobe moves the in-flight strobe end;TURN_ONmid-strobe with a changed source lights two ports until strobe end; an edge-mode retrigger while the strobe is HIGH postpones its end.🤖 Generated with Claude Code