[TRPD-29][feat] driver hamamatsurx: add support for photon-counting acquisition and new simulator - #3541
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the Hamamatsu HPDTA RemoteEx streak camera driver to support photon-counting acquisition, and introduces a built-in HPDTA simulator so the driver (and configs/tests) can run without hardware.
Changes:
- Added photon-counting VAs/RemoteEx handling (PC exposure, integration counts, threshold) and acquisition-path support in
hamamatsurx.py. - Added a local
HPDTASimRemoteEx simulator and wiredhost: fake-*to start it automatically. - Updated streak camera tests and simulator microscope configs to use the new simulator and photon-counting capabilities.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/odemis/driver/test/hamamatsurx_test.py |
Enables simulator-based testing and adds photon-counting acquisition tests/paths. |
src/odemis/driver/hamamatsurx.py |
Implements photon-counting mode, refactors acquisition thread/message handling, and adds HPDTASim. |
install/linux/usr/share/odemis/sim/sparc2-streakcam-sim.odm.yaml |
Switches streak camera simulator to hamamatsurx.StreakCamera using fake-synchroscan + enables photon counting. |
install/linux/usr/share/odemis/sim/sparc2-ek-streakcam-sim.odm.yaml |
Switches streak camera simulator to hamamatsurx.StreakCamera using fake-singlesweep + enables photon counting. |
Suppressed comments (2)
src/odemis/driver/test/hamamatsurx_test.py:806
- Same as above: avoid unconditional file writes and stdout printing during tests; gate dumps behind an env var and log instead.
img = self.readoutcam.data.get()
hdf5.export("test_acq_photon_counting2.hdf5", img)
print(img.shape)
src/odemis/driver/test/hamamatsurx_test.py:857
- In simulator runs (TEST_NOHW),
pcIntegrationCounts=100makes synchronized photon-counting acquisitions slow (and this test does 40 images). Consider reducing the exposure settings when TEST_NOHW is set while keepingnum_imageshigh enough to exercise the >19-window behavior.
self.readoutcam.pcExposureTime.value = 10e-3 # s
self.readoutcam.pcIntegrationCounts.value = 100
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Hamamatsu driver now supports photon-counting controls and typed acquisition commands. It handles live, synchronized, monitor, and photon-counting acquisition. Sequence Diagram(s)sequenceDiagram
participant SimulationConfig
participant StreakCamera
participant HPDTASim
participant ReadoutCamera
SimulationConfig->>StreakCamera: configure fake HPDTA host and port
StreakCamera->>HPDTASim: start RemoteEx connection
ReadoutCamera->>StreakCamera: request acquisition
StreakCamera->>HPDTASim: send acquisition command
HPDTASim-->>StreakCamera: return image and monitor data
StreakCamera-->>ReadoutCamera: publish image with acquisition metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
src/odemis/driver/hamamatsurx.py (4)
2037-2037: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
ini_fileasOptional[str].The default is
None, so the annotation must allowNone. Ruff reports RUF013 for this line.Optionalis already imported and used at line 1643.♻️ 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):🤖 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` at line 2037, Update the AppStart method signature to annotate ini_file as Optional[str], preserving its existing default value and behavior; Optional is already available in the module.Source: Linters/SAST tools
904-929: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLower the per-frame log at line 929 to debug.
This
logging.infocall runs for every image in live mode. At a 10 Hz frame rate it floods the back-end log. Line 923 already useslogging.debugfor the same loop.♻️ Proposed change
- logging.info("No more images in queue, will read the latest one.") + logging.debug("No more images in queue, will read the latest one.")🤖 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 904 - 929, Change the per-frame logging call after the live-mode queue-draining loop from logging.info to logging.debug, leaving the message and surrounding acquisition logic unchanged.
750-762: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
CMD_SW_TRIGGERin the validity check.
CMD_SW_TRIGGERappears twice in the accepted-command tuple on line 752. The duplicate has no effect, but it hides which commands the filter really accepts.♻️ Proposed cleanup
- 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):🤖 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 750 - 762, Remove the duplicated CMD_SW_TRIGGER entry from the accepted-command tuple in the queue message loop, preserving all other valid commands and the existing handling in the surrounding acquisition message flow.
490-540: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFactor the exposure-time accessors by location.
_get_pc_exp_time_range,_get_pc_exp_time, and_set_pc_exp_timeduplicate_getCamExpTimeRange,GetCamExpTime, and_setCamExpTime. Only the location string differs ("PC" instead of "Live"). Add a location parameter to the three original methods and call them with "PC" from the photon-counting setters. Note also that_set_pc_exp_timeonly logs a warning on failure while_setCamExpTimeraisesIOError; pick one behaviour for both locations.🤖 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 490 - 540, Refactor the duplicated photon-counting exposure methods by adding a location parameter to _getCamExpTimeRange, GetCamExpTime, and _setCamExpTime, then have the PC accessors delegate to them with "PC" while the existing camera paths use "Live". Align _setCamExpTime and _set_pc_exp_time to the same failure behavior, preferably propagating the IOError rather than only logging a warning.src/odemis/driver/test/hamamatsurx_test.py (4)
784-813: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClean up the exported HDF5 files and replace
The test writes
test_acq_photon_counting1.hdf5andtest_acq_photon_counting2.hdf5into the current working directory and never removes them. Repeated runs leave artifacts in the repository. Usetempfile.mkdtemp()or delete the files intearDown. Replace the twologging.debug, so the output follows the convention used by the other tests in this file.I kept the export itself, because the project keeps in-loop debug exports on purpose.
Based on learnings: "Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources."
🤖 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 784 - 813, The test_acq_photon_counting method should export both HDF5 files into a temporary directory created with tempfile.mkdtemp(), clean up that directory after the test, and replace both print(img.shape) calls with logging.debug. Preserve the existing in-loop hdf5.export calls and assertions.Source: Learnings
591-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe two time ranges in this test now resolve to almost the same value.
TestHamamatsurxCamnow usesKWARGS_STREAKCAM_SYNC_NOD, so the streak unit is the synchroscan variant with time ranges from 78 ps to 3.6 ns.util.find_closest(0.001, ...)at line 611 therefore returns 3.6e-9, not 1 ms, and the comment "1ms" no longer describes the selected range. The test still passes, but it no longer covers two different time magnitudes. Select the two extremes ofself.streakunit.timeRange.choicesinstead of fixed absolute values, and update the comments.🤖 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 591 - 621, Update test_acq_get_scaling_table to select the minimum and maximum values from self.streakunit.timeRange.choices rather than fixed absolute values, ensuring the two acquisitions exercise distinct time-range extremes. Revise the associated comments to describe the selected minimum and maximum ranges accurately.
833-834: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet the synchronization before subscribing.
subscribestarts the acquisition.synchronizedOn(None)is called after it.test_acq_sync_photon_countinguses the opposite order at lines 869-870. MovesynchronizedOn(None)beforesubscribe, or remove it, because the dataflow is not synchronized in this test.♻️ Proposed change
- self.readoutcam.data.subscribe(self.receive_image) 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, In the test setup around readoutcam.data, call synchronizedOn(None) before subscribe(self.receive_image), or remove the synchronization call since this dataflow is unsynchronized. Match the ordering used by test_acq_sync_photon_counting and ensure subscribing does not start acquisition before synchronization is configured.
76-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive each fake Hamamatsu simulator its own port.
HPDTASimbinds its command/data TCP servers directly and does not enable address reuse before binding. If oneStreakCamerasimulator is not already shut down before the next class starts on port11001, bind failure can turn into a misleading connection error. Define shared port constants and assign different ports fromTEST_NOHWsetups so each fake configuration can bind independently.🤖 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 76 - 99, Define shared constants for distinct fake Hamamatsu simulator ports, then update the TEST_NOHW port values in KWARGS_STREAKCAM, KWARGS_STREAKCAM_SYNC, KWARGS_STREAKCAM_NO_DELAYBOX, KWARGS_STREAKCAM_SYNC_NOD, and KWARGS_STREAKCAM_NO_READOUT_CAM to use separate ports; preserve the existing real-hardware port selection.install/linux/usr/share/odemis/sim/sparc2-streakcam-sim.odm.yaml (1)
275-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign
TIME_RANGE_TO_DELAYwith the new synchroscan time ranges.The streak unit now exposes time ranges of 78 ps to 3600 ps. The
Streak Delay Generatormetadata at lines 292-316 still maps 1 ns to 10 ms. Every lookup throughutil.find_closesttherefore collapses onto the 1 ns or 2 ns entry, and the simulated trigger delay is meaningless for the four shortest ranges. Add entries for the five simulated ranges so the simulation reproduces the real mapping.🔧 Proposed metadata for the synchroscan ranges
metadata: { TIME_RANGE_TO_DELAY: { # timeRange (s) -> triggerDelay (s) + 78.e-12: 7.99e-9, + 235.e-12: 9.63e-9, + 720.e-12: 3.32e-8, + 1600.e-12: 4.59e-8, + 3600.e-12: 6.64e-8, 1.0e-09: 7.99e-9, # 1ns🤖 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 `@install/linux/usr/share/odemis/sim/sparc2-streakcam-sim.odm.yaml` around lines 275 - 282, Update the Streak Delay Generator’s TIME_RANGE_TO_DELAY metadata near the existing 1 ns–10 ms mapping to include entries for all five synchroscan ranges: 78 ps, 235 ps, 720 ps, 1600 ps, and 3600 ps. Preserve the existing util.find_closest lookup and ensure each simulated range maps to its corresponding delay value rather than collapsing onto the current nanosecond entries.
🤖 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 `@src/odemis/driver/hamamatsurx.py`:
- Around line 868-874: Update the photon-counting branch in the sync-event
timeout calculation to use pcExposureTime.value multiplied by
pcIntegrationCounts.value, matching the PC exposure used by _get_image for
MD_EXP_TIME. Leave the non-photon-counting exposureTime path unchanged.
- Around line 3238-3288: Update _acq_worker to accept and use the
per-acquisition stop event (acq_stop) for the exposure wait and post-wait
cancellation check instead of relying only on _must_stop. When single-mode
acquisition completes, clear _acq_mode only if self._acq_stop is the same
acq_stop event, preventing an old worker from stopping a newer acquisition;
update the worker’s caller/state setup to pass and retain this event.
- Around line 367-371: Preserve the user-selected MCPGain when changing binning
during an active acquisition: in the live-acquisition branch around _stop(),
save the current gain before stopping and restore it before or during _start().
Alternatively, bypass _stop() with the lower-level stop command while retaining
the existing AcqStop, CamParamSet, and restart sequence.
- Around line 1976-1977: Update the error_code == 4 image-queue condition in
sendCommand to compare a normalized lowercase rfunc against the lowercase
Livemonitor and Acqmonitor names, preserving queue_img routing for any casing.
- Around line 4054-4138: Guard variant-specific attribute access in
_handle_devparaminfo and _handle_devparaminfoex so unsupported parameters do not
raise AttributeError when DevParamsList omits them. Reuse the streak-unit checks
from _get_streak_param/_get_delay_param, or use getattr with appropriate
defaults, for _db_delay_time, _db_repetition_rate, _su_trig_mode, _db_setting,
and _db_lock_mode while preserving supported-variant responses.
In `@src/odemis/driver/test/hamamatsurx_test.py`:
- Around line 674-675: Update the assertion failure message in the test to
reference the existing images_left attribute instead of the undefined
_images_left attribute, so the original assertion failure remains visible.
- Around line 764-782: Reset the camera data synchronization at the end of the
test by calling synchronizedOn(None) on self.readoutcam.data after the
queue-empty assertion. Keep the existing subscription cleanup and assertions
unchanged, ensuring later tests do not inherit the software trigger
synchronization.
---
Nitpick comments:
In `@install/linux/usr/share/odemis/sim/sparc2-streakcam-sim.odm.yaml`:
- Around line 275-282: Update the Streak Delay Generator’s TIME_RANGE_TO_DELAY
metadata near the existing 1 ns–10 ms mapping to include entries for all five
synchroscan ranges: 78 ps, 235 ps, 720 ps, 1600 ps, and 3600 ps. Preserve the
existing util.find_closest lookup and ensure each simulated range maps to its
corresponding delay value rather than collapsing onto the current nanosecond
entries.
In `@src/odemis/driver/hamamatsurx.py`:
- Line 2037: Update the AppStart method signature to annotate ini_file as
Optional[str], preserving its existing default value and behavior; Optional is
already available in the module.
- Around line 904-929: Change the per-frame logging call after the live-mode
queue-draining loop from logging.info to logging.debug, leaving the message and
surrounding acquisition logic unchanged.
- Around line 750-762: Remove the duplicated CMD_SW_TRIGGER entry from the
accepted-command tuple in the queue message loop, preserving all other valid
commands and the existing handling in the surrounding acquisition message flow.
- Around line 490-540: Refactor the duplicated photon-counting exposure methods
by adding a location parameter to _getCamExpTimeRange, GetCamExpTime, and
_setCamExpTime, then have the PC accessors delegate to them with "PC" while the
existing camera paths use "Live". Align _setCamExpTime and _set_pc_exp_time to
the same failure behavior, preferably propagating the IOError rather than only
logging a warning.
In `@src/odemis/driver/test/hamamatsurx_test.py`:
- Around line 784-813: The test_acq_photon_counting method should export both
HDF5 files into a temporary directory created with tempfile.mkdtemp(), clean up
that directory after the test, and replace both print(img.shape) calls with
logging.debug. Preserve the existing in-loop hdf5.export calls and assertions.
- Around line 591-621: Update test_acq_get_scaling_table to select the minimum
and maximum values from self.streakunit.timeRange.choices rather than fixed
absolute values, ensuring the two acquisitions exercise distinct time-range
extremes. Revise the associated comments to describe the selected minimum and
maximum ranges accurately.
- Around line 833-834: In the test setup around readoutcam.data, call
synchronizedOn(None) before subscribe(self.receive_image), or remove the
synchronization call since this dataflow is unsynchronized. Match the ordering
used by test_acq_sync_photon_counting and ensure subscribing does not start
acquisition before synchronization is configured.
- Around line 76-99: Define shared constants for distinct fake Hamamatsu
simulator ports, then update the TEST_NOHW port values in KWARGS_STREAKCAM,
KWARGS_STREAKCAM_SYNC, KWARGS_STREAKCAM_NO_DELAYBOX, KWARGS_STREAKCAM_SYNC_NOD,
and KWARGS_STREAKCAM_NO_READOUT_CAM to use separate ports; preserve the existing
real-hardware port selection.
🪄 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: 96cbe39a-62a7-4361-8b84-d1a5c624b4c8
📒 Files selected for processing (4)
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.py
26d370f to
1ccbf84
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/odemis/driver/hamamatsurx.py (1)
4063-4066: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winVariant-specific attributes are still read without a guard in
DevParamInfoandDevParamInfoEx.
sim._db_delay_timeandsim._db_lock_modeexist only forsynchroscan.sim._db_repetition_rate,sim._db_setting, andsim._su_trig_modeexist only forsinglesweep. These handlers read them for either variant, so anAttributeErroris raised and returned as RemoteEx error 8. Apply the same variant checks used by_get_streak_paramand_get_delay_param, or usegetattr(sim, name, default).Also applies to: 4107-4136
🤖 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 4063 - 4066, Guard the variant-specific attribute reads in the DevParamInfo and DevParamInfoEx handlers, including the branches around “delaytime”, “repetitionrate”, and the additional settings at the referenced range. Follow the variant checks used by _get_streak_param and _get_delay_param, or provide appropriate defaults via getattr, so synchroscan-only and singlesweep-only attributes are never accessed on the other variant.
🧹 Nitpick comments (6)
src/odemis/driver/hamamatsurx.py (4)
526-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport failures when setting the photon-counting exposure time.
_set_pc_exp_timeswallows theCamParamSeterror and logs a warning without the traceback._setCamExpTime(line 476) propagates the same failure. A user who setspcExposureTimegets no error, and the reason for the failure is lost. Uselogging.exceptionat minimum, or let the error propagate for consistency.🔧 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.exception("Failed to set exposure time %s for photon-counting mode.", exp_time_raw)🤖 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 _set_pc_exp_time so failures from parent.CamParamSet("PC", "Exposure", exp_time_raw) are not silently swallowed: either let the exception propagate consistently with _setCamExpTime or replace the warning with logging.exception to preserve the traceback and report the failure.
3384-3399: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDetect data-port disconnection instead of polling until shutdown.
_HPDTADataHandler.handlesleeps in a loop and never reads from the socket, so it cannot detect that the client closed the connection. The handler thread stays alive untilterminate(). When a test creates and terminates severalStreakCamerainstances against long-lived simulators, stale handlers accumulate. A blockingrecvwith the existing 1 s timeout would exit the loop on a clean close.♻️ Proposed change
while not self.sim._must_stop.is_set(): - time.sleep(0.1) + self.request.settimeout(1.0) + try: + if not self.request.recv(4096): + logging.debug("HPDTASim: data connection closed by client") + break + except socket.timeout: + continue🤖 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 3384 - 3399, Update _HPDTADataHandler.handle to monitor the data socket with blocking recv calls using the existing 1-second timeout, rather than only sleeping until _must_stop is set. Exit the loop when recv returns no data or raises the timeout/connection error expected during normal disconnection, while preserving cleanup of _data_conn in the finally block.
2029-2029: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an explicit
Optional[str]annotation forini_file.
ini_file: str = Noneis an implicit Optional and contradicts the annotated default.Optionalis already imported in this module.♻️ 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):As 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` at line 2029, Update the ini_file parameter annotation in AppStart to Optional[str] while preserving its existing default value and behavior; Optional is already available in the module.Sources: Coding guidelines, Linters/SAST tools
744-744: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
CMD_SW_TRIGGERentry.The membership tuple lists
CMD_SW_TRIGGERtwice. The behaviour is unchanged, but the duplication hides which commands are accepted.♻️ 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):🤖 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` at line 744, Remove the duplicated CMD_SW_TRIGGER entry from the command membership tuple in the surrounding command-handling logic, leaving each accepted command listed exactly once and preserving the existing behavior.src/odemis/driver/test/hamamatsurx_test.py (2)
810-813: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not swallow
KeyboardInterruptin the test.The
except KeyboardInterruptbranch prints a message and lets the test report success. An interrupted run then looks like a passing run. Remove the handler and keep thefinallyblock for cleanup.♻️ 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 except KeyboardInterrupt handler surrounding the test, including its print statement, so interruptions propagate and the test is not reported as successful. Keep the finally block that resets self.readoutcam.photonCounting.value to False.
833-840: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemove the synchronization before subscribing, and use the image event to wait.
Line 834 calls
synchronizedOn(None)aftersubscribehas already started the acquisition. The driver notes in_acquire_imagesthat a synchronization change during an acquisition is not handled, so this order can leave the acquisition thread in a stale state. CallsynchronizedOn(None)first.The wait loop also sleeps a full
2 * est_time + 1per iteration and only then checks the counter, so the worst case is about 150 s.self.image_receivedis already prepared at line 830; waiting on it makes the test both faster and more precise.♻️ Proposed change
- self.readoutcam.data.subscribe(self.receive_image) self.readoutcam.data.synchronizedOn(None) + self.readoutcam.data.subscribe(self.receive_image) # Wait for the image - for i in range(num_images): - time.sleep(2 * est_time + 1) - if self.images_left == 0: - break + for i in range(num_images): + if not self.image_received.wait(timeout=2 * est_time + 1): + self.fail(f"Timeout waiting for image {i + 1}/{num_images}") + self.image_received.clear() + if self.images_left == 0: + break🤖 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 - 840, Update the acquisition setup around self.readoutcam.data so synchronizedOn(None) runs before subscribe(self.receive_image), then replace the polling sleep loop with waits on the existing self.image_received event while preserving the num_images limit and images_left completion condition.Source: Linters/SAST tools
🤖 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 `@src/odemis/driver/hamamatsurx.py`:
- Around line 3222-3228: Update _stop_acquisition_locked and the acquisition
worker to use a stop event scoped to each acquisition rather than relying on
_must_stop, and signal that event before joining so AcqStop interrupts the
worker’s wait promptly. Ensure the worker captures its acquisition mode/token
and only clears _acq_mode if it still owns the current acquisition, preventing
an older worker from clearing a newer mode after a join timeout.
- Around line 3068-3076: Update HPDTASim.__init__ around the _cmd_server and
_data_server construction so a failure while creating _data_server closes the
already-created _cmd_server before propagating the exception. Reuse the server’s
existing shutdown/close mechanism, and ensure cleanup also covers any partial
data-server initialization without changing normal startup behavior.
- Around line 4227-4232: Fix the scalingtable branch in the request handler so
the announced value count matches the data returned by
_generate_scaling_table(). Either make _generate_scaling_table() generate the
requested horizontal length, or reject non-vertical directions before sending a
response; preserve correct vertical behavior and ensure clients never receive a
declared length different from the transmitted float32 data.
---
Duplicate comments:
In `@src/odemis/driver/hamamatsurx.py`:
- Around line 4063-4066: Guard the variant-specific attribute reads in the
DevParamInfo and DevParamInfoEx handlers, including the branches around
“delaytime”, “repetitionrate”, and the additional settings at the referenced
range. Follow the variant checks used by _get_streak_param and _get_delay_param,
or provide appropriate defaults via getattr, so synchroscan-only and
singlesweep-only attributes are never accessed on the other variant.
---
Nitpick comments:
In `@src/odemis/driver/hamamatsurx.py`:
- Around line 526-532: Update _set_pc_exp_time so failures from
parent.CamParamSet("PC", "Exposure", exp_time_raw) are not silently swallowed:
either let the exception propagate consistently with _setCamExpTime or replace
the warning with logging.exception to preserve the traceback and report the
failure.
- Around line 3384-3399: Update _HPDTADataHandler.handle to monitor the data
socket with blocking recv calls using the existing 1-second timeout, rather than
only sleeping until _must_stop is set. Exit the loop when recv returns no data
or raises the timeout/connection error expected during normal disconnection,
while preserving cleanup of _data_conn in the finally block.
- Line 2029: Update the ini_file parameter annotation in AppStart to
Optional[str] while preserving its existing default value and behavior; Optional
is already available in the module.
- Line 744: Remove the duplicated CMD_SW_TRIGGER entry from the command
membership tuple in the surrounding command-handling logic, leaving each
accepted command listed exactly once and preserving the existing behavior.
In `@src/odemis/driver/test/hamamatsurx_test.py`:
- Around line 810-813: Remove the except KeyboardInterrupt handler surrounding
the test, including its print statement, so interruptions propagate and the test
is not reported as successful. Keep the finally block that resets
self.readoutcam.photonCounting.value to False.
- Around line 833-840: Update the acquisition setup around self.readoutcam.data
so synchronizedOn(None) runs before subscribe(self.receive_image), then replace
the polling sleep loop with waits on the existing self.image_received event
while preserving the num_images limit and images_left completion condition.
🪄 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: b85c5e6e-e852-40ba-a64b-1a9e41892e1b
📒 Files selected for processing (2)
src/odemis/driver/hamamatsurx.pysrc/odemis/driver/test/hamamatsurx_test.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/odemis/driver/test/hamamatsurx_test.py:731
- This assertion can raise KeyError when no image was received, which makes failures harder to diagnose than a clean assertion with context.
time.sleep(num_images * exp_time + 2)
self.assertTrue(self._last_md[model.MD_STREAK_MODE])
self.assertIn(model.MD_TIME_LIST, self._last_md)
self.assertEqual(self.images_left, 0)
src/odemis/driver/test/hamamatsurx_test.py:832
- The photon-counting acquisition tests use long exposure/integration settings (and high num_images) even under TEST_NOHW, which can make the simulator test suite take minutes to run. It’s better to scale these values down when running against the local HPDTASim, while keeping the same code paths exercised.
# Set typical settings used of series of acquisitions in photon counting mode
self.streakunit.streakMode.value = True
self.streakunit.timeRange.value = self.streakunit.timeRange.clip(2e-9) # 2ns
self.readoutcam.photonCounting.value = True
self.readoutcam.pcExposureTime.value = 25e-3 # s
self.readoutcam.pcIntegrationCounts.value = 100
num_images = 25
size = self.readoutcam.resolution.value
self._expected_shape = size[::-1] # Y, X
self._last_md = {}
self.images_left = num_images # unsubscribe after receiving number of images
self.image_received.clear()
est_time = self.readoutcam.pcExposureTime.value * self.readoutcam.pcIntegrationCounts.value
src/odemis/driver/test/hamamatsurx_test.py:867
- Similarly, this synchronized photon-counting test uses settings that can make simulator runs very slow under TEST_NOHW. Reducing the per-frame acquisition time in the simulator keeps runtime reasonable while still validating the >19-window behavior.
# Set typical settings used of series of acquisitions in photon counting mode
self.streakunit.streakMode.value = True
self.streakunit.timeRange.value = self.streakunit.timeRange.clip(2e-9) # 2ns
self.readoutcam.photonCounting.value = True
self.readoutcam.pcExposureTime.value = 10e-3 # s
self.readoutcam.pcIntegrationCounts.value = 100
# HPDTA supports up to 19 windows/images. As each acquisition creates a new window, testing
# above this limit is important.
num_images = 40
size = self.readoutcam.resolution.value
self._expected_shape = size[::-1] # Y, X
self._last_md = {}
self.images_left = num_images # unsubscribe after receiving number of images
self.image_received.clear()
est_time = self.readoutcam.pcExposureTime.value * self.readoutcam.pcIntegrationCounts.value
src/odemis/driver/hamamatsurx.py:425
- The photonCounting VA setter says changing while acquiring is not supported, but it still accepts the new value when acquisition is active. That can leave the VA reporting a mode that the acquisition thread will not actually use until the next start, causing inconsistent behavior.
if self.data.active:
# We could support it, but it's a lot of extra complexity to the code, and in reality, never used.
logging.warning("Photon-counting mode changed to %s while acquiring: not supported", value)
return value
src/odemis/driver/test/hamamatsurx_test.py:25
- Unused import: hdf5 is imported but never referenced in this test module.
from odemis import model, util
from odemis.dataio import hdf5
from odemis.driver import hamamatsurx
src/odemis/driver/test/hamamatsurx_test.py:705
- This assertion can raise KeyError when no image was received (e.g. if acquisition silently failed), which makes the test error less informative than a normal assertion failure.
This issue also appears on line 728 of the same file.
self.assertEqual(self.streakunit.MCPGain.value, 0) # MCPGain should be zero when acq finished
self.assertFalse(self._last_md[model.MD_STREAK_MODE])
self.assertEqual(self.images_left, 0)
1ccbf84 to
7957387
Compare
7957387 to
4b489aa
Compare
pieleric
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (9)
src/odemis/driver/test/hamamatsurx_test.py:699
- The per-trigger sleep of ~2s each adds significant wall time without strengthening the assertion. Reducing this delay (it’s only simulating processing) will make the test suite much faster and less flaky on CI.
time.sleep(2 + i * 0.1) # wait a bit to simulate some processing
src/odemis/driver/test/hamamatsurx_test.py:725
- Same as above: the 2s sleep per trigger makes the test suite very slow. Reducing it keeps the intent (simulate processing jitter) while avoiding long waits.
time.sleep(2 + i * 0.1) # wait a bit to simulate some processing
src/odemis/driver/test/hamamatsurx_test.py:825
- Even with shorter photon-counting exposure settings,
num_images = 25makes this test take at least ~25s (the loop sleeps ~>=1s per iteration). Consider using fewer images in simulator mode to keep the default test suite fast.
num_images = 25
src/odemis/driver/test/hamamatsurx_test.py:857
- Photon-counting synchronized acquisition uses settings that make each acquisition ~1s+ (10ms * 100). This makes the test suite very slow. Consider using much smaller PC settings in simulator mode while keeping the same assertions.
self.readoutcam.pcExposureTime.value = 10e-3 # s
self.readoutcam.pcIntegrationCounts.value = 100
src/odemis/driver/test/hamamatsurx_test.py:861
num_images = 40makes this synchronized photon-counting test take a long time (eachEvent.waithas a minimum ~1s timeout). To still test the “>19 windows” scenario while keeping runtime down, you can use just above the limit in simulator mode.
num_images = 40
src/odemis/driver/test/hamamatsurx_test.py:25
- Unused import:
from odemis.dataio import hdf5is never referenced in this test file, which can trigger lint failures and slows import slightly. Remove it unless it’s needed for upcoming assertions.
from odemis import model, util
from odemis.dataio import hdf5
from odemis.driver import hamamatsurx
src/odemis/driver/test/hamamatsurx_test.py:159
test_synchronization_removalis permanently skipped with a generic message, which silently reduces coverage. Prefer removing it until implemented, or converting it to anexpectedFailurewith a specific tracking reference so it’s visible in test results.
def test_synchronization_removal(self):
self.skipTest("Known to not work yet")
src/odemis/driver/test/hamamatsurx_test.py:823
- Photon-counting live acquisition tests will be extremely slow on CI: with
pcExposureTime=25msandpcIntegrationCounts=100, a single image takes ~2.5s, and the loop waits at least2*est_time+1per image. Use smaller values inTEST_NOHWmode (simulator) to keep runtime manageable while still exercising the code path.
This issue also appears in the following locations of the same file:
- line 825
- line 856
- line 861
self.readoutcam.pcExposureTime.value = 25e-3 # s
self.readoutcam.pcIntegrationCounts.value = 100
src/odemis/driver/test/hamamatsurx_test.py:686
- When running against the simulator (
TEST_NOHW=1), keepingexposureTimeat 2s makes this test (and the suite) very slow. You can override it here to a shorter value for the simulator while keeping the longer exposure for real HW runs.
This issue also appears in the following locations of the same file:
- line 699
- line 725
exp_time = self.readoutcam.exposureTime.value
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/odemis/driver/test/hamamatsurx_test.py:159
- This test is permanently skipped, which reduces signal from CI. If the behavior is not implemented yet, consider at least linking the skip to a tracked issue so it doesn't get forgotten.
def test_synchronization_removal(self):
self.skipTest("Known to not work yet")
src/odemis/driver/test/hamamatsurx_test.py:825
test_acq_live_photon_countingcan take ~150s in the simulator with the current parameters (25 images × (25ms × 100 exposures) plus overhead). This is likely to exceed typical unit-test time budgets; consider using much smaller integration settings whenTEST_NOHWis enabled.
self.readoutcam.photonCounting.value = True
self.readoutcam.pcExposureTime.value = 25e-3 # s
self.readoutcam.pcIntegrationCounts.value = 100
num_images = 25
src/odemis/driver/test/hamamatsurx_test.py:858
test_acq_sync_photon_countingalso uses long photon-counting integration settings. Keeping shorter settings forTEST_NOHWmakes the simulator-backed CI much faster while still exercising the >19 window limit vianum_images = 40.
self.readoutcam.photonCounting.value = True
self.readoutcam.pcExposureTime.value = 10e-3 # s
self.readoutcam.pcIntegrationCounts.value = 100
src/odemis/driver/test/hamamatsurx_test.py:25
- Unused import:
odemis.dataio.hdf5is imported but never referenced in this test module, which can cause lint failures and makes dependencies unclear.
This issue also appears in the following locations of the same file:
- line 158
- line 821
- line 855
from odemis import model, util
from odemis.dataio import hdf5
from odemis.driver import hamamatsurx
4b489aa to
427cfa9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 266: Remove the spectrograph entry from the “Streak Readout CCD”
dependencies so ReadoutCamera does not receive it twice; retain the spectrograph
dependency on the parent “Streak Camera” component, which passes it explicitly.
🪄 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: 733d5e6b-e020-4578-994d-798644dd83f3
📒 Files selected for processing (2)
install/linux/usr/share/odemis/sim/sparc2-streakcam-sim.odm.yamlsrc/odemis/driver/hamamatsurx.py
427cfa9 to
d28a5a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/odemis/driver/test/hamamatsurx_test.py (1)
836-840: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winWait for the image event instead of sleeping for the full timeout.
Each iteration sleeps for
2 * est_time + 1even when an image arrives earlier. This adds about 88 seconds of avoidable delay to this test. Wait onself.image_receivedwith that duration as the timeout, then clear the event for the next 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 836 - 840, Replace the fixed sleep in the image-wait loop with waiting on self.image_received using 2 * est_time + 1 as the timeout. After each wait, clear self.image_received before continuing to the next image, while preserving the early exit when self.images_left reaches zero.
🤖 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 `@src/odemis/driver/test/hamamatsurx_test.py`:
- Around line 158-159: Add concise plain-text docstrings to
test_synchronization_removal in
src/odemis/driver/test/hamamatsurx_test.py:158-159, the per-test state
initialization method at src/odemis/driver/test/hamamatsurx_test.py:340-342, and
the photon-counting acquisition test at
src/odemis/driver/test/hamamatsurx_test.py:787-789. Document each method’s
purpose without type fields, inline markup, or RST directives.
- Around line 158-159: Remove the skip in test_synchronization_removal and
implement synchronizedOn(None) so synchronized acquisition teardown restores
normal dataflow acquisition. Keep the test active and ensure the existing
synchronization-removal behavior passes without suppression.
- Around line 782-785: In the acquisition test, add an assertion that
self.images_left equals 0 after waiting for completion and before calling
self.readoutcam.data.synchronizedOn(None). Keep the existing _queue_events
assertion, ensuring both trigger consumption and delivery of all expected images
are verified.
- Around line 811-814: Update the exception handling around the test to keep
cleanup in the existing finally block while allowing KeyboardInterrupt to
propagate instead of consuming it. Remove or re-raise from the KeyboardInterrupt
handler so an interrupted test is not reported as passing, and preserve the
photonCounting reset in finally.
- Around line 606-610: The time-range assertions in
src/odemis/driver/test/hamamatsurx_test.py at lines 606-610, 619-622, and
641-644 only verify positivity; update each check after selecting timeRange to
compare the final MD_TIME_LIST value with the selected
self.streakunit.timeRange.value (or the clipped range at that call site) using
get_time_scale_factor(), covering the 2 ns, 1 ms, and restored operate-mode
checks.
---
Nitpick comments:
In `@src/odemis/driver/test/hamamatsurx_test.py`:
- Around line 836-840: Replace the fixed sleep in the image-wait loop with
waiting on self.image_received using 2 * est_time + 1 as the timeout. After each
wait, clear self.image_received before continuing to the next image, while
preserving the early exit when self.images_left reaches zero.
🪄 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: df9f7a9f-aa11-496e-b253-82a0e5a4f0cb
📒 Files selected for processing (1)
src/odemis/driver/test/hamamatsurx_test.py
| # check last value in table is positive and in the expected range relative to the scale factor | ||
| lastCorrectedValue = img.metadata[model.MD_TIME_LIST][-1] | ||
| conversionFactor = self.streakunit.get_time_scale_factor() | ||
| self.assertGreater(lastCorrectedValue / conversionFactor, 0) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '560,660p' src/odemis/driver/test/hamamatsurx_test.py
echo
echo "== relevant symbols =="
rg -n "MD_TIME_LIST|get_time_scale_factor|timeRange|streakunit|lastCorrectedValue" src/odemis/driver/test/hamamatsurx_test.py src/odemis/driver | head -n 120Repository: delmic/odemis
Length of output: 18475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== hamamatsurx relevant time scaling =="
sed -n '560,720p' src/odemis/driver/hamamatsurx.py
echo
rg -n "get_time_scale_factor|MD_TIME_LIST|timeRange|timeList|scaling table|calibration|Streak" src/odemis/driver/hamamatsurx.py
echo "== definitions/usages outside rx =="
rg -n "def get_time_scale_factor|class .*Streak|timeRange|MD_TIME_LIST" src/odemis -g '*.py' | head -n 200Repository: delmic/odemis
Length of output: 34990
Assert the time-range relationship, not only positivity.
lastCorrectedValue / conversionFactor > 0 accepts any positive value. After selecting timeRange, assert that MD_TIME_LIST matches that selected range, for example by checking the final entry against the previous self.streakunit.timeRange.value (or the clipped range at that call site) and get_time_scale_factor() at each of the 2 ns, 1 ms, and restored operate-mode checks.
📍 Affects 1 file
src/odemis/driver/test/hamamatsurx_test.py#L606-L610(this comment)src/odemis/driver/test/hamamatsurx_test.py#L619-L622src/odemis/driver/test/hamamatsurx_test.py#L641-L644
🤖 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 606 - 610, The
time-range assertions in src/odemis/driver/test/hamamatsurx_test.py at lines
606-610, 619-622, and 641-644 only verify positivity; update each check after
selecting timeRange to compare the final MD_TIME_LIST value with the selected
self.streakunit.timeRange.value (or the clipped range at that call site) using
get_time_scale_factor(), covering the 2 ns, 1 ms, and restored operate-mode
checks.
| time.sleep(num_images * 0.2 * 2 + 1) # wait some time for acquisition to finish | ||
| self.assertEqual(len(self.readoutcam._queue_events), 0) | ||
|
|
||
| self.readoutcam.data.synchronizedOn(None) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify that all queued triggers produced images.
An empty _queue_events queue only shows that the driver dequeued the triggers. It does not show that it delivered all expected images. Assert self.images_left == 0 before clearing synchronization.
Proposed fix
time.sleep(num_images * 0.2 * 2 + 1) # wait some time for acquisition to finish
self.assertEqual(len(self.readoutcam._queue_events), 0)
+ self.assertEqual(self.images_left, 0)
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.
| time.sleep(num_images * 0.2 * 2 + 1) # wait some time for acquisition to finish | |
| self.assertEqual(len(self.readoutcam._queue_events), 0) | |
| self.readoutcam.data.synchronizedOn(None) | |
| time.sleep(num_images * 0.2 * 2 + 1) # wait some time for acquisition to finish | |
| self.assertEqual(len(self.readoutcam._queue_events), 0) | |
| self.assertEqual(self.images_left, 0) | |
| self.readoutcam.data.synchronizedOn(None) |
🤖 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 782 - 785, In the
acquisition test, add an assertion that self.images_left equals 0 after waiting
for completion and before calling self.readoutcam.data.synchronizedOn(None).
Keep the existing _queue_events assertion, ensuring both trigger consumption and
delivery of all expected images are verified.
| except KeyboardInterrupt: | ||
| print("Test interrupted by user.") | ||
| finally: | ||
| self.readoutcam.photonCounting.value = False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not convert an interrupted test into a passing test.
The except KeyboardInterrupt block consumes the interruption. The test then completes successfully after finally runs. Keep finally for cleanup, but let KeyboardInterrupt propagate.
Proposed fix
- except KeyboardInterrupt:
- print("Test interrupted by user.")
finally:
self.readoutcam.photonCounting.value = False📝 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.
| except KeyboardInterrupt: | |
| print("Test interrupted by user.") | |
| finally: | |
| self.readoutcam.photonCounting.value = False | |
| 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 811 - 814, Update
the exception handling around the test to keep cleanup in the existing finally
block while allowing KeyboardInterrupt to propagate instead of consuming it.
Remove or re-raise from the KeyboardInterrupt handler so an interrupted test is
not reported as passing, and preserve the photonCounting reset in finally.
d28a5a1 to
0aebcf0
Compare
| min_value_raw, min_unit = min_value.split(' ')[0:2] | ||
| max_value_raw, max_unit = max_value.split(' ')[0:2] | ||
|
|
||
| min_exp = self.parent.convertUnit2Time(min_value_raw, min_unit) |
There was a problem hiding this comment.
Feels like a strange place for such a generic utility method. I know you did not add the method, but since you are using it, maybe good to aggregate such instances on the tech debt board?
There was a problem hiding this comment.
You mean convertUnit2Time() or _getCamExpTimeRange()? Where would you place it which is better? If convertUnit2Time, it's a hamamatsurx-specific function (ie, not meant to handle every way to pass time information). So maybe as a separate function in the module?
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.
0aebcf0 to
fad0ded
Compare
Extend the streak camera to switch to photon-counting mode. The user is expected to have first done the calibration directly on the HPDTA computer.
Also introduce a proper simulator: 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.