[GANIL-154] create a odemis backend component to control the nd filter#3493
Open
nandishjpatel wants to merge 2 commits into
Conversation
…(ON/OFF) axes controlled via MCC DAQ digital outputs, with position feedback via separate digital inputs
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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: 2
🧹 Nitpick comments (8)
src/odemis/driver/test/pwrmccdaq_test.py (4)
274-342: ⚡ Quick winAdd return annotations and setup/teardown docstrings.
The new test methods should be annotated with
-> None, andsetUp/tearDownneed docstrings.As per coding guidelines,
**/*.pymust use type hints for function parameters and return types and include reStructuredText docstrings for all functions/classes.♻️ Example cleanup
- def setUp(self): + def setUp(self) -> None: + """Create the simulated MCC DIO actuator.""" self.controller = pwrmccdaq.MCCDeviceDIOActuator(**CONFIG_CONTROLLER_DEVICE) - def tearDown(self): + def tearDown(self) -> None: + """Terminate the simulated MCC DIO actuator.""" if self.controller: self.controller.terminate() - def test_initial_state(self): + def test_initial_state(self) -> None:🤖 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/pwrmccdaq_test.py` around lines 274 - 342, The test class methods are missing required type annotations and docstrings per coding guidelines. Add `-> None` return type annotations to all test methods (test_initial_state, test_move_abs, test_move_abs_invalid, test_interlock, test_terminate) and to the setUp and tearDown methods. Additionally, add reStructuredText docstrings to setUp and tearDown methods to document their purpose, following the same format as the docstrings in the test_interlock method.Source: Coding guidelines
337-342: ⚡ Quick winAssert
terminate()stops the DI polling thread too.This actuator starts
MCCDeviceDIStatusfor feedback/interlock DI channels. The teardown test should verify the inherited polling thread is terminated, not only that the DO latch is low.🧪 Proposed assertion
self.controller.terminate() # After terminate, the DO pin should be low port, bit = self.controller.channel_to_port(0) val = self.controller.device.DBitIn(port, bit) self.assertEqual(val, 0) + self.assertTrue(self.controller._status_thread.terminated) + self.assertFalse(self.controller._status_thread.is_alive()) self.controller = None # prevent tearDown from calling terminate() again🤖 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/pwrmccdaq_test.py` around lines 337 - 342, The test currently only verifies that the DO pin becomes low after calling terminate() on the controller, but it does not verify that the DI polling thread started by MCCDeviceDIStatus is actually terminated. Add an assertion after the self.controller.terminate() call to verify that the polling thread associated with MCCDeviceDIStatus (which provides feedback/interlock for DI channels) has been stopped. Check the thread's state or alive status to confirm it is no longer running, ensuring complete cleanup of all resources when terminate() is invoked.
285-295: ⚡ Quick winAdd coverage for the cancellable move path.
test_move_absonly covers completed moves, while the new actuator adds_cancelCurrentMoveand the cancellation branch in_waitEndMove. Add a long-duration move test that cancels/stops the future and assertsmodel.CancelledErrorplus the final reported position.🤖 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/pwrmccdaq_test.py` around lines 285 - 295, The test_move_abs method currently only covers completed moves and does not exercise the cancellation code path in _cancelCurrentMove and _waitEndMove. Add a test case that initiates a long-duration move operation using moveAbs on the controller, cancel or stop the returned future before completion, verify that model.CancelledError is raised when calling result on the cancelled future, and assert that the final position value reported by the controller is correct after the cancellation occurs.
311-329: ⚡ Quick winReplace fixed sleeps with a bounded polling wait.
The
0.15second sleeps are only slightly aboveINTERLOCK_POLL_INTERVAL, so this can flake under scheduler load. Poll until the VA reaches the expected value or a clear timeout expires.🧪 Proposed helper
+ def _wait_for_interlock(self, expected: bool) -> None: + """ + Wait until the interlock VA reaches the expected state. + + :param expected: expected interlock state. + """ + deadline = time.monotonic() + 1.0 + while time.monotonic() < deadline: + if self.controller.interlockTriggered.value == expected: + return + time.sleep(pwrmccdaq.INTERLOCK_POLL_INTERVAL / 2) + self.assertEqual(self.controller.interlockTriggered.value, expected) + @@ - time.sleep(0.15) # let the polling thread do one cycle - self.assertFalse(self.controller.interlockTriggered.value) + self._wait_for_interlock(False) @@ - time.sleep(0.15) # wait longer than the poll interval - - self.assertTrue(self.controller.interlockTriggered.value) + self._wait_for_interlock(True) @@ - time.sleep(0.15) - - self.assertFalse(self.controller.interlockTriggered.value) + self._wait_for_interlock(False)🤖 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/pwrmccdaq_test.py` around lines 311 - 329, Replace the three fixed time.sleep(0.15) calls with a bounded polling mechanism that repeatedly checks self.controller.interlockTriggered.value until it reaches the expected state (False initially, True after setting the bit, then False after clearing it) or a clear timeout expires. This approach will make the test more robust by not relying on a fixed sleep duration that may be insufficient under scheduler load while still providing a safety timeout to prevent the test from hanging indefinitely.src/odemis/driver/pwrmccdaq.py (4)
532-559: ⚡ Quick winUse a monotonic clock for transition timing.
Line 543 and Line 555 use wall-clock time for elapsed-duration logic; an NTP/system clock jump can make moves finish early or wait too long. Use
time.monotonic()for the transition deadline.⏱️ Proposed timing change
with future._moving_lock: - end = 0 + end = 0.0 @@ - end = max(end, time.time() + dur) + end = max(end, time.monotonic() + dur) @@ - left = end - time.time() + left = end - time.monotonic()🤖 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/pwrmccdaq.py` around lines 532 - 559, The transition timing logic in the _waitEndMove method and the end calculation uses wall-clock time (time.time()) which can be affected by NTP or system clock adjustments, causing timing issues. Replace all instances of time.time() with time.monotonic() in both locations where the end deadline is calculated and where the remaining time is checked. The monotonic clock is not affected by system clock adjustments and provides reliable elapsed-time measurements for the transition duration logic.
463-463: ⚡ Quick winAdd the required annotations and docstrings to the new methods.
Most new methods omit return annotations, and
moveAbs,moveRel,stop,_checkMoveAbs,_doMoveAbs,_waitEndMove,_cancelCurrentMove, andterminatestill need complete parameter/return annotations;stop,_checkMoveAbs, andterminatealso need function docstrings.As per coding guidelines,
**/*.pymust use type hints for function parameters and return types and include reStructuredText docstrings for all functions.♻️ Example shape for the signatures/docstrings
- def _updatePosition(self): + def _updatePosition(self) -> None: @@ - def _createMoveFuture(self): + def _createMoveFuture(self) -> model.CancellableFuture: @@ - def stop(self, axes=None): + def stop(self, axes: Optional[List[str]] = None) -> None: + """ + Stop queued or active moves. + + :param axes: axes to stop; ignored because this actuator uses one worker. + """ self._executor.cancel()Also applies to: 486-486, 496-496, 508-508, 513-516, 526-526, 550-550, 566-566, 574-574
🤖 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/pwrmccdaq.py` at line 463, Add complete type annotations and docstrings to the methods listed: _updatePosition, moveAbs, moveRel, stop, _checkMoveAbs, _doMoveAbs, _waitEndMove, _cancelCurrentMove, and terminate. For each method, add type hints for all function parameters and ensure each method has a return type annotation (including -> None for methods that do not return values). Additionally, add reStructuredText docstrings to all methods that lack them, with particular focus on stop, _checkMoveAbs, and terminate. The docstrings should follow the project's conventions and document the purpose, parameters, and return values of each function.Source: Coding guidelines
583-587: ⚡ Quick winNarrow the termination reset exception handling.
Line 586 swallows all exceptions, including programming/configuration errors such as invalid channels, and Line 587 drops the traceback. Catch the expected MCC hardware exception type, or document and log traceback if broader cleanup handling is intentional.
Ruff flags BLE001 here.
🛠️ Proposed narrower catch
try: with self._connection_lock: self.device.DBitOut(port, bit, 0) - except Exception: - logging.warning("Failed to reset DO channel %d on terminate", channel) + except HwError: + logging.exception("Failed to reset DO channel %d on terminate", channel)🤖 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/pwrmccdaq.py` around lines 583 - 587, The exception handling in the termination reset code for the DO channel (in the section with self.device.DBitOut call) is too broad. Replace the generic Exception catch with a more specific MCC hardware exception type to avoid masking programming or configuration errors. If broader exception handling is necessary for cleanup purposes, document why and include the exception traceback in the logging.warning call to preserve debugging information. Reference the specific exception class that DBitOut raises to make the catch more precise.Source: Linters/SAST tools
374-378: ⚡ Quick winClear the constructor guideline and Ruff violations.
Line 377 keeps a shared mutable default, and Line 448 unpacks unused loop variables. Use
None, initialize a local dict, and add the required constructor return annotation.As per coding guidelines,
**/*.pymust use type hints for function parameters and return types; Ruff also flags B006/B007 here.♻️ Proposed cleanup
def __init__(self, name: str, role: str, mcc_device: Optional[str], do_axes: Dict[int, Tuple[str, str, str, float]], di_feedback: Dict[str, Tuple[int, bool]], - di_channels: Dict[int, Tuple[str, bool]] = {}, - **kwargs): + di_channels: Optional[Dict[int, Tuple[str, bool]]] = None, + **kwargs) -> None: @@ - all_di_channels = dict(di_channels) + all_di_channels = dict(di_channels or {}) @@ - for channel, (an, hpos, lpos, dur) in self._do_axes.items(): + for channel in self._do_axes:Also applies to: 434-434, 448-451
🤖 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/pwrmccdaq.py` around lines 374 - 378, In the __init__ method of the pwrmccdaq.py file, change the mutable default argument for the di_channels parameter from {} to None, then add a -> None return type annotation to the method signature. Inside the method body, check if di_channels is None and initialize it as an empty dictionary if needed. Additionally, locate the loop at line 448 and fix any unused loop variable unpacking by either using the variables appropriately or replacing them with underscores as needed, and apply the same mutable default and annotation fixes to any other similar constructors mentioned at lines 434 and 448-451.Sources: Coding guidelines, 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/pwrmccdaq.py`:
- Around line 550-564: The _waitEndMove method has a logic issue where
self._updatePosition() is called after the else block that raises
model.CancelledError(), meaning the position VA is never updated if the move is
cancelled. Move the self._updatePosition() call to execute before raising the
CancelledError in the else block (after setting future._was_stopped = True but
before the raise statement), so the position is refreshed even when cancellation
occurs after the DO bit has been changed.
- Around line 432-440: The code currently validates that di_feedback channels
don't conflict with user-provided di_channels, but it doesn't check if any
channels configured in do_axes are also present in di_channels or di_feedback.
Add validation before the merge loop that iterates through do_axes and checks
each channel against both the di_channels dictionary and the self._di_feedback
dictionary keys. If any do_axes channel is found in either location, raise a
ValueError with a message indicating that the channel is configured as both
output and input, which would break hardware control. This validation should be
added in the same section where the existing conflict check is performed.
---
Nitpick comments:
In `@src/odemis/driver/pwrmccdaq.py`:
- Around line 532-559: The transition timing logic in the _waitEndMove method
and the end calculation uses wall-clock time (time.time()) which can be affected
by NTP or system clock adjustments, causing timing issues. Replace all instances
of time.time() with time.monotonic() in both locations where the end deadline is
calculated and where the remaining time is checked. The monotonic clock is not
affected by system clock adjustments and provides reliable elapsed-time
measurements for the transition duration logic.
- Line 463: Add complete type annotations and docstrings to the methods listed:
_updatePosition, moveAbs, moveRel, stop, _checkMoveAbs, _doMoveAbs,
_waitEndMove, _cancelCurrentMove, and terminate. For each method, add type hints
for all function parameters and ensure each method has a return type annotation
(including -> None for methods that do not return values). Additionally, add
reStructuredText docstrings to all methods that lack them, with particular focus
on stop, _checkMoveAbs, and terminate. The docstrings should follow the
project's conventions and document the purpose, parameters, and return values of
each function.
- Around line 583-587: The exception handling in the termination reset code for
the DO channel (in the section with self.device.DBitOut call) is too broad.
Replace the generic Exception catch with a more specific MCC hardware exception
type to avoid masking programming or configuration errors. If broader exception
handling is necessary for cleanup purposes, document why and include the
exception traceback in the logging.warning call to preserve debugging
information. Reference the specific exception class that DBitOut raises to make
the catch more precise.
- Around line 374-378: In the __init__ method of the pwrmccdaq.py file, change
the mutable default argument for the di_channels parameter from {} to None, then
add a -> None return type annotation to the method signature. Inside the method
body, check if di_channels is None and initialize it as an empty dictionary if
needed. Additionally, locate the loop at line 448 and fix any unused loop
variable unpacking by either using the variables appropriately or replacing them
with underscores as needed, and apply the same mutable default and annotation
fixes to any other similar constructors mentioned at lines 434 and 448-451.
In `@src/odemis/driver/test/pwrmccdaq_test.py`:
- Around line 274-342: The test class methods are missing required type
annotations and docstrings per coding guidelines. Add `-> None` return type
annotations to all test methods (test_initial_state, test_move_abs,
test_move_abs_invalid, test_interlock, test_terminate) and to the setUp and
tearDown methods. Additionally, add reStructuredText docstrings to setUp and
tearDown methods to document their purpose, following the same format as the
docstrings in the test_interlock method.
- Around line 337-342: The test currently only verifies that the DO pin becomes
low after calling terminate() on the controller, but it does not verify that the
DI polling thread started by MCCDeviceDIStatus is actually terminated. Add an
assertion after the self.controller.terminate() call to verify that the polling
thread associated with MCCDeviceDIStatus (which provides feedback/interlock for
DI channels) has been stopped. Check the thread's state or alive status to
confirm it is no longer running, ensuring complete cleanup of all resources when
terminate() is invoked.
- Around line 285-295: The test_move_abs method currently only covers completed
moves and does not exercise the cancellation code path in _cancelCurrentMove and
_waitEndMove. Add a test case that initiates a long-duration move operation
using moveAbs on the controller, cancel or stop the returned future before
completion, verify that model.CancelledError is raised when calling result on
the cancelled future, and assert that the final position value reported by the
controller is correct after the cancellation occurs.
- Around line 311-329: Replace the three fixed time.sleep(0.15) calls with a
bounded polling mechanism that repeatedly checks
self.controller.interlockTriggered.value until it reaches the expected state
(False initially, True after setting the bit, then False after clearing it) or a
clear timeout expires. This approach will make the test more robust by not
relying on a fixed sleep duration that may be insufficient under scheduler load
while still providing a safety timeout to prevent the test from hanging
indefinitely.
🪄 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: 5dc49dc0-0749-4aec-baac-637bf8d9d6f8
📒 Files selected for processing (2)
src/odemis/driver/pwrmccdaq.pysrc/odemis/driver/test/pwrmccdaq_test.py
pieleric
requested changes
Jun 17, 2026
| :param do_axes: | ||
| Digital output channel -> [axis_name, high_position_name, low_position_name, transition_duration_s]. | ||
| Each entry defines a binary axis controlled via a DO channel. | ||
| Example: {0: ["nd-filter", "on", "off", 0.5]} |
Member
There was a problem hiding this comment.
Typically, in Odemis we use floats for the position of the axes. If you want to associate a user-friendly name to a position, you can use a dict in .choices: float -> str. (eg: {0.0: "on", 1.57: "off"})
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/odemis/driver/pwrmccdaq.py (3)
455-513: ⚡ Quick winAdd return type hints per coding guidelines.
Methods like
_updatePosition,_createMoveFuture,moveAbs,moveRel,stop, and_checkMoveAbslack return type annotations. As per coding guidelines: "Always use type hints for function parameters and return types in Python code."♻️ Example fixes
- def _updatePosition(self): + def _updatePosition(self) -> None: """Read the actual position from the DI feedback channels.""" - def _createMoveFuture(self): + def _createMoveFuture(self) -> model.CancellableFuture: """Return a CancellableFuture for a move.""" - def stop(self, axes=None): + def stop(self, axes: Optional[Set[str]] = None) -> None: - def _checkMoveAbs(self, pos): + def _checkMoveAbs(self, pos: Dict[str, float]) -> None:🤖 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/pwrmccdaq.py` around lines 455 - 513, The methods _updatePosition, _createMoveFuture, moveAbs, moveRel, stop, and _checkMoveAbs are missing return type annotations. Add return type hints to all these method definitions by including the -> notation in each method signature (e.g., def _updatePosition() -> None:). Determine the appropriate return type for each method: _updatePosition, stop, and _checkMoveAbs should return None, while _createMoveFuture should return the model.CancellableFuture type, and moveAbs and moveRel should return the corresponding future types returned by the actual implementations.Source: Coding guidelines
66-66: 💤 Low valueReplace mutable default arguments with
None.Using mutable
{}as default argument is a known Python pitfall. While the current code doesn't mutate it, it's safer to useNoneand initialize inside.♻️ Proposed fix
def __init__(self, name: str, role: str, mcc_device: Optional[str], - di_channels: Dict[int, Tuple[str, bool]] = {}, children: Dict[str, dict] = {}, daemon=None, **kwargs): + di_channels: Dict[int, Tuple[str, bool]] = None, children: Dict[str, dict] = None, daemon=None, **kwargs):Then at the start of the method body:
di_channels = di_channels or {} children = children or {}🤖 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/pwrmccdaq.py` at line 66, Replace the mutable default arguments in the function signature that contains the parameters di_channels and children. Change both default values from empty dictionaries {} to None, then add initialization statements at the start of the function body that use the pattern "parameter = parameter or {}" to set them to empty dictionaries only when None is passed, avoiding the Python pitfall of shared mutable default arguments across function calls.Source: Linters/SAST tools
424-428: 💤 Low valueUse exception chaining for clarity.
Per B904, re-raised exceptions should use
from Noneto indicate intentional replacement.♻️ Proposed fix
try: parent.channel_to_port(do_ch) except ValueError: - raise ValueError(f"Axis '{axis_name}' has invalid 'do' channel {do_ch}") + raise ValueError(f"Axis '{axis_name}' has invalid 'do' channel {do_ch}") from NoneApply the same pattern at line 438.
🤖 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/pwrmccdaq.py` around lines 424 - 428, The ValueError exception being raised in the try-except block should use explicit exception chaining by adding `from None` at the end of the raise statement to indicate intentional exception replacement per B904. Update the raise statement that catches the ValueError from parent.channel_to_port(do_ch) and raises a new ValueError with the axis validation message to include `from None`. Apply the same pattern to the similar exception handling at line 438.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/pwrmccdaq.py`:
- Around line 136-141: The variable kwargs is assigned on line 137 in the try
block but is never actually used—the else block directly accesses
children["actuator"] when creating the MCCDeviceDIOActuator instance. Remove the
unused kwargs assignment while keeping the try-except-else structure intact, as
it is still needed to check if the "actuator" key exists and handle the KeyError
and TypeError exceptions appropriately.
- Around line 432-439: The DI feedback channels stored in _di_feedback for each
axis_name are not being configured as inputs at the hardware level through
MCCDevice. After storing the di_ch and di_inverted values in _di_feedback within
MCCDeviceDIOActuator, you need to ensure these channels are explicitly
configured as inputs in the parent MCCDevice. Add a call to a parent method
(such as one that configures specific DIO channels as inputs) immediately after
the _di_feedback assignment to register these channels with MCCDevice for proper
hardware configuration before any subsequent read operations occur on these
channels.
---
Nitpick comments:
In `@src/odemis/driver/pwrmccdaq.py`:
- Around line 455-513: The methods _updatePosition, _createMoveFuture, moveAbs,
moveRel, stop, and _checkMoveAbs are missing return type annotations. Add return
type hints to all these method definitions by including the -> notation in each
method signature (e.g., def _updatePosition() -> None:). Determine the
appropriate return type for each method: _updatePosition, stop, and
_checkMoveAbs should return None, while _createMoveFuture should return the
model.CancellableFuture type, and moveAbs and moveRel should return the
corresponding future types returned by the actual implementations.
- Line 66: Replace the mutable default arguments in the function signature that
contains the parameters di_channels and children. Change both default values
from empty dictionaries {} to None, then add initialization statements at the
start of the function body that use the pattern "parameter = parameter or {}" to
set them to empty dictionaries only when None is passed, avoiding the Python
pitfall of shared mutable default arguments across function calls.
- Around line 424-428: The ValueError exception being raised in the try-except
block should use explicit exception chaining by adding `from None` at the end of
the raise statement to indicate intentional exception replacement per B904.
Update the raise statement that catches the ValueError from
parent.channel_to_port(do_ch) and raises a new ValueError with the axis
validation message to include `from None`. Apply the same pattern to the similar
exception handling at line 438.
🪄 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: 9732035e-3d9c-4a26-a1d3-fb21f9501f3a
📒 Files selected for processing (2)
src/odemis/driver/pwrmccdaq.pysrc/odemis/driver/test/pwrmccdaq_test.py
pieleric
requested changes
Jun 23, 2026
pieleric
requested changes
Jun 23, 2026
| if not 0 <= duration < 1000: | ||
| raise ValueError(f"Axis '{axis_name}' duration {duration} should be between 0 and 1000 s") | ||
| try: | ||
| parent.channel_to_port(do_ch) |
Member
There was a problem hiding this comment.
I missed something in the first reviews: the device must have the (whole) port (A/B) configured explicitely either for reading or writing. This is done originally by this line self.device.DConfig(port, val). However, until now, that was easy, because the code only needed reading. Now, we need to be extra careful, and make sure that a given port only has either DO or DI channels. It should also be documented in the docstring.
nandishjpatel
force-pushed
the
GANIL-154-create-a-odemis-backend-component-to-control-the-nd-filter
branch
from
c9551a5 to
7bc5d43
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/odemis/driver/test/pwrmccdaq_test.py (1)
270-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return type hints to the new test methods.
The added test methods omit
-> None, which violates the repo-wide Python typing rule.Suggested change
- def test_child_nd_filter_port_conflict(self): + def test_child_nd_filter_port_conflict(self) -> None: ... - def test_child_nd_filter_di_conflict_with_parent(self): + def test_child_nd_filter_di_conflict_with_parent(self) -> None: ... - def test_child_nd_filter(self): + def test_child_nd_filter(self) -> None: ... - def test_child_nd_filter_move(self): + def test_child_nd_filter_move(self) -> None: ... - def test_child_nd_filter_invalid_move(self): + def test_child_nd_filter_invalid_move(self) -> 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/driver/test/pwrmccdaq_test.py` around lines 270 - 360, The new test methods in MCCDeviceLight’s test class are missing explicit return type annotations, which breaks the repo typing convention. Add an explicit “-> None” return type to each added test method, including test_child_nd_filter_port_conflict, test_child_nd_filter_di_conflict_with_parent, test_child_nd_filter, test_child_nd_filter_move, and test_child_nd_filter_invalid_move, keeping the existing method names unchanged so they remain easy to locate.Source: Coding guidelines
src/odemis/driver/pwrmccdaq.py (1)
65-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete annotations and docstrings on changed Python functions.
Several changed/new functions still miss return annotations or parameter annotations, and
stop(),_checkMoveAbs(), andterminate()lack docstrings. 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”.Also applies to: 229-230, 389-389, 482-629, 635-639
🤖 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/pwrmccdaq.py` around lines 65 - 66, The changed Python functions in pwrmccdaq are missing required type hints and some method docstrings. Update the relevant class initializer and other modified methods to include complete parameter and return annotations, and add docstrings for stop(), _checkMoveAbs(), and terminate(). Make sure the affected class/method signatures in pwrmccdaq follow the project’s typing and documentation conventions consistently.Source: Coding guidelines
🤖 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/pwrmccdaq.py`:
- Around line 136-143: Create and validate the actuator before starting the DI
polling thread in the MCC device setup logic. In the constructor path that
creates MCCDeviceDIStatus and MCCDeviceDIOActuator, move the actuator
initialization ahead of MCCDeviceDIStatus.start() so any validation failure
happens before the poller begins. Make the same ordering change in the other
matching construction path so DIO is not reconfigured while the status thread
may be reading.
- Line 66: The constructor in pwrmccdaq still uses shared mutable default
dictionaries for di_channels and children, which Ruff flags and can leak state
between instances. Update the affected initializer(s) to default those
parameters to None, then normalize them to empty dicts inside the constructor
before use; apply the same change to the other matching occurrence in the same
module.
- Around line 591-592: The DI feedback wait logic currently uses a timeout
derived directly from duration, so a zero duration can make the wait expire
immediately before feedback settles. Update the timeout calculation in the
pwrmccdaq feedback path to enforce a small non-zero minimum timeout, or guard
DI-backed axes so they require a positive duration before entering this wait
loop. Keep the change localized to the block that sets timeout and sleept so the
settling behavior remains consistent.
- Around line 418-460: The axis validation in the pwrmccdaq configuration
currently rejects shared ports only against parent DI channels, but it must also
block any axis DI port that overlaps the parent light DO ports because setting
parent.dconfig[di_port] to input can disable _update_power() writes. Update the
validation in the axis setup logic around channel_to_port, _di_channels, and
_do_channels so any axis 'di' port conflicts with either parent DI or parent DO
usage, and raise a clear ValueError before storing the feedback config.
In `@src/odemis/driver/test/pwrmccdaq_test.py`:
- Around line 351-359: Exercise the unsupported-position branch in the ND filter
move test: the current test_child_nd_filter_invalid_move only covers the
unknown-axis error path in MCCDeviceDIOActuator._checkMoveAbs() by passing an
invalid axis name. Update this test to also call actuator.moveAbs() with a valid
axis name from the actuator contract but an unsupported value, and assert it
raises ValueError so the known-axis/invalid-position branch is covered as well.
---
Nitpick comments:
In `@src/odemis/driver/pwrmccdaq.py`:
- Around line 65-66: The changed Python functions in pwrmccdaq are missing
required type hints and some method docstrings. Update the relevant class
initializer and other modified methods to include complete parameter and return
annotations, and add docstrings for stop(), _checkMoveAbs(), and terminate().
Make sure the affected class/method signatures in pwrmccdaq follow the project’s
typing and documentation conventions consistently.
In `@src/odemis/driver/test/pwrmccdaq_test.py`:
- Around line 270-360: The new test methods in MCCDeviceLight’s test class are
missing explicit return type annotations, which breaks the repo typing
convention. Add an explicit “-> None” return type to each added test method,
including test_child_nd_filter_port_conflict,
test_child_nd_filter_di_conflict_with_parent, test_child_nd_filter,
test_child_nd_filter_move, and test_child_nd_filter_invalid_move, keeping the
existing method names unchanged so they remain easy to locate.
🪄 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: 86b44d43-e3a3-45de-b25f-8e709e7b8870
📒 Files selected for processing (3)
src/odemis/driver/_mccdaq.pysrc/odemis/driver/pwrmccdaq.pysrc/odemis/driver/test/pwrmccdaq_test.py
💤 Files with no reviewable changes (1)
- src/odemis/driver/_mccdaq.py
There was a problem hiding this comment.
Pull request overview
This PR introduces a new MCC DAQ “child actuator” component to control binary ON/OFF axes via digital outputs, with optional digital-input feedback (intended for an ND filter use case), and wires it into MCCDevice/MCCDeviceLight via a children configuration entry.
Changes:
- Add
MCCDeviceDIOActuatorand support for instantiating it as a child ofMCCDevice/MCCDeviceLight. - Capture initial DIO pin states at startup and reconfigure DIO directions based on DI usage and child actuator needs.
- Extend unit tests to validate child actuator creation/moves and configuration conflict checks; adjust MCC DAQ USB class init defaults.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/odemis/driver/pwrmccdaq.py | Adds child actuator support, initial DIO state capture, and new MCCDeviceDIOActuator implementation. |
| src/odemis/driver/test/pwrmccdaq_test.py | Adds ND filter child configuration and tests for conflicts and move behavior. |
| src/odemis/driver/_mccdaq.py | Removes default DIO/AO initialization from usb_1208LS constructor. |
Comment on lines
+65
to
+66
| def __init__(self, name: str, role: str, mcc_device: Optional[str], | ||
| di_channels: Dict[int, Tuple[str, bool]] = {}, **kwargs): | ||
| di_channels: Dict[int, Tuple[str, bool]] = {}, children: Dict[str, dict] = {}, daemon=None, **kwargs): |
nandishjpatel
force-pushed
the
GANIL-154-create-a-odemis-backend-component-to-control-the-nd-filter
branch
from
7bc5d43 to
3d1fd45
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/odemis/driver/pwrmccdaq.py (3)
589-590: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRefresh position before raising cancellation.
If cancellation is requested after the DO bit was written,
_checkCancelled()raises beforepositionis refreshed, leaving the reported state stale.Proposed cancellation handling
""" future._must_stop.wait(duration) - self._checkCancelled(future) + if future._must_stop.is_set(): + self._updatePosition() + future._was_stopped = True + raise model.CancelledError() di_axes = {axis: v for axis, v in pos.items() if axis in self._di_feedback}🤖 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/pwrmccdaq.py` around lines 589 - 590, The cancellation path in pwrmccdaq’s position-setting flow leaves stale state because _checkCancelled() can raise before the latest position is refreshed. Update the logic around the wait/check sequence so the current position is read or assigned immediately after the DO write and before calling _checkCancelled(future), using the existing future and position update path in the same method.
486-499: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not drive actuator outputs during initialization.
_initPosition()writes each DO bit on startup. For the ND filter, initialization should only report the current state; writing the output can actuate hardware before an explicit move request.Proposed change
- # Calling DConfig sets channel value to LOW, set the DO pins to their initial values - # and update the position accordingly + # Read the current position without moving the axes. self._initPosition() @@ def _initPosition(self): - """Set DO pins to their initial hardware values and update position accordingly.""" - pos = {} - for axis_name, (do_ch, low, high, _) in self._axes_cfg.items(): - port, bit = self.parent.channel_to_port(do_ch) - val = self.parent.init_dio_values[do_ch] - pos[axis_name] = high if val else low - with self.parent._connection_lock: - self.parent.device.DBitOut(port, bit, val) - self.position._set_value(pos, force_write=True) + """Read the initial hardware position.""" + self._updatePosition()🤖 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/pwrmccdaq.py` around lines 486 - 499, The _initPosition() startup routine in pwrmccdaq.py is actively driving DO outputs by calling DBitOut for each axis, which can move the actuator during initialization. Update _initPosition() in the PwrMccDaq driver so it only reads or reports the current hardware state and updates self.position without writing any output bits, keeping any hardware actuation confined to explicit move requests.
599-600: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive DI feedback a non-zero timeout floor.
For a DI-backed axis with
duration = 0,timeout = time.time() + 2 * durationexpires immediately and may raise before feedback can settle.Proposed timeout floor
- timeout = time.time() + 2 * duration - sleept = max(0.01, min(duration / 2, 0.1)) + feedback_timeout = max(0.1, 2 * duration) + timeout = time.time() + feedback_timeout + sleept = max(0.01, min(feedback_timeout / 2, 0.1))🤖 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/pwrmccdaq.py` around lines 599 - 600, The DI feedback timeout calculation in pwrmccdaq.py can expire immediately when duration is zero, causing premature failure before feedback settles. Update the timeout logic in the DI-backed axis path to apply a small non-zero minimum floor when computing the timeout in the feedback wait flow, so the existing settle/wait behavior in the relevant method has time to complete even for zero-duration moves.
🧹 Nitpick comments (2)
src/odemis/driver/pwrmccdaq.py (2)
549-552: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd docstrings to the new helper methods.
stop,_checkMoveAbs, andterminateare new/changed methods without local docstrings. When adding them, keep them plain text. As per coding guidelines, "**/*.py: Include docstrings for all functions and classes". Based on learnings, "keep docstrings as plain text only. Do not use reStructuredText (RST) markup/directives such as:param:,:return:, or:type:".Also applies to: 643-647
🤖 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/pwrmccdaq.py` around lines 549 - 552, Add plain-text docstrings for the new/changed methods in pwrmccdaq, specifically stop, _checkMoveAbs, and terminate, so they comply with the project rule that all functions and classes must have docstrings. Keep the docstrings short and descriptive, and avoid any reStructuredText markup such as :param:, :return:, or :type:; update the method definitions themselves rather than nearby code so the documentation stays attached to the symbols.Sources: Coding guidelines, Learnings
65-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing type annotations on the changed callables. Several of the new/modified methods still omit parameter or return annotations, including
daemon,kwargs,pos,future,axes, andshift.🤖 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/pwrmccdaq.py` around lines 65 - 67, The updated callables still have incomplete typing, so add the missing annotations on the affected signatures in pwrmccdaq, especially for __init__, moveAbs, and any helpers using pos, future, axes, and shift. Explicitly annotate daemon and kwargs in the constructor, and make sure each changed method has fully specified parameter and return types so the public API remains consistently typed.Source: Coding guidelines
🤖 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/pwrmccdaq.py`:
- Around line 419-461: The axis validation in the pwrmccdaq setup currently only
guards against port-level conflicts, so exact channel reuse can still slip
through. Update the validation around the axes loop to reject any `do` or `di`
channel that matches an already used channel in `parent._do_channels`,
`parent._di_channels`, or a previously processed axis, using the existing
`channel_to_port`, `_axes_cfg`, and `_di_feedback` flow to locate the checks.
Ensure each axis has a unique exact channel assignment for both output and
feedback, not just a unique port.
---
Duplicate comments:
In `@src/odemis/driver/pwrmccdaq.py`:
- Around line 589-590: The cancellation path in pwrmccdaq’s position-setting
flow leaves stale state because _checkCancelled() can raise before the latest
position is refreshed. Update the logic around the wait/check sequence so the
current position is read or assigned immediately after the DO write and before
calling _checkCancelled(future), using the existing future and position update
path in the same method.
- Around line 486-499: The _initPosition() startup routine in pwrmccdaq.py is
actively driving DO outputs by calling DBitOut for each axis, which can move the
actuator during initialization. Update _initPosition() in the PwrMccDaq driver
so it only reads or reports the current hardware state and updates self.position
without writing any output bits, keeping any hardware actuation confined to
explicit move requests.
- Around line 599-600: The DI feedback timeout calculation in pwrmccdaq.py can
expire immediately when duration is zero, causing premature failure before
feedback settles. Update the timeout logic in the DI-backed axis path to apply a
small non-zero minimum floor when computing the timeout in the feedback wait
flow, so the existing settle/wait behavior in the relevant method has time to
complete even for zero-duration moves.
---
Nitpick comments:
In `@src/odemis/driver/pwrmccdaq.py`:
- Around line 549-552: Add plain-text docstrings for the new/changed methods in
pwrmccdaq, specifically stop, _checkMoveAbs, and terminate, so they comply with
the project rule that all functions and classes must have docstrings. Keep the
docstrings short and descriptive, and avoid any reStructuredText markup such as
:param:, :return:, or :type:; update the method definitions themselves rather
than nearby code so the documentation stays attached to the symbols.
- Around line 65-67: The updated callables still have incomplete typing, so add
the missing annotations on the affected signatures in pwrmccdaq, especially for
__init__, moveAbs, and any helpers using pos, future, axes, and shift.
Explicitly annotate daemon and kwargs in the constructor, and make sure each
changed method has fully specified parameter and return types so the public API
remains consistently typed.
🪄 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: 7f91cc99-f2c2-4498-9034-31bb1d3a3961
📒 Files selected for processing (3)
src/odemis/driver/_mccdaq.pysrc/odemis/driver/pwrmccdaq.pysrc/odemis/driver/test/pwrmccdaq_test.py
💤 Files with no reviewable changes (1)
- src/odemis/driver/_mccdaq.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/driver/test/pwrmccdaq_test.py
Comment on lines
+419
to
+461
| axes_def = {} | ||
| do_ports = set() | ||
| di_ports = set() | ||
| do_ports_parent = set() | ||
| if hasattr(parent, "_do_channels"): | ||
| do_ports_parent = {parent.channel_to_port(ch)[0] for ch in parent._do_channels} | ||
| di_ports_parent = {parent.channel_to_port(ch)[0] for ch in parent._di_channels} | ||
| for axis_name, axis_cfg in axes.items(): | ||
| for required in ("do", "low", "high", "duration"): | ||
| if required not in axis_cfg: | ||
| raise ValueError(f"Axis '{axis_name}' is missing required key '{required}'") | ||
|
|
||
| do_ch = axis_cfg["do"] | ||
| low = axis_cfg["low"] | ||
| high = axis_cfg["high"] | ||
| unit = axis_cfg.get("unit", "") | ||
| duration = axis_cfg["duration"] | ||
|
|
||
| if low == high: | ||
| raise ValueError(f"Axis '{axis_name}' 'low' and 'high' positions must be different") | ||
| if not 0 <= duration < 1000: | ||
| raise ValueError(f"Axis '{axis_name}' duration {duration} should be between 0 and 1000 s") | ||
| try: | ||
| do_port, _ = parent.channel_to_port(do_ch) | ||
| do_ports.add(do_port) | ||
| except ValueError: | ||
| raise ValueError(f"Axis '{axis_name}' has invalid 'do' channel {do_ch}") | ||
|
|
||
| self._axes_cfg[axis_name] = (do_ch, low, high, duration) | ||
| axes_def[axis_name] = model.Axis(unit=unit, choices={low, high}) | ||
|
|
||
| if "di" in axis_cfg: | ||
| di_ch = axis_cfg["di"] | ||
| if di_ch in parent._di_channels: | ||
| raise ValueError(f"Axis '{axis_name}' 'di' channel {di_ch} is already used for a parent DI status") | ||
| di_inverted = axis_cfg.get("di_inverted", False) | ||
| try: | ||
| di_port, _ = parent.channel_to_port(di_ch) | ||
| di_ports.add(di_port) | ||
| except ValueError: | ||
| raise ValueError(f"Axis '{axis_name}' has invalid 'di' channel {di_ch}") | ||
| parent.dconfig[di_port] = 0xff # set all channel to input | ||
| self._di_feedback[axis_name] = (di_ch, di_inverted) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject exact channel reuse, not only port conflicts.
The current checks allow an actuator axis to reuse a parent._do_channels bit, or two axes to share the same do/di bit. That makes independent controls race on one hardware channel or report ambiguous feedback.
Proposed validation
axes_def = {}
do_ports = set()
di_ports = set()
+ used_do_channels = {}
+ used_di_channels = {}
+ parent_do_channels = set(getattr(parent, "_do_channels", ()))
do_ports_parent = set()
if hasattr(parent, "_do_channels"):
do_ports_parent = {parent.channel_to_port(ch)[0] for ch in parent._do_channels}
@@
do_ch = axis_cfg["do"]
@@
+ if do_ch in parent_do_channels:
+ raise ValueError(
+ f"Axis '{axis_name}' 'do' channel {do_ch} is already used by the parent")
+ if do_ch in used_do_channels:
+ raise ValueError(
+ f"Axis '{axis_name}' 'do' channel {do_ch} is already used by axis "
+ f"'{used_do_channels[do_ch]}'")
+ used_do_channels[do_ch] = axis_name
+
if low == high:
raise ValueError(f"Axis '{axis_name}' 'low' and 'high' positions must be different")
@@
if "di" in axis_cfg:
di_ch = axis_cfg["di"]
if di_ch in parent._di_channels:
raise ValueError(f"Axis '{axis_name}' 'di' channel {di_ch} is already used for a parent DI status")
+ if di_ch in used_di_channels:
+ raise ValueError(
+ f"Axis '{axis_name}' 'di' channel {di_ch} is already used by axis "
+ f"'{used_di_channels[di_ch]}'")
+ used_di_channels[di_ch] = axis_name
di_inverted = axis_cfg.get("di_inverted", False)📝 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.
Suggested change
| axes_def = {} | |
| do_ports = set() | |
| di_ports = set() | |
| do_ports_parent = set() | |
| if hasattr(parent, "_do_channels"): | |
| do_ports_parent = {parent.channel_to_port(ch)[0] for ch in parent._do_channels} | |
| di_ports_parent = {parent.channel_to_port(ch)[0] for ch in parent._di_channels} | |
| for axis_name, axis_cfg in axes.items(): | |
| for required in ("do", "low", "high", "duration"): | |
| if required not in axis_cfg: | |
| raise ValueError(f"Axis '{axis_name}' is missing required key '{required}'") | |
| do_ch = axis_cfg["do"] | |
| low = axis_cfg["low"] | |
| high = axis_cfg["high"] | |
| unit = axis_cfg.get("unit", "") | |
| duration = axis_cfg["duration"] | |
| if low == high: | |
| raise ValueError(f"Axis '{axis_name}' 'low' and 'high' positions must be different") | |
| if not 0 <= duration < 1000: | |
| raise ValueError(f"Axis '{axis_name}' duration {duration} should be between 0 and 1000 s") | |
| try: | |
| do_port, _ = parent.channel_to_port(do_ch) | |
| do_ports.add(do_port) | |
| except ValueError: | |
| raise ValueError(f"Axis '{axis_name}' has invalid 'do' channel {do_ch}") | |
| self._axes_cfg[axis_name] = (do_ch, low, high, duration) | |
| axes_def[axis_name] = model.Axis(unit=unit, choices={low, high}) | |
| if "di" in axis_cfg: | |
| di_ch = axis_cfg["di"] | |
| if di_ch in parent._di_channels: | |
| raise ValueError(f"Axis '{axis_name}' 'di' channel {di_ch} is already used for a parent DI status") | |
| di_inverted = axis_cfg.get("di_inverted", False) | |
| try: | |
| di_port, _ = parent.channel_to_port(di_ch) | |
| di_ports.add(di_port) | |
| except ValueError: | |
| raise ValueError(f"Axis '{axis_name}' has invalid 'di' channel {di_ch}") | |
| parent.dconfig[di_port] = 0xff # set all channel to input | |
| self._di_feedback[axis_name] = (di_ch, di_inverted) | |
| axes_def = {} | |
| do_ports = set() | |
| di_ports = set() | |
| used_do_channels = {} | |
| used_di_channels = {} | |
| parent_do_channels = set(getattr(parent, "_do_channels", ())) | |
| do_ports_parent = set() | |
| if hasattr(parent, "_do_channels"): | |
| do_ports_parent = {parent.channel_to_port(ch)[0] for ch in parent._do_channels} | |
| di_ports_parent = {parent.channel_to_port(ch)[0] for ch in parent._di_channels} | |
| for axis_name, axis_cfg in axes.items(): | |
| for required in ("do", "low", "high", "duration"): | |
| if required not in axis_cfg: | |
| raise ValueError(f"Axis '{axis_name}' is missing required key '{required}'") | |
| do_ch = axis_cfg["do"] | |
| low = axis_cfg["low"] | |
| high = axis_cfg["high"] | |
| unit = axis_cfg.get("unit", "") | |
| duration = axis_cfg["duration"] | |
| if do_ch in parent_do_channels: | |
| raise ValueError( | |
| f"Axis '{axis_name}' 'do' channel {do_ch} is already used by the parent") | |
| if do_ch in used_do_channels: | |
| raise ValueError( | |
| f"Axis '{axis_name}' 'do' channel {do_ch} is already used by axis " | |
| f"'{used_do_channels[do_ch]}'") | |
| used_do_channels[do_ch] = axis_name | |
| if low == high: | |
| raise ValueError(f"Axis '{axis_name}' 'low' and 'high' positions must be different") | |
| if not 0 <= duration < 1000: | |
| raise ValueError(f"Axis '{axis_name}' duration {duration} should be between 0 and 1000 s") | |
| try: | |
| do_port, _ = parent.channel_to_port(do_ch) | |
| do_ports.add(do_port) | |
| except ValueError: | |
| raise ValueError(f"Axis '{axis_name}' has invalid 'do' channel {do_ch}") | |
| self._axes_cfg[axis_name] = (do_ch, low, high, duration) | |
| axes_def[axis_name] = model.Axis(unit=unit, choices={low, high}) | |
| if "di" in axis_cfg: | |
| di_ch = axis_cfg["di"] | |
| if di_ch in parent._di_channels: | |
| raise ValueError(f"Axis '{axis_name}' 'di' channel {di_ch} is already used for a parent DI status") | |
| if di_ch in used_di_channels: | |
| raise ValueError( | |
| f"Axis '{axis_name}' 'di' channel {di_ch} is already used by axis " | |
| f"'{used_di_channels[di_ch]}'") | |
| used_di_channels[di_ch] = axis_name | |
| di_inverted = axis_cfg.get("di_inverted", False) | |
| try: | |
| di_port, _ = parent.channel_to_port(di_ch) | |
| di_ports.add(di_port) | |
| except ValueError: | |
| raise ValueError(f"Axis '{axis_name}' has invalid 'di' channel {di_ch}") | |
| parent.dconfig[di_port] = 0xff # set all channel to input | |
| self._di_feedback[axis_name] = (di_ch, di_inverted) |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 445-445: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 459-459: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 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/pwrmccdaq.py` around lines 419 - 461, The axis validation
in the pwrmccdaq setup currently only guards against port-level conflicts, so
exact channel reuse can still slip through. Update the validation around the
axes loop to reject any `do` or `di` channel that matches an already used
channel in `parent._do_channels`, `parent._di_channels`, or a previously
processed axis, using the existing `channel_to_port`, `_axes_cfg`, and
`_di_feedback` flow to locate the checks. Ensure each axis has a unique exact
channel assignment for both output and feedback, not just a unique port.
pieleric
requested changes
Jun 30, 2026
nandishjpatel
force-pushed
the
GANIL-154-create-a-odemis-backend-component-to-control-the-nd-filter
branch
from
3d1fd45 to
7a3bce5
Compare
tmoerkerken
reviewed
Jul 2, 2026
|
|
||
| def _waitEndMove(self, future, duration, pos): | ||
| """ | ||
| Wait until the max transition duration has elapsed or cancellation is requested. |
Contributor
There was a problem hiding this comment.
lacking docstrings for the parameters
tmoerkerken
approved these changes
Jul 6, 2026
|
|
||
| # Default to every channel is output (both ports set to 0) | ||
| dconfig = { | ||
| self.dconfig = { |
Contributor
There was a problem hiding this comment.
Adding it to self seems unnecessary since it is only accessed inside __init__
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.
[feat] created MCCDeviceDIOActuator which is an actuator with binary (ON/OFF) axes controlled via MCC DAQ digital outputs, with position feedback via separate digital inputs