diff --git a/.gitignore b/.gitignore index c55a1b8..ae24170 100644 --- a/.gitignore +++ b/.gitignore @@ -5,5 +5,7 @@ /docs/Manifest.toml /docs/build/ .DS_Store +# local-only symlink to the lab instrument archive on the NAS (see CLAUDE.md) +/manuals # Local run output: test records, logs, handoffs (lab decision 0028) dev/output/ diff --git a/CHANGELOG.md b/CHANGELOG.md index b6a94c3..ff5f9fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,16 @@ the next version with `-DEV`). ## [Unreleased] ### Fixed +- **TCube laser: Kinesis boolean arguments are passed as four bytes.** Every + Kinesis boolean was bound as a one-byte `Bool`. That is right for return + values, but the headers declare arguments as a four-byte type, so the + controller could read three bytes of whatever the register held. Arguments + (`LD_EnableMaxCurrentAdjust`, `LD_EnableTIAGainAdjust`, + `LD_EnableLastMsgTimer`) are now a zero-extended `Cuint` (`KBOOL_ARG`), and + returns stay one byte (`KBOOL_RET`); the vendor facts are in + `manuals/Thorlabs/TLD001/BINDING.md` (see CLAUDE.md, "Instrument + documentation archive"). No driver method calls those three functions yet, + so no current behaviour changes; not yet run on hardware. - **PI stage: `initialize` no longer finishes on an unreferenced stage.** It ignored the return of the reference move (`PI_FRF`), so when the controller rejected it (GCS error 5, e.g. one axis's servo off) the later move to the diff --git a/CLAUDE.md b/CLAUDE.md index 31f4b85..5c4a248 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -186,3 +186,50 @@ pull request that raises `Z`, and merge the same fix into `main`. hand, after checking the same `lab/tests` coverage. What the numbers mean (decision 0033): before 1.0, in `0.Y.Z` **raising `Z` is any non-breaking change, new features included, and raising `Y` is an interface break** -- `0.2.4 -> 0.3.0` declares a break and `0.2.4 -> 0.2.5` a compatible release, which is also how Julia's `^0.2` compat bound reads them. A break is anything that lets working downstream code behave differently: a signature, an export, or what a call returns or throws. A bug fix that changes behaviour only on a path that was already broken is not a break. Config types are built by keyword (lab decision 0035): adding a field with a default is not a break, and a positional argument's meaning never changes (add a keyword and deprecate the old form instead). Hardware verification is not tracked in this repo; it is recorded by the downstream rig repo that pins to a given tag. The merge gate is the local suite (see "Testing policy" above) plus `test/contract.jl`'s "Interface Contract" testset, which guards the no-ambiguous-exports and core-method invariants described above; CI confirms it on a reduced matrix. + +## Instrument documentation archive (`manuals/`) + +`manuals/` is a **local-only symlink** to the lab-wide instrument archive on +the NAS — vendor manuals, SDK headers, API references and the driver-facing +notes that explain why a binding is the way it is. It is gitignored and must +**never** be committed: git stores a symlink as a mode-120000 blob, and on a +Windows rig without the symlink privilege that checks out as a text file +containing the path, which is worse than nothing. Each clone makes its own. + +```bash +# Linux (any of the four hosts) +ln -sfn /mnt/nas/lidkelab/Projects/lab_instruments manuals +``` +```bat +REM Windows rig, from the repo root. Needs an elevated prompt, OR Developer +REM Mode enabled once (Settings > Privacy & security > For developers). +mklink /D manuals \\192.168.1.21\lidke-lrs\Projects\lab_instruments +``` + +`[limitation]` The junction form, `mklink /J`, does **not** work here: junctions +resolve only to local volumes, so a UNC target fails. `/D` is required, and it +is the one step in this arrangement that needs a privilege — once per rig, not +once per clone. This has not been run on either rig yet; if `/D` is refused, +say so rather than reaching for a mapped drive letter, which differs between +user sessions and services. + +Nothing else is required — no environment variable and no shell profile edit. +If `manuals/` is absent, make it with the line above. + +Layout is manufacturer first, then model: `manuals/Thorlabs/TLD001/`, +`manuals/Hamamatsu/C11440-22CU/`, with shared vendor SDKs under +`manuals//SDK//`. Start at `manuals/INDEX.md`; the rules +for adding anything are in `manuals/README.md`. + +Within a model directory, `source/` is the vendor original, verbatim and never +renamed, and `docs/` is the working copy with a predictable name. A +`BINDING.md`, where one exists, is the distillate a driver author actually +needs — for example `manuals/Thorlabs/TLD001/BINDING.md` records that a Kinesis +C++ boolean must be *passed* as a 4-byte `Cuint` but *read back* as a 1-byte +`Bool`, which this package got wrong twice in opposite directions. + +`[policy]` When a driver's behaviour turns on a vendor fact -- a struct layout, +an ABI width, a scaling constant, a status bit -- record it in that model's +`BINDING.md` and cite the document in `source/` it came from. Three of the four +defects in v0.2.3 were found by a rig holding hardware rather than by review, +because the vendor fact was not written down anywhere a reviewer could check. diff --git a/src/hardware_implementations/tcube_laser/constants_Tlaser.jl b/src/hardware_implementations/tcube_laser/constants_Tlaser.jl index 4406eda..9206591 100644 --- a/src/hardware_implementations/tcube_laser/constants_Tlaser.jl +++ b/src/hardware_implementations/tcube_laser/constants_Tlaser.jl @@ -8,23 +8,71 @@ const __int64 = Clonglong const __int32 = Cint +""" + BOOL + +Four bytes. **Retained for `TLI_DeviceInfo`'s fields only**, which nothing in +this package calls — see that struct's docstring. Do not use it for a new +binding: use [`KBOOL_ARG`](@ref) for an argument and [`KBOOL_RET`](@ref) for a +return. + +`[limitation]` The vendor headers do **not** declare `BOOL` for these fields. +Two rigs have now read their own vendor-installed +`Thorlabs.MotionControl.TCube.LaserDiode.h` — Kinesis 1.14.10 on the seq-sr rig +and 1.14.47.22504 on quickbeam — and **both declare lowercase C++ `bool` with +`#pragma pack(1)` active**. This `Cuint` is therefore wrong for +`TLI_DeviceInfo`, and is left in place only because no single layout fits both +versions anyway (they disagree on `serialNo`'s length), so there is nothing to +change it *to*. + +**History, because this was got wrong twice in opposite directions.** v0.2.3 +retyped both boolean roles to a one-byte `Bool`. A copy of the header on the lab +NAS appeared to contradict that, carrying `typedef unsigned int BOOL` and no +lowercase `bool` at all, and on its strength the change was reverted — but that +file is a Clang.jl generation input, hand-edited to parse without the Windows +SDK, and its typedefs are artefacts of the editing rather than the vendor's ABI. +Splitting the two roles is the correct answer, and the vendor-installed headers +since read off both rigs confirm it. +""" const BOOL = Cuint """ - CPPBOOL - -Julia's `Bool`, i.e. one byte, for the Kinesis entry points the header declares -as C++ `bool` rather than as a Windows `BOOL`. The distinction is not cosmetic: -a `bool` return sets only the low byte of the return register, so reading it as -a 4-byte `BOOL` reads three bytes of whatever happened to be there, and -`LD_CheckConnection(...) != 0` can then report a disconnected controller as -connected. Reported by the 642 nm rig from the Kinesis header, 2026-09-23; the -same file's `tcubeapi.jl` already used `::Bool` for `LD_StartPolling` and -`LD_StopPolling`, so the package disagreed with itself. Not hardware-verified. - -`BOOL` above stays `Cuint` for the genuine Windows `BOOL` uses. + KBOOL_ARG + +The type to pass a Kinesis C++ `bool` ARGUMENT: a zero-extended `Cuint` +carrying exactly 0 or 1. + +This is robust whichever width the callee really reads. A `bool` callee takes +the low byte and sees 0 or 1; a `BOOL` callee takes all four and sees 0 or 1. +Passing a 1-byte `Bool` is NOT robust in this direction: the upper three bytes +of the register are undefined, so a `false` can arrive as true. That matters +for `LD_EnableMaxCurrentAdjust(serialNo, enableAdjust, enableDiode)`, whose +second flag enables the laser diode during a max-current adjustment. +""" +const KBOOL_ARG = Cuint + +""" + KBOOL_RET + +The type to read a Kinesis C++ `bool` RETURN: one byte. + +The installed vendor header declares these `bool`, which on MSVC x86-64 +returns in `AL` and leaves the rest of `EAX` **undefined**. Reading four bytes +can therefore turn a `false` into a nonzero value — `LD_CheckConnection` +reporting a disconnected controller as connected. Reading the low byte is +correct under the `bool` ABI and still correct under a `BOOL` ABI returning +0 or 1. + +**History, because this was got wrong twice.** v0.2.3 retyped both roles to a +1-byte `Bool`, which was right for returns and wrong for arguments. A copy of +the header on the lab NAS appeared to contradict it — but that copy is a +Clang.jl generation input, hand-edited to parse without Windows headers: its +`typedef unsigned int BOOL` and its commented-out `#pragma pack` are artefacts +of that editing, not the vendor's ABI. The installed header has 32 lowercase +`bool`, zero `BOOL`, and an active `#pragma pack(1)`. Splitting the two roles +is what is actually correct, and is safe under either reading. """ -const CPPBOOL = Bool +const KBOOL_RET = Bool struct tagSAFEARRAYBOUND cElements::Culong @@ -67,22 +115,39 @@ end """ TLI_DeviceInfo -**[limitation] This layout is suspect and unverified.** The Kinesis header -declares the `is*` fields as C++ `bool` (one byte), and they are typed here as -`BOOL` = `Cuint` (four). If that is right, every field from `isKnownType` -onward is misaligned and `TLI_GetDeviceInfo` returns nonsense. Nothing in this -package calls `TLI_GetDeviceInfo`, so the defect is latent rather than active, -and it is left alone as deferred ABI repair rather than corrected in passing. - -Correcting it is more than retyping the five fields, and does NOT need a -controller -- it needs the vendor header, which this repo does not carry. The -header declares the structure `#pragma pack(1)` at 100 bytes; this declaration -is 120, and the divergence starts at `PID`, *before* the first `bool`. So -retyping the booleans alone would leave it wrong. Repair it against the header, -with the size asserted, or do not call it. The function signatures in -`functions_Tlaser.jl` had a related discrepancy and WERE corrected -- see -`CPPBOOL` -- because there the fix moves no field and the ABI rule is -unambiguous. +**[limitation] This layout does not match either Kinesis version we have +looked at, and it is left alone deliberately, because there is no single +layout that would match both.** + +Two rigs read their installed headers and reported different declarations: + +| | Kinesis 1.14.10 (seq-sr rig) | Kinesis 1.14.47.22504 (quickbeam) | +|---|---|---| +| `serialNo` | `char serialNo[9]` | `char serialNo[16]` | +| flags | 1-byte C++ `bool` | 1-byte C++ `bool` | +| `#pragma pack(1)` | ACTIVE (lines 63-125) | ACTIVE | +| reported size | — | 100 bytes, `PID` at 85 | + +This declaration uses `NTuple{16,Cchar}`, 4-byte `BOOL` flags and default +alignment, measuring **120 bytes with `PID` at offset 88** (verified by +execution). It is wrong for both, and the field that differs between the two +vendor versions is an array LENGTH, so no reinterpretation fixes both at once. + +`[policy]` Do not call `TLI_GetDeviceInfo` through this declaration. Nothing +in this package does — `initialize` uses only `TLI_BuildDeviceList` and +`TLI_GetDeviceListSize`, both of which return counts and touch no struct +(verified). If a caller ever needs device info, declare the struct for the +SDK version in use, assert `sizeof`, and keep it beside the header it was +read from. + +**One copy of this header on the lab NAS contradicts all of the above; do not +trust it.** `Personal Folders/Sheng/code/generate_lib/lib/` holds a Clang.jl +generation input, hand-edited to parse without Windows headers: local +typedefs including `typedef unsigned int BOOL`, `OaIdl.h` and `__declspec` +stripped, both pack pragmas commented out, every lowercase `bool` rewritten. +Those are artefacts of the editing. This docstring briefly asserted, on the +strength of that file, that our layout was correct; it is not, and the round +trip cost two wrong rulings in opposite directions. """ struct TLI_DeviceInfo typeID::DWORD diff --git a/src/hardware_implementations/tcube_laser/functions_Tlaser.jl b/src/hardware_implementations/tcube_laser/functions_Tlaser.jl index 0680825..7d6a5a0 100644 --- a/src/hardware_implementations/tcube_laser/functions_Tlaser.jl +++ b/src/hardware_implementations/tcube_laser/functions_Tlaser.jl @@ -55,7 +55,7 @@ function LD_Close(serialNo) end function LD_CheckConnection(serialNo) - ccall((:LD_CheckConnection, Thorlabs_Tcube_laser), CPPBOOL, (Ptr{Cchar},), serialNo) + ccall((:LD_CheckConnection, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar},), serialNo) end function LD_Identify(serialNo) @@ -79,15 +79,15 @@ function LD_GetSoftwareVersion(serialNo) end function LD_LoadSettings(serialNo) - ccall((:LD_LoadSettings, Thorlabs_Tcube_laser), CPPBOOL, (Ptr{Cchar},), serialNo) + ccall((:LD_LoadSettings, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar},), serialNo) end function LD_LoadNamedSettings(serialNo, settingsName) - ccall((:LD_LoadNamedSettings, Thorlabs_Tcube_laser), CPPBOOL, (Ptr{Cchar}, Ptr{Cchar}), serialNo, settingsName) + ccall((:LD_LoadNamedSettings, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar}, Ptr{Cchar}), serialNo, settingsName) end function LD_PersistSettings(serialNo) - ccall((:LD_PersistSettings, Thorlabs_Tcube_laser), CPPBOOL, (Ptr{Cchar},), serialNo) + ccall((:LD_PersistSettings, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar},), serialNo) end function LD_Disable(serialNo) @@ -111,11 +111,11 @@ function LD_MessageQueueSize(serialNo) end function LD_GetNextMessage(serialNo, messageType, messageID, messageData) - ccall((:LD_GetNextMessage, Thorlabs_Tcube_laser), CPPBOOL, (Ptr{Cchar}, Ptr{WORD}, Ptr{WORD}, Ptr{DWORD}), serialNo, messageType, messageID, messageData) + ccall((:LD_GetNextMessage, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar}, Ptr{WORD}, Ptr{WORD}, Ptr{DWORD}), serialNo, messageType, messageID, messageData) end function LD_WaitForMessage(serialNo, messageType, messageID, messageData) - ccall((:LD_WaitForMessage, Thorlabs_Tcube_laser), CPPBOOL, (Ptr{Cchar}, Ptr{WORD}, Ptr{WORD}, Ptr{DWORD}), serialNo, messageType, messageID, messageData) + ccall((:LD_WaitForMessage, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar}, Ptr{WORD}, Ptr{WORD}, Ptr{DWORD}), serialNo, messageType, messageID, messageData) end function LD_SetOpenLoopMode(serialNo) @@ -127,7 +127,7 @@ function LD_SetClosedLoopMode(serialNo) end function LD_EnableMaxCurrentAdjust(serialNo, enableAdjust, enableDiode) - ccall((:LD_EnableMaxCurrentAdjust, Thorlabs_Tcube_laser), Cshort, (Ptr{Cchar}, CPPBOOL, CPPBOOL), serialNo, enableAdjust, enableDiode) + ccall((:LD_EnableMaxCurrentAdjust, Thorlabs_Tcube_laser), Cshort, (Ptr{Cchar}, KBOOL_ARG, KBOOL_ARG), serialNo, enableAdjust, enableDiode) end function LD_RequestMaxCurrentDigPot(serialNo) @@ -147,7 +147,7 @@ function LD_FindTIAGain(serialNo) end function LD_EnableTIAGainAdjust(serialNo, enable) - ccall((:LD_EnableTIAGainAdjust, Thorlabs_Tcube_laser), Cshort, (Ptr{Cchar}, CPPBOOL), serialNo, enable) + ccall((:LD_EnableTIAGainAdjust, Thorlabs_Tcube_laser), Cshort, (Ptr{Cchar}, KBOOL_ARG), serialNo, enable) end function LD_DisableOutput(serialNo) @@ -267,7 +267,7 @@ function LD_GetStatusBits(serialNo) end function LD_StartPolling(serialNo, milliseconds) - ccall((:LD_StartPolling, Thorlabs_Tcube_laser), CPPBOOL, (Ptr{Cchar}, Cint), serialNo, milliseconds) + ccall((:LD_StartPolling, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar}, Cint), serialNo, milliseconds) end function LD_PollingDuration(serialNo) @@ -279,15 +279,15 @@ function LD_StopPolling(serialNo) end function LD_TimeSinceLastMsgReceived(serialNo, arg2) - ccall((:LD_TimeSinceLastMsgReceived, Thorlabs_Tcube_laser), CPPBOOL, (Ptr{Cchar}, __int64), serialNo, arg2) + ccall((:LD_TimeSinceLastMsgReceived, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar}, __int64), serialNo, arg2) end function LD_EnableLastMsgTimer(serialNo, enable, lastMsgTimeout) - ccall((:LD_EnableLastMsgTimer, Thorlabs_Tcube_laser), Cvoid, (Ptr{Cchar}, CPPBOOL, __int32), serialNo, enable, lastMsgTimeout) + ccall((:LD_EnableLastMsgTimer, Thorlabs_Tcube_laser), Cvoid, (Ptr{Cchar}, KBOOL_ARG, __int32), serialNo, enable, lastMsgTimeout) end function LD_HasLastMsgTimerOverrun(serialNo) - ccall((:LD_HasLastMsgTimerOverrun, Thorlabs_Tcube_laser), CPPBOOL, (Ptr{Cchar},), serialNo) + ccall((:LD_HasLastMsgTimerOverrun, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar},), serialNo) end function LD_RequestSettings(serialNo)