TCube laser: pass Kinesis boolean arguments as four bytes - #70
Merged
Merged
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <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.
Keith's
fix/revert-cppboolwork, intomain: the Kinesis boolean binding split, plus themanuals/archive note.What changes
Bool. The headers declareboolean arguments as a four-byte type, so an argument is now a zero-extended
Cuint(KBOOL_ARG), andreturns stay one byte (
KBOOL_RET). The affected arguments areLD_EnableMaxCurrentAdjust,LD_EnableTIAGainAdjustandLD_EnableLastMsgTimer. No driver method calls those three yet, so no currentbehaviour changes; not yet run on hardware. The vendor facts are in
manuals/Thorlabs/TLD001/BINDING.md.manuals/is documented in CLAUDE.md as a local-only, gitignored symlink to the lab's instrument archive on theNAS, and it is added to
.gitignore.[Unreleased]→ Fixed has the entry.How this branch was made
fix/revert-cppbool, cherry-picked ontomainas of Fold 0.3rc1 into main; main is the development branch at 0.2.5-DEV #69. That branch is untouched.dev/output/are dropped here. Plans live in the gitignoreddev/output(decision 0028), and the files stay infix/revert-cppbool's history..gitignoreconflicted with Fold 0.3rc1 into main; main is the development branch at 0.2.5-DEV #69. The resolution keeps main's text beside the branch's, and the fourthcommit's message says so.
fix/revert-cppboolinto Fold 0.3rc1 into main; main is the development branch at 0.2.5-DEV #69's head, which is the tree the suite ran on.Compatibility (decisions 0033 and 0035)
-DEV.Tests
xvfb-run -a): 998 pass, 8 broken, 0 fail.🤖 Generated with Claude Code