[GANIL-27] extend gui to support park engage for 3 axes - #3505
[GANIL-27] extend gui to support park engage for 3 axes#3505nandishjpatel wants to merge 1 commit into
Conversation
nandishjpatel
commented
Jul 3, 2026
nandishjpatel
commented
There was a problem hiding this comment.
Pull request overview
Extends the SPARC2 GUI/controller logic to support a park/engage mirror actuator with 3 axes (x/y/z), including updated XRC widgets and actuator binding/mapping so existing controls can be reused for the additional axis.
Changes:
- Updated SPARC2 align panel XRC to provide separate XY vs per-axis step-size sliders (with runtime hide/show).
- Added mirror axis set constants + helper (
get_mirror_pos_parked) and updated SPARC2 chamber/align tabs to work with LS or XYZ mirror axes. - Extended
ActuatorControllerto allow overriding slider↔stepsize and button↔(actuator, axis, factor) bindings; added a stageNoneguard in acquisition.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc | Adds new named step-size labels/sliders for XY and individual axes (X/Y hidden by default). |
| src/odemis/gui/model/tab_gui_data.py | Renames mirror XY step-size key and adds a mirror Z step-size definition. |
| src/odemis/gui/main_xrc.py | Wires up new XRC controls and embeds the updated XRC resource XML. |
| src/odemis/gui/cont/tabs/sparc2_chamber_tab.py | Generalizes mirror park/engage logic to LS and XYZ actuators using parked-position helper/constants. |
| src/odemis/gui/cont/tabs/sparc2_align_tab.py | Reuses existing stage Z widgets/buttons to drive mirror Z for XYZ mirrors; updates parked distance calc. |
| src/odemis/gui/cont/tabs/sparc_acquisition_tab.py | Adds main_data.stage existence guard before dereferencing. |
| src/odemis/gui/cont/tabs/_constants.py | Introduces mirror axis constants and parked-position helper. |
| src/odemis/gui/cont/actuators.py | Adds optional override maps for slider/button bindings to support control reuse. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes generalize mirror actuator handling from fixed Sequence Diagram(s)sequenceDiagram
participant Sparc2AlignTab
participant TabData
participant ActuatorController
participant Panel
Sparc2AlignTab->>TabData: read mirror axes and stepsizes
Sparc2AlignTab->>ActuatorController: provide slider and button overrides
ActuatorController->>TabData: derive default bindings
ActuatorController->>Panel: bind available slider and button widgets
ActuatorController-->>Sparc2AlignTab: controller ready
sequenceDiagram
participant ChamberTab
participant Mirror
participant ParkedPosition
participant UI
ChamberTab->>Mirror: read axes, position, and metadata
ChamberTab->>ParkedPosition: compute parked position
ParkedPosition-->>ChamberTab: return metadata or zero fallback
ChamberTab->>Mirror: execute axis-ordered park or reference moves
ChamberTab->>UI: update progress and switch state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
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/model/tab_gui_data.py (1)
763-775: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the LS mirror axis mapping
main.mirroris also used for LS mirrors ({"l", "s"}), but this branch only registers{"x", "y", "z"}. For LS hardware that leavestab_data.axesempty, which hides the mirror controls and breaks auto-align. Preserve thel/sbindings formain.mirrorhere.🤖 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/tab_gui_data.py` around lines 763 - 775, The main.mirror fallback in tab_gui_data should preserve LS axis bindings, not just X/Y/Z. Update the mirror registration logic so the branch in the tab data setup that handles main.mirror also keeps the {"l", "s"} mappings used by LS hardware, while still supporting the existing {"x", "y", "z"} cases. Use the mirror_xy/mirror handling around ss_def.update in tab_gui_data as the place to adjust the axis map so tab_data.axes is populated correctly for LS mirrors.
🧹 Nitpick comments (5)
src/odemis/gui/cont/actuators.py (3)
98-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing type hints on
tab_data,tab_panel,tab_prefix, and return type.Only the new override parameters are typed; the existing parameters and the return type (
None) remain unannotated.🔧 Suggested fix
def __init__( - self, tab_data, tab_panel, tab_prefix, slider_ss_map: Optional[dict] = None, - btn_actuator_map: Optional[dict] = None - ): + self, tab_data: "ActuatorGUIData", tab_panel: wx.Frame, tab_prefix: str, + slider_ss_map: Optional[dict] = None, + btn_actuator_map: Optional[dict] = None + ) -> None:As per coding guidelines, "Always use type hints for function parameters and return types in Python code".
🤖 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/actuators.py` around lines 98 - 101, The __init__ method in the Actuators controller is missing type annotations for the existing parameters and return type. Update the signature of __init__ in the Actuators class so tab_data, tab_panel, and tab_prefix all have explicit type hints, and add the None return annotation as well; keep the existing Optional dict annotations for slider_ss_map and btn_actuator_map unchanged.Source: Coding guidelines
102-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring embeds type information in parentheses.
Entries like "tab_data (ActuatorGUIData): the data model of the tab" and "tab_panel: (wx.Frame): the main frame of the GUI" include type info inline, which the coding guidelines ask to avoid since types are now expressed via annotations.
As per coding guidelines, "Include docstrings for all functions and classes, following the reStructuredText style guide, without type information and without using inline formatting markers or backticks".
🤖 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/actuators.py` around lines 102 - 113, The docstring on the actuator binding helper still embeds type information inline, which should be removed to match the docstring guidelines. Update the docstring for the binding function in actuators.py so parameter descriptions only describe the values and purpose, without parenthesized types or other inline type markers; keep the reStructuredText style and rely on the existing function annotations for types. Focus on the docstring attached to the actuator/button binding routine that mentions tab_data, tab_panel, tab_prefix, slider_ss_map, and btn_actuator_map.Source: Coding guidelines
125-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing-widget lookups now fail silently.
AttributeErrorongetattr(tab_panel, slider_name)/getattr(tab_panel, btn_name)are swallowed with no logging, whereas the previous implementation logged these misses for debugging. If an override map contains a typo'd widget name, the mistake will go unnoticed instead of surfacing in logs.🔧 Suggested fix
try: slider = getattr(tab_panel, slider_name) except AttributeError: + logging.debug("Skipping slider %s, not found on panel", slider_name) continuetry: btn = getattr(tab_panel, btn_name) except AttributeError: + logging.debug("Skipping button %s, not found on panel", btn_name) continueAlso applies to: 152-167
🤖 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/actuators.py` around lines 125 - 138, The widget lookup in the actuator setup is swallowing missing attributes too quietly: when getatt r(tab_panel, slider_name) or getatt r(tab_panel, btn_name) raises AttributeError, the code in the slider/button mapping loop should log which widget name was not found before continuing. Update the lookup handling in the relevant actuator initialization logic (the slider and button connector paths in actuators.py) so typos in override maps are surfaced through logging instead of failing silently.src/odemis/gui/cont/tabs/_constants.py (2)
51-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFunction returns shared mutable module-level constants directly.
MIRROR_POS_PARKED_LS/MIRROR_POS_PARKED_XYZ(and the metadata dict) are returned by reference. If any current or future caller mutates the returned dict, it would corrupt the shared constant for all subsequent callers.🛡️ Suggested fix
pos_parked = mirror.getMetadata().get(model.MD_FAV_POS_DEACTIVE, None) if pos_parked is not None: - return pos_parked + return dict(pos_parked) axes = set(mirror.axes.keys()) if axes == MIRROR_AXES_XYZ: - return MIRROR_POS_PARKED_XYZ + return dict(MIRROR_POS_PARKED_XYZ) elif axes == MIRROR_AXES_LS: - return MIRROR_POS_PARKED_LS + return dict(MIRROR_POS_PARKED_LS)🤖 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/tabs/_constants.py` around lines 51 - 59, The helper that resolves parked mirror positions is returning shared mutable module-level data directly, so callers can accidentally mutate constants. Update the logic in the function that checks MD_FAV_POS_DEACTIVE and branches on MIRROR_AXES_XYZ / MIRROR_AXES_LS to return a fresh copy of the metadata dict each time instead of the module-level MIRROR_POS_PARKED_LS or MIRROR_POS_PARKED_XYZ object, and preserve the same behavior for the metadata fallback path.
44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring uses RST directive markup (
:param:,:return:,:raises:).Based on learnings, this repo's convention is plain-text docstrings without RST markup/directives like ":param:", ":return:", or ":type:"; write documentation as unformatted text instead.
🔧 Suggested fix
def get_mirror_pos_parked(mirror: model.HwComponent) -> Dict[str, float]: """ - Return the parked position dict for the given mirror actuator. - :param mirror: the mirror component (must have .axes) - :return: parked position - :raises: ValueError if the mirror has unknown axes + Return the parked position dict for the given mirror actuator (must have .axes). + Raises ValueError if the mirror has unknown axes. """🤖 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/tabs/_constants.py` around lines 44 - 50, The docstring on get_mirror_pos_parked uses RST-style directives, but this codebase expects plain-text docstrings. Update the documentation in get_mirror_pos_parked to remove :param:, :return:, and :raises: markup and rewrite the description as simple unformatted text that still explains the mirror argument, the returned parked position dict, and the ValueError condition.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.
Outside diff comments:
In `@src/odemis/gui/model/tab_gui_data.py`:
- Around line 763-775: The main.mirror fallback in tab_gui_data should preserve
LS axis bindings, not just X/Y/Z. Update the mirror registration logic so the
branch in the tab data setup that handles main.mirror also keeps the {"l", "s"}
mappings used by LS hardware, while still supporting the existing {"x", "y",
"z"} cases. Use the mirror_xy/mirror handling around ss_def.update in
tab_gui_data as the place to adjust the axis map so tab_data.axes is populated
correctly for LS mirrors.
---
Nitpick comments:
In `@src/odemis/gui/cont/actuators.py`:
- Around line 98-101: The __init__ method in the Actuators controller is missing
type annotations for the existing parameters and return type. Update the
signature of __init__ in the Actuators class so tab_data, tab_panel, and
tab_prefix all have explicit type hints, and add the None return annotation as
well; keep the existing Optional dict annotations for slider_ss_map and
btn_actuator_map unchanged.
- Around line 102-113: The docstring on the actuator binding helper still embeds
type information inline, which should be removed to match the docstring
guidelines. Update the docstring for the binding function in actuators.py so
parameter descriptions only describe the values and purpose, without
parenthesized types or other inline type markers; keep the reStructuredText
style and rely on the existing function annotations for types. Focus on the
docstring attached to the actuator/button binding routine that mentions
tab_data, tab_panel, tab_prefix, slider_ss_map, and btn_actuator_map.
- Around line 125-138: The widget lookup in the actuator setup is swallowing
missing attributes too quietly: when getatt r(tab_panel, slider_name) or getatt
r(tab_panel, btn_name) raises AttributeError, the code in the slider/button
mapping loop should log which widget name was not found before continuing.
Update the lookup handling in the relevant actuator initialization logic (the
slider and button connector paths in actuators.py) so typos in override maps are
surfaced through logging instead of failing silently.
In `@src/odemis/gui/cont/tabs/_constants.py`:
- Around line 51-59: The helper that resolves parked mirror positions is
returning shared mutable module-level data directly, so callers can accidentally
mutate constants. Update the logic in the function that checks
MD_FAV_POS_DEACTIVE and branches on MIRROR_AXES_XYZ / MIRROR_AXES_LS to return a
fresh copy of the metadata dict each time instead of the module-level
MIRROR_POS_PARKED_LS or MIRROR_POS_PARKED_XYZ object, and preserve the same
behavior for the metadata fallback path.
- Around line 44-50: The docstring on get_mirror_pos_parked uses RST-style
directives, but this codebase expects plain-text docstrings. Update the
documentation in get_mirror_pos_parked to remove :param:, :return:, and :raises:
markup and rewrite the description as simple unformatted text that still
explains the mirror argument, the returned parked position dict, and the
ValueError condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34a35a3e-53d4-4d2d-9735-f63c8b24f75f
📒 Files selected for processing (8)
src/odemis/gui/cont/actuators.pysrc/odemis/gui/cont/tabs/_constants.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.pysrc/odemis/gui/cont/tabs/sparc2_chamber_tab.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
pieleric
left a comment
pieleric
left a comment
There was a problem hiding this comment.
As we are soon going to have another system, with a X,Y, and RZ axes, let's try to keep the code generic whenever possible.
I'd made suggestions so that the only part required to support RZ will be the actuators related code.
f5576c3 to
8501476
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/odemis/gui/cont/tabs/_constants.py (1)
38-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring uses RST field markup contrary to recent reviewer guidance.
The docstring uses
:param:/:return:directives. A recent learning from this same repo states docstrings should be plain text, without RST directives like:param:,:return:,:type:.📝 Proposed plain-text docstring
""" Return the position dict corresponding to the parked position of the given mirror actuator. If MD_FAV_POS_DEACTIVE metadata is defined for the mirror, it is used. Otherwise, default to 0 for each axis. - :param mirror: the mirror component (must have .axes) - :return: parked position as a dict of axis name -> position (m) + mirror is the mirror component (must have .axes). + Returns the parked position as a dict of axis name to position, in meters. """Based on learnings, "keep docstrings as plain text only. Do not use reStructuredText (RST) markup/directives such as ':param:', ':return:', or ':type:' in docstrings; write the documentation as unformatted text instead." Note the coding guideline also asks to follow "the reStructuredText style guide, without type information and without using inline formatting markers or backticks" for
**/*.py, so please reconcile these two expectations going forward.🤖 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/tabs/_constants.py` around lines 38 - 45, The docstring for get_mirror_pos_parked still uses RST-style field directives, which conflicts with the repo guidance for plain-text docstrings. Update the function’s documentation to remove :param: and :return: markup and rewrite the descriptions as simple prose while keeping the same meaning. Keep the docstring associated with get_mirror_pos_parked clear and readable, and avoid adding any other RST directives or inline markup.Sources: Coding guidelines, Learnings
src/odemis/gui/cont/tabs/sparc2_chamber_tab.py (1)
234-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the raised exception to the original
KeyError(Ruff B904).Static analysis flags this: raising inside an
exceptclause withoutfrom ...obscures the original cause in tracebacks.As per static analysis hints: "Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling (B904)."🔧 Proposed fix
- try: - axes_order = mirror_md[model.MD_AXES_ORDER_REF] - except KeyError: - raise ValueError("Mirror actuator has no metadata AXES_ORDER_REF") + try: + axes_order = mirror_md[model.MD_AXES_ORDER_REF] + except KeyError as ex: + raise ValueError("Mirror actuator has no metadata AXES_ORDER_REF") from ex🤖 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/tabs/sparc2_chamber_tab.py` around lines 234 - 237, The try/except in the mirror metadata lookup should chain the new ValueError to the original KeyError to satisfy Ruff B904. Update the exception handling around the `axes_order = mirror_md[model.MD_AXES_ORDER_REF]` access in `sparc2_chamber_tab.py` so the raised ValueError preserves the caught `KeyError` as its cause, keeping the existing message while making the traceback explicit.Source: Linters/SAST tools
src/odemis/gui/cont/tabs/sparc2_align_tab.py (1)
858-858: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a named constant for the mirror x/y/z axis set.
{"x", "y", "z"}is an inline literal here, whereas the LS case has a dedicatedMIRROR_AXES_LSconstant in_constants.py. A matchingMIRROR_AXES_XYZconstant would avoid duplicating this literal if other layers need the same check (e.g., tab_gui_data.py per the stack summary) and keeps the axis-set vocabulary centralized.🤖 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/tabs/sparc2_align_tab.py` at line 858, The mirror axis check in the tab logic uses an inline {"x", "y", "z"} literal, so replace it with a shared named constant like MIRROR_AXES_XYZ in _constants.py and use that constant in the relevant mirror-axis checks (including the condition in the align tab and any other matching callers such as tab_gui_data.py). Keep the axis-set definition centralized alongside MIRROR_AXES_LS so the vocabulary is consistent and reused instead of duplicated.
🤖 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/tabs/_constants.py`:
- Around line 46-49: The get_mirror_pos_parked() helper currently returns
MD_FAV_POS_DEACTIVE unchanged, so partial parked-position metadata can still
break callers like the chamber and align tabs when they access every axis.
Update the logic in get_mirror_pos_parked() to validate the returned metadata
against mirror.axes and either fill any missing axes with 0 or ignore incomplete
metadata and fall back to the default zeroed position.
In `@src/odemis/gui/cont/tabs/sparc2_align_tab.py`:
- Around line 854-882: The x/y/z mirror branch in the auto-align setup is
rebinding stage-Z UI controls to the mirror via slider_ss_map and
btn_actuator_map, so the visible slider_stage, btn_p_stage_z, and btn_m_stage_z
end up driving mirror_z instead of the stage. Update the logic around the
mirror-axis handling in this tab setup so the auto-align path and the x/y/z
mirror remapping are mutually exclusive, or explicitly skip remapping those
controls when auto-align is active. Use the existing panel and
ActuatorController wiring to keep the control labels and bound actuators
consistent.
In `@src/odemis/gui/cont/tabs/sparc2_chamber_tab.py`:
- Around line 362-373: The switch-mirror idle label logic in
sparc2_chamber_tab.py is choosing text from mirror_axes and mstate only, which
makes it diverge from the in-flight wording used by _on_switch_btn. Update the
label selection in the switch-mirror refresh path so it also considers
reference_is_deactive, matching the same decision used for “PARKING MIRROR” vs
“REFERENCING MIRROR” and keeping the idle button text consistent with the action
that will actually run. Use the existing switch-mirror label update block and
_on_switch_btn as the reference points for the fix.
---
Nitpick comments:
In `@src/odemis/gui/cont/tabs/_constants.py`:
- Around line 38-45: The docstring for get_mirror_pos_parked still uses
RST-style field directives, which conflicts with the repo guidance for
plain-text docstrings. Update the function’s documentation to remove :param: and
:return: markup and rewrite the descriptions as simple prose while keeping the
same meaning. Keep the docstring associated with get_mirror_pos_parked clear and
readable, and avoid adding any other RST directives or inline markup.
In `@src/odemis/gui/cont/tabs/sparc2_align_tab.py`:
- Line 858: The mirror axis check in the tab logic uses an inline {"x", "y",
"z"} literal, so replace it with a shared named constant like MIRROR_AXES_XYZ in
_constants.py and use that constant in the relevant mirror-axis checks
(including the condition in the align tab and any other matching callers such as
tab_gui_data.py). Keep the axis-set definition centralized alongside
MIRROR_AXES_LS so the vocabulary is consistent and reused instead of duplicated.
In `@src/odemis/gui/cont/tabs/sparc2_chamber_tab.py`:
- Around line 234-237: The try/except in the mirror metadata lookup should chain
the new ValueError to the original KeyError to satisfy Ruff B904. Update the
exception handling around the `axes_order = mirror_md[model.MD_AXES_ORDER_REF]`
access in `sparc2_chamber_tab.py` so the raised ValueError preserves the caught
`KeyError` as its cause, keeping the existing message while making the traceback
explicit.
🪄 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
Run ID: 4c0e2154-9175-4b84-8d54-56be2a4f661e
📒 Files selected for processing (8)
src/odemis/gui/cont/actuators.pysrc/odemis/gui/cont/tabs/_constants.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.pysrc/odemis/gui/cont/tabs/sparc2_chamber_tab.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
🚧 Files skipped from review as they are similar to previous changes (5)
- src/odemis/gui/cont/tabs/sparc_acquisition_tab.py
- src/odemis/gui/model/tab_gui_data.py
- src/odemis/gui/main_xrc.py
- src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
- src/odemis/gui/cont/actuators.py
|
This GUI change don't seem to align with the hardware change (PR #3508): |
8501476 to
4c8bd99
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/odemis/gui/cont/tabs/sparc2_chamber_tab.py (1)
369-375: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign idle switch-mirror labels with
reference_at_onceaction.The idle switch-mirror UI logic incorrectly relies on the mirror axis set instead of checking the
reference_at_onceconfiguration, causing a mismatch with the in-flight actions executed by_on_switch_btn.
src/odemis/gui/cont/tabs/sparc2_chamber_tab.py#L369-L375: Update the button label selection to checkreference_at_onceinstead of the axis set, choosing "REFERENCE MIRROR" when true and "PARK MIRROR" otherwise.src/odemis/gui/cont/tabs/sparc2_chamber_tab.py#L346-L355: Update the warning text to similarly depend onreference_at_once.🐛 Proposed fixes to align with reference_at_once
For the warning text (
src/odemis/gui/cont/tabs/sparc2_chamber_tab.py#L346-L355):if mstate == MIRROR_NOT_REFD: - if mirror_axes == MIRROR_AXES_LS: + reference_at_once = mirror.getMetadata().get(model.MD_CALIB, {}).get("reference_at_once", False) + if not reference_at_once: txt_warning = ("Parking the mirror is required at least once in order " "to reference the actuators.") else: txt_warning = "Referencing the mirror is required at least once."For the button label (
src/odemis/gui/cont/tabs/sparc2_chamber_tab.py#L369-L375):if mstate == MIRROR_PARKED: btn_text = "ENGAGE MIRROR" else: - if mirror_axes == MIRROR_AXES_LS: - btn_text = "PARK MIRROR" - else: - if mstate == MIRROR_NOT_REFD: - btn_text = "REFERENCE MIRROR" - else: - btn_text = "PARK MIRROR" + reference_at_once = mirror.getMetadata().get(model.MD_CALIB, {}).get("reference_at_once", False) + if mstate == MIRROR_NOT_REFD and reference_at_once: + btn_text = "REFERENCE MIRROR" + else: + btn_text = "PARK MIRROR"🤖 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/tabs/sparc2_chamber_tab.py` around lines 369 - 375, Update the idle switch-mirror logic in src/odemis/gui/cont/tabs/sparc2_chamber_tab.py at lines 369-375 and 346-355 to use the reference_at_once configuration, matching _on_switch_btn: show “REFERENCE MIRROR” and the corresponding warning when true, otherwise show “PARK MIRROR” and its warning. Remove the mirror_axes-based decision while preserving the existing mstate handling where applicable.src/odemis/gui/cont/tabs/sparc2_align_tab.py (1)
854-882: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStage-Z controls are rebound to the mirror in x/y/z mode, conflicting with auto-align.
The x/y/z mirror branch rebinds
slider_stage,btn_p_stage_z, andbtn_m_stage_ztomirror_z. If auto-align is also active, it exposes these exact widgets to control the stage, but they will end up driving the mirror instead. Keep these branches mutually exclusive or explicitly skip remapping those controls when auto-align is active.🤖 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/tabs/sparc2_align_tab.py` around lines 854 - 882, The SPARCv2 x/y/z mirror setup in the initialization branch remaps stage-Z widgets that auto-align requires for stage control. Make the mirror remapping conditional on auto-align being inactive, or otherwise exclude slider_stage, btn_p_stage_z, and btn_m_stage_z from btn_actuator_map/slider_ss_map when auto-align is active, while preserving mirror x/y mappings.
🤖 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/tabs/sparc2_align_tab.py`:
- Around line 2409-2411: Update the listed function signatures with the
requested parameter and return annotations: in
src/odemis/gui/cont/tabs/sparc2_align_tab.py lines 2409-2411, annotate
_onMirrorPos as pos: dict[str, float] -> None; in lines 2619, annotate
get_display_priority as main_data: model.MainGUIData -> int | None; in
src/odemis/gui/cont/tabs/sparc2_chamber_tab.py lines 141-157, annotate
_get_mirror_state as mirror: model.HwComponent -> int; lines 184-187, annotate
_update_progress_bar as pos: dict[str, float] -> None; lines 228-242, annotate
_on_switch_btn as evt: wx.Event -> None; and line 420, annotate
get_display_priority as main_data: model.MainGUIData -> int | None.
In `@src/odemis/gui/cont/tabs/sparc2_chamber_tab.py`:
- Around line 234-237: Update the KeyError handler in the axes_order lookup to
raise the existing ValueError with explicit exception suppression using “from
None”, while preserving the current error message and behavior.
---
Duplicate comments:
In `@src/odemis/gui/cont/tabs/sparc2_align_tab.py`:
- Around line 854-882: The SPARCv2 x/y/z mirror setup in the initialization
branch remaps stage-Z widgets that auto-align requires for stage control. Make
the mirror remapping conditional on auto-align being inactive, or otherwise
exclude slider_stage, btn_p_stage_z, and btn_m_stage_z from
btn_actuator_map/slider_ss_map when auto-align is active, while preserving
mirror x/y mappings.
In `@src/odemis/gui/cont/tabs/sparc2_chamber_tab.py`:
- Around line 369-375: Update the idle switch-mirror logic in
src/odemis/gui/cont/tabs/sparc2_chamber_tab.py at lines 369-375 and 346-355 to
use the reference_at_once configuration, matching _on_switch_btn: show
“REFERENCE MIRROR” and the corresponding warning when true, otherwise show “PARK
MIRROR” and its warning. Remove the mirror_axes-based decision while preserving
the existing mstate handling where applicable.
🪄 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
Run ID: 793b9761-7ed4-496e-a0cd-c69a47a8dd64
📒 Files selected for processing (8)
src/odemis/gui/cont/actuators.pysrc/odemis/gui/cont/tabs/_constants.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.pysrc/odemis/gui/cont/tabs/sparc2_chamber_tab.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
🚧 Files skipped from review as they are similar to previous changes (6)
- src/odemis/gui/cont/tabs/sparc_acquisition_tab.py
- src/odemis/gui/model/tab_gui_data.py
- src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
- src/odemis/gui/main_xrc.py
- src/odemis/gui/cont/tabs/_constants.py
- src/odemis/gui/cont/actuators.py
4c8bd99 to
e11a3f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/odemis/gui/cont/tabs/sparc2_chamber_tab.py (1)
347-374: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
reference_at_onceinstead ofmirror_axesfor UI state text to avoid mismatch with the in-flight action.The idle switch-mirror button label and status warning currently ignore
reference_at_once, relying instead onmirror_axes == MIRROR_AXES_LS. This causes a mismatch with the in-flight action triggered in_on_switch_btn, where the logic and in-flight labels ("PARKING MIRROR" vs "REFERENCING MIRROR") are chosen based onreference_at_once. Using the flag uniformly ensures the visual state exactly matches what will execute.🐛 Proposed fix
- mirror_axes = set(mirror.axes.keys()) + reference_at_once = mirror.getMetadata().get(model.MD_CALIB, {}).get("reference_at_once", False) if mstate == MIRROR_NOT_REFD: - if mirror_axes == MIRROR_AXES_LS: + if not reference_at_once: txt_warning = ("Parking the mirror is required at least once in order " "to reference the actuators.") else: txt_warning = "Referencing the mirror is required at least once." elif mstate == MIRROR_BAD: txt_warning = "The mirror is neither fully parked nor entirely engaged." else: txt_warning = None self.panel.pnl_ref_msg.Show(txt_warning is not None) if txt_warning: self.panel.txt_warning.SetLabel(txt_warning) self.panel.txt_warning.Wrap(self.panel.pnl_ref_msg.Size[0] - 16) if mstate == MIRROR_PARKED: btn_text = "ENGAGE MIRROR" else: - if mirror_axes == MIRROR_AXES_LS: - btn_text = "PARK MIRROR" - else: - if mstate == MIRROR_NOT_REFD: - btn_text = "REFERENCE MIRROR" - else: - btn_text = "PARK MIRROR" + if mstate == MIRROR_NOT_REFD and reference_at_once: + btn_text = "REFERENCE MIRROR" + else: + btn_text = "PARK MIRROR"🤖 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/tabs/sparc2_chamber_tab.py` around lines 347 - 374, Update the idle warning and button-label logic in the mirror state handler to use reference_at_once instead of mirror_axes == MIRROR_AXES_LS when choosing reference-versus-parking text. Keep the existing mstate handling and labels unchanged otherwise, so the UI matches the action selection in _on_switch_btn.
🤖 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/tabs/sparc_acquisition_tab.py`:
- Line 277: Update the centering guard in the SPARC acquisition flow to use `if
not main_data.stage or main_data.stage.name not in sstage.affects.value`,
ensuring centering runs when no main stage is configured or when the stage is
independent. Preserve the existing centering behavior for affected stages.
---
Duplicate comments:
In `@src/odemis/gui/cont/tabs/sparc2_chamber_tab.py`:
- Around line 347-374: Update the idle warning and button-label logic in the
mirror state handler to use reference_at_once instead of mirror_axes ==
MIRROR_AXES_LS when choosing reference-versus-parking text. Keep the existing
mstate handling and labels unchanged otherwise, so the UI matches the action
selection in _on_switch_btn.
🪄 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
Run ID: 664dc4c9-1f7e-43a3-b935-1640fba156c7
📒 Files selected for processing (8)
src/odemis/gui/cont/actuators.pysrc/odemis/gui/cont/tabs/_constants.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.pysrc/odemis/gui/cont/tabs/sparc2_chamber_tab.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
🚧 Files skipped from review as they are similar to previous changes (6)
- src/odemis/gui/cont/tabs/_constants.py
- src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
- src/odemis/gui/model/tab_gui_data.py
- src/odemis/gui/main_xrc.py
- src/odemis/gui/cont/actuators.py
- src/odemis/gui/cont/tabs/sparc2_align_tab.py
e11a3f4 to
41a502d
Compare
…g 3 axes (x, y, z)
41a502d to
304b47a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tabs/sparc2_chamber_tab.py`:
- Around line 237-239: Update the axes-order validation near the warning in the
chamber tab so an `axes_order` that is not exactly a permutation of
`mirror_axes` is rejected before any move-scheduling loops run. Replace the
warning-only behavior with the method’s established invalid-input handling,
ensuring invalid metadata cannot omit, duplicate, or introduce mirror axes while
valid orders continue unchanged.
🪄 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: 5f84d242-9ad4-4dfe-8e44-3eb06b9e3a2f
📒 Files selected for processing (8)
src/odemis/gui/cont/actuators.pysrc/odemis/gui/cont/tabs/_constants.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.pysrc/odemis/gui/cont/tabs/sparc2_chamber_tab.pysrc/odemis/gui/cont/tabs/sparc_acquisition_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/model/tab_gui_data.pysrc/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
🚧 Files skipped from review as they are similar to previous changes (7)
- src/odemis/gui/cont/tabs/sparc_acquisition_tab.py
- src/odemis/gui/main_xrc.py
- src/odemis/gui/model/tab_gui_data.py
- src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
- src/odemis/gui/cont/actuators.py
- src/odemis/gui/cont/tabs/_constants.py
- src/odemis/gui/cont/tabs/sparc2_align_tab.py
| if set(axes_order) != mirror_axes: | ||
| logging.warning("Axes order of mirror is %s, while should have %s axes", | ||
| axes_order, mirror_axes) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject an invalid axis order before scheduling moves.
At Line 237, an invalid axes_order only writes a warning. The later loops can then omit a mirror axis, move one axis twice, or send a command for an unknown axis. Reject metadata that is not a permutation of the mirror axes.
Proposed fix
- if set(axes_order) != mirror_axes:
- logging.warning("Axes order of mirror is %s, while should have %s axes",
- axes_order, mirror_axes)
+ if len(axes_order) != len(mirror_axes) or set(axes_order) != mirror_axes:
+ raise ValueError(
+ "Mirror actuator metadata AXES_ORDER_REF must contain each mirror axis exactly once"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if set(axes_order) != mirror_axes: | |
| logging.warning("Axes order of mirror is %s, while should have %s axes", | |
| axes_order, mirror_axes) | |
| if len(axes_order) != len(mirror_axes) or set(axes_order) != mirror_axes: | |
| raise ValueError( | |
| "Mirror actuator metadata AXES_ORDER_REF must contain each mirror axis exactly once" | |
| ) |
🤖 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/tabs/sparc2_chamber_tab.py` around lines 237 - 239,
Update the axes-order validation near the warning in the chamber tab so an
`axes_order` that is not exactly a permutation of `mirror_axes` is rejected
before any move-scheduling loops run. Replace the warning-only behavior with the
method’s established invalid-input handling, ensuring invalid metadata cannot
omit, duplicate, or introduce mirror axes while valid orders continue unchanged.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/odemis/gui/cont/tabs/sparc2_chamber_tab.py:237
- Raising ValueError here will likely bubble up from the GUI event handler and can crash the tab if a non-{l,s} mirror actuator lacks MD_AXES_ORDER_REF metadata. It would be safer to fall back to a deterministic axis order (e.g. mirror.axes key order) and log a warning instead of raising.
else:
try:
axes_order = mirror_md[model.MD_AXES_ORDER_REF]
except KeyError:
raise ValueError("Mirror actuator has no metadata AXES_ORDER_REF")
if set(axes_order) != mirror_axes:
| dist_parked = math.hypot(pos["l"] - MIRROR_POS_PARKED["l"], | ||
| pos["s"] - MIRROR_POS_PARKED["s"]) | ||
| pos_parked = get_mirror_pos_parked(mirror) | ||
|
|
There was a problem hiding this comment.
maybe an empty line is not needed
