[MSD-240][feature] Multiple sharksem connections #3297
Conversation
tmoerkerken
force-pushed
the
multiple-sharksem-connections
branch
from
2fab3fb to
2db561d
Compare
📝 WalkthroughWalkthroughDeviceHandler now accepts host, port, socket_timeout, and optional device_type; it opens TCP sockets, constructs a sem.Sem connection, sets socket options, and stores a shared _sharksem_lock for serialized SharkSEM calls. A del attempts a safe Disconnect. Device-type handling accepts ELECTRON, ION, or None and maps FIB names via FIB_NAME_MAP with a warning fallback. Many components (Stage, Detector, Scanner, EbeamFocus, Chamber*, etc.) were changed to use DeviceHandler and route all SharkSEM API calls through its lock-protected methods. Sequence Diagram(s)mermaid 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
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/odemis/driver/tescan.py (2)
119-181: Shared per-device lock is good, but FIBNonemappings will still blow upThe shared
_sharksem_lockattached to thesem.Seminstance is a sensible way to serialize access per device across multipleDeviceHandlers. However, the ION path in__getattr__has a bug for methods whose FIB mapping is explicitlyNoneinFIB_NAME_MAP(e.g.ScGetBlanker,SetBeamCurrent, etc.):fib_name = FIB_NAME_MAP.get(name) if not fib_name: logging.warning(f"No FIB equivalent for '{name}'") try: func = getattr(self.device, fib_name) except AttributeError: ...When
fib_nameisNone, this logs a warning and then callsgetattr(self.device, None), which raises aTypeError, not the intendedAttributeError. That means your “log instead of raising immediately” behaviour doesn’t work and will surface as an unexpected runtime error if any of these unmapped methods are used with an ION handler.Consider failing fast (or becoming a no-op) before the
getattr:elif self.device_type == DeviceType.ION: fib_name = FIB_NAME_MAP.get(name) - if not fib_name: - logging.warning(f"No FIB equivalent for '{name}'") - try: - func = getattr(self.device, fib_name) - except AttributeError: - raise AttributeError(f"FIB equivalent method '{fib_name}' for '{name}' does not exist in the API") + if fib_name is None: + # No FIB equivalent – log and surface a clear error instead of + # trying getattr(None, ...) which would raise a TypeError. + logging.warning("No FIB equivalent for '%s'", name) + raise AttributeError(f"No FIB equivalent for '{name}' on FIB device") + try: + func = getattr(self.device, fib_name) + except AttributeError: + raise AttributeError( + f"FIB equivalent method '{fib_name}' for '{name}' does not exist in the API" + )(Separately, the
methoddocstring and the__getattr__return type still mention adevice_typeargument that no longer exists; updating those would make the API clearer.)
550-553: Socket tuning is applied to the wrong connection after per-scanner devices
_single_acquisitionnow runs the scan viascanner._device_handler(and thusscanner._device), but the socket options are still applied toself._device(the SEM-level connection):# make sure socket settings are always set self._device.connection.socket_c.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) self._device.connection.socket_d.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)After the change to instantiate a separate
SemperScanner, this no longer affects the sockets actually used forScScanXY/FetchImageEx, so you may reintroduce the 200 ms Nagle-related latency on the real scan connection.It would be safer to configure the scanner’s connection:
- # make sure socket settings are always set - self._device.connection.socket_c.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) - self._device.connection.socket_d.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) + # Make sure socket settings are always set on the scanner's connection + scan_device = scanner._device_handler.device + scan_device.connection.socket_c.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) + scan_device.connection.socket_d.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)If you still need these options on
SEM._devicefor camera/vacuum, that can be done separately, but the acquisition path should at least hit the scanner’s sockets.
🧹 Nitpick comments (1)
src/odemis/driver/tescan.py (1)
1578-1580: EbeamFocus connection and WD move pathEbeamFocus now uses its own
connect_device+DeviceHandler, and_updatePosition/_doMovego throughGetWD/SetWDon that handler, which is consistent with the new per-component connection model.One minor nit:
_doMoveignores itsposparameter and usesself._position["z"]instead, which is kept in sync by the callers (moveRel/moveAbs) but makes the signature somewhat misleading. Either:
- drop the unused
posparameter, or- use
posexplicitly in_doMove(e.g.SetWD(pos["z"] * 1e3)and then assign back to_position).Purely cosmetic, but it makes the intent clearer and reduces the chance of future refactors accidentally desynchronizing
_positionvs.pos.Also applies to: 1608-1613, 1620-1623
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/driver/tescan.py(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/odemis/driver/tescan.py (2)
src/odemis/model/_components.py (2)
HwError(37-43)model(570-571)src/odemis/driver/phenom.py (6)
GetWD(1407-1408)GetWD(1514-1515)GetWD(1730-1733)SetWD(1411-1412)SetWD(1517-1518)SetWD(1735-1737)
🪛 GitHub Actions: Linting
src/odemis/driver/tescan.py
[error] 1-1: PNG metadata check detected forbidden chunks (tEXt|zTXt|iTXt|eXIf|tIME) in file. Contains forbidden metadata chunks.
🔇 Additional comments (1)
src/odemis/driver/tescan.py (1)
1322-1363: Pass current position instead ofNonetoStgMoveTofor unchanged axes; remove redundantStgStop()callThe
_moveAbsmethod setstescan_pos = Nonefor axes below movement thresholds or not included in the move, then passes theseNonevalues toStgMoveTo(). Without explicit API documentation confirming that thetescansharksembinding acceptsNoneas a sentinel value, this pattern is unsafe and likely causesTypeErrorat runtime, since the underlying C API expects numeric values.Replace
Nonewith the current hardware position for "unchanged" axes:
- Where
tescan_pos = Noneis assigned for below-threshold moves, usetescan_pos = current_pos[axis]instead- Where
tescan_pos = Noneis assigned for axes not in the move request, usetescan_pos = current_pos[axis]insteadThis preserves the "skip tiny moves" semantics while guaranteeing valid numeric arguments to the API.
Additionally,
StgStop()is called twice in thestop()method—once implicitly via executor cancellation and once explicitly. Remove the redundant explicit call.
src/odemis/driver/tescan.py
Outdated
| fib_name = FIB_NAME_MAP.get(name) | ||
| if not fib_name: | ||
| raise AttributeError(f"No FIB equivalent for '{name}'") | ||
| logging.warning(f"No FIB equivalent for '{name}'") |
There was a problem hiding this comment.
Why changing this? Now it will call getattr(..., None) which will fail and return an odd exception message.
Or did you mean to replace return the getattr(..., name) in such case?
There was a problem hiding this comment.
Well, it does not always fail! For instance, sometimes the method does exist on the API, but it's not yet part of the map. This is more for running it in development, and testing new API calls. But I see it is not correctly implemented, so I'll fix that!
tmoerkerken
force-pushed
the
multiple-sharksem-connections
branch
from
2db561d to
a9eb567
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/driver/tescan.py (1)
1616-1626: Correct the misleading comment.Line 1621 states "Position from m to mm and inverted" but line 1622 only converts units (m to mm via
* 1e03) without applying inversion. Unlike Stage axes (line 1443), the focus working distance is not inverted.Apply this diff:
- # Position from m to mm and inverted + # Position from m to mm self._device_handler.SetWD(self._position["z"] * 1e03)
♻️ Duplicate comments (3)
src/odemis/driver/tescan.py (3)
1125-1126: Cleanup pattern correct for Scanner.Setting
_device_handler = Noneallows the__del__destructor to close the connection. However, note that Stage and EbeamFocus components (which also create DeviceHandlers) do not follow this same cleanup pattern in their terminate methods, as flagged in previous reviews.
1565-1571: Add DeviceHandler cleanup to Stage.terminate().The Stage creates its own DeviceHandler connection at line 1326, but unlike Scanner (line 1126), it never cleans up this connection. Over time, this can exhaust the 8-connection limit mentioned in the PR description.
Apply this diff:
def terminate(self): self._xyz_poll.cancel() self._xyz_poll.join(5) if self._executor: self.stop() self._executor.shutdown() self._executor = None + # Close dedicated SharkSEM connection for this stage + self._device_handler = None
1660-1664: Add DeviceHandler cleanup to EbeamFocus.terminate().The EbeamFocus creates its own DeviceHandler connection at line 1581, but never cleans up this connection in terminate(). This can contribute to exhausting the 8-connection limit.
Apply this diff:
def terminate(self): if self._executor: self.stop() self._executor.shutdown() self._executor = None + # Close dedicated SharkSEM connection for this focus actuator + self._device_handler = None
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/driver/tescan.py(25 hunks)src/odemis/driver/test/tescan_test.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/odemis/driver/tescan.py (2)
src/odemis/model/_components.py (2)
HwError(37-43)model(570-571)src/odemis/driver/phenom.py (6)
GetWD(1407-1408)GetWD(1514-1515)GetWD(1730-1733)SetWD(1411-1412)SetWD(1517-1518)SetWD(1735-1737)
🪛 Ruff (0.14.8)
src/odemis/driver/tescan.py
212-214: try-except-pass detected, consider logging the exception
(S110)
212-212: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (4)
src/odemis/driver/tescan.py (2)
191-206: FIB method resolution is permissive by design.The fallback at line 196 (
FIB_NAME_MAP.get(name, name)) allows unmapped method names to attempt calling the SEM equivalent directly. This is intentional for development/testing, but be aware it may mask API incompatibilities at runtime rather than failing fast.
552-553: Socket options correctly configured per connection.Direct access to
device.connection.socket_candsocket_dis acceptable here since each DeviceHandler maintains its ownsem.Sem()instance (line 138) with independent socket connections.src/odemis/driver/test/tescan_test.py (2)
47-47: Excellent improvement to test configurability.Introducing the
MIC_IPenvironment variable withos.getenv("ODEMIS_MIC_IP", "192.168.2.35")allows tests to be easily configured for different network setups without modifying code. The consistent application across all config dictionaries is correct.Also applies to: 72-72, 80-80, 90-90, 99-99
52-52: Remove or clarify detector name reference—this is a new configuration, not a change.The detector name "se" is set in a newly created test file configuration, not changed from "e-t". The
get_detector_idx()method validates detector names against hardware viaDtEnumDetectors(), which will raiseValueErrorif the detector is unavailable. Ensure the "se" detector is available on your test hardware (examples in the codebase show "MD" and "E-T"); this will be caught at runtime when tests connect to the device.
tmoerkerken
force-pushed
the
multiple-sharksem-connections
branch
from
a9eb567 to
668a3ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/odemis/driver/tescan.py (4)
1565-1571: Stage doesn't clean up its DeviceHandler connection on terminate.
Stage.__init__creates its ownDeviceHandlerat line 1326, butterminate()never cleans it up. This was flagged in a past review comment and remains unresolved.Apply this diff to properly release the connection:
def terminate(self): self._xyz_poll.cancel() self._xyz_poll.join(5) if self._executor: self.stop() self._executor.shutdown() self._executor = None + # Close dedicated SharkSEM connection for this stage + if self._device_handler: + self._device_handler.Disconnect() + self._device_handler = NoneWithout this, Stage connections accumulate and can exhaust the 8-connection limit mentioned in the PR description.
Based on past review comments.
1660-1664: EbeamFocus doesn't clean up its DeviceHandler connection on terminate.
EbeamFocus.__init__creates its ownDeviceHandlerat line 1581, butterminate()never cleans it up. This was flagged in a past review comment and remains unresolved.Apply this diff to properly release the connection:
def terminate(self): if self._executor: self.stop() self._executor.shutdown() self._executor = None + # Close dedicated SharkSEM connection for this focus actuator + if self._device_handler: + self._device_handler.Disconnect() + self._device_handler = NoneWithout this, EbeamFocus connections accumulate and can exhaust the 8-connection limit mentioned in the PR description.
Based on past review comments.
1686-1692: Critical: ChamberView accesses non-existentparent._deviceattribute.Lines 1686-1692 (and lines 1711, 1721, 1746, 1755) access
self.parent._device, but the SEM class no longer has a_deviceattribute after the refactoring. Line 238 creates_device_handlerinstead, and line 618 sets_device_handler = Nonein terminate.This will cause
AttributeErrorwhen ChamberView tries to access the camera.Update ChamberView to use the device handler's device attribute:
- self.parent._device.CameraEnable(0, 1, 5, 0) + self.parent._device_handler.CameraEnable(0, 1, 5, 0) # Wait for camera to be enabled - while (self.parent._device.CameraGetStatus(0))[0] != 1: + while (self.parent._device_handler.CameraGetStatus(0))[0] != 1: time.sleep(0.5) # Get a first image to determine the resolution - width, height, img_str = self.parent._device.FetchCameraImage(0) - self.parent._device.CameraDisable() + width, height, img_str = self.parent._device_handler.FetchCameraImage(0) + self.parent._device_handler.CameraDisable()Also update lines 1711, 1721, 1746, and 1755 similarly.
1902-1910: ChamberPressure inconsistently accessesparent._device.While lines 1861 and 1875 correctly use
self.parent._device_handler, lines 1908 and 1910 still access the non-existentself.parent._deviceattribute.Apply this diff:
def _changePressure(self, p): """ Synchronous change of the pressure p (float): target pressure """ if p["vacuum"] == PRESSURE_VENTED: - self.parent._device.VacVent() + self.parent._device_handler.VacVent() else: - self.parent._device.VacPump() + self.parent._device_handler.VacPump()
♻️ Duplicate comments (1)
src/odemis/driver/tescan.py (1)
209-214: Log exceptions in destructor for debugging.While destructors should fail gracefully, completely suppressing all exceptions makes debugging resource cleanup issues very difficult.
Apply this diff to log exceptions at debug level:
def __del__(self): try: self.Disconnect() except Exception: - # Destructors should fail silently - pass + # Destructors should fail silently, but log for debugging + logging.debug("Exception during DeviceHandler cleanup", exc_info=True)This preserves the fail-safe behavior while providing visibility into cleanup issues.
Based on past review comments and static analysis hints.
🧹 Nitpick comments (2)
src/odemis/driver/tescan.py (2)
191-206: Missing log when FIB method falls back to SEM name.Line 196 uses
FIB_NAME_MAP.get(name, name)to fall back to the original SEM method name when no FIB mapping exists. However, the warning at line 198 only logs whenfib_nameis explicitlyNone(from the map), not when the fallback occurs.This means unmapped methods will silently attempt to use the SEM name on a FIB device, which could call incorrect methods or fail unexpectedly.
Consider logging when the fallback is used:
elif self.device_type == DeviceType.ION: - fib_name = FIB_NAME_MAP.get(name, name) - if not fib_name: + fib_name = FIB_NAME_MAP.get(name) + if fib_name is None: + logging.debug(f"No FIB mapping for '{name}', attempting to use SEM method name") + fib_name = name + elif not fib_name: logging.warning(f"No known FIB equivalent for '{name}'") try: func = getattr(self.device, fib_name)This makes the fallback behavior more visible during development.
551-553: Redundant socket option configuration during acquisition.Lines 552-553 re-apply
TCP_NODELAYto both sockets during each acquisition, but these options are already set duringDeviceHandler.__init__at lines 151-155. Socket options typically persist for the lifetime of the socket.Unless there's evidence that socket options are being reset elsewhere, consider removing these redundant calls:
- # make sure socket settings are always set - self._device_handler.device.connection.socket_c.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) - self._device_handler.device.connection.socket_d.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) - with self._acq_progress_lock:If the re-application is necessary due to some external reset behavior, please add a comment explaining why.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/driver/tescan.py(25 hunks)src/odemis/driver/test/tescan_test.py(0 hunks)
💤 Files with no reviewable changes (1)
- src/odemis/driver/test/tescan_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/odemis/driver/tescan.py (1)
src/odemis/model/_components.py (2)
HwError(37-43)model(570-571)
🪛 Ruff (0.14.8)
src/odemis/driver/tescan.py
212-214: try-except-pass detected, consider logging the exception
(S110)
212-212: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
tmoerkerken
force-pushed
the
multiple-sharksem-connections
branch
from
668a3ad to
cab39b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/odemis/driver/tescan.py (6)
1690-1696: Critical:ChamberViewstill references removedself.parent._deviceattribute.The SEM class no longer has a
_deviceattribute after the refactoring—it only has_device_handler. All references toself.parent._deviceinChamberViewwill raiseAttributeErrorat runtime.🔎 Apply this diff to fix the broken references:
- self.parent._device.CameraEnable(0, 1, 5, 0) + self.parent._device_handler.CameraEnable(0, 1, 5, 0) # Wait for camera to be enabled - while (self.parent._device.CameraGetStatus(0))[0] != 1: + while (self.parent._device_handler.CameraGetStatus(0))[0] != 1: time.sleep(0.5) # Get a first image to determine the resolution - width, height, img_str = self.parent._device.FetchCameraImage(0) - self.parent._device.CameraDisable() + width, height, img_str = self.parent._device_handler.FetchCameraImage(0) + self.parent._device_handler.CameraDisable()Also update
GetStatus,start_flow,req_stop_flow, and_acquire_thread_continuousmethods similarly.
1714-1716: Fix remaining_devicereferences inGetStatus.def GetStatus(self): with self.parent._acq_progress_lock: - status = self.parent._device.CameraGetStatus(0) # channel 0, reserved + status = self.parent._device_handler.CameraGetStatus(0) # channel 0, reserved return status[0]
1724-1725: Fix_devicereference instart_flow.def start_flow(self, callback): with self.parent._acq_progress_lock: - self.parent._device.CameraEnable(0, 1, 5, 0) + self.parent._device_handler.CameraEnable(0, 1, 5, 0)
1749-1750: Fix_devicereference inreq_stop_flow.with self.parent._acq_progress_lock: - self.parent._device.CameraDisable() + self.parent._device_handler.CameraDisable()
1758-1759: Fix_devicereference in_acquire_thread_continuous.with self.parent._acq_progress_lock: - width, height, img_str = self.parent._device.FetchCameraImage(0) + width, height, img_str = self.parent._device_handler.FetchCameraImage(0)
1911-1914: Critical:_changePressurestill references removedself.parent._deviceattribute.The
_changePressuremethod usesself.parent._devicewhich no longer exists. This will raiseAttributeErrorwhen attempting to vent or pump the chamber.🔎 Apply this diff to fix:
def _changePressure(self, p): if p["vacuum"] == PRESSURE_VENTED: - self.parent._device.VacVent() + self.parent._device_handler.VacVent() else: - self.parent._device.VacPump() + self.parent._device_handler.VacPump()
🧹 Nitpick comments (2)
src/odemis/driver/tescan.py (2)
157-162: Update misleading comment about shared lock.Based on the learnings, each
DeviceHandlerinstance creates its own independentsem.Sem()connection and has its own lock. The current comment implies lock sharing across multiple instances using the same device, which is not the case.🔎 Suggested comment update:
- # Use a shared lock attached to the underlying SharkSEM device - # so multiple DeviceHandler instances using the same device work nicely together. - if not hasattr(self.device, "_sharksem_lock"): - # attach a lock to the device object - self.device._sharksem_lock = threading.Lock() - self._call_lock = self.device._sharksem_lock + # Use a per-connection lock to synchronize API calls within this DeviceHandler instance. + # Each connection is independent and has its own device object and lock. + self._call_lock = threading.Lock()
209-214: Consider logging withexc_info=Truefor better debugging.While silently handling exceptions in destructors is appropriate, including the exception details in the debug log would help diagnose cleanup issues.
🔎 Suggested improvement:
def __del__(self): try: self.Disconnect() except Exception: # Destructors should fail silently - logging.debug("Could not disconnect from TESCAN server") + logging.debug("Could not disconnect from TESCAN server", exc_info=True)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/driver/tescan.py(27 hunks)src/odemis/driver/test/tescan_test.py(0 hunks)
💤 Files with no reviewable changes (1)
- src/odemis/driver/test/tescan_test.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:06.675Z
Learning: In src/odemis/driver/tescan.py, each DeviceHandler instance creates an independent connection to the Tescan API with its own sem.Sem() device object and per-connection lock. Multiple connections are independent and do not require cross-connection synchronization.
📚 Learning: 2025-12-17T19:02:06.675Z
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:06.675Z
Learning: In src/odemis/driver/tescan.py, ensure that each DeviceHandler instance creates its own independent Tescan API connection using a separate sem.Sem() device object and a per-instance lock. Do not share a single connection or lock across multiple DeviceHandler instances. This implies that multiple concurrent connections are independent and do not require cross-connection synchronization. Reviewers should verify that connections and locks are instance-scoped and that there is no global or class-level shared device object or lock for Tescan interactions.
Applied to files:
src/odemis/driver/tescan.py
🧬 Code graph analysis (1)
src/odemis/driver/tescan.py (1)
src/odemis/model/_components.py (2)
HwError(37-43)model(570-571)
🪛 Ruff (0.14.8)
src/odemis/driver/tescan.py
212-212: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (6)
src/odemis/driver/tescan.py (6)
238-238: LGTM!The SEM class properly instantiates
DeviceHandlerwith connection parameters and uses it consistently throughout.
658-658: LGTM!Scanner correctly creates its own independent
DeviceHandlerconnection with the appropriatedevice_typeparameter.
1143-1143: LGTM!Detector appropriately shares the scanner's
DeviceHandlersince detector operations are tightly coupled with scanner acquisition.
1326-1326: LGTM!Stage correctly creates its own independent
DeviceHandlerconnection.
1583-1583: LGTM!EbeamFocus correctly creates its own independent
DeviceHandlerconnection.
1941-1941: LGTM!Light class correctly uses
_device_handlerfor all API calls.
tmoerkerken
force-pushed
the
multiple-sharksem-connections
branch
from
cab39b7 to
47149e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/driver/tescan.py (1)
191-198: Missing early return when FIB equivalent isNone.When
FIB_NAME_MAP.get(name, name)returnsNone(for methods explicitly mapped toNonelikeScGetBlanker), the code logs a warning but continues to callgetattr(self.device, None), which will raiseTypeError: attribute name must be string, not 'NoneType'instead of the expected behavior.🐛 Proposed fix
elif self.device_type == DeviceType.ION: fib_name = FIB_NAME_MAP.get(name, name) if not fib_name: logging.warning(f"No known FIB equivalent for '{name}'") + return lambda *args, **kwargs: None # No-op for unsupported methods try: func = getattr(self.device, fib_name) except AttributeError: raise AttributeError(f"FIB equivalent method '{fib_name}' for '{name}' does not exist in the API")Alternatively, raise an explicit
AttributeErrorif the intention is to disallow calling these methods on FIB devices.
🤖 Fix all issues with AI agents
In @src/odemis/driver/tescan.py:
- Around line 205-210: The destructor __del__ can trigger AttributeError if
__init__ failed before self.device was set because calling self.Disconnect()
will invoke __getattr__ and access self.device; modify __del__ to first check
for the presence of the device attribute (e.g., using hasattr or getattr(...,
None)) or guard the Disconnect call so it only runs when self.device exists, and
keep the try/except around Disconnect to swallow any remaining exceptions;
reference the __del__ method and the Disconnect method when making the change.
- Around line 1475-1477: The debug log currently prints tescan_pos after you set
it to None, so change the order and/or use a separate variable for the original
requested position: compute/keep the original requested value (e.g.,
requested_pos or requested_tescan_pos) before you set tescan_pos = None, then
call logging.debug with that requested variable along with current_pos and axis;
apply the same fix for the rotational-axis branch (where move_distance <
MIN_MOVE_SIZE_ROT_RAD) to log the original requested rotation rather than None.
🧹 Nitpick comments (1)
src/odemis/driver/tescan.py (1)
548-549: Direct access to internal device bypasses DeviceHandler abstraction.These lines access
self._device_handler.device.connection.socket_*directly, bypassing theDeviceHandlerabstraction layer. While the socket options are already set inDeviceHandler.__init__, the comment suggests they need to be reapplied during acquisition.Consider either:
- Adding a method to
DeviceHandlerto ensure socket options are set- Documenting why these need to be reapplied (if there's a known issue)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/driver/tescan.pysrc/odemis/driver/test/tescan_test.py
💤 Files with no reviewable changes (1)
- src/odemis/driver/test/tescan_test.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:13.022Z
Learning: In src/odemis/driver/tescan.py, each DeviceHandler instance creates an independent connection to the Tescan API with its own sem.Sem() device object and per-connection lock. Multiple connections are independent and do not require cross-connection synchronization.
📚 Learning: 2025-12-17T19:02:06.675Z
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:06.675Z
Learning: In src/odemis/driver/tescan.py, ensure that each DeviceHandler instance creates its own independent Tescan API connection using a separate sem.Sem() device object and a per-instance lock. Do not share a single connection or lock across multiple DeviceHandler instances. This implies that multiple concurrent connections are independent and do not require cross-connection synchronization. Reviewers should verify that connections and locks are instance-scoped and that there is no global or class-level shared device object or lock for Tescan interactions.
Applied to files:
src/odemis/driver/tescan.py
🧬 Code graph analysis (1)
src/odemis/driver/tescan.py (1)
src/odemis/model/_components.py (2)
HwError(37-43)model(570-571)
🪛 Ruff (0.14.10)
src/odemis/driver/tescan.py
208-208: Do not catch blind exception: Exception
(BLE001)
1691-1691: Unpacked variable img_str is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (8)
src/odemis/driver/tescan.py (8)
119-158: Connection initialization looks correct.The connection setup properly creates an independent
sem.Sem()instance perDeviceHandler, configures socket options for low-latency communication, and establishes a per-instance lock. This aligns with the PR objective of enabling multiple parallel connections.One minor observation: the lock is stored on
self.device._sharksem_lockand then aliased toself._call_lock. Since this is instance-scoped and eachDeviceHandlerhas its ownsem.Sem(), the design is correct per the learnings about independent connections.
166-171: Lock-protected API calls look good.The
call_and_log_funcwrapper properly acquires the per-connection lock before invoking the underlying API method, ensuring thread-safe access to each connection.
1139-1142: Detector correctly shares scanner's connection.The detector appropriately reuses the scanner's
DeviceHandlerinstance rather than creating a new connection, which makes sense since they work together during image acquisition.
1322-1322: Stage correctly creates its own independent connection.The Stage component properly instantiates its own
DeviceHandler, enabling parallel access to the Tescan API as intended by the PR.
1579-1579: EbeamFocus correctly creates its own independent connection.
1686-1692: ChamberView uses parent's connection instead of its own.Unlike
Stage,Scanner, andEbeamFocuswhich create their ownDeviceHandlerinstances,ChamberViewusesself.parent._device_handler. This inconsistency may limit parallelism for camera operations.Additionally,
img_stron line 1691 is unused (only width and height are needed to determine resolution).Is this intentional? If camera operations should be parallelizable with other components, consider creating a dedicated
DeviceHandlerforChamberView.🔧 Minor fix for unused variable
- width, height, img_str = self.parent._device_handler.FetchCameraImage(0) + width, height, _img_str = self.parent._device_handler.FetchCameraImage(0)
1861-1861: ChamberPressure and Light use parent's connection.Similar to
ChamberView, these components useself.parent._device_handlerrather than creating independent connections. This is consistent within the "simpler" components but limits parallelism.Please confirm this is intentional. The PR description mentions "one connection per component", but
ChamberView,ChamberPressure, andLightshare the parent SEM's connection.
654-654: Scanner correctly creates its own connection with device type.The Scanner properly instantiates an independent
DeviceHandlerwith the specifieddevice_type(ELECTRON or ION), enabling parallel operation for SEM and FIB scanners.
tmoerkerken
force-pushed
the
multiple-sharksem-connections
branch
from
47149e6 to
f7a78c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/driver/tescan.py (1)
196-207: Bug: Missing early return when FIB method has no equivalent.When
FIB_NAME_MAP.get(name, name)returnsNone(for methods likeScGetBlanker,ScSetBlanker, etc.), the code logs a warning but continues togetattr(self.device, None), which will raise aTypeErrorrather than handling the unsupported method gracefully.🐛 Proposed fix
elif self.device_type == DeviceType.ION: fib_name = FIB_NAME_MAP.get(name, name) if not fib_name: logging.warning(f"No known FIB equivalent for '{name}'") + return lambda *args, **kwargs: None # No-op for unsupported FIB methods try: func = getattr(self.device, fib_name) except AttributeError: raise AttributeError(f"FIB equivalent method '{fib_name}' for '{name}' does not exist in the API")Alternatively, raise a descriptive exception if the caller should know the method isn't supported:
elif self.device_type == DeviceType.ION: fib_name = FIB_NAME_MAP.get(name, name) if not fib_name: - logging.warning(f"No known FIB equivalent for '{name}'") + raise NotImplementedError(f"No known FIB equivalent for '{name}'") try:
🤖 Fix all issues with AI agents
In @src/odemis/driver/tescan.py:
- Around line 1483-1495: The debug messages in the axis handling block log
tescan_pos after it’s been set to None, so change the code in the
move-preparation logic (the branches handling axes "x","y","z" and "rz","rx") to
capture the computed requested position into a temporary variable (e.g.,
requested_pos = tescan_pos) before checking MIN_MOVE_SIZE_*; if the move is
dropped set tescan_pos = None but use requested_pos in the logging call so the
debug message shows the original calculated position (refer to variables
tescan_pos, requested_pos, current_pos, pos and constants MIN_MOVE_SIZE_LIN_MM /
MIN_MOVE_SIZE_ROT_DEG).
🧹 Nitpick comments (4)
src/odemis/driver/tescan.py (4)
210-215: Consider logging exception details in__del__for debugging.While catching broad exceptions in
__del__is acceptable (destructors should fail silently), logging the exception details would aid debugging without changing behavior.♻️ Suggested improvement
def __del__(self): try: self.Disconnect() except Exception: # Destructors should fail silently - logging.debug("Could not disconnect from TESCAN server") + logging.debug("Could not disconnect from TESCAN server", exc_info=True)
585-586: Prefer explicitis Nonecheck for clarity.The
if not self._device_handlerworks but is less explicit thanif self._device_handler is None. This improves readability and matches the intended semantics of checking for "already terminated."♻️ Suggested improvement
- if not self._device_handler: + if self._device_handler is None: return
1697-1703: Consider: ChamberView shares parent's DeviceHandler.Unlike Scanner and Stage which create independent connections, ChamberView uses
parent._device_handler. This design choice means camera operations share the connection/lock with the parent SEM component.Also,
img_stron line 1702 is fetched but unused (only width/height are used for resolution). Per static analysis hint, prefix with underscore.♻️ Suggested fix for unused variable
# Get a first image to determine the resolution - width, height, img_str = self.parent._device_handler.FetchCameraImage(0) + width, height, _img_str = self.parent._device_handler.FetchCameraImage(0)
124-127: Add documentation clarifying the connection budget for integrators.With the current implementation, a full SEM configuration creates up to 5 independent DeviceHandler connections: SEM (1) + Scanner ELECTRON (1) + Scanner ION (1) + Stage (1) + EbeamFocus (1). Other components (Detector, ChamberView, ChamberPressure, Light) reuse parent connections. Document this connection model so integrators understand the resource budget when combining this driver with external sharksem connections or other Tescan API clients.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/driver/tescan.pysrc/odemis/driver/test/tescan_test.py
💤 Files with no reviewable changes (1)
- src/odemis/driver/test/tescan_test.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:13.022Z
Learning: In src/odemis/driver/tescan.py, each DeviceHandler instance creates an independent connection to the Tescan API with its own sem.Sem() device object and per-connection lock. Multiple connections are independent and do not require cross-connection synchronization.
📚 Learning: 2025-12-17T19:02:06.675Z
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:06.675Z
Learning: In src/odemis/driver/tescan.py, ensure that each DeviceHandler instance creates its own independent Tescan API connection using a separate sem.Sem() device object and a per-instance lock. Do not share a single connection or lock across multiple DeviceHandler instances. This implies that multiple concurrent connections are independent and do not require cross-connection synchronization. Reviewers should verify that connections and locks are instance-scoped and that there is no global or class-level shared device object or lock for Tescan interactions.
Applied to files:
src/odemis/driver/tescan.py
🧬 Code graph analysis (1)
src/odemis/driver/tescan.py (1)
src/odemis/model/_components.py (2)
HwError(37-43)model(570-571)
🪛 Ruff (0.14.10)
src/odemis/driver/tescan.py
213-213: Do not catch blind exception: Exception
(BLE001)
1702-1702: Unpacked variable img_str is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (6)
src/odemis/driver/tescan.py (6)
142-163: LGTM on connection setup and per-instance locking.The connection establishment, socket configuration, and per-instance lock creation correctly implement independent connections as intended. Each
DeviceHandlergets its ownsem.Sem()device object and lock.Minor observation: The lock is stored in two places (
self.device._sharksem_lockandself._call_lock) but since eachDeviceHandlerowns its owndevice, storing onself.deviceis redundant. Consider simplifying to justself._call_lock = threading.Lock().
655-655: LGTM: Scanner creates independent DeviceHandler.Each Scanner instance correctly creates its own independent connection via
DeviceHandlerwith the appropriatedevice_type. This aligns with the PR objective of per-component connections.
1149-1153: LGTM: Detector shares Scanner's DeviceHandler.The Detector appropriately shares its Scanner's
DeviceHandlersince they operate together during acquisition. The shared lock ensures synchronized access for detector/scanner operations on the same connection.
1590-1590: LGTM: EbeamFocus creates independent DeviceHandler.EbeamFocus correctly creates its own independent connection, allowing focus operations to proceed without blocking other components.
1871-1872: ChamberPressure shares parent's DeviceHandler.Like ChamberView, this component shares the parent SEM's connection rather than creating its own. This is acceptable for pressure operations which are infrequent.
1948-1948: LGTM: Light uses parent's DeviceHandler appropriately.Simple LED control operations appropriately share the parent's connection.
pieleric
left a comment
pieleric
left a comment
There was a problem hiding this comment.
Looks good to me. Please do go through the last 2 remaining comments from CodeRabbit, and although they are about code which was there previously, it would be good to fix them at the same time.
tmoerkerken
force-pushed
the
multiple-sharksem-connections
branch
from
f7a78c1 to
de6a433
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/driver/tescan.py (1)
196-203: Logic error when FIB method is explicitly unsupported.When
FIB_NAME_MAPreturnsNone(e.g., forScGetBlanker,SetWD), the code logs a warning but then proceeds to callgetattr(self.device, None), which raisesTypeError: attribute name must be string, not 'NoneType'instead of a meaningful error.Proposed fix
elif self.device_type == DeviceType.ION: fib_name = FIB_NAME_MAP.get(name, name) if not fib_name: logging.warning(f"No known FIB equivalent for '{name}'") + raise AttributeError(f"Method '{name}' is not supported for FIB/ION devices") try: func = getattr(self.device, fib_name) except AttributeError: raise AttributeError(f"FIB equivalent method '{fib_name}' for '{name}' does not exist in the API")
🧹 Nitpick comments (3)
src/odemis/driver/tescan.py (3)
162-163: Redundant lock storage.The lock is stored in both
self.device._sharksem_lockandself._call_lock, but onlyself._call_lockappears to be used (in__getattr__). Consider removing the unusedself.device._sharksem_lockassignment unless it's needed for external access.Suggested simplification
- self.device._sharksem_lock = threading.Lock() - self._call_lock = self.device._sharksem_lock + self._call_lock = threading.Lock()
210-215: Destructor may fail silently due to partially collected state.The
__del__method relies onself.Disconnect()which goes through__getattr__, requiringself.deviceandself._call_lockto still exist. During garbage collection, these may already beNoneor missing. Consider guarding against this:More robust destructor
def __del__(self): try: - self.Disconnect() + # Access device directly to avoid __getattr__ during GC + if hasattr(self, 'device') and self.device is not None: + self.device.Disconnect() except Exception: # Destructors should fail silently logging.debug("Could not disconnect from TESCAN server")
1702-1702: Unused variableimg_strshould be prefixed with underscore.The
img_strvariable is unpacked but not used in this initialization context. Per Python convention, prefix it with_to indicate it's intentionally discarded.- width, height, img_str = self.parent._device_handler.FetchCameraImage(0) + width, height, _img_str = self.parent._device_handler.FetchCameraImage(0)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/driver/tescan.pysrc/odemis/driver/test/tescan_test.py
💤 Files with no reviewable changes (1)
- src/odemis/driver/test/tescan_test.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:13.022Z
Learning: In src/odemis/driver/tescan.py, each DeviceHandler instance creates an independent connection to the Tescan API with its own sem.Sem() device object and per-connection lock. Multiple connections are independent and do not require cross-connection synchronization.
📚 Learning: 2025-12-17T19:02:06.675Z
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3297
File: src/odemis/driver/tescan.py:119-162
Timestamp: 2025-12-17T19:02:06.675Z
Learning: In src/odemis/driver/tescan.py, ensure that each DeviceHandler instance creates its own independent Tescan API connection using a separate sem.Sem() device object and a per-instance lock. Do not share a single connection or lock across multiple DeviceHandler instances. This implies that multiple concurrent connections are independent and do not require cross-connection synchronization. Reviewers should verify that connections and locks are instance-scoped and that there is no global or class-level shared device object or lock for Tescan interactions.
Applied to files:
src/odemis/driver/tescan.py
📚 Learning: 2026-01-12T12:37:35.155Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:35.155Z
Learning: Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources. Do not remove or restructure this behavior in refactors if it serves debugging consistency; document the debugging purpose with comments and ensure the behavior remains consistent across modules (e.g., acquisition/localization loops such as src/odemis/acq/align/z_localization.py).
Applied to files:
src/odemis/driver/tescan.py
🧬 Code graph analysis (1)
src/odemis/driver/tescan.py (2)
src/odemis/model/_components.py (2)
HwError(37-43)model(570-571)src/odemis/driver/phenom.py (6)
GetWD(1407-1408)GetWD(1514-1515)GetWD(1730-1733)SetWD(1411-1412)SetWD(1517-1518)SetWD(1735-1737)
🪛 Ruff (0.14.10)
src/odemis/driver/tescan.py
213-213: Do not catch blind exception: Exception
(BLE001)
1702-1702: Unpacked variable img_str is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (7)
src/odemis/driver/tescan.py (7)
239-256: LGTM!The SEM class properly initializes its own
DeviceHandlerfor metadata retrieval and correctly propagates connection parameters to child components. The metadata fetching viaTcpGetDevice,TcpGetSWVersion, andTcpGetVersionis appropriately done through the handler.
655-655: LGTM!Each Scanner correctly creates its own independent
DeviceHandlerwith the appropriatedevice_type. This aligns with the design goal of per-component connections. Based on learnings, multiple connections are independent and don't require cross-connection synchronization.
1149-1157: LGTM!The Detector correctly shares its Scanner's
DeviceHandlersince they operate on the same channel and need coordinated access. This ensures proper synchronization during acquisition operations.
1333-1339: LGTM!Stage correctly creates its own independent
DeviceHandlerfor stage control operations. The initialization properly retrieves axis limits and motorization status through the handler.
1590-1590: LGTM!EbeamFocus correctly creates its own independent
DeviceHandlerfor focus control. The working distance operations (GetWD/SetWD) are properly routed through the handler.
1697-1703: Inconsistent connection handling: ChamberView shares parent's DeviceHandler.While Scanner, Stage, and EbeamFocus create their own
DeviceHandlerinstances (per the PR objective of "one connection per component"), ChamberView, ChamberPressure, and Light shareself.parent._device_handler. This creates potential contention when these components operate concurrently with the parent's metadata operations.Is this intentional? If these operations are infrequent or always serialized with
parent._acq_progress_lock, sharing may be acceptable. Otherwise, consider creating independent handlers for consistency.
369-369: LGTM!The acquisition management correctly routes
ScStopScan()calls through the detector's device handler, which is shared with its scanner. This maintains proper coordination during acquisition start/stop operations.
3 participants
We found that the Tescan API allows for 8 simultaneous connections at once. This PR makes sure we utilize multiple parallel connections. Changes introduced:
Note that we might want to optimize it a bit later, since we now use quite some connections, and there might be external systems requiring a sharksem connection as well.