PI N-472: check the connect and every setup step, make the object retryable, fix stopmotion

CHANGELOG.md

@@ -22,6 +22,68 @@ the next version with `-DEV`).
   unchanged. Reported by the MicroscopeAdapt rig; not yet run on hardware
   (#64).
 
+PI N-472 actuator driver (`PI_N472`): initialize, shutdown and stop. The
+lifecycle is covered by fake-GCS2 tests that run on every machine
+(`test/pi_n472_fake_sdk.jl`). Hardware: not verified in this repository. The
+author reports exercising initialize, `stopmotion` with no motion in
+progress, shutdown, re-initialize and a refused second object on a C-885
+(SN 124014300), with no motion commanded; stop during motion and the
+connect-failure branch were not exercised on hardware.
+
+- **A failed connect was silent and poisoned the object.** `connectionstatus`
+  was set before `PI_ConnectUSB` was called and its `-1` return never checked,
+  so every later GCS call failed quietly, "Stage initialized" was still logged,
+  and a retry was refused as "already initialized". The flag is now set only
+  after a successful connect; a failure logs the description and
+  `PI_GetInitError()` and leaves the object retryable. The intermittent
+  initialize seen on the rig is more likely another process holding the
+  controller, made sticky by this bug, than anything in the connect string;
+  that is not established, so do not treat it as fixed until the rig says so.
+- **`shutdown` never cleared `connectionstatus`**, so re-initializing the same
+  object in one session was always refused. It now clears the flag.
+- **`shutdown` could close another object's connection.** `id` defaulted to
+  `0`, a valid GCS ID, and `shutdown` never reset it, so a second `shutdown` on
+  a closed object, or a `shutdown` on one never initialized, closed whichever
+  controller then held ID 0. `id` now defaults to `-1` and `shutdown` resets it.
+- **`stopmotion` could not reach the controller.** It passed `stage.axes`, a
+  `Vector{String}`, where the DLL wants one space-separated `Ptr{Cchar}`
+  string; the pointer handed over pointed at string references, not
+  characters. It now joins the axes like every other call in the driver.
+- **The connect string relied on an implementation detail.** `initialize`
+  filtered every `0x00` out of the enumeration buffer and passed the bare
+  `Vector{UInt8}`. On current Julia that happens to leave a zero just past the
+  shrunk vector, so the DLL did see a terminated string, but by accident of
+  `filter`'s implementation, and carrying the enumeration's trailing newline
+  (and every further description when several controllers are attached). It
+  now passes the first description, whitespace-stripped, as a `String`, which
+  Julia always NUL-terminates at a `Ptr{Cchar}` boundary. The buffer grew from
+  128 to 1024 bytes to match the C-867 driver.
+
+### Changed
+
+Behaviour a rig pinned to an earlier tag will see from the PI N-472 driver,
+each on its own:
+
+- **`initialize(::N472)` now throws when a setup step fails.** After the
+  connect, every step (reference mode, `PI_POS`, servos, travel range,
+  velocity) is checked; a failure closes the connection, clears
+  `connectionstatus` and `id`, and throws with the step and its GCS error
+  code. Before, failures were ignored and "Stage initialized" was logged on a
+  half-initialized stage. Enumeration and connect failures still log `@error`
+  and return, as `initialize(::PIStage)` does. Not a break under this
+  package's versioning rule: it changes behaviour only on a path that was
+  already broken.
+- **Re-initializing after `shutdown` now re-zeroes the frame.** A second
+  `initialize` on the same object was refused and did nothing; it now runs the
+  full sequence: reference mode off, `PI_POS` redefining the current position
+  as `homepos`, servos on. A script that used shutdown-then-initialize as a
+  reconnect now resets its coordinates to wherever the actuator sits.
+- **With several C-885s attached, `initialize` connects to the first
+  enumerated one.** There is no selection by serial number.
+- **`N472()` defaults `id` to `-1`**, not `0`.
+- **`setvel(::N472)` returns `FALSE` when `PI_VEL` fails.** It used to return
+  only the status of the `PI_qVEL` read-back.
+
 ### Changed (release process)
 - **`main` is the development branch**, carrying the next version with
   `-DEV`; every pull request goes into it. The `0.3rc1` release-candidate

CLAUDE.md

@@ -119,6 +119,23 @@ Hardware implementations use `ccall` for vendor SDKs:
 - `mcl_stage/*.jl` - Mad City Labs NanoDrive
 - Serial devices (CrystaLaser, Vortran, Triggerscope) use `LibSerialPort`
 
+Rules at the `ccall` boundary, learned from the C-867 servo bug (v0.1.1), the
+N-472 connect string that worked only by accident of `filter`, and the N-472
+`stopmotion` that never worked:
+- A `Ptr{Cchar}` argument (GCS2 axes lists, USB descriptions) gets a Julia
+  `String`, which is always NUL-terminated. Never a `Vector{UInt8}` with the
+  zeros filtered out, and never a `Vector{String}`; join axes with a space first.
+- A GCS2 `BOOL*` argument is 32-bit (`Cuint`/`Cint`), one element per axis,
+  never `UInt8`.
+- Set a driver's `connectionstatus` only after the connect call's return value
+  is checked, and clear it and the device id in `shutdown`, so a failed or
+  closed object can be initialized again and a stale id cannot close another
+  object's connection.
+
+The test suite must never command attached hardware. Driver tests replace the
+vendor wrappers with a recorder (`test/tcube_fake_sdk.jl`,
+`test/pi_n472_fake_sdk.jl`), so they run on every machine and never reach a DLL.
+
 ### Camera Image Data Convention
 
 **Convention:** Image data is stored and displayed as column-major `(H, W, N)` arrays where `data[row, col]` = `data[y, x]`.

skills/mc-extend/references/rig-causes.md

@@ -9,7 +9,7 @@ and each looks like a broken `ccall`.
 
 | Observed symptom | Possible causes | How to tell |
 |---|---|---|
-| `initialize(stage::PIStage)` logs `@error` ("No PI C-867 found ..." or "PI_ConnectUSB failed ...") and returns; `stage.connectionstatus` stays `false`. It does **not** throw. | Controller absent or unpowered; USB enumeration; **or** held by another process (a second Julia session with an initialized stage, PIMikroMove, an open COM port). None of these is established by the error alone. | Check `stage.connectionstatus` after every `initialize`. Close PIMikroMove and every other Julia; check Task Manager for stray `julia.exe`. If it then connects, it was contention, not the driver. |
+| `initialize` on a PI C-867 (`PIStage`) or C-885 (`N472`) logs `@error` ("No PI C-867 found ...", "No PI C-885 found ..." or "PI_ConnectUSB failed ...") and returns; `stage.connectionstatus` stays `false`. It does **not** throw; a failure *after* the connect does (a refused reference move on the C-867, any setup step on the C-885). **[N472 fixed in v0.3.0]** Before that release the N472 set the flag before checking the connect, so a failed connect logged "Stage initialized" and a retry was refused as "already initialized". | Controller absent or unpowered; USB enumeration; **or** held by another process (a second Julia session with an initialized stage, PIMikroMove, an open COM port). None of these is established by the error alone. | Check `stage.connectionstatus` after every `initialize`. Close PIMikroMove and every other Julia; check Task Manager for stray `julia.exe`. If it then connects, it was contention, not the driver. On an N472 before 0.3.0, build a fresh `N472()` before retrying: neither a failed connect nor `shutdown` cleared the flag. |
 | `MLSLM` SDK calls do nothing useful | The board is claimed by another process (vendor GUI or a dead Julia), or the SDK never found it. | `MLSLM()` is pure: its `n_boards_found` field is a **default of 0** and is never updated, so it diagnoses nothing. The only board count is what `initializesdk()` prints to stdout from `Create_SDK`. Read that output; if it reports 0 boards with the board powered, close the vendor GUI and every other Julia, then reboot if a dead process still holds it. |
 | Serial device times out, returns garbage, or "port busy" | Wrong COM assignment (Windows renumbers COM ports when USB topology changes), or the port is open elsewhere. Applies only to the **Triggerscope** (`Triggerscope4`, default `portname="COM3"`) and, through it, `LCC1620`. | Device Manager: match the port to the device. From Julia, `MicroscopeControl.HardwareImplementations.Triggerscope.LibSerialPort.list_ports()`. One `Triggerscope4` per physical port, shared by its dependents (`mc-system-design`). |
 | A DAQ-backed light (`CrystaLaser`, `VortranLaser`, `DaqTrLight`) `@warn`s at construction ("No NI-DAQ devices found ..." or "Failed to initialize NI-DAQ ..."), then `initialize` returns normally (Vortran's may `@warn` "insufficient DO channels for initialization") and `setpower`/`light_on` `@warn` about missing channels and do nothing, or only some calls work | These devices are **not** serial. They drive NI-DAQ AO/DO channels through an `NIdaq` built in their constructor, which picks `devs[1]` (Crysta, Vortran) or `devs[device_index]` (DaqTrLight, default 2). Discovery runs AO then DO inside one `try`, so it can **partially** succeed: AO channels kept, DO empty. Causes: wrong device index, missing NI-DAQmx runtime, card not enumerated, or a DO-less card. | Inspect `light.channelsAO` and `light.channelsDO` right after construction; `NIDAQcard.showdevices(NIdaq())` from Julia; compare with NI MAX. Fix the index or the runtime, not the driver. |

skills/mc-system-design/references/driver-caveats.md

@@ -93,6 +93,16 @@ Resolving to a device-specific method is not the same as the operation working:
 - `getposition(::N472)` returns the SDK success code and stores positions in
   `stage.pos`; `move(::N472, pos::Vector{Float64})` takes a vector, not
   `x, y, z` (traced).
+- `stopmotion(::N472)` **[fixed in v0.3.0]**: before that release it passed
+  `stage.axes` (a `Vector{String}`) where the DLL wants one space-separated
+  string, so the halt never reached the controller (traced).
+- `initialize(::N472)` / `shutdown(::N472)` **[fixed in v0.3.0]**: `initialize`
+  sets `connectionstatus` only after `PI_ConnectUSB` succeeds and leaves the
+  object retryable on failure, and throws (after closing the connection) when
+  a setup step fails; `shutdown` clears the flag and resets `id` to `-1`.
+  Before 0.3.0 a failed connect was reported as "Stage initialized", both a
+  retry and a re-initialize after `shutdown` were refused, and a stale `id` of
+  `0` could close another object's controller (fake-SDK tests).
 - `capture` returns the `SINGLE_FRAME` enum on `SimCamera` and the frame on the
   hardware cameras, and `getdata` after `capture` is valid **only** on
   `SimCamera` (DCAM4 and DCX release their buffers before `capture` returns).

src/hardware_implementations/pi_n472/helper.jl

@@ -71,9 +71,9 @@ function setvel(stage::N472,vel::Vector{Float64})
         @error "Failed to set velocity"
     end
 
-    success = PI_qVEL(stage.id, axes, stage.velocity)
-    if success == FALSE
+    qsuccess = PI_qVEL(stage.id, axes, stage.velocity)
+    if qsuccess == FALSE
         @error "Failed to query velocity"
     end
-    return success
-end
+    return success == FALSE ? FALSE : qsuccess
+end

src/hardware_implementations/pi_n472/interface_methods.jl

@@ -1,59 +1,75 @@
+"""
+    _cstring(buf) -> String
+
+The bytes of `buf` up to its first NUL, as a `String`.
+"""
+function _cstring(buf::Vector{UInt8})
+    i = findfirst(==(0x00), buf)
+    return String(buf[1:(i === nothing ? end : i - 1)])
+end
 
 function initialize(stage::N472)
     if stage.connectionstatus == true
         @error "Stage already initialized"
         return
     end
 
-    # Create a buffer string
-    buffersize = 128
-    devstring = zeros(UInt8, buffersize)
+    # An absent, unpowered or held controller fails here or at the connect below.
+    buffersize = 1024
+    buffer = zeros(UInt8, buffersize)
     controllername = "C-885"
-    numdevice = PI_EnumerateUSB(devstring, buffersize, controllername)
-    devstring = filter(x -> x != 0x00, devstring)
-
-
-
-    #Set connection status to true
-    if numdevice > 0
-        stage.connectionstatus = true
-    else
-        @error "No devices connected"
+    numdevice = PI_EnumerateUSB(buffer, buffersize, controllername)
+    if numdevice <= 0
+        @error "No PI C-885 found by the GCS2 library (absent, unpowered, or held by another process)"
         stage.connectionstatus = false
         return
     end
 
+    # Descriptions are '\n'-separated with one NUL at the end; pass the first as a String (NUL-terminated for Ptr{Cchar}).
+    devstring = String(strip(first(split(_cstring(buffer), '\n'))))
+    @info "PI device: " * devstring
+
     #Connect to usb device
     stage.id = PI_ConnectUSB(devstring)
-    @info "PI device: " * String(devstring)
     @info "Device ID: " * string(stage.id)
+    if stage.id < 0
+        # Connect failed (id -1): leave the flag cleared so initialize can be retried.
+        stage.connectionstatus = false
+        @error "PI_ConnectUSB failed for \"$devstring\" (init error $(PI_GetInitError())); the controller may be held by another process"
+        return
+    end
+    stage.connectionstatus = true
 
-    #Query the unit of the physical position
-    axes = join(stage.axes, " ")
-    #unitstring = zeros(UInt8, buffersize)
-    #success = PI_qPUN(stage.id, axes, unitstring, buffersize)
-    #stage.units = String(unitstring)
+    # Every step from here is checked; on failure close the connection so a retry starts clean.
+    try
+        axes = join(stage.axes, " ")
+        failed(step) = error("N472 initialize: $step failed (GCS error $(PI_GetError(stage.id)))")
 
-    #query reference mode
-    refmode = zeros(BOOL, 3)
-
-    success = PI_RON(stage.id, axes, refmode)
-    success = PI_qRON(stage.id, axes, refmode)
-    @info "Reference mode: " * string(refmode)
+        #query reference mode
+        refmode = zeros(BOOL, 3)
 
-    # set the current position as the reference position
-    success = set_refpos(stage)
+        PI_RON(stage.id, axes, refmode) == FALSE && failed("PI_RON")
+        PI_qRON(stage.id, axes, refmode) == FALSE && failed("PI_qRON")
+        @info "Reference mode: " * string(refmode)
 
-    # turn on servo 
-    for i in eachindex(stage.axes)
-        servo(stage, i, TRUE)
-    end
-    #Query the travel range
-    success = PI_qTMN(stage.id, axes, stage.minpos)
-    success = PI_qTMX(stage.id, axes, stage.maxpos)
+        # set the current position as the reference position
+        set_refpos(stage) == FALSE && failed("set_refpos (PI_POS)")
 
-    #set velocity
-    success = setvel(stage, stage.velocity)
+        # turn on servo
+        for i in eachindex(stage.axes)
+            servo(stage, i, TRUE) == FALSE && failed("servo axis $i")
+        end
+
+        #Query the travel range
+        PI_qTMN(stage.id, axes, stage.minpos) == FALSE && failed("PI_qTMN")
+        PI_qTMX(stage.id, axes, stage.maxpos) == FALSE && failed("PI_qTMX")
+
+        #set velocity
+        setvel(stage, stage.velocity) == FALSE && failed("setvel")
+    catch
+        shutdown(stage)
+        rethrow()
+    end
 
     @info "Stage initialized"
     return
@@ -67,6 +83,9 @@ function shutdown(stage::N472)
     else
         @info "Stage not connected"
     end
+    # Clearing both lets the object be re-initialized and stops a stale id closing another object's connection.
+    stage.connectionstatus = false
+    stage.id = Cint(-1)
     return
 end
 
@@ -95,7 +114,9 @@ function StageInterface.home(stage::N472)
 end
 
 function StageInterface.stopmotion(stage::N472)
-    success = PI_HLT(stage.id, stage.axes)
+    # GCS2 axes arguments are one space-separated string.
+    axes = join(stage.axes, " ")
+    success = PI_HLT(stage.id, axes)
     return success
 end
 

src/hardware_implementations/pi_n472/types.jl

@@ -27,7 +27,7 @@ function N472(;
     dimensions::Int=1,
     axes::Vector{String}=["1", "3", "5"],
     connectionstatus::Bool=false,
-    id::Cint=Cint(0),
+    id::Cint=Cint(-1),
     pos::Vector{Float64}=[0.0, 0.0, 0.0],
     minpos::Vector{Float64}=[0.0, 0.0, 0.0],
     maxpos::Vector{Float64}=[7.0, 7.0, 7.0],

test/pi_n472.jl

@@ -0,0 +1,112 @@
+@testset "PI N472 (no hardware)" begin
+    N = MicroscopeControl.HardwareImplementations.PI_N472
+    F = Main.FakeGCS2
+    quiet(f) = Base.CoreLogging.with_logger(f, Base.CoreLogging.NullLogger())
+    setup_ops = ["PI_RON", "PI_qRON", "PI_POS", "PI_SVO", "PI_qSVO", "PI_qTMN", "PI_qTMX", "PI_VEL", "PI_qVEL"]
+
+    @testset "_cstring stops at the first NUL" begin
+        buf = zeros(UInt8, 32)
+        buf[1:14] .= codeunits("PI C-885 SN 42")
+        buf[20] = UInt8('x')   # stale bytes past the terminator are ignored
+        @test N._cstring(buf) == "PI C-885 SN 42"
+        @test N._cstring(codeunits("abc") |> collect) == "abc"
+        @test N._cstring(UInt8[0x00]) == ""
+    end
+
+    @testset "first description is passed as a String" begin
+        F.reset!()
+        F.enum_bytes[] = UInt8[codeunits("d1\nd2")..., 0x00, codeunits("junk")...]
+        F.enum_count[] = 2
+        stage = N472()
+        quiet(() -> initialize(stage))
+        @test F.lastarg["PI_ConnectUSB"] isa String
+        @test F.lastarg["PI_ConnectUSB"] == "d1"
+        @test stage.connectionstatus == true
+        @test stage.id == 0
+    end
+
+    @testset "description is stripped" begin
+        F.reset!()
+        F.enum_bytes[] = UInt8[codeunits("  d1\r\n")..., 0x00]
+        quiet(() -> initialize(N472()))
+        @test F.lastarg["PI_ConnectUSB"] == "d1"
+    end
+
+    @testset "failed connect leaves the object retryable" begin
+        F.reset!()
+        append!(F.connect_ids, [-1, 0])
+        stage = N472()
+        @test_logs (:error, r"PI_ConnectUSB failed") match_mode=:any initialize(stage)
+        @test stage.connectionstatus == false
+        @test stage.id == -1
+        @test !any(op -> op in setup_ops, F.calls)
+        quiet(() -> initialize(stage))
+        @test stage.connectionstatus == true
+        @test stage.id == 0
+    end
+
+    @testset "no controller found" begin
+        F.reset!()
+        F.enum_count[] = 0
+        stage = N472()
+        @test_logs (:error, r"No PI C-885 found") match_mode=:any initialize(stage)
+        @test stage.connectionstatus == false
+        @test !("PI_ConnectUSB" in F.calls)
+        @test_logs (:error, r"No PI C-885 found") match_mode=:any initialize(stage)
+    end
+
+    @testset "stopmotion sends one axes string" begin
+        F.reset!()
+        stage = N472()
+        quiet(() -> initialize(stage))
+        quiet(() -> stopmotion(stage))
+        @test F.lastarg["PI_HLT"] == "1 3 5"
+    end
+
+    @testset "shutdown closes only its own connection" begin
+        F.reset!()
+        A = N472(); B = N472()
+        quiet(() -> initialize(A))
+        quiet(() -> shutdown(A))
+        append!(F.connect_ids, [0, 0])
+        quiet(() -> initialize(B))
+        @test 0 in F.open_ids
+        quiet(() -> shutdown(A))
+        @test 0 in F.open_ids
+        @test count(==("PI_CloseConnection"), F.calls) == 1
+        @test A.id == -1
+    end
+
+    @testset "a fresh object holds no connection" begin
+        F.reset!()
+        @test N472().id == -1
+        B = N472()
+        quiet(() -> initialize(B))
+        @test 0 in F.open_ids
+        quiet(() -> shutdown(N472()))
+        @test 0 in F.open_ids
+        @test !("PI_CloseConnection" in F.calls)
+    end
+
+    @testset "a failing setup step closes the connection: $op" for op in setup_ops
+        F.reset!()
+        F.error_code[] = 7
+        push!(F.failing, op)
+        stage = N472()
+        err = try
+            quiet(() -> initialize(stage))
+            nothing
+        catch e
+            e
+        end
+        @test err isa ErrorException
+        @test occursin("GCS error 7", err.msg)
+        @test !(0 in F.open_ids)
+        @test stage.connectionstatus == false
+        @test stage.id == -1
+        empty!(F.failing)
+        F.error_code[] = 0
+        quiet(() -> initialize(stage))
+        @test stage.connectionstatus == true
+    end
+end