[feat] andorcam3: better handling of USB camera opening#3399
[feat] andorcam3: better handling of USB camera opening#3399pieleric wants to merge 1 commit intodelmic:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a udev rule Sequence Diagram(s)sequenceDiagram
actor App as Application
participant Driver as AndorCam3 Driver
participant SysFS as Linux SysFS
participant USB as USB Device
participant Udev as udev
App->>Driver: Open()/connect
Driver->>Driver: _check_connection(serial?)
Driver->>SysFS: _find_device_usb_path(serial?)
SysFS-->>Driver: sysfs path or None
alt AT_Open raises ERR_CONNECTION
Driver->>Driver: _reset_usb_connection(sys_path)
Driver->>SysFS: write "0" to /sys/bus/usb/devices/%k/authorized
SysFS->>USB: disable device
Driver->>SysFS: write "1" to /sys/bus/usb/devices/%k/authorized
SysFS->>Udev: device re-enumeration
Udev-->>SysFS: apply 80-andor-reset.rules
SysFS-->>USB: device re-initialized
Driver->>Driver: retry AT_Open()
alt retry succeeds
Driver-->>App: connection established
else retry fails
Driver-->>App: raise ATError
end
else no error
Driver-->>App: connection established
end
🚥 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 |
![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 (2)
src/odemis/driver/test/andorcam3_test.py (1)
110-179: The new reset/retry branch still isn't covered here.These cases only exercise
_check_connection(). The main behavior added inOpen()is theERR_CONNECTION -> _reset_usb_connection() -> retryflow, so I'd add one small unit test that makesAT_Openfail once with errno10, then succeed, and asserts_reset_usb_connection()is called exactly once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/test/andorcam3_test.py` around lines 110 - 179, Add a unit test that exercises the new Open() reset/retry branch by creating a camera via _make_camera and patching andorcam3.AT_Open on the camera instance to raise OSError(errno=10) on the first call and return success on the second (use side_effect), patching camera._reset_usb_connection to a mock, then call camera.Open() and assert that _reset_usb_connection was called exactly once and that AT_Open was invoked twice (first failing, second succeeding); reference the methods Open, AT_Open, and _reset_usb_connection to locate where to patch and what to assert.install/linux/lib/udev/rules.d/80-andor-reset.rules (1)
6-6: Confirm udev rule is correctly designed, but verify that non-root users can write to the authorized file if needed.The udev rule correctly grants group write access to
/sys/bus/usb/devices/%k/authorizedfor theodemisgroup. However, theodemisgroup is only created by the package—no users are automatically added to it. If the backend ever runs as a non-root user, that user must be manually added to theodemisgroup (as documented indoc/INSTALL.txtline 68). Currently,odemisdruns as root viasudo, so the group permissions are not strictly required for function, but they are good practice for future flexibility or alternative deployment scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/linux/lib/udev/rules.d/80-andor-reset.rules` at line 6, The udev rule grants group write to /sys/bus/usb/devices/%k/authorized for group "odemis" but non-root backend users may not be in that group; update the packaging or deployment so the service user for the backend (or the odemisd runtime user) is added to the "odemis" group (e.g., in the package postinst or systemd unit user setup), or explicitly document the manual step in INSTALL.txt near the existing note, and ensure the udev rule line (SUBSYSTEM=="usb", ATTRS{idVendor}=="136e", RUN+="/bin/chmod g+w /sys/bus/usb/devices/%k/authorized", RUN+="/bin/chgrp odemis /sys/bus/usb/devices/%k/authorized") remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/driver/andorcam3.py`:
- Around line 607-608: Replace the unclosed open() call and the single-letter
iteration variable: open "/proc/modules" using a context manager (with
open("/proc/modules") as f:) and assign fmodules = f.readlines(), and change the
generator expression to use a clear variable name (e.g., line) instead of l so
the any(...) check becomes any(re.match("bitflow", line) for line in fmodules);
this fixes the unclosed file and the Ruff E741 warning.
- Around line 515-534: The current logic returns valid_paths[0] too eagerly and
lets "VSC-ANDOR" placeholders override exact serials; change the selection so
that when serial is None and len(valid_paths) > 1 the function returns None (do
not guess), and when a serial is provided iterate over valid_paths reading the
"serial" file into serial_path and collect exact_matches (serial_path == serial)
and placeholder_matches (serial_path == "VSC-ANDOR"); if exact_matches non-empty
return the first exact match, else if there is exactly one placeholder_match
return that single placeholder, otherwise log a warning (mentioning serial and
the ambiguous paths) and return None so callers like Open() and
_check_connection() do not accidentally use the wrong USB path.
---
Nitpick comments:
In `@install/linux/lib/udev/rules.d/80-andor-reset.rules`:
- Line 6: The udev rule grants group write to /sys/bus/usb/devices/%k/authorized
for group "odemis" but non-root backend users may not be in that group; update
the packaging or deployment so the service user for the backend (or the odemisd
runtime user) is added to the "odemis" group (e.g., in the package postinst or
systemd unit user setup), or explicitly document the manual step in INSTALL.txt
near the existing note, and ensure the udev rule line (SUBSYSTEM=="usb",
ATTRS{idVendor}=="136e", RUN+="/bin/chmod g+w
/sys/bus/usb/devices/%k/authorized", RUN+="/bin/chgrp odemis
/sys/bus/usb/devices/%k/authorized") remains unchanged.
In `@src/odemis/driver/test/andorcam3_test.py`:
- Around line 110-179: Add a unit test that exercises the new Open() reset/retry
branch by creating a camera via _make_camera and patching andorcam3.AT_Open on
the camera instance to raise OSError(errno=10) on the first call and return
success on the second (use side_effect), patching camera._reset_usb_connection
to a mock, then call camera.Open() and assert that _reset_usb_connection was
called exactly once and that AT_Open was invoked twice (first failing, second
succeeding); reference the methods Open, AT_Open, and _reset_usb_connection to
locate where to patch and what to assert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9bf42bb-38c1-4bd4-8610-702dcec1c96e
📒 Files selected for processing (3)
install/linux/lib/udev/rules.d/80-andor-reset.rulessrc/odemis/driver/andorcam3.pysrc/odemis/driver/test/andorcam3_test.py
src/odemis/driver/andorcam3.py
Outdated
| if not valid_paths: | ||
| return None | ||
| if serial is None or len(valid_paths) == 1: | ||
| return valid_paths[0] | ||
|
|
||
| # Multiple Andor USB devices found, try to find the one matching the serial | ||
| for sys_path in valid_paths: | ||
| try: | ||
| with open(os.path.join(sys_path, "serial")) as f: | ||
| serial_path = f.read().strip() | ||
| except (IOError, UnicodeDecodeError): | ||
| continue | ||
| # Some cameras don't have the actual serial number, but just VSC-ANDOR. In such case, | ||
| # let's be optimistic and consider it a match. | ||
| if serial_path in (serial, "VSC-ANDOR"): | ||
| return sys_path | ||
|
|
||
| logging.warning("Multiple Andor USB devices found and none matched serial %s, using %s", | ||
| serial, valid_paths[0]) | ||
| return valid_paths[0] |
There was a problem hiding this comment.
Avoid guessing the USB path when more than one Andor camera is attached.
Lines 517-518 currently pick valid_paths[0] whenever Open() asks for a reset without a serial, so a failed open can disconnect an unrelated camera. And on Lines 521-530, a VSC-ANDOR placeholder can win before a later exact serial match, which makes _check_connection() validate the wrong USB path. Prefer an exact serial match first, only fall back to a single placeholder match, and return None when the match is ambiguous.
🔧 Proposed fix
if not valid_paths:
return None
- if serial is None or len(valid_paths) == 1:
+ if len(valid_paths) == 1:
return valid_paths[0]
+ if serial is None:
+ logging.warning("Multiple Andor USB devices found; refusing to guess without a serial")
+ return None
- # Multiple Andor USB devices found, try to find the one matching the serial
+ placeholder_paths: List[str] = []
for sys_path in valid_paths:
try:
with open(os.path.join(sys_path, "serial")) as f:
serial_path = f.read().strip()
except (IOError, UnicodeDecodeError):
continue
- # Some cameras don't have the actual serial number, but just VSC-ANDOR. In such case,
- # let's be optimistic and consider it a match.
- if serial_path in (serial, "VSC-ANDOR"):
+ if serial_path == serial:
return sys_path
+ if serial_path == "VSC-ANDOR":
+ placeholder_paths.append(sys_path)
- logging.warning("Multiple Andor USB devices found and none matched serial %s, using %s",
- serial, valid_paths[0])
- return valid_paths[0]
+ if len(placeholder_paths) == 1:
+ return placeholder_paths[0]
+ logging.warning("Multiple Andor USB devices found and serial %s was ambiguous", serial)
+ return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/driver/andorcam3.py` around lines 515 - 534, The current logic
returns valid_paths[0] too eagerly and lets "VSC-ANDOR" placeholders override
exact serials; change the selection so that when serial is None and
len(valid_paths) > 1 the function returns None (do not guess), and when a serial
is provided iterate over valid_paths reading the "serial" file into serial_path
and collect exact_matches (serial_path == serial) and placeholder_matches
(serial_path == "VSC-ANDOR"); if exact_matches non-empty return the first exact
match, else if there is exactly one placeholder_match return that single
placeholder, otherwise log a warning (mentioning serial and the ambiguous paths)
and return None so callers like Open() and _check_connection() do not
accidentally use the wrong USB path.
| fmodules = open("/proc/modules").readlines() | ||
| if not any(re.match("bitflow", l) for l in fmodules): |
There was a problem hiding this comment.
Use a context manager for /proc/modules.
Line 607 leaves the file open, and Line 608's l trips Ruff E741 in this hunk.
🧹 Proposed fix
- fmodules = open("/proc/modules").readlines()
- if not any(re.match("bitflow", l) for l in fmodules):
+ with open("/proc/modules") as modules_file:
+ if not any(re.match("bitflow", line) for line in modules_file):📝 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.
| fmodules = open("/proc/modules").readlines() | |
| if not any(re.match("bitflow", l) for l in fmodules): | |
| with open("/proc/modules") as modules_file: | |
| if not any(re.match("bitflow", line) for line in modules_file): |
🧰 Tools
🪛 Ruff (0.15.5)
[error] 608-608: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/driver/andorcam3.py` around lines 607 - 608, Replace the unclosed
open() call and the single-letter iteration variable: open "/proc/modules" using
a context manager (with open("/proc/modules") as f:) and assign fmodules =
f.readlines(), and change the generator expression to use a clear variable name
(e.g., line) instead of l so the any(...) check becomes any(re.match("bitflow",
line) for line in fmodules); this fixes the unclosed file and the Ruff E741
warning.
There was a problem hiding this comment.
Pull request overview
Improves robustness of the AndorCam3 driver when opening USB cameras by better identifying devices in sysfs (including the special VSC-ANDOR serial marker) and attempting a software USB reset on connection errors.
Changes:
- Add sysfs-based USB device discovery using Andor VID/PID and tolerate
VSC-ANDORas a serial marker. - On
ERR_CONNECTIONduringAT_Open, attempt to reset USB authorization and retry opening. - Add unit tests for
_check_connection()USB validation logic and install a udev rule to permit USB authorization toggling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/odemis/driver/andorcam3.py |
Adds USB sysfs discovery + USB reset helper and retries open on connection error. |
src/odemis/driver/test/andorcam3_test.py |
Adds unit tests covering the updated USB-path selection/validation behavior. |
install/linux/lib/udev/rules.d/80-andor-reset.rules |
Grants permissions needed to toggle authorized for Andor USB devices. |
You can also share your feedback on Copilot code review. Take the survey.
pieleric
force-pushed
the
feat-andorcam3-better-handling-of-usb-camera-opening
branch
from
654d983 to
d7f6d3d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/odemis/driver/test/andorcam3_test.py (1)
128-153: Consider clarifying the test name.The test name
test_check_connection_vsc_andor_serial_is_acceptedmight be misleading since the test expects anHwErrorto be raised. The test validates that the VSC-ANDOR serial is accepted as a match (allowing the USB version check to proceed), not that the connection is accepted. Consider renaming to something liketest_check_connection_vsc_andor_serial_matches_and_usb2_raises.The test logic itself is correct and valuable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/test/andorcam3_test.py` around lines 128 - 153, The test name test_check_connection_vsc_andor_serial_is_accepted is misleading because it expects an HwError; rename the test function to a clearer name such as test_check_connection_vsc_andor_serial_matches_and_usb2_raises (or similar) to reflect that the VSC-ANDOR serial is matched but a USB2 version triggers an HwError, updating the function definition and any references to the old name in the test class (locate the method named test_check_connection_vsc_andor_serial_is_accepted in andorcam3_test.py and replace it with the new descriptive name).src/odemis/driver/andorcam3.py (1)
38-38: Unused import:Tupleis not used in the new code.The
Tupletype is imported but doesn't appear to be used in the newly added methods (_find_device_usb_path,_reset_usb_connection). Consider removing it if not needed elsewhere.🧹 Proposed fix
-from typing import Dict, Set, List, Optional, Tuple +from typing import Dict, Set, List, Optional🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorcam3.py` at line 38, Remove the unused Tuple type from the typing import in andorcam3.py: update the import that currently includes Dict, Set, List, Optional, Tuple to exclude Tuple, and run a quick grep/IDE search for "Tuple" to confirm there are no other references (particularly around the methods _find_device_usb_path and _reset_usb_connection) before committing.
🤖 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/andorcam3.py`:
- Line 38: Remove the unused Tuple type from the typing import in andorcam3.py:
update the import that currently includes Dict, Set, List, Optional, Tuple to
exclude Tuple, and run a quick grep/IDE search for "Tuple" to confirm there are
no other references (particularly around the methods _find_device_usb_path and
_reset_usb_connection) before committing.
In `@src/odemis/driver/test/andorcam3_test.py`:
- Around line 128-153: The test name
test_check_connection_vsc_andor_serial_is_accepted is misleading because it
expects an HwError; rename the test function to a clearer name such as
test_check_connection_vsc_andor_serial_matches_and_usb2_raises (or similar) to
reflect that the VSC-ANDOR serial is matched but a USB2 version triggers an
HwError, updating the function definition and any references to the old name in
the test class (locate the method named
test_check_connection_vsc_andor_serial_is_accepted in andorcam3_test.py and
replace it with the new descriptive name).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96a5ca3e-6c0b-43df-a994-b86e5515a117
📒 Files selected for processing (3)
install/linux/lib/udev/rules.d/80-andor-reset.rulessrc/odemis/driver/andorcam3.pysrc/odemis/driver/test/andorcam3_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- install/linux/lib/udev/rules.d/80-andor-reset.rules
pieleric
force-pushed
the
feat-andorcam3-better-handling-of-usb-camera-opening
branch
from
d7f6d3d to
13901f8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/odemis/driver/andorcam3.py (1)
38-38: UnusedTupleimport.
Tupleis imported but not used in the new code. The new functions useOptional[str],List[str], andboolas return types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorcam3.py` at line 38, The import list in andorcam3.py includes an unused symbol Tuple; remove Tuple from the typing import (i.e., change "from typing import Dict, Set, List, Optional, Tuple" to exclude Tuple) so only the actually used types (Dict, Set, List, Optional) are imported and unused-import warnings are eliminated; search for the import line or any references to Tuple in this module (e.g., top-level imports in andorcam3.py) and delete Tuple from that import statement.
🤖 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/andorcam3.py`:
- Line 38: The import list in andorcam3.py includes an unused symbol Tuple;
remove Tuple from the typing import (i.e., change "from typing import Dict, Set,
List, Optional, Tuple" to exclude Tuple) so only the actually used types (Dict,
Set, List, Optional) are imported and unused-import warnings are eliminated;
search for the import line or any references to Tuple in this module (e.g.,
top-level imports in andorcam3.py) and delete Tuple from that import statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4cf0d3d1-6cf4-4fe4-b02b-e30cd62a5e7e
📒 Files selected for processing (3)
install/linux/lib/udev/rules.d/80-andor-reset.rulessrc/odemis/driver/andorcam3.pysrc/odemis/driver/test/andorcam3_test.py
✅ Files skipped from review due to trivial changes (1)
- install/linux/lib/udev/rules.d/80-andor-reset.rules
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/driver/test/andorcam3_test.py
Some USB cameras, instead of reporting their actual serial number have "VSC-ANDOR" as serial number. => Concider it as a valid serial number too. We check also the USB product ID to be extra careful. Also, some cameras have sometimes a bad mood and don't connect at init. From testing, it appears that just resetting the USB connection in software helps the connection. So detect such case, and try it. This helps in particular with the Zyla 5.5.
pieleric
force-pushed
the
feat-andorcam3-better-handling-of-usb-camera-opening
branch
from
13901f8 to
cc71dc8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/odemis/driver/andorcam3.py (1)
585-599: Consider passing serial number for more precise device identification during recovery.When
_find_device_usb_path()is called without a serial (line 588), it returns the first valid Andor device found. On systems with multiple Andor cameras, this could potentially reset the wrong device.At this point in
Open(), the camera handle isn't established yet soGetString("SerialNumber")isn't available. However, if thedeviceparameter correlates with a known serial (perhaps cached duringscan()), passing it would improve precision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/andorcam3.py` around lines 585 - 599, Open() may reset the wrong USB device because _find_device_usb_path() is called without a serial and returns the first Andor device; change the recovery path to try locating the USB sys_path for the specific camera before resetting by passing a serial identifier (if available) to _find_device_usb_path(serial) instead of calling it with no args, deriving that serial from the device-to-serial cache built by scan() or another stored mapping, and only call _reset_usb_connection(sys_path) when the returned sys_path matches the serial for the requested device (functions to touch: Open(), _find_device_usb_path, _reset_usb_connection, and the scan() cache).src/odemis/driver/test/andorcam3_test.py (1)
128-153: Test name is misleading given the expected outcome.The test name
test_check_connection_vsc_andor_serial_is_acceptedsuggests that connections with "VSC-ANDOR" serial are accepted successfully, but the test actually expectsHwErrorto be raised (line 152). The test verifies that VSC-ANDOR serial matching works as a fallback mechanism, after which the USB2 version check triggers the error.Consider renaming to clarify intent:
📝 Suggested rename
- def test_check_connection_vsc_andor_serial_is_accepted(self) -> None: + def test_check_connection_vsc_andor_fallback_with_usb2_raises(self) -> None: """ - Ensure "VSC-ANDOR" serial is accepted when VID:PID is valid. + Ensure VSC-ANDOR serial fallback works and USB2 still triggers error. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/test/andorcam3_test.py` around lines 128 - 153, The test method name is misleading: test_check_connection_vsc_andor_serial_is_accepted implies success but the body expects andorcam3.HwError from camera._check_connection(); rename the test to reflect the actual behavior (e.g., test_check_connection_vsc_andor_serial_matches_but_usb2_version_raises_HwError or similar) so the name matches that camera._check_connection() accepts the VSC-ANDOR serial as a fallback but then fails on the USB2 version check. Update only the test method name to a descriptive, accurate identifier.
🤖 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/andorcam3.py`:
- Around line 585-599: Open() may reset the wrong USB device because
_find_device_usb_path() is called without a serial and returns the first Andor
device; change the recovery path to try locating the USB sys_path for the
specific camera before resetting by passing a serial identifier (if available)
to _find_device_usb_path(serial) instead of calling it with no args, deriving
that serial from the device-to-serial cache built by scan() or another stored
mapping, and only call _reset_usb_connection(sys_path) when the returned
sys_path matches the serial for the requested device (functions to touch:
Open(), _find_device_usb_path, _reset_usb_connection, and the scan() cache).
In `@src/odemis/driver/test/andorcam3_test.py`:
- Around line 128-153: The test method name is misleading:
test_check_connection_vsc_andor_serial_is_accepted implies success but the body
expects andorcam3.HwError from camera._check_connection(); rename the test to
reflect the actual behavior (e.g.,
test_check_connection_vsc_andor_serial_matches_but_usb2_version_raises_HwError
or similar) so the name matches that camera._check_connection() accepts the
VSC-ANDOR serial as a fallback but then fails on the USB2 version check. Update
only the test method name to a descriptive, accurate identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8ca0c84-b891-4078-a0a1-eb42e877ee0f
📒 Files selected for processing (3)
install/linux/lib/udev/rules.d/80-andor-reset.rulessrc/odemis/driver/andorcam3.pysrc/odemis/driver/test/andorcam3_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- install/linux/lib/udev/rules.d/80-andor-reset.rules
Some USB cameras, instead of reporting their actual serial number have
"VSC-ANDOR" as serial number. => Concider it as a valid serial number
too. We check also the USB product ID to be extra careful.
Also, some cameras have sometimes a bad mood and don't connect at init.
From testing, it appears that just resetting the USB connection in
software helps the connection. So detect such case, and try it.
This helps in particular with the Zyla 5.5.