dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Merge branch 'worktree-agent-aaf0e64363ca270b1' into feat/scripted-alarm-shelve-routing

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-22 09:40:45 -04:00
parent 1a1b3df098 ee5d7ad51e
commit b21585767b
9 changed files with 372 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
code-reviews/Driver.AbLegacy/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 11 |
| Open findings | 3 |
## Checklist coverage
@@ -66,7 +66,7 @@ RMW arithmetic to the native width so sign-extension can no longer corrupt high
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `AbLegacyDriver.cs:368` |
| Status | Open |
| Status | Resolved |
**Description:** In `WriteBitInWordAsync` the parent word is decoded with
`Convert.ToInt32(parentRuntime.DecodeValue(AbLegacyDataType.Int, ...))`.
@@ -82,7 +82,7 @@ will break silently. Combined with finding 001 this is a latent correctness haza
operate on an explicitly 16-bit value, or document the reliance on low-16-bit
preservation explicitly.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `current & widthMask` already applied in `WriteBitInWordAsync` by the -001 fix; no additional change needed.
### Driver.AbLegacy-003
@@ -91,7 +91,7 @@ preservation explicitly.
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `AbLegacyAddress.cs:62-95` |
| Status | Open |
| Status | Resolved |
**Description:** `TryParse` does not reject several malformed PCCC addresses that the
XML docs imply are invalid:
@@ -108,7 +108,7 @@ through to libplctag rather than rejected early with a clear error.
reject file numbers on I/O/S, and restrict which file letters may carry a sub-element
(T/C/R only). Add unit coverage for the rejection cases.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `TryParse` now rejects sub-element+bit-index combinations, file numbers on I/O/S files, and sub-elements on non-T/C/R files; unit tests added in `AbLegacyAddressTests`.
### Driver.AbLegacy-004
@@ -117,7 +117,7 @@ reject file numbers on I/O/S, and restrict which file letters may carry a sub-el
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `LibplctagLegacyTagRuntime.cs:36-37` |
| Status | Open |
| Status | Resolved |
**Description:** `DecodeValue` for `AbLegacyDataType.Bit` with `bitIndex == null`
returns `_tag.GetInt8(0) != 0`. A bit-file element (`B3:0/0`) is a single bit inside
@@ -132,7 +132,7 @@ but a `Bit`-typed tag configured with an address that has no `/bit` suffix (e.g.
bit suffix on `Bit`-typed tags (validate in `CreateInstance`/`DiscoverAsync`) or
decode the full 16-bit word and test bit 0.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `DecodeValue` for `Bit` with no `bitIndex` now reads the full 16-bit word via `GetInt16(0)` and tests bit 0, avoiding the silent half-word truncation from `GetInt8`.
### Driver.AbLegacy-005
@@ -194,7 +194,7 @@ shared libplctag `Tag` handle is never touched by two threads at once.
| Severity | Medium |
| Category | Concurrency & thread safety |
| Location | `AbLegacyDriver.cs:411-438`, `AbLegacyDriver.cs:386-409` |
| Status | Open |
| Status | Resolved |
**Description:** `EnsureTagRuntimeAsync` and `EnsureParentRuntimeAsync` are
check-then-act: `device.Runtimes.TryGetValue(...)` then, after `await
@@ -209,7 +209,7 @@ corrupt internal state. `ParentRuntimes` has the identical pattern.
`GetOrAdd`, or guard runtime creation under a per-device lock. Ensure the losing
runtime of any race is disposed.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `Runtimes` and `ParentRuntimes` changed to `ConcurrentDictionary`; `EnsureTagRuntimeAsync` and `EnsureParentRuntimeAsync` now hold a per-key `GetCreationLock` semaphore around the double-checked create+initialize+store sequence so exactly one runtime is created per key and no race-loser is leaked.
### Driver.AbLegacy-008
@@ -218,7 +218,7 @@ runtime of any race is disposed.
| Severity | Medium |
| Category | Concurrency & thread safety |
| Location | `AbLegacyDriver.cs:21`, `AbLegacyDriver.cs:138-146`, `AbLegacyDriver.cs:216-229` |
| Status | Open |
| Status | Resolved |
**Description:** `_health` is a plain non-volatile reference field mutated from
`ReadAsync`, `WriteAsync` (both can run on multiple threads / poll loops) and
@@ -233,7 +233,7 @@ successful read can clobber a `Degraded` write from a concurrent failing read.
lock / `Interlocked.Exchange`. Consider only downgrading on failure and upgrading on a
successful poll so a single failed read does not flap the surface.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `_health` marked `volatile`; memory barrier comment documents the acquire/release ordering guarantee.
### Driver.AbLegacy-009
@@ -242,7 +242,7 @@ successful poll so a single failed read does not flap the surface.
| Severity | Medium |
| Category | Error handling & resilience |
| Location | `AbLegacyDriver.cs:41-74` |
| Status | Open |
| Status | Resolved |
**Description:** `InitializeAsync` starts probe loops with `Task.Run` inside the try
block. If `InitializeAsync` fails - or is re-entered - after some probe loops are
@@ -257,7 +257,7 @@ and `CancellationTokenSource`s alive holding libplctag handles. Separately,
`ShutdownAsync` (cancel probe CTSs, dispose runtimes, clear dictionaries) before
rethrowing, so a failed initialise leaves no live background work.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `InitializeAsync` catch block now cancels and disposes probe CTSs, calls `DisposeRuntimes`, and clears `_devices`/`_tagsByName` before rethrowing, leaving no orphaned background tasks or handles.
### Driver.AbLegacy-010
@@ -266,7 +266,7 @@ rethrowing, so a failed initialise leaves no live background work.
| Severity | Medium |
| Category | Error handling & resilience |
| Location | `AbLegacyStatusMapper.cs:26-56` |
| Status | Open |
| Status | Resolved |
**Description:** `MapLibplctagStatus` maps the integer codes -5/-7/-14/-16/-17. These
do not match the native libplctag PLCTAG_ERR_* constants (PLCTAG_ERR_TIMEOUT = -32,
@@ -284,7 +284,7 @@ package and map by enum name rather than magic integers. Either wire `MapPcccSta
into a real PCCC-STS path or delete it as dead code. The same defect exists in
`AbCipStatusMapper` and should be fixed in lockstep.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `MapLibplctagStatus` now casts to `libplctag.Status` and switches on named enum members (matching the AbCip mapper pattern); `MapPcccStatus` retained with a comment documenting it as a reference mapping for future PCCC-STS inspection; tests updated to use `Status` enum members.
### Driver.AbLegacy-011
@@ -315,7 +315,7 @@ rather than blocking on the async path.
| Severity | Medium |
| Category | Design-document adherence |
| Location | `PlcFamilies/AbLegacyPlcFamilyProfile.cs:7-54`, `AbLegacyDriver.cs:48-52` |
| Status | Open |
| Status | Resolved |
**Description:** `AbLegacyPlcFamilyProfile` declares four record properties -
`DefaultCipPath`, `MaxTagBytes`, `SupportsStringFile`, `SupportsLongFile` - and only
@@ -336,7 +336,7 @@ the host CIP path is empty; reject `Long`/`String` tags against families whose p
sets the corresponding flag false; use `MaxTagBytes` for validation) or remove the
unused fields and the doc comments that imply they are load-bearing.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `DeviceState.EffectiveCipPath` applies `DefaultCipPath` when the parsed host address has an empty CIP path; `InitializeAsync` validates `Long`/`String` tag types against `SupportsLongFile`/`SupportsStringFile` and throws early; `MaxTagBytes` tracked as a follow-up (string/array chunking requires broader design work).
### Driver.AbLegacy-013