[TRPD-29][feat] Show HPDTA warning as user notification - #3542
Conversation
📝 WalkthroughWalkthroughThe Hamamatsu driver now supports photon-counting exposure, integration counts, thresholds, and queue-driven live and synchronized acquisition. A local Sequence Diagram(s)sequenceDiagram
participant StreakCamera
participant HPDTASim
participant ReadoutCamera
StreakCamera->>HPDTASim: Connect to simulator endpoint
ReadoutCamera->>StreakCamera: Start acquisition
StreakCamera->>HPDTASim: Send acquisition command
HPDTASim-->>StreakCamera: Return image notification and data
StreakCamera-->>ReadoutCamera: Publish image metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds user-facing desktop notifications for HPDTA warnings coming from the Hamamatsu streak camera driver, and expands the simulator/test setup to exercise HPDTA/RemoteEx behavior (including photon-counting) without real hardware.
Changes:
- Introduces
get_active_gui_users()andnotify_to_user()helpers to target GUI users and send desktop notifications (systemd-run + notify-send). - Routes RemoteEx warning messages (error code 5) from
hamamatsurx.StreakCamerainto a background notification loop for the active GUI user. - Switches streak camera simulation to use the new
HPDTASim(RemoteEx protocol) and updates/extends Hamamatsu streak camera tests accordingly.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/util/test/driver_test.py | Adds tests for GUI user discovery and desktop notifications. |
| src/odemis/util/driver.py | Adds GUI-user discovery and a helper to send desktop notifications. |
| src/odemis/driver/test/hamamatsurx_test.py | Updates tests to run against a “fake-*” RemoteEx simulator and adds photon-counting acquisition tests. |
| src/odemis/driver/hamamatsurx.py | Adds notification plumbing for warnings, refactors acquisition thread signaling, and introduces HPDTASim. |
| install/linux/usr/share/odemis/sim/sparc2-streakcam-sim.odm.yaml | Switches sim streak camera to hamamatsurx.StreakCamera with fake-* host simulator. |
| install/linux/usr/share/odemis/sim/sparc2-ek-streakcam-sim.odm.yaml | Same simulator switch for the EK sim configuration. |
Suppressed comments (1)
src/odemis/util/test/driver_test.py:107
test_notify_to_usercurrently depends onloginctl,systemd-run, andnotify-sendbeing present and on having an active GUI user; this is not reliable in CI. Mockingsubprocess.runkeeps the unit test deterministic while still validating that the function is invoked.
def test_notify_to_user(self):
users = get_active_gui_users()
first_user = next(iter(users))
# just check it doesn't raise an exception
odemis.util.driver.notify_to_user(first_user, "Test notification", "This is a test notification from the odemis.util.driver module.",
pieleric
changed the title
6c419a6 to
dec38d1
Compare
Instead of relying on a completely different codebase to simulate "a" streakcam (simstreakcam), provide a HPDTA simulator. This way, it's possible to test most of the code of the hamamatsurx, and avoid duplicating every change of the hamamasturx into the simstreakcam.
…ication from the backend The notify2 option doesn't work reliably enough, even when installing the notification-daemon.
dec38d1 to
30d7ed9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (7)
src/odemis/util/driver.py:170
- The notify-send options (--app-name/--icon) are appended after the title/message, but notify-send expects options before positional arguments; this can be interpreted as extra positional args and fail. Build the command with options first, then append title/message.
cmd = [
"systemd-run",
f"--machine={target_user}@.host",
"--user",
"notify-send",
src/odemis/util/test/driver_test.py:101
- get_active_gui_users() is documented to return an empty set on error/no GUI session, so asserting len(users) >= 1 makes this test fail in headless/CI environments.
def test_get_active_gui_users(self):
users = get_active_gui_users()
self.assertIsInstance(users, set)
self.assertGreaterEqual(len(users), 1)
src/odemis/driver/test/hamamatsurx_test.py:861
- This synchronized photon-counting test can take minutes with the current exposure/count settings (est_time=1s, num_images=40, per-image timeout 2*est_time+1). For simulator (TEST_NOHW) runs, use much smaller exposure/count values while still keeping num_images > 19 to exercise the window-limit logic.
num_images = 40
src/odemis/util/driver.py:151
- This new function is missing an explicit return type annotation; elsewhere in this module newly added functions use type hints, so this should be
-> Nonefor consistency.
def notify_to_user(target_user: str, title: str, message: str, app: Optional[str] = None, icon: Optional[str] = None):
src/odemis/util/test/driver_test.py:108
- This test can raise StopIteration when no GUI user is detected, and can also fail on systems without systemd-run/notify-send. It should skip when notifications cannot be delivered in the test environment.
users = get_active_gui_users()
first_user = next(iter(users))
# just check it doesn't raise an exception
odemis.util.driver.notify_to_user(first_user, "Test notification", "This is a test notification from the odemis.util.driver module.",
app="Odemis testing", icon="info")
src/odemis/driver/test/hamamatsurx_test.py:25
- This import appears to be unused in this test module (no other hdf5 references), which can break linting and adds unnecessary dependency loading during tests.
from odemis import model, util
from odemis.dataio import hdf5
from odemis.driver import hamamatsurx
src/odemis/driver/test/hamamatsurx_test.py:825
- In TEST_NOHW (simulator) runs, these photon-counting settings make the test very slow (est_time=pcExposureTimepcIntegrationCounts, then sleeping 2est_time+1 in a loop). Use faster settings and/or fewer images under TEST_NOHW to keep CI runtime reasonable.
This issue also appears on line 861 of the same file.
self.readoutcam.photonCounting.value = True
self.readoutcam.pcExposureTime.value = 25e-3 # s
self.readoutcam.pcIntegrationCounts.value = 100
num_images = 25
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
src/odemis/driver/test/hamamatsurx_test.py (3)
810-813: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not swallow
KeyboardInterruptin the test.The
except KeyboardInterruptblock prints a message and lets the test report success after an interruption. Remove the handler and keep only thefinallyblock, which already restoresphotonCounting.♻️ Proposed change
- except KeyboardInterrupt: - print("Test interrupted by user.") finally: self.readoutcam.photonCounting.value = False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/test/hamamatsurx_test.py` around lines 810 - 813, Remove the KeyboardInterrupt handler surrounding the test cleanup so interruptions propagate and the test does not report success. Keep the finally block that resets self.readoutcam.photonCounting.value to False.
386-387: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the docstring of
test_pc_exposure_time.The docstring repeats the text of
test_exposure_time. It does not state that the test covers the photon-counting exposure VA.♻️ Proposed change
def test_pc_exposure_time(self): - """Test exposure time VA for readout camera.""" + """Test the photon-counting exposure time VA (pcExposureTime) of the readout camera."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/test/hamamatsurx_test.py` around lines 386 - 387, Update the docstring in test_pc_exposure_time to describe testing the photon-counting exposure VA, rather than repeating the readout-camera exposure description from test_exposure_time.
605-621: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated time-list assertion into a helper.
The same four lines appear three times in
test_acq_get_scaling_table. The comment states the value is checked "in the expected range relative to the scale factor", but the assertion only checks that the value is positive. A helper makes the intent explicit and removes the duplication.♻️ Proposed change
+ def _check_time_list(self, img): + """Check the time list metadata is present and ends with a positive value.""" + self.assertIn(model.MD_TIME_LIST, img.metadata) + self.assertIsNotNone(img.metadata[model.MD_TIME_LIST]) + last_value = img.metadata[model.MD_TIME_LIST][-1] + conversion_factor = self.streakunit.get_time_scale_factor() + self.assertGreater(last_value / conversion_factor, 0)Then replace each of the three blocks with
self._check_time_list(img).Also applies to: 640-643
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/test/hamamatsurx_test.py` around lines 605 - 621, In test_acq_get_scaling_table, add a _check_time_list(img) helper that validates the time-list metadata and checks the final value against the scale factor using the intended expected-range assertion, then replace all three duplicated validation blocks with self._check_time_list(img).src/odemis/driver/hamamatsurx.py (6)
3033-3041: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the class-level time-range lists as
ClassVar.Ruff reports RUF012 for these mutable class attributes. Annotating them documents that they are constants shared by all instances.
♻️ Proposed change
- SINGLESWEEP_TIME_RANGES = [ + SINGLESWEEP_TIME_RANGES: ClassVar[List[str]] = [- SYNCHROSCAN_TIME_RANGES = ["1", "2", "3", "4", "5"] + SYNCHROSCAN_TIME_RANGES: ClassVar[List[str]] = ["1", "2", "3", "4", "5"]Add
ClassVarto thetypingimport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/hamamatsurx.py` around lines 3033 - 3041, Annotate the mutable class-level lists SINGLESWEEP_TIME_RANGES and SYNCHROSCAN_TIME_RANGES as ClassVar, and add ClassVar to the existing typing imports in their containing class. Keep the list contents unchanged.Source: Linters/SAST tools
2078-2086: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Optional[str]forini_file.
ini_file: str = Noneis an implicit Optional, which PEP 484 prohibits and Ruff reports as RUF013.Optionalis already imported in this module. Document thevisibleparameter as well.♻️ Proposed change
- def AppStart(self, visible: bool = True, ini_file: str = None, no_dialog: bool = True): + def AppStart(self, visible: bool = True, ini_file: Optional[str] = None, no_dialog: bool = True): """ Start RemoteEx. If the application is already running, it will not do anything. Blocks until the application is started. + :param visible: if True, the HPDTA application window is shown. :param ini_file: (str) path to the INI file for HPDTA, default is HDPTA8.INIAs per coding guidelines: "Always use type hints for function parameters and return types in Python code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/hamamatsurx.py` around lines 2078 - 2086, Update AppStart’s ini_file annotation from str to Optional[str] while keeping its None default, using the existing Optional import. Extend the docstring to document the visible parameter and its behavior.Sources: Coding guidelines, Linters/SAST tools
888-894: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHandle
CMD_STARTexplicitly in the outer command branch.The outer branch chain covers
CMD_SW_TRIGGER,CMD_STOPandCMD_IMG. ACMD_STARTmessage, for example from the binning restart path or a duplicated subscribe, falls into theelsebranch and is logged as "unknown command". The inner flush loop at Line 906 already treatsCMD_STARTas a known, ignorable message. Add the same branch here so the log stays accurate.♻️ Proposed change
elif cmd == CMD_STOP: return + elif cmd == CMD_START: + logging.debug("Received start command, but acquisition already started. Ignoring.") + continue elif cmd == CMD_IMG: # info from the HPDTA image monitor🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/hamamatsurx.py` around lines 888 - 894, Add an explicit CMD_START branch to the outer command dispatch chain alongside CMD_SW_TRIGGER, CMD_STOP, and CMD_IMG, treating it as a known ignorable command like the inner flush loop does; leave the unknown-command warning for all other values.
276-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the suppressed exception from
AcqStop.The bare
except Exception: passhides real communication failures during shutdown. Static analysis reports this as S110. Add a debug log so the failure is traceable.♻️ Proposed change
# Just in case the acquisition thread failed, directly stop the acquisition try: self.parent.AcqStop() except Exception: - pass + logging.debug("Failed to stop the acquisition during termination", exc_info=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/hamamatsurx.py` around lines 276 - 285, Update the AcqStop cleanup in the acquisition shutdown path to catch the exception as a variable and emit it through the driver's debug logger before continuing shutdown. Preserve the existing best-effort behavior and subsequent _va_poll cancellation.Source: Linters/SAST tools
742-754: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
CMD_SW_TRIGGERand the unused unpacking.Line 744 lists
CMD_SW_TRIGGERtwice in the membership test. The duplicate has no effect. Line 784 unpacksargsand never uses it, which Ruff reports as RUF059.♻️ Proposed change
- if cmd in (CMD_START, CMD_SW_TRIGGER, CMD_STOP, CMD_SW_TRIGGER, CMD_IMG, CMD_QUIT): + if cmd in (CMD_START, CMD_SW_TRIGGER, CMD_STOP, CMD_IMG, CMD_QUIT):And in
_acq_wait_start:- cmd, *args = self._get_acq_msg(block=True) + cmd, *_args = self._get_acq_msg(block=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/hamamatsurx.py` around lines 742 - 754, Remove the duplicate CMD_SW_TRIGGER entry from the command membership check in the acquisition message loop, and update _acq_wait_start to avoid unpacking the unused args value so Ruff RUF059 is satisfied.Source: Linters/SAST tools
526-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport the cause when the photon-counting exposure set fails.
The
except Exceptionblock hides the RemoteEx error and continues. The caller then receives the unchanged hardware value with no indication of failure. Log the exception details, or raise, as_setCamExpTimedoes.♻️ Proposed change
try: self.parent.CamParamSet("PC", "Exposure", exp_time_raw) except Exception: - logging.warning("Failed to set exposure time for photon-counting mode.") + logging.warning("Failed to set exposure time %s for photon-counting mode.", + exp_time_raw, exc_info=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/driver/hamamatsurx.py` around lines 526 - 532, Update the exception handling in the photon-counting exposure path around CamParamSet to include the caught RemoteEx exception details in the warning, matching the behavior of _setCamExpTime. Preserve the subsequent _get_pc_exp_time return flow unless the existing API requires propagating the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install/linux/usr/share/odemis/sim/sparc2-streakcam-sim.odm.yaml`:
- Around line 274-283: Update the synchroscan time-range mapping used by
DelayGenerator and its MD_TIME_RANGE_TO_DELAY lookup so 78 ps, 235 ps, 720 ps,
1.6 ns, and 3.6 ns resolve to their intended trigger delays; ensure triggerDelay
is assigned for these configured ranges instead of remaining unchanged.
In `@src/odemis/driver/hamamatsurx.py`:
- Around line 1683-1688: Initialize self._notification_thread to None before
checking self._gui_user in the initialization flow, then retain the existing
thread creation and start behavior when a GUI user is available. Ensure this
initialization occurs before any error path can call terminate(), so terminate()
can safely evaluate the attribute during normal and failed construction.
- Around line 831-843: Bound the pending-command wait in the acquisition flow
around the _sync_event and AsyncCommandStatus loop: track AcqStop retries, exit
the loop once a defined retry limit is reached, and preserve the existing
timeout-based AcqStop behavior before that limit. Ensure control returns to
queue_img processing so CMD_STOP and CMD_QUIT can be handled.
- Around line 2018-2022: Update the warning-queue trimming loop in the
error_code == 5 branch to use a non-blocking queue removal operation, avoiding a
stall if another thread drains the queue after the size check. Correct the
comment typo from “wernings” to “warnings” while preserving the existing
trimming behavior.
In `@src/odemis/driver/test/hamamatsurx_test.py`:
- Around line 737-747: Update receive_image to record each observed image shape
in a collection instead of asserting image.shape there, while continuing to
store metadata and manage image completion. In each test body after waiting for
acquisition, assert every recorded shape matches _expected_shape and that
_last_md contains model.MD_EXP_TIME; initialize and reset the collection
consistently with the existing test state.
- Around line 833-834: Reorder the setup in the test so
self.readoutcam.data.synchronizedOn(None) executes before
self.readoutcam.data.subscribe(self.receive_image), ensuring synchronization is
configured before acquisition begins.
In `@src/odemis/util/driver.py`:
- Line 151: Update the notify_to_user function signature to include a None
return type annotation, preserving its existing parameter annotations and
behavior.
- Around line 137-144: Update the session filtering logic around the session
type check to query loginctl’s Active property and only add the user from an
active graphical user session; exclude inactive, greeter, and other non-user
sessions before inserting into gui_users. Add mocked coverage for competing
graphical sessions to verify the active user is selected.
In `@src/odemis/util/test/driver_test.py`:
- Around line 98-108: Update test_get_active_gui_users and test_notify_to_user
to annotate self and return None, and add concise docstrings describing each
test’s behavior. Keep the existing test logic unchanged.
- Around line 98-108: Update test_get_active_gui_users and test_notify_to_user
to mock the GUI-session command via a session fixture patching
subprocess.check_output, and mock subprocess.run for notification delivery.
Assert the expected notification command rather than relying on a live session,
host users, or desktop services, while preserving the existing return-type and
non-empty user assertions.
---
Nitpick comments:
In `@src/odemis/driver/hamamatsurx.py`:
- Around line 3033-3041: Annotate the mutable class-level lists
SINGLESWEEP_TIME_RANGES and SYNCHROSCAN_TIME_RANGES as ClassVar, and add
ClassVar to the existing typing imports in their containing class. Keep the list
contents unchanged.
- Around line 2078-2086: Update AppStart’s ini_file annotation from str to
Optional[str] while keeping its None default, using the existing Optional
import. Extend the docstring to document the visible parameter and its behavior.
- Around line 888-894: Add an explicit CMD_START branch to the outer command
dispatch chain alongside CMD_SW_TRIGGER, CMD_STOP, and CMD_IMG, treating it as a
known ignorable command like the inner flush loop does; leave the
unknown-command warning for all other values.
- Around line 276-285: Update the AcqStop cleanup in the acquisition shutdown
path to catch the exception as a variable and emit it through the driver's debug
logger before continuing shutdown. Preserve the existing best-effort behavior
and subsequent _va_poll cancellation.
- Around line 742-754: Remove the duplicate CMD_SW_TRIGGER entry from the
command membership check in the acquisition message loop, and update
_acq_wait_start to avoid unpacking the unused args value so Ruff RUF059 is
satisfied.
- Around line 526-532: Update the exception handling in the photon-counting
exposure path around CamParamSet to include the caught RemoteEx exception
details in the warning, matching the behavior of _setCamExpTime. Preserve the
subsequent _get_pc_exp_time return flow unless the existing API requires
propagating the failure.
In `@src/odemis/driver/test/hamamatsurx_test.py`:
- Around line 810-813: Remove the KeyboardInterrupt handler surrounding the test
cleanup so interruptions propagate and the test does not report success. Keep
the finally block that resets self.readoutcam.photonCounting.value to False.
- Around line 386-387: Update the docstring in test_pc_exposure_time to describe
testing the photon-counting exposure VA, rather than repeating the
readout-camera exposure description from test_exposure_time.
- Around line 605-621: In test_acq_get_scaling_table, add a
_check_time_list(img) helper that validates the time-list metadata and checks
the final value against the scale factor using the intended expected-range
assertion, then replace all three duplicated validation blocks with
self._check_time_list(img).
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4567b12f-e1a0-43fc-81ba-b6ed2319a16a
📒 Files selected for processing (6)
install/linux/usr/share/odemis/sim/sparc2-ek-streakcam-sim.odm.yamlinstall/linux/usr/share/odemis/sim/sparc2-streakcam-sim.odm.yamlsrc/odemis/driver/hamamatsurx.pysrc/odemis/driver/test/hamamatsurx_test.pysrc/odemis/util/driver.pysrc/odemis/util/test/driver_test.py
| if self._sync_event and not is_receiving_image: | ||
| timeout = 2 | ||
| # Wait until HPDTA is ready again: there is no "pending" command (ie, either running or about to run) | ||
| timeout = 2 # s | ||
| start = time.time() | ||
| while int(self.parent.AsyncCommandStatus()[0]): | ||
| time.sleep(0) | ||
| logging.debug("Asynchronous RemoteEx command still in process. Wait until finished.") | ||
| if time.time() > start + timeout: # most likely camera is in live-mode, so stop camera | ||
| time.sleep(1e-3) | ||
| if time.time() > start + timeout: | ||
| logging.info("Asynchronous RemoteEx command still in process after %g s. " | ||
| "Stopping acquisition to reset state.", timeout) | ||
| # most likely camera is in live-mode, so stop camera, and wait a bit more | ||
| self.parent.AcqStop() | ||
| start = time.time() | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the wait for the pending RemoteEx command.
The inner while int(self.parent.AsyncCommandStatus()[0]): loop resets start after each AcqStop(). If HPDTA never reports the command as finished, the loop repeats forever. The acquisition thread then never reads queue_img again, so CMD_STOP and CMD_QUIT are never processed, and terminate() returns after the 5 s join with the thread still running.
Limit the number of AcqStop retries and leave the loop when the limit is reached.
🐛 Proposed fix
timeout = 2 # s
start = time.time()
+ retries = 0
while int(self.parent.AsyncCommandStatus()[0]):
time.sleep(1e-3)
if time.time() > start + timeout:
+ if retries >= 3:
+ logging.warning("HPDTA still busy after %d stop attempts, "
+ "continuing anyway.", retries)
+ break
logging.info("Asynchronous RemoteEx command still in process after %g s. "
"Stopping acquisition to reset state.", timeout)
# most likely camera is in live-mode, so stop camera, and wait a bit more
self.parent.AcqStop()
+ retries += 1
start = time.time()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self._sync_event and not is_receiving_image: | |
| timeout = 2 | |
| # Wait until HPDTA is ready again: there is no "pending" command (ie, either running or about to run) | |
| timeout = 2 # s | |
| start = time.time() | |
| while int(self.parent.AsyncCommandStatus()[0]): | |
| time.sleep(0) | |
| logging.debug("Asynchronous RemoteEx command still in process. Wait until finished.") | |
| if time.time() > start + timeout: # most likely camera is in live-mode, so stop camera | |
| time.sleep(1e-3) | |
| if time.time() > start + timeout: | |
| logging.info("Asynchronous RemoteEx command still in process after %g s. " | |
| "Stopping acquisition to reset state.", timeout) | |
| # most likely camera is in live-mode, so stop camera, and wait a bit more | |
| self.parent.AcqStop() | |
| start = time.time() | |
| if self._sync_event and not is_receiving_image: | |
| # Wait until HPDTA is ready again: there is no "pending" command (ie, either running or about to run) | |
| timeout = 2 # s | |
| start = time.time() | |
| retries = 0 | |
| while int(self.parent.AsyncCommandStatus()[0]): | |
| time.sleep(1e-3) | |
| if time.time() > start + timeout: | |
| if retries >= 3: | |
| logging.warning("HPDTA still busy after %d stop attempts, " | |
| "continuing anyway.", retries) | |
| break | |
| logging.info("Asynchronous RemoteEx command still in process after %g s. " | |
| "Stopping acquisition to reset state.", timeout) | |
| # most likely camera is in live-mode, so stop camera, and wait a bit more | |
| self.parent.AcqStop() | |
| retries += 1 | |
| start = time.time() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/driver/hamamatsurx.py` around lines 831 - 843, Bound the
pending-command wait in the acquisition flow around the _sync_event and
AsyncCommandStatus loop: track AcqStop retries, exit the loop once a defined
retry limit is reached, and preserve the existing timeout-based AcqStop behavior
before that limit. Ensure control returns to queue_img processing so CMD_STOP
and CMD_QUIT can be handled.
| def receive_image(self, dataflow, image: model.DataArray): | ||
| """Callback for readout camera""" | ||
| self.assertEqual(image.shape, self._expected_shape) | ||
| self._last_md = image.metadata | ||
| self.assertIn(model.MD_EXP_TIME, image.metadata) | ||
| self.images_left -= 1 | ||
| self.image_received.set() | ||
| logging.debug("Got image of shape %s.", image.shape) | ||
| if self.images_left == 0: | ||
| dataflow.unsubscribe(self.receive_image) | ||
| self.end_time = time.time() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assertions inside receive_image do not fail the test.
receive_image runs in the driver acquisition thread. An AssertionError raised here propagates into ReadoutCamera._acquire, which catches Exception and only logs it. The test then continues and reports a confusing failure on images_left, or passes if the counter still reaches zero through later images.
Record the observed values in the callback and assert in the test body.
♻️ Proposed change
def receive_image(self, dataflow, image: model.DataArray):
"""Callback for readout camera"""
- self.assertEqual(image.shape, self._expected_shape)
+ self._shapes.append(image.shape)
self._last_md = image.metadata
- self.assertIn(model.MD_EXP_TIME, image.metadata)
self.images_left -= 1Then in each test, after the wait, assert all(s == self._expected_shape for s in self._shapes) and model.MD_EXP_TIME in self._last_md.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/driver/test/hamamatsurx_test.py` around lines 737 - 747, Update
receive_image to record each observed image shape in a collection instead of
asserting image.shape there, while continuing to store metadata and manage image
completion. In each test body after waiting for acquisition, assert every
recorded shape matches _expected_shape and that _last_md contains
model.MD_EXP_TIME; initialize and reset the collection consistently with the
existing test state.
| self.readoutcam.data.subscribe(self.receive_image) | ||
| self.readoutcam.data.synchronizedOn(None) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Call synchronizedOn(None) before subscribe.
The test subscribes first and removes the synchronization afterwards. The driver documents that changing the synchronization during an acquisition is not handled, and the acquisition thread can then need up to about 10 s to reach the correct state. Set the synchronization before starting the acquisition.
🐛 Proposed fix
+ self.readoutcam.data.synchronizedOn(None)
self.readoutcam.data.subscribe(self.receive_image)
- self.readoutcam.data.synchronizedOn(None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.readoutcam.data.subscribe(self.receive_image) | |
| self.readoutcam.data.synchronizedOn(None) | |
| self.readoutcam.data.synchronizedOn(None) | |
| self.readoutcam.data.subscribe(self.receive_image) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/driver/test/hamamatsurx_test.py` around lines 833 - 834, Reorder
the setup in the test so self.readoutcam.data.synchronizedOn(None) executes
before self.readoutcam.data.subscribe(self.receive_image), ensuring
synchronization is configured before acquisition begins.
| # Check session type (wayland or x11) | ||
| stype = subprocess.check_output( | ||
| ["loginctl", "show-session", session_id, "-p", "Type", "--value"], | ||
| text=True | ||
| ).strip() | ||
|
|
||
| if stype in ("wayland", "x11"): | ||
| gui_users.add(user) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter for the active user session.
This code accepts every graphical session. It does not check Active or exclude non-user sessions. The downstream driver selects an arbitrary member from this set. HPDTA warnings can go to an inactive or greeter session when multiple graphical sessions exist.
Query and require the active user session before adding its user name. Add mocked coverage for competing graphical sessions.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 137-140: Command coming from incoming request
Context: subprocess.check_output(
["loginctl", "show-session", session_id, "-p", "Type", "--value"],
text=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 137-140: Avoid command injection
Context: subprocess.check_output(
["loginctl", "show-session", session_id, "-p", "Type", "--value"],
text=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-python)
🪛 Ruff (0.16.1)
[error] 138-138: subprocess call: check for execution of untrusted input
(S603)
[error] 139-139: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/util/driver.py` around lines 137 - 144, Update the session
filtering logic around the session type check to query loginctl’s Active
property and only add the user from an active graphical user session; exclude
inactive, greeter, and other non-user sessions before inserting into gui_users.
Add mocked coverage for competing graphical sessions to verify the active user
is selected.
| return gui_users | ||
|
|
||
|
|
||
| def notify_to_user(target_user: str, title: str, message: str, app: Optional[str] = None, icon: Optional[str] = None): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the return type annotation.
notify_to_user returns no value. Declare -> None. The coding guideline requires type hints for function parameters and return types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/util/driver.py` at line 151, Update the notify_to_user function
signature to include a None return type annotation, preserving its existing
parameter annotations and behavior.
Source: Coding guidelines
| def test_get_active_gui_users(self): | ||
| users = get_active_gui_users() | ||
| self.assertIsInstance(users, set) | ||
| self.assertGreaterEqual(len(users), 1) | ||
|
|
||
| def test_notify_to_user(self): | ||
| users = get_active_gui_users() | ||
| first_user = next(iter(users)) | ||
| # just check it doesn't raise an exception | ||
| odemis.util.driver.notify_to_user(first_user, "Test notification", "This is a test notification from the odemis.util.driver module.", | ||
| app="Odemis testing", icon="info") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add annotations and docstrings to the new test methods.
Type self, declare -> None, and add a short docstring to both test methods. The coding guideline requires type hints and docstrings for all Python functions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/util/test/driver_test.py` around lines 98 - 108, Update
test_get_active_gui_users and test_notify_to_user to annotate self and return
None, and add concise docstrings describing each test’s behavior. Keep the
existing test logic unchanged.
Source: Coding guidelines
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Mock the GUI-session and notification commands.
These tests depend on a live graphical session. get_active_gui_users() returns an empty set when loginctl is unavailable or no GUI user exists. Line 105 then raises StopIteration.
The notification test also sends a real desktop notification to the test host. Patch subprocess.check_output with session fixtures and patch subprocess.run. Assert the generated command instead of using host services.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/util/test/driver_test.py` around lines 98 - 108, Update
test_get_active_gui_users and test_notify_to_user to mock the GUI-session
command via a session fixture patching subprocess.check_output, and mock
subprocess.run for notification delivery. Assert the expected notification
command rather than relying on a live session, host users, or desktop services,
while preserving the existing return-type and non-empty user assertions.
… screen When runnning in remote mode, the message boxes/warnings of HPDTA are supressed. This ensures that the remote code can run without relying on the user. However, sometimes this is useful information. So instead of completely hiding it, show it on the screen as a notification.
30d7ed9 to
309077a
Compare
Add a (new) way to send notification to the user. We already had a way, in xt_client, however it has a few issues:
So this new way relies on the fact that we still have root privilege, and so can run commands on behalf of any user. The main trick is to find out first who is the user which has a GUI. This is provided by a separate function.
The HPDTA part ensures that the warnings of HPDTA, which are normally shown as a dialog box when it's not running in "remote mode" are still shown somewhere.