Skip to content

[feat] odemis-park-mirror: allow to cancel when using the GUI mode #3380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pieleric merged 1 commit into from
Mar 6, 2026
Merged

[feat] odemis-park-mirror: allow to cancel when using the GUI mode #3380

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 112 additions & 65 deletions util/odemis-park-mirror
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
#!/usr/bin/env python3
# -*- coding: utf-8 -*-
# Attempt to park the mirror of the SPARCv2, even if no backend is running
"""
Created on October 2015

@author: Éric Piel

Copyright © 2015 Éric Piel, Delmic
Copyright © 2015-2026 Éric Piel, Delmic

Odemis is free software: you can redistribute it and/or modify it under the terms
of the GNU General Public License version 2 as published by the Free Software
Expand All @@ -25,8 +24,9 @@ import logging
import os
import sys
import threading
import time
from typing import Optional
from concurrent.futures import CancelledError, Future
from dataclasses import dataclass
from typing import Optional, Tuple

import Pyro4
import wx
Expand All @@ -52,80 +52,104 @@ if TEST_NOHW:
REDUX_KWARGS["address"] = None


def park(mirror):
@dataclass
class FutureHolder:
"""Mutable container for sharing a Future between threads, enabling cancellation."""
future: Optional[Future] = None


@dataclass
class ParkState(FutureHolder):
"""Shared state for the GUI parking operation: current future and any exception raised."""
error: Optional[Exception] = None


def park(mirror: model.Actuator, future_holder: Optional[FutureHolder] = None) -> None:
"""

:param mirror: The mirror with l and s axes.
:param future_holder: To pass back the current move futures, and allow cancellation from the
caller (e.g. GUI thread).
:raise: CancelledError if the move was cancelled, or other exceptions if the move failed.
"""
# Need to park in two moves: first S, then L
f = mirror.reference({"s"})
if future_holder is not None:
future_holder.future = f
logging.info("Parking the mirror...")
try:
logging.debug("Moving S axis")
f.result()
f.result() # Can raise CancelledError if cancelled, or other exceptions if move failed
except KeyboardInterrupt:
f.cancel()
logging.warning("Cancelled parking move")
raise
raise CancelledError()

Comment thread
pieleric marked this conversation as resolved.
f = mirror.reference({"l"})
if future_holder is not None:
future_holder.future = f
try:
logging.debug("Moving L axis")
f.result()
except KeyboardInterrupt:
f.cancel()
logging.warning("Cancelled parking move")
raise
raise CancelledError()

Comment on lines +67 to 98
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Minor race window between creating the new future and storing it in future_holder.

If the user presses Cancel between mirror.reference({"l"}) (Line 81) and the assignment on Line 83, _on_cancel will cancel the already-completed first future (a no-op), and the second move will proceed uninterrupted. The cancel press is silently lost.

This is a narrow window with a safe fallback (the move just completes), so it's unlikely to matter in practice, but worth being aware of.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@util/odemis-park-mirror` around lines 67 - 91, There’s a tiny race between
mirror.reference(...) and assigning future_holder.future so a cancel can slip
past; fix by giving FutureHolder a thread-safe API (e.g., add methods
set_future(self, f: Future) and cancel(self)) that use a lock and a cancelled
flag: implement set_future to store the future and immediately cancel it if
cancelled==True, and implement cancel() to flip cancelled and cancel any stored
future; then in park() replace direct assignments to future_holder.future with
future_holder.set_future(f) and ensure _on_cancel calls future_holder.cancel().

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 26, 2026

Choose a reason for hiding this comment

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

@nandishjpatel You're absolutely right that once CancelledError() is raised, execution stops and the second move won't be attempted.

The specific scenario CodeRabbit flagged is narrower: if Cancel is pressed in the tiny window between creating the new future (f = mirror.reference({"l"}) on Line 81) and storing it (future_holder.future = f on Line 83), the _on_cancel handler would cancel the old (first) future instead of the new one, and the second move would briefly proceed without being cancellable.

That said, this window is microseconds-wide and the worst outcome is just that the second move completes anyway—so it's extremely unlikely to matter in practice. Your implementation handles the common cases correctly!


def park_via_backend():
def get_mirror() -> Tuple[model.Actuator, bool]:
"""
Try to use the backend to park the stage
raise:
CommunicationError if no backend present
LookupError: backend is present but doesn't have mirror
IOError: if move failed
"""
mirror = model.getComponent(role="mirror")
logging.debug("Using the backend to park the mirror")
park(mirror)

Get the mirror component, either from the backend or directly from the hardware.

def park_direct():
:return:
mirror: The mirror component
should_terminate: a boolean indicating whether the component should be terminated when not used
anymore by the caller (False if it was obtained from the backend)
:raises: Various exceptions if the mirror cannot be accessed
"""
Try to directly connect to the TMCM board and park the mirror
"""
mirror = tmcm.TMCLController("Mirror stage", "mirror", **REDUX_KWARGS)
logging.info("Connected to %s", mirror.hwVersion)

try:
park(mirror)
finally:
mirror.terminate()
mirror = model.getComponent(role="mirror")
logging.debug("Using the backend to park the mirror")
return mirror, False # False indicates we got it from the backend
except (Pyro4.errors.CommunicationError, LookupError):
logging.info("Failed to access the backend, will try directly")
mirror = tmcm.TMCLController("Mirror stage", "mirror", **REDUX_KWARGS)
logging.info("Connected to %s", mirror.hwVersion)
return mirror, True # True indicates we got a direct connection


def do_park_mirror() -> None:
def do_park_mirror(future_holder: Optional[FutureHolder] = None) -> None:
"""
Execute the mirror parking operation.

:raises: Various exceptions from park_via_backend() or park_direct()
:param future_holder: Optional FutureHolder to store the current future,
allowing external cancellation.
:raises: Various exceptions from park()
"""
mirror, must_terminate = get_mirror()
try:
park_via_backend()
except (Pyro4.errors.CommunicationError, IOError, LookupError):
logging.info("Failed to access the backend, will try directly")
park_direct()
park(mirror, future_holder)
finally:
if must_terminate:
mirror.terminate()


class ProgressDialog(wx.Dialog):
"""
Dialog showing an indeterminate progress bar during mirror parking operation.
"""

def __init__(self, parent: Optional[wx.Window] = None):
def __init__(self, parent: Optional[wx.Window] = None, future_holder: Optional[FutureHolder] = None):
"""
Initialize the progress dialog.

:param parent: Parent window (None for top-level)
:param future_holder: Optional FutureHolder to store the current future,
allowing cancellation from the Cancel button.
"""
super().__init__(parent, title="Parking Mirror",
style=wx.DEFAULT_DIALOG_STYLE)
style=wx.CAPTION) # No close button, so user can't hide the progress bar
self._future_holder = future_holder

# Create UI elements
panel = wx.Panel(self)
Expand All @@ -139,6 +163,12 @@ class ProgressDialog(wx.Dialog):
self.gauge = wx.Gauge(panel, range=100, size=(300, 25))
sizer.Add(self.gauge, 0, wx.ALL | wx.EXPAND, 10)

# Cancel button
Comment thread
pieleric marked this conversation as resolved.
if self._future_holder is not None:
cancel_btn = wx.Button(panel, wx.ID_CANCEL, "Cancel")
sizer.Add(cancel_btn, 0, wx.ALL | wx.ALIGN_CENTER, 10)
cancel_btn.Bind(wx.EVT_BUTTON, self._on_cancel)

panel.SetSizer(sizer)
sizer.Fit(self)
self.Centre()
Expand All @@ -152,8 +182,20 @@ class ProgressDialog(wx.Dialog):
"""Update the progress gauge to show activity."""
self.gauge.Pulse()

def _on_cancel(self, event: wx.Event) -> None:
"""Cancel the ongoing parking move and close the dialog."""
if self._future_holder.future:
self._future_holder.future.cancel()
else:
return # No future to cancel, just ignore the cancel request
logging.warning("Cancelled parking move")
self.timer.Stop()
self.EndModal(wx.ID_CANCEL)

def close_dialog(self) -> None:
"""Stop the timer and close the dialog."""
if not self.IsShown():
return # Already closed
self.timer.Stop()
self.EndModal(wx.ID_OK)

Expand All @@ -176,54 +218,63 @@ def park_mirror_with_gui() -> int:

if result != wx.YES:
logging.info("User cancelled mirror parking")
app.Destroy()
return 1

# Create progress dialog
dlg = ProgressDialog()
# Shared state
state = ParkState() # holds the current future (for cancellation) and any exception raised

# Parking result
error = [None] # Use list to allow modification in thread
# Create progress dialog
dlg = ProgressDialog(future_holder=state)

def park_thread():
"""Thread function to park the mirror."""
try:
do_park_mirror()
do_park_mirror(state)
except CancelledError as e:
state.error = e
except Exception as e:
error[0] = e
logging.exception("Unexpected error while parking mirror")
state.error = e
finally:
# Close dialog on main thread
# Close dialog on main thread, if not already closed by Cancel
wx.CallAfter(dlg.close_dialog)

# Start parking in background thread
thread = threading.Thread(target=park_thread, daemon=True)
thread.start()

# Show modal dialog (blocks until closed)
dlg.ShowModal()
# Show modal dialog (blocks until closed by park_thread or Cancel button)
modal_result = dlg.ShowModal()
dlg.Destroy()

# Wait for thread to complete
thread.join(timeout=2.0)
Comment thread
pieleric marked this conversation as resolved.
if thread.is_alive():
logging.warning("Parking thread is still running after dialog closed.")

# Check for errors
if error[0]:
exc = error[0]
logging.exception("Unexpected error while performing action.")
# Determine outcome
ex = state.error
if modal_result == wx.ID_CANCEL or isinstance(ex, CancelledError):
wx.MessageBox(
"Mirror parking was cancelled.",
"Cancelled",
wx.OK | wx.ICON_INFORMATION
)
ret = 1
elif ex:
wx.MessageBox(
f"Error while attempting to park the mirror: {str(exc)}",
f"Error while attempting to park the mirror: {str(ex)}",
"Error",
wx.OK | wx.ICON_ERROR
)
return 130

# Success
wx.MessageBox(
"Mirror has been successfully parked.",
"Success",
wx.OK | wx.ICON_INFORMATION
)
ret = 130
else:
# No need for a success window, the user does see the mirror has moved.
ret = 0

return 0
app.Destroy()
return ret


def main(args):
Expand Down Expand Up @@ -257,12 +308,8 @@ def main(args):
if options.no_gui:
# CLI mode - original behavior
try:
try:
park_via_backend()
except (Pyro4.errors.CommunicationError, IOError, LookupError):
logging.info("Failed to access the backend, will try directly")
park_direct()
except KeyboardInterrupt:
do_park_mirror()
except (KeyboardInterrupt, CancelledError):
logging.info("Interrupted before the end of the execution")
return 1
except ValueError as exp:
Expand Down