docs(archreview): remediation plans + fix flagged doc drift
Add the 7 per-domain design+implementation plans (archreview/plans/) with an index, produced from the 2026-07-08 architecture review. Fix two confirmed doc drifts the review flagged (theme #5): - CLAUDE.md KNOWN LIMITATION 2: the continuous-historization historized-ref feed IS wired (AddressSpaceApplier.FeedHistorizedRefs -> UpdateHistorizedRefs -> recorder); rewrite to reflect that value-capture is code-complete and only the live end-to-end + restart-convergence verification remains. - CLAUDE.md ScriptAnalysis gating: endpoints use Roles=Administrator,Designer via RequireAuthorization, not the FleetAdmin policy.
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,808 @@
|
||||
# Design + Implementation Plan — 05 Protocol Drivers
|
||||
|
||||
**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.
|
||||
Reference in New Issue
Block a user