[GANIL-159] create nenovision litescope odemis backend driver - #3534
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an Odemis backend driver for the NenoVision LiteScope (NI-DAQmx analog-output controlled XY closed-loop piezo scanners) and a corresponding test suite validating waveform generation and core actuator behavior.
Changes:
- Introduces
LiteScopeactuator driver with NI-DAQmx canary check, position/velocity VAs, and hardware-timed smoothstep ramping with cancellation support. - Adds unit tests for
smooth_stepmath and integration tests for move/stop/cancel/speed validation against NI hardware (or simulator).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/odemis/driver/nenovision.py |
New LiteScope actuator driver using NI-DAQmx finite AO waveforms with cancellable moves. |
src/odemis/driver/test/nenovision_test.py |
New math + integration tests for LiteScope waveform generation and actuator semantics. |
Comments suppressed due to low confidence (3)
src/odemis/driver/nenovision.py:476
- LiteScope.terminate() shuts down the executor but never calls Component.terminate(), so the component may remain registered (Pyro/VAs/dataflows) and leak resources. Call super().terminate() after local cleanup.
def terminate(self) -> None:
"""
Cleans up the component, halts physical motion, and safely shuts down
the background executor thread pool.
"""
if self._executor:
src/odemis/driver/test/nenovision_test.py:298
- test_stop_interrupts_move() currently passes even if stop() does nothing, because it only asserts the final position is <= the target (which is always true). Make the test assert that the move future was cancelled (or skip if the move finishes before stop() can be applied).
# Use a slow speed so the move takes several seconds
self.scan_stage.speed.value = {"x": LiteScope.MIN_SPEED_M_S * 100, "y": LiteScope.MIN_SPEED_M_S * 100}
target = {"x": 80e-6, "y": 80e-6}
f = self.scan_stage.moveAbs(target)
time.sleep(0.05)
self.scan_stage.stop()
src/odemis/driver/nenovision.py:448
- When a move is cancelled, _do_move_abs() returns normally. This triggers CancellableFuture to log "Task was cancelled but returned result instead of raising an exception." (see model/_futures.py:334). Raise CancelledError after updating internal state when cancellation was requested to avoid spurious warnings.
self._voltage.update(final_v)
self.position._set_value(
{ax: self._v_to_m(ax, self._voltage[ax]) for ax in self.axes},
force_write=True
)
📝 WalkthroughWalkthroughAdds a new NI-DAQmx Sequence Diagram(s)sequenceDiagram
participant Client
participant LiteScope
participant NIDAQmxTask
Client->>LiteScope: request absolute or relative move
LiteScope->>LiteScope: calculate target and smoothstep waveform
LiteScope->>NIDAQmxTask: configure and stream finite analog output
NIDAQmxTask-->>LiteScope: report generated samples
LiteScope-->>Client: complete future and updated position
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
🧹 Nitpick comments (6)
src/odemis/driver/nenovision.py (3)
90-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the original
DaqError.
raise ... from exkeeps the underlying NI error visible in the traceback (Ruff B904).♻️ Proposed fix
- except nidaqmx.DaqError: - raise ValueError(f"Failed to find NI DAQ device '{self._device}'. Please check the connection.") + except nidaqmx.DaqError as ex: + raise ValueError(f"Failed to find NI DAQ device '{self._device}'. " + "Please check the connection.") from ex🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/nenovision.py` around lines 90 - 94, Update the exception handler around the device lookup in the constructor to bind the caught nidaqmx.DaqError and raise the existing ValueError using explicit exception chaining with the original error. Preserve the current message and device lookup behavior.Source: Linters/SAST tools
450-479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winActuator lifecycle polish:
referencedVA andsuper().terminate().Exposing
reference()without areferencedVA is inconsistent with the Odemis Actuator contract — either drop the method or add the VA (set toTruefor both axes).terminate()should also callsuper().terminate()so base-class cleanup runs.♻️ Proposed fix
def terminate(self) -> None: if self._executor: self.stop() self._executor.shutdown(wait=True) self._executor = None + super().terminate()🤖 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/nenovision.py` around lines 450 - 479, Update the actuator class around reference and termination: add a referenced VA initialized to true for both axes so the existing reference() method satisfies the Actuator contract, and update terminate() to invoke super().terminate() after shutting down the executor while preserving the current cleanup behavior.
162-167: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the canary subprocess with a timeout.
The whole point is to survive a broken NI-DAQmx install, but a hanging C library would block init indefinitely since
subprocess.runhas no timeout. (The S603/command-injection hints are false positives here: the argv is fixed and usessys.executable.)♻️ Proposed fix
- proc = subprocess.run(canary_cmd) + try: + proc = subprocess.run(canary_cmd, timeout=30) + except subprocess.TimeoutExpired: + raise model.HwError("NI-DAQmx failed to load: the canary check timed out.")🤖 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/nenovision.py` around lines 162 - 167, Update the canary subprocess invocation in the NI-DAQmx initialization flow to pass a finite timeout to subprocess.run, ensuring a hung C library cannot block initialization indefinitely. Preserve the existing return-code handling for completed processes and handle the timeout consistently with the existing hardware-error behavior.src/odemis/driver/test/nenovision_test.py (3)
135-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the original exception and annotate the fixtures.
raise unittest.SkipTest(...) from ex(Ruff B904). Also add return type hints to the test methods/fixtures in this class —setUpClassinTestLiteScopeMathis annotated while these are not.As per coding guidelines: "Always use type hints for function parameters and return types in Python code".
♻️ Proposed fix
`@classmethod` - def setUpClass(cls): + def setUpClass(cls) -> None: if not nenovision: raise unittest.SkipTest("nenovision driver is not available. Check if python3-nidaqmx is installed.") try: cls.scan_stage = LiteScope(**CONFIG_LITESCOPE) except (model.HwError, ValueError) as ex: - raise unittest.SkipTest(f"Cannot connect to NI DAQ device: {ex}") + raise unittest.SkipTest(f"Cannot connect to NI DAQ device: {ex}") from ex🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/test/nenovision_test.py` around lines 135 - 142, Update TestLiteScopeMath.setUpClass to chain the unittest.SkipTest exception from the caught exception using “from ex”. Add return type annotations to setUpClass and the other test methods/fixtures in this class, using appropriate types while preserving existing test behavior.Sources: Coding guidelines, Linters/SAST tools
33-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGate hardware tests on
TEST_NOHWas well.The repo convention runs tests with
env TEST_NOHW=1, but this module decides purely on driver importability, soTestLiteScopewill attempt to openDev1in a no-hardware run and rely on theSkipTestfallback. ReadingTEST_NOHWmakes the intent explicit and avoids the slow canary subprocess. Also,as exin the import guard is unused.As per coding guidelines: "Run tests with the template command:
env TEST_NOHW=1 python3 src/odemis/.../name_of_the_test_file.py TestCaseClassName.test_method_name".♻️ Proposed change
+import os + try: from odemis.driver import nenovision from odemis.driver.nenovision import LiteScope -except ImportError as ex: +except ImportError: nenovision = None + +TEST_NOHW = os.environ.get("TEST_NOHW", "0") != "0"🤖 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/nenovision_test.py` around lines 33 - 40, Update the module-level hardware-test gating around the nenovision import to also check the TEST_NOHW environment variable, preventing TestLiteScope from running when no hardware tests are requested; remove the unused exception binding in the ImportError handler and preserve the existing importability fallback.Source: Coding guidelines
345-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfig-validation tests are needlessly hardware-gated.
test_init_raises_on_empty_axesandtest_init_raises_on_missing_axis_keyfail before any DAQ access, so they belong inTestLiteScopeMathwhere they still run without a device. Keeping them here means they are skipped exactly in the no-hardware CI runs where they are cheapest to exercise.Note that
test_init_raises_on_missing_axis_keyonly reaches the key check after the device lookup succeeds, so it does depend on hardware as written; asserting a bad-key config against a valid device is fine, but the empty-axes case can move.🤖 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/nenovision_test.py` around lines 345 - 372, Move test_init_raises_on_empty_axes from the hardware-dependent test class into TestLiteScopeMath so it runs without DAQ access. Keep test_init_raises_on_missing_axis_key in its current class because LiteScope performs device lookup before validating the axis key; retain the valid-device setup needed for that test.
🤖 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/nenovision.py`:
- Around line 60-63: Ensure MAX_SPEED_ENGAGED_M_S is enforced whenever the probe
is engaged, using a probe-state-dependent VA speed range alongside the existing
MAX_SPEED_RETRACTED_M_S enforcement. If engaged-state enforcement is not
intended, remove MAX_SPEED_ENGAGED_M_S and update the class documentation to
match; do not leave the constant unused.
- Around line 96-101: Update the component initialization around the metadata
assignment so self._metadata is populated only after HwComponent.__init__()
completes, preserving MD_HW_NAME, MD_SW_VERSION, and MD_HW_VERSION.
Alternatively, use HwComponent’s supported swVersion and hwVersion attributes,
but ensure the metadata values are not overwritten during super().__init__().
- Around line 265-272: Update _write_ao_finite to clear future._running_task
while holding future._moving_lock before the nidaqmx.Task context exits,
ensuring _cancel_current_move cannot call stop() on a closed task. Preserve
cancellation signaling and existing task cleanup behavior.
- Around line 397-408: Update the timeout branch in the task-wait loop of the
move execution method to avoid returning n_samples when task.is_task_done() is
still false. Explicitly fail the operation or return
task.out_stream.total_samp_per_chan_generated so _do_move_abs cannot report the
target position and resolve the future successfully after a hardware stall;
preserve the existing cancellation behavior.
- Around line 116-132: Validate each axis’s rng_m and rng_v in the axis
initialization loop before storing them or computing the center voltage. Reject
equal endpoints to prevent zero-denominator conversions, and ensure rng_v lies
within the device AO limits before accepting the configuration; raise a clear
ValueError identifying the axis and invalid range.
- Around line 311-318: Update the relative-move flow in `moveRel` and
`_do_move_rel` so target coordinates are calculated inside the queued worker
from the live `_voltage`, converted via `_v_to_m`, and combined with the
inversion-adjusted shift. Remove the submit-time `self.position.value` target
calculation and delegate the resolved target to `_do_move_abs`, preserving the
existing future and executor behavior.
---
Nitpick comments:
In `@src/odemis/driver/nenovision.py`:
- Around line 90-94: Update the exception handler around the device lookup in
the constructor to bind the caught nidaqmx.DaqError and raise the existing
ValueError using explicit exception chaining with the original error. Preserve
the current message and device lookup behavior.
- Around line 450-479: Update the actuator class around reference and
termination: add a referenced VA initialized to true for both axes so the
existing reference() method satisfies the Actuator contract, and update
terminate() to invoke super().terminate() after shutting down the executor while
preserving the current cleanup behavior.
- Around line 162-167: Update the canary subprocess invocation in the NI-DAQmx
initialization flow to pass a finite timeout to subprocess.run, ensuring a hung
C library cannot block initialization indefinitely. Preserve the existing
return-code handling for completed processes and handle the timeout consistently
with the existing hardware-error behavior.
In `@src/odemis/driver/test/nenovision_test.py`:
- Around line 135-142: Update TestLiteScopeMath.setUpClass to chain the
unittest.SkipTest exception from the caught exception using “from ex”. Add
return type annotations to setUpClass and the other test methods/fixtures in
this class, using appropriate types while preserving existing test behavior.
- Around line 33-40: Update the module-level hardware-test gating around the
nenovision import to also check the TEST_NOHW environment variable, preventing
TestLiteScope from running when no hardware tests are requested; remove the
unused exception binding in the ImportError handler and preserve the existing
importability fallback.
- Around line 345-372: Move test_init_raises_on_empty_axes from the
hardware-dependent test class into TestLiteScopeMath so it runs without DAQ
access. Keep test_init_raises_on_missing_axis_key in its current class because
LiteScope performs device lookup before validating the axis key; retain the
valid-device setup needed for that test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe38920b-9a6f-4fec-98ec-96bc74ecadc4
📒 Files selected for processing (2)
src/odemis/driver/nenovision.pysrc/odemis/driver/test/nenovision_test.py
| # Physical speed limits in m/s (Assuming +/-10V maps to +/-80um) | ||
| MAX_SPEED_ENGAGED_M_S = 20e-6 # 2.5 V/s | ||
| MAX_SPEED_RETRACTED_M_S = 320e-6 # 40.0 V/s | ||
| MIN_SPEED_M_S = 1e-7 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
MAX_SPEED_ENGAGED_M_S is never enforced.
The class docstring claims strict speed-limit enforcement, but only MAX_SPEED_RETRACTED_M_S bounds the speed VA, so an engaged probe can be driven at 16x the intended limit. Confirm whether the engaged limit is meant to be applied (e.g. via a probe-state-dependent VA range) or the constant should be dropped.
🤖 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/nenovision.py` around lines 60 - 63, Ensure
MAX_SPEED_ENGAGED_M_S is enforced whenever the probe is engaged, using a
probe-state-dependent VA speed range alongside the existing
MAX_SPEED_RETRACTED_M_S enforcement. If engaged-state enforcement is not
intended, remove MAX_SPEED_ENGAGED_M_S and update the class documentation to
match; do not leave the constant unused.
| while not task.is_task_done(): | ||
| left = end_time - time.monotonic() | ||
| if left <= 0: | ||
| break | ||
|
|
||
| sleept = max(0.001, min(left / 2.0, 0.1)) | ||
| if future._must_stop.wait(sleept): | ||
| logging.debug(f"[{self.name}] Move cancelled mid-execution.") | ||
| task.stop() | ||
| return task.out_stream.total_samp_per_chan_generated | ||
|
|
||
| return n_samples |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Timeout is reported as a successful full move.
When the wait deadline expires, the loop breaks with the task still not done, yet the function returns n_samples. _do_move_abs then sets the position VA to the target and the future resolves successfully, hiding a hardware stall. Fail explicitly (or return the actual generated count) instead.
🐛 Proposed fix
while not task.is_task_done():
left = end_time - time.monotonic()
if left <= 0:
- break
+ task.stop()
+ raise TimeoutError(f"AO generation did not complete within {timeout:.3f}s "
+ f"({task.out_stream.total_samp_per_chan_generated}/{n_samples} samples)")📝 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.
| while not task.is_task_done(): | |
| left = end_time - time.monotonic() | |
| if left <= 0: | |
| break | |
| sleept = max(0.001, min(left / 2.0, 0.1)) | |
| if future._must_stop.wait(sleept): | |
| logging.debug(f"[{self.name}] Move cancelled mid-execution.") | |
| task.stop() | |
| return task.out_stream.total_samp_per_chan_generated | |
| return n_samples | |
| while not task.is_task_done(): | |
| left = end_time - time.monotonic() | |
| if left <= 0: | |
| task.stop() | |
| raise TimeoutError( | |
| f"AO generation did not complete within {timeout:.3f}s " | |
| f"({task.out_stream.total_samp_per_chan_generated}/{n_samples} samples)" | |
| ) | |
| sleept = max(0.001, min(left / 2.0, 0.1)) | |
| if future._must_stop.wait(sleept): | |
| logging.debug(f"[{self.name}] Move cancelled mid-execution.") | |
| task.stop() | |
| return task.out_stream.total_samp_per_chan_generated | |
| return n_samples |
🤖 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/nenovision.py` around lines 397 - 408, Update the timeout
branch in the task-wait loop of the move execution method to avoid returning
n_samples when task.is_task_done() is still false. Explicitly fail the operation
or return task.out_stream.total_samp_per_chan_generated so _do_move_abs cannot
report the target position and resolve the future successfully after a hardware
stall; preserve the existing cancellation behavior.
589f4fa to
31912d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/odemis/driver/test/nenovision_test.py (4)
345-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the hardware-independent init tests to
TestLiteScopeMath.test_init_raises_on_missing_axis_keyandtest_init_raises_on_empty_axesraise before any DAQ access. InTestLiteScopethey are skipped whenever no device is present, so the validation logic gets no coverage in the standardTEST_NOHW=1run.🤖 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/nenovision_test.py` around lines 345 - 362, Move test_init_raises_on_missing_axis_key and test_init_raises_on_empty_axes from TestLiteScope to TestLiteScopeMath, preserving their assertions and inputs so they run without hardware and are not skipped when TEST_NOHW=1.Source: Coding guidelines
33-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBind
LiteScopetoNonein the import guard. If the import fails,LiteScopestays undefined. Any later reference at module level or in a test that runs before the skip raisesNameErrorinstead of a clear skip. Also log the captured exception so the cause is visible.♻️ Proposed change
try: from odemis.driver import nenovision from odemis.driver.nenovision import LiteScope except ImportError as ex: + logging.info(f"nenovision driver not available: {ex}") nenovision = None + LiteScope = 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/nenovision_test.py` around lines 33 - 37, Update the import guard around nenovision and LiteScope so the ImportError path assigns LiteScope to None alongside nenovision, and log the captured exception using the test module’s existing logging mechanism. Preserve the successful import behavior and existing skip handling.
287-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that
stop()really interrupted the move. The current assertions pass even when the stage reaches the target, because the target equals the upper bound. Compare against the target withassertLess, or assert on the future state, so the test can fail.🤖 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/nenovision_test.py` around lines 287 - 308, Update test_stop_interrupts_move so it verifies that stop() interrupted the motion rather than allowing the target position: use strict position comparisons against the target, or assert that the move future is cancelled, while preserving the existing timeout and cancellation handling.
132-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHonor
TEST_NOHWin the hardware-dependent test class. The repository test command isenv TEST_NOHW=1 python3 src/odemis/.../nenovision_test.py .... WithTEST_NOHW=1this class still tries to open the NI device and relies on the connection failure to skip. Skip explicitly instead.♻️ Proposed change
+import os + +TEST_NOHW = os.environ.get("TEST_NOHW", "0") # Default: run with real hardware + ... `@classmethod` def setUpClass(cls): if not nenovision: raise unittest.SkipTest("nenovision driver is not available. Check if python3-nidaqmx is installed.") + if TEST_NOHW == "1": + raise unittest.SkipTest("No hardware available, skipping the LiteScope integration tests.") try: cls.scan_stage = LiteScope(**CONFIG_LITESCOPE) except (model.HwError, ValueError) as ex: raise unittest.SkipTest(f"Cannot connect to NI DAQ device: {ex}") from exAs per coding guidelines: "Run tests with the template command:
env TEST_NOHW=1 python3 src/odemis/.../name_of_the_test_file.py TestCaseClassName.test_method_name".🤖 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/nenovision_test.py` around lines 132 - 142, Update TestLiteScope.setUpClass to check the TEST_NOHW environment flag before constructing LiteScope, raising unittest.SkipTest immediately when hardware tests are disabled; retain the existing nenovision availability check and connection-error skip behavior for hardware-enabled runs.Sources: Coding guidelines, Linters/SAST tools
src/odemis/driver/nenovision.py (2)
90-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the original
DaqError. Ruff flags B904. The original NI error text helps to diagnose connection failures.♻️ Proposed change
- except nidaqmx.DaqError: - raise ValueError(f"Failed to find NI DAQ device '{self._device}'. Please check the connection.") + except nidaqmx.DaqError as ex: + raise ValueError(f"Failed to find NI DAQ device '{self._device}'. " + f"Please check the connection.") from ex🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/nenovision.py` around lines 90 - 94, Update the exception handling around device lookup in the driver initialization flow to re-raise the ValueError from the caught nidaqmx.DaqError using explicit exception chaining, preserving the existing user-facing message while retaining the original NI error as the cause.Source: Linters/SAST tools
195-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the ambiguous parameter
l. Ruff flags E741. Usen_samplesfor clarity, and update the internal call at line 354 and the tests insrc/odemis/driver/test/nenovision_test.py.♻️ Proposed rename
- def smooth_step(l: int, start: float, end: float, vstart: float = 0.0, vend: float = 1.0) -> numpy.ndarray: + def smooth_step(n_samples: int, start: float, end: float, vstart: float = 0.0, + vend: float = 1.0) -> numpy.ndarray: ... - :param l: Number of values (samples) to generate. + :param n_samples: Number of values (samples) to generate. ... - x = numpy.linspace(start, end, l, dtype=numpy.float64) + x = numpy.linspace(start, end, n_samples, dtype=numpy.float64)🤖 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/nenovision.py` around lines 195 - 196, Rename the smooth_step parameter l to n_samples in the smooth_step method, update its internal references, and adjust the call site around the noted driver logic plus all affected tests in nenovision_test.py to use the new keyword or positional contract consistently.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/nenovision.py`:
- Around line 162-167: Update the canary subprocess invocation in the NI-DAQmx
initialization flow to pass an appropriate timeout to subprocess.run. Catch the
resulting timeout exception and raise model.HwError with a clear message
indicating that the NI-DAQmx C-library hung during loading, while preserving the
existing handling for negative and nonzero return codes.
- Around line 450-460: Add a self.referenced value attribute in the nenovision
driver for the axes handled by reference(), using the same representation and
initialization conventions as other drivers. Update reference() as needed to
maintain that attribute while preserving its instantly resolved future behavior,
so reference-based callers can validate and discover the referenceable axes.
---
Nitpick comments:
In `@src/odemis/driver/nenovision.py`:
- Around line 90-94: Update the exception handling around device lookup in the
driver initialization flow to re-raise the ValueError from the caught
nidaqmx.DaqError using explicit exception chaining, preserving the existing
user-facing message while retaining the original NI error as the cause.
- Around line 195-196: Rename the smooth_step parameter l to n_samples in the
smooth_step method, update its internal references, and adjust the call site
around the noted driver logic plus all affected tests in nenovision_test.py to
use the new keyword or positional contract consistently.
In `@src/odemis/driver/test/nenovision_test.py`:
- Around line 345-362: Move test_init_raises_on_missing_axis_key and
test_init_raises_on_empty_axes from TestLiteScope to TestLiteScopeMath,
preserving their assertions and inputs so they run without hardware and are not
skipped when TEST_NOHW=1.
- Around line 33-37: Update the import guard around nenovision and LiteScope so
the ImportError path assigns LiteScope to None alongside nenovision, and log the
captured exception using the test module’s existing logging mechanism. Preserve
the successful import behavior and existing skip handling.
- Around line 287-308: Update test_stop_interrupts_move so it verifies that
stop() interrupted the motion rather than allowing the target position: use
strict position comparisons against the target, or assert that the move future
is cancelled, while preserving the existing timeout and cancellation handling.
- Around line 132-142: Update TestLiteScope.setUpClass to check the TEST_NOHW
environment flag before constructing LiteScope, raising unittest.SkipTest
immediately when hardware tests are disabled; retain the existing nenovision
availability check and connection-error skip behavior for hardware-enabled runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fb42fc7-8827-4886-a903-4d8038aa1630
📒 Files selected for processing (2)
src/odemis/driver/nenovision.pysrc/odemis/driver/test/nenovision_test.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Suppressed comments (6)
src/odemis/driver/nenovision.py:193
- _v_to_m() has the same inversion issue as _m_to_v(): it uses the advertised (possibly inverted) axis range, but it is used to compute internal coordinates from voltages. For inverted axes with non-symmetric ranges this returns the wrong physical coordinate and makes the position VA inconsistent.
rng_m = self.axes[axis].range
rng_v = self._range_v[axis]
return rng_m[0] + (val_v - rng_v[0]) / (rng_v[1] - rng_v[0]) * (rng_m[1] - rng_m[0])
src/odemis/driver/nenovision.py:263
- _set_voltage() updates the position VA from internal voltage state, but does not convert to the external coordinate system. This breaks inverted axes: after any move the reported position will be inconsistent with commanded external coordinates.
self._voltage.update(volts)
self.position._set_value(
{ax: self._v_to_m(ax, self._voltage[ax]) for ax in self.axes},
force_write=True
)
src/odemis/driver/nenovision.py:448
- _do_move_abs() publishes position updates in internal coordinates (derived from voltages) without applying _applyInversion(). This makes the public position VA inconsistent with the external coordinate system for inverted axes.
self._voltage.update(final_v)
self.position._set_value(
{ax: self._v_to_m(ax, self._voltage[ax]) for ax in self.axes},
force_write=True
)
src/odemis/driver/test/nenovision_test.py:140
- These integration tests always attempt to connect to a physical NI DAQ device. Other driver tests commonly honor TEST_NOHW to avoid touching real hardware in CI (e.g., src/odemis/driver/test/tucsen_test.py:36-45). Consider skipping this hardware suite when TEST_NOHW=1 so no-hardware runs don’t try to enumerate/init NI devices.
@classmethod
def setUpClass(cls):
if not nenovision:
raise unittest.SkipTest("nenovision driver is not available. Check if python3-nidaqmx is installed.")
try:
cls.scan_stage = LiteScope(**CONFIG_LITESCOPE)
src/odemis/driver/nenovision.py:318
- moveRel() computes an internal target position by adding an internally-inverted shift to self.position.value, but self.position is an external-facing VA. For inverted axes this will move in the wrong direction once position reporting is fixed to be external. Compute the internal current position via _applyInversion(self.position.value) first.
shift = self._applyInversion(shift)
target = {ax: self.position.value[ax] + shift.get(ax, 0.0) for ax in self.axes if ax in shift}
f = self._create_future()
return self._executor.submitf(f, self._do_move_abs, f, target)
src/odemis/driver/nenovision.py:350
- _generate_waveform() can allocate extremely large waveforms at low speeds (n_samples scales with duration * sample_rate). With MIN_SPEED_M_S=1e-7 and a full-range move, this can easily reach tens of millions of samples per axis (hundreds of MB), risking OOM or very long precompute times. Consider capping n_samples / downsampling the sample_rate for long moves, or raising a clear error when the request would exceed a safe limit.
# Smoothstep peak derivative is 1.5x the average. Multiply duration to ensure safety.
duration = max(durations) * 1.5
max_dv = max(abs(target_v[ax] - start_v[ax]) for ax in axes)
sample_rate = max(self._ao_min_rate, min(TARGET_SAMPLE_RATE, self._ao_max_rate))
n_samples = max(2, math.ceil(duration * sample_rate))
| # Initialize tracking VAs | ||
| init_positions = {ax: self._v_to_m(ax, self._voltage[ax]) for ax in axes} | ||
| self.position = model.VigilantAttribute(init_positions, unit="m", readonly=True) | ||
|
|
There was a problem hiding this comment.
What you could also do, is to forbid axis inversion, via the "inverted" parameter, and instead just make sure that it's possible to pass v_min > v_max (in which case this will behave the same as axis inversion). Or you could even make it "fancy", and interpret here the "inverted" parameter and swap v_min/v_max in such case.
31912d8 to
ff3db11
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/odemis/driver/test/nenovision_test.py (4)
390-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
figbinding. Ruff flags RUF059.♻️ Proposed change
- fig, (ax_voltage, ax_dvdt) = plt.subplots(2, 1, figsize=(10, 6), sharex=True) + _, (ax_voltage, ax_dvdt) = plt.subplots(2, 1, figsize=(10, 6), sharex=True)🤖 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/nenovision_test.py` at line 390, Update the plt.subplots assignment in the test to discard the unused figure return value while preserving the ax_voltage and ax_dvdt axes bindings.Source: Linters/SAST tools
80-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd return type hints to the test methods.
setUpClassat Line 76 declares-> None, but the test methods in this class and inTestLiteScopedo not. The math assertions themselves are correct.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/nenovision_test.py` around lines 80 - 125, Add an explicit -> None return annotation to each test method shown, including the smooth-step tests and the corresponding methods in TestLiteScope. Preserve the existing test parameters and assertion logic, matching the setUpClass annotation style.Source: Coding guidelines
135-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGate the integration tests on
TEST_NOHW.setUpClassskips only when device construction fails. If a simulated NI device exists on the machine, these tests still drive the DAQ underTEST_NOHW=1. Add an explicit check so the documented no-hardware run is honoured. Also chain the caught exception to silence Ruff B904.♻️ Proposed change
+import os + +TEST_NOHW = os.environ.get("TEST_NOHW", "0") # Default: run with hardware ... `@classmethod` def setUpClass(cls): if not nenovision: raise unittest.SkipTest("nenovision driver is not available. Check if python3-nidaqmx is installed.") + if TEST_NOHW == "1": + raise unittest.SkipTest("No hardware available.") try: cls.scan_stage = LiteScope(**CONFIG_LITESCOPE) except (model.HwError, ValueError) as ex: - raise unittest.SkipTest(f"Cannot connect to NI DAQ device: {ex}") + raise unittest.SkipTest(f"Cannot connect to NI DAQ device: {ex}") from exAs per path instructions: "Run tests with the template command:
env TEST_NOHW=1 python3 src/odemis/.../name_of_the_test_file.py TestCaseClassName.test_method_name".🤖 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/nenovision_test.py` around lines 135 - 142, Update setUpClass in the nenovision integration tests to immediately raise unittest.SkipTest when TEST_NOHW indicates a no-hardware run, before constructing LiteScope; preserve the existing availability and connection checks. When converting model.HwError or ValueError into SkipTest, explicitly chain the caught exception to satisfy Ruff B904.Sources: Path instructions, Linters/SAST tools
33-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the caught
ImportError. The handler bindsexbut discards it, so the reason for the skip is invisible. Log the message, or include it in theSkipTestreason.♻️ Proposed change
try: from odemis.driver import nenovision from odemis.driver.nenovision import LiteScope except ImportError as ex: + logging.info(f"Skipping nenovision tests: {ex}") nenovision = 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/nenovision_test.py` around lines 33 - 37, Update the ImportError handler around the nenovision and LiteScope imports to use the captured ex value: log the exception or include its message in the SkipTest reason, while preserving the existing nenovision = None behavior.src/odemis/driver/nenovision.py (2)
197-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the parameter
l. Ruff flags E741 for the ambiguous name.num_samplesreads better and the tests call the method positionally, so the rename is safe.♻️ Proposed change
- def smooth_step(l: int, start: float, end: float, vstart: float = 0.0, vend: float = 1.0) -> numpy.ndarray: + def smooth_step(num_samples: int, start: float, end: float, + vstart: float = 0.0, vend: float = 1.0) -> numpy.ndarray:Update the body and docstring accordingly.
🤖 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/nenovision.py` around lines 197 - 198, Rename the smooth_step parameter l to num_samples in the static method smooth_step, updating all references in its body and docstring while preserving positional-call behavior.Source: Linters/SAST tools
92-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the original
DaqError. Ruff flags B904 here. The underlying NI error text is lost, which makes device-lookup failures harder to diagnose.♻️ Proposed change
- except nidaqmx.DaqError: - raise ValueError(f"Failed to find NI DAQ device '{self._device}'. Please check the connection.") + except nidaqmx.DaqError as ex: + raise ValueError(f"Failed to find NI DAQ device '{self._device}'. Please check the connection.") from ex🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/nenovision.py` around lines 92 - 96, Update the exception handling around device lookup in the initialization flow to preserve the original nidaqmx.DaqError by chaining it when raising the ValueError. Keep the existing user-facing message and device lookup behavior unchanged.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/nenovision.py`:
- Around line 152-153: The initialization path currently calls
_set_voltage(self._voltage), bypassing the speed-limited ramp. Read the current
analog-output value and use _generate_waveform to ramp safely to the center
voltage before applying it, preserving the class’s S-curve speed-limit behavior.
- Around line 473-481: Update LiteScope.terminate to call super().terminate()
after stopping and shutting down the executor, preserving the existing cleanup
order and ensuring Component.terminate unregisters the component’s dataflows,
VAs, events, and daemon registration.
- Around line 344-360: Adjust waveform sizing in the move-generation flow around
smooth_step and n_samples so long-duration, slow moves cannot allocate
excessively large buffers. Cap n_samples to a safe maximum and derive the
effective sample_rate from the capped sample count and duration, while
preserving the minimum sample count and existing duration/max_dv outputs.
In `@src/odemis/driver/test/nenovision_test.py`:
- Around line 276-279: Update both interruption-test assertion sites in
src/odemis/driver/test/nenovision_test.py (lines 276-279 and 305-308): use
strict less-than checks so reaching the target fails the test, and assert that
the stage position has moved beyond its start position; alternatively skip the
second test when the device is simulated, as documented.
---
Nitpick comments:
In `@src/odemis/driver/nenovision.py`:
- Around line 197-198: Rename the smooth_step parameter l to num_samples in the
static method smooth_step, updating all references in its body and docstring
while preserving positional-call behavior.
- Around line 92-96: Update the exception handling around device lookup in the
initialization flow to preserve the original nidaqmx.DaqError by chaining it
when raising the ValueError. Keep the existing user-facing message and device
lookup behavior unchanged.
In `@src/odemis/driver/test/nenovision_test.py`:
- Line 390: Update the plt.subplots assignment in the test to discard the unused
figure return value while preserving the ax_voltage and ax_dvdt axes bindings.
- Around line 80-125: Add an explicit -> None return annotation to each test
method shown, including the smooth-step tests and the corresponding methods in
TestLiteScope. Preserve the existing test parameters and assertion logic,
matching the setUpClass annotation style.
- Around line 135-142: Update setUpClass in the nenovision integration tests to
immediately raise unittest.SkipTest when TEST_NOHW indicates a no-hardware run,
before constructing LiteScope; preserve the existing availability and connection
checks. When converting model.HwError or ValueError into SkipTest, explicitly
chain the caught exception to satisfy Ruff B904.
- Around line 33-37: Update the ImportError handler around the nenovision and
LiteScope imports to use the captured ex value: log the exception or include its
message in the SkipTest reason, while preserving the existing nenovision = None
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e6e60d01-c0f6-4a86-841e-9c5265c630bc
📒 Files selected for processing (2)
src/odemis/driver/nenovision.pysrc/odemis/driver/test/nenovision_test.py
| # Apply initial physical voltages to hardware | ||
| self._set_voltage(self._voltage) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The initial voltage write bypasses the speed limit. _set_voltage uses write_one_sample, so the DAC jumps immediately from its present output to the center of rng_v. If the DAC was previously at a range end, this is a full-scale step (up to 20 V) applied in one sample. That contradicts the class docstring, which states that the driver enforces speed limits with S-curve ramps to prevent overshoot and mechanical stress.
Consider reading back the current AO value and ramping to the center with _generate_waveform, or document that the probe must be retracted and the stage voltage already near center before initialization.
🤖 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/nenovision.py` around lines 152 - 153, The initialization
path currently calls _set_voltage(self._voltage), bypassing the speed-limited
ramp. Read the current analog-output value and use _generate_waveform to ramp
safely to the center voltage before applying it, preserving the class’s S-curve
speed-limit behavior.
| # Position must be consistent — somewhere between start and target | ||
| pos = self.scan_stage.position.value | ||
| self.assertLessEqual(pos["x"], 80e-6) | ||
| self.assertLessEqual(pos["y"], 80e-6) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Both interruption tests use non-strict inequalities against the target. assertLessEqual(pos[ax], 80e-6) is satisfied when the stage reaches the target, so neither test detects a failed cancel or stop.
src/odemis/driver/test/nenovision_test.py#L276-L279: replace the twoassertLessEqualchecks withassertLess(pos[ax], 80e-6), and assert that the position moved past the start.src/odemis/driver/test/nenovision_test.py#L305-L308: apply the same strict check, or skip the test when the device is simulated, as the docstring notes.
📍 Affects 1 file
src/odemis/driver/test/nenovision_test.py#L276-L279(this comment)src/odemis/driver/test/nenovision_test.py#L305-L308
🤖 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/nenovision_test.py` around lines 276 - 279, Update
both interruption-test assertion sites in
src/odemis/driver/test/nenovision_test.py (lines 276-279 and 305-308): use
strict less-than checks so reaching the target fails the test, and assert that
the stage position has moved beyond its start position; alternatively skip the
second test when the device is simulated, as documented.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/odemis/driver/nenovision.py:141
init_positionsis computed in internal coordinates butpositionshould be exposed in external coordinates (respectingActuatoraxis inversion). Without applying_applyInversionhere, inverted axes will report the wrong sign from startup.
init_positions = {ax: self._v_to_m(ax, self._voltage[ax]) for ax in axes}
src/odemis/driver/nenovision.py:403
- If the finite AO task fails to complete before
timeout, the loop breaks and the method currently returnsn_samples, which makes the caller treat the move as fully completed (and snap software state to the target). On timeout, stop the task and raise an error (or at least return the actual generated sample count).
while not task.is_task_done():
left = end_time - time.monotonic()
if left <= 0:
break
src/odemis/driver/nenovision.py:265
- The position VA update after
_set_voltage()uses internal coordinates; for inverted axes this will report the wrong sign unless_applyInversion()is applied when publishingposition.
self.position._set_value(
{ax: self._v_to_m(ax, self._voltage[ax]) for ax in self.axes},
force_write=True
)
src/odemis/driver/nenovision.py:450
- The position VA is updated with internal coordinates; inverted axes will be reported with the wrong sign unless
_applyInversion()is applied. Also, when aCancellableFutureis cancelled, letting the worker return normally triggersCancellableFuture.set_result()to log a warning; raisingCancelledErroravoids that noise while keeping the future cancelled.
self._voltage.update(final_v)
self.position._set_value(
{ax: self._v_to_m(ax, self._voltage[ax]) for ax in self.axes},
force_write=True
)
src/odemis/driver/nenovision.py:320
moveRel()applies_applyInversion()toshiftbefore adding it toself.position.value. Ifpositionis exposed in external coordinates (as in other actuators), this will double-apply inversion and compute the wrong target for inverted axes. Compute the target in external coordinates first, then apply_applyInversion()once when sending to the hardware thread.
self._checkMoveRel(shift)
shift = self._applyInversion(shift)
target = {ax: self.position.value[ax] + shift.get(ax, 0.0) for ax in self.axes if ax in shift}
f = self._create_future()
return self._executor.submitf(f, self._do_move_abs, f, target)
src/odemis/driver/nenovision.py:358
- Waveform generation starts at
x=1.0/n_samples, so the first hardware-timed sample is notstart_v. For smalln_samples(notably the enforced minimum of 2) this creates a large initial voltage step, undermining the intended speed/acceleration limiting.
# 1.0 / n_samples prevents 1-sample flat spots since DAC is already holding start_v
waves = [
self.smooth_step(n_samples, 1.0 / n_samples, 1.0, start_v[ax], target_v[ax])
for ax in axes
]
| try: | ||
| from odemis.driver import nenovision | ||
| from odemis.driver.nenovision import LiteScope | ||
| except ImportError as ex: | ||
| nenovision = None |
There was a problem hiding this comment.
This module tests nenovision. There is no reason it's not there. So no need to avoid it.
Or is that for the nidaq import?
If so, please clarify it as a comment.
| CONFIG_LITESCOPE = { | ||
| "name": "LiteScope", | ||
| "role": "scan-stage", | ||
| "device": "Dev1", | ||
| "settle_time": 0.0, | ||
| "axes": CONFIG_AXES, | ||
| } |
There was a problem hiding this comment.
Please add somewhere here or, at the top of the nenovision module how to create a simulated device. With nidaqmxconfig you can export/import a "nce" file that makes it easy to recreated the simulator (without the special NI Hardware config utility).
For the semnidaq, I had:
nidaqmxconfig --import ni-pci6361-sim.nce --replace . You can place the new .nce file next to ni-pci6361-sim.nce in the Google Drive.
| self.assertLessEqual(pos["x"], 80e-6) | ||
| self.assertLessEqual(pos["y"], 80e-6) |
There was a problem hiding this comment.
I agree a little bit with codderabbit here. It'd be good to also check it has moved away from the original position (0).
There was a problem hiding this comment.
Well, there was a bug and I needed to extend the functionality to solve this. Nice catch!
| # Initialize tracking VAs | ||
| init_positions = {ax: self._v_to_m(ax, self._voltage[ax]) for ax in axes} | ||
| self.position = model.VigilantAttribute(init_positions, unit="m", readonly=True) | ||
|
|
There was a problem hiding this comment.
What you could also do, is to forbid axis inversion, via the "inverted" parameter, and instead just make sure that it's possible to pass v_min > v_max (in which case this will behave the same as axis inversion). Or you could even make it "fancy", and interpret here the "inverted" parameter and swap v_min/v_max in such case.
ff3db11 to
e4aa1d6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/odemis/driver/nenovision.py (2)
106-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the original
DaqError.Ruff reports B904 here. Attach the cause so the DAQ error text survives in the traceback.
♻️ Proposed change
- except nidaqmx.DaqError: - raise ValueError(f"Failed to find NI DAQ device '{self._device}'. Please check the connection.") + except nidaqmx.DaqError as ex: + raise ValueError(f"Failed to find NI DAQ device '{self._device}'. Please check the connection.") from ex🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/nenovision.py` around lines 106 - 107, Update the nidaqmx.DaqError handler to chain the caught exception when raising ValueError, preserving the original DAQ error details in the traceback while keeping the existing message and behavior.Source: Linters/SAST tools
217-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
lparameter.Ruff reports E741 for the ambiguous name
l.n_samplesmatches the naming already used by the callers in_generate_waveform. Update the docstring and the test call sites if you rename it.🤖 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/nenovision.py` at line 217, Rename the ambiguous l parameter in smooth_step to n_samples, update its docstring and all references within the function, and adjust test call sites and any _generate_waveform usage to use the new keyword or positional-compatible name.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/nenovision.py`:
- Around line 286-295: The _cancel_current_move method must avoid waiting when
the move worker has not started or cancellation has already succeeded for a
pending future. Track whether _running_task is active before signaling stop,
wait on _position_updated only for an actually running task, and adjust the
cancellation/shutdown flow so terminate() does not wait for queued executor work
after motion is stopped.
- Around line 418-426: Update the wait loop around task.is_task_done() to exit
when the monotonic deadline, including the required margin, is reached instead
of continuing with 0.001-second sleeps. On deadline expiry, raise TimeoutError
while preserving the existing cancellation handling and normal completion
behavior.
- Around line 370-374: Update the capped-sample branch in the waveform
generation flow around `sample_rate` and `n_samples` so the recomputed rate is
clamped to `self._ao_min_rate`, then recompute `n_samples` from the clamped rate
and duration. Log when this adjustment causes the actual rate to exceed the
requested speed-limit-derived rate, while preserving the `MAX_SAMPLES` cap and
ensuring the final timing configuration uses a device-valid sample rate.
In `@src/odemis/driver/test/nenovision_test.py`:
- Around line 341-358: Move test_init_raises_on_missing_axis_key and
test_init_raises_on_empty_axes out of TestLiteScope into a class whose setup
only verifies that nenovision is importable and does not create a Dev1 device.
Preserve both ValueError assertions while ensuring they run on systems without
hardware.
- Line 380: Update both full-range position sequences in
src/odemis/driver/test/nenovision_test.py at lines 380-380 and 399-399 to use an
endpoint-inclusive range from -80 µm through +80 µm, such as 161-point linspace,
so both tests measure the final endpoint.
- Around line 77-81: Add return type annotations to every function definition in
the test file, including test methods and lifecycle callbacks, following the
project’s Python typing conventions. Add concise plain-text docstrings to
definitions that lack them, prioritizing both setUpClass methods, tearDownClass,
and on_pos; preserve the existing test behavior and signatures otherwise.
---
Nitpick comments:
In `@src/odemis/driver/nenovision.py`:
- Around line 106-107: Update the nidaqmx.DaqError handler to chain the caught
exception when raising ValueError, preserving the original DAQ error details in
the traceback while keeping the existing message and behavior.
- Line 217: Rename the ambiguous l parameter in smooth_step to n_samples, update
its docstring and all references within the function, and adjust test call sites
and any _generate_waveform usage to use the new keyword or positional-compatible
name.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 412c8550-10f2-415d-af96-4a3dd24ec780
📒 Files selected for processing (2)
src/odemis/driver/nenovision.pysrc/odemis/driver/test/nenovision_test.py
| def _cancel_current_move(self, future: CancellableFuture) -> bool: | ||
| """Called by CancellableFuture cancel() to interrupt an in-progress move.""" | ||
| logging.debug(f"[{self.name}] Cancelling current move...") | ||
| with future._moving_lock: | ||
| future._must_stop.set() | ||
| if future._running_task is not None: | ||
| future._running_task.stop() | ||
| # Wait briefly for the background thread to update position | ||
| future._position_updated.wait(5.0) | ||
| return True |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect odemis CancellableFuture.cancel() and CancellableThreadPoolExecutor.cancel()
# to determine when task_canceller runs for pending versus running futures.
set -euo pipefail
fd -t f '_futures.py' src/odemis/model --exec rg -n -C 20 'def cancel\(self\)|task_canceller|class Cancellable'Repository: delmic/odemis
Length of output: 13012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== nenovision relevant section =="
sed -n '250,310p' src/odemis/driver/nenovision.py
sed -n '440,505p' src/odemis/driver/nenovision.py
echo "== future submit/stop/cancel usages in nenovision =="
rg -n -C 3 'CancellableFuture|task_canceller|_cancel_current_move|_do_move_abs|\.stop\(|\.cancel\(' src/odemis/driver/nenovision.pyRepository: delmic/odemis
Length of output: 9210
Avoid unbounded waits when cancelling futures.
CancellableThreadPoolExecutor.cancel() calls Future.cancel() for pending futures, so _cancel_current_move() can return True before the worker runs or before _do_move_abs() sets _position_updated. If cancellation succeeds, terminate() can also shut down the executor after calling stop(), potentially leaving a 2-minute waiting shutdown for queued moves. Set up the cancellation wait so it only waits when the worker is actually running, and avoid waiting on executor shutdown after motion is stopped.
🤖 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/nenovision.py` around lines 286 - 295, The
_cancel_current_move method must avoid waiting when the move worker has not
started or cancellation has already succeeded for a pending future. Track
whether _running_task is active before signaling stop, wait on _position_updated
only for an actually running task, and adjust the cancellation/shutdown flow so
terminate() does not wait for queued executor work after motion is stopped.
| sample_rate = max(self._ao_min_rate, min(self.TARGET_SAMPLE_RATE, self._ao_max_rate)) | ||
| n_samples = max(2, math.ceil(duration * sample_rate)) | ||
| if n_samples > self.MAX_SAMPLES: | ||
| n_samples = self.MAX_SAMPLES | ||
| sample_rate = n_samples / duration |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The capped sample rate can fall below the device AO minimum.
sample_rate is recomputed as n_samples / duration without clamping to self._ao_min_rate. A slow long move produces a very low rate: at MIN_SPEED_M_S a full 160 µm move gives duration of about 2400 s, so sample_rate becomes about 27 Hz. If that is below self._ao_min_rate, cfg_samp_clk_timing raises DaqError and the move fails. If you clamp the rate silently, the move finishes faster than the requested speed limit allows.
Clamp the rate to the device minimum, recompute n_samples, and log the deviation so the speed-limit contract stays visible.
🛡️ Proposed fix
if n_samples > self.MAX_SAMPLES:
n_samples = self.MAX_SAMPLES
- sample_rate = n_samples / duration
+ sample_rate = n_samples / duration
+ if sample_rate < self._ao_min_rate:
+ # The device cannot clock this slowly: keep the minimum rate and accept more samples.
+ sample_rate = self._ao_min_rate
+ n_samples = max(2, math.ceil(duration * sample_rate))
+ logging.warning(f"[{self.name}] Move needs {n_samples} samples at the minimum AO rate "
+ f"{sample_rate} Hz to keep the requested {duration:.1f}s duration.")Based on learnings that generated analog-output waveforms are capped at 2**16 samples with the sample rate derived from the capped count and the required move duration, the cap itself is correct; only the rate clamp is missing.
🤖 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/nenovision.py` around lines 370 - 374, Update the
capped-sample branch in the waveform generation flow around `sample_rate` and
`n_samples` so the recomputed rate is clamped to `self._ao_min_rate`, then
recompute `n_samples` from the clamped rate and duration. Log when this
adjustment causes the actual rate to exceed the requested speed-limit-derived
rate, while preserving the `MAX_SAMPLES` cap and ensuring the final timing
configuration uses a device-valid sample rate.
Source: Learnings
| end_time = time.monotonic() + duration | ||
| task.start() | ||
|
|
||
| while not task.is_task_done(): | ||
| remaining = end_time - time.monotonic() | ||
| sleept = remaining / 2.0 if remaining > 0 else 0.001 | ||
| if future._must_stop.wait(sleept): | ||
| logging.debug(f"[{self.name}] Move cancelled mid-execution.") | ||
| break |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The wait loop can never exit on a hardware stall.
end_time is only used to size the sleep interval. After the deadline passes, remaining <= 0, so sleept becomes 0.001 and the loop repeats forever while task.is_task_done() stays False. A stalled AO task then blocks the single executor worker permanently, and stop() / terminate() can only recover through cancellation. The previous revision left the loop on timeout; that exit path is now gone.
Add a hard deadline with margin and raise TimeoutError, as requested in the earlier discussion.
🛡️ Proposed fix
- end_time = time.monotonic() + duration
+ end_time = time.monotonic() + duration + 1.0
task.start()
while not task.is_task_done():
remaining = end_time - time.monotonic()
- sleept = remaining / 2.0 if remaining > 0 else 0.001
+ if remaining <= 0:
+ raise TimeoutError(
+ f"AO generation did not complete within {duration + 1.0:.3f}s "
+ f"({task.out_stream.total_samp_per_chan_generated}/{n_samples} samples)")
+ sleept = min(max(remaining / 2.0, 0.001), 0.1)
if future._must_stop.wait(sleept):
logging.debug(f"[{self.name}] Move cancelled mid-execution.")
break🤖 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/nenovision.py` around lines 418 - 426, Update the wait loop
around task.is_task_done() to exit when the monotonic deadline, including the
required margin, is reached instead of continuing with 0.001-second sleeps. On
deadline expiry, raise TimeoutError while preserving the existing cancellation
handling and normal completion behavior.
| def setUpClass(cls) -> None: | ||
| if not nenovision: | ||
| raise unittest.SkipTest("nenovision driver is not available. Check if python3-nidaqmx is installed.") | ||
|
|
||
| def test_smooth_step_reaches_vend(self): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/odemis/driver/test/nenovision_test.py"
echo "== file exists and line count =="
if [ -f "$FILE" ]; then
wc -l "$FILE"
else
echo "missing $FILE"
exit 0
fi
echo
echo "== relevant line ranges =="
sed -n '1,170p' "$FILE" | cat -n
echo
sed -n '120,340p' "$FILE" | cat -n
echo
echo "== function definitions in file =="
python3 - <<'PY'
import ast
from pathlib import Path
p = Path("src/odemis/driver/test/nenovision_test.py")
tree = ast.parse(p.read_text())
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
line = tree.body.index(node) + 1 if node in tree.body else node.lineno
args = ast.dump(node.args, include_attributes=False)
ret = ast.dump(node.returns, include_attributes=False) if node.returns else None
doc = ast.get_docstring(node)
print(f"{node.name}@{line}: args={args}")
print(f" return={ret}, docstring={doc is not None}, call_count={sum(1 for n in ast.walk(tree) if isinstance(n, ast.Call) and isinstance(n.func, ast.Name) and n.func.id == node.name)}")
print(f" {ast.get_docstring(node)[:120] if doc else ''}")
PY
echo
echo "== quality config references =="
git ls-files | rg '(^|/)(pyproject\.toml|noxfile\.py|tox\.ini|setup\.cfg|setup\.py|poetry\.lock|requirements.*\.txt|mypy\.ini|\.ruff\.toml|ruff\.toml)' || true
for f in pyproject.toml noxfile.py tox.ini setup.cfg ruff.toml .ruff.toml; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f"; }
doneRepository: delmic/odemis
Length of output: 27488
Add function type hints and the missing docstrings.
All functions in this test file need value annotations per Python guidelines. Add plain-text docstrings to the docstring-less definitions, especially the two setUpClass methods, tearDownClass, and on_pos.
🤖 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/nenovision_test.py` around lines 77 - 81, Add return
type annotations to every function definition in the test file, including test
methods and lifecycle callbacks, following the project’s Python typing
conventions. Add concise plain-text docstrings to definitions that lack them,
prioritizing both setUpClass methods, tearDownClass, and on_pos; preserve the
existing test behavior and signatures otherwise.
Sources: Coding guidelines, Learnings
| def test_init_raises_on_missing_axis_key(self): | ||
| """Missing required axis key must raise ValueError before touching hardware.""" | ||
| bad_axes = { | ||
| "x": { | ||
| "channel": 0, | ||
| "speed": 50e-6, | ||
| "rng_m": [-80e-6, 80e-6], | ||
| # "rng_v" intentionally omitted | ||
| "unit": "m", | ||
| } | ||
| } | ||
| with self.assertRaises(ValueError): | ||
| LiteScope(name="bad", role="test", axes=bad_axes, device="Dev1") | ||
|
|
||
| def test_init_raises_on_empty_axes(self): | ||
| """Empty axes dict must raise ValueError.""" | ||
| with self.assertRaises(ValueError): | ||
| LiteScope(name="bad", role="test", axes={}, device="Dev1") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Run pre-hardware validation tests without Dev1.
TestLiteScope.setUpClass creates a device before these tests run. Systems without Dev1 skip the class, so these tests do not verify validation before hardware access.
Move these tests to a class that only checks that nenovision is importable.
🤖 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/nenovision_test.py` around lines 341 - 358, Move
test_init_raises_on_missing_axis_key and test_init_raises_on_empty_axes out of
TestLiteScope into a class whose setup only verifies that nenovision is
importable and does not create a Dev1 device. Preserve both ValueError
assertions while ensuring they run on systems without hardware.
| f.result(timeout=60) | ||
|
|
||
| step = 1e-6 | ||
| positions = numpy.arange(-80e-6, 80e-6, step) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include +80 µm in both full-range sequences.
numpy.arange(-80e-6, 80e-6, step) ends at +79 µm. The first test then asserts +80 µm and fails. The second test does not measure the final endpoint.
src/odemis/driver/test/nenovision_test.py#L380-L380: use an endpoint-inclusive sequence, such asnumpy.linspace(-80e-6, 80e-6, 161).src/odemis/driver/test/nenovision_test.py#L399-L399: use the same endpoint-inclusive sequence.
📍 Affects 1 file
src/odemis/driver/test/nenovision_test.py#L380-L380(this comment)src/odemis/driver/test/nenovision_test.py#L399-L399
🤖 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/nenovision_test.py` at line 380, Update both
full-range position sequences in src/odemis/driver/test/nenovision_test.py at
lines 380-380 and 399-399 to use an endpoint-inclusive range from -80 µm through
+80 µm, such as 161-point linspace, so both tests measure the final endpoint.
| overheads.append(overhead) | ||
|
|
||
| mean_overhead_ms = (sum(overheads) / len(overheads)) * 1000 | ||
| logging.info(f"Mean move overhead: {mean_overhead_ms:.3f} ms (target < 15 ms)") |
There was a problem hiding this comment.
@pieleric the mean overhead is around ~11-13 ms for this testcase, is it acceptable?
nenovision_test:418 Move 116/161: elapsed=0.0163s, theoretical=0.0047s, overhead=11.580ms
There was a problem hiding this comment.
11ms per move? For what it does, it sounds a little excessive, especially for a move that takes 5ms. It'd good to check with an app specialist what they think would be the smallest dwell time per pixel/position the user would realistically use. If it's above 100ms, it's really not an issue, otherwise it might be a little annoying.
In parallel, you could instrument the code a little bit to check where the overhead time comes from. Most likely, it's either the Future scheduling, the NI task creation, or finding out that the NI task is finished.
If it's the task creation, it might be easy to fix it by reusing the task and writer.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/odemis/driver/nenovision.py:42
- Avoid importing CancelledError from the private concurrent.futures._base module; use the public concurrent.futures API instead to reduce breakage across Python versions.
from concurrent.futures._base import CancelledError
src/odemis/driver/test/nenovision_test.py:389
- This test asserts a tight timing/performance threshold (mean overhead < 15ms), which is highly dependent on host load, NI-DAQ driver state, and hardware model, making it prone to flakiness. Consider marking it as a manual benchmark (skipped by default) like the debug-plot test below.
def test_move_start_stop_overhead_full_range(self):
src/odemis/driver/nenovision.py:375
- When the move duration is long enough to hit MAX_SAMPLES, sample_rate is recomputed as MAX_SAMPLES/duration without re-checking the device minimum AO rate. This can produce a sample_rate below self._ao_min_rate and then fail at cfg_samp_clk_timing with a DAQ error; it’s better to validate and raise a clear ValueError (or otherwise handle it) at waveform-generation time.
if n_samples > self.MAX_SAMPLES:
n_samples = self.MAX_SAMPLES
sample_rate = n_samples / duration
| step = 1e-6 | ||
| positions = numpy.arange(-80e-6, 80e-6, step) | ||
|
|
||
| for target_x in positions: | ||
| f = self.scan_stage.moveAbs({"x": float(target_x)}) | ||
| f.result(timeout=10) | ||
| time.sleep(0.001) | ||
|
|
||
| self.assertAlmostEqual(self.scan_stage.position.value["x"], 80e-6, places=7) |
| v_per_m = abs(rng_v[1] - rng_v[0]) / (rng_m[1] - rng_m[0]) | ||
| speed_v_s = LiteScope.MAX_SPEED_RETRACTED_M_S * v_per_m | ||
| dv = abs(self.scan_stage._m_to_v("x", target_x) - self.scan_stage._m_to_v("x", x_start)) | ||
| theoretical_s = (dv / speed_v_s) * 1.5 # smoothstep peak factor |
There was a problem hiding this comment.
That seems like a very convoluted way to get the duration of the move while the driver provides a speed information (m/s). But I guess it's because you want the exact duration it'll take, right? If so, wouldn't be simpler to call _generate_waveform() and deduce the duration based on the number of samples and sample rate?
| to prevent overshoot and mechanical stress on the piezo stages. | ||
| """ | ||
|
|
||
| TARGET_SAMPLE_RATE = 10000.0 # 10 kHz target sample rate for smooth analog output |
There was a problem hiding this comment.
These constants at the top of the class are a bit atypical for our codebase I think. Most drivers have them at the top of the module.
| rng_v = self._range_v[axis] | ||
| return rng_v[0] + (val_m - rng_m[0]) / (rng_m[1] - rng_m[0]) * (rng_v[1] - rng_v[0]) | ||
|
|
||
| def _v_to_m(self, axis: str, val_v: float) -> float: |
There was a problem hiding this comment.
_voltage_to_meters gives it more context I would say
|
|
||
| return numpy.vstack(waves), sample_rate, n_samples, duration, max_dv | ||
|
|
||
| def _write_ao_finite(self, axes: List[str], waveform: numpy.ndarray, sample_rate: float, |
There was a problem hiding this comment.
ao stands for analog output? Maybe clarify it in the docstring (or even function name).
Uh oh!
There was an error while loading. Please reload this page.