Skip to content

merge to upstream repo - #65

Closed
kiwibogo wants to merge 3 commits into
LidkeLab:mainfrom
LiuLabClemson:main
Closed

kiwibogo wants to merge 3 commits into
LidkeLab:mainfrom
LiuLabClemson:main

Conversation

@kiwibogo

Copy link
Copy Markdown
Contributor
  1. add XEM dac
  2. add smarAct stage
  3. add iDS camera

@kiwibogo
kiwibogo requested a review from kalidke September 24, 2026 21:21
@kalidke

kalidke commented Oct 28, 2026

Copy link
Copy Markdown
Member

Thank you for sending this upstream. We're closing this pull request instead of merging it, but we'd welcome a new, narrower one for the IDS camera. Here is what we found and what a mergeable version would need.

Two of the three parts are already in main. The XEM DAC came in with cc83bc9 and the SmarAct stage with #55, so only the IDS camera driver is new here.

As submitted it breaks existing users. setexposuretime! and setroi! are defined as new functions in the IDS module rather than as methods of the camera interface's generics. Once re-exported, the top-level names become ambiguous, so an unqualified setroi!(camera) on a DCAM4 camera throws UndefVarError for every downstream using MicroscopeControl. With this branch merged into main, the suite has 2 failures and 12 errors, all from those two names (the contract test's ambiguous-export and shadowed-generic checks, and the generated API map).

The driver has six defects in its acquisition and failure paths:

  1. The frame copy length comes from the mutable camera.roi, not from the delivered frame's size. Changing the ROI during live view makes the next getlastframe read past the SDK buffer.
  2. initialize sets library_initialized = true before device discovery and never cleans up. With no camera attached it throws; after you connect one and retry, the early return hands back a null handle.
  3. sequence sets is_running, and only getdata/abort clear it, so a poll loop never ends. Preview reads also consume the frames that getdata then waits for.
  4. The frame timeout is 3000 / frame_rate ms and ignores the exposure, so a 1 s exposure at a cached 20 fps times out at 150 ms.
  5. A failed read leaves the SDK acquisition running while the object says it has stopped, so the next start can fail.
  6. trigger_mode is never applied and has no setter, yet export_state records it.

We found no ABI mistake in the SDK calls the driver uses: they match the vendor header.

What a new pull request would need:

  • based on current main (now 0.2.7-DEV);
  • setexposuretime! and setroi! as methods of the existing camera generics, and setframerate! not exported at top level;
  • the six fixes above;
  • a test that drives the real driver code against a fake SDK, in the style of test/tcube_fake_sdk.jl, so the suite runs with no camera attached.

The small fix to the shared camera panel (camera_interface/gui.jl: passing camera into the menu callback) fixes a bug that main still has. It is welcome on its own as a separate pull request. If you send it, please also route each of the two menus to its own field: as written, both still assign capture_mode, so choosing a trigger mode throws.

@kalidke kalidke closed this Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants