[GANIL-134] extend smaract driver to support ganil actuators - #3508
Open
nandishjpatel wants to merge 3 commits into
Open
[GANIL-134] extend smaract driver to support ganil actuators#3508nandishjpatel wants to merge 3 commits into
nandishjpatel wants to merge 3 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds configurable standard and multi-phase referencing to the SmarAct MCS2 driver. Multi-phase mode performs metadata-driven alignment, referencing, active positioning, and optional deactivation through an asynchronous cancellable workflow with retries. Tests cover successful operation and metadata errors. A SPARC v2 simulator configuration defines SEM, optical, mirror actuator, camera, spectrometer, and spectrograph components using the new referencing settings. Sequence Diagram(s)sequenceDiagram
participant Client
participant MCS2
participant Executor
participant SmarActHardware
Client->>MCS2: reference(axes)
MCS2->>Executor: schedule multi-phase reference
Executor->>SmarActHardware: reference aligned axes and move to alignment positions
Executor->>SmarActHardware: move to active positions and reference remaining axes
Executor->>SmarActHardware: optionally move axes to deactive positions
Executor->>MCS2: update positions and referenced state
MCS2-->>Client: complete or HwError
🚥 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: 4
🧹 Nitpick comments (4)
src/odemis/driver/test/smaract_test.py (1)
853-947: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return annotations and a
setUpdocstring.The new test hooks/methods should use
-> None, andsetUp()needs a docstring. As per coding guidelines,**/*.py: Always use type hints for function parameters and return types in Python code, and include docstrings for all functions and classes.🤖 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/driver/test/smaract_test.py` around lines 853 - 947, The new test methods and hooks in this Smaract test class are missing required Python annotations and documentation. Add an explicit -> None return type to setUp and any other test lifecycle methods in this block, and add a docstring for setUp to match the existing style used by test_reference_all_axes and related methods. Keep the changes within the smaract_test test class so the code follows the project’s type-hinting and docstring conventions.Source: Coding guidelines
src/odemis/driver/smaract.py (3)
2937-2939: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRewrite the new docstrings as plain text.
These docstrings use RST directives and inline markers such as
:param:,:return:,:raises:and backticks. Please keep them plain text. As per coding guidelines, docstrings must avoid inline formatting markers; Based on learnings, keep docstrings as plain text only and do not use RST markup/directives such as:param:,:return:, or:type:.Also applies to: 3652-3661, 3735-3755, 3801-3828
🤖 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/driver/smaract.py` around lines 2937 - 2939, The new docstrings in the SmarAct driver still use RST-style markup, so rewrite them as plain text only. Update the affected docstrings around the referencing/homing procedure and the other listed sections in smaract.py to remove directives like :param:, :return:, :raises:, :type:, and inline backticks, while keeping the same meaning in simple sentence form. Use the relevant method/docstring blocks in smaract.py to locate and convert all newly added docstrings consistently.Sources: Coding guidelines, Learnings
2918-2918: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required Python type annotations.
The new/changed functions are missing return annotations, and some new parameters use unparameterized
list/dicttypes. As per coding guidelines,**/*.py: Always use type hints for function parameters and return types in Python code.Also applies to: 3651-3651, 3733-3734, 3800-3800
🤖 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/driver/smaract.py` at line 2918, Add the missing Python type annotations on the affected functions: include explicit return types for the changed methods and replace any bare list/dict parameter types with parameterized generics. Update the signatures in the relevant SmarAct methods, including __init__ and the other affected functions referenced by the review, so all parameters and returns follow the project’s type-hinting conventions.Source: Coding guidelines
3777-3778: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCatch only expected referencing failures.
The broad
Exceptioncatch can hide programming errors while the retry loop continues. Limit this to the hardware/timeout failures that are actually recoverable here.Proposed fix
- except Exception as e: + except (model.HwError, SA_CTLError, TimeoutError) as e: logging.debug("Axis %s failed to reference: %s", a, e)🤖 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/driver/smaract.py` around lines 3777 - 3778, The retry loop in the SmarAct axis referencing path is catching all Exception instances, which can mask real bugs and let the loop continue incorrectly. Narrow the handler in the referencing logic around the axis retry block to only the expected recoverable hardware/timeout exceptions, and let unexpected errors propagate. Keep the debug logging in the same reference flow, but ensure it only handles the specific failures the code can actually retry.Source: Linters/SAST tools
🤖 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/driver/smaract.py`:
- Around line 3062-3068: Validate refproc before any hardware setup in the
SmarAct initialization path, since the current reference-procedure selection in
the controller setup can raise after SA_CTL_Open and _executor creation. Move
the refproc check in the initialization flow (around the logic that sets
self.reference for REFPROC_MULTI_PHASE and REFPROC_STD) ahead of opening the
controller, and fail fast with ValueError before allocating device handles or
thread-pool resources.
- Around line 3840-3862: Validate that every already-referenced Phase 1 axis has
a corresponding FAV_POS_ACTIVE entry before starting Phase 2 in the SmarAct
referencing flow. In the Phase 2 preparation logic inside the method that builds
pre_phase2_target, fail fast if any axis in self.referenced._value is missing
from active_pos instead of silently skipping it; use the existing active_pos,
phase1_axes, and phase2_axes checks to locate the affected axes. Keep the
existing overall FAV_POS_ACTIVE presence check, but add a per-axis validation
step before moving referenced axes to their active/engaged position so no Phase
1 axis remains at alignment position unexpectedly.
- Around line 3651-3666: The _reference_multi_phase method is ignoring the
caller-supplied axis selection and always referencing every mapped axis, which
can cause unexpected movement. Update _reference_multi_phase (and the
_do_reference_multi_phase path if needed) to either honor the requested axes
passed in by the caller or explicitly reject partial axis requests with a clear
error before submitting work via _executor.submitf. Use the existing
_checkReference and _axis_map handling to validate the requested axes instead of
silently replacing them with all keys.
In `@src/odemis/driver/test/smaract_test.py`:
- Around line 946-947: The loop in the referenced-values assertion uses an
unused variable, so rename the unused key variable in the iteration over
self.dev.referenced.value.items() to a throwaway name or change the loop to
iterate only over the referenced values. Keep the check in the same test method
and preserve the existing assertion on referenced.
---
Nitpick comments:
In `@src/odemis/driver/smaract.py`:
- Around line 2937-2939: The new docstrings in the SmarAct driver still use
RST-style markup, so rewrite them as plain text only. Update the affected
docstrings around the referencing/homing procedure and the other listed sections
in smaract.py to remove directives like :param:, :return:, :raises:, :type:, and
inline backticks, while keeping the same meaning in simple sentence form. Use
the relevant method/docstring blocks in smaract.py to locate and convert all
newly added docstrings consistently.
- Line 2918: Add the missing Python type annotations on the affected functions:
include explicit return types for the changed methods and replace any bare
list/dict parameter types with parameterized generics. Update the signatures in
the relevant SmarAct methods, including __init__ and the other affected
functions referenced by the review, so all parameters and returns follow the
project’s type-hinting conventions.
- Around line 3777-3778: The retry loop in the SmarAct axis referencing path is
catching all Exception instances, which can mask real bugs and let the loop
continue incorrectly. Narrow the handler in the referencing logic around the
axis retry block to only the expected recoverable hardware/timeout exceptions,
and let unexpected errors propagate. Keep the debug logging in the same
reference flow, but ensure it only handles the specific failures the code can
actually retry.
In `@src/odemis/driver/test/smaract_test.py`:
- Around line 853-947: The new test methods and hooks in this Smaract test class
are missing required Python annotations and documentation. Add an explicit ->
None return type to setUp and any other test lifecycle methods in this block,
and add a docstring for setUp to match the existing style used by
test_reference_all_axes and related methods. Keep the changes within the
smaract_test test class so the code follows the project’s type-hinting and
docstring conventions.
🪄 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: c2f0e84b-0956-48f8-abf8-39dc90534647
📒 Files selected for processing (2)
src/odemis/driver/smaract.pysrc/odemis/driver/test/smaract_test.py
nandishjpatel
force-pushed
the
GANIL-134-extend-smaract-driver-to-support-ganil-actuators
branch
from
July 8, 2026 09:01
a294c48 to
51e54ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/odemis/driver/test/smaract_test.py (2)
818-821: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the mid-file placeholder comment and redundant imports.
unittestandtimeare already imported at the top of the module, and the# Assuming your framework imports are here ...line is a leftover scaffolding comment. Drop these to avoid confusion.Proposed cleanup
-import unittest -import time -# Assuming your framework imports are here (e.g., smaract, testing, model) - CONFIG_MULTI_PHASE = {🤖 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/driver/test/smaract_test.py` around lines 818 - 821, Remove the leftover scaffolding from the test module: in smaract_test, delete the mid-file placeholder comment and the redundant local imports of unittest and time since they already exist at the top of the file. Keep the surrounding test code unchanged and ensure no duplicate imports or placeholder comments remain near the affected section.
847-851: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring references a non-existent method.
The class docstring mentions
_doReferenceMultiPhase, but the implementation is_do_reference_multi_phase. Update to avoid a misleading reference.🤖 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/driver/test/smaract_test.py` around lines 847 - 851, The class docstring in TestMCS2MultiPhase points to the wrong helper name, so update the reference from _doReferenceMultiPhase to the actual implementation symbol _do_reference_multi_phase. Keep the rest of the docstring aligned with the multi-phase referencing test description and ensure any similar mentions in nearby test documentation use the same method name.src/odemis/driver/smaract.py (1)
3733-3755: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse plain-text docstrings without backticks/RST directives.
_reference_with_retryand_do_reference_multi_phasedocstrings use backticks (e.g.`FAV_POS_ALIGN`,`post_ref_positions`) and:param:/:raises:directives. Convert to plain text to match project convention.Based on learnings: keep docstrings as plain text only; do not use RST markup/directives such as ":param:". As per coding guidelines: docstrings should be written "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/driver/smaract.py` around lines 3733 - 3755, The docstring for _reference_with_retry still uses RST-style formatting, backticks, and :param:/ :raises: directives, which should be converted to the project’s plain-text style. Update the docstring in _reference_with_retry, and also check _do_reference_multi_phase for the same pattern, so both use simple descriptive text only with no inline markup or directive tags.Sources: Coding guidelines, 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/driver/smaract.py`:
- Around line 3832-3843: The phased referencing logic in the smaract driver is
reading metadata with raw string keys instead of the public model constants, so
update the lookups in the referencing flow to use the corresponding model.MD_*
symbols. In the code around the ordered_axes, align_pos, active_pos, and
deactive position handling, replace literal metadata names with
model.MD_AXES_ORDER_REF, model.MD_FAV_POS_ALIGN, model.MD_FAV_POS_ACTIVE, and
model.MD_FAV_POS_DEACTIVE so data set via updateMetadata({...}) is found
consistently. Keep the existing validation and error handling in the same
referencing method, only changing the metadata key references.
---
Nitpick comments:
In `@src/odemis/driver/smaract.py`:
- Around line 3733-3755: The docstring for _reference_with_retry still uses
RST-style formatting, backticks, and :param:/ :raises: directives, which should
be converted to the project’s plain-text style. Update the docstring in
_reference_with_retry, and also check _do_reference_multi_phase for the same
pattern, so both use simple descriptive text only with no inline markup or
directive tags.
In `@src/odemis/driver/test/smaract_test.py`:
- Around line 818-821: Remove the leftover scaffolding from the test module: in
smaract_test, delete the mid-file placeholder comment and the redundant local
imports of unittest and time since they already exist at the top of the file.
Keep the surrounding test code unchanged and ensure no duplicate imports or
placeholder comments remain near the affected section.
- Around line 847-851: The class docstring in TestMCS2MultiPhase points to the
wrong helper name, so update the reference from _doReferenceMultiPhase to the
actual implementation symbol _do_reference_multi_phase. Keep the rest of the
docstring aligned with the multi-phase referencing test description and ensure
any similar mentions in nearby test documentation use the same method name.
🪄 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: 136aa8c4-7fd2-4dbb-afa2-2bca8ad99513
📒 Files selected for processing (2)
src/odemis/driver/smaract.pysrc/odemis/driver/test/smaract_test.py
pieleric
requested changes
Jul 13, 2026
pieleric
left a comment
pieleric
left a comment
Member
There was a problem hiding this comment.
Please as you implement this change, also use the option in a (new) microscope file, which would be combined with the GUI changes.
nandishjpatel
force-pushed
the
GANIL-134-extend-smaract-driver-to-support-ganil-actuators
branch
from
July 16, 2026 09:49
51e54ac to
0a15532
Compare
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/driver/smaract.py (1)
2918-2945: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd the required annotations and compliant docstrings.
src/odemis/driver/smaract.py#L2918-L2945: annotate the complete constructor signature and remove type information and inline backticks from the new parameter documentation.src/odemis/driver/smaract.py#L3658-L3673: annotate the parameter and return type; remove inline backticks.src/odemis/driver/smaract.py#L3739-L3761: use parameterized collection types and add-> None; remove inline backticks.src/odemis/driver/smaract.py#L3808-L3837: add-> Noneand remove inline backticks.src/odemis/driver/test/smaract_test.py#L849-L856: add-> Noneand a docstring tosetUp; annotatetearDown.src/odemis/driver/test/smaract_test.py#L858-L943: add-> Noneto every added test method.As per coding guidelines, “Always use type hints for function parameters and return types” and include docstrings for all functions and classes without type information or inline formatting markers.
🤖 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/driver/smaract.py` around lines 2918 - 2945, Annotate the complete constructor signature in src/odemis/driver/smaract.py:2918-2945 and update its parameter documentation to remove type information and inline backticks; apply the requested parameter/return annotations and compliant docstrings without inline backticks at src/odemis/driver/smaract.py:3658-3673, 3739-3761, and 3808-3837, including parameterized collection types and -> None where specified. In src/odemis/driver/test/smaract_test.py:849-856, add a setUp docstring and -> None plus annotate tearDown; add -> None to every test method at src/odemis/driver/test/smaract_test.py:858-943.Source: Coding guidelines
♻️ Duplicate comments (2)
src/odemis/driver/smaract.py (2)
3658-3673: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not discard the requested axes.
This entry point still replaces the caller’s selection with every configured axis. Honor the selection or fall back to standard referencing for partial requests, as previously discussed.
🤖 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/driver/smaract.py` around lines 3658 - 3673, Update _reference_multi_phase to preserve the caller’s requested axes instead of always using set(self.axes.keys()). When the request covers only a subset of axes, route it through the existing standard referencing behavior; retain the multi-phase submission only for the appropriate full-axis request.
3068-3075: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate
refprocbefore allocating hardware resources.The final code still validates this option after opening the controller and creating the executor, so invalid configuration can leak those resources.
🤖 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/driver/smaract.py` around lines 3068 - 3075, Move the refproc normalization and validation in the initialization flow before opening the controller or creating the executor, accepting only REFPROC_MULTI_PHASE and REFPROC_STD. Preserve the procedure-specific self.reference assignment and logging after resources are allocated, and raise ValueError immediately for unknown values.
🧹 Nitpick comments (1)
src/odemis/driver/smaract.py (1)
3783-3784: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOnly retry expected reference failures.
Catching
Exceptiontreats programming and state-update errors as transient hardware failures. Restrict this to the expected motion/hardware exceptions so unexpected faults abort immediately.🤖 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/driver/smaract.py` around lines 3783 - 3784, Update the exception handling around the axis referencing flow to catch only the expected motion or hardware exception types, rather than the broad Exception class. Preserve the existing debug logging and retry behavior for those expected failures, while allowing programming and state-update errors to propagate immediately.Source: Linters/SAST tools
🤖 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/driver/smaract.py`:
- Around line 3863-3869: Update the Phase 1 axis handling in the surrounding
pre-phase2 target construction to require an active position for every
referenced axis outside phase2_axes. Do not silently skip such axes when
active_pos lacks an entry; ensure the missing FAV_POS_ACTIVE condition is
surfaced according to the existing validation or error-handling path, while
preserving the current target assignment for axes with valid active positions.
- Around line 3894-3903: Before the Phase 3 sequential movement loop in the
referencing flow, validate that deactive_pos contains a target for every axis in
ordered_axes (or every referenced axis used by the phase). Reject or raise an
appropriate error when any target is missing, and only then proceed to
_checkMoveAbs, _doMoveAbs, and _waitEndMove so deactivation cannot start with
incomplete coordinates.
In `@src/odemis/driver/test/smaract_test.py`:
- Around line 836-840: Use the public metadata constants consistently: update
MULTI_PHASE_METADATA in smaract_test.py to use model.MD_AXES_ORDER_REF,
model.MD_FAV_POS_ALIGN, model.MD_FAV_POS_ACTIVE, and model.MD_FAV_POS_DEACTIVE
as keys; update the Phase 3 metadata lookup in smaract.py to use
model.MD_FAV_POS_DEACTIVE instead of the literal key.
---
Outside diff comments:
In `@src/odemis/driver/smaract.py`:
- Around line 2918-2945: Annotate the complete constructor signature in
src/odemis/driver/smaract.py:2918-2945 and update its parameter documentation to
remove type information and inline backticks; apply the requested
parameter/return annotations and compliant docstrings without inline backticks
at src/odemis/driver/smaract.py:3658-3673, 3739-3761, and 3808-3837, including
parameterized collection types and -> None where specified. In
src/odemis/driver/test/smaract_test.py:849-856, add a setUp docstring and ->
None plus annotate tearDown; add -> None to every test method at
src/odemis/driver/test/smaract_test.py:858-943.
---
Duplicate comments:
In `@src/odemis/driver/smaract.py`:
- Around line 3658-3673: Update _reference_multi_phase to preserve the caller’s
requested axes instead of always using set(self.axes.keys()). When the request
covers only a subset of axes, route it through the existing standard referencing
behavior; retain the multi-phase submission only for the appropriate full-axis
request.
- Around line 3068-3075: Move the refproc normalization and validation in the
initialization flow before opening the controller or creating the executor,
accepting only REFPROC_MULTI_PHASE and REFPROC_STD. Preserve the
procedure-specific self.reference assignment and logging after resources are
allocated, and raise ValueError immediately for unknown values.
---
Nitpick comments:
In `@src/odemis/driver/smaract.py`:
- Around line 3783-3784: Update the exception handling around the axis
referencing flow to catch only the expected motion or hardware exception types,
rather than the broad Exception class. Preserve the existing debug logging and
retry behavior for those expected failures, while allowing programming and
state-update errors to propagate immediately.
🪄 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: edce50ab-37e0-4bb5-a8e6-f31dc5a7c29f
📒 Files selected for processing (3)
install/linux/usr/share/odemis/sim/sparc2-smaract-mirror-sim.odm.yamlsrc/odemis/driver/smaract.pysrc/odemis/driver/test/smaract_test.py
nandishjpatel
force-pushed
the
GANIL-134-extend-smaract-driver-to-support-ganil-actuators
branch
from
July 16, 2026 10:07
0a15532 to
98586d3
Compare
nandishjpatel
marked this pull request as ready for review
July 16, 2026 11:14
There was a problem hiding this comment.
Pull request overview
Extends the SmarAct MCS2 actuator driver with an optional “multi-phase” referencing (homing) procedure intended for mechanically constrained systems (e.g., L-slot geometries), and adds simulator configuration plus tests to validate the new behavior.
Changes:
- Added
refprocselection tosmaract.MCS2with a new metadata-driven multi-phase referencing implementation. - Added a dedicated simulator ODM YAML configuration for a SPARC v2 SmarAct mirror setup using multi-phase referencing.
- Added unit tests covering success paths and metadata validation failures for the multi-phase procedure.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/odemis/driver/smaract.py |
Adds refproc selection and implements multi-phase referencing with retry/phase sequencing. |
src/odemis/driver/test/smaract_test.py |
Adds simulator-based tests for multi-phase referencing and metadata error cases. |
install/linux/usr/share/odemis/sim/sparc2-smaract-mirror-sim.odm.yaml |
Introduces a SPARC v2 simulator configuration using refproc: "multi-phase" and required metadata. |
nandishjpatel
requested review from
Copilot,
pieleric and
tmoerkerken
and removed request for
tepals
July 21, 2026 07:44
nandishjpatel
force-pushed
the
GANIL-134-extend-smaract-driver-to-support-ganil-actuators
branch
from
July 23, 2026 09:47
98586d3 to
e85c3a9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/odemis/driver/smaract.py (1)
3741-3761: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse plain-text docstrings (no RST directives or backticks). The new/updated docstrings use
:param:/:return:/:raises:directives and backtick-quoted names, which violate the repo convention of plain, unformatted docstrings. As per coding guidelines ("Include docstrings ... without using inline formatting markers or backticks") and based on learnings (keep docstrings plain text; do not use RST markup such as ":param:", ":return:", ":type:").
src/odemis/driver/smaract.py#L3741-L3761: rewrite_reference_with_retrydocstring as plain prose; drop:param:/:raises:and backticks.src/odemis/driver/smaract.py#L3659-L3668: rewrite_reference_multi_phasedocstring; drop:return:and backticks.src/odemis/driver/smaract.py#L3809-L3836: rewrite_do_reference_multi_phasedocstring; drop:param:/:raises:and backticks.src/odemis/driver/smaract.py#L2937-L2945: remove backticks aroundAXES_ORDER_REF,FAV_POS_ALIGN,FAV_POS_ACTIVE,FAV_POS_DEACTIVE.🤖 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/driver/smaract.py` around lines 3741 - 3761, Rewrite the docstrings for _reference_with_retry (src/odemis/driver/smaract.py lines 3741-3761), _reference_multi_phase (lines 3659-3668), and _do_reference_multi_phase (lines 3809-3836) as plain prose, removing all RST directives such as :param:, :return:, and :raises: plus backtick formatting while preserving their descriptions. At lines 2937-2945, remove backticks around AXES_ORDER_REF, FAV_POS_ALIGN, FAV_POS_ACTIVE, and FAV_POS_DEACTIVE; no other changes are required.Sources: Coding guidelines, 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.
Nitpick comments:
In `@src/odemis/driver/smaract.py`:
- Around line 3741-3761: Rewrite the docstrings for _reference_with_retry
(src/odemis/driver/smaract.py lines 3741-3761), _reference_multi_phase (lines
3659-3668), and _do_reference_multi_phase (lines 3809-3836) as plain prose,
removing all RST directives such as :param:, :return:, and :raises: plus
backtick formatting while preserving their descriptions. At lines 2937-2945,
remove backticks around AXES_ORDER_REF, FAV_POS_ALIGN, FAV_POS_ACTIVE, and
FAV_POS_DEACTIVE; no other changes are required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be675b31-6ae7-4ba1-9f18-fb421af64428
📒 Files selected for processing (3)
install/linux/usr/share/odemis/sim/sparc2-smaract-mirror-sim.odm.yamlsrc/odemis/driver/smaract.pysrc/odemis/driver/test/smaract_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- install/linux/usr/share/odemis/sim/sparc2-smaract-mirror-sim.odm.yaml
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/odemis/driver/smaract.py:3661
- _reference_multi_phase ignores the provided
axesargument and does not call_checkReference(...). This means callers requesting a subset (or even an invalid set) will silently run a full-device multi-phase reference, which is inconsistent withreference(self, axes)and can be surprising for generic callers (e.g., CLI axis-level referencing). Consider validating the requested axes and explicitly documenting/logging that multi-phase references all axes together.
@isasync
def _reference_multi_phase(self, _=None):
"""
Asynchronous entry point to execute the multi-phase referencing sequence.
src/odemis/driver/smaract.py:3740
- New multi-phase helpers have incomplete/too-broad type hints and no return type annotations. Adding precise container types and
-> Noneimproves readability and static checking (and matches the project’s typed style in new code).
def _reference_with_retry(self, future: model.CancellableFuture, axes_to_ref: list,
post_ref_positions: Optional[dict] = None, reference_lbl: str = ""):
src/odemis/driver/smaract.py:3808
_do_reference_multi_phaseis missing an explicit return type annotation. Declaring-> Nonemakes the intent clear and keeps typing consistent with the new helper methods.
def _do_reference_multi_phase(self, future: model.CancellableFuture, axes: Set[str]):
pieleric
requested changes
Aug 5, 2026
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/odemis/driver/smaract.py:3673
- _reference_multi_phase currently ignores any provided axes and always references all mapped axes. This breaks the Actuator.reference(axes) contract (model/_components.py:863) and can cause unintended motion if a caller requests only a subset (e.g., {"x"}). Consider validating the argument and failing fast when a subset is requested.
@isasync
def _reference_multi_phase(self, _=None):
"""
Asynchronous entry point to execute the multi-phase referencing sequence.
This method automatically collects all mapped axes, verifies their states,
and submits the `_do_reference_multi_phase` operation to the background executor.
No arguments are required, as axis handling is derived entirely from the
device metadata properties (`AXES_ORDER_REF`, etc.).
:return: A CancellableFuture object tracking the state of the background operation.
"""
axes = set(self.axes.keys())
f = self._createMoveFuture()
f = self._executor.submitf(f, self._do_reference_multi_phase, f, axes)
return f
src/odemis/driver/smaract.py:3783
- _reference_with_retry is intended to tolerate failures and retry, but self.Reference(channel) is outside the try/except. If the SDK call raises immediately, the whole multi-phase reference aborts instead of retrying the remaining axes.
logging.info("Referencing %s axis %s (Attempt %d)", reference_lbl, a, attempt + 1)
self.Reference(channel)
try:
self._waitEndMove(future, {channel}, time.time() + 100)
except CancelledError:
raise
except Exception as e:
logging.debug("Axis %s failed to reference: %s", a, e)
install/linux/usr/share/odemis/sim/sparc2-smaract-mirror-sim.odm.yaml:26
- Typo in comment: “spectograph” should be “spectrograph”.
"Spectrograph": 2, # no spectograph in SPARC Compact
pieleric
approved these changes
Aug 6, 2026
tmoerkerken
requested changes
Aug 10, 2026
… _doMoveAbs already handles it internally
nandishjpatel
force-pushed
the
GANIL-134-extend-smaract-driver-to-support-ganil-actuators
branch
from
August 15, 2026 13:00
08f0b6d to
f3ec659
Compare
tmoerkerken
approved these changes
Aug 20, 2026
tmoerkerken
approved these changes
Aug 20, 2026
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.
No description provided.