Panels must not command hardware on open; DCAM4Camera throws instead of returning a non-camera - #60
Merged
Merged
Conversation
…of returning a non-camera Both fixes were requested by the downstream rig repo as its top two blockers. Light source panel: setpower and light_on/light_off were wired to their widgets with `lift`, which evaluates immediately when created, so merely opening the panel pushed the slider's initial value (0.5) to the device and commanded the on/off state. On a 642 nm laser that floods a quarter of the camera chip. Side effects now use `on`, and both widgets initialise from the device's current power and on/off state instead of hardcoded defaults, so constructing a panel is observably read-only. The one remaining `lift` only derives the displayed On/Off string. DCAM4Camera: dcamapi_init failure returned a DCAMERR and dcamdev_open failure returned nothing with the SDK left initialised, so callers that expect a camera or a throw got neither, and a second open in that state crashed. Both paths now throw with the device id and the DCAMERR, and the open-failure path uninitialises the SDK first. Adds test/gui.jl asserting that constructing a panel changes no device state. The Sim stages are @test_broken there: the shared panel reads `stagelabel` while they carry `label`, which is a separate known defect. Hardware verification: NOT DONE for the DCAM4 change; no Hamamatsu camera here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… cleanup claim The state-snapshot test could not detect the defect it guarded. SimLight starts at power 0.0 and off, which is what the widgets now initialise from, so reverting both callbacks to `lift` re-issued setpower(light, 0.0) and light_off(light) while every assertion still passed. Verified: against the reverted code the old assertions pass and only the new ones fail. test/gui.jl now uses a RecordingLight that logs every setpower/light_on/ light_off call and asserts construction issues none, across three starting states a snapshot cannot probe: on at construction, stored power off the slider grid, and stored power outside the range. The Sim stage catch is narrowed to the known stagelabel FieldError and closes windows in a finally. Also narrows the shipped claim that the DCAM4 SDK is always uninitialised on a failed construction: cleanup is attempted only on the two checked error returns, an exception after a successful open has no guard, and dcamapi_uninit can itself fail. Qualifies the panel audit, since the DAQ panel does query devices and channels at construction and the disabled objective-positioner panel calls initialize. Adds the caveat that only SimLight updates properties.power in setpower, so the slider opens from cached state that may not match the device. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FieldError only exists from Julia 1.12; on 1.11 the same missing-field access raises a plain ErrorException, so the narrowed catch failed to compile there and CI errored on the 1.11 job. Match on the message instead, which stays just as narrow on both versions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merge-gate follow-ups, none touching production source.
The Sim-stage `@test_broken` catch accepted any exception whose message
mentioned `stagelabel`, so an unrelated failure in `gui(::Stage)` would have
been absorbed as the expected missing-field error. It now requires the
receiver type and the field name to appear in that order, which stays
portable to Julia 1.11 (where the access raises a plain `ErrorException`
rather than a `FieldError`). Verified by injecting
`error("unrelated stagelabel rendering failure")` into `gui(::Stage)`: the
testset now errors out instead of reporting 10 pass / 3 broken.
Three claims were wider than the code:
- The DCAM4 cleanup is only *attempted*. `dcamapi_uninit()` does check its
own SDK result and logs an `@error`, but the constructor ignores that
return and throws either way, so a failed uninitialize is visible only in
the log. The earlier wording said the call itself was unchecked.
- "Opening any panel is now observably read-only" was a blanket guarantee
in three places. The fix is scoped to the shared light panel; `gui(::DAQ)`
still queries `showdevices`/`showchannels` at construction.
- "Only `SimLight` updates `properties.power`" is false: `TCubeLaser`
updates it too, with a calculated power, which the same sentence went on
to say. What differs by driver is *what* the field holds, so the caveat
now says that instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate showed the matcher had no boundary after `stagelabel`, so a *different* missing field whose name merely starts with it was still absorbed: replacing `gui(::Stage)` with a `:stagelabel_extra` access left the testset reporting 3 passed / 3 broken on both Julia 1.11.9 and 1.12.7. The tail is now a negative lookahead rather than an optional backtick, which keeps 1.11 (no backticks, field name ends the message) and 1.12+ (backtick follows) both matching while rejecting a longer name. Verified against all three cases: `stagelabel_extra` and an unrelated error mentioning `stagelabel` are now rethrown, and the genuine missing-field error is still caught (10 passed / 3 broken). `driver-caveats.md` still carried the "can fail silently" wording for `dcamapi_uninit()` that was corrected in the two SKILL.md copies. It does check its own result and log; the constructor is what ignores the return. Suite unchanged: 782 passed, 8 broken, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An identifier character class cannot bound a Julia field name: `stagelabel!` and `stagelabelα` are both legal, and the gate showed both were still being absorbed as the expected missing-field error on 1.11.9 and 1.12.7 alike. The field name is now delimited by what actually follows it in each message — end of string on 1.11, or the closing backtick and ", available fields:" from 1.12 — so no longer name can match. Verified two ways: 36 checks of the matcher against both message shapes for every receiver type and four longer field names, and the live testset against all five mutations, where `stagelabel_extra`, `stagelabel!`, `stagelabelα` and an unrelated error mentioning `stagelabel` each rethrow while the genuine error is still caught. Suite unchanged: 782 passed, 8 broken, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
No pattern over the message text can bound a Julia field name. The gate defeated three successive attempts: an identifier character class lets `stagelabel!` and `stagelabelα` through, and keying on the surrounding message text lets `var"stagelabel "` through on 1.11 (the trailing space is part of the name) and ``var"stagelabel`, available fields:extra"`` through on 1.12+ (the field name contains the delimiter). All are legal field names. So stop matching text. Julia 1.12+ raises a structured `FieldError`, and the receiver type and field name are compared directly. Julia 1.11 has no such type and raises a plain `ErrorException`, where exact message equality is the tightest check available; `@static` picks the branch at load. Verified on both runtimes that CI builds. The guard itself, lifted verbatim, passes 27 checks on each of 1.11.9 and 1.13.0: the genuine error accepted for all three Sim stage types, and rejected for five adversarial field names, an unrelated error mentioning `stagelabel`, and the right field on the wrong receiver. Against the live testset on 1.13, all seven counterexamples rethrow and the genuine error is still caught. Suite unchanged: 782 passed, 8 broken, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Julia reads `0.x.y` with `x` as the breaking component and `y` as the non-breaking one — that is also how a `^0.2` compat bound reads it — so `0.3.0` announces a break. This release does not break a working caller on any path that was working: the DCAM4 constructor change only alters what comes back when the open FAILS, which is the path both downstream repos reported as already broken. The repo's own policy text had the convention inverted, calling `0.x.0` a minor bump and `0.0.x` a patch bump, which is what produced the wrong version here. Rewritten in CLAUDE.md, README.md and the CHANGELOG header to say plainly which component means what, and when to bump which. Every "Fixed in 0.3.0" attribution in the shipped skills and in test/gui.jl now reads 0.2.1, so a reader holding an installed copy can tell which release actually carries the fix. Suite unchanged: 782 passed, 8 broken, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kalidke
added a commit
that referenced
this pull request
Sep 23, 2026
) Actions minutes are a shared resource and a workflow re-run is not a cheap way to find out whether code compiles. Six pushes to PR #60 fired four workflows each, most of it re-running identical tests on commits that touched only prose. Pull requests now run one Julia version instead of two, skip the docs build entirely, and do not run at all when the change is confined to `docs/`, `dev/`, `.claude/` or the top-level prose files. The full matrix, the coverage upload and the docs build run once on `main` and on tags, where a version actually ships. Superseded runs are cancelled on every ref rather than only on pull requests, so a branch pushed repeatedly builds once. CompatHelper drops from daily to weekly, since this package is unregistered and its consumers pin exact tags. `paths-ignore` deliberately omits a blanket `**.md`: test/skills.jl reads each shipped SKILL.md and asserts on its frontmatter and version stamp, so a change under `skills/` must still be tested. CLAUDE.md gains a testing policy stating what was already true in practice — the local suite is the gate, CI is confirmation — with the command for the oldest supported Julia version that pull requests no longer run, and how to get full signal on a branch without opening one. The tiering scheme MicroscopeSeqSR.jl is building was considered and does not apply here: its suite is 1553 assertions in four minutes, ours is 790 in twenty-five seconds, so test execution is not our cost. Runner setup and job count are, which is what this changes. Its cancel-in-progress advice is adopted above. The Julia cache was checked and is healthy: 718 MB restored successfully, test jobs at ~3.4 minutes. Local suite green: 782 passed, 8 broken, 0 failed. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the two defects the downstream rig repo (MicroscopeAdapt.jl) ranked as its top blockers when asked which of the thirteen known items to order by. Version bumped to
0.3.0: theDCAM4Cameraconstructor contract changes.1. Opening a panel commanded the hardware (safety)
gui(::LightSource)wiredsetpowerandlight_on/light_offto their widgets withlift. Aliftevaluates immediately when it is created, so constructing the panel pushed the slider'sstartvalueof0.5to the device and commanded the on/off state.The downstream reported this as a safety item in their own words: their main GUI's hardware menu opens laser panels, and 50% on the 642 nm TCube floods a quarter of the camera chip.
on, which fires only when the observable changes.light.properties.powerand the toggle fromlight.properties.is_on, so the panel reflects the device instead of imposing a default.liftderives the displayed "On"/"Off" string and has no side effect.Audited every other panel for the same pattern.
stage_interface,camera_interface,attenuator_interface,daq_interface,triggerscope_interfaceandpi_n472contain no side-effectinglift. The only other hits are inobjective_positioner_interface/gui.jl, which is not included in the build, and they derive display strings anyway.2.
DCAM4Camera()returned a non-cameradcamapi_initfailure returned aDCAMERR.dcamdev_openfailure logged and returnednothing, leaving the SDK initialised.The downstream's
check_hardwareandinitialize_allassume a constructor returns a camera or throws, and a second open after that state is what crashes them.Both paths now
error(...)with the device id and theDCAMERR, and the open-failure path callsdcamapi_uninit()first so the SDK is not left held. The messages name the common cause, another process holding the device.Tests
test/gui.jl, included as a "GUI panels" testset, asserts that constructing a panel changes no device state, usingexport_stateas a generic per-device snapshot.gui(::SimLight)is the direct regression test, with explicitpowerandis_onchecks alongside the generic one.The Sim stages are
@test_brokenthere rather than skipped:gui(::Stage)throws for all three because the shared panel readsstagelabelwhileSimStage1d/2d/3dcarrylabel. That is a separate known defect, item 4 on the downstream's list, and is out of scope here. The@test_brokenflips to a failure when that is fixed, which is the signal to update this test.Full suite: 779 passed, 8 broken (5 pre-existing plus the 3 new Sim-stage markers), 0 failed.
Skills
The shipped skills documented both defects as limitations, so they are updated in the same commit to avoid contradicting the fixed code:
mc-system-designand itsdriver-caveats.md,mc-extendand itsdriver-scaffold.md,mc-testing, andmc-api-map'sgui-fields.md. Each says which version fixed it, since a reader may have an older installed copy.Hardware verification
Not done for the DCAM4 change. It touches a driver runtime path and there is no Hamamatsu camera on this machine. Per the 0.x policy, verification is recorded by the downstream repo when it advances its pin.
🤖 Generated with Claude Code