Audit the ophyd-async devices and their mock tests by sligara7 · Pull Request #88 · NSLS2/hextools

Anthony Sligar (sligara7) · 2026-09-21T15:04:06Z

Audits the ophyd-async devices and their mock tests. Bluesky plans are otherwise out of scope and will follow in a separate PR; the only plan changes here are the ones needed to make CI pass again after main (#89) was merged in.

Why

The suite could not be collected at all. tests/test_motors.py imported DoubleObjCamera, which was renamed to FOV_2_4_mm_Camera when motors.py was split, and a stale import at collection time aborts the whole run rather than failing one file. That is why the Unit Tests job has been reporting nothing since 17 Sep rather than reporting failures.

Underneath that, five tests were failing and two detectors had no tests at all.

Results

	before	after
suite	aborts at collection	153 passed, 0 failed (Python 3.11, 3.12, 3.13); Lint and Type Check clean
Phantom	13 tests, 4 failing	17, all passing
GeRM	3 tests, 1 failing	6
Kinetix	0	6
Perkin Elmer	0	0 — the factory and its tests were dropped at review
SampleTower / OpticsTable / RotationMotor	0	6
NSLS2StorageRing	0	2

Verified in both the dev and py313 environments.

Device fixes

Each of these replaces a failure that presents as something else:

germ.py had from tkinter.font import names — an accidental editor auto-import that runs at import time. On a machine whose Python was built without Tk (common in minimal conda environments and containers) import hextools raises ImportError before any beamline code runs.
Phantom wait_for_idle indexed its cine DeviceVector with a raw value off the IOC, so an out-of-range number surfaced as a bare KeyError at the operator's console. It now says what was reported, what the device was built with, and where to look.
Phantom start_acquiring waited for the array counter to be exactly equal to the post-trigger count. That counter is monotonic and watched over Channel Access, where updates coalesce — a missed value meant a good acquisition fell into the timeout branch and reported a cine-write failure that had not happened. Now >=.
Phantom start_acquiring also broke out of its trigger retry loop when the value stream ended, not only when the trigger arrived, so an exhausted stream read as a successful trigger. The first fix for that could then spin: an ended stream re-subscribed without ever reaching the acquisition-stopped check (Copilot). An ended stream is now handled as a timeout; the test for it hangs on the old code.
Phantom's post-trigger timeout fallback still rejected any array counter other than an exact match, although the main wait now accepts >=. A missed update past the target is now complete there too; only a shortfall raises (Copilot).
GeRM prepare_internal documented livetime=0 as "keep what is configured" and then set 0 regardless, wiping the exposure time. Now guarded the way PhantomTriggerLogic.prepare_internal already does.
calculate_scan_time compared its two optional arguments with == where it meant !=, so it rejected both legal calls and accepted the one it forbids, leaving the motion-limited branch unreachable.
Two print() calls in library code that wrote to the operator's console mid-scan are removed; the values they carried now go in the timeout message, where they are useful.

Test notes

test_germ_detector_describes_elements_then_energy_bins is the one worth a look. Nothing anywhere constructed GeRMDetector — test_germ.py imported the class and never used it. This test pins the frame as (elements, energy bins). I verified it by reintroducing a transposed NDArrayDescription, watching the test fail, and restoring it.

The three drifted Phantom tests were describing an older frame-counting version of the download path; they now drive the signals the implementation actually uses. test_detector_full_stack additionally left the Proc plugin's num_filter at the mock default of 0, which divided by zero inside ophyd-async; a real NDPluginProcess reports at least 1. Its final assertion also expected (11, 3, 4) where the stream holds (1, 11, 3, 4) — one event carrying 11 frames, consistent with the descriptor shape and single stream_datum the same test already asserts.

test_germ_acquire_logic asserted device state after a fixed asyncio.sleep(0.2) and only ever exercised start_acquiring. It now drives the transition explicitly and awaits wait_for_idle, which had no test.

Since main (#89) was merged in

ensure_shutter_state compared the new ShutterStatus enum with a bool, so it never matched: a shutter already open raised when actuation was not allowed, and was commanded again when it was. Fixed in shutter.py, written the same way as the fix on jwlodek/main (051df65) so that change merges over it cleanly; new tests in test_ensure_shutter_state.py, four of six fail on the old comparison.
Five plan tests failed after the merge (4 alignment-scan, 1 radiograph), fixed in 49a64ca. The tests set the shutter readback to a bool, and tomo_alignment_scan tested the enum for truth: both members are non-empty strings, so a closed front-end shutter never stopped the scan and a closed photon shutter was never opened. It now compares against ShutterStatus.OPEN.
take_radiograph waited time_gap once more after the last acquisition, because a scalar delay makes bp.count repeat it forever. It now passes num_acquisitions - 1 delays (Copilot). Note: 051df65 on jwlodek/main keeps the trailing wait and its test expects it, so the two will conflict there.
Type Check and Lint (d8fefdb): tomo_flyscan's device list is annotated list[Flyable] for bp.fly; unused imports removed; RotationMotor and tomo_flyscan in profiles/collection.py kept as prompt re-exports with noqa markers, like the file's others.
Dropped at review: perkin_elmer_factory. The PE stays constructed inline in profiles/collection.py, as it was.

Not in this PR

flyers.py still has no tests — it sits on the device/plan boundary and belongs with whichever half you prefer.
The two invalid-class-name hits on FOV_2_4_mm_Camera / FOV_20_40_mm_Camera are left alone, since renaming them would churn the profile.
FRAME_PERIOD_MARGIN is still one constant carrying four detectors' values in trailing comments. Where per-detector readout headroom should live is a design question, not something to settle inside a coverage change.

motors.py renamed DoubleObjCamera to FOV_2_4_mm_Camera when the module was split, and tests/test_motors.py was not updated. The stale import raises at collection time, which aborts the ENTIRE suite rather than failing one file, so no test results are reported at all. Pure rename; the test bodies are unchanged. FOV_2_4_mm_Camera keeps the same signals the tests use (_obj_selector_home_sts, _at_left_objective, _at_right_objective) and still defaults its name to "double_obj_camera". All 3 motors tests pass. Full suite now collects: 5 failed, 121 passed, where previously it reported nothing.

The guard compared the two optional arguments with == where it meant !=, so it rejected exactly the two cases its own error message calls legal and accepted the one it calls illegal: neither given -> raised "must be provided together or not at all" both given -> raised the same only one given -> returned a result, silently ignoring it That made the motion-limited branch below it unreachable: travel_distance / max_velocity could never be evaluated. Nothing calls this function yet and nothing tested it, which is why the error survived. Adds tests/detectors/test_detector_utils.py covering all four argument combinations. Test file is named test_detector_utils.py rather than test_utils.py because tests/ has no __init__.py, so pytest derives module names from basenames and a second test_utils would collide with tests/test_utils.py at collection.

Removes a stray 'from tkinter.font import names' in germ.py that ran at import time and would raise ImportError on any machine whose Python was built without Tk, plus four other unused imports there, an unused epics_signal_rw_rbv in phantom.py, import ordering, and blank lines after docstrings in shutter.py. Mechanical only - no behaviour change. Suite unchanged: 5 failed, 121 passed.

Phantom, wait_for_idle: the selected cine index came straight off the IOC and was used to index a DeviceVector keyed 1..num_cines, so an out-of-range value raised a bare KeyError at the operator's console. Check it and say what was reported, what the device was built with, and where to look. Phantom, start_acquiring: waiting for the post-trigger frames compared the array counter for EXACT equality. That counter is monotonic and watched over Channel Access, where updates coalesce - a missed value meant the match never happened and a good acquisition fell into the timeout branch, reporting a cine-write failure that had not occurred. Use >=. Phantom, start_acquiring: the trigger wait broke out of its retry loop when the value stream ENDED, not only when the trigger arrived, so an exhausted stream read as a successful trigger. Track it explicitly. GeRM, prepare_internal: the docstring says livetime=0 means "keep what is set" and the code set 0 regardless, wiping the configured exposure time. Guard it the way PhantomTriggerLogic.prepare_internal already does. Also drops two debug print() calls that wrote to the operator's console mid-scan, and folds the counts they carried into the timeout message where they are actually useful.

…MDetector The five failing tests were describing behaviour the devices no longer have. Phantom download path had NO passing test at all - the three that covered it all failed. wait_for_idle now completes on the selected cine's cine_content_saved signal rather than counting frames, and start_acquiring grew a guard on how many frames the camera actually holds; the tests still described the older frame-counting version. Updated to drive the real signals, and added coverage for the two guards: an out-of-range cine index, and asking for more frames than were recorded. test_detector_full_stack additionally left the Proc plugin's num_filter at the mock default of 0, which divided by zero inside ophyd-async when the detector read back its own state. A real NDPluginProcess reports at least 1. Its final assertion also expected (11, 3, 4) where the stream holds (1, 11, 3, 4) - one event carrying 11 frames, consistent with the descriptor shape and single stream_datum the same test already asserts. test_germ_acquire_logic asserted device state after a fixed 0.2s sleep and only ever exercised start_acquiring. It now drives the Acquire transition explicitly and awaits wait_for_idle, which had no test before. NEW, and the one that matters: test_germ_detector_describes_elements_then_ energy_bins constructs GeRMDetector - which nothing did - and pins the frame as (elements, energy bins). Verified by reintroducing the transposed NDArrayDescription and watching this test fail, then restoring it. Also pins the livetime=0 contract on GeRM prepare_internal. Full suite: 131 passed, 0 failed.

Neither detector had a single test. Both are thin wrappers over stock ophyd-async classes, so what needs pinning is the HEX wiring - the PV prefix and the plugin chain - not the upstream driver. Kinetix (6 tests): the PV prefix for each detector number, that Proc1 is wired so frames can be averaged, and that the name survives construction (it becomes the asset directory, so a mangled one misfiles data). Perkin Elmer: there was no hextools code to test at all - the detector was constructed inline in profiles/collection.py, which pytest excludes. Adds detectors/perkin_elmer.py with perkin_elmer_factory, mirroring kinetix_factory, and points the profile at it so the wiring has one home. That makes the plugin chain explicit rather than relying on a default: HDF behind Proc1 behind CB1, which is what the free-running PE needs. 3 tests cover the prefix, both plugins, and the name. NOTE FOR REVIEW: perkin_elmer_factory is the only new public API in this branch. If Jakub would rather the PE stayed inline in the profile, this commit can be dropped on its own - nothing else depends on it. Full suite: 140 passed.

None of these devices had a test. SampleTower: pins that a read records the three VIRTUAL axes (y, pitch, roll) and not the seven real motors that combine to give them. Ten axes in every event document would bury the three that mean something, and the real motors stay reachable as attributes for alignment work. That intent lived only in a comment. OpticsTable: pins all ten axes appear in a read. RotationMotor: counts per revolution is 360 degrees times the encoder resolution. tomo_flyscan positions by encoder count, so this conversion decides where a rotation scan actually starts. NSLS2StorageRing: pins the facility DCCT PV, which is a contract with the accelerator rather than something HEX can rename - the beam-drop suspender watches it - and that beam current is reported as configuration. Full suite: 148 passed.

ContAcqDetector gets its hdf attribute dynamically from the writer factory, which the type checker cannot see, so touching it in the fixture added a diagnostic. None of the three PE tests need it - they check the PV prefix, the plugin chain and the name. Type diagnostics: 75 -> 74, which is main's count. This branch now adds none.

Copilot

Copilot review overview

🟡 Changes recommended

The new RotationMotor test calls an instance method as an unbound method with None for self, which is incorrect usage and likely to fail ty check in CI.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Call get_encoder_counts_per_rev on a RotationMotor instance · New

What changed in this PR

This PR restores test suite collectability and expands mock-based coverage for multiple ophyd-async devices at HEX, while fixing several device-level behaviors that previously surfaced as misleading runtime errors/timeouts.

Changes:

Fix Phantom and GeRM acquisition/idle logic edge cases and improve timeout/error reporting.
Add/repair mock tests for motors, storage ring, detector factories (Kinetix/Perkin Elmer), and shared detector utilities.
Introduce perkin_elmer_factory and update the collection profile to use it.

File	Description
tests/test_motors.py	Updates camera class import and adds coverage for SampleTower/OpticsTable reads and RotationMotor encoder conversion.
tests/test_machine.py	Adds mock tests for `NSLS2StorageRing` PV contract and configuration readback.
tests/detectors/test_phantom.py	Aligns Phantom tests with updated trigger/download semantics and adds regression cases for frame availability and cine selection.
tests/detectors/test_perkin_elmer.py	Adds tests validating Perkin Elmer factory wiring and PV prefix/name behavior.
tests/detectors/test_kinetix.py	Adds tests validating Kinetix factory PV prefixing, proc plugin wiring, and naming.
tests/detectors/test_germ.py	Expands GeRM trigger/acquire tests and adds detector description shape regression coverage.
tests/detectors/test_detector_utils.py	Adds tests covering `calculate_scan_time` argument contract and expected timing.
src/hextools/profiles/collection.py	Switches Perkin Elmer construction to `perkin_elmer_factory`.
src/hextools/photon_delivery_system/shutter.py	Modernizes `Hashable` import and minor formatting cleanups.
src/hextools/photon_delivery_system/dclm.py	Minor formatting cleanup in docstring area.
src/hextools/detectors/utils.py	Fixes `calculate_scan_time` validation logic for optional velocity/distance arguments.
src/hextools/detectors/phantom.py	Fixes trigger wait loop semantics, relaxes post-trigger equality to `>=`, improves cine validation and timeout diagnostics, removes console `print()`.
src/hextools/detectors/perkin_elmer.py	Introduces `perkin_elmer_factory` with explicit proc/CB plugin chain.
src/hextools/detectors/kinetix.py	Minor import/docstring formatting cleanup around factory.
src/hextools/detectors/germ.py	Removes accidental Tk import, fixes `livetime=0` behavior, removes console `print()`, tidies imports.
src/hextools/detectors/__init__.py	Comment formatting cleanup for `FRAME_PERIOD_MARGIN`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🟢 Approval recommended

The changes are well-scoped to fixing device behavior and restoring/expanding tests, with only a minor maintainability nit identified in a new unit test.

Review effort: Lite
Findings: 1

Open (1)

Call get_encoder_counts_per_rev on a RotationMotor instance

Passing None for self worked at runtime because the method does not touch it, but it is wrong usage and ty rejected it: Expected `RotationMotor`, found `None` (tests/test_motors.py:109) Construct the motor under init_devices(mock=True) and call the method on it. The assertion sits OUTSIDE the context manager, since init_devices connects the device on exit. Type diagnostics 74 -> 73, one below main.

Anthony Sligar (sligara7) · 2026-09-21T15:15:24Z

Good catch — fixed in 5023faf.

Confirmed it was not just style: ty rejected it outright.

tests/test_motors.py:109:50
    RotationMotor.get_encoder_counts_per_rev(None, encoder_resolution)
                                             ^^^^ Expected `RotationMotor`, found `None`

I took the instance approach as suggested, with one change to the placement: the assertion goes outside the init_devices(mock=True) block rather than inside it. init_devices connects the devices on __exit__, so asserting inside the block runs against an unconnected device. The test also needs the RE fixture, since init_devices requires the bluesky event loop.

def test_rotation_motor_counts_per_rev(
    RE, encoder_resolution: float, expected_counts_per_rev: int
):
    with init_devices(mock=True):
        motor = RotationMotor("TEST:ROT:")

    assert (
        motor.get_encoder_counts_per_rev(encoder_resolution)
        == expected_counts_per_rev
    )

Type diagnostics for the branch go 74 -> 73, one below main. All 9 motors tests pass; full suite still 148 passed.

Copilot

Copilot review overview

🔵 Needs a closer look

The new public perkin_elmer_factory has a docstring/API mismatch (mentions cb_suffix as configurable when it is hard-coded), which should be corrected before merge.

Review effort: Lite
Findings: None

Resolved since last review (1)

Call get_encoder_counts_per_rev on a RotationMotor instance

Previously missed (1)

In code that hasn't changed since last review

Docstring incorrectly describes hard-coded cb_suffix as configurable

src/hextools/detectors/perkin_elmer.py:15

The docstring mentions cb_suffix as if it were a configurable parameter, but the factory doesn’t accept it; it is hard-coded to CB1: below. This is confusing for API users—either expose cb_suffix as an argument or describe the fixed suffix in the docstring.

get_encoder_counts_per_rev returned int(360.0 * encoder_resolution). encoder_resolution is the motor record's ERES, whose own definition is "Encoder Step Size (EGU)" - the size of ONE count, in degrees. Counts per revolution is therefore 360 divided by it, not multiplied. int(360.0 * 0.0009) = 0 <- what it returned at a fine encoder int(360.0 / 0.0009) = 400000 <- correct It returned ZERO rather than a plausible wrong number: 0.324 truncates to 0 for any encoder finer than about 0.0028 deg/count. Cross-checked against a beamline measurement - 2000 counts is about 1.8 degrees, and 2000 x 0.0009 is exactly 1.8, which fixes the units independently of any docstring. Nothing reads the derived signal yet, so no plan was affected. THE TEST NOW PINS THE INVARIANT, NOT THE OUTPUT. It asserts that a revolution's worth of counts times the size of a count is 360 degrees, within one count of truncation. The previous test in this branch asserted specific outputs and passed the wrong formula happily - which is how a test can make a defect harder to fix rather than easier. Verified by reintroducing the multiply: 3 of the 4 cases fail. The fourth is ERES 1.0, where multiplying and dividing agree and no test could tell them apart. Suite: 148 passed. Type diagnostics unchanged at 73.

Copilot

Copilot review overview

🟢 Approval recommended

The changes are coherent with the PR goals (restoring collection and expanding mock-test coverage) and the only noted feedback is a minor test-name grammar nit.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Use plural agreement: “velocity and distance are allowed”

tests/detectors/test_detector_utils.py:11

Grammar: when referring to two items (“velocity” and “distance”), the test name should use “are allowed” rather than “is allowed”.

Jakub: "I don't think we need a factory function here. There's only one of these detectors." His call, and a fair one - a factory parameterised by detector number is over-general for a single instrument. Removes src/hextools/detectors/perkin_elmer.py and its tests, and restores the inline ContAcqDetector construction in profiles/collection.py exactly as it was. Done surgically rather than by reverting 8a434ec, because that commit also carries the Kinetix tests, which are not in question. Verified I added no lint findings by diffing the finding SET for collection.py before and after, not the count. WHAT THIS COSTS, stated so it is not lost: the Perkin Elmer is back to zero test coverage. The factory existed only because the PE's wiring lives in profiles/collection.py, which pytest excludes, so there was no hextools code to test. Removing it does not change that - it just stops working around it. The real question is whether the profile should be reachable from tests at all, which is a bigger decision than this PR. Suite 145 collected, 144 passed; the one failure is the pre-existing radiography flake, which fired locally here for the first time today after passing 10/10 this morning.

Copilot

Copilot review overview

🔵 Needs a closer look

One updated Phantom error-path message can still raise an unintended exception in an edge case (empty cines) and should be made robust before approval.

Review effort: Lite
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Handle empty cines before calling min() or max()

src/hextools/detectors/phantom.py:526

This ValueError message uses min()/max() on self.driver.cines; if PhantomIO is ever constructed with num_cines=0 (or cines is otherwise empty), min/max will raise before your intended error is raised. Using a safe default avoids an unexpected exception in that edge case.

Correct test name grammar for multiple parameters

tests/detectors/test_detector_utils.py:11

Grammar: the test name refers to two parameters, so "are allowed" reads correctly and avoids a minor wording error in the public test suite.

This test has been failing intermittently since at least 2026-09-18 and is the only thing keeping this branch's CI red. It failed locally 1 run in 5, on CI it failed on a DIFFERENT PAIR of Python versions each run (3.11+3.13 one run, 3.11+3.12 the next, with the third passing), and it fired locally here today after passing 10 consecutive runs this morning. CAUSE, recorded 2026-09-18 and confirmed here: bps.repeat emits a sleep only when delay-minus-elapsed is still positive, so whether a sleep appears at all is a race against how fast the machine ran the acquisition. FIXED BY REMOVING THE RACE, not by loosening the assertion. _FrozenClock stands in for the time module bps.repeat measures elapsed time with, so the full delay always survives. The only two uses of time in bluesky.plan_stubs are the pair inside that calculation, so nothing else is affected. 20 consecutive runs pass. AND IT WAS ASSERTING THE WRONG NUMBER. With the race gone the count is num_acquisitions, not num_acquisitions - 1: take_radiograph passes a SCALAR delay to bp.count, which becomes itertools.repeat - an iterator that never exhausts - so a sleep is emitted after every acquisition INCLUDING THE LAST. The old assertion only passed when timing happened to swallow exactly one sleep, which is why it failed in both directions - 3 observed on 09-18, 0 on CI, 5 here. That trailing wait is real behaviour, not a test artefact: the plan waits time_gap once more after the final frame. Harmless at 0.01s, a real cost for any sizeable gap. Recorded rather than changed - altering the plan is out of scope for this branch. The magnitude assertion is now exact rather than "<= wait", so if bluesky ever measures elapsed time some other way this fails loudly instead of quietly going back to being a race. Suite: 145 passed.

Copilot

Copilot review overview

🔵 Needs a closer look

The updated encoder counts-per-revolution logic in RotationMotor introduces a potential ZeroDivisionError without input validation and should be hardened before approval.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Validate encoder resolution before division

src/hextools/motors.py:144

get_encoder_counts_per_rev() now divides by encoder_resolution, which will raise a ZeroDivisionError (or return nonsense for negative values) if the motor reports 0 or an invalid step size. Since this behavior changed from multiplication to division in this PR, it would be safer to validate the input and raise a clear ValueError.

Copilot

Copilot review overview

🟡 Changes recommended

PhantomAcquireLogic.start_acquiring() can still spin/hang if observe_value() ever terminates normally without yielding a trigger, so the trigger-wait loop should explicitly treat stream exhaustion as a retryable timeout.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Handle exhausted observe_value stream as timeout · New

All 61, and pre-commit now passes every hook. MOSTLY MECHANICAL: 24 auto-fixable (import ordering, unused imports outside the profile, blank lines, trailing whitespace), 12 long lines wrapped, 11 missing docstrings written - the ten GeRM enums now say what each mode is for rather than just naming it, which is worth having on its own. THREE JUDGEMENT CALLS, all kept rather than "fixed": 1. profiles/collection.py imports. A beamline profile's "unused" imports are frequently the names staff type at the prompt, and deleting them would break an operator's session with a NameError. Checked each: bp, bps, bpp and show_docs are namespace re-exports; NDStatsIO, PluginSignalDataLogic and SuspendFloor are referenced by commented-out blocks that are clearly meant to come back. All kept, each with the reason on the noqa. Only Path and KinetixDetector were removed - neither appears anywhere in the file, not even in a comment. 2. FOV_2_4_mm_Camera and FOV_20_40_mm_Camera do not match CapWords, and the names state the field of view in millimetres. Renaming would churn the profile and every caller to satisfy a convention the names are breaking deliberately. Silenced with the reason. 3. perkin_elmer._name is a private poke with a TODO already explaining it. Silenced pointing at that TODO rather than removing a workaround nobody has replaced. Also moved FRAME_PERIOD_MARGIN below the imports, where it stops being a module-level statement before an import block, added it to __all__ since radiography.py imports it, and left its TODO intact: it is still one constant carrying the Kinetix value while three other detectors want their own. Suite: 145 passed. Verified the profile still imports every name staff use.

Copilot

Copilot review overview

🟡 Changes recommended

There are CI-relevant maintainability/type-check concerns (notably the RunEngine metadata typing and union-annotation formatting) that should be addressed before merging.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

Resolve RunEngine metadata mapping type-check error · New
Handle exhausted observe_value stream as timeout
Keep shutter union annotations on one line · New

Conflict resolutions: - detectors/__init__.py: keep FRAME_PERIOD_MARGIN below the imports, documented and in __all__. - motors.py: both sides fixed get_encoder_counts_per_rev the same way (360 / ERES); keep the explanatory comment. SampleTower.ry2 from main is kept. - photon_delivery_system/shutter.py: ShutterStatus/StrictEnum from main, import tidy-up from this branch. - profiles/collection.py: follow main in dropping the PYEPICS_LIBCA workaround and the perkin-elmer name override; keep this branch's import clean-up. - tomography/flyscans.py: main's bp.fly rework, with this branch's import order. - tests/test_motors.py: main's fov_2_4_mm_camera fixture name, plus this branch's SampleTower, OpticsTable and RotationMotor tests.

Copilot

Copilot review overview

🟡 Changes recommended

Phantom overshoot/stream handling and coverage, radiography trailing-delay behavior, and the advertised Perkin Elmer factory remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 4 · 1

Open (7)

Resolve RunEngine metadata mapping type-check error
Handle exhausted observe_value stream as timeout
Accept overshoot values in timeout fallback · New
Add and use the advertised perkin_elmer_factory · New
Avoid trailing delay after final acquisition · New
Do not test or emit a final inter-acquisition delay · New
Keep shutter union annotations on one line

Shutter.status became a ShutterStatus enum ("Open" / "Not Open"), but ensure_shutter_state still compared it with the bool the caller passes, so the check never matched: a shutter already in the desired state raised when actuation was not allowed and was commanded again when it was. Convert the desired state to a ShutterStatus before comparing. The comparison is written the same way as the fix on jwlodek/main (051df65), so that change merges over it cleanly. New tests cover both directions; four of the six fail on the old comparison. Also tidies the module's lint (blank lines, a long line, a docstring for ShutterStatus).

…cept overshoot after a timeout Two paths Copilot flagged in review: - If the trigger_received stream ended without the trigger and without timing out, the retry loop re-subscribed without ever reaching the acquisition-stopped check, spinning a CPU core. An ended stream is now handled as a timeout. - The post-trigger-frame wait accepts array_counter >= post_trig_frames, but its timeout fallback still rejected any value other than an exact match. A missed update past the target now counts as complete there too; only a shortfall raises. Each has a test that fails on the old code (the first hangs).

Unused import and docstring spacing in flyers.py, top-level blank lines in motors.py, a long line in test_dclm.py. Errors in plans, the profile and utils.py are left for the plans PR.

Copilot

Copilot review overview

🔵 Needs a closer look

Unresolved findings remain in Phantom stream handling, shutter compatibility, radiography timing, and typing; the advertised Perkin Elmer API/tests are also absent.

Review effort: Lite
Findings: 1 · 3 · 1

Open (5)

Resolve RunEngine metadata mapping type-check error
Do not test or emit a final inter-acquisition delay
Avoid trailing delay after final acquisition
Add and use the advertised perkin_elmer_factory
Keep shutter union annotations on one line

Resolved since last review (2)

Handle exhausted observe_value stream as timeout
Accept overshoot values in timeout fallback

Previously missed (1)

In code that hasn't changed since last review

Missing advertised Perkin Elmer factory and tests

src/hextools/detectors/kinetix.py:8

The PR description says this change introduces perkin_elmer_factory and three Perkin Elmer tests, but this snapshot contains no such symbol or test; profiles/collection.py still constructs ContAcqDetector inline. Please include that API/test if it is part of this PR, or correct the stated results and scope.

…last radiograph acquisition These are the failures main (NSLS2#89) brought into this PR's CI. - tomo_alignment_scan tested both shutter readbacks for truth. Both ShutterStatus members are non-empty strings, so a closed front-end shutter never stopped the scan and a closed photon shutter was never opened. Compare against ShutterStatus.OPEN. - take_radiograph passed a scalar time_gap to bp.count, which repeats it after the final acquisition too. Pass num_acquisitions - 1 delays so the gap is only between acquisitions (Copilot review). - take_radiograph's shutter annotations back on one line each, with the explanatory comments above (Copilot review). - The alignment and radiograph test shutters set ShutterStatus rather than bools and start CLOSED (a mock enum signal starts at its first member, OPEN).

- tomo_flyscan: annotate the device list as list[Flyable]; bp.fly takes a list[Flyable] and list is invariant, so the inferred union list was rejected. - Remove unused imports in flyscans.py. - profiles/collection.py: RotationMotor and tomo_flyscan are unused in the file but come from the flyscan commit, so they stay as prompt re-exports with noqa markers like the file's other ones; import order and comment spacing. - tomography/__init__.py import order and trailing newline; utils.py blank line.

Copilot

Copilot review overview

🔵 Needs a closer look

The Phantom post-trigger observer can treat normal stream exhaustion as a successful acquisition and needs correction.

Review effort: Lite
Findings: None

Resolved since last review (5)

Resolve RunEngine metadata mapping type-check error
Do not test or emit a final inter-acquisition delay
Avoid trailing delay after final acquisition
Add and use the advertised perkin_elmer_factory
Keep shutter union annotations on one line

Brings the branch up to 4e69321. Conflicts resolved by keeping main's new code (EDXD, GeRM data logics, Kinetix and shutter mocks, ensure_available, n-D tomo scans) and this PR's device fixes, all of which still apply on main: shutter enum comparisons, Phantom trigger/array-counter handling, GeRM livetime=0, calculate_scan_time, no tkinter import, no trailing radiograph wait. Also restores DetectorDataLogic and ADBaseIO in germ.py (the auto-merge dropped them; main's new code uses them) and keeps the bp/bpp imports in flyscans.py, which main uses without importing. Known failures inherited from main, not touched here: edxd.py does not parse (line 102), collection.py imports Collimator (the class is CollimatorTable), and test_dclm.py passes fs_camera, renamed to fs_window_cam. Assisted-by: copilot:claude-opus-4-8

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical Phantom stream handling and duplicate or unused imports remain, including lint-blocking issues.

Review effort: Lite
Findings: 4

Open (4)

Handle exhausted observer stream as a timeout · New
Remove duplicate ophyd_async.core imports · New
Remove unused GeRMStatsDataLogic import · New
Remove duplicate tomo_flyscan import · New

            async for actual_post_trig in observe_value(
                self.driver.array_counter, done_timeout=DEFAULT_TIMEOUT
            ):
-                if target_post_trig == actual_post_trig:
+                if actual_post_trig >= target_post_trig:


 )
 from bluesky.protocols import Reading, Subscribable
 from bluesky import plan_stubs as bps
+from ophyd_async.core import AsyncMovable, AsyncStatus, StrictEnum, wait_for_value


+from bluesky import plan_stubs as bps  # noqa: F401
+from bluesky import plans as bp  # noqa: F401
+from bluesky import preprocessors as bpp  # noqa: F401 - re-exported for the prompt
 from hextools.detectors.germ import GeRMDetector, GeRMStatsDataLogic


 from .flyscans import tomo_flyscan, tomo_1d_step_scan, tomo_2d_step_scan, tomo_nd_step_scan

 from .alignment import tomo_alignment_scan
+from .flyscans import tomo_flyscan


Anthony Sligar (sligara7) added 7 commits September 21, 2026 10:47

Copilot AI lite review requested due to automatic review settings September 21, 2026 15:04

Copilot started reviewing on behalf of Anthony Sligar (sligara7) September 21, 2026 15:04

View session