From 83a15145e54919907d66c9543fdb68363a2019af Mon Sep 17 00:00:00 2001 From: kalidke Date: Thu, 24 Sep 2026 08:37:54 -0600 Subject: [PATCH 1/4] Revert the boolean binding change: the header says four bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.2.3 retyped nine returns and three arguments from `BOOL` (`Cuint`) to a one-byte `Bool`, on a second-hand report that the Kinesis header declares C++ `bool`. The header says otherwise, and it was on the lab NAS the whole time: Thorlabs.MotionControl.TCube.LaserDiode.h:37 typedef unsigned int BOOL; 27 uppercase `BOOL`, zero lowercase `bool`. The original binding was correct and the change was a regression. It is wrong in the dangerous direction for ARGUMENTS: passing one byte where the callee reads four leaves the upper three undefined, so a `false` can arrive as true. The call that matters is `LD_EnableMaxCurrentAdjust(serial, enableAdjust, enableDiode)`, whose second flag enables the laser diode during a max-current adjustment — the call the closed-loop clamp sequence depends on being able to pass as false. Four bytes is safe under either ABI for an argument; one byte is not. The same report also drove a `[limitation]` warning on `TLI_DeviceInfo` saying it was packed to 100 bytes and misaligned from `PID` onward. The header's `#pragma pack(1)` is COMMENTED OUT, and the fields are the same 4-byte `BOOL`. Verified: our declaration is 120 bytes with `PID` at offset 88, which is exactly the header's default-alignment arithmetic. That warning was false and is deleted — it would have sent the next reader hunting a defect that does not exist. Both docstrings now record the history, so nobody re-applies either "fix". Nothing in this package calls the affected functions, so the regression is latent here; a downstream calls `LD_CheckConnection`. Suite: 959 passed, 8 broken, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) --- .../tcube_laser/constants_Tlaser.jl | 61 ++++++++++--------- .../tcube_laser/functions_Tlaser.jl | 24 ++++---- 2 files changed, 44 insertions(+), 41 deletions(-) diff --git a/src/hardware_implementations/tcube_laser/constants_Tlaser.jl b/src/hardware_implementations/tcube_laser/constants_Tlaser.jl index 4406eda..fa201b6 100644 --- a/src/hardware_implementations/tcube_laser/constants_Tlaser.jl +++ b/src/hardware_implementations/tcube_laser/constants_Tlaser.jl @@ -11,20 +11,26 @@ const __int32 = Cint 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. + BOOL + +The Kinesis headers `typedef unsigned int BOOL` (verified at line 37 of +`Thorlabs.MotionControl.TCube.LaserDiode.h`), so this is four bytes, and every +`BOOL` argument and return in `functions_Tlaser.jl` is that width. + +**History, because this was got wrong once.** v0.2.3 changed these to a +one-byte `Bool` on a report that the header declared C++ `bool`. It does not: +that header contains no lowercase `bool` at all. The change was reverted +because it is wrong in the dangerous direction for ARGUMENTS — passing one +byte where the callee reads four leaves the upper three undefined, so a +`false` can arrive as true. `LD_EnableMaxCurrentAdjust(serial, true, false)` +is the call that matters: its second flag enables the laser diode during a +max-current adjustment. + +If a future Kinesis version really does declare `bool`, check the header for +that version before changing this, and change arguments and returns +separately — four bytes is safe for an argument under either ABI, one byte is +not. """ -const CPPBOOL = Bool struct tagSAFEARRAYBOUND cElements::Culong @@ -67,22 +73,19 @@ 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. +Mirrors the `TLI_DeviceInfo` struct in the Kinesis headers. `BOOL` is four +bytes there (`typedef unsigned int BOOL`, line 37), the fields are in this +order, and the struct is NOT packed -- the header's `#pragma pack(1)` is +commented out, so default alignment applies and both declarations come to 120 +bytes with `PID` at offset 88. + +**This declaration carried a `[limitation]` warning in v0.2.3 saying it was +suspect and wrong from `PID` onward. That warning was itself wrong** and is +removed. It came from a second-hand report that the header packs to one byte +and declares the flags as C++ `bool`; the header on the lab NAS does neither. +Nothing in this package calls `TLI_GetDeviceInfo`, so nothing depended on +either claim -- but a false warning costs the next reader a hunt for a defect +that is not there, which is why it is deleted rather than softened. """ 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..124459c 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), BOOL, (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), BOOL, (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), BOOL, (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), BOOL, (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), BOOL, (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), BOOL, (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}, BOOL, BOOL), 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}, BOOL), 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), BOOL, (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), BOOL, (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}, BOOL, __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), BOOL, (Ptr{Cchar},), serialNo) end function LD_RequestSettings(serialNo) From 7a4cb59ad669805c4d7be42585cc25ad7773b547 Mon Sep 17 00:00:00 2001 From: kalidke Date: Thu, 24 Sep 2026 09:48:02 -0600 Subject: [PATCH 2/4] Split the two Kinesis boolean roles; they have different right answers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rig repo diffed the installed vendor header against the copy on the lab NAS and settled this properly. The NAS copy is a Clang.jl generation input, hand-edited to parse without Windows headers: the vendor preamble was replaced with local typedefs including `typedef unsigned int BOOL`, `OaIdl.h` and `__declspec` were removed, both pack pragmas were commented out, and every lowercase `bool` was rewritten to `BOOL`. It carries no ABI information of its own, and I treated it as authoritative. The INSTALLED header has 32 lowercase `bool`, zero `BOOL`, and an active `#pragma pack(1)`. So v0.2.3 was half right and my revert was half wrong, and neither pure type is correct. The two roles differ: - `KBOOL_ARG = Cuint`, zero-extended 0 or 1, for arguments. Robust under either ABI: a `bool` callee reads the low byte, a `BOOL` callee reads four, both see 0 or 1. A 1-byte argument is NOT robust — the upper three bytes are undefined, so a `false` can arrive as true, and `LD_EnableMaxCurrentAdjust`'s second flag enables the diode. - `KBOOL_RET = Bool`, one byte, for returns. A `bool` return sets only `AL` and leaves the rest of `EAX` undefined, so reading four bytes can turn `false` into nonzero — `LD_CheckConnection` reporting a disconnected controller as connected. Reading the low byte is right under `bool` and still right under `BOOL` returning 0 or 1. Nine returns and four argument positions, split accordingly. `TLI_DeviceInfo`'s warning is restored and corrected rather than deleted: the real layout is 100 bytes with `PID` at 85 under `pack(1)` with 1-byte flags, against this declaration's verified 120 and 88. Still not fixed — correcting it needs field types AND packing, and a controller to read a device list back from. The docstring now names the NAS copy as untrustworthy so nobody relitigates this from it a third time. Suite: 959 passed, 8 broken, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) --- .../tcube_laser/constants_Tlaser.jl | 80 +++++++++++++++---- .../tcube_laser/functions_Tlaser.jl | 24 +++--- 2 files changed, 78 insertions(+), 26 deletions(-) diff --git a/src/hardware_implementations/tcube_laser/constants_Tlaser.jl b/src/hardware_implementations/tcube_laser/constants_Tlaser.jl index fa201b6..9327f85 100644 --- a/src/hardware_implementations/tcube_laser/constants_Tlaser.jl +++ b/src/hardware_implementations/tcube_laser/constants_Tlaser.jl @@ -10,10 +10,49 @@ const __int32 = Cint const BOOL = Cuint +""" + 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 KBOOL_RET = Bool + """ BOOL -The Kinesis headers `typedef unsigned int BOOL` (verified at line 37 of +Retained for `TLI_DeviceInfo`'s fields only. Note the Kinesis headers +`typedef unsigned int BOOL` (verified at line 37 of `Thorlabs.MotionControl.TCube.LaserDiode.h`), so this is four bytes, and every `BOOL` argument and return in `functions_Tlaser.jl` is that width. @@ -73,19 +112,32 @@ end """ TLI_DeviceInfo -Mirrors the `TLI_DeviceInfo` struct in the Kinesis headers. `BOOL` is four -bytes there (`typedef unsigned int BOOL`, line 37), the fields are in this -order, and the struct is NOT packed -- the header's `#pragma pack(1)` is -commented out, so default alignment applies and both declarations come to 120 -bytes with `PID` at offset 88. - -**This declaration carried a `[limitation]` warning in v0.2.3 saying it was -suspect and wrong from `PID` onward. That warning was itself wrong** and is -removed. It came from a second-hand report that the header packs to one byte -and declares the flags as C++ `bool`; the header on the lab NAS does neither. -Nothing in this package calls `TLI_GetDeviceInfo`, so nothing depended on -either claim -- but a false warning costs the next reader a hunt for a defect -that is not there, which is why it is deleted rather than softened. +**[limitation] This layout is wrong for the real DLL, and is left alone +deliberately.** + +The installed vendor header declares this struct under an ACTIVE +`#pragma pack(1)` with C++ `bool` flags. Packed, the real layout is +**100 bytes with `PID` at offset 85**: `typeID` 4 + `description` 65 + +`serialNo` 16 puts `PID` at 85 with no padding, and the five flags are one +byte each. This declaration uses 4-byte `BOOL` and default alignment, so it +measures **120 bytes with `PID` at offset 88** (verified by execution) and is +wrong from `PID` onward. + +Nothing in this package calls `TLI_GetDeviceInfo`, so this is latent rather +than a hazard, and correcting it means changing field types AND adding +packing — not a change to make without a controller to read a real device +list back from. Fix it against the installed header with the size asserted, +or do not call it. + +**A copy of this header on the lab NAS says otherwise; do not trust it.** That +copy (`Personal Folders/Sheng/code/generate_lib/lib/`) is a Clang.jl +generation input, hand-edited to parse without Windows headers: the vendor +preamble was replaced with local typedefs including +`typedef unsigned int BOOL`, `OaIdl.h` and `__declspec` were removed, both +pack pragmas were commented out, and every lowercase `bool` was rewritten to +`BOOL`. Those are artefacts of the editing, not the vendor's ABI. This +docstring briefly claimed, on the strength of that copy, that the layout was +correct; it is not. """ 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 124459c..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), BOOL, (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), BOOL, (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), BOOL, (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), BOOL, (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), BOOL, (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), BOOL, (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}, BOOL, BOOL), 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}, BOOL), 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), BOOL, (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), BOOL, (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}, BOOL, __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), BOOL, (Ptr{Cchar},), serialNo) + ccall((:LD_HasLastMsgTimerOverrun, Thorlabs_Tcube_laser), KBOOL_RET, (Ptr{Cchar},), serialNo) end function LD_RequestSettings(serialNo) From 36e1323d73d6fc749fe1f901fc8ec362c5425615 Mon Sep 17 00:00:00 2001 From: kalidke Date: Thu, 24 Sep 2026 12:47:05 -0600 Subject: [PATCH 3/4] Record both Kinesis header variants; no single struct layout fits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 405 rig read its installed Kinesis 1.14.10 header and confirmed the 642 rig's reading independently: 32 lowercase C++ `bool`, zero `BOOL`, no `BOOL` typedef, and `#pragma pack(1)` ACTIVE around `TLI_DeviceInfo`. It also found something neither of us had — the two versions differ in an ARRAY LENGTH: 1.14.10 char serialNo[9] the 642 rig char serialNo[16] So the struct layout varies by SDK version in more than one dimension, and no single declaration can be right for both. Ours matches neither: 120 bytes with `PID` at 88, against a packed 100-with-85 on one and something different again on the other. The docstring now records both observed variants as a table, states the policy (do not call `TLI_GetDeviceInfo` through this declaration; declare it per SDK version beside the header it came from, and assert `sizeof`), and confirms by inspection that `initialize` touches only the two counting `TLI_` functions, so nothing reaches the struct. It also names the NAS copy as untrustworthy, with the reason, so this is not relitigated from that file a fourth time. The boolean split stands and both rigs endorse it: 4-byte zero-extended arguments, low-byte returns. Suite: 959 passed, 8 broken, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) --- .../tcube_laser/constants_Tlaser.jl | 59 +++++++++++-------- 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/src/hardware_implementations/tcube_laser/constants_Tlaser.jl b/src/hardware_implementations/tcube_laser/constants_Tlaser.jl index 9327f85..1f675c3 100644 --- a/src/hardware_implementations/tcube_laser/constants_Tlaser.jl +++ b/src/hardware_implementations/tcube_laser/constants_Tlaser.jl @@ -112,32 +112,39 @@ end """ TLI_DeviceInfo -**[limitation] This layout is wrong for the real DLL, and is left alone -deliberately.** - -The installed vendor header declares this struct under an ACTIVE -`#pragma pack(1)` with C++ `bool` flags. Packed, the real layout is -**100 bytes with `PID` at offset 85**: `typeID` 4 + `description` 65 + -`serialNo` 16 puts `PID` at 85 with no padding, and the five flags are one -byte each. This declaration uses 4-byte `BOOL` and default alignment, so it -measures **120 bytes with `PID` at offset 88** (verified by execution) and is -wrong from `PID` onward. - -Nothing in this package calls `TLI_GetDeviceInfo`, so this is latent rather -than a hazard, and correcting it means changing field types AND adding -packing — not a change to make without a controller to read a real device -list back from. Fix it against the installed header with the size asserted, -or do not call it. - -**A copy of this header on the lab NAS says otherwise; do not trust it.** That -copy (`Personal Folders/Sheng/code/generate_lib/lib/`) is a Clang.jl -generation input, hand-edited to parse without Windows headers: the vendor -preamble was replaced with local typedefs including -`typedef unsigned int BOOL`, `OaIdl.h` and `__declspec` were removed, both -pack pragmas were commented out, and every lowercase `bool` was rewritten to -`BOOL`. Those are artefacts of the editing, not the vendor's ABI. This -docstring briefly claimed, on the strength of that copy, that the layout was -correct; it is not. +**[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 (405 rig) | the 642 rig's install | +|---|---|---| +| `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 From 8ee364dabaf7d8240c4c2ab657143adc6c5d42d8 Mon Sep 17 00:00:00 2001 From: kalidke Date: Thu, 24 Sep 2026 13:32:22 -0600 Subject: [PATCH 4/4] Correct the BOOL docstring against the vendor headers, and attach it Two rigs have now read their own vendor-installed Kinesis header -- 1.14.10 on seq-sr, 1.14.47.22504 on quickbeam -- and both declare lowercase C++ `bool` with `#pragma pack(1)` active. The `BOOL` docstring still asserted the opposite, on the strength of a copy on the NAS that turns out to be a Clang.jl generation input hand-edited to parse without the Windows SDK. That claim also contradicted the `KBOOL_RET` docstring three lines above it. The docstring was additionally unattached: it sat between `const KBOOL_RET` and `struct tagSAFEARRAYBOUND`, so Julia bound it to the SAFEARRAY bound struct rather than to `BOOL`. Moved above `const BOOL = Cuint`, where it belongs. No binding changes. `BOOL` is still four bytes and still used only by `TLI_DeviceInfo`, which nothing calls; it stays because the two vendor versions disagree on `serialNo`'s length, so there is no single layout to change it to. Also record the instrument archive: `manuals/` is a local-only, gitignored symlink to the lab-wide archive on the NAS, documented in CLAUDE.md with the reason it must never be committed. Local suite: 959 pass, 8 broken, 0 fail. Rebased onto main for its pull request: the two dev/output plan files are dropped (decision 0028; they stay in fix/revert-cppbool's history), CLAUDE.md and .gitignore keep main's text beside this commit's, and CHANGELOG gains the [Unreleased] entry for the Kinesis boolean split. Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Claude Opus 5.5 --- .gitignore | 2 + CHANGELOG.md | 10 ++++ CLAUDE.md | 47 +++++++++++++++++ .../tcube_laser/constants_Tlaser.jl | 51 ++++++++++--------- 4 files changed, 86 insertions(+), 24 deletions(-) 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 1f675c3..9206591 100644 --- a/src/hardware_implementations/tcube_laser/constants_Tlaser.jl +++ b/src/hardware_implementations/tcube_laser/constants_Tlaser.jl @@ -8,6 +8,32 @@ 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 """ @@ -48,29 +74,6 @@ is what is actually correct, and is safe under either reading. """ const KBOOL_RET = Bool -""" - BOOL - -Retained for `TLI_DeviceInfo`'s fields only. Note the Kinesis headers -`typedef unsigned int BOOL` (verified at line 37 of -`Thorlabs.MotionControl.TCube.LaserDiode.h`), so this is four bytes, and every -`BOOL` argument and return in `functions_Tlaser.jl` is that width. - -**History, because this was got wrong once.** v0.2.3 changed these to a -one-byte `Bool` on a report that the header declared C++ `bool`. It does not: -that header contains no lowercase `bool` at all. The change was reverted -because it is wrong in the dangerous direction for ARGUMENTS — passing one -byte where the callee reads four leaves the upper three undefined, so a -`false` can arrive as true. `LD_EnableMaxCurrentAdjust(serial, true, false)` -is the call that matters: its second flag enables the laser diode during a -max-current adjustment. - -If a future Kinesis version really does declare `bool`, check the header for -that version before changing this, and change arguments and returns -separately — four bytes is safe for an argument under either ABI, one byte is -not. -""" - struct tagSAFEARRAYBOUND cElements::Culong lLbound::Clong @@ -118,7 +121,7 @@ layout that would match both.** Two rigs read their installed headers and reported different declarations: -| | Kinesis 1.14.10 (405 rig) | the 642 rig's install | +| | 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` |