Joseph Doherty
fcd36bd53d
STATUS.md + 05 plan banner: crit3 branch 25c0c6f6, IS7Plc seam + lazy reconnect,
4 unit guards + full S7 suite, LIVE snap7 container-bounce proof (recovers with no
redeploy). Next: Critical 4 (TwinCAT ADS re-registration, unit-only — no TC3 fixture).
815 lines
51 KiB
Markdown
815 lines
51 KiB
Markdown
# Design + Implementation Plan — 05 Protocol Drivers
|
||
|
||
> **Status (2026-07-08):** **STAB-1 (S7 reconnect, Critical, task 4) DONE** — `fix/archreview-crit3-s7-reconnect`
|
||
> `25c0c6f6`; `IS7Plc`/`IS7PlcFactory` seam + lazy `EnsureConnectedAsync`; 4 unit guards + LIVE-verified
|
||
> against real snap7 (container bounce → recovers, no redeploy). **STAB-2 (TwinCAT orphaned ADS subs,
|
||
> Critical, task 5) NEXT** — unit-only (no TwinCAT docker fixture). Then STAB-4/5 (AbCip lock, FOCAS caches)
|
||
> + Part B shared-template consolidation. See [`STATUS.md`](STATUS.md).
|
||
|
||
**Source report:** `archreview/05-protocol-drivers.md`
|
||
**Review commit:** `9cad9ed0` (== current HEAD, so no drift)
|
||
**Verification date:** 2026-07-08
|
||
**Scope:** Modbus, S7, AbCip, AbLegacy, TwinCAT, FOCAS, OpcUaClient (+ Contracts, + Cli)
|
||
|
||
---
|
||
|
||
## Verification summary
|
||
|
||
Every finding in the report was opened at its cited file:line and checked against
|
||
the current tree. **37 actionable findings; 37 CONFIRMED; 0 stale.** (STAB-13 is a
|
||
"positive" observation carrying no work.) The line references were accurate to
|
||
within 1–2 lines throughout. The zero-drift result is expected: the review commit
|
||
is the current HEAD.
|
||
|
||
Confirmed counts by section: Stability 12 actionable (STAB-1…12) + 1 positive
|
||
(STAB-13); Performance 7 (PERF-1…7); Conventions 8 (CONV-1…8); Underdeveloped 9
|
||
(UNDER-1…9).
|
||
|
||
**Top 3 by priority** (align with OVERALL actions #3/#4 and report remediation
|
||
order):
|
||
1. **STAB-1 — S7 no reconnect path** (Critical). `Plc.OpenAsync` only at
|
||
`S7Driver.cs:165`; grep confirms it is the *sole* open site, no `EnsureConnected`.
|
||
2. **STAB-2 — TwinCAT native subscriptions orphaned after reconnect** (Critical).
|
||
`NativeSubscription` (`TwinCATDriver.cs:517-519`) stores only disposable handles,
|
||
not replayable intent; `EnsureConnectedAsync` swaps the client
|
||
(`:614-644`) and nothing re-runs `AddNotificationAsync`; `AdsTwinCATClient.Dispose`
|
||
clears `_notifications` (`:426`).
|
||
3. **STAB-4 + STAB-5 — silent-corruption class.** AbCip runs Read→GetStatus→Decode
|
||
on shared libplctag handles with no per-runtime lock (`AbCipDriver.cs:544-575`;
|
||
only `GetRmwLock` exists, bit-writes only), while AbLegacy guards the identical
|
||
hazard with `GetRuntimeLock` (`AbLegacyDriver.cs:786`, used at `:227`/`:331`).
|
||
FOCAS's fixed-tree caches are plain `Dictionary` (`FocasDriver.cs:1184,1190,1192,1194`)
|
||
read/written across threads.
|
||
|
||
The two systemic threads the report calls out — **theme #2** (fixes don't flow back
|
||
to shared templates) and **theme #6** (equipment-tag/TagConfig paths are
|
||
second-class) — are both real and are given a dedicated consolidation section
|
||
(Part B) so the fleet fixes land in one home rather than being re-patched per driver.
|
||
|
||
---
|
||
|
||
# Part A — Per-driver Criticals & Highs
|
||
|
||
## A1. STAB-1 (Critical) — S7 has no reconnect path
|
||
|
||
**Restatement:** the S7 `Plc` is opened once in `InitializeAsync`; a transient PLC
|
||
reboot/network blip permanently kills the instance until redeploy.
|
||
|
||
**Verification:** CONFIRMED. `grep OpenAsync S7Driver.cs` → single hit at line 165,
|
||
inside `InitializeAsync`. No `EnsureConnected`/`Reconnect` symbol. Reads/writes call
|
||
`RequirePlc()` under `_gate` and reuse the dead handle
|
||
(`S7Driver.cs:479-483, 995-1001`). The probe loop detects the outage and transitions
|
||
`Stopped` but never recycles. `ReinitializeAsync` (`:212`) just re-calls `InitializeAsync`
|
||
(externally driven only). Zero S7 reconnect tests (`tests/Drivers/.../S7.Tests` has no
|
||
reconnect file; only `Modbus`, `OpcUaClient` do).
|
||
|
||
**Root cause:** the `Plc` is a concrete `S7.Net` type `new`-ed inline
|
||
(`S7Driver.cs:156`, field at `:122`), with no factory seam and no lazy-connect
|
||
gate. The template calls for a lazy `EnsureConnectedAsync` (TwinCAT/AbCip have one;
|
||
S7 never got it).
|
||
|
||
**Design (mirror TwinCAT + OpcUaClient):**
|
||
- Introduce a testable seam: `IS7Plc` (wrapping the members S7Driver uses:
|
||
`OpenAsync`, `Close`, `ReadBytesAsync`/`ReadMultiple`, `Write*`, `IsConnected`)
|
||
and `IS7PlcFactory` with `Create(cpu, host, port, rack, slot)`. Default impl wraps
|
||
`S7.Net.Plc`; the ctor takes an optional `IS7PlcFactory? = null` defaulting to the
|
||
real one — exactly TwinCAT's `ITwinCATClientFactory` pattern
|
||
(`TwinCATDriver.cs:20,58,64`).
|
||
- Add `private async Task<IS7Plc> EnsureConnectedAsync(CancellationToken ct)` guarded
|
||
by the existing `_gate`: fast-path returns a live `Plc`; under the gate, if the
|
||
`Plc` is null or a prior socket-level `PlcException` marked it dead, dispose and
|
||
re-`OpenAsync` a fresh instance (with the same timeout-linked CTS as `:163-164`).
|
||
- Route `ReadAsync`/`WriteAsync`/`PollOnceAsync` through `EnsureConnectedAsync`
|
||
instead of `RequirePlc()`. On a socket-level `PlcException` during a read/write,
|
||
null the `Plc` (mark dead) so the next call reopens, and set `Degraded` (keep
|
||
`LastSuccessfulRead`) rather than `Faulted`. Preserve the existing `Faulted`
|
||
escalation for PUT/GET-denied (`S7Driver.cs:921-1006`) — that is a permanent
|
||
config fault, not a transient drop.
|
||
- Classify socket-level vs protocol-level `PlcException` in a small
|
||
`IsS7ConnectionFatal(Exception)` helper (S7.Net surfaces `ErrorCode.ConnectionError`
|
||
/ `WriteData` / `ReadData` / IO/socket exceptions). Do not reopen on a
|
||
data-address/type error.
|
||
|
||
**Alternatives considered:** (a) let the probe loop own recycle-on-Stopped — rejected
|
||
as sole mechanism because reads between probe ticks would keep failing on the dead
|
||
handle; the lazy `EnsureConnectedAsync` repairs on the very next data call and the
|
||
probe becomes a backstop. (b) Reconnect handler à la OpcUaClient's
|
||
`SessionReconnectHandler` — overkill; S7.Net has no session-transfer semantics, a
|
||
plain reopen is correct.
|
||
|
||
**Implementation steps:**
|
||
1. New file `Driver.S7/IS7Plc.cs` (+ `S7PlcAdapter`, `IS7PlcFactory`,
|
||
`S7PlcFactory`). Change `S7Driver.Plc` field type to `IS7Plc?`.
|
||
2. Thread `IS7PlcFactory` through the ctor + `S7DriverFactoryExtensions.CreateInstance`.
|
||
3. Add `EnsureConnectedAsync` + `IsS7ConnectionFatal`; convert the two `RequirePlc`
|
||
call clusters and `PollOnceAsync` (`:1295-1309`).
|
||
4. Optionally: on probe transition to `Stopped`, null the `Plc` so the next data
|
||
call reconnects even if no read failed in between.
|
||
|
||
**Tests:**
|
||
- Unit (new `S7DriverReconnectTests.cs`): fake `IS7PlcFactory` whose `IS7Plc` throws
|
||
a connection-fatal `PlcException` on read N, succeeds on read N+1; assert the driver
|
||
creates a *second* `IS7Plc`, health goes `Degraded`→`Healthy`, and the tag recovers
|
||
to Good. Assert a protocol error (illegal address) does **not** trigger a reopen.
|
||
- Integration (`S7.IntegrationTests`, skip-gated on `Snap7ServerFixture`): subscribe,
|
||
bounce the snap7 container (`lmxopcua-fix down s7 s7_1500` / `up`), assert values
|
||
resume without redeploy. Note: no per-test container hook exists, so gate this as a
|
||
manual/`[Trait("Category","Reconnect")]` recipe.
|
||
|
||
**Effort:** M. **Risk:** Medium — touches the hot read/write path; the `IS7Plc` seam
|
||
is mechanical but broad. Mitigated by the fake-factory unit tests (which also close
|
||
the TEST-1 gap).
|
||
|
||
## A2. STAB-2 (Critical) — TwinCAT native subscriptions orphaned after reconnect
|
||
|
||
**Restatement:** native ADS notifications (the *default* mode) silently stop after a
|
||
client swap — no Bad status, no error.
|
||
|
||
**Verification:** CONFIRMED. `SubscribeAsync` registers `AddNotificationAsync` once
|
||
per tag (`TwinCATDriver.cs:472`) and stores the resulting handles in
|
||
`NativeSubscription(Handle, Registrations)` (`:495, :517-519`) — **no** record of the
|
||
symbol/type/interval/handler needed to replay. `EnsureConnectedAsync` disposes a
|
||
stale client and creates a fresh one (`:614-644`); `AdsTwinCATClient.Dispose` clears
|
||
`_notifications` (`:426`). Compounding: the fast path keys on `IsConnected` (`:606`),
|
||
the local AMS-port state, not wire liveness; and a probe failure only transitions
|
||
state, never forces a client recycle (`ProbeLoopAsync :540-547`).
|
||
|
||
**Root cause:** subscription state is stored as *opaque handles* rather than
|
||
*replayable intent*, and there is no "client changed → replay" hook. Native ADS is
|
||
push, so unlike the poll fallback it cannot self-heal by re-reading.
|
||
|
||
**Design:**
|
||
- Change `NativeSubscription` to store replayable registrations:
|
||
`record NativeRegistrationIntent(string DeviceHostAddress, string Reference,
|
||
string SymbolName, TwinCATDataType Type, int? BitIndex, TimeSpan Interval,
|
||
Action<string,object?> OnChange)` plus the live `ITwinCATNotificationHandle`.
|
||
- Give `DeviceState` an epoch/generation counter incremented every time
|
||
`EnsureConnectedAsync` installs a *new* client (`:644`). After installing a new
|
||
client, replay every `NativeRegistrationIntent` whose `DeviceHostAddress` matches:
|
||
call `AddNotificationAsync` again and swap the stored live handle. Do this inside
|
||
`EnsureConnectedAsync` under `ConnectGate` so it cannot race a concurrent subscribe.
|
||
- Fix the liveness signal: the probe should call a real read (`ProbeAsync` already
|
||
does a symbol read) and, on failure, dispose+null the device client so
|
||
`EnsureConnectedAsync` rebuilds — i.e. make probe-detected outage force a recycle
|
||
(`ProbeLoopAsync` catch block at `:541-545` should null `device.Client`). Consider
|
||
keying the fast path (`:606`) additionally on a "known-good since last probe" flag
|
||
so a locally-connected-but-wire-dead port still triggers the rebuild-and-replay.
|
||
|
||
**Alternatives:** switch the default to the poll fallback (self-healing) — rejected;
|
||
native push is the performance-correct default and the report explicitly wants replay,
|
||
not abandonment.
|
||
|
||
**Implementation steps:**
|
||
1. Introduce a per-device `ConcurrentDictionary<subId, List<NativeRegistrationIntent>>`
|
||
(or hang intents off `DeviceState`). Populate in `SubscribeAsync`.
|
||
2. Extract the per-tag registration into `RegisterNotificationAsync(device, intent)`
|
||
used by both initial subscribe and replay.
|
||
3. In `EnsureConnectedAsync`, after `device.Client = client` (`:644`), iterate the
|
||
device's intents and replay; log a re-registration count.
|
||
4. In `ProbeLoopAsync`, on probe failure null the device client (force rebuild).
|
||
|
||
**Tests:**
|
||
- Unit (new `TwinCATReconnectReplayTests.cs`) using the existing fake
|
||
`ITwinCATClientFactory`/`ITwinCATClient`: subscribe a tag, simulate a client that
|
||
reports `IsConnected=false`, force `EnsureConnectedAsync` to build a replacement,
|
||
assert `AddNotificationAsync` is re-invoked with the same symbol/type/interval and
|
||
that a subsequent fake push reaches `OnDataChange`. Assert the old handle is disposed.
|
||
- Unit: probe-failure recycles the client (assert a new client instance built).
|
||
- Note there is **no** TwinCAT docker fixture (13 integration tests need a real TC3
|
||
XAR); the fake-client unit tests are the only automated coverage — make them
|
||
authoritative.
|
||
|
||
**Effort:** M. **Risk:** Medium — concurrency around `ConnectGate`/replay; must not
|
||
double-register. The fake client makes it fully unit-testable (the report's key gap).
|
||
|
||
## A3. STAB-3 (High) — Modbus transport desyncs after a response timeout
|
||
|
||
**Restatement:** a per-op timeout or TxId mismatch leaves the single-flight socket
|
||
half-read; the next txn reads a stale response and the stream stays desynchronized.
|
||
|
||
**Verification:** CONFIRMED. `IsSocketLevelFailure` (`ModbusTcpTransport.cs:257-261`)
|
||
matches only `EndOfStreamException`/`IOException`/`SocketException`/
|
||
`ObjectDisposedException`. The per-op deadline (`:226-227` linked CTS `CancelAfter`)
|
||
surfaces as `OperationCanceledException`; the TxId guard throws `InvalidDataException`
|
||
(`:234-235`); the truncated-length guard throws `InvalidDataException` (`:237`) — none
|
||
of which are socket-level, so the reconnect-and-retry catch (`:155-165`) is skipped
|
||
and the socket is retained mid-desync.
|
||
|
||
**Root cause:** the transport conflates "protocol-layer error, keep the socket" with
|
||
"framing/timeout error, the socket is unusable." Timeout and any framing violation
|
||
leave unknown bytes in the receive buffer; the connection is fatally desynchronized
|
||
even though the exception type is not `IOException`.
|
||
|
||
**Design:** treat per-op timeout and any framing violation as **connection-fatal**:
|
||
tear the socket down *before* propagating, so the next `SendAsync` reconnects clean.
|
||
- In `SendOnceAsync`, wrap the read/write in a try that, on `OperationCanceledException`
|
||
where the *caller's* `ct` is NOT cancelled (i.e. the timeout linked-CTS fired), and
|
||
on `InvalidDataException`, calls `TearDownAsync()` then rethrows a normalized
|
||
`ModbusTransportDesyncException : IOException` (subclassing IOException lets the
|
||
existing `:155` catch optionally reconnect-and-retry once). Distinguish caller
|
||
cancellation (propagate as OCE, no teardown) from timeout (teardown).
|
||
- Simplest correct implementation: after `TearDownAsync`, rethrow as `IOException`
|
||
so both the retry classifier and the desync-repair share one path.
|
||
|
||
**Alternatives:** add timeout/framing to `IsSocketLevelFailure` directly — works but
|
||
must still tear down the socket first (the current retry path does `TearDownAsync`
|
||
before reconnect, so adding the types *and* ensuring teardown covers it). Preferred to
|
||
keep `IsSocketLevelFailure` about exception *type* and add an explicit teardown at the
|
||
throw site for framing/timeout, since those need teardown even when auto-reconnect is
|
||
off.
|
||
|
||
**Implementation steps:**
|
||
1. In `SendOnceAsync`, catch `OperationCanceledException` (timeout branch) and
|
||
`InvalidDataException`; `await TearDownAsync()`; rethrow as `IOException`
|
||
(`ModbusTransportDesyncException`).
|
||
2. Keep the outer `SendAsync` single-retry (`:155-165`) — it now covers the desync.
|
||
3. Ensure teardown even when `_autoReconnect` is false (the socket must not be reused
|
||
desynced regardless).
|
||
|
||
**Tests:**
|
||
- Unit (`ModbusTcpReconnectTests.cs` extension): a fake stream that (a) stalls past
|
||
the deadline, (b) returns a wrong TxId, (c) returns a truncated header. Assert each
|
||
triggers `TearDownAsync` (socket disposed) and the following call reconnects. Assert
|
||
a caller-cancellation (external ct) does NOT tear the socket down.
|
||
- Integration (`Modbus.IntegrationTests` + `exception_injector`): the existing rig
|
||
already forces FC exceptions; add a timeout scenario (non-responding unit) and
|
||
assert recovery on the next poll.
|
||
|
||
**Effort:** S. **Risk:** Low-Medium — must get the timeout-vs-cancellation distinction
|
||
right or a legitimate shutdown could look like a desync (harmless, just an extra
|
||
reconnect).
|
||
|
||
## A4. STAB-4 (High) — AbCip concurrent ops on shared libplctag handles, no lock
|
||
|
||
**Restatement:** AbCip read/group/write run Read→GetStatus→Decode /
|
||
Encode→Write→GetStatus on cached `IAbCipTagRuntime` with zero serialization while the
|
||
server read path, poll loops, and the alarm-projection loop can hit the same handle.
|
||
|
||
**Verification:** CONFIRMED. `ReadSingleAsync` (`AbCipDriver.cs:544-575`),
|
||
`ReadGroupAsync` (`:614-637`), `WriteAsync` (`:708-726`) take no runtime lock; the only
|
||
locks are `GetRmwLock` (bit-in-DINT writes, `:1243`) and a creation lock. AbLegacy
|
||
documents the exact hazard and guards it: `GetRuntimeLock` (`AbLegacyDriver.cs:786`),
|
||
applied on both read (`:227`) and write (`:331`), comment "A libplctag `Tag` handle is
|
||
not safe for concurrent Read/GetStatus/DecodeValue." `AbCipAlarmProjection.cs:149/228`
|
||
reads through the unlocked path. libplctag's `GetStatus` returns the *last* operation's
|
||
status, so concurrent Read/GetStatus can cross-attribute results → torn values with
|
||
Good status.
|
||
|
||
**Root cause:** the AbLegacy fix never propagated to AbCip (theme #2). AbCip caches a
|
||
per-tag runtime but never serializes operations on it.
|
||
|
||
**Design:** port AbLegacy's per-runtime operation lock verbatim.
|
||
- Add `GetRuntimeLock(string tagName)` to AbCip's `DeviceState` (a
|
||
`ConcurrentDictionary<string,SemaphoreSlim>` keyed by tag name, matching
|
||
`AbLegacyDriver.cs:786`).
|
||
- Wrap the whole Read/GetStatus/Decode critical section in `ReadSingleAsync` and
|
||
`ReadGroupAsync`, and the Encode/Write/GetStatus section in `WriteAsync`, in
|
||
`await opLock.WaitAsync(ct)` / `finally Release`. The existing `GetRmwLock` (bit RMW)
|
||
must nest *inside or replace-by* the runtime lock consistently — since RMW does
|
||
read-modify-write on the same handle, take the runtime lock for the whole RMW too
|
||
(this also fixes STAB-11's "direct parent write bypasses the lock" for AbCip: a
|
||
direct parent write and an RMW now serialize on the same per-tag runtime lock).
|
||
|
||
**Alternatives:** a single per-device lock — rejected; too coarse, serializes
|
||
independent tags. Per-runtime lock matches AbLegacy and preserves cross-tag parallelism.
|
||
|
||
**Implementation steps:**
|
||
1. Add `_runtimeLocks` + `GetRuntimeLock` to AbCip `DeviceState`.
|
||
2. Wrap the three op bodies. Ensure eviction-on-failure (`EvictRuntime`, `:554`) also
|
||
disposes/removes the tag's lock or leaves it (a fresh runtime under the same name
|
||
reuses the lock — fine).
|
||
3. Reconcile RMW: make `WriteBitInDIntAsync` acquire the per-runtime lock instead of
|
||
(or in addition to) `GetRmwLock`; collapse to one lock domain per parent to also
|
||
close STAB-11.
|
||
|
||
**Tests:**
|
||
- Unit (new `AbCipRuntimeConcurrencyTests.cs`, mirror `AbLegacyRuntimeConcurrencyTests`):
|
||
a fake `IAbCipTagRuntime` that asserts no overlapping Read/GetStatus (e.g. a reentrancy
|
||
counter that throws if >1); hammer concurrent `ReadAsync`+`WriteAsync`+alarm-read on
|
||
one tag; assert serialization and no torn/cross-attributed status.
|
||
- Integration (`abcip` fixture on `:44818`): concurrent subscribe + write to overlapping
|
||
tags, assert Good values stay coherent.
|
||
|
||
**Effort:** S. **Risk:** Low — additive locking, blast radius is AbCip only.
|
||
|
||
## A5. STAB-5 (High) — FOCAS fixed-tree caches are plain Dictionaries mutated across threads
|
||
|
||
**Restatement:** `LastFixedSnapshots`/`LastServoLoads`/`LastSpindleLoads`/`LastTimers`
|
||
are plain `Dictionary`, written by `FixedTreeLoopAsync` and read on `ReadAsync` threads.
|
||
|
||
**Verification:** CONFIRMED. Fields at `FocasDriver.cs:1184` (`Dictionary<string,double>`),
|
||
`:1190` (`Dictionary<FocasTimerKind,FocasTimer> = []`), `:1192`
|
||
(`Dictionary<string,double>`), `:1194` (`Dictionary<int,int> = []`). Writes at
|
||
`:752, :793, :836-839`; reads at `:912, :920, :945`. Contrast the neighbouring
|
||
`_parsedAddressesByTagName = new ConcurrentDictionary` (`:40`) — that field got the
|
||
treatment, these did not. Concurrent write-during-`Dictionary`-resize is UB (throw or
|
||
corruption).
|
||
|
||
**Root cause:** partial application of the concurrent-collection fix (theme #2).
|
||
|
||
**Design:** convert the four caches to `ConcurrentDictionary<,>`. `TryGetValue` and
|
||
indexer-set are the only operations used, both supported. `LastTimers` and
|
||
`LastSpindleLoads` collection-initializer `[]` becomes `new()`.
|
||
- Alternative (immutable snapshot swap): build a fresh dictionary per loop pass and
|
||
`Volatile.Write` the reference; readers `Volatile.Read`. Cleaner isolation but more
|
||
churn; `ConcurrentDictionary` is the lower-risk minimal change and matches the
|
||
in-tree precedent (`:40`).
|
||
|
||
**Implementation steps:** change the four field types (on `DeviceState`) + collection
|
||
initializers; no call-site changes needed (indexer + `TryGetValue` are identical).
|
||
|
||
**Tests:**
|
||
- Unit (`FocasDriver.Tests`): a stress test spinning `FixedTreeLoopAsync`-style writes
|
||
while readers call `TryReadFixedTree`; assert no exception over N iterations (would
|
||
reliably throw on the plain `Dictionary` today).
|
||
|
||
**Effort:** S. **Risk:** Very low — type-only change, FOCAS-local.
|
||
|
||
## A6. STAB-6 (High) — AbLegacy probe-loop shutdown race; S7 unsubscribe race
|
||
|
||
**Verification:** CONFIRMED both.
|
||
- AbLegacy: probe task fire-and-forgot `_ = Task.Run(...)` (`AbLegacyDriver.cs:118`);
|
||
`ShutdownAsync` (`:156-170`) cancels then immediately disposes `ProbeCts` + runtimes
|
||
without awaiting loop exit — a winding-down loop can raise `OnHostStatusChanged` and
|
||
touch a disposed CTS. AbCip already fixed this: retained `ProbeTask` (`:298`) + phased
|
||
shutdown with `await probeTask.WaitAsync(10s)` (`:332-365`).
|
||
- S7: `UnsubscribeAsync` (`:1185-1193`) cancels + disposes the CTS without draining
|
||
`PollTask`; `Task.Delay` on the disposed source can throw `ObjectDisposedException`
|
||
the loop only catches as OCE (`:1233-1234`). `ShutdownAsync` *does* drain PollTask
|
||
(`:239`), so the bug is confined to the `Unsubscribe` path.
|
||
|
||
**Root cause:** cancel-then-dispose-without-await; `PollGroupEngine.StopState`
|
||
(`PollGroupEngine.cs:99-112`) already solves this by awaiting the loop (bounded) before
|
||
disposing the CTS — neither bespoke path adopted it.
|
||
|
||
**Design:**
|
||
- AbLegacy: retain the probe task in `DeviceState.ProbeTask` and phase shutdown like
|
||
AbCip (`await probeTask.WaitAsync(bounded)` before `ProbeCts.Dispose()`).
|
||
- S7: in `UnsubscribeAsync`, `Cancel()`, then `state.PollTask.Wait(bounded)` (or await),
|
||
then `Cts.Dispose()` — mirroring `StopState`. Better: once S7 migrates to
|
||
`PollGroupEngine` (see B1) this deletes itself.
|
||
|
||
**Tests:** unit — subscribe/unsubscribe rapidly in a loop and assert no
|
||
`ObjectDisposedException` escapes; shutdown while a probe tick is mid-transition.
|
||
|
||
**Effort:** S. **Risk:** Low.
|
||
|
||
## A7. UNDER-1 (High) — FOCAS advertises writes it cannot perform
|
||
|
||
**Verification:** CONFIRMED. `WireFocasClient.WriteAsync` returns
|
||
`FocasStatusMapper.BadNotWritable` for every address (`WireFocasClient.cs:71-73`), yet
|
||
`FocasDriverOptions.Writable` defaults `true` (`FocasDriverOptions.cs:168`) and
|
||
`FocasEquipmentTagParser` hard-codes `Writable: true` (`FocasEquipmentTagParser.cs:35`).
|
||
The driver's write exception-mapping (`FocasDriver.cs:353-381`) is dead code against
|
||
this backend. Net: equipment tags surface as Operate-writable OPC UA nodes whose writes
|
||
always fail.
|
||
|
||
**Root cause:** the writable default was written speculatively (doc cites "write tests")
|
||
but the only real backend is read-only.
|
||
|
||
**Design (force writable-false at the seams until PMC writes exist):**
|
||
- `FocasEquipmentTagParser.cs:35` → `Writable: false`.
|
||
- `FocasDriverOptions.Writable` default → `false` (and/or force it false at discovery
|
||
materialization). Keep the option field for the future PMC-write feature but do not
|
||
advertise writable nodes today.
|
||
- Leave the write status-mapping in place (it becomes live if/when PMC writes ship).
|
||
- Alternative (larger): implement PMC writes in `WireFocasClient` via `pmc_wrpmcrng` —
|
||
a separate feature, out of scope for a hardening pass.
|
||
|
||
**Tests:** unit — assert a FOCAS equipment/authored tag materializes without the
|
||
`CurrentWrite` access bit; assert a write attempt returns `BadNotWritable` (guards the
|
||
regression if someone flips the default back).
|
||
|
||
**Effort:** S. **Risk:** Very low (removes a false capability).
|
||
|
||
## A8. UNDER-2 / UNDER-3 (High) — OpcUaClient: dead `UnsMappingTable` gate; broken alarm-filter encoding
|
||
|
||
**UNDER-2 verification:** CONFIRMED. `ValidateNamespaceKind` hard-fails Equipment-kind
|
||
configs lacking `UnsMappingTable` (`OpcUaClientDriver.cs:335-338`) but grep shows the
|
||
table is read nowhere except that gate and the options DTO
|
||
(`OpcUaClientDriverOptions.cs:169,188`). `DiscoverAsync` renders refs purely from remote
|
||
browse via `StableReference` (`:808-809`). Operators must author a table with zero
|
||
runtime effect.
|
||
|
||
**UNDER-3 verification:** CONFIRMED. Persisted refs are stable `nsu=` strings
|
||
(`:808-809`), but the alarm source filter is an ordinal `HashSet` of caller refs
|
||
(`:1337`) matched against `EventFields[SourceNode].ToString()` — a session-relative
|
||
`ns=N;…` rendering (`:1538-1539`). The two encodings never match, so any non-empty
|
||
filter drops every event; only empty-filter subscriptions work.
|
||
|
||
**Root cause:** UNDER-2 is a validation gate for an unbuilt feature; UNDER-3 is an
|
||
encoding mismatch between the stored `nsu=` form and the runtime `ns=N` form.
|
||
|
||
**Design:**
|
||
- UNDER-2: **delete the gate** (remove the Equipment-kind `UnsMappingTable` mandatory
|
||
check) — the honest minimal change, since the feature does not exist. Keep the option
|
||
DTO field (forward-compatible) but stop forcing operators to author dead config.
|
||
Alternative (build the feature): have `DiscoverAsync` translate remote refs → UNS refs
|
||
via the table — a real feature, larger, defer.
|
||
- UNDER-3: normalize both sides to the same encoding before compare. Convert the
|
||
incoming `EventFields[SourceNode]` `NodeId` to the stable `nsu=` reference via the
|
||
driver's `NamespaceMap.ToStableReference` (the same function used at `:808-809`) before
|
||
the `HashSet.Contains` check; build the filter set from stable refs. Then a non-empty
|
||
filter works.
|
||
|
||
**Implementation steps:** remove `ValidateNamespaceKind`'s Equipment branch (or make it
|
||
a no-op warning); in `OnEventNotification`, map the source `NodeId` through
|
||
`_namespaceMap` before comparing (`:1538-1539`); build `sourceFilter` from stable refs
|
||
(`:1337`).
|
||
|
||
**Tests:** unit — event with a known source arrives, non-empty stable-ref filter
|
||
matches and the event is delivered (today it is dropped); Equipment config without a
|
||
mapping table no longer throws at validation.
|
||
|
||
**Effort:** S (both). **Risk:** Low — OpcUaClient-local; UNDER-3 fix is a behavior
|
||
change that *enables* previously-dead filtering (verify no consumer depended on the
|
||
"filter silently drops all" bug).
|
||
|
||
## A9. STAB-13 (Positive) — no action
|
||
|
||
Write-outcome surfacing is honoured by every protocol driver (per-item `WriteResult`
|
||
with typed exception→status), and OpcUaClient's reconnect machine is exemplary
|
||
(double-arm guard, re-arm on exhaustion, session swap under `_probeLock`,
|
||
`:1908-2058`). CONFIRMED. Use OpcUaClient's machine as the reference for A1/A2. One
|
||
caveat surfaced: `Session.TransferSubscriptionsOnReconnect` is **left unset**
|
||
(mentioned only in the doc at `:1948`), so subscription transfer rests on the SDK
|
||
default — worth explicitly setting it (folds into UNDER-9 cleanup).
|
||
|
||
---
|
||
|
||
# Part B — Shared-template consolidation (themes #2 and #6)
|
||
|
||
The report's cross-cutting message: *every* High is a fix present in one sibling and
|
||
absent in another. These four items give the fleet a single home for the fixes so they
|
||
stop being re-patched per driver. Do these **after** the per-driver criticals but treat
|
||
them as the highest-leverage structural work.
|
||
|
||
## B1. PollGroupEngine v2 — absorb S7's fork; wire `onError`; add backoff (CONV-1, STAB-9, STAB-8, PERF-3, STAB-6-S7)
|
||
|
||
**Verification of the pieces:**
|
||
- `PollGroupEngine` already has the `onError` sink (`PollGroupEngine.cs:36,55-67,164-169`),
|
||
`StopState` draining (`:99-112`), and structural array compare (`:203-209`). It does
|
||
**not** have failure backoff — `PollLoopAsync` uses a fixed `Task.Delay(state.Interval)`
|
||
(`:131`).
|
||
- **STAB-9 CONFIRMED:** no consumer passes `onError` — the five constructions
|
||
(`ModbusDriver.cs:115`, `AbCipDriver.cs:133`, `AbLegacyDriver.cs:65`,
|
||
`TwinCATDriver.cs:69`, `FocasDriver.cs:72`) omit it, so the sink is dead fleet-wide.
|
||
- **CONV-1 / PERF-3 / STAB-6-S7 CONFIRMED:** S7 re-ships the loop (`S7Driver.cs:1165-1307`),
|
||
which *has* backoff (`ComputeBackoffDelay :1284-1293`) and failure-health
|
||
(`HandlePollFailure :1258-1274`) the engine lacks, but *lacks* the structural array
|
||
diff (uses `Equals(lastSeen?.Value, current.Value)` `:1304` → arrays fire every tick)
|
||
and the drain-before-dispose (`UnsubscribeAsync :1189-1190`).
|
||
|
||
**Design (single home for the poll loop):**
|
||
1. **Add optional backoff to `PollGroupEngine`.** Port S7's `ComputeBackoffDelay`
|
||
(capped exponential, ticks-based, saturating at a `PollBackoffCap`) into the engine.
|
||
Constructor gains `TimeSpan? backoffCap = null` (null ⇒ current fixed-cadence
|
||
behavior, preserving every current consumer). `PollLoopAsync` tracks
|
||
`consecutiveFailures`, resets on success, and delays `ComputeBackoffDelay(interval,
|
||
failures)`.
|
||
2. **Route failures to health.** The engine already forwards to `onError`; make each of
|
||
the five drivers pass an `onError` that degrades `_health` (keeping
|
||
`LastSuccessfulRead`) + logs — i.e. lift S7's `HandlePollFailure` into a small shared
|
||
`Action<Exception>` per driver. Consider making `onError` **required** (the report
|
||
suggests it) once all five pass it.
|
||
3. **Migrate S7 onto the engine and delete the fork.** Replace `S7Driver`'s
|
||
`_subscriptions` dict + `PollLoopAsync`/`PollOnceAsync`/`ComputeBackoffDelay`/
|
||
`HandlePollFailure` (`:1162-1310`) with a `PollGroupEngine` instance (reader =
|
||
`ReadAsync`, onChange = forward `OnDataChange`, onError = degrade health, backoffCap
|
||
= 30 s). This deletes PERF-3 (structural compare now inherited) and STAB-6-S7
|
||
(StopState drain now inherited) for free.
|
||
|
||
**Implementation steps:**
|
||
1. Extend `PollGroupEngine` ctor + `PollLoopAsync` with backoff; add unit tests for
|
||
`ComputeBackoffDelay` parity with S7's (move the existing S7 test asserts over).
|
||
2. Add an `onError` handler in Modbus/AbCip/AbLegacy/TwinCAT/FOCAS constructions that
|
||
degrades health; assert via unit test that a throwing reader degrades health.
|
||
3. S7: construct the engine, delete the fork, keep the 100 ms floor and the
|
||
`PollFailureHealthThreshold`/`Degraded`-not-`Faulted` nuance in the onError handler.
|
||
|
||
**Tests:**
|
||
- Engine unit: backoff schedule (0 failures = interval, N failures = capped
|
||
exponential); onError invoked once per caught reader exception; structural array
|
||
equality still suppresses no-change arrays; StopState drains before CTS dispose.
|
||
- S7 unit: array tag no longer fires every tick (regression for PERF-3); rapid
|
||
subscribe/unsubscribe no `ObjectDisposedException` (STAB-6); sustained poll failure
|
||
degrades health with backoff.
|
||
|
||
**Effort:** M (engine change is S; S7 migration + 5 onError wirings is M). **Risk:**
|
||
Medium — S7 migration touches its subscription surface; the array-diff change alters
|
||
publish cadence (intended). Gate behind the existing S7 subscribe tests + a new
|
||
array-cadence test.
|
||
|
||
## B2. Shared strict `ReadEnum` for equipment-tag parsers (CONV-2, UNDER-6)
|
||
|
||
**Verification:** CONFIRMED. An identical private generic `ReadEnum<TEnum>(…, TEnum
|
||
fallback)` with **silent fallback** is copy-pasted six times — a typo'd `dataType`
|
||
quietly becomes the default type and reads/writes the wrong width with Good status:
|
||
`ModbusEquipmentTagParser.cs:65-67`, `S7EquipmentTagParser.cs:59-61`,
|
||
`AbCipEquipmentTagParser.cs:86-88`, `AbLegacyEquipmentTagParser.cs:60-62`,
|
||
`TwinCATEquipmentTagParser.cs:47-49`, `FocasEquipmentTagParser.cs:43-45` (all
|
||
byte-identical bodies). Contrast the factory's fail-loud `ParseEnum`
|
||
(`ModbusDriverFactoryExtensions.cs:191-201`, lists valid names). Probe-parity is also
|
||
broken for Modbus: `ModbusDriverProbe` binds the *runtime* `ModbusDriverOptions`
|
||
(`ModbusDriverProbe.cs:36`) not the factory DTO, so `timeoutMs` is silently dropped.
|
||
|
||
**Root cause:** duplication-by-convention (theme #2); the three parse tiers (factory
|
||
strict / probe permissive / parser silent) diverged.
|
||
|
||
**Design:**
|
||
- **Hoist one strict helper into `Core.Abstractions`**, living beside
|
||
`EquipmentTagRefResolver` (theme #6's home): e.g.
|
||
`TagConfigJson.ReadEnum<TEnum>(JsonElement o, string name, TEnum @default)` with the
|
||
rule **present-but-invalid ⇒ throw** (parse failure), **absent ⇒ default**. The parser
|
||
wraps the throw so the resolver's `_parseRef` returns null on a bad enum, and the
|
||
driver surfaces `BadNodeIdUnknown` instead of silently reading the wrong width.
|
||
- Delete all six private copies; the parsers call the shared helper. Enums stay
|
||
per-driver (the helper is generic over `TEnum`), so no coupling of driver vocab.
|
||
- **Probe parity (Modbus):** make `ModbusDriverProbe` deserialize the same factory DTO
|
||
shape the factory uses (or share a `TryParseConfig` used by both), so `timeoutMs`
|
||
binds identically. OpcUaClient already documents this rule
|
||
(`OpcUaClientDriverProbe.cs:22`); Modbus should follow.
|
||
|
||
**Writable/capability parity (UNDER-6, folds in here):** Modbus/AbLegacy/FOCAS parsers
|
||
hard-code `Writable: true` (`ModbusEquipmentTagParser.cs:54-57`,
|
||
`AbLegacyEquipmentTagParser.cs:52`, `FocasEquipmentTagParser.cs:35`), so read-only
|
||
equipment tags are unauthorable on three drivers (AbCip honours a `writable` key,
|
||
`AbCipEquipmentTagParser.cs:53-54`). FOCAS additionally must force writable-false
|
||
(A7). Add a shared `ReadBool(o, "writable", default)` helper and honour it in all six
|
||
parsers; FOCAS overrides to false until PMC writes exist. FOCAS equipment tags also
|
||
bypass the `FocasCapabilityMatrix` gate authored tags get (`FocasDriver.cs:243-247` vs
|
||
`:115-119`) — route equipment-tag resolution through the same matrix validate.
|
||
|
||
**Implementation steps:**
|
||
1. New `Core.Abstractions/TagConfigJson.cs` with strict `ReadEnum` + `ReadBool`.
|
||
2. Replace the six `ReadEnum` copies; wrap parse failure so `_parseRef` returns null.
|
||
3. Honour `writable` in the three hard-coded parsers; FOCAS → false + capability gate.
|
||
4. Fix `ModbusDriverProbe` to parse the factory DTO shape.
|
||
|
||
**Tests:**
|
||
- **Shared conformance test** (the report explicitly wants one): a parameterized suite
|
||
run per driver asserting (a) unknown `dataType` ⇒ resolve failure ⇒ `BadNodeIdUnknown`
|
||
(not silent default); (b) `writable:false` honoured; (c) absent enum ⇒ documented
|
||
default. Put it in a shared test helper consumed by each driver's `.Tests`.
|
||
- Modbus probe: a config with `timeoutMs` round-trips the timeout.
|
||
|
||
**Effort:** M. **Risk:** Medium — changing silent-default to hard-fail is a *behavior
|
||
change*: a deployment with a currently-silently-defaulted typo will start returning Bad.
|
||
That is the correct outcome, but call it out in the migration notes and verify live on
|
||
the rig (author a typo'd tag, confirm Bad rather than wrong-width Good).
|
||
|
||
## B3. `ResolveHost` via the resolver — fix per-host breaker isolation (CONV-4, theme #6)
|
||
|
||
**Verification:** CONFIRMED. Every multi-device driver's `ResolveHost` consults only
|
||
`_tagsByName` then falls back to the first device: `AbCipDriver.cs:485-487`,
|
||
`AbLegacyDriver.cs:498-500`, `TwinCATDriver.cs:587-591`, `FocasDriver.cs:1082-1087`
|
||
(Modbus `:125-131` same shape, but per-tag `UnitId` isn't in its equipment parser).
|
||
An equipment-tag reference (raw TagConfig JSON, resolved via `_resolver`, not in
|
||
`_tagsByName`) misses and returns the first device's host — so the per-host Polly
|
||
bulkhead/circuit-breaker key is wrong for equipment tags, and device B's outage can trip
|
||
device A's breaker. Since equipment tags are the primary post-Galaxy authoring model,
|
||
the `IPerCallHostResolver` is effectively inert for the flagship path.
|
||
|
||
**Root cause:** `ResolveHost` never learned about the equipment-tag path; the resolver
|
||
resolves defs but exposes nothing host-specific (`EquipmentTagRefResolver` is generic
|
||
over `TDef` and never inspects it — confirmed).
|
||
|
||
**Design:** route `ResolveHost` through `EquipmentTagRefResolver.TryResolve`, then
|
||
derive the host from the parsed def. The per-driver `TDef` records already carry the
|
||
host: `AbCipTagDefinition.DeviceHostAddress`, `AbLegacyTagDefinition.DeviceHostAddress`,
|
||
`TwinCATTagDefinition.DeviceHostAddress`, `FocasTagDefinition.DeviceHostAddress` (all
|
||
parsed by the equipment parsers). So:
|
||
- Each driver's `ResolveHost(ref)` becomes: `if (_resolver.TryResolve(ref, out var def))
|
||
return def.DeviceHostAddress; return <first-device-or-sentinel>`. This covers both
|
||
authored (`_byName`) and equipment (`_parseRef`) paths through one call and reuses the
|
||
parse cache.
|
||
- Optional shared sugar: add a `string? ResolveHost(string ref, Func<TDef,string> host)`
|
||
overload to `EquipmentTagRefResolver` so the pattern is one call. Keeps the host
|
||
extraction driver-specific (the resolver stays generic) while giving theme #6 a single
|
||
home.
|
||
|
||
**Implementation steps:** rewrite the four `ResolveHost` bodies to call
|
||
`_resolver.TryResolve`; add the resolver overload if desired. Modbus: note `UnitId`
|
||
isn't in the equipment parser — either add `unitId` to `ModbusEquipmentTagParser`
|
||
(UNDER-6) or document that Modbus per-tag host resolution is out of scope.
|
||
|
||
**Tests:** unit per driver — an equipment-tag ref for device B resolves to device B's
|
||
host (today returns device A/first); assert the Polly breaker key differs per device.
|
||
|
||
**Effort:** S-M. **Risk:** Low — corrects a mis-key; verify the breaker-key wiring
|
||
downstream consumes the corrected host.
|
||
|
||
## B4. Shared reconnect/backoff primitive (STAB-8, supports A1/A2)
|
||
|
||
**Verification:** CONFIRMED. No reconnect backoff exists except the Modbus transport's
|
||
geometric backoff (`ModbusTcpTransport.cs:179-208`) and S7's poll-loop backoff. Dead
|
||
devices pay full reconnect cost per tick: AbCip full Tag create + Forward Open per tag
|
||
per tick (`AbCipDriver.cs:891-897` evict-recreate), FOCAS full two-socket reconnect per
|
||
tick of each of three loops (`FocasDriver.cs:1089-1120`), TwinCAT connect-attempt per
|
||
call (`:602-651`).
|
||
|
||
**Design:** a small shared `ConnectionBackoff` primitive in `Core.Abstractions`
|
||
(capped exponential, the same `ComputeBackoffDelay` math B1 adds to the poll engine —
|
||
factor it out so both use it). Lazy-reconnect drivers gate their
|
||
`EnsureConnected`/evict-recreate behind a per-device throttle: record
|
||
`_lastConnectAttemptUtc` + a growing backoff, and short-circuit a reconnect attempt that
|
||
is inside the backoff window (return the current Bad/Degraded state instead of hammering).
|
||
|
||
**Implementation steps:**
|
||
1. Extract `ComputeBackoffDelay` into `Core.Abstractions/ConnectionBackoff.cs`
|
||
(shared by PollGroupEngine v2 and the lazy-reconnect drivers).
|
||
2. Add a per-device backoff gate to TwinCAT `EnsureConnectedAsync`, FOCAS
|
||
`EnsureConnectedAsync`, and AbCip's evict-recreate path (skip the recreate if within
|
||
the backoff window; reset on success).
|
||
|
||
**Tests:** unit — a device that fails to connect is not retried more often than the
|
||
backoff schedule; success resets the schedule.
|
||
|
||
**Effort:** M. **Risk:** Low-Medium — must not *delay* recovery when the device comes
|
||
back (reset promptly on the first success).
|
||
|
||
---
|
||
|
||
# Part C — Remaining Mediums & Lows (batched)
|
||
|
||
All CONFIRMED. Grouped for efficient sweeps.
|
||
|
||
## C1. One-liner conventions sweep (CONV-3, CONV-5)
|
||
- **CONV-3** (Medium): S7/TwinCAT/AbCip/FOCAS `Register` take no `ILoggerFactory`
|
||
(`S7DriverFactoryExtensions.cs:19`, `TwinCATDriverFactoryExtensions.cs:18`,
|
||
`AbCipDriverFactoryExtensions.cs:21`, `FocasDriverFactoryExtensions.cs:36`), so
|
||
`DriverFactoryBootstrap.Register` (`:100,102,106,107`) can't pass one and all four log
|
||
to `NullLogger` in production — their poll-failure/prohibition/live-risk warnings are
|
||
invisible. AbLegacy/Galaxy/Modbus/OpcUaClient already have the overload
|
||
(`:101,103,104,105`). Fix: add the `ILoggerFactory? = null` param + thread to the
|
||
driver ctor in the four factories; update the four bootstrap call sites. **Effort S,
|
||
risk very low.** (Also unblocks A1/A2/B1 logging.)
|
||
- **CONV-5** (Medium): four health-idioms (`Volatile` vs `volatile` vs plain field);
|
||
AbLegacy no health-refresh on successful write (`AbLegacyDriver.cs:344-346` vs AbCip
|
||
`:725`); AbLegacy silently clobbers duplicate tag names (`:91`) where AbCip fails fast
|
||
(`:249-256`); TwinCAT declares `Healthy` at init with zero wire contact (`:115`) and
|
||
reads host state without the probe lock (`:524-525`). Fix: standardize on
|
||
`Volatile.Read/Write` (or `volatile` record ref) across the fleet; add write-success
|
||
health refresh to AbLegacy; make AbLegacy fail-fast on duplicate names; make TwinCAT
|
||
prove the connection before `Healthy`. **Effort S-M, risk low.**
|
||
|
||
## C2. Per-protocol read batching (PERF-1, PERF-2) — the scalability lever
|
||
- **PERF-1** (High): five of seven drivers do one tag per round-trip. Priority order per
|
||
the report: TwinCAT ADS **sum-read** (`TwinCATDriver.cs:203-248`), S7 **multi-var**
|
||
(`ReadMultipleVarsAsync`, unused — S7 loops per tag `:443-484` under one `_gate`),
|
||
FOCAS **PMC range reads** (`WireFocasClient.cs:296-334` widens to one value width),
|
||
AbLegacy **file-element spans** (N7:0..N7:2). Each is a separate protocol feature.
|
||
- **PERF-2** (High): AbCip's whole-UDT batched read is half-built — the planner collapses
|
||
N members via *declaration-order* layout, off by default (safe-but-unused,
|
||
`AbCipDriverOptions.cs:72` default `false`), while true member offsets are already
|
||
fetched via the CIP Template Object (`AbCipDriver.cs:151-182`,
|
||
`AbCipTemplateCache`) but consulted only by discovery, never the planner
|
||
(`AbCipUdtReadPlanner.cs:69`). **Highest-ROI batching win** (design already in-tree):
|
||
feed `AbCipUdtShape` template offsets into the planner, retire the unsafe
|
||
declaration-order mode. **Effort M-L, risk Medium (correctness of offset mapping —
|
||
test against the abcip fixture with a known UDT).**
|
||
|
||
Sequencing: do PERF-2 first (design exists), then TwinCAT sum-read + S7 multi-var. Each
|
||
is independently shippable. **Effort L overall.**
|
||
|
||
## C3. OpcUaClient performance + monolith (PERF-4, PERF-5, UNDER-9)
|
||
- **PERF-4** (Medium): all traffic serializes on one `SemaphoreSlim(1,1)` `_gate`
|
||
(12 `WaitAsync` sites, `OpcUaClientDriver.cs:78,626,713,836,1186,1377,1451,1653,1825`).
|
||
OPC UA sessions multiplex natively; a multi-second HistoryRead/re-discovery blocks
|
||
every read/write. The gate is only needed for session-swap consistency, which
|
||
`_probeLock` + re-read-in-critical-section already provide. Fix: scope the gate to
|
||
session-swap only; let reads/writes/history run concurrently on the session. **Effort
|
||
M, risk Medium — must preserve session-swap safety; test the reconnect race.**
|
||
- **PERF-5** (Medium): `EnrichAndRegisterVariablesAsync` sends `pending.Count*4`
|
||
ReadValueIds in one call (`:1029-1046`); a server enforcing `MaxNodesPerRead` returns
|
||
`BadTooManyOperations` and the blanket catch (`:1049-1057`) silently registers *every*
|
||
var as Int32/ViewOnly/non-historized with no log/health. Fix: chunk client-side
|
||
(respect `MaxNodesPerRead` / a conservative cap); log + health-signal the fallback.
|
||
**Effort S, risk low.**
|
||
- **UNDER-9** (Low): 2154-line monolith; extract the reconnect state machine for isolated
|
||
testability; set `Session.TransferSubscriptionsOnReconnect` explicitly (`:1946-1949`
|
||
— currently unset, relies on SDK default); `BuildCertificateIdentity` loads only
|
||
PKCS#12 (`:483-484`) while the option doc promises PEM
|
||
(`OpcUaClientDriverOptions.cs:68-75`) — either implement PEM load or fix the doc.
|
||
**Effort M (extraction) / S (the two nits), risk low.**
|
||
|
||
## C4. Remaining stability/correctness Mediums (STAB-10, STAB-11, STAB-12)
|
||
- **STAB-10** (Medium): AbLegacy never evicts failed tag handles on the data path
|
||
(`AbLegacyDriver.cs:246-259,269-274`) though AbCip does (`AbCipDriver.cs:891-897`) and
|
||
AbLegacy's own probe loop does (`:456-461`). Fix: evict-recreate on non-zero
|
||
status/transport exception in the data path (port AbCip's `EvictRuntime`). Pairs with
|
||
A4/A6 as an AbLegacy/AbCip parity pass. **Effort S, risk low.**
|
||
- **STAB-11** (Medium): RMW locks don't cover direct parent writes (AbCip/AbLegacy/
|
||
TwinCAT) → lost update when a direct parent write interleaves an in-flight RMW; AbCip
|
||
RMW hard-codes DINT while `AbCipTagPath` accepts `.N` on any parent
|
||
(`AbCipTagPath.cs:17-19`); TwinCAT RMW reads the parent as 4-byte `UDInt`
|
||
(`AdsTwinCATClient.cs:96-107`), an unverified assumption that will fail
|
||
`DeviceInvalidSize` on WORD/BYTE parents. Fix: make direct parent writes take the same
|
||
per-parent/per-runtime lock (AbCip's is subsumed by A4's runtime lock); width-aware RMW
|
||
for TwinCAT (read the parent as its actual declared width). **Effort M, risk Medium
|
||
(TwinCAT width needs real-hardware verification — flag under UNDER-7).**
|
||
- **STAB-12** (Medium): FOCAS sync `Dispose()` does a close-PDU exchange (`SendPdu`+
|
||
`ReadPdu`) on a `NetworkStream` with no `ReadTimeout`/token
|
||
(`FocasWireClient.cs:164-175`) — a stalled CNC wedges `ShutdownAsync`. TwinCAT
|
||
`ConnectAsync` calls the blocking SDK `Connect` ignoring token+timeout
|
||
(`AdsTwinCATClient.cs:73-80`). Fix: bound the FOCAS close read with a `ReadTimeout`;
|
||
run TwinCAT `Connect` on a worker with the timeout/token honoured (or set
|
||
`_client.Timeout` and wrap in `Task.Run` + `WaitAsync(ct)`). **Effort S, risk low.**
|
||
|
||
## C5. Perf Lows (PERF-6, PERF-7)
|
||
- **PERF-6** (Low): TwinCAT re-parses `TwinCATSymbolPath` per call
|
||
(`TwinCATDriver.cs:220,284,468`) — cache like S7's `_parsedByName`; Modbus fresh
|
||
`DriverHealth` per read (`:285`); AbCip `BuildArray<T>` boxes per element. None urgent.
|
||
- **PERF-7** (Low): deadband + `WriteOnChangeOnly` exist only in Modbus
|
||
(`ModbusDriver.cs:136-159`); generalize into/beside `PollGroupEngine` (driver-agnostic
|
||
publish suppression) so noisy analog signals on the other poll drivers don't publish
|
||
every jitter. Natural PollGroupEngine-v2 add-on (B1). **Effort M, risk low.**
|
||
|
||
## C6. Underdeveloped Mediums (UNDER-4, UNDER-5, UNDER-7, UNDER-8)
|
||
- **UNDER-4** (Medium, S7): `S7TagDto` has no `ArrayCount` and `BuildTag` never sets it
|
||
(`S7DriverFactoryExtensions.cs:80-90,137-151`) → driver-config array tags silently
|
||
scalar (arrays only work as equipment tags, `S7EquipmentTagParser.cs:73-87`); array
|
||
writes throw `NotSupportedException` (`S7Driver.cs:1014-1017`); `UInt32` surfaces
|
||
lossily as `Int32` (`:1149-1152`). Fix: add `ArrayCount` to the DTO + `BuildTag`;
|
||
implement array writes; map `UInt32`→`UInt32`/`Int64`. **Effort M.**
|
||
- **UNDER-5** (Medium, AbCip): `AcknowledgeAsync` fire-and-forgets write results
|
||
(`AbCipAlarmProjection.cs:149`) → a failed ack is invisible to operator/health/Part 9
|
||
caller; with `EnableControllerBrowse` on, one unreachable PLC faults the entire
|
||
multi-device `DiscoverAsync` (uncaught `await foreach`, `AbCipDriver.cs:970-1037`);
|
||
browsed tags default writable/Operate (`CipSymbolObjectDecoder.cs:91`). Fix: surface
|
||
ack outcomes to health + the caller; per-device try/catch in discovery so one dead PLC
|
||
doesn't fault all; default browsed tags read-only. **Effort M.**
|
||
- **UNDER-7** (Medium): aggregate the honest "unverified against real hardware" blocks
|
||
into a **"needs bench time" checklist** — TwinCAT array-read shape
|
||
(`AdsTwinCATClient.cs:109-116`), Flat-mode `SubSymbols` population (`:263-277`), bit-RMW
|
||
width (STAB-11); AbLegacy array-decode's five unproven layout assumptions
|
||
(`LibplctagLegacyTagRuntime.cs:64-88`); FOCAS `cnc_getfigure` id + servo-load scaling
|
||
(`FocasWireClient.cs:391-395,943-947`). Deliverable: a doc checklist, not code.
|
||
**Effort S (doc).**
|
||
- **UNDER-8** (Medium): test coverage is codec-heavy, failure-path-light. The concrete
|
||
gaps this plan closes with new tests: S7 reconnect (A1), TwinCAT replay (A2), AbCip
|
||
concurrency (A4), FOCAS cache concurrency (A5), Modbus desync (A3), shared
|
||
parser-strictness conformance (B2). Also: `Cli.Common.DriverCommandBase` has no direct
|
||
tests — add a small suite.
|
||
|
||
## C7. Lows — hygiene (CONV-6, CONV-7, CONV-8)
|
||
- **CONV-6** (Low/Medium): `Driver.OpcUaClient.Contracts` drags the full OPC UA SDK
|
||
because `NamespaceMap` lives there (`OpcUaClient.Contracts.csproj:9`
|
||
`OPCFoundation.NetStandard.Opc.Ua.Client`); every options-DTO consumer transitively
|
||
loads the SDK. This is the previously-deferred OpcUaClient.Contracts-002 finding — move
|
||
`NamespaceMap` into the driver project (or a new `.Namespace` project), leaving Contracts
|
||
POCO-only like the other six. **Effort M (new project boundary), risk low.**
|
||
- **CONV-7** (Low/Medium): CLI divergences — `ParseValue`/`ParseBool` copy-pasted six
|
||
times (`*/Commands/WriteCommand.cs`); option `Validate()` absent in AbLegacy
|
||
(`AbLegacyCommandBase.cs` — `--timeout-ms 0` reaches the driver); `Timeout` init-setter
|
||
throws in AbCip (`AbCipCommandBase.cs:43`) but no-ops in five others; only TwinCAT
|
||
exposes `browse` though AbCip supports controller browse
|
||
(`AbCipCommandBase.cs:63` hardcodes `EnableControllerBrowse=false`); OpcUaClient has no
|
||
CLI. Fix: hoist value parsing + a standard `Validate()` into `Cli.Common`; add an
|
||
OpcUaClient CLI or document the gap. **Effort M, risk low.**
|
||
- **CONV-8** (Low): dead/no-op knobs + stale docs — `DisableFC23` no-op
|
||
(`ModbusDriverOptions.cs:83-89`); AbCip `ConnectionSize` plumbed-never-applied
|
||
(`:97-103`); AbLegacy class doc says read/write "ship in PRs 2 and 3"
|
||
(`AbLegacyDriver.cs:9-11`); OpcUaClient header says "PR 66 ships the scaffold: IDriver
|
||
only" (`:12-14`). No options class has `Validate()`. Fix: delete/wire the dead knobs;
|
||
refresh the stale headers; optionally add per-driver options `Validate()` (S7's init
|
||
guards `:314-385` are the best-of-breed template). **Effort S, risk very low.**
|
||
|
||
---
|
||
|
||
# Sequencing & effort roll-up
|
||
|
||
Aligns with the report's "Recommended remediation order" and OVERALL actions #3/#4.
|
||
|
||
| # | Item | Findings | Effort | Risk | Notes |
|
||
|---|---|---|---|---|---|
|
||
| 1 | S7 reconnect (`IS7Plc` seam + `EnsureConnected`) + reconnect test | STAB-1, UNDER-8 | M | Med | Critical; template = TwinCAT/OpcUaClient |
|
||
| 2 | TwinCAT native-sub replay on client swap + probe recycle | STAB-2 | M | Med | Critical; fake-client unit tests are the only automated coverage |
|
||
| 3 | AbCip per-runtime op lock + FOCAS ConcurrentDictionary caches | STAB-4, STAB-5 | S each | Low | Silent-corruption class; port AbLegacy's `GetRuntimeLock` |
|
||
| 4 | Modbus timeout/framing ⇒ connection teardown | STAB-3 | S | Low-Med | Distinguish timeout vs caller-cancel |
|
||
| 5 | Shutdown/unsub race fixes | STAB-6 | S | Low | Deleted for S7 by item 8 |
|
||
| 6 | FOCAS writable-false; OpcUaClient dead gate + alarm-filter encoding | UNDER-1, UNDER-2, UNDER-3 | S each | Low | Kill false capabilities |
|
||
| 7 | Factory `ILoggerFactory` on 4 drivers | CONV-3 | S | V.Low | Unblocks item 1/2/8 logging |
|
||
| 8 | **PollGroupEngine v2**: backoff + onError-health + migrate S7 off fork | CONV-1, STAB-8, STAB-9, PERF-3, STAB-6-S7 | M | Med | Single home for poll fixes |
|
||
| 9 | **Shared strict `ReadEnum`/`ReadBool` + probe parity + writable parity** | CONV-2, UNDER-6 | M | Med | Behavior change: typo ⇒ Bad; live-verify |
|
||
| 10 | **`ResolveHost` via resolver** (per-host breaker isolation) | CONV-4 | S-M | Low | Flagship equipment-tag path |
|
||
| 11 | Shared reconnect/backoff primitive for lazy drivers | STAB-8 | M | Low-Med | Extract `ConnectionBackoff`; don't delay recovery |
|
||
| 12 | Per-protocol batching: AbCip planner⇄template-cache, TwinCAT sum-read, S7 multi-var | PERF-1, PERF-2 | L | Med | Biggest scalability lever; AbCip design in-tree |
|
||
| 13 | OpcUaClient gate-scoping + chunk discovery + monolith split | PERF-4, PERF-5, UNDER-9 | M | Med | Preserve session-swap safety |
|
||
| 14 | Remaining mediums: evict-on-fail, RMW parent lock/width, blocking teardown | STAB-10, STAB-11, STAB-12 | M | Med | TwinCAT width needs bench time |
|
||
| 15 | Underdeveloped: S7 arrays/UInt32, AbCip acks/discovery, bench checklist | UNDER-4, UNDER-5, UNDER-7, UNDER-8 | M | Low-Med | |
|
||
| 16 | Lows: Contracts SDK split, CLI hoist, dead knobs/docs, perf lows | CONV-6/7/8, PERF-6/7 | M | Low | |
|
||
|
||
**Where shared primitives live:** `Core.Abstractions` — `PollGroupEngine` (v2 with
|
||
backoff, item 8), `ConnectionBackoff` (item 11, shared with the engine),
|
||
`TagConfigJson.ReadEnum/ReadBool` (item 9, beside `EquipmentTagRefResolver`), and the
|
||
`ResolveHost`-via-resolver pattern (item 10). This is the concrete answer to themes #2
|
||
and #6: fleet fixes get one home instead of six copies.
|
||
|
||
**Live-verify recipes (docker rig on `10.100.0.35`):** Modbus `:5020` +
|
||
`exception_injector` for STAB-3 (item 4) and write-failure paths; S7 `:1102`
|
||
(`Snap7ServerFixture`, bounce via `lmxopcua-fix down/up s7 s7_1500`) for STAB-1 (item 1);
|
||
AbCip `:44818` for STAB-4 concurrency + PERF-2 UDT (items 3, 12). TwinCAT has **no docker
|
||
fixture** (needs a real TC3 XAR) — the fake-`ITwinCATClient` unit tests are authoritative
|
||
for STAB-2/item 2.
|