Skip to content

Conversation

@tmoerkerken
Copy link
Contributor

@tmoerkerken tmoerkerken commented Jan 23, 2026

Adds shutter control for Tescan. The posture manager is updated to make use of it:

  • FM transition (pre-move): Retract shutter → prevents collisions
  • MILLING transition (pre-move): Protect shutter → prevents FIB damage
  • SEM transition (post-move): Verify focus + retract shutter → unobstructed imaging

This was tested on an Essence system at EMBL

@coderabbitai
Copy link

coderabbitai bot commented Jan 23, 2026

📝 Walkthrough

Walkthrough

Adds shutter control support to the Tescan SEM driver and surfaces it as a public VA. Scanner discovers a shutter ID, records the original mode, exposes self.shutter (VAEnumerated with choices True/False/None) and implements _set_shutter_position and _get_shutter_id_by_type. _set_shutter_position toggles auto/manual via ShutterSetMode, sets position via ShutterSet, and waits up to 20s for hardware convergence. _pollVAs reads shutter mode/position and updates the VA. Termination attempts to restore the original shutter mode. Tests and simulator configuration entries and posture-manager integrations were added.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant App as Application
    participant VA as Shutter VA
    participant Scanner as Scanner Driver
    participant API as Tescan API

    User->>App: request shutter change
    App->>VA: set shutter.value = True/False/None
    VA->>Scanner: _set_shutter_position(value)
    
    alt value is None (auto)
        Scanner->>API: ShutterSetMode(shutter_id, 1)
        API-->>Scanner: ok / error
    else value is True or False (manual)
        Scanner->>API: ShutterSetMode(shutter_id, 0)
        API-->>Scanner: ok / error
        Scanner->>API: ShutterSet(shutter_id, pos)
        API-->>Scanner: ok / error
        loop until target or timeout (<=20s)
            Scanner->>API: ShutterGetMode(shutter_id) / ShutterGet(shutter_id)
            API-->>Scanner: mode/pos
        end
    end

    Scanner-->>VA: update shutter.value
    VA-->>App: reflect new state
Loading
sequenceDiagram
    participant Poller as Polling Thread
    participant Scanner as Scanner Driver
    participant API as Tescan API
    participant VA as Shutter VA

    Poller->>Scanner: _pollVAs()
    Scanner->>API: ShutterGetMode(shutter_id)
    API-->>Scanner: mode (0=manual,1=auto)
    
    alt mode == 1 (auto)
        Scanner->>VA: new_value = None
    else mode == 0 (manual)
        Scanner->>API: ShutterGet(shutter_id)
        API-->>Scanner: pos (0=protecting,1=retracted)
        alt pos == 0
            Scanner->>VA: new_value = True
        else pos == 1
            Scanner->>VA: new_value = False
        end
    end

    Scanner->>VA: update and notify if changed
    VA-->>Poller: state reflected
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: adding shutter control functionality for Tescan, which matches the core feature implemented across multiple files in the changeset.
Description check ✅ Passed The pull request description accurately describes the changeset, detailing shutter control implementation for Tescan with specific transitions (FM, MILLING, SEM) and their behaviors.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/odemis/driver/tescan.py`:
- Around line 762-782: The code reads the current shutter mode but never stores
it, so update the initialization block to save the device's mode into
self._original_shutter_mode after calling ShutterGetMode(SEM_SHUTTER_ID) (only
when the read succeeds) so the termination/restoration logic can detect and
restore the original mode; keep the existing conversion to va_value and creation
of model.VAEnumerated (with setter _set_shutter_position) and preserve the
try/except semantics, only assign self._original_shutter_mode = shutter_mode
before creating the VAEnumerated.
🧹 Nitpick comments (3)
src/odemis/driver/test/tescan_test.py (1)

601-616: Test should restore original shutter value.

Following the pattern of other hardware tests in this file (e.g., test_blanker, test_external), the test should save the original shutter value and restore it at the end to avoid leaving the hardware in an unexpected state for subsequent tests.

Proposed fix
 def test_shutter_setter(self):
     """Test that shutter setter can be called and polling updates the value."""
+    # Save original value
+    orig_shutter = self.scanner.shutter.value
+
     # Set shutter to retracted
     self.scanner.shutter.value = False
     time.sleep(15)  # Wait for physically moving shutter and polling
     self.assertEqual(self.scanner.shutter.value, False)

     # Set shutter to protecting
     self.scanner.shutter.value = True
     time.sleep(15)  # Wait for physically moving shutter and polling
     self.assertEqual(self.scanner.shutter.value, True)

     # Set shutter to auto
     self.scanner.shutter.value = None
     time.sleep(5)  # Only wait for next polling cycle
     self.assertEqual(self.scanner.shutter.value, None)
+
+    # Reset to original value
+    self.scanner.shutter.value = orig_shutter
+    time.sleep(15 if orig_shutter is not None else 5)
+    self.assertEqual(self.scanner.shutter.value, orig_shutter)
src/odemis/driver/tescan.py (2)

993-1017: Consider adding acquisition lock for thread safety.

Other setter methods that interact with the device (e.g., _setBlanker, _setVoltage, _setPC) acquire self.parent._acq_progress_lock to prevent conflicts during acquisition. This setter should follow the same pattern for consistency and thread safety.

Proposed fix
 def _set_shutter_position(self, value: Union[bool, None]) -> Union[bool, None]:
     """
     Set shutter position to protecting, retracted, or auto mode.

     :param value: True (protecting), False (retracted), or None (auto)
     :return: The set value
     """
-    if value is None:
-        # Switch to auto mode
-        result = self._device_handler.ShutterSetMode(SEM_SHUTTER_ID, 1)
-        if result != 0:
-            raise HwError(f"ShutterSetMode failed: {result}")
-    else:
-        # Manual mode: ensure we're in manual mode first
-        result = self._device_handler.ShutterSetMode(SEM_SHUTTER_ID, 0)
-        if result != 0:
-            raise HwError(f"ShutterSetMode failed: {result}")
-
-        # Set position: True (protecting) → 0 (closed), False (retracted) → 1 (open)
-        shutter_pos = 0 if value else 1
-        result = self._device_handler.ShutterSet(SEM_SHUTTER_ID, shutter_pos)
-        if result != 0:
-            raise HwError(f"ShutterSet failed: {result}")
+    with self.parent._acq_progress_lock:
+        if value is None:
+            # Switch to auto mode
+            result = self._device_handler.ShutterSetMode(SEM_SHUTTER_ID, 1)
+            if result != 0:
+                raise HwError(f"ShutterSetMode failed: {result}")
+        else:
+            # Manual mode: ensure we're in manual mode first
+            result = self._device_handler.ShutterSetMode(SEM_SHUTTER_ID, 0)
+            if result != 0:
+                raise HwError(f"ShutterSetMode failed: {result}")
+
+            # Set position: True (protecting) → 0 (closed), False (retracted) → 1 (open)
+            shutter_pos = 0 if value else 1
+            result = self._device_handler.ShutterSet(SEM_SHUTTER_ID, shutter_pos)
+            if result != 0:
+                raise HwError(f"ShutterSet failed: {result}")

     return value

1196-1203: Use logging.exception for better error diagnostics.

When catching an exception, logging.exception automatically includes the traceback, which aids in debugging hardware issues.

Proposed fix
         # Restore original shutter mode if it was initialized
         if hasattr(self, '_original_shutter_mode') and self._original_shutter_mode is not None:
             try:
                 self._device_handler.ShutterSetMode(SEM_SHUTTER_ID, self._original_shutter_mode)
                 logging.info(f"Restored shutter mode to {self._original_shutter_mode}")
             except Exception as e:
-                logging.error(f"Failed to restore shutter mode: {e}")
+                logging.exception(f"Failed to restore shutter mode: {e}")

@tmoerkerken tmoerkerken force-pushed the tescan-shutter-control branch 3 times, most recently from e93cefa to d5e428c Compare January 23, 2026 10:25
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/odemis/acq/move.py`:
- Around line 2077-2087: The code closes the shutter when transitioning into
MILLING (setting self.shutter.value = True) but never re-opens it when
transitioning back to SEM imaging, which can leave the SEM beam blocked; update
the transition logic that checks target/current_posture (the same block handling
FM_IMAGING and MILLING) to also retract the shutter (set self.shutter.value =
False) when target == SEM_IMAGING and current_posture != SEM_IMAGING and
self.shutter is not None, or alternatively store and restore the previous
shutter state when leaving MILLING so the shutter is reopened for SEM imaging;
modify the code around the existing checks for FM_IMAGING and MILLING
(referencing target, current_posture, and self.shutter.value) to implement this
complementary retraction/restoration.
♻️ Duplicate comments (1)
src/odemis/driver/tescan.py (1)

763-782: Restore logic won’t run because original mode is never captured.

_original_shutter_mode stays None, so the terminate block never restores the previous mode. Capture the initial mode right after ShutterGetMode.

🔧 Proposed fix
             # Read current position and mode to create VA
             shutter_mode = self._device_handler.ShutterGetMode(SEM_SHUTTER_ID)
+            self._original_shutter_mode = shutter_mode  # Save for restoration on termination
             shutter_pos = self._device_handler.ShutterGet(SEM_SHUTTER_ID)

Also applies to: 1219-1225

🧹 Nitpick comments (1)
src/odemis/driver/tescan.py (1)

993-1037: Consider narrowing the long-held acquisition lock while waiting for shutter movement.

Holding _acq_progress_lock for up to 20s can block acquisitions and other scanner operations. A dedicated shutter lock (also used by the polling path) would keep shutter state consistent without stalling unrelated work.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/odemis/driver/tescan.py`:
- Around line 1009-1011: The error message raised after calling
_device_handler.ShutterSetMode(SEM_SHUTTER_ID, 1) is missing the subject; update
the HwError to include what failed (e.g., "Failed to set shutter mode") and
include the SEM_SHUTTER_ID and the result code for context (e.g., "Failed to set
shutter mode for SEM_SHUTTER_ID {SEM_SHUTTER_ID}: {result}"), so replace the
current message in the block that calls ShutterSetMode with a clear descriptive
message containing those identifiers.
♻️ Duplicate comments (1)
src/odemis/acq/move.py (1)

2077-2081: Re-open shutter when leaving MILLING to avoid SEM beam staying blocked.

The shutter is closed when entering MILLING (self.shutter.value = True), but there is no complementary retraction when transitioning away from MILLING (e.g., to SEM_IMAGING or FM_IMAGING). This could leave the SEM shutter in the "protecting" state and block SEM imaging after a MILLING → SEM transition.

Consider retracting the shutter when leaving MILLING:

🔧 Suggested fix
                 # Protect shutter when transitioning to MILLING to prevent damage from FIB
                 # TODO: later extend for FIB_IMAGING
                 if target == MILLING and current_posture != MILLING and self.shutter is not None:
                     logging.info("Protecting shutter before milling transition")
                     self.shutter.value = True  # True = protecting (closed)
+
+                # Retract shutter when leaving MILLING (restore for SEM imaging)
+                if current_posture == MILLING and target != MILLING and self.shutter is not None:
+                    logging.info("Retracting shutter after milling transition")
+                    self.shutter.value = False  # False = retracted (open)
🧹 Nitpick comments (1)
src/odemis/driver/tescan.py (1)

1223-1226: Use logging.exception instead of logging.error to preserve traceback.

When logging inside an except block, logging.exception automatically includes the traceback, which aids debugging.

🔧 Suggested fix
             except Exception as e:
-                logging.error(f"Failed to restore shutter mode: {e}")
+                logging.exception("Failed to restore shutter mode")

@tmoerkerken tmoerkerken force-pushed the tescan-shutter-control branch from d5e428c to 8fdca93 Compare January 23, 2026 10:43
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/odemis/driver/test/tescan_test.py`:
- Around line 594-616: Add missing return type hints to the two test functions
so they follow the project's typing rules: update the test_shutter_va_exists and
test_shutter_setter function signatures to include a return type of None (i.e.,
def test_shutter_va_exists(self) -> None: and def test_shutter_setter(self) ->
None:), keeping all other logic unchanged.
♻️ Duplicate comments (2)
src/odemis/driver/tescan.py (1)

1008-1011: Clarify the auto‑mode failure message.
The error text omits the action/ID, which makes logs harder to interpret.

🔧 Suggested fix
-                    raise HwError(f"Failed to set the : {result}")
+                    raise HwError(
+                        f"Failed to set shutter mode to auto for SEM_SHUTTER_ID {SEM_SHUTTER_ID}: {result}"
+                    )
src/odemis/acq/move.py (1)

2077-2081: Re‑open shutter when leaving MILLING to avoid SEM beam staying blocked.
Currently the shutter is closed on entry to MILLING but never retracted on return to SEM imaging.

🔧 Suggested minimal fix
-                if target == MILLING and current_posture != MILLING and self.shutter is not None:
+                if target == MILLING and current_posture != MILLING and self.shutter is not None:
                     logging.info("Protecting shutter before milling transition")
                     self.shutter.value = True  # True = protecting (closed)
+
+                if target in (FM_IMAGING, SEM_IMAGING) and current_posture != target and self.shutter is not None:
+                    logging.info("Retracting shutter before transition")
+                    self.shutter.value = False  # False = retracted (open)

@tmoerkerken tmoerkerken force-pushed the tescan-shutter-control branch 2 times, most recently from ec4cb4b to 548cdb2 Compare January 23, 2026 10:59
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/odemis/driver/tescan.py`:
- Around line 1189-1204: Shutter polling currently maps unknown/transient
returns (e.g., moving=2 or None) to False and overwrites the VA; change the
logic in the block using ShutterGetMode, ShutterGet and SEM_SHUTTER_ID so you
only update self.shutter._value and call self.shutter.notify when you have a
stable/definitive position: treat shutter_pos == 0 as True (protecting) and
shutter_pos == 1 as False (retracted), skip any update (do not change
self.shutter._value or call notify) when ShutterGet returns None or transient
values like 2; keep the existing handling for auto mode (ShutterGetMode == 1)
but similarly avoid overwriting if that would replace a stable VA with an
unknown transient.
♻️ Duplicate comments (1)
src/odemis/acq/move.py (1)

2077-2082: FM transition still lacks pre-move shutter retraction.

If the shutter was set to protecting during MILLING, transitioning directly to FM_IMAGING leaves it closed. That contradicts the safety intent (prevent collisions) and can block FM workflows. Consider retracting before FM transitions as well.

🔧 Proposed fix
-                # Protect shutter when transitioning to MILLING to prevent damage from FIB
+                # Retract shutter before FM transition to prevent collisions
+                if target == FM_IMAGING and current_posture != FM_IMAGING and self.shutter is not None:
+                    logging.info("Retracting shutter before FM transition")
+                    self.shutter.value = False  # False = retracted (open)
+
+                # Protect shutter when transitioning to MILLING to prevent damage from FIB
                 # TODO: later extend for FIB_IMAGING
                 if target == MILLING and current_posture != MILLING and self.shutter is not None:
                     logging.info("Protecting shutter before milling transition")
                     self.shutter.value = True  # True = protecting (closed)
🧹 Nitpick comments (2)
src/odemis/driver/tescan.py (1)

763-783: Log stack traces for shutter init failures.

The fallback is fine, but the current warning loses the stack, making root-cause debugging harder. Consider logging the exception with a traceback.

🔧 Suggested tweak
-            except Exception as e:
-                logging.warning(f"Shutter initialization failed: {e}")
+            except Exception:
+                logging.exception("Shutter initialization failed")
src/odemis/acq/test/move_tescan_test.py (1)

167-178: Add return type hint to the new test.

Please add -> None to satisfy the project’s type-hint requirement for Python functions.

🔧 Proposed fix
-    def test_fm_shutter_control(self):
+    def test_fm_shutter_control(self) -> None:

As per coding guidelines, please ensure all functions include type hints.

@tmoerkerken tmoerkerken force-pushed the tescan-shutter-control branch 2 times, most recently from 1cdbf2b to dcd1897 Compare January 26, 2026 16:13
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/acq/move.py (1)

2072-2112: Missing shutter retraction for FM_IMAGING transition.

The PR objectives state that FM transitions should "retract shutter to prevent collisions," and the test at move_tescan_test.py:197 expects shutter.value == False after transitioning to FM_IMAGING. However, there's no code that explicitly sets the shutter to retracted (False) when transitioning to FM_IMAGING.

The current implementation only:

  • Protects shutter (True) before MILLING (lines 2077-2081)
  • Retracts shutter (False) after SEM_IMAGING (lines 2121-2129)
🔧 Suggested fix - add FM_IMAGING shutter handling
                 # Protect shutter when transitioning to MILLING to prevent damage from FIB
                 # TODO: later extend for FIB_IMAGING
                 if target == MILLING and current_posture != MILLING and self.shutter is not None:
                     logging.info("Protecting shutter before milling transition")
                     self.shutter.value = True  # True = protecting (closed)

+                # Retract shutter when transitioning to FM_IMAGING to prevent collisions
+                if target == FM_IMAGING and current_posture != FM_IMAGING and self.shutter is not None:
+                    logging.info("Retracting shutter before FM transition")
+                    self.shutter.value = False  # False = retracted (open)
+
                 if current_posture == MILLING:
🤖 Fix all issues with AI agents
In `@src/odemis/acq/move.py`:
- Around line 2121-2129: Remove the redundant metadata fetch (drop the second
call to self.focus.getMetadata() and reuse the already-fetched
focus_md/focus_deactive values) and change the shutter-safety handling so the
completed move isn't masked by a generic exception: instead of raising a generic
ValueError when isNearPosition(...) fails, either log a warning and leave
self.shutter unchanged (so the move is considered successful) or raise a
dedicated ShutterSafetyError and ensure the surrounding generic except Exception
block does not swallow it (rethrow or handle it separately). Update references
in this block to use the existing focus_md/focus_deactive variables and adjust
the except logic so "Failure to move to %s position." is only logged for real
move failures.

In `@src/odemis/acq/test/move_tfs1_test.py`:
- Around line 52-55: The test method
test_moving_in_grid1_fm_imaging_area_after_loading assumes self.linked_stage is
always present but linked_stage can be None; add a guard at the start of that
test (and any other tests that access self.linked_stage) to skip or return early
when self.linked_stage is None (e.g., call self.skipTest("no linked_stage") or
pytest.skip) and ensure all uses of self.linked_stage.position.value and
self.linked_stage.moveRel() are only executed after that check so AttributeError
is avoided.
♻️ Duplicate comments (2)
src/odemis/driver/test/tescan_test.py (1)

876-898: Add return type hints to test methods.

Per coding guidelines and a previous review comment, the new test methods should include -> None return type annotations.

🔧 Suggested fix
-    def test_shutter_va_exists(self):
+    def test_shutter_va_exists(self) -> None:
         """Test that shutter VA is created on scanner."""
         self.assertTrue(hasattr(self.scanner, 'shutter'), "Shutter VA not created")
         self.assertIn(False, self.scanner.shutter.choices)
         self.assertIn(True, self.scanner.shutter.choices)
         self.assertIn(None, self.scanner.shutter.choices)

-    def test_shutter_setter(self):
+    def test_shutter_setter(self) -> None:
         """Test that shutter setter blocks until position is reached."""
src/odemis/driver/tescan.py (1)

1226-1241: Avoid overwriting shutter VA on transient/unknown states.

ShutterGet() can return None or a moving state (e.g., 2). Mapping these to False can briefly misreport state; skip updates unless the position is stable.

🛠️ Proposed fix
-                            shutter_mode = self._device_handler.ShutterGetMode(self._shutter_id)
-                            if shutter_mode == 1:
-                                # Auto mode
-                                new_shutter = None
-                            else:
-                                # Manual mode: read position
-                                shutter_pos = self._device_handler.ShutterGet(self._shutter_id)
-                                # Convert: 0 → True (protecting), 1 → False (retracted), 2 (moving) → False (worst case)
-                                new_shutter = True if shutter_pos == 0 else False
-
-                            if new_shutter != self.shutter._value:
-                                self.shutter._value = new_shutter
-                                self.shutter.notify(new_shutter)
+                            shutter_mode = self._device_handler.ShutterGetMode(self._shutter_id)
+                            if shutter_mode == 1:
+                                # Auto mode
+                                new_shutter = None
+                            elif shutter_mode == 0:
+                                # Manual mode: read position only when stable
+                                shutter_pos = self._device_handler.ShutterGet(self._shutter_id)
+                                if shutter_pos in (0, 1):
+                                    new_shutter = True if shutter_pos == 0 else False
+                                else:
+                                    new_shutter = self.shutter._value
+                            else:
+                                # Unknown / transient mode; keep current value
+                                new_shutter = self.shutter._value
+
+                            if new_shutter != self.shutter._value:
+                                self.shutter._value = new_shutter
+                                self.shutter.notify(new_shutter)
🧹 Nitpick comments (4)
src/odemis/acq/test/move_tescan_test.py (2)

169-206: Add return type hints to test methods.

Per coding guidelines, all Python functions should include return type hints. The new test class methods are missing -> None return type annotations.

🔧 Suggested fix
     `@classmethod`
-    def setUpClass(cls):
+    def setUpClass(cls) -> None:
         """Set up the test case with Tescan 1 configuration"""
         super().setUpClass()
         cls.stage_bare_md = cls.stage.getMetadata()

-    def setUp(self):
+    def setUp(self) -> None:
         super().setUp()
         # reset stage-bare metadata, so that even if a test modifies it, the next test starts fresh
         self.stage.updateMetadata(self.stage_bare_md)

-    def test_fm_shutter_control(self):
+    def test_fm_shutter_control(self) -> None:
         """Test shutter state changes during FM mode transitions."""

As per coding guidelines, ensure all Python functions include parameter and return type hints.


174-174: Consider annotating mutable class attribute with ClassVar.

Static analysis flags ROTATION_AXES as a mutable class attribute. While the set is never mutated in practice, adding a ClassVar annotation improves type safety and clarity.

+from typing import ClassVar, Set
+
 class TestMeteorTescan1ShutterMove(TestMeteorTFS1Move):
-    ROTATION_AXES = {'rx', 'rz'}
+    ROTATION_AXES: ClassVar[Set[str]] = {'rx', 'rz'}

Note: This applies to the parent class as well (line 43).

src/odemis/driver/tescan.py (2)

765-789: Narrow the shutter init exception handling.

Catching all Exception can hide programming errors; prefer expected exceptions and log the stack trace.

♻️ Suggested adjustment
-            except Exception as e:
-                logging.warning(f"Shutter initialization failed: {e}")
+            except (HwError, ValueError, OSError) as exc:
+                logging.exception("Shutter initialization failed: %s", exc)

1257-1263: Use logging.exception and narrower exceptions for restore.

This improves diagnostics and avoids swallowing unexpected failures.

♻️ Suggested adjustment
-            except Exception as e:
-                logging.error(f"Failed to restore shutter mode: {e}")
+            except (HwError, ValueError, OSError) as exc:
+                logging.exception("Failed to restore shutter mode: %s", exc)

Comment on lines +52 to +55
try:
cls.linked_stage = model.getComponent(role="stage")
except LookupError:
cls.linked_stage = None
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Guard linked_stage usage in test methods that depend on it.

The change to allow linked_stage to be None is good for test flexibility, but test_moving_in_grid1_fm_imaging_area_after_loading (lines 96-104) accesses self.linked_stage.position.value and self.linked_stage.moveRel() without checking if it's None. This will cause AttributeError when linked_stage is unavailable.

Consider adding a guard at the start of dependent test methods:

🔧 Suggested fix
     def test_moving_in_grid1_fm_imaging_area_after_loading(self):
         """Check if the stage moves in the right direction when moving in the fm imaging grid 1 area."""
+        if self.linked_stage is None:
+            self.skipTest("linked_stage not available")
         # move the stage to the loading position
         f = self.posture_manager.cryoSwitchSamplePosition(LOADING)
🤖 Prompt for AI Agents
In `@src/odemis/acq/test/move_tfs1_test.py` around lines 52 - 55, The test method
test_moving_in_grid1_fm_imaging_area_after_loading assumes self.linked_stage is
always present but linked_stage can be None; add a guard at the start of that
test (and any other tests that access self.linked_stage) to skip or return early
when self.linked_stage is None (e.g., call self.skipTest("no linked_stage") or
pytest.skip) and ensure all uses of self.linked_stage.position.value and
self.linked_stage.moveRel() are only executed after that check so AttributeError
is avoided.

@tmoerkerken tmoerkerken force-pushed the tescan-shutter-control branch from dcd1897 to 95bf06f Compare January 27, 2026 14:55
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/odemis/acq/move.py`:
- Around line 2077-2081: The code handles shutter protection for MILLING but
neglects FM_IMAGING, causing tests to expect the shutter retracted when
transitioning to FM_IMAGING; update the transition logic in the move handler
where target and current_posture are checked (same area with MILLING handling)
to set self.shutter.value = False when target == FM_IMAGING and self.shutter is
not None (ensure it mirrors the SEM_IMAGING post-move retraction behavior),
while keeping existing MILLING protection (self.shutter.value = True) and using
the same condition variables (target, current_posture, self.shutter) to locate
and modify the code.
🧹 Nitpick comments (4)
src/odemis/acq/test/move_tescan_test.py (2)

176-185: Add return type hints to class methods.

As per coding guidelines, add return type hints to setUpClass and setUp:

🔧 Suggested fix
     `@classmethod`
-    def setUpClass(cls):
+    def setUpClass(cls) -> None:
         """Set up the test case with Tescan 1 configuration"""
         super().setUpClass()
         cls.stage_bare_md = cls.stage.getMetadata()

-    def setUp(self):
+    def setUp(self) -> None:
         super().setUp()
         # reset stage-bare metadata, so that even if a test modifies it, the next test starts fresh
         self.stage.updateMetadata(self.stage_bare_md)

187-205: Add return type hint to test method.

🔧 Suggested fix
-    def test_fm_shutter_control(self):
+    def test_fm_shutter_control(self) -> None:
         """Test shutter state changes during FM mode transitions."""
src/odemis/driver/tescan.py (2)

847-847: Remove redundant re import.

The re module is already imported at the module level (line 28), so this local import is unnecessary.

♻️ Proposed fix
-        import re

1262-1263: Use logging.exception to preserve stack trace.

Using logging.exception instead of logging.error will include the stack trace, which is helpful for debugging hardware failures.

♻️ Proposed fix
             except Exception as e:
-                logging.error(f"Failed to restore shutter mode: {e}")
+                logging.exception(f"Failed to restore shutter mode: {e}")

@tmoerkerken tmoerkerken force-pushed the tescan-shutter-control branch from 95bf06f to 2453f4e Compare January 27, 2026 16:02
@pieleric pieleric changed the title [feature] Tescan shutter control [MSD-249][feature] Tescan shutter control Jan 27, 2026
@tmoerkerken tmoerkerken marked this pull request as ready for review January 28, 2026 14:12
@tmoerkerken tmoerkerken assigned pieleric and ilyushkin and unassigned pieleric and ilyushkin Jan 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants