Joseph Doherty
c21e21423f
Running S7_1500ConnectTimeoutOutageTests (docker-pause blackhole, self-serviceable over SSH) uncovered + fixed the read-leg wall-clock gap STAB-14 left. Updates the R2-01 STATUS row, the needs-user/still-open lists, and the STAB-14 remediation note.
798 lines
56 KiB
Markdown
798 lines
56 KiB
Markdown
# Architecture Review 05 — Protocol Drivers
|
||
|
||
**Date:** 2026-07-12
|
||
**Commit:** `f6eaa267` (master, clean tree)
|
||
**Updates:** the 2026-07-08 review at `9cad9ed0`. Since then the arch-review
|
||
remediation branches merged to master; the delta touching this domain is
|
||
exactly: Critical 3 — S7 lazy reconnect (`25c0c6f6`, merged `dfc02af3`),
|
||
Critical 4 — TwinCAT native-notification replay (`af318fb4`, merged
|
||
`8f7b0017`), the OTOPCUA0001 analyzer wiring (`f0082af5` — pragma/NoWarn-only
|
||
in this domain: two driver-internal self-call pragmas in
|
||
`AbCipAlarmProjection.cs`, one in `S7Driver.PollOnceAsync`, `NoWarn` in the
|
||
six CLI csprojs), plus the new reconnect test suites under `tests/Drivers/`.
|
||
No other driver file changed.
|
||
**Scope:** the seven cross-platform protocol drivers and their satellites —
|
||
`Driver.Modbus` (+ `.Addressing`, `.Contracts`), `Driver.S7` (+ `.Contracts`),
|
||
`Driver.AbCip` (+ `.Contracts`), `Driver.AbLegacy` (+ `.Contracts`),
|
||
`Driver.TwinCAT` (+ `.Contracts`), `Driver.FOCAS` (+ `.Contracts`),
|
||
`Driver.OpcUaClient` (+ `.Contracts`, `.Browser`), and `src/Drivers/Cli/*`
|
||
(six per-driver CLI harnesses + `Cli.Common`). Test projects under
|
||
`tests/Drivers/` and `tests/Drivers/Cli/` were assessed for coverage only.
|
||
Galaxy and the Historian drivers are out of scope.
|
||
|
||
**Method:** every prior finding was re-verified against current code (the
|
||
delta above means findings in untouched files carry forward with their line
|
||
references intact; only `S7Driver.cs` and `TwinCATDriver.cs` shifted). The
|
||
two remediation commits were read in full and adversarially — the fresh
|
||
findings STAB-14..17 come from that reading. The original method (Modbus as
|
||
the exhaustively-read reference driver, structural diffing of each sibling)
|
||
stands from the 2026-07-08 pass.
|
||
|
||
---
|
||
|
||
## Prior-finding status (9cad9ed0 → f6eaa267)
|
||
|
||
| ID | One-liner | Status @ f6eaa267 |
|
||
|---|---|---|
|
||
| STAB-1 | S7 has no reconnect path at all | **FIXED** — `25c0c6f6`: `IS7Plc`/`IS7PlcFactory` seam (`Driver.S7/IS7Plc.cs`), lazy `EnsureConnectedAsync` under `_gate` (`S7Driver.cs:1188-1224`), connection-fatal classification `MarkConnectionDeadIfFatal`/`IsS7ConnectionFatal` (`:1226-1251`), probe loop as reconnect backstop (`:1532-1536`). 4 unit guards (`S7DriverReconnectTests`) + env-gated live bounce test (`S7_1500ReconnectTests`), live-verified against python-snap7. **However, the fix itself introduces new findings STAB-14 (High) and STAB-15 (High)** — see below. |
|
||
| STAB-2 | TwinCAT native subs silently orphaned after reconnect | **FIXED** — `af318fb4`: `NativeRegistration` replayable-intent registry per device (`TwinCATDriver.cs:551-613, 921`), `EnsureConnectedUnderGateAsync` replays every intent onto a fresh client (`:731-773, 784-823`), probe-failure `RecycleClientAsync` closes the `IsConnected`-lies compounding hole (`:631-643, 826-843`). Register and replay both run under `ConnectGate` — verified no register/replay double-registration race (intent stored only after the handle is live, `:508-520`). Unit-only verification (4 tests, fake client) — no TC3 fixture exists, accepted. **Residuals: STAB-16 (Medium) and STAB-17 (Low)** — see below. Data-path reads on a wire-dead-but-port-open client still fail Bad until the next probe tick recycles (bounded by probe interval — acceptable); the wrapper's pre-gate fast path (`:709-710`) can hand a data-path caller a client the probe is concurrently disposing (transient Bad read — acceptable). |
|
||
| STAB-3 | Modbus transport desyncs after response timeout / TxId mismatch | **STILL OPEN** — `ModbusTcpTransport.cs:155-165, 233-237, 257-261` unchanged. Note: STAB-15 shows the new S7 reconnect path reproduces this same failure mode. |
|
||
| STAB-4 | AbCip concurrent ops on shared libplctag handles, no lock | **STILL OPEN** — `AbCipDriver.cs:544-575, 614-637, 708-726` unchanged; alarm-projection read loop now `AbCipAlarmProjection.cs:230-232` (pragma lines shifted it +4). |
|
||
| STAB-5 | FOCAS fixed-tree caches are plain `Dictionary`s mutated cross-thread | **STILL OPEN** — `FocasDriver.cs:1184-1194, 752-839, 912-945` unchanged. |
|
||
| STAB-6 | AbLegacy probe-shutdown race; S7 unsubscribe race | **STILL OPEN** — AbLegacy unchanged (`AbLegacyDriver.cs:118, 156-170`); S7 `UnsubscribeAsync` still cancels + disposes the CTS without draining `PollTask` (now `S7Driver.cs:1350-1354`; the loop's `Task.Delay` still catches only OCE, `:1396-1397`). |
|
||
| STAB-7 | `InitializeAsync` ignores `driverConfigJson` (Modbus, AbLegacy); TwinCAT empty-config quirk | **STILL OPEN** — all three sites unchanged (`ModbusDriver.cs:167-197`, `AbLegacyDriver.cs:78-153`, `TwinCATDriver.cs:89-94`). |
|
||
| STAB-8 | No reconnect backoff fleet-wide (except Modbus transport, S7 poll loop) | **STILL OPEN — and slightly worse on S7**: the new lazy reconnect attempts a full TCP connect (bounded only by `_options.Timeout`) on *every* data call while the PLC is down, with no attempt throttle; poll-loop backoff shields subscriptions but not server-initiated reads/writes or the probe. |
|
||
| STAB-9 | `PollGroupEngine` `onError` sink dead — no driver passes it | **STILL OPEN** — verified all five construct without it (`ModbusDriver.cs:115`, `AbCipDriver.cs:133`, `AbLegacyDriver.cs:65`, `TwinCATDriver.cs:69`, `FocasDriver.cs:72`). |
|
||
| STAB-10 | AbLegacy never evicts failed tag handles on the data path | **STILL OPEN** — `AbLegacyDriver.cs:246-274` unchanged. |
|
||
| STAB-11 | RMW locks don't cover direct parent writes; AbCip DINT / TwinCAT 4-byte assumptions | **STILL OPEN** — all sites unchanged (`AdsTwinCATClient.cs` untouched by the replay commit). |
|
||
| STAB-12 | Blocking/uncancellable teardown paths (FOCAS close-PDU, TwinCAT blocking Connect) | **STILL OPEN — and compounded**: the new `RecycleClientAsync` waits on `ConnectGate` with `CancellationToken.None` (`TwinCATDriver.cs:835`), so a connect wedged under the gate now also wedges the probe loop uncancellably. |
|
||
| STAB-13 | (Positive) write-outcome surfacing universal; OpcUaClient reconnect machine strongest | **STILL HOLDS** — S7 write path now `S7Driver.cs:981-1083` (per-item statuses preserved through the reconnect wrapper); all other cites unchanged. |
|
||
| PERF-1 | Five of seven drivers read one tag per wire round-trip | **STILL OPEN** — no batching work landed. |
|
||
| PERF-2 | AbCip whole-UDT read exists in two never-joined halves | **STILL OPEN** — planner/template-cache unchanged. |
|
||
| PERF-3 | S7 forked poll loop diffs arrays by reference → publishes every tick | **STILL OPEN** — now `S7Driver.cs:1469` (`Equals(lastSeen?.Value, current.Value)`). |
|
||
| PERF-4 | OpcUaClient serializes all traffic on one global gate | **STILL OPEN** — `OpcUaClientDriver.cs` untouched. |
|
||
| PERF-5 | OpcUaClient discovery enrichment un-chunked Read + silent degrade | **STILL OPEN**. |
|
||
| PERF-6 | Assorted per-tick allocation churn (TwinCAT re-parse, etc.) | **STILL OPEN** — TwinCAT symbol re-parse now `TwinCATDriver.cs:220, 284, 468`. |
|
||
| PERF-7 | Deadband only in Modbus | **STILL OPEN**. |
|
||
| CONV-1 | S7 forked the poll loop; fork diverged both directions | **STILL OPEN** — fork now `S7Driver.cs:1373-1412` (backoff `:1447`, cap `:1363`); neither direction merged. The fork's bare OCE-return is now load-bearing for the new STAB-14. |
|
||
| CONV-2 | Three config-parse strictness tiers; silent enum defaulting in all six equipment-tag parsers | **STILL OPEN** — all six `ReadEnum` copies + Modbus probe shape unchanged. |
|
||
| CONV-3 | Four factories take no `ILoggerFactory` → NullLogger in production | **STILL OPEN** — verified `S7DriverFactoryExtensions.cs:19` still `Register(DriverFactoryRegistry)` only; ditto TwinCAT/AbCip/FOCAS. Now more costly: the new S7 reconnect and TwinCAT replay log recovery *only* via `_logger` — invisible in production for S7/TwinCAT until this lands. |
|
||
| CONV-4 | `ResolveHost` mis-keys equipment-tag refs in every multi-device driver | **STILL OPEN** — TwinCAT's now `TwinCATDriver.cs:689-696`; others unchanged. |
|
||
| CONV-5 | Health-field publication idioms drift (plain fields in S7/AbCip/TwinCAT/OpcUaClient) | **STILL OPEN** — S7's new `_plcDead`/`_initialized` are gate-guarded (fine), but `_health` remains a plain field written from data paths, poll loops and the probe. |
|
||
| CONV-6 | OpcUaClient.Contracts drags the OPC UA SDK | **STILL OPEN** (previously-deferred NamespaceMap move). |
|
||
| CONV-7 | CLI harness divergences | **STILL OPEN** — only change is the uniform `NoWarn OTOPCUA0001` (wire-level projects, commented; consistent with the analyzer triage — neutral). |
|
||
| CONV-8 | Dead config knobs + stale scaffold docs; no options `Validate()` | **STILL OPEN**. |
|
||
| UNDER-1 | FOCAS advertises writes it cannot perform | **STILL OPEN**. |
|
||
| UNDER-2 | OpcUaClient `UnsMappingTable` mandatory-but-unconsumed | **STILL OPEN**. |
|
||
| UNDER-3 | OpcUaClient alarm filter compares mismatched NodeId encodings | **STILL OPEN**. |
|
||
| UNDER-4 | S7 authored-array/write/UInt32 gaps | **STILL OPEN** — refs shifted: array-write guard `S7Driver.cs:~1088-1110`, wide-type arrays out of scope `:344, :357`, `UInt32 → DriverDataType.Int32` lossy map `:1315`; `S7TagDto` still has no `ArrayCount`. |
|
||
| UNDER-5 | AbCip alarm-ack outcomes discarded | **STILL OPEN** — `AbCipAlarmProjection.cs:149-151` (fire-and-forget now sits inside an intentional-self-call pragma — the *dispatch* annotation is correct, the discarded per-item outcomes are still the finding). |
|
||
| UNDER-6 | Equipment-tag parsers lag the authored-tag feature set | **STILL OPEN**. |
|
||
| UNDER-7 | Live-risk assumptions documented but ungated | **STILL OPEN** — note the TwinCAT replay path is itself unit-only (no TC3 fixture), joining this list's "needs bench time" set. |
|
||
| UNDER-8 | Test coverage codec-heavy, failure-path-light | **PARTIALLY FIXED** — S7 reconnect gap closed (`S7DriverReconnectTests` ×4 incl. fatal-vs-data-error classification + reopen-fails-then-recovers; live `S7_1500ReconnectTests` double-gated on sim + bounce cmd); TwinCAT gained `TwinCATReconnectReplayTests` ×4 (replay, multi-tag, unsub-after-reconnect, probe-recycle — fake client). Still absent: AbLegacy reconnect tests, S7/AbCip concurrency tests, connect-*timeout* (vs connect-refused) outage coverage — which is exactly where the new STAB-14 lives. |
|
||
| UNDER-9 | OpcUaClient 2,154-line monolith; PKCS#12-only identity | **STILL OPEN**. |
|
||
|
||
---
|
||
|
||
## Architecture Overview
|
||
|
||
### The common driver shape
|
||
|
||
Every protocol driver is expected to follow the same template:
|
||
|
||
1. **Factory extension** (`<X>DriverFactoryExtensions.Register`) registers a
|
||
`DriverTypeName → CreateInstance(id, configJson)` closure with the
|
||
`DriverFactoryRegistry` (wired centrally in
|
||
`src/Server/ZB.MOM.WW.OtOpcUa.Host/Drivers/DriverFactoryBootstrap.cs`).
|
||
Config parses through a string-typed DTO with fail-loud `ParseEnum`
|
||
helpers and `PropertyNameCaseInsensitive` + comment/trailing-comma
|
||
tolerance.
|
||
2. **Driver class** implements `IDriver, ITagDiscovery, IReadable,
|
||
IWritable, ISubscribable, IHostConnectivityProbe, IPerCallHostResolver,
|
||
IDisposable, IAsyncDisposable` (Modbus,
|
||
`Driver.Modbus/ModbusDriver.cs:21-22`), with optional capability
|
||
interfaces (`IAlarmSource`, `IRediscoverable`, `IHistoryProvider`) per
|
||
protocol.
|
||
3. **Reference resolution** goes through the shared
|
||
`EquipmentTagRefResolver<TDef>`
|
||
(`src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/EquipmentTagRefResolver.cs`),
|
||
bridging authored tag-table names and raw equipment-tag `TagConfig` JSON
|
||
blobs (parsed once by a per-driver `<X>EquipmentTagParser` in the
|
||
Contracts project, cached, cleared on teardown).
|
||
4. **Subscriptions** overlay a polling loop via the shared `PollGroupEngine`
|
||
(`Core.Abstractions/PollGroupEngine.cs`) for protocols without a push
|
||
model; the driver supplies the batch reader + on-change bridge.
|
||
5. **Health** is an immutable `DriverHealth` snapshot published via
|
||
`Volatile.Read/Write`; failures set `Degraded` while retaining
|
||
`LastSuccessfulRead`; `Faulted` is reserved for permanent config faults.
|
||
6. **Writes** return per-item `WriteResult` with protocol-error → OPC UA
|
||
StatusCode mapping (a per-driver `<X>StatusMapper` or switch). Accurate
|
||
per-item statuses are load-bearing: the server's write-outcome
|
||
self-correction (`OtOpcUaNodeManager` reverting the node value on a
|
||
failed device write) only works if the driver reports the failure.
|
||
7. **Probe loop** — a background task issuing a cheap protocol-level
|
||
handshake, raising `OnHostStatusChanged` on Running↔Stopped transitions
|
||
under a probe lock; plus a stateless `<X>DriverProbe : IDriverProbe`
|
||
backing the AdminUI Test Connect button.
|
||
8. **Teardown** — one shared `TeardownAsync` used by both `ShutdownAsync`
|
||
and `DisposeAsync`: cancel loops, await them, dispose CTSs, clear
|
||
caches, dispose the transport.
|
||
|
||
Since the prior review the template gained a ninth expectation, enforced
|
||
tree-wide by the OTOPCUA0001 analyzer: **driver capability calls are wrapped
|
||
by `CapabilityInvoker` at the dispatch layer**, and a driver's *internal*
|
||
self-calls (its own poll loop, its own alarm projection reading through its
|
||
own `ReadAsync`) carry an explanatory `#pragma` marking them intentional
|
||
(`S7Driver.cs:1460-1462`, `AbCipAlarmProjection.cs:149-151, 230-232`). This
|
||
matters here because dispatch-level Polly *timeouts* can now cancel driver
|
||
I/O mid-PDU — see STAB-15.
|
||
|
||
Modbus adds the most advanced read planner in the fleet: block-read
|
||
coalescing (`MaxReadGap`), auto-prohibition of failing ranges with
|
||
bisection re-probing (`ModbusDriver.cs:687-795, 553-677`), chunking over
|
||
per-device caps, per-register bit-RMW locks, deadband publish filtering,
|
||
and `WriteOnChangeOnly` suppression with read-side invalidation.
|
||
|
||
### Per-driver conformance matrix
|
||
|
||
| Aspect | Modbus (ref) | S7 | AbCip | AbLegacy | TwinCAT | FOCAS | OpcUaClient |
|
||
|---|---|---|---|---|---|---|---|
|
||
| LOC (driver proj) | 2,374 | ~2,170 | 3,762 | 1,830 | ~2,460 | 5,007 | 2,347 |
|
||
| `IPerCallHostResolver` | yes | **no** | yes | yes | yes | yes | no (single endpoint, defensible) |
|
||
| Extra capabilities | — | — | `IAlarmSource` | — | `IRediscoverable` | `IAlarmSource` | `IAlarmSource`, `IHistoryProvider` |
|
||
| Subscription model | PollGroupEngine | **bespoke forked loop** | PollGroupEngine | PollGroupEngine | native ADS push (default) + PollGroupEngine fallback | PollGroupEngine | **real OPC UA MonitoredItems** |
|
||
| `onError` sink passed to engine | no | n/a | no | no | no | no | n/a |
|
||
| Reconnect story | transport auto-reconnect + geometric backoff | **NEW: lazy reconnect via `IS7Plc` seam + probe backstop** (but see STAB-14/15) | evict + recreate handle, no backoff | **no eviction on data path** | **NEW: lazy reconnect + native-sub intent replay + probe-failure recycle** | lazy reconnect per tick, no backoff | `SessionReconnectHandler` + re-arm (best) |
|
||
| Read batching | block coalescing + bisection | per-tag | per-tag; UDT planner **half-built** | per-tag | per-tag (no ADS sum-read) | per-address, single-flight socket | batched `ReadValueIdCollection` |
|
||
| Deadband / WriteOnChangeOnly | yes / yes | no / no | no / no | no / no | no / no | no / no | n/a (passthrough) |
|
||
| Bit-level writes | RMW + per-register lock | native S7 bit write | RMW + per-parent lock (DINT-only assumption) | RMW + per-parent lock, width-aware | RMW + per-parent lock (**4-byte width assumption unverified**) | n/a (read-only backend) | n/a |
|
||
| Health via `Volatile`/`volatile` | yes | **plain field** | **plain field** | `volatile` | **plain field** | yes | **plain field** |
|
||
| `InitializeAsync` honours configJson | **no** | yes | yes | **no** | yes (empty-config quirk) | yes | yes |
|
||
| Factory `Register` takes `ILoggerFactory` | yes | **no** | **no** | yes | **no** | **no** | yes |
|
||
| Options `Validate()` | no (none in fleet) | no | no | no | no | no | no |
|
||
| Live browse / discovery | declared tags only | declared tags only | declared + opt-in controller browse + UDT fan-out | declared only (correct for PCCC) | declared + opt-in symbol browse + expander | declared + capability-gated fixed tree | full recursive remote browse |
|
||
| CLI harness | yes | yes | yes | yes | yes (+`browse`) | yes | **none** |
|
||
|
||
---
|
||
|
||
## Findings
|
||
|
||
Severity scale: **Critical** = live data-loss/unavailability in a realistic
|
||
field scenario; **High** = correctness or stability defect likely to bite;
|
||
**Medium** = architectural debt / divergence with concrete failure modes;
|
||
**Low** = hygiene.
|
||
|
||
STAB-1 and STAB-2 are fixed (see the status table); their residual defects
|
||
are the new STAB-14..17 below.
|
||
|
||
### 1. Stability
|
||
|
||
> **REMEDIATED by R2-01 (branch `r2/01-s7-fault-hardening`).** Connect-timeout now surfaces as a
|
||
> `TimeoutException` at the `EnsureConnectedAsync`/`InitializeAsync` source (not an escaping OCE), plus
|
||
> `when (token.IsCancellationRequested)` filters on the read/write ensure-wrappers (`:488`/`:1000`) and all
|
||
> three poll-loop OCE catches — so a non-teardown OCE degrades/backs-off instead of killing the loop. Unit
|
||
> guards T1–T4 (`S7DriverReconnectTests`, incl. the poll-loop-survival regression). Live gate
|
||
> `S7_1500ConnectTimeoutOutageTests` **CLOSED 2026-07-15** — running it (blackhole via `docker pause`)
|
||
> exposed that the fix above covered only the *connect* leg: S7.Net's socket `ReadTimeout`/`WriteTimeout`
|
||
> are ignored by its async read/write paths, so a read on an established-but-frozen peer blocked for
|
||
> minutes and wedged the poll loop. Fixed (PR #453, master `4dc937fc`) by a new `S7OperationDeadline`
|
||
> wall-clock ceiling on every data-plane wire op (`S7PlcAdapter` → `TimeoutException`) + `IsS7ConnectionFatal`
|
||
> now treating `TimeoutException` as fatal → mark-dead → reopen (the READ-leg sibling of STAB-14). Live gate
|
||
> now GREEN; Driver.S7.Tests 246→260.
|
||
|
||
**STAB-14 — High — NEW — S7's reconnect wrapper rethrows connect-timeout
|
||
cancellation, and the poll loop's bare OCE catch then permanently kills the
|
||
subscription.** `EnsureConnectedAsync` bounds the reopen with a linked
|
||
`CancelAfter(_options.Timeout)` CTS (`S7Driver.cs:1206-1208`). When the PLC
|
||
is unreachable-but-not-refusing — pulled cable, powered-off device on a
|
||
switched network, firewall drop: the *classic* field outage Critical 3
|
||
targeted — `OpenAsync` throws `TaskCanceledException`. `ReadAsync`'s
|
||
ensure-wrapper catches `OperationCanceledException` and rethrows
|
||
**unconditionally** (`:488`, no `when (cancellationToken.IsCancellationRequested)`
|
||
filter; `WriteAsync` same at `:1000`), so the timeout escapes as OCE to the
|
||
caller. In the poll loop that caller is `PollLoopAsync`, whose
|
||
`catch (OperationCanceledException) { return; }` (`:1383, :1404`) treats
|
||
*any* OCE as teardown and exits — silently: no log, no health change, no
|
||
`HandlePollFailure`. Net: after a connect-timeout outage, S7 reads and
|
||
writes self-heal (the very fix), but **every S7 subscription poll loop that
|
||
ticked during the outage is permanently dead until redeploy** — data-loss in
|
||
the exact scenario class Critical 3 was meant to close. The live bounce test
|
||
(`S7_1500ReconnectTests`) cannot catch this: `docker restart` produces
|
||
connection-*refused* (`SocketException` → the degrade-to-Bad path the unit
|
||
tests cover), never connect-*timeout*. *Recommendation:* filter the
|
||
wrapper's OCE rethrow on the caller's token (timeout-OCE joins the
|
||
degrade-to-`BadCommunicationError` path), and filter the poll loop's OCE
|
||
catches on `ct.IsCancellationRequested`; add a fake-factory unit test where
|
||
`OpenAsync` honours a token that only the `CancelAfter` fires.
|
||
|
||
> **REMEDIATED by R2-01 (branch `r2/01-s7-fault-hardening`).** `IsS7ConnectionFatal` broadened to the
|
||
> ISO-on-TCP framing surface (`TPKTInvalidException`/`TPDUInvalidException`/`WrongNumberOfBytesException` +
|
||
> `PlcException{WrongNumberReceivedBytes}`; deliberately NOT `InvalidDataException` — that is the driver's own
|
||
> config-mismatch decode backstop on a healthy socket). Cancellation observed mid-PDU on read + write now
|
||
> marks `_plcDead` before propagating; the probe restructured to classify + mark dead under `_gate` so the next
|
||
> tick reopens a lying/hung handle. Unit guards T5–T9 (`S7DriverReconnectTests`) + negative control
|
||
> `Read_does_not_reopen_on_a_data_address_error`.
|
||
|
||
**STAB-15 — High — NEW — S7's connection-fatal classification misses
|
||
framing/desync errors and cancellation-during-I/O, reproducing Modbus's
|
||
STAB-3 desync; and the probe never marks the handle dead.**
|
||
`IsS7ConnectionFatal` (`S7Driver.cs:1239-1251`) treats only
|
||
`SocketException`/`IOException`/`ObjectDisposedException` (walking inners)
|
||
and `PlcException{ErrorCode.ConnectionError}` as fatal. Two gaps:
|
||
(a) a caller-token cancellation mid-PDU — server request deadline, or a
|
||
dispatch-layer Polly timeout now that `CapabilityInvoker` wraps driver calls
|
||
— abandons a half-read ISO-on-TCP response on the socket; the connection is
|
||
kept (OCE is not classified), and the next call reads the stale bytes.
|
||
S7.Net then surfaces framing faults (`TPKTInvalidException`,
|
||
`PlcException` `WrongNumberReceivedBytes`, `InvalidDataException`) — none of
|
||
which are classified fatal either — so the single gated connection stays
|
||
**permanently desynchronized**, returning `BadDeviceFailure`/
|
||
`BadCommunicationError` forever on a socket that a reopen would fix. This is
|
||
the exact Modbus STAB-3 failure mode rebuilt in the new S7 path.
|
||
(b) The probe loop swallows every failure (`:1542`) without calling
|
||
`MarkConnectionDeadIfFatal`, so its backstop role only works when
|
||
`IsConnected` flips false on its own; a wire-dead or desynced handle whose
|
||
`TcpClient.Connected` stays true is re-probed and re-fails forever
|
||
(contrast TwinCAT, whose probe now force-recycles on failure —
|
||
`TwinCATDriver.cs:631-643`). *Recommendation:* classify framing exceptions
|
||
and cancellation-observed-during-I/O as connection-fatal (tear down before
|
||
propagating, mirroring the STAB-3 fix recommendation), and mark-dead from
|
||
the probe's catch.
|
||
|
||
**STAB-16 — Medium — NEW — TwinCAT replay failure is silent to subscribers
|
||
and never retried while the client stays up.**
|
||
`ReplayNativeRegistrationsAsync` logs and skips a failed intent
|
||
(`TwinCATDriver.cs:801-816`) — deliberately, so one bad symbol can't abort
|
||
recovery of the rest (good) — but the failed registration keeps its dead old
|
||
handle (`SwapHandle` runs only on success), the `OnChange` callback shape
|
||
(`Action<string, object?>`) has no quality channel so nothing can push a Bad
|
||
snapshot, and nothing schedules a retry: replay runs only when a *new client*
|
||
is installed. If the fresh client is healthy and one `AddNotificationAsync`
|
||
failed transiently (ADS notification-quota blip, symbol mid-redeploy), that
|
||
tag's push feed is dead with only a warning log — the same
|
||
silent-subscription failure STAB-2 was about, now scoped to single tags.
|
||
*Recommendation:* on replay failure degrade driver health and emit a
|
||
Bad-quality `OnDataChange` for the affected reference (the driver holds
|
||
handle + reference even though the closure doesn't), and re-attempt failed
|
||
intents on the next probe tick rather than the next reconnect.
|
||
|
||
**STAB-17 — Low — NEW — Unsubscribe racing a replay leaks a live ADS
|
||
notification.** `NativeRegistration.Dispose` (`TwinCATDriver.cs:608-613`)
|
||
runs gate-free: it removes the intent from the device registry and disposes
|
||
the current handle. A concurrent reconnect-replay has already snapshotted
|
||
the intent array (`:786`) and may complete `AddNotificationAsync` *after*
|
||
the dispose; `SwapHandle` then installs the fresh handle into a registration
|
||
nobody owns — the ADS notification stays registered on the new client for
|
||
its lifetime, firing `OnDataChange` for an unsubscribed handle. Narrow
|
||
window (unsubscribe must race the replay of a reconnect), bounded blast
|
||
radius (one orphan notification per race until the next client swap).
|
||
*Recommendation:* after `SwapHandle` in the replay, re-check
|
||
`device.NativeRegistrations.ContainsKey(reg.Id)` and dispose the new handle
|
||
if absent (or take `ConnectGate` in `Dispose`).
|
||
|
||
**STAB-3 — High — Modbus transport desynchronizes after a response
|
||
timeout.** `SendAsync` reconnect-retries only on `IsSocketLevelFailure`
|
||
(`Driver.Modbus/ModbusTcpTransport.cs:155-165, 257-261`). A per-op timeout
|
||
surfaces as `OperationCanceledException` — not socket-level — so the socket
|
||
is kept with a half-read (or still-in-flight) response. The next
|
||
transaction then reads the stale response, hits the TxId mismatch check
|
||
(`ModbusTcpTransport.cs:233-235`), and throws `InvalidDataException` —
|
||
which is *also* not socket-level, so the stream is still not torn down.
|
||
The single-flight connection stays permanently desynchronized until a real
|
||
socket error or the (optional, default-off) idle-disconnect fires.
|
||
*Recommendation:* treat per-op timeout and any framing violation
|
||
(TxId mismatch, truncated header) as connection-fatal: tear down the socket
|
||
before propagating. Fix this together with STAB-15 — it is the same defect
|
||
class in two drivers now.
|
||
|
||
**STAB-4 — High — AbCip runs concurrent operations on shared libplctag
|
||
handles with no serialization.** `ReadSingleAsync` / `ReadGroupAsync` /
|
||
`WriteAsync` execute Read→GetStatus→Decode and Encode→Write→GetStatus on
|
||
cached `IAbCipTagRuntime` instances with zero locking
|
||
(`Driver.AbCip/AbCipDriver.cs:544-575, 614-637, 708-726`) while the server
|
||
read path, every poll-group loop, and the alarm-projection loop
|
||
(`AbCipAlarmProjection.cs:230-232`) can hit the same handle concurrently.
|
||
AbLegacy documents this exact hazard and guards it with a per-runtime
|
||
operation lock ("A libplctag Tag handle is not safe for concurrent
|
||
Read/GetStatus/DecodeValue", `Driver.AbLegacy/AbLegacyDriver.cs:775-787`);
|
||
the fix never propagated to AbCip. Consequences: torn/wrong values decoded
|
||
with Good status, cross-attributed `GetStatus` results.
|
||
*Recommendation:* port AbLegacy's `GetRuntimeLock` pattern to AbCip.
|
||
|
||
**STAB-5 — High — FOCAS fixed-tree caches are plain `Dictionary`s mutated
|
||
across threads.** `LastFixedSnapshots` / `LastServoLoads` /
|
||
`LastSpindleLoads` / `LastTimers` (`Driver.FOCAS/FocasDriver.cs:1184-1194`)
|
||
are written by `FixedTreeLoopAsync` (`FocasDriver.cs:752, 793, 836-839`)
|
||
while `TryReadFixedTree` reads them on `ReadAsync` callers' threads
|
||
(`FocasDriver.cs:912, 920, 945`). Concurrent write-during-read on
|
||
`Dictionary<K,V>` is undefined behaviour (corruption or throw on resize).
|
||
The neighbouring `_parsedAddressesByTagName` already got the
|
||
`ConcurrentDictionary` treatment (`FocasDriver.cs:40`); these did not.
|
||
*Recommendation:* convert to `ConcurrentDictionary` or swap immutable
|
||
snapshots.
|
||
|
||
**STAB-6 — High — AbLegacy probe-loop shutdown race; S7 unsubscribe
|
||
race.** AbLegacy discards the probe task at spawn (`_ = Task.Run(...)`,
|
||
`Driver.AbLegacy/AbLegacyDriver.cs:118`) and `ShutdownAsync` cancels then
|
||
immediately disposes the CTS and runtimes without awaiting loop exit
|
||
(`AbLegacyDriver.cs:156-170`) — a winding-down loop can raise
|
||
`OnHostStatusChanged` after shutdown and touch a disposed CTS. AbCip fixed
|
||
this identical bug (retained `ProbeTask` + phased shutdown,
|
||
`Driver.AbCip/AbCipDriver.cs:295-298, 332-365`); the fix never propagated
|
||
back. S7 has the mirror-image bug in `UnsubscribeAsync`: cancel + dispose
|
||
the CTS without draining `PollTask` (`Driver.S7/S7Driver.cs:1350-1354`),
|
||
so `Task.Delay` on the disposed source can throw `ObjectDisposedException`
|
||
that the loop only catches as OCE (`S7Driver.cs:1396-1397`) —
|
||
`PollGroupEngine.StopState` exists precisely to prevent this
|
||
(`PollGroupEngine.cs:99-112, 136`). (S7's `ShutdownAsync` does drain
|
||
`PollTask` bounded — the gap is specifically per-subscription unsubscribe.)
|
||
*Recommendation:* await the loop task (bounded) before disposing the CTS in
|
||
both places.
|
||
|
||
**STAB-7 — High — `InitializeAsync` ignores `driverConfigJson` in Modbus
|
||
and AbLegacy.** AbLegacy's `_options` is readonly ctor state and
|
||
`Initialize/Reinitialize` never parse the parameter
|
||
(`Driver.AbLegacy/AbLegacyDriver.cs:16, 78-153`); Modbus likewise uses only
|
||
ctor options (`Driver.Modbus/ModbusDriver.cs:167-197`). S7, AbCip and
|
||
TwinCAT deliberately re-parse (`Driver.AbCip/AbCipDriver.cs:229-236`
|
||
documents "rather than being silently discarded"). Any config change
|
||
delivered through `ReinitializeAsync` on a live instance is silently
|
||
discarded for these two drivers. TwinCAT has a related quirk: a parsed
|
||
config with zero devices AND zero tags is silently discarded in favour of
|
||
ctor options (`Driver.TwinCAT/TwinCATDriver.cs:89-94`), so an intentional
|
||
"empty the driver" redeploy is ignored. *Recommendation:* make config
|
||
re-parse on initialize a template requirement; add a consuming test per
|
||
driver.
|
||
|
||
**STAB-8 — Medium — No reconnect backoff anywhere except the Modbus
|
||
transport and S7's poll loop.** A dead device pays: AbCip — a full `Tag`
|
||
create + Forward Open per tag per poll tick (evict-recreate,
|
||
`AbCipDriver.cs:891-897`); FOCAS — a full two-socket reconnect attempt per
|
||
tick of *each* of three loops (`Driver.FOCAS/FocasDriver.cs:1089-1120`);
|
||
TwinCAT — a connect attempt per call (`TwinCATDriver.cs:706-721`); **S7
|
||
(new) — a full TCP connect attempt bounded only by `_options.Timeout` on
|
||
every read/write batch and every probe tick while the PLC is down**
|
||
(`S7Driver.cs:1188-1224` retries unconditionally on each call; the poll
|
||
loop's capped backoff `:1393-1397` shields subscriptions only).
|
||
`PollGroupEngine` itself polls at a fixed cadence regardless of failures —
|
||
S7's bespoke loop is the only subscriber-side backoff in the fleet.
|
||
*Recommendation:* add optional failure backoff to `PollGroupEngine` (adopt
|
||
S7's `ComputeBackoffDelay`, `S7Driver.cs:1447`) and a small connect-attempt
|
||
throttle to the lazy-reconnect drivers (S7's `EnsureConnectedAsync` is now
|
||
the natural home for a last-attempt timestamp).
|
||
|
||
> **SEAM DOCUMENTED by R2-01 (branch `r2/01-s7-fault-hardening`).** The S7 connect-throttle is left to
|
||
> R2-09 (fleet-wide reconnect-backoff), not implemented driver-locally. `EnsureConnectedAsync`'s `<remarks>`
|
||
> now names itself as the single gate-serialized choke-point where a last-failed-attempt timestamp plugs in.
|
||
> (Note: R2-01's STAB-14/15 fixes already *reduce* dead-PLC cost — poll ticks that used to die now back off to
|
||
> `PollBackoffCap`, and connect-timeouts no longer escape as anomalous OCEs.)
|
||
|
||
**STAB-9 — Medium — `PollGroupEngine`'s `onError` sink is dead code: no
|
||
driver passes it.** All five consumers construct the engine without the
|
||
error callback (`ModbusDriver.cs:115`, `AbCipDriver.cs:133`,
|
||
`AbLegacyDriver.cs:65`, `TwinCATDriver.cs:69`, `FocasDriver.cs:72`), so a
|
||
reader exception in a poll tick (e.g. `RequireTransport` after teardown,
|
||
reader contract violation) is silently swallowed — the precise
|
||
"silently-broken subscription" failure mode the engine's own doc comment
|
||
warns about (`PollGroupEngine.cs:21-25, 164-169`). *Recommendation:* pass
|
||
`onError` routing to the driver's health surface + logger in all five;
|
||
consider making the parameter required.
|
||
|
||
**STAB-10 — Medium — AbLegacy never evicts failed tag handles on the data
|
||
path.** Non-zero status and transport exceptions leave the cached runtime
|
||
in place (`AbLegacyDriver.cs:246-259, 269-274`) with no recreate-on-failure
|
||
(contrast `AbCipDriver.cs:891-897` and AbLegacy's own probe loop at
|
||
`AbLegacyDriver.cs:456-461`). A handle invalidated by a PLC state change
|
||
pins the tag Bad until full driver reinit.
|
||
|
||
**STAB-11 — Medium — RMW locks don't cover direct parent writes (AbCip,
|
||
AbLegacy, TwinCAT).** The per-parent semaphore serializes bit-RMW callers
|
||
against each other, but a direct write to the parent word/DINT itself goes
|
||
through the normal write path without taking the same lock
|
||
(`AbCipDriver.cs:708-710` vs `787-818`; same shape in AbLegacy and
|
||
TwinCAT), so it can interleave with an in-flight read-modify-write → lost
|
||
update. Additionally AbCip's RMW hard-codes DINT while `AbCipTagPath`
|
||
accepts `.N` on any parent (`Driver.AbCip/AbCipTagPath.cs:17-19`), and
|
||
TwinCAT's RMW always accesses the parent as 4-byte `UDInt` — explicitly
|
||
flagged as an unverified assumption that will likely fail
|
||
`DeviceInvalidSize` on WORD/BYTE parents on real hardware
|
||
(`Driver.TwinCAT/AdsTwinCATClient.cs:96-107`).
|
||
|
||
**STAB-12 — Medium — Blocking/uncancellable teardown paths.** FOCAS's sync
|
||
`Dispose()` performs a close-PDU exchange (`SendPdu` + `ReadPdu`) on a
|
||
`NetworkStream` with no ReadTimeout and no token
|
||
(`Driver.FOCAS/Wire/FocasWireClient.cs:164-175`) — a stalled CNC can wedge
|
||
`ShutdownAsync` indefinitely. TwinCAT's `ConnectAsync` calls the blocking
|
||
SDK `Connect` and ignores both its token and timeout parameter for the
|
||
connect itself (`AdsTwinCATClient.cs:73-80`). **New compounding:** the
|
||
probe loop's `RecycleClientAsync` waits on `ConnectGate` with
|
||
`CancellationToken.None` (`TwinCATDriver.cs:835`), so a connect wedged under
|
||
the gate now also wedges the probe loop beyond cancellation.
|
||
|
||
**STAB-13 — Positive.** Write-outcome surfacing — the seam the server's
|
||
value-revert self-correction depends on — is honoured by every protocol
|
||
driver: per-item `WriteResult` with typed exception→status mapping (Modbus
|
||
`ModbusDriver.cs:897-942`; S7 `S7Driver.cs:981-1083` including the
|
||
PUT/GET-denied → `Faulted` escalation, preserved through the reconnect
|
||
rework; AbCip `AbCipDriver.cs:732-775`; AbLegacy
|
||
`AbLegacyDriver.cs:348-366`; TwinCAT `TwinCATDriver.cs:332-349` with a
|
||
numerically-verified ADS error table; OpcUaClient passes upstream codes
|
||
verbatim and distinguishes post-dispatch cancellation as `BadTimeout`
|
||
"outcome unknown" — `OpcUaClientDriver.cs:753-779`, the most careful
|
||
non-idempotency handling in the fleet). Nothing is fire-and-forget except
|
||
the AbCip alarm *acknowledge* path (UNDER-5). OpcUaClient's reconnect state
|
||
machine (double-arm guard, re-arm on exhaustion, session swap under
|
||
`_probeLock` — `OpcUaClientDriver.cs:1908-2058`) remains the strongest
|
||
reconnect implementation in the subsystem. The two new reconnect paths are
|
||
also well-shaped where it counts: S7's dead-handle flag guarantees
|
||
exactly-once disposal under the gate (`S7Driver.cs:1188-1204`), and
|
||
TwinCAT's register/replay share one `ConnectGate` with
|
||
intent-stored-only-after-handle-live ordering (`TwinCATDriver.cs:508-520`)
|
||
— the obvious double-registration race was designed out.
|
||
|
||
### 2. Performance
|
||
|
||
**PERF-1 — High — Five of seven drivers read one tag per wire round-trip.**
|
||
Only Modbus (block coalescing + auto-prohibition/bisection,
|
||
`ModbusDriver.cs:687-795`) and OpcUaClient (single
|
||
`ReadValueIdCollection` per batch, `OpcUaClientDriver.cs:640-666`) batch.
|
||
The others are O(N) round trips per poll tick on serialized links:
|
||
- S7: one PDU per tag under one `_gate`d connection
|
||
(`S7Driver.cs:466-545`); S7comm multi-var reads (S7.Net
|
||
`ReadMultipleVarsAsync`) unused.
|
||
- TwinCAT: one ADS call per symbol; ADS **sum-read** — Beckhoff's
|
||
documented batching mechanism — unused (`TwinCATDriver.cs:203-248`).
|
||
- FOCAS: one request/response per address on a strictly single-flight
|
||
socket (`SynchronizedFocasClient.cs:34`); `pmc_rdpmcrng` supports ranges
|
||
but each read fetches exactly one value's width
|
||
(`Wire/WireFocasClient.cs:296-334`). Throughput ceiling ≈ tags × RTT.
|
||
- AbLegacy: one PCCC transaction per scalar; no span coalescing of
|
||
adjacent file elements (N7:0..N7:2), the natural Modbus analog.
|
||
*Recommendation:* per-protocol batching is the single biggest scalability
|
||
lever; priority order = TwinCAT sum-read, S7 multi-var, FOCAS PMC range
|
||
reads, AbLegacy spans.
|
||
|
||
**PERF-2 — High — AbCip's whole-UDT batched read exists in two halves that
|
||
were never joined.** The read planner
|
||
(`Driver.AbCip/AbCipUdtReadPlanner.cs:32-100`) can collapse N members into
|
||
one parent read, but only via declaration-order layout — off by default
|
||
because it produces "silently-plausible wrong numbers" when member order
|
||
diverges (`AbCipDriverOptions.cs:60-72`). Meanwhile the driver already
|
||
fetches and caches *true* member offsets via the CIP Template Object
|
||
(`AbCipDriver.cs:151-182`, `AbCipTemplateCache.cs`) — used only for
|
||
discovery, never consulted by the planner
|
||
(`AbCipUdtMemberLayout.cs:22-24` even points at the richer path). Net: in
|
||
every safe configuration, N UDT members = N CIP round trips per tick.
|
||
*Recommendation:* feed `AbCipUdtShape` offsets into the planner; retire
|
||
the unsafe declaration-order mode.
|
||
|
||
**PERF-3 — High — S7's forked poll loop diffs arrays by reference.**
|
||
`PollOnceAsync` compares `Equals(lastSeen?.Value, current.Value)`
|
||
(`S7Driver.cs:1469`); array reads return a fresh CLR array every poll, so
|
||
every subscribed array tag fires `OnDataChange` **every tick**, flooding
|
||
the dependency mux/DPS downstream. `PollGroupEngine` fixed exactly this
|
||
with `StructuralComparisons` (`PollGroupEngine.cs:203-209`); the bespoke
|
||
S7 copy never picked it up. (Also a Conventions finding — see CONV-1.)
|
||
|
||
**PERF-4 — Medium — OpcUaClient serializes all traffic on one global
|
||
`SemaphoreSlim(1,1)`.** Reads, writes, discovery, subscription creates,
|
||
acks and HistoryReads all queue on `_gate`
|
||
(`OpcUaClientDriver.cs:78, 626, 713, 836, 1186, 1377, 1451, 1653, 1825`).
|
||
OPC UA sessions multiplex service calls natively; a multi-second
|
||
HistoryRead or full re-discovery blocks every live read/write behind it.
|
||
The gate is only needed for session-swap consistency, which `_probeLock` +
|
||
re-read-inside-critical-section already provide.
|
||
|
||
**PERF-5 — Medium — OpcUaClient discovery enrichment issues one giant
|
||
un-chunked Read and silently degrades on rejection.**
|
||
`EnrichAndRegisterVariablesAsync` sends `pending.Count * 4` ReadValueIds in
|
||
a single call (`OpcUaClientDriver.cs:1029-1046`) — 40k operations for a
|
||
10k-node server. A server enforcing `MaxNodesPerRead` returns
|
||
`BadTooManyOperations`; the blanket catch (`:1049-1057`) then registers
|
||
*every* variable as Int32/ViewOnly/non-historized with no log or health
|
||
signal. *Recommendation:* chunk client-side; log the fallback.
|
||
|
||
**PERF-6 — Low — Assorted per-tick allocation churn.** TwinCAT re-parses
|
||
`TwinCATSymbolPath` on every read/write/subscribe call — lists +
|
||
StringBuilder per tag per tick (`TwinCATDriver.cs:220, 284, 468`); S7
|
||
caches parses (`_parsedByName`) and is the model. Modbus publishes a fresh
|
||
`DriverHealth` record per successful tag read (`ModbusDriver.cs:285`).
|
||
AbCip's `BuildArray<T>` boxes per element
|
||
(`LibplctagTagRuntime.cs:107-118`). None are hot enough to be urgent.
|
||
|
||
**PERF-7 — Low — Deadband exists only in Modbus.** The publish-suppression
|
||
deadband (`ModbusDriver.cs:136-159`) and `WriteOnChangeOnly` never
|
||
generalized; noisy analog CNC/PLC signals on the other five poll-based
|
||
drivers publish every jitter. The mechanism is driver-agnostic and belongs
|
||
in or beside `PollGroupEngine`.
|
||
|
||
### 3. Conventions
|
||
|
||
**CONV-1 — High — S7 forked the poll loop instead of using
|
||
`PollGroupEngine`, and the fork has diverged in both directions.**
|
||
`PollGroupEngine`'s own doc claims it serves "Modbus, AB CIP, S7, FOCAS"
|
||
(`PollGroupEngine.cs:8-9`) but S7 re-ships the entire loop
|
||
(`S7Driver.cs:1373-1412`). The fork is *better* in two ways the engine
|
||
never absorbed (capped exponential failure backoff `:1393-1397, 1447`;
|
||
poll failures logged + degrade health `:1421-1437`) and *worse* in three
|
||
ways the engine already fixed or avoided (reference-equality array diff —
|
||
PERF-3; CTS disposed without draining the loop — STAB-6; **and the bare
|
||
`catch (OperationCanceledException) { return; }` that STAB-14 turns into
|
||
permanent subscription death**). This is the textbook cost of template
|
||
divergence: each side owns fixes the other needs, and the fork's OCE
|
||
handling is now load-bearing for a High-severity defect. *Recommendation:*
|
||
extend `PollGroupEngine` with backoff + onError-driven health, then move
|
||
S7 onto it and delete the fork.
|
||
|
||
**CONV-2 — High — Three different strictness tiers parse the same config,
|
||
and the most-used one is the most permissive.** For every driver the same
|
||
enum field is parsed three ways:
|
||
- **Factory**: string-typed DTO + fail-loud `ParseEnum` listing valid
|
||
values (`ModbusDriverFactoryExtensions.cs:191-201` and equivalents) —
|
||
deliberate, good.
|
||
- **Probe**: deserializes with `JsonStringEnumConverter`; Modbus's probe
|
||
binds the *runtime options type* (`ModbusDriverOptions`, TimeSpan
|
||
fields) rather than the factory DTO shape
|
||
(`ModbusDriverProbe.cs:19-24, 36`), so a DB config with `timeoutMs`
|
||
silently loses its timeout in the probe and tag-shape mismatches can
|
||
make the probe reject/misread a config the factory accepts. OpcUaClient
|
||
documents probe/factory parse-parity as the rule
|
||
(`OpcUaClientDriverProbe.cs:22`); Modbus violates it.
|
||
- **Equipment-tag parser**: silent fallback — an unknown or typo'd
|
||
`dataType` string quietly becomes the default type (`Int16`/`DInt`/
|
||
`Int`/`Int32` per driver) instead of a rejection
|
||
(`Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs:65-67`,
|
||
`S7EquipmentTagParser.cs:59-61`, `AbCipEquipmentTagParser.cs:86-88`,
|
||
`AbLegacyEquipmentTagParser.cs:60-62`,
|
||
`TwinCATEquipmentTagParser.cs:47-49`,
|
||
`FocasEquipmentTagParser.cs:43-45`). A mis-typed equipment tag reads and
|
||
writes the wrong width with Good status. The identical private
|
||
`ReadEnum` helper is copy-pasted six times instead of living beside
|
||
`EquipmentTagRefResolver` in Core.Abstractions.
|
||
*Recommendation:* hoist a shared strict `ReadEnum` (present-but-invalid ⇒
|
||
parse failure ⇒ `BadNodeIdUnknown`); make probes parse the factory DTO.
|
||
|
||
**CONV-3 — Medium — Factory registration asymmetry leaves four drivers
|
||
logging to `NullLogger` in production.** S7, TwinCAT, AbCip and FOCAS
|
||
`Register(registry)` take no `ILoggerFactory`
|
||
(`S7DriverFactoryExtensions.cs:19`, `TwinCATDriverFactoryExtensions.cs:18`,
|
||
`AbCipDriverFactoryExtensions.cs:21`, `FocasDriverFactoryExtensions.cs:36`)
|
||
even though all four driver classes accept a logger; the bootstrap
|
||
consequently can't pass one
|
||
(`Host/Drivers/DriverFactoryBootstrap.cs:100-107`). This finding got
|
||
*more* expensive since the prior review: the S7 reconnect path
|
||
("S7Driver reconnected", replay tallies) and the TwinCAT replay path
|
||
("re-registered {Replayed}/{Total} native notifications") log recovery and
|
||
partial-replay failures **only** through `_logger` — in production those
|
||
two brand-new recovery mechanisms run blind. Modbus/AbLegacy/OpcUaClient/
|
||
Galaxy have the logger-aware overload. *Recommendation:* one-line fix per
|
||
factory; align all eight `Register` signatures.
|
||
|
||
**CONV-4 — Medium — `ResolveHost` mis-keys equipment-tag references in
|
||
every multi-device driver.** The implementations consult only
|
||
`_tagsByName` (authored names); an equipment-tag TagConfig-JSON reference
|
||
never matches and falls back to the driver-level/first-device host
|
||
(`AbCipDriver.cs:483-488`, `AbLegacyDriver.cs:496-501`,
|
||
`TwinCATDriver.cs:689-696`, `FocasDriver.cs:1082-1087`; Modbus
|
||
`ModbusDriver.cs:125-131` has the same shape but per-tag `UnitId` isn't in
|
||
the equipment parser anyway). Per-host Polly bulkhead/circuit-breaker keys
|
||
are therefore wrong for equipment tags on multi-device instances — a
|
||
broken device B can trip device A's breaker and vice versa. Since
|
||
equipment tags are the *primary* authoring model post-Galaxy-standardization,
|
||
and the CapabilityInvoker dispatch wiring (task #10) now actually consumes
|
||
these keys in production, the mis-keying is no longer latent.
|
||
*Recommendation:* route `ResolveHost` through
|
||
`EquipmentTagRefResolver` and derive the host from the parsed def
|
||
(`deviceHostAddress` / `unitId`).
|
||
|
||
**CONV-5 — Medium — Health-field publication conventions drift.** The
|
||
template documents `Volatile.Read/Write` (`ModbusDriver.cs:217-228`);
|
||
FOCAS conforms (`FocasDriver.cs:43-46`), AbLegacy uses a `volatile` field
|
||
with rationale (`AbLegacyDriver.cs:29-34`), while S7
|
||
(`S7Driver.cs:240`), AbCip (`AbCipDriver.cs:95`), TwinCAT and
|
||
OpcUaClient (`OpcUaClientDriver.cs:96`) use plain fields. Immutable record
|
||
swap means no torn reads — this is a visibility/staleness nit — but four
|
||
different idioms for the same one-line concern is exactly the divergence
|
||
this review is hunting. Related one-offs: TwinCAT declares `Healthy` at
|
||
init with zero wire contact (`TwinCATDriver.cs:115`) where every other
|
||
driver proves the connection first; TwinCAT reads host state without its
|
||
probe lock (`~TwinCATDriver.cs:554`); AbLegacy never refreshes health
|
||
on successful writes (`AbLegacyDriver.cs:344-346` vs
|
||
`AbCipDriver.cs:725`); AbLegacy silently clobbers duplicate tag names
|
||
(`AbLegacyDriver.cs:91`) where AbCip fails fast
|
||
(`AbCipDriver.cs:249-256`).
|
||
|
||
**CONV-6 — Medium — Contracts layering is uniform except OpcUaClient,
|
||
which drags the whole OPC UA SDK into its Contracts.** Six Contracts
|
||
projects are POCO options + equipment-tag parser (FOCAS's csproj even
|
||
enforces "NO PackageReference. NO ProjectReference."), but
|
||
`Driver.OpcUaClient.Contracts` carries a full OPC UA stack reference
|
||
because `NamespaceMap` lives there
|
||
(`OpcUaClient.Contracts.csproj:9`) — every consumer wanting just the
|
||
options DTO (probe hosts, AdminUI editors, Browser) transitively loads the
|
||
SDK. This is the previously-deferred OpcUaClient.Contracts-002 finding;
|
||
it remains the right call to move `NamespaceMap` out. Minor: all Contracts
|
||
projects use the parent namespace without a `.Contracts` suffix —
|
||
consistent, just surprising.
|
||
|
||
**CONV-7 — Low/Medium — CLI harness divergences.** The six CLIs share
|
||
`DriverCommandBase` + `SnapshotFormatter` and a uniform
|
||
probe/read/write/subscribe verb set, but: `ParseValue`/`ParseBool` is
|
||
copy-pasted six times (`*/Commands/WriteCommand.cs`), option validation is
|
||
a different shape in every base and **absent entirely in AbLegacy**
|
||
(`AbLegacyCommandBase.cs` — `--timeout-ms 0` reaches the driver), the
|
||
abstract `Timeout` init-setter is a silent no-op in five bases but throws
|
||
`NotSupportedException` in AbCip (`AbCipCommandBase.cs:43`), and only
|
||
TwinCAT exposes a `browse` command despite AbCip also supporting
|
||
controller browse (hardcoded `EnableControllerBrowse=false`,
|
||
`AbCipCommandBase.cs:63`). OpcUaClient has no CLI at all (the server-side
|
||
Client.CLI tests the *server*, not this driver against a third-party
|
||
endpoint). The six csprojs gained a commented `NoWarn OTOPCUA0001` in the
|
||
analyzer wiring — consistent and correct for wire-level harnesses.
|
||
*Recommendation:* hoist value parsing + a standard `Validate()`
|
||
into Cli.Common; add an OpcUaClient CLI or document the gap.
|
||
|
||
**CONV-8 — Low — Dead/no-op config knobs and stale scaffold docs.**
|
||
`DisableFC23` is a documented no-op (`ModbusDriverOptions.cs:83-89`);
|
||
AbCip `ConnectionSize` is plumbed but never applied
|
||
(`AbCipDriverOptions.cs:97-103`); AbLegacy's class doc still says
|
||
read/write "ship in PRs 2 and 3" (`AbLegacyDriver.cs:9-11`); OpcUaClient's
|
||
header still says "PR 66 ships the scaffold: IDriver only"
|
||
(`OpcUaClientDriver.cs:12-14`). No options class in the fleet has a
|
||
`Validate()` method — validation is scattered across factory throws and
|
||
init guards (S7's init guards are the best of breed,
|
||
`S7Driver.cs:340-420`).
|
||
|
||
### 4. Underdeveloped areas
|
||
|
||
**UNDER-1 — High — FOCAS advertises writes it cannot perform.** The only
|
||
real backend returns `BadNotWritable` for every address
|
||
(`Wire/WireFocasClient.cs:71-73`), yet `FocasTagDefinition.Writable`
|
||
defaults `true` with a doc-comment citing write tests
|
||
(`FOCAS.Contracts/FocasDriverOptions.cs:152-158`) and
|
||
`FocasEquipmentTagParser` hard-codes `Writable: true`
|
||
(`FocasEquipmentTagParser.cs:35`) — so equipment tags can surface as
|
||
Operate-writable OPC UA nodes whose writes always fail. The driver-side
|
||
write status mapping (`FocasDriver.cs:353-381`) is currently dead code.
|
||
*Recommendation:* either implement PMC writes in the wire client or force
|
||
`Writable:false` at the parser/discovery seams until it exists.
|
||
|
||
**UNDER-2 — High — OpcUaClient's `UnsMappingTable` is validated as
|
||
mandatory but consumed nowhere.** `ValidateNamespaceKind` hard-fails
|
||
Equipment-kind configs lacking a mapping table
|
||
(`OpcUaClientDriver.cs:330-355`), yet no code reads the table —
|
||
`DiscoverAsync` builds the local tree purely from remote browse structure
|
||
(`:824-1105`). Operators are forced to author config with zero runtime
|
||
effect; the promised remote→UNS remapping feature does not exist.
|
||
*Recommendation:* delete the gate or build the feature.
|
||
|
||
**UNDER-3 — High — OpcUaClient alarm source-node filtering compares
|
||
mismatched NodeId encodings.** `OnEventNotification` filters
|
||
session-relative `ns=N;…` renderings against caller-supplied refs by
|
||
ordinal string compare (`OpcUaClientDriver.cs:1337, 1538-1539`), while the
|
||
driver's own persisted references are stable `nsu=…` strings
|
||
(`:808-809`). Any non-empty filter built from stored refs silently drops
|
||
every event; only empty-filter subscriptions work today.
|
||
|
||
**UNDER-4 — Medium — S7 feature gaps make authored array tags
|
||
unreachable.** `S7TagDto` has no `ArrayCount` field and `BuildTag` never
|
||
sets it (`S7DriverFactoryExtensions.cs:80-90, 137-151`) — driver-config
|
||
array tags are silently scalar; arrays only work as equipment tags
|
||
(`S7EquipmentTagParser.cs:73-87`). Array *writes* are unsupported
|
||
(`S7Driver.cs:~1088-1110`), wide-type arrays are a stated follow-up
|
||
(`:344, :357`), and `UInt32` surfaces lossily as `Int32`
|
||
(`S7Driver.cs:1315`).
|
||
|
||
**UNDER-5 — Medium — AbCip alarm acknowledge outcomes are discarded.**
|
||
`AcknowledgeAsync` fire-and-forgets the write results
|
||
(`AbCipAlarmProjection.cs:149-151`); a failed ack (undeclared `.Acked`
|
||
member, non-writable, comms error) is invisible to the operator, health,
|
||
and the Part 9 caller — conditions stick un-acked with no visible cause.
|
||
(The new OTOPCUA0001 pragma at this site correctly annotates the
|
||
*self-call* as intentional; the discarded outcomes remain the defect.)
|
||
Related discovery weak spots: with `EnableControllerBrowse` on, one
|
||
unreachable PLC faults the entire multi-device `DiscoverAsync`
|
||
(uncaught `await foreach`, `AbCipDriver.cs:970-1037`), and browsed tags
|
||
default to writable/Operate (`CipSymbolObjectDecoder.cs:91`).
|
||
|
||
**UNDER-6 — Medium — Equipment-tag parsers lag the authored-tag feature
|
||
set.** Modbus's parser ignores `unitId`, `deadband`, `stringByteOrder`,
|
||
`writable`, `coalesceProhibited` and the address-grammar string, and
|
||
forces `Writable: true` (`ModbusEquipmentTagParser.cs:54-57`); AbLegacy
|
||
and FOCAS also hard-code writable-true
|
||
(`AbLegacyEquipmentTagParser.cs:52`, `FocasEquipmentTagParser.cs:35`) —
|
||
read-only equipment tags are unauthorable on three drivers (AbCip honours
|
||
a `writable` key, `AbCipEquipmentTagParser.cs:53-54`). FOCAS equipment
|
||
tags additionally bypass the `FocasCapabilityMatrix` pre-flight gate that
|
||
authored tags get (`FocasDriver.cs:243-247` vs `:115-119`). As equipment
|
||
tags are the primary authoring model, the parsers deserve parity plus a
|
||
shared conformance test.
|
||
|
||
**UNDER-7 — Medium — Live-risk assumptions documented but ungated.**
|
||
TwinCAT carries two explicit "unverified against real hardware" blocks on
|
||
core paths — array read shape (`AdsTwinCATClient.cs:109-116`) and
|
||
Flat-mode `SubSymbols` population (`:263-277`, struct members could
|
||
silently vanish from discovery) — plus the bit-RMW width assumption
|
||
(STAB-11), **and now the entire native-notification replay path, which is
|
||
fake-client-verified only** (no TC3 docker fixture exists; STATUS.md
|
||
acknowledges the unit tests are authoritative). AbLegacy's array decode
|
||
rests on five explicitly unproven layout assumptions with no PCCC fixture
|
||
(`LibplctagLegacyTagRuntime.cs:64-88`). FOCAS's `cnc_getfigure` command id
|
||
and servo-load scaling are sim-validated only
|
||
(`FocasWireClient.cs:391-395, 943-947`). These are honest, well-documented
|
||
debts — but nothing aggregates them into a "needs bench time" checklist.
|
||
|
||
**UNDER-8 — Medium — Test coverage is codec-heavy, failure-path-light, and
|
||
uneven — partially improved.** The prior review's worst gap — **zero S7
|
||
reconnect tests** — is closed: `S7DriverReconnectTests` (4 tests: fatal
|
||
fault → reopen, data-address error → no reopen, raw socket exception →
|
||
reopen, reopen-fails-then-recovers) plus the env-gated live
|
||
`S7_1500ReconnectTests` (a real `docker restart` bounce, live-verified per
|
||
STATUS.md). TwinCAT gained `TwinCATReconnectReplayTests` (4 tests: replay
|
||
onto fresh client, all-tags replay, unsubscribe-after-reconnect disposes +
|
||
stops replaying, probe-failure recycle) — fake-client only, per UNDER-7.
|
||
Remaining systemic gaps: **zero reconnect tests for AbLegacy** (STAB-10
|
||
lives there), zero concurrency tests for S7 and AbCip (STAB-4 lives
|
||
there; only AbLegacy has a dedicated runtime-concurrency suite), **no
|
||
connect-timeout (vs connect-refused) outage test anywhere** — precisely
|
||
the STAB-14 blind spot — and OpcUaClient still has the widest feature
|
||
surface on the lowest test density. `Cli.Common`'s `DriverCommandBase` has
|
||
no direct tests. There remains **zero TODO/HACK/FIXME/
|
||
NotImplementedException debt** in the subsystem.
|
||
|
||
**UNDER-9 — Low — OpcUaClient is a 2,154-line monolith.** Six separable
|
||
responsibilities share one file (config/PKI/identity, failover sweep,
|
||
recursive discovery, live subscriptions, alarms/acks, the full
|
||
IHistoryProvider, plus the reconnect state machine). The reconnect state
|
||
machine is the trickiest concurrency in the subsystem and deserves
|
||
extraction for isolated testability. Also: subscription transfer rests on
|
||
the *unset* SDK default `TransferSubscriptionsOnReconnect`
|
||
(`OpcUaClientDriver.cs:1946-1949`), and `BuildCertificateIdentity` only
|
||
loads PKCS#12 while the options doc promises PEM support
|
||
(`:479-484` vs `OpcUaClientDriverOptions.cs:68-75`).
|
||
|
||
---
|
||
|
||
## Maturity ratings
|
||
|
||
| Dimension | Rating (1-5) | Prior | Justification |
|
||
|---|---|---|---|
|
||
| Stability | **3** | 3 | Both Criticals are genuinely closed — a routine PLC power-cycle no longer permanently kills S7, and TwinCAT native subs now replay — and the fixes are well-shaped (seam-based, gate-disciplined, guard-tested). Held at 3 rather than promoted because the S7 fix ships a High regression in the same scenario class (STAB-14: connect-*timeout* outages permanently kill S7 subscription polls) and reproduces the Modbus desync defect (STAB-15), while the untouched silent-corruption Highs (STAB-3/4/5) and the shutdown races (STAB-6) all remain. |
|
||
| Performance | **2** | 2 | Unchanged — no batching work landed. Only 2 of 7 drivers batch reads; AbCip's flagship UDT batching is still half-built; deadband exists only in Modbus; the subsystem scales by tag count, not request count. S7's new per-call reconnect attempt marginally worsens dead-device cost (STAB-8). |
|
||
| Conventions | **3** | 3 | Unchanged in substance. The shared seams are real and the analyzer/pragma discipline is a genuine improvement, but the S7 poll-loop fork (now hosting a High defect in its OCE handling), three-tier config-parse strictness, six-way `ReadEnum` copy-paste, logger-registration asymmetry (now hiding the new recovery logs in production), and drifting health idioms all persist. |
|
||
| Underdeveloped areas | **3** | 3 | Slightly improved within the band: the S7 reconnect test gap is closed with both unit and live coverage, TwinCAT replay has fake-client guards. Advertised-but-dead features (FOCAS writes, `UnsMappingTable`, `DisableFC23`, `ConnectionSize`), equipment-tag parser lag, AbLegacy/concurrency test gaps, and the growing unverified-against-hardware set (now including the replay path) keep it at 3. |
|
||
|
||
---
|
||
|
||
## Recommended remediation order
|
||
|
||
1. **STAB-14 — S7 connect-timeout OCE kills subscription polls** — a
|
||
two-line filter class fix (`when (cancellationToken.IsCancellationRequested)`
|
||
on the ensure-wrapper rethrow + the poll-loop catches) plus a
|
||
fake-factory connect-timeout unit test. This is a regression inside the
|
||
Critical-3 fix and belongs at the top.
|
||
2. **STAB-15 + STAB-3 together — timeout/framing ⇒ connection-fatal** in
|
||
both the new S7 classification and the Modbus transport; same defect
|
||
class, same fix shape (tear down before propagating). Add probe-side
|
||
`MarkConnectionDeadIfFatal` in S7.
|
||
3. **AbCip per-runtime operation lock** (STAB-4, port from AbLegacy) and
|
||
**FOCAS ConcurrentDictionary caches** (STAB-5) — silent-corruption
|
||
class.
|
||
4. **TwinCAT replay hardening** (STAB-16 Bad-quality surfacing + probe-tick
|
||
retry; STAB-17 ownership re-check after `SwapHandle`).
|
||
5. **PollGroupEngine v2**: absorb S7's backoff + failure-health, wire
|
||
`onError` in all five consumers, migrate S7 off its fork — this also
|
||
deletes the fork's dangerous OCE handling for good
|
||
(CONV-1, STAB-9, PERF-3, STAB-6, STAB-8).
|
||
6. **Factory `ILoggerFactory`** (CONV-3) — one line per factory, and now a
|
||
prerequisite for the new reconnect/replay paths being observable in
|
||
production at all.
|
||
7. **Shared strict equipment-tag enum parsing + writable/capability
|
||
parity** (CONV-2, UNDER-6) — one Core.Abstractions helper, six deletions.
|
||
8. **Per-protocol read batching** (PERF-1/PERF-2), starting with the AbCip
|
||
planner⇄template-cache join (design already exists in-tree).
|
||
9. Sweep the one-liners: `ResolveHost` via resolver (CONV-4, now live via
|
||
CapabilityInvoker dispatch), FOCAS writable-false (UNDER-1), delete
|
||
`UnsMappingTable` gate (UNDER-2), OpcUaClient alarm-filter encoding
|
||
(UNDER-3), AbLegacy reconnect-test template (UNDER-8).
|