[GANIL-155] implement light protector/nd filter usage in the gui - #3535
Conversation
A light protector role is to provide protection from light component's excessive or harmful light. Light protector is an actuator with fixed positions/choices. main_gui_data.py (MainGUIData) - Add set_light_protector_position(position_name), a convenience method that moves all axes of the light protector to a named position (e.g. "on", "off").
There was a problem hiding this comment.
Pull request overview
This PR adds GUI-level integration for a “light protector” hardware component, so the GUI can automatically move it to safe/operational positions during alignment, streaming, acquisition, and interlock events.
Changes:
- Adds a
light_protectorcomponent hookup toMainGUIDataand a helper to move it by named position (“on”/“off”). - Automatically sets the light protector to “on” when entering the SPARC2 alignment tab.
- Toggles the light protector based on stream activity and interlock state, and manages it during acquisition + interlock transitions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/odemis/gui/model/main_gui_data.py |
Adds light_protector component mapping and a helper to move it to named positions. |
src/odemis/gui/cont/tabs/sparc2_align_tab.py |
Forces light protector to “on” when the alignment tab is shown. |
src/odemis/gui/cont/stream_bar.py |
Toggles light protector on stream start/stop and considers interlock state. |
src/odemis/gui/cont/acquisition/sparc_acq.py |
Stores/restores protector position around interlock and toggles it around acquisition. |
| def set_light_protector_position(self, position_name: str) -> bool: | ||
| """ | ||
| Set the light protector axes to a given position name. | ||
|
|
||
| :param position_name: Position name to set. | ||
| :raises: Exception if an error occured during the move operation. | ||
|
|
||
| NOTE: The light protector must contain axes with choices that are dictionaries mapping float values | ||
| to string names. If the position name is not found in the choices, a warning is logged and the | ||
| axis is skipped. | ||
| """ | ||
| if self.light_protector is None: | ||
| return |
| interlock_triggered = False | ||
| if model.hasVA(self._main_data_model.light, "interlockTriggered"): | ||
| interlock_triggered = self._main_data_model.light.interlockTriggered.value | ||
|
|
||
| if updated and not interlock_triggered: | ||
| self._main_data_model.set_light_protector_position("off") | ||
| else: | ||
| self._main_data_model.set_light_protector_position("on") |
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant MainGUIData
participant SPARCController
participant LightProtector
MainGUIData->>SPARCController: tab change notification
SPARCController->>SPARCController: select "off" or "on" position
SPARCController->>LightProtector: moveAbs(changed axes)
SPARCController->>SPARCController: select "on" after interlock activation
SPARCController->>LightProtector: moveAbs(changed axes)
SPARCController->>SPARCController: select "off" after interlock reset on SPARC tab
SPARCController->>LightProtector: moveAbs(changed axes)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/cont/acquisition/sparc_acq.py (1)
284-300: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUpdate interlock state before attempting protector movement.
If setting the protector to
"on"fails, execution exits before Line 308 updates_interlockTriggered. It remains false, allowingon_acquisitionto later issue an"off"move while the physical interlock is active. Update state first and fail closed on movement errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/gui/cont/acquisition/sparc_acq.py` around lines 284 - 300, Update the interlock-handling method so `_interlockTriggered` is set before calling `set_light_protector_position("on")` or restoring the protector via `moveAbs`. Ensure protector movement failures leave the interlock state active (fail closed), preventing `on_acquisition` from issuing an unsafe `"off"` move.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/gui/cont/acquisition/sparc_acq.py`:
- Around line 455-456: Introduce a shared prioritized coordinator for
light-protector state requests across acquisition, stream, alignment, and
interlock paths. In src/odemis/gui/cont/acquisition/sparc_acq.py lines 455-456,
serialize or revalidate the interlock before issuing the "off" request; in
src/odemis/gui/cont/stream_bar.py lines 2145-2152, route stream updates through
that coordinator; and in src/odemis/gui/cont/tabs/sparc2_align_tab.py line 2542,
use the same state machine for alignment visibility so callbacks cannot reorder
safety-critical requests.
- Line 534: Update the acquisition flow around set_light_protector_position so a
protector movement exception cannot bypass the existing future.result() error
handling and cleanup. Catch or defer that exception within the same protected
flow, then ensure cancellation, failure handling, and UI cleanup execute
normally.
In `@src/odemis/gui/model/main_gui_data.py`:
- Around line 467-480: Update set_light_protector_position so every execution
path returns an explicit boolean consistent with its declared -> bool contract,
including when light_protector is None, when positions are skipped or
unavailable, and after a successful move; preserve exception propagation for
move failures rather than returning a value there.
---
Outside diff comments:
In `@src/odemis/gui/cont/acquisition/sparc_acq.py`:
- Around line 284-300: Update the interlock-handling method so
`_interlockTriggered` is set before calling `set_light_protector_position("on")`
or restoring the protector via `moveAbs`. Ensure protector movement failures
leave the interlock state active (fail closed), preventing `on_acquisition` from
issuing an unsafe `"off"` move.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19f28e53-7984-478e-9dec-eb6d476f2a3e
📒 Files selected for processing (4)
src/odemis/gui/cont/acquisition/sparc_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.pysrc/odemis/gui/model/main_gui_data.py
| interlock_triggered = False | ||
| if model.hasVA(self._main_data_model.light, "interlockTriggered"): | ||
| interlock_triggered = self._main_data_model.light.interlockTriggered.value | ||
|
|
||
| if updated and not interlock_triggered: | ||
| self._main_data_model.set_light_protector_position("off") | ||
| else: | ||
| self._main_data_model.set_light_protector_position("on") |
There was a problem hiding this comment.
That path is quite critical in the GUI, and happens every time a stream plays/pause. That will add delay to every time a stream is played (and if directly playing a new stream, it'll cause it to first get activated and immediately after disabled again).
I also worry a bit about the mix of concerns: this handle the high-level play/pause of a stream, and it's mixed with low-level hardware movement.
So, I suggest to change the logic by a simpler rule: if the acquisition tab is active, then the protection is disabled, otherwise it's enabled. This way, the movement would happen only once per tab switch. Also, it should simplify computing the "right" position for the protector when the interlock is untriggered.
You could move all the code into the sparc_acq controller too, with a call back when the tab changes, which keeps it simpler.
There was a problem hiding this comment.
I just implemented it as mentioned by Noémie in the requirements. But looking at the code changes, I also agree with you the acquisition tab can handle this logic instead of the stream_bar, while we always have the interlock for safety.
| if self._main_data_model.light_protector: | ||
| self._pre_interlock_protector = self._main_data_model.light_protector.position.value | ||
| self._main_data_model.set_light_protector_position("on") | ||
| message += " Light protector set to 'on'." |
There was a problem hiding this comment.
This is a message for the user, so just write "laser protection activated."
| message += " E-beam blanker set back to automatic mode." | ||
| if self._main_data_model.light_protector and self._pre_interlock_protector is not None: | ||
| self._main_data_model.light_protector.moveAbs(self._pre_interlock_protector).result() | ||
| message += " Light protector position restored." |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/odemis/gui/model/main_gui_data.py:481
- The function is annotated as returning bool but it never returns a value (it returns None implicitly, including in the early exit when light_protector is None). This makes the API contract misleading for callers and type-checkers.
def set_light_protector_position(self, position_name: str) -> bool:
src/odemis/gui/model/main_gui_data.py:486
- Spelling typo in docstring: "occured" should be "occurred".
:raises: Exception if an error occured during the move operation.
src/odemis/gui/model/main_gui_data.py:28
- Importing CancelledError from concurrent.futures._base relies on a private module and can break across Python versions; use the public concurrent.futures.CancelledError instead.
from concurrent.futures._base import CancelledError
src/odemis/gui/cont/acquisition/sparc_acq.py:315
- Calling .result() here blocks the current thread until the hardware move completes, which can freeze the GUI (or delay handling of further interlock updates). Prefer starting the move asynchronously and handling completion/errors via a done-callback.
if self._main_data_model.light_protector and self._pre_interlock_protector is not None:
self._main_data_model.light_protector.moveAbs(self._pre_interlock_protector).result()
message += " Laser protection reset."
| def _on_tab_change(self, tab: Tab): | ||
| """ | ||
| Callback when the user changes the tab. | ||
|
|
||
| :param tab: the tab selected by the user | ||
| """ | ||
| if tab.name == TabName.SPARC_ACQUI.value and not self._interlockTriggered: | ||
| self._main_data_model.set_light_protector_position("off") | ||
| else: | ||
| self._main_data_model.set_light_protector_position("on") |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/cont/acquisition/sparc_acq.py (1)
298-302: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReport protection only after the asynchronous move succeeds.
set_light_protector_position("on")startsmoveAbsand returns before completion. The code appends" Laser protection activated."immediately. If the future is cancelled or fails, the warning still claims that the protector is active.Move this message to the successful completion path, or show a pending state and report completion or failure from the callback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/gui/cont/acquisition/sparc_acq.py` around lines 298 - 302, Update the light-protector handling around set_light_protector_position so “Laser protection activated.” is appended only after the asynchronous move completes successfully. Use the returned future’s success callback or completion path, and ensure cancellation or failure does not report the protector as active.
🧹 Nitpick comments (1)
src/odemis/gui/model/main_gui_data.py (1)
28-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the public
concurrent.futuresimport.Import
CancelledErrorfromconcurrent.futures, notconcurrent.futures._base. The public module exposes this exception, while_baseis an internal module path. (docs.python.org)Proposed change
-from concurrent.futures._base import CancelledError +from concurrent.futures import CancelledErrorAs per coding guidelines, the code must remain valid for Python 3.10 and above.
#!/usr/bin/env bash set -euo pipefail python - <<'PY' from concurrent.futures import CancelledError from concurrent.futures._base import CancelledError as private_cancelled_error assert CancelledError is private_cancelled_error PY🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/gui/model/main_gui_data.py` around lines 28 - 35, Update the CancelledError import in main_gui_data.py to use the public concurrent.futures module instead of concurrent.futures._base, preserving Python 3.10+ compatibility and all existing usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/gui/cont/acquisition/sparc_acq.py`:
- Around line 179-180: Update SparcAcquiController._on_tab_change to accept Tab
| None, return immediately when the tab is None before accessing tab.name, and
add the explicit -> None return annotation; preserve the existing tab-change
handling for non-None tabs.
---
Outside diff comments:
In `@src/odemis/gui/cont/acquisition/sparc_acq.py`:
- Around line 298-302: Update the light-protector handling around
set_light_protector_position so “Laser protection activated.” is appended only
after the asynchronous move completes successfully. Use the returned future’s
success callback or completion path, and ensure cancellation or failure does not
report the protector as active.
---
Nitpick comments:
In `@src/odemis/gui/model/main_gui_data.py`:
- Around line 28-35: Update the CancelledError import in main_gui_data.py to use
the public concurrent.futures module instead of concurrent.futures._base,
preserving Python 3.10+ compatibility and all existing usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d29be582-2bec-4a25-bea1-a3ed6afef600
📒 Files selected for processing (2)
src/odemis/gui/cont/acquisition/sparc_acq.pysrc/odemis/gui/model/main_gui_data.py
de6948f to
5d724f9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/odemis/gui/cont/acquisition/sparc_acq.py:274
main_data.tabis aVAEnumeratedthat starts asNoneuntilTabBarControllerinitializes it; because this subscription usesinit=True,_on_tab_changewill be called withtab=Noneduring startup, causing anAttributeErrorontab.name. Guard againstNone(and add an explicit-> Nonereturn type).
def _on_tab_change(self, tab: Tab):
"""
Callback when the user changes the tab.
:param tab: the tab selected by the user
"""
if tab.name == TabName.SPARC_ACQUI.value and not self._interlockTriggered:
self._main_data_model.set_light_protector_position("off")
else:
self._main_data_model.set_light_protector_position("on")
src/odemis/gui/model/main_gui_data.py:486
- Typo in docstring: "occured" -> "occurred".
:raise: Exception if an error occured when requesting the move.
src/odemis/gui/cont/acquisition/sparc_acq.py:315
moveAbs(...).result()blocks the thread that is propagating the VA update (VAs notify listeners synchronously). If the move takes time or hangs, this can stall interlock handling and other notifications. Prefer requesting the move asynchronously (similar toset_light_protector_position) and handle completion via a done-callback.
if self._main_data_model.light_protector and self._pre_interlock_protector is not None:
self._main_data_model.light_protector.moveAbs(self._pre_interlock_protector).result()
message += " Laser protection reset."
| elif self._pre_interlock_blanker is None: # Automatic mode (= blanker active when not acquiring) | ||
| message += " E-beam blanker set back to automatic mode." | ||
| if self._main_data_model.light_protector and self._pre_interlock_protector is not None: | ||
| self._main_data_model.light_protector.moveAbs(self._pre_interlock_protector).result() |
There was a problem hiding this comment.
Don't use the previous position (as the tab could have changed in between). Just check what is the current tab, to know whether the filter protection should set again.
| # TODO: add more detectors here | ||
| # Example time-correlator shutter | ||
|
|
||
| def _on_light_protector_move(self, future: model.CancellableFuture) -> None: |
There was a problem hiding this comment.
As it's only used by sparc_acq, you can move these 2 functions to that module.
| except CancelledError: | ||
| logging.debug("Light protector move was cancelled.") |
There was a problem hiding this comment.
We don't support cancelling. So it should be considered an error as well here. Don't treat it explicitly.
5d724f9 to
b9a27a6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/odemis/gui/cont/acquisition/sparc_acq.py:336
- Re-raising here can interrupt
on_interlock_change()/tab-change handlers before they update internal state and show the safety popup (the VA notification layer logs the exception and stops executing the callback). Since the error is already logged, return instead of propagating to keep the GUI logic consistent even when the move request fails.
raise
src/odemis/gui/cont/acquisition/sparc_acq.py:293
- This docstring says the method raises on move-request errors, but the method is used from VA callbacks (tab/interlock changes) where propagating exceptions prevents the handler from completing. If you follow the suggested change to not propagate, update the docstring accordingly (and fix the typo in “occurred”).
This issue also appears on line 336 of the same file.
:param position_name: Position name to set.
:raise: Exception if an error occured when requesting the move.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/odemis/gui/cont/acquisition/sparc_acq.py (1)
264-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse plain-text docstrings.
Remove the
:param:and:raise:fields from these new docstrings. Describe the behavior in unformatted sentences instead.Based on learnings, keep docstrings as plain text only and do not use RST directives such as ":param:" or ":return:".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/gui/cont/acquisition/sparc_acq.py` around lines 264 - 298, Update the new docstrings in _on_tab_change, _on_light_protector_move, and set_light_protector_position to use plain-text descriptions only. Remove the RST :param: and :raise: directives, while preserving the existing behavioral information in sentence form.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/gui/cont/acquisition/sparc_acq.py`:
- Around line 299-336: The light-protector helper must not prevent interlock
state updates when position lookup or movement setup fails. Move the current
position read into the existing protected block, then handle failures at the
interlock event boundary or make the helper return after logging instead of
re-raising; ensure on_interlock_change always updates _interlockTriggered and
the acquisition UI for both activation and reset events.
---
Nitpick comments:
In `@src/odemis/gui/cont/acquisition/sparc_acq.py`:
- Around line 264-298: Update the new docstrings in _on_tab_change,
_on_light_protector_move, and set_light_protector_position to use plain-text
descriptions only. Remove the RST :param: and :raise: directives, while
preserving the existing behavioral information in sentence form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c9d12cf0-a822-43ae-bc53-8ee966c6de88
📒 Files selected for processing (2)
src/odemis/gui/cont/acquisition/sparc_acq.pysrc/odemis/gui/model/main_gui_data.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/model/main_gui_data.py
b9a27a6 to
4d395bf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/odemis/gui/cont/acquisition/sparc_acq.py:306
light_protector.position.valueis read outside the surroundingtryblock. If the backend raises (e.g., transient Pyro/HW error), the exception will bypass the handler and can break the tab-change/interlock callback. Move the position read (and related locals) inside thetryso it is caught and logged consistently.
current_pos = light_protector.position.value
target = {}
try:
axes = light_protector.axes
|
|
||
| :param position_name: Position name to set. | ||
|
|
||
| NOTE: The light protector must contain axes with choices that are dictionaries mapping float values |
There was a problem hiding this comment.
I think the new lines should start from beginning without space just like the function description
| "Failed to set light protector position to '%s', " | ||
| "it is not safe to perform light alignment.", | ||
| position_name, | ||
| ) |
There was a problem hiding this comment.
Is the finally block not needed such that in case of an exception it reaches a safe value?
| from odemis.acq.align.fastem import Calibrations | ||
| from odemis.acq.fastem import FastEMCalibration, FastEMROC | ||
| from odemis.acq.move import MicroscopePostureManager, MeteorTFS3PostureManager | ||
| from odemis.acq.move import MicroscopePostureManager |
There was a problem hiding this comment.
Was it by mistake MeteorTFS3 was removed or is it perhaps already not used?
No description provided.