[feat] driver tucsen: support stopping acquisition at any time#3332
Conversation
d7ac480 to
451810c
Compare
📝 WalkthroughWalkthroughThis PR converts the Tucsen driver to a callback-driven capture model. It adds Sequence Diagram(s)sequenceDiagram
participant App as Application
participant TUCam as TUCam (wrapper)
participant DLL as TUCamDLL / FakeTUCamDLL
participant Cam as Camera SDK / Hardware
participant Flow as DataFlow
App->>TUCam: start_acquisition()
activate TUCam
TUCam->>DLL: register_data_callback(on_frame)
activate DLL
DLL->>Cam: install C callback (or start fake thread)
deactivate DLL
TUCam->>Cam: start_capture()
deactivate TUCam
Note over Cam: camera produces frames asynchronously
Cam->>DLL: invoke C callback(frame_ptr)
activate DLL
DLL->>TUCam: call Python on_frame(numpy_array)
deactivate DLL
activate TUCam
TUCam->>Flow: emit frame + metadata
TUCam->>TUCam: post AcqMessage.FRAME
deactivate TUCam
App->>TUCam: acquisition loop processes FRAME
activate TUCam
TUCam->>App: deliver frame via DataFlow
deactivate TUCam
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/odemis/driver/test/tucsen_test.py`:
- Around line 51-72: The test TestCallBack.test_callback_registration currently
instantiates TUCamDLL unconditionally, has no assertions that the on_frame
callback ran, and may leak camera resources on failure; update the test to
respect TEST_NOHW by using existing KWARGS/KWARGS_SIM selection when creating
TUCamDLL (use KWARGS_SIM if TEST_NOHW is set), add a thread-safe counter or
threading.Event updated by on_frame to assert that at least one frame was
received, and ensure camera open/close and capture start/end are done in
setUp/tearDown or a try/finally so resources are always released (refer to
TestCallBack, on_frame, test_callback_registration, and TUCamDLL for where to
apply these changes).
In `@src/odemis/driver/tucsen.py`:
- Around line 1884-1893: capture_frame currently blocks twice for the exposure
period (time.sleep then self._capture_stopped.wait), causing a 2x delay; remove
the redundant sleep and rely on self._capture_stopped.wait(self._exposure_time)
to both wait the exposure time and detect aborts so capture_frame behavior
matches _capture_loop. Reference: function capture_frame and synchronization
primitive self._capture_stopped.wait.
- Around line 1580-1597: The _data_callback currently only catches TUCamError so
exceptions from numpy operations or the user callback (self._on_data) can escape
into the C thread; update _data_callback to catch all exceptions (e.g., a bare
except Exception) around the whole body after invoking TUCAM_Buf_GetData and the
numpy conversion/callback invocation, log the full exception (including
traceback) via logging.exception, and ensure the function returns cleanly on any
error; keep the existing TUCamError handling behavior but broaden it to
Exception to prevent crashes in the C-thread context while still calling the
user callback when safe.
🧹 Nitpick comments (2)
src/odemis/driver/tucsen.py (2)
1208-1208: Type hint uses lowercasecallableinstead ofCallablefrom typing.The type hint
Optional[callable]uses the lowercasecallablewhich is a built-in function, not a type. For proper type hints, useCallablefrom thetypingmodule (already imported on line 33).Suggested fix
- self._on_data: Optional[callable] = None + self._on_data: Optional[Callable] = NoneAlso update line 1599 and line 1811 similarly.
2444-2453: Consider implementing a timeout for frame reception.The TODO comment on line 2447 raises a valid concern. If the camera hardware fails silently and stops sending frames, the acquisition loop will hang indefinitely. Consider adding a timeout based on a multiple of the expected frame time (e.g.,
3 * exp) and logging a warning or raising an error if triggered.Would you like me to propose an implementation with a watchdog timeout?
There was a problem hiding this comment.
Pull request overview
This PR refactors the Tucsen camera driver to use a callback-based acquisition structure instead of polling for frames. This change enables stopping acquisition at any time and follows Tucsen's recommended approach for optimal performance.
Changes:
- Modified the frame acquisition architecture to use SDK callbacks instead of polling with
capture_frame() - Added callback registration and handling in both real and simulated camera implementations
- Updated acquisition thread logic to work with the callback-based approach using message passing
- Added test for callback mechanism
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 21 comments.
| File | Description |
|---|---|
| src/odemis/driver/tucsen.py | Refactored acquisition to use callbacks; added _data_callback, register_data_callback methods; updated FakeTUCamDLL to simulate async capture with threading; modified acquisition loop to use message-based synchronization |
| src/odemis/driver/test/tucsen_test.py | Added TestCallBack class to test callback registration and frame capture; updated logging format to include line numbers |
451810c to
e9e0f1f
Compare
Change acquisition structure to use a callback when the camera sends a frame. This way, it's possible to stop at any time. It is also the suggested way by Tucsen to optimise for speed.
e9e0f1f to
29e6356
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/odemis/driver/tucsen.py (1)
1205-1210: UseCallableinstead ofcallablefor type hints.The lowercase
callableis a built-in function, not a type. For type annotations, useCallablefrom thetypingmodule.Suggested fix
Add to imports at line 33:
from typing import Tuple, Optional, Dict, Any, CallableThen update line 1208:
- self._on_data: Optional[callable] = None + self._on_data: Optional[Callable[[numpy.ndarray], None]] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/tucsen.py` around lines 1205 - 1210, Replace the incorrect use of the builtin `callable` in the type annotation with the typing `Callable`: update the import list to include `Callable` from `typing` and change the attribute annotation `self._on_data: Optional[callable] = None` to `Optional[Callable]`; ensure this change is applied near the definitions of `_callback_func`, `_callback_none`, `_on_data`, and `m_raw_header` (symbols: BUFFER_CALLBACK, _data_callback, _callback_none, _on_data, TUCAM_RAWIMG_HEADER).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/odemis/driver/tucsen.py`:
- Around line 1205-1210: Replace the incorrect use of the builtin `callable` in
the type annotation with the typing `Callable`: update the import list to
include `Callable` from `typing` and change the attribute annotation
`self._on_data: Optional[callable] = None` to `Optional[Callable]`; ensure this
change is applied near the definitions of `_callback_func`, `_callback_none`,
`_on_data`, and `m_raw_header` (symbols: BUFFER_CALLBACK, _data_callback,
_callback_none, _on_data, TUCAM_RAWIMG_HEADER).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 449b6031-7e5f-4f15-8f63-92d237656b0e
📒 Files selected for processing (2)
src/odemis/driver/test/tucsen_test.pysrc/odemis/driver/tucsen.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/driver/test/tucsen_test.py
Change acquisition structure to use a callback when the camera sends a frame. This way, it's possible to stop at any time. It is also the suggested way by Tucsen to optimise for speed.