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

Compare commits

base: dohertj2/lmxopcua:5c513f99fd9c5dbe65d8774b6dbc244b878d3a7e
Branches Tags
dohertj2/lmxopcua:master dohertj2/lmxopcua:chore/fixdocs-xml-doc-comments dohertj2/lmxopcua:feat/adopt-zb-health dohertj2/lmxopcua:docs/native-alarms-verified dohertj2/lmxopcua:fix/adminui-probes-and-live-panels dohertj2/lmxopcua:v2-gap-closeout dohertj2/lmxopcua:v2-akka-fuse dohertj2/lmxopcua:refactor/galaxy-mxgateway-client-rename dohertj2/lmxopcua:docs/alarms-plan-banner-reconcile dohertj2/lmxopcua:auto/driver-gaps-stash dohertj2/lmxopcua:auto/driver-gaps
...
compare: dohertj2/lmxopcua:bccff1339d100131f61ba8f68fd6790d8ed4d698
Branches Tags
dohertj2/lmxopcua:master dohertj2/lmxopcua:chore/fixdocs-xml-doc-comments dohertj2/lmxopcua:feat/adopt-zb-health dohertj2/lmxopcua:docs/native-alarms-verified dohertj2/lmxopcua:fix/adminui-probes-and-live-panels dohertj2/lmxopcua:v2-gap-closeout dohertj2/lmxopcua:v2-akka-fuse dohertj2/lmxopcua:refactor/galaxy-mxgateway-client-rename dohertj2/lmxopcua:docs/alarms-plan-banner-reconcile dohertj2/lmxopcua:auto/driver-gaps-stash dohertj2/lmxopcua:auto/driver-gaps

6 Commits
5c513f99fd ... bccff1339d

Author SHA1 Message Date
Joseph Doherty bccff1339d docs(code-reviews): regenerate index — 22 Low findings resolved 
Batch 3 cleared Open findings in Driver.Galaxy, Driver.AbCip,
Driver.AbLegacy, Driver.FOCAS, and Driver.S7.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-23 07:45:53 -04:00
Joseph Doherty af0f09d07e fix(driver-s7): resolve Low code-review findings (Driver.S7-003,005,009,010,013) 
- Driver.S7-003: ArgumentNullException.ThrowIfNull on the references
  argument at the top of ReadAsync / WriteAsync (was reaching .Count
  before any null check).
- Driver.S7-005: drop the redundant global::S7.Net.Plc qualifiers in
  ReadOneAsync / WriteOneAsync — using S7.Net already covers Plc.
- Driver.S7-009: PollLoopAsync degrades _health to Degraded after
  sustained failure and backs off exponentially up to PollBackoffCap;
  resets on a healthy tick so an operator can see the loop wedge.
- Driver.S7-010: Dispose runs the synchronous teardown directly with a
  bounded WhenAll Wait drain instead of bridging via DisposeAsync().
- Driver.S7-013: reject unsupported S7DataType values (Int64 / UInt64 /
  Float64 / String / DateTime) at InitializeAsync so half-implemented
  types no longer leak BadNotSupported live nodes into the address space.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-23 07:45:45 -04:00
Joseph Doherty 6575c6e5f6 fix(driver-focas): resolve Low code-review findings (Driver.FOCAS-007,008,009,010,011) 
- Driver.FOCAS-007: optional ILogger<FocasDriver> + alarm-projection
  logger; log Debug around every formerly-empty catch (probe / shutdown
  / fixed-tree / recycle / alarms-read / projection).
- Driver.FOCAS-008: cache the parsed FocasAddress per tag at
  InitializeAsync; Read/WriteAsync look it up instead of re-parsing on
  every call.
- Driver.FOCAS-009: ProbeLoopAsync now wraps client.ProbeAsync in a
  linked CTS honouring Probe.Timeout so a hung CNC socket can't block
  past the configured limit.
- Driver.FOCAS-010: FocasOperationModeExtensions.ToText delegates to
  FocasOpMode.ToText — single canonical op-mode label surface.
- Driver.FOCAS-011: FocasAlarmType constants are typed short to match
  the cnc_rdalmmsg2 wire field and the projection switch arms.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-23 07:45:38 -04:00
Joseph Doherty f7e3e9885e fix(driver-ablegacy): resolve Low code-review findings (Driver.AbLegacy-005,011,013) 
- Driver.AbLegacy-005: optional ILogger<AbLegacyDriver> ctor parameter,
  logged init failure / probe transitions / first non-zero libplctag
  status per device.
- Driver.AbLegacy-011: Dispose() runs the synchronous teardown directly
  instead of bridging via DisposeAsync().AsTask().GetAwaiter().GetResult()
  to remove the documented sync-over-async deadlock pattern.
- Driver.AbLegacy-013: documented the ResolveHost three-tier fallback
  chain in XML and pointed DiscoverAsync's IsArray=false comment at the
  Modbus ArrayCount pattern for the eventual multi-element follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-23 07:45:31 -04:00
Joseph Doherty 77b8686199 fix(driver-abcip): resolve Low code-review findings (Driver.AbCip-007,011,012,013,015) 
- Driver.AbCip-007: inject an optional ILogger<AbCipDriver> /
  ILogger<AbCipAlarmProjection> (default NullLogger) and log around
  every read / write / template-fetch / probe / alarm-poll failure path.
- Driver.AbCip-011: LogWarning when InitializeAsync is configured with
  Probe.Enabled=true but ProbeTagPath is blank — operators now see why
  GetHostStatuses keeps reporting Unknown.
- Driver.AbCip-012: documented the LibplctagTemplateReader per-call
  Tag cost as accepted given libplctag's own connection pool and the
  low-frequency discovery use-case.
- Driver.AbCip-013: per-device AllowPacking + ConnectionSize overrides
  on AbCipDeviceOptions, threaded through AbCipTagCreateParams; central
  BuildCreateParams helper replaces five ad-hoc clones; AllowPacking
  now reaches Tag.AllowPacking at runtime.
- Driver.AbCip-015: stale-comment sweep — every PR-N forward-reference
  is rewritten to describe present behaviour.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-23 07:45:19 -04:00
Joseph Doherty 9f7ae20995 fix(driver-galaxy): resolve Low code-review findings (Driver.Galaxy-005,010,012,013) 
- Driver.Galaxy-005: rewrite the EventPump BoundedChannelOptions comment
  to honestly describe the Wait+TryWrite pattern.
- Driver.Galaxy-010: ResolveApiKey now warns when a literal API key is
  used in production wiring; added an explicit dev: prefix for known
  cleartext-in-dev cases and rewrote the GalaxyGatewayOptions doc.
- Driver.Galaxy-012: O(1) reverse-lookup for SubscriptionRegistry
  dispatch via per-entry FullRefByItemHandle map; immutable hash-set for
  the cross-binding reverse map; SubscribeAsync / ReadViaSubscribeOnce
  use BuildResultIndex for per-reference correlation.
- Driver.Galaxy-013: ReinitializeAsync now validates the incoming JSON
  against the running options; ReplayOnSessionLost honoured by the
  Replay path; class summary rewritten to describe the shipped surface.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-05-23 07:45:08 -04:00
42 changed files with 2374 additions and 270 deletions
Expand all files Collapse all files
code-reviews/Driver.AbCip/findings.md
+11 -11
View File
@@ -7,7 +7,7 @@
| Review date | 2026-05-22 | | Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` | | Commit reviewed | `76d35d1` |
| Status | Reviewed | | Status | Reviewed |
| Open findings | 5 | | Open findings | 0 |
## Checklist coverage ## Checklist coverage
@@ -123,13 +123,13 @@
| Severity | Low | | Severity | Low |
| Category | OtOpcUa conventions | | Category | OtOpcUa conventions |
| Location | `AbCipDriver.cs` (whole file), `AbCipAlarmProjection.cs`, `LibplctagTagRuntime.cs` | | Location | `AbCipDriver.cs` (whole file), `AbCipAlarmProjection.cs`, `LibplctagTagRuntime.cs` |
| Status | Open | | Status | Resolved |
**Description:** `CLAUDE.md` Library Preferences mandate Serilog with a rolling daily file sink. The driver has no logging at all: no `ILogger`/Serilog dependency is injected or used. Failure paths instead swallow exceptions into the `_health` string (`ReadSingleAsync`, `WriteAsync`, `FetchUdtShapeAsync` catch-all, `ProbeLoopAsync` empty catch, `AbCipAlarmProjection.RunPollLoopAsync` empty catch). An operator looking at server logs sees nothing for a probe loop failing every tick for hours, a template decode that silently returned null, or an alarm poll loop throwing every interval. The health surface carries only the last error message, so a transient error immediately overwrites a more important earlier one. **Description:** `CLAUDE.md` Library Preferences mandate Serilog with a rolling daily file sink. The driver has no logging at all: no `ILogger`/Serilog dependency is injected or used. Failure paths instead swallow exceptions into the `_health` string (`ReadSingleAsync`, `WriteAsync`, `FetchUdtShapeAsync` catch-all, `ProbeLoopAsync` empty catch, `AbCipAlarmProjection.RunPollLoopAsync` empty catch). An operator looking at server logs sees nothing for a probe loop failing every tick for hours, a template decode that silently returned null, or an alarm poll loop throwing every interval. The health surface carries only the last error message, so a transient error immediately overwrites a more important earlier one.
**Recommendation:** Inject an `ILogger` (Serilog) and log at least device init failures, per-call read/write transport errors (debounced), probe-loop failures, template-read failures, and alarm-poll-loop exceptions. The health surface is for state, not for the audit trail. **Recommendation:** Inject an `ILogger` (Serilog) and log at least device init failures, per-call read/write transport errors (debounced), probe-loop failures, template-read failures, and alarm-poll-loop exceptions. The health surface is for state, not for the audit trail.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `AbCipDriver` and `AbCipAlarmProjection` now accept an optional `ILogger<AbCipDriver>` / `ILogger` (defaulting to `NullLogger` so the existing constructor surface stays compatible). Failure paths log through it: `InitializeAsync` (`LogError` on fault), `ReadSingleAsync` / `ReadGroupAsync` / `WriteAsync` (`LogWarning` on non-zero libplctag status + transport / type-conversion exceptions, with the affected tag + device on each entry), `ProbeLoopAsync` (`LogDebug` per swallowed tick), `FetchUdtShapeAsync` (`LogWarning` on template-read failure), and `AbCipAlarmProjection.RunPollLoopAsync` (`LogDebug` on swallowed tick). Six regression tests in `AbCipLoggingTests` exercise the new logger seam.
### Driver.AbCip-008 ### Driver.AbCip-008
@@ -183,13 +183,13 @@
| Severity | Low | | Severity | Low |
| Category | Error handling & resilience | | Category | Error handling & resilience |
| Location | `AbCipDriver.cs:144-152`, `AbCipDriverOptions.cs:131-143` | | Location | `AbCipDriver.cs:144-152`, `AbCipDriverOptions.cs:131-143` |
| Status | Open | | Status | Resolved |
**Description:** `InitializeAsync` only starts probe loops when `_options.Probe.Enabled` is true AND `Probe.ProbeTagPath` is non-blank. When `Probe.Enabled` is true (the default) but `ProbeTagPath` is null (also the default; the doc comment says "PR 8 wires this up"), no probe runs at all and the device `HostState` stays `HostState.Unknown` forever. `GetHostStatuses()` then reports every device as Unknown indefinitely with no warning. An operator who enables the probe but does not set a probe tag gets a silently inert health surface rather than an error or a log line. **Description:** `InitializeAsync` only starts probe loops when `_options.Probe.Enabled` is true AND `Probe.ProbeTagPath` is non-blank. When `Probe.Enabled` is true (the default) but `ProbeTagPath` is null (also the default; the doc comment says "PR 8 wires this up"), no probe runs at all and the device `HostState` stays `HostState.Unknown` forever. `GetHostStatuses()` then reports every device as Unknown indefinitely with no warning. An operator who enables the probe but does not set a probe tag gets a silently inert health surface rather than an error or a log line.
**Recommendation:** When `Probe.Enabled` is true but no `ProbeTagPath` is configured, either fail initialization with a clear message, fall back to a family-default probe tag (the doc comment stated intent), or at minimum log a warning that the probe is enabled-but-inert. **Recommendation:** When `Probe.Enabled` is true but no `ProbeTagPath` is configured, either fail initialization with a clear message, fall back to a family-default probe tag (the doc comment stated intent), or at minimum log a warning that the probe is enabled-but-inert.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `InitializeAsync` now emits a `LogWarning` when `Probe.Enabled` is `true`, devices are configured, but `Probe.ProbeTagPath` is null/blank. The warning names the driver instance and explicitly states that no probe loops were started and `GetHostStatuses()` will report every device as `Unknown` until either a `ProbeTagPath` is set or `Probe.Enabled` is set to `false`. Initialization still succeeds (the probe is optional telemetry, not a hard requirement). Two `AbCipLoggingTests` cases cover the warn-on-enabled-but-blank and no-warn-on-disabled paths. The `AbCipProbeOptions.ProbeTagPath` doc-comment was also updated so the misconfiguration is documented in-place.
### Driver.AbCip-012 ### Driver.AbCip-012
@@ -198,13 +198,13 @@
| Severity | Low | | Severity | Low |
| Category | Performance & resource management | | Category | Performance & resource management |
| Location | `LibplctagTemplateReader.cs:15-35`, `AbCipDriver.cs:88-92` | | Location | `LibplctagTemplateReader.cs:15-35`, `AbCipDriver.cs:88-92` |
| Status | Open | | Status | Resolved |
**Description:** `LibplctagTemplateReader` is created per `FetchUdtShapeAsync` call, and each call constructs a fresh libplctag `Tag` for the @udt pseudo-tag, initializes it (a CIP connection handshake), reads, and disposes it. There is no reuse of the `Tag` across template reads for the same device: every UDT shape fetch pays a full connect/init cost. `AbCipTemplateCache` caches the decoded shape so this only bites on the first fetch of each type, but discovery of a UDT-heavy controller still does one connect per type. The same per-call `Tag` construction applies to `LibplctagTagEnumerator`. **Description:** `LibplctagTemplateReader` is created per `FetchUdtShapeAsync` call, and each call constructs a fresh libplctag `Tag` for the @udt pseudo-tag, initializes it (a CIP connection handshake), reads, and disposes it. There is no reuse of the `Tag` across template reads for the same device: every UDT shape fetch pays a full connect/init cost. `AbCipTemplateCache` caches the decoded shape so this only bites on the first fetch of each type, but discovery of a UDT-heavy controller still does one connect per type. The same per-call `Tag` construction applies to `LibplctagTagEnumerator`.
**Recommendation:** Acceptable for a low-frequency discovery path, but consider pooling/reusing a single @udt-capable `Tag` per device for the duration of a discovery run, or document that the per-type connect cost is accepted. **Recommendation:** Acceptable for a low-frequency discovery path, but consider pooling/reusing a single @udt-capable `Tag` per device for the duration of a discovery run, or document that the per-type connect cost is accepted.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — accepted per the recommendation's "document the per-type connect cost is accepted" branch; `AbCipTemplateCache` caches the decoded shape so only the first fetch per `(device, templateInstanceId)` pays the connect cost, and libplctag itself pools the underlying CIP connections per gateway+path so the TCP/EIP session is reused even when individual `Tag` instances are torn down. The class-level remarks on `LibplctagTemplateReader` now spell that out and call out when to revisit (telemetry showing discovery latency dominated by template-read connects).
### Driver.AbCip-013 ### Driver.AbCip-013
@@ -213,13 +213,13 @@
| Severity | Low | | Severity | Low |
| Category | Design-document adherence | | Category | Design-document adherence |
| Location | `AbCipDriverOptions.cs:70-73`, `PlcFamilies/AbCipPlcFamilyProfile.cs:13-19`, `LibplctagTagRuntime.cs:16-27` | | Location | `AbCipDriverOptions.cs:70-73`, `PlcFamilies/AbCipPlcFamilyProfile.cs:13-19`, `LibplctagTagRuntime.cs:16-27` |
| Status | Open | | Status | Resolved |
**Description:** `driver-specs.md` specifies the AB CIP per-device connection settings as discrete fields: Host, Path, PlcType, TimeoutMs, AllowPacking, ConnectionSize. The implementation instead collapses host + path into a single opaque ab:// URL string and exposes `PlcFamily` (which adds GuardLogix, not in the spec table). AllowPacking and ConnectionSize from the spec are not configurable per device: `AbCipPlcFamilyProfile` hard-codes `SupportsRequestPacking` and `DefaultConnectionSize` per family, and `LibplctagTagRuntime` never passes a connection-size or packing attribute to the `Tag` (it is constructed with only Gateway/Path/PlcType/Protocol/Name/Timeout). The family profile `DefaultConnectionSize`/`SupportsRequestPacking`/`MaxFragmentBytes` fields are computed but never applied to the wire layer: dead configuration. **Description:** `driver-specs.md` specifies the AB CIP per-device connection settings as discrete fields: Host, Path, PlcType, TimeoutMs, AllowPacking, ConnectionSize. The implementation instead collapses host + path into a single opaque ab:// URL string and exposes `PlcFamily` (which adds GuardLogix, not in the spec table). AllowPacking and ConnectionSize from the spec are not configurable per device: `AbCipPlcFamilyProfile` hard-codes `SupportsRequestPacking` and `DefaultConnectionSize` per family, and `LibplctagTagRuntime` never passes a connection-size or packing attribute to the `Tag` (it is constructed with only Gateway/Path/PlcType/Protocol/Name/Timeout). The family profile `DefaultConnectionSize`/`SupportsRequestPacking`/`MaxFragmentBytes` fields are computed but never applied to the wire layer: dead configuration.
**Recommendation:** Either update `driver-specs.md` to describe the actual ab:// host-address model and the family-profile approach, and wire the profile ConnectionSize/packing values through to the libplctag `Tag` attributes; or expose AllowPacking/ConnectionSize as per-device options per the spec. **Recommendation:** Either update `driver-specs.md` to describe the actual ab:// host-address model and the family-profile approach, and wire the profile ConnectionSize/packing values through to the libplctag `Tag` attributes; or expose AllowPacking/ConnectionSize as per-device options per the spec.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — took the "expose per-device options per the spec" branch. `AbCipDeviceOptions` now carries optional `AllowPacking` and `ConnectionSize` overrides (both default to `null` to inherit the family profile); `AbCipTagCreateParams` carries the resolved values; `DeviceState.BuildCreateParams` collapses every old per-call-site clone (read, write, probe, template, enumerator) into one helper that combines the per-device override with the family profile's `SupportsRequestPacking` / `DefaultConnectionSize` defaults. `LibplctagTagRuntime` now honours `AllowPacking` via the `Tag.AllowPacking` property — fixing the previously-dead family-profile setting. `ConnectionSize` is plumbed through `AbCipTagCreateParams` for forward-compat; libplctag.NET 1.5.2 has no direct `ConnectionSize` property, so an XML comment on `LibplctagTagRuntime` documents that current builds rely on the family-profile default at the wire layer until the wrapper exposes a direct property or we ship a custom tag-attribute path. `AbCipDriverFactoryExtensions` ParseOptions now reads `AllowPacking` + `ConnectionSize` from the driver-config JSON. Six regression tests in `AbCipPerDeviceConnectionOptionsTests` cover the new options.
### Driver.AbCip-014 ### Driver.AbCip-014
@@ -243,10 +243,10 @@
| Severity | Low | | Severity | Low |
| Category | Documentation & comments | | Category | Documentation & comments |
| Location | `AbCipDriver.cs:9-11`, `PlcTagHandle.cs:23-27,53-58`, `AbCipTemplateCache.cs:12-15`, `IAbCipTagEnumerator.cs:6-11`, `AbCipDriverOptions.cs:21` | | Location | `AbCipDriver.cs:9-11`, `PlcTagHandle.cs:23-27,53-58`, `AbCipTemplateCache.cs:12-15`, `IAbCipTagEnumerator.cs:6-11`, `AbCipDriverOptions.cs:21` |
| Status | Open | | Status | Resolved |
**Description:** Numerous comments are stale relative to the commit under review. `AbCipDriver.cs:9-11` says the driver "Implements IDriver only for now" with capabilities shipping "in subsequent PRs (3-8)" while the class already implements all of them. `PlcTagHandle.cs` says the plc_tag_destroy P/Invoke "is deferred to PR 3 ... PR 2 ships the lifetime scaffold + tests only" and `ReleaseHandle` "is a no-op", which now reads as a permanent unfinished-work marker (see Driver.AbCip-006). `AbCipTemplateCache.cs:12-15` says "Template shape read ... lands with PR 6 ... no reader writes to it yet" while `CipTemplateObjectDecoder` and `LibplctagTemplateReader` both exist and `FetchUdtShapeAsync` writes to the cache. `IAbCipTagEnumerator.cs:6-11` says the enumerator "Defaults to EmptyAbCipTagEnumeratorFactory" while the production default is `LibplctagTagEnumeratorFactory`. `AbCipDriverOptions.cs:21` says "AB discovery lands in PR 5", already shipped. `StyleGuide.md` explicitly says not to leave stale coming-soon notes. **Description:** Numerous comments are stale relative to the commit under review. `AbCipDriver.cs:9-11` says the driver "Implements IDriver only for now" with capabilities shipping "in subsequent PRs (3-8)" while the class already implements all of them. `PlcTagHandle.cs` says the plc_tag_destroy P/Invoke "is deferred to PR 3 ... PR 2 ships the lifetime scaffold + tests only" and `ReleaseHandle` "is a no-op", which now reads as a permanent unfinished-work marker (see Driver.AbCip-006). `AbCipTemplateCache.cs:12-15` says "Template shape read ... lands with PR 6 ... no reader writes to it yet" while `CipTemplateObjectDecoder` and `LibplctagTemplateReader` both exist and `FetchUdtShapeAsync` writes to the cache. `IAbCipTagEnumerator.cs:6-11` says the enumerator "Defaults to EmptyAbCipTagEnumeratorFactory" while the production default is `LibplctagTagEnumeratorFactory`. `AbCipDriverOptions.cs:21` says "AB discovery lands in PR 5", already shipped. `StyleGuide.md` explicitly says not to leave stale coming-soon notes.
**Recommendation:** Sweep the module for PR-N forward references and "lands in PR X" notes that have been delivered; update them to describe present behavior. Where a comment marks genuinely unfinished work (e.g. `PlcTagHandle.ReleaseHandle`), convert it to a tracked TODO with an issue reference rather than a PR-number milestone. **Recommendation:** Sweep the module for PR-N forward references and "lands in PR X" notes that have been delivered; update them to describe present behavior. Where a comment marks genuinely unfinished work (e.g. `PlcTagHandle.ReleaseHandle`), convert it to a tracked TODO with an issue reference rather than a PR-number milestone.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — swept the module for stale PR-N forward references and replaced each with a description of present behaviour: `AbCipDriver.TemplateCache` summary, `AbCipDataType.cs` (PR 5 / PR 6 → references `CipTemplateObjectDecoder` + `AbCipTemplateCache`), `AbCipTagPath.cs` (PR 6 → references `AbCipTemplateCache`), `AbCipTemplateCache.cs` (the "lands with PR 6" remarks and the `AbCipUdtShape` summary), `IAbCipTagEnumerator.cs` (the `EmptyAbCipTagEnumeratorFactory`-defaults claim and the PR-5 stub line; `EmptyAbCipTagEnumerator` summary), `LibplctagTagEnumerator.cs` ("Task #178 closed the stub gap from PR 5"), `LibplctagTagRuntime.cs` (`Whole-UDT writes land in PR 6`), `AbCipDriverOptions.cs` (`Tags` summary, `ProbeTagPath` summary), and `AbCipPlcFamilyProfile.cs` ("Family-specific wire tests ship in PRs 9–12"). `PlcTagHandle.cs` was already deleted as part of Driver.AbCip-006's resolution. The only remaining "lands in" reference is the `AbCipDataType.Dt` ⇒ `Date/Time` mapping, which is product-domain wording, not a PR reference.
code-reviews/Driver.AbLegacy/findings.md
+38 -7
View File
@@ -7,7 +7,7 @@
| Review date | 2026-05-22 | | Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` | | Commit reviewed | `76d35d1` |
| Status | Reviewed | | Status | Reviewed |
| Open findings | 3 | | Open findings | 0 |
## Checklist coverage ## Checklist coverage
@@ -141,7 +141,7 @@ decode the full 16-bit word and test bit 0.
| Severity | Low | | Severity | Low |
| Category | OtOpcUa conventions | | Category | OtOpcUa conventions |
| Location | `AbLegacyDriver.cs` (whole file) | | Location | `AbLegacyDriver.cs` (whole file) |
| Status | Open | | Status | Resolved |
**Description:** The driver uses no `ILogger`/Serilog at all. Probe-loop failures, **Description:** The driver uses no `ILogger`/Serilog at all. Probe-loop failures,
runtime initialisation failures, libplctag non-zero statuses, and read/write runtime initialisation failures, libplctag non-zero statuses, and read/write
@@ -155,7 +155,16 @@ string that the next read or write immediately clobbers.
log probe transitions, runtime-init failures, and the first occurrence of a non-zero log probe transitions, runtime-init failures, and the first occurrence of a non-zero
libplctag status per device. libplctag status per device.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `AbLegacyDriver` now accepts an optional
`ILogger<AbLegacyDriver>` (falls back to `NullLogger`), mirroring the Modbus / S7 /
Galaxy driver pattern. `InitializeAsync` catch-path logs the init failure at Error
level; `TransitionDeviceState` logs every probe transition (Warning on downgrade to
Stopped, Information on recovery); `ReadAsync` logs the first non-zero libplctag
status per device at Warning level via a re-armable `DeviceState.FirstNonZeroStatusLogged`
latch so a permanently-bad PLC doesn't flood the rolling file. `AbLegacyDriverFactoryExtensions.Register`
gains an optional `ILoggerFactory` parameter so the Server bootstrap can wire DI
logging when it chooses; the legacy single-arg `CreateInstance` overload stays for
back-compat. Regression coverage in `AbLegacyLoggerInjectionTests`.
### Driver.AbLegacy-006 ### Driver.AbLegacy-006
@@ -293,7 +302,7 @@ into a real PCCC-STS path or delete it as dead code. The same defect exists in
| Severity | Low | | Severity | Low |
| Category | Performance & resource management | | Category | Performance & resource management |
| Location | `AbLegacyDriver.cs:440` | | Location | `AbLegacyDriver.cs:440` |
| Status | Open | | Status | Resolved |
**Description:** `Dispose()` is implemented as **Description:** `Dispose()` is implemented as
`DisposeAsync().AsTask().GetAwaiter().GetResult()` - sync-over-async. `ShutdownAsync` `DisposeAsync().AsTask().GetAwaiter().GetResult()` - sync-over-async. `ShutdownAsync`
@@ -306,7 +315,16 @@ single-threaded synchronization context.
must exist, perform the synchronous teardown directly (cancel CTSs, dispose runtimes) must exist, perform the synchronous teardown directly (cancel CTSs, dispose runtimes)
rather than blocking on the async path. rather than blocking on the async path.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `Dispose()` now performs the synchronous
teardown directly (cancel probe CTSs, dispose runtimes, clear maps) rather than
wrapping `DisposeAsync().AsTask().GetAwaiter().GetResult()`. The poll engine's
`DisposeAsync` is drained with `.ConfigureAwait(false).GetAwaiter().GetResult()` so a
captured single-threaded `SynchronizationContext` can never be the resumption target —
the classic sync-over-async deadlock is structurally ruled out. Regression test
`Dispose_under_single_threaded_sync_context_does_not_deadlock` drives the path
through a cooperative single-threaded `SynchronizationContext` with a 2s pump timeout;
`Dispose_runs_teardown_without_blocking_on_async_wait` and `Dispose_is_idempotent`
cover the cleanup invariants.
### Driver.AbLegacy-012 ### Driver.AbLegacy-012
@@ -345,7 +363,7 @@ unused fields and the doc comments that imply they are load-bearing.
| Severity | Low | | Severity | Low |
| Category | Code organization & conventions | | Category | Code organization & conventions |
| Location | `AbLegacyDriver.cs:340-345`, `AbLegacyDriver.cs:238-264` | | Location | `AbLegacyDriver.cs:340-345`, `AbLegacyDriver.cs:238-264` |
| Status | Open | | Status | Resolved |
**Description:** Two minor organisational issues: **Description:** Two minor organisational issues:
1. `ResolveHost` returns `_options.Devices.FirstOrDefault()?.HostAddress ?? 1. `ResolveHost` returns `_options.Devices.FirstOrDefault()?.HostAddress ??
@@ -362,4 +380,17 @@ unused fields and the doc comments that imply they are load-bearing.
document why falling back to the instance id is acceptable. For (2), record the document why falling back to the instance id is acceptable. For (2), record the
array-addressing gap as a tracked follow-up. array-addressing gap as a tracked follow-up.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 —
(1) `ResolveHost` carries a new XML-doc block that documents the three-step fallback
chain (known tag → first device → `DriverInstanceId`) and explicitly cites the
`IPerCallHostResolver` contract which requires implementations to return the driver's
default-host string rather than throw on an unknown reference. The instance-id
fallback is therefore the documented single-host behaviour, not a leaky fake. Three
regression tests in `AbLegacyDisposeAndResolveHostTests` pin each branch of the chain
(`ResolveHost_known_reference_returns_tag_device`,
`ResolveHost_unknown_reference_with_devices_returns_first_device`,
`ResolveHost_unknown_reference_no_devices_returns_driver_instance_id`).
(2) `DiscoverAsync` now carries an inline tracked-follow-up comment that calls out
the PCCC-file-as-array gap, notes the consistency with the PR-staged scope in
`docs/v2/driver-specs.md`, and points to the Modbus `ArrayCount` flow as the pattern
to mirror when multi-element addressing lands.
code-reviews/Driver.FOCAS/findings.md
+11 -11
View File
@@ -7,7 +7,7 @@
| Review date | 2026-05-22 | | Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` | | Commit reviewed | `76d35d1` |
| Status | Reviewed | | Status | Reviewed |
| Open findings | 5 | | Open findings | 0 |
## Checklist coverage ## Checklist coverage
@@ -200,7 +200,7 @@ stale object.
| Severity | Low | | Severity | Low |
| Category | Error handling & resilience | | Category | Error handling & resilience |
| Location | `FocasDriver.cs:140-148`, `FocasDriver.cs:478-484`, `FocasDriver.cs:529-533`, `FocasAlarmProjection.cs:61-63` | | Location | `FocasDriver.cs:140-148`, `FocasDriver.cs:478-484`, `FocasDriver.cs:529-533`, `FocasAlarmProjection.cs:61-63` |
| Status | Open | | Status | Resolved |
**Description:** Numerous `try { ... } catch {}` blocks swallow every exception with no **Description:** Numerous `try { ... } catch {}` blocks swallow every exception with no
logging - `ShutdownAsync` (CTS cancel/dispose), `RecycleLoopAsync` (`DisposeClient`), logging - `ShutdownAsync` (CTS cancel/dispose), `RecycleLoopAsync` (`DisposeClient`),
@@ -215,7 +215,7 @@ solely on `GetHealth()`.
poll/probe/recycle loops at `Debug`/`Warning`. Pass a logger into `FocasWireClient` so poll/probe/recycle loops at `Debug`/`Warning`. Pass a logger into `FocasWireClient` so
the per-response `Debug` entries it already emits are actually captured. the per-response `Debug` entries it already emits are actually captured.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `FocasDriver` now takes an optional `ILogger<FocasDriver>` (defaulting to `NullLogger`) and every previously-empty `catch { }` in `ShutdownAsync` / `ProbeLoopAsync` / `FixedTreeLoopAsync` / `RecycleLoopAsync` / `ReadActiveAlarmsAcrossDevicesAsync` now logs at `Debug` with the host address + context. `FocasAlarmProjection` also accepts an optional `ILogger` (forwarded by the driver) so its unsubscribe / dispose / per-tick poll swallows log. `WireFocasClientFactory` gained a logger-accepting overload that threads through to `FocasWireClient`, so its per-response `Debug` entries actually reach the host pipeline.
### Driver.FOCAS-008 ### Driver.FOCAS-008
@@ -224,7 +224,7 @@ the per-response `Debug` entries it already emits are actually captured.
| Severity | Low | | Severity | Low |
| Category | Performance & resource management | | Category | Performance & resource management |
| Location | `FocasDriver.cs:201`, `FocasDriver.cs:253` | | Location | `FocasDriver.cs:201`, `FocasDriver.cs:253` |
| Status | Open | | Status | Resolved |
**Description:** `ReadAsync` and `WriteAsync` call `FocasAddress.TryParse(def.Address)` **Description:** `ReadAsync` and `WriteAsync` call `FocasAddress.TryParse(def.Address)`
on every operation, even though `InitializeAsync` already parsed and validated every on every operation, even though `InitializeAsync` already parsed and validated every
@@ -235,7 +235,7 @@ re-parses and allocates a `FocasAddress` record per tag per tick unnecessarily.
parsed `FocasAddress` on `FocasTagDefinition` (or in a side dictionary), so the runtime parsed `FocasAddress` on `FocasTagDefinition` (or in a side dictionary), so the runtime
read/write paths use the cached value. read/write paths use the cached value.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `FocasDriver` now holds a `_parsedAddressesByTagName` side dictionary populated at `InitializeAsync`. `ReadAsync` and `WriteAsync` look up the cached `FocasAddress` instance; the defensive fallback `TryParse` only fires if a tag was somehow not seeded. The cache is cleared on `ShutdownAsync`. Regression test `ReadAsync_uses_cached_FocasAddress_when_tag_definition_has_a_malformed_address_after_init` (and the matching `WriteAsync` variant) asserts the same `FocasAddress` instance is reused across calls.
### Driver.FOCAS-009 ### Driver.FOCAS-009
@@ -244,7 +244,7 @@ read/write paths use the cached value.
| Severity | Low | | Severity | Low |
| Category | Design-document adherence | | Category | Design-document adherence |
| Location | `FocasDriverOptions.cs:110-115`, `FocasDriver.cs:468-486`, `FocasDriverFactoryExtensions.cs:75-80` | | Location | `FocasDriverOptions.cs:110-115`, `FocasDriver.cs:468-486`, `FocasDriverFactoryExtensions.cs:75-80` |
| Status | Open | | Status | Resolved |
**Description:** `FocasProbeOptions.Timeout` is parsed by the factory **Description:** `FocasProbeOptions.Timeout` is parsed by the factory
(`FocasProbeDto.TimeoutMs` to `FocasProbeOptions.Timeout`) but never consumed. (`FocasProbeDto.TimeoutMs` to `FocasProbeOptions.Timeout`) but never consumed.
@@ -257,7 +257,7 @@ until the OS TCP timeout rather than the configured `Probe.Timeout`.
around the `ProbeAsync` call, or remove the dead `Timeout` field from around the `ProbeAsync` call, or remove the dead `Timeout` field from
`FocasProbeOptions` / `FocasProbeDto` if it is genuinely not intended. `FocasProbeOptions` / `FocasProbeDto` if it is genuinely not intended.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `FocasDriver.ProbeLoopAsync` now wraps `client.ProbeAsync` in a linked `CancellationTokenSource` that fires after `Probe.Timeout` (skipped when the timeout is `<= TimeSpan.Zero`). On timeout the loop logs the cancellation at Debug and surfaces it as a failed probe, so a hung CNC socket transitions the host to `Stopped` at the configured budget instead of blocking on the OS TCP timeout. Regression test `ProbeLoop_cancels_a_slow_ProbeAsync_at_Probe_Timeout` asserts the cancellation reaches the fake `ProbeAsync` within the configured 100 ms.
### Driver.FOCAS-010 ### Driver.FOCAS-010
@@ -266,7 +266,7 @@ around the `ProbeAsync` call, or remove the dead `Timeout` field from
| Severity | Low | | Severity | Low |
| Category | Code organization & conventions | | Category | Code organization & conventions |
| Location | `IFocasClient.cs:210-227` (`FocasOpMode`), `FocasConstants.cs:42-78` (`FocasOperationMode`) | | Location | `IFocasClient.cs:210-227` (`FocasOpMode`), `FocasConstants.cs:42-78` (`FocasOperationMode`) |
| Status | Open | | Status | Resolved |
**Description:** There are two parallel operation-mode-to-text mappings with divergent **Description:** There are two parallel operation-mode-to-text mappings with divergent
labels. `FocasOpMode.ToText` (used by the driver fixed-tree `OperationMode/ModeText` labels. `FocasOpMode.ToText` (used by the driver fixed-tree `OperationMode/ModeText`
@@ -278,7 +278,7 @@ inconsistent results depending on which path renders it.
**Recommendation:** Consolidate to a single op-mode enum + `ToText` helper shared by **Recommendation:** Consolidate to a single op-mode enum + `ToText` helper shared by
both the wire layer and the driver projection, with one canonical label set. both the wire layer and the driver projection, with one canonical label set.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `FocasOperationModeExtensions.ToText` now delegates to `FocasOpMode.ToText((short)mode)`, so the wire layer and the driver fixed-tree projection render identical labels. `FocasOpMode` keeps its existing labels (`TJOG`, `TEACH_IN_HANDLE`, `Mode{n}` fallback), which are now the single canonical surface. Regression theory `OpMode_ToText_yields_the_same_label_in_both_namespaces` cross-checks every defined code; `OpMode_ToText_fallback_label_is_consistent` covers the unknown-code path.
### Driver.FOCAS-011 ### Driver.FOCAS-011
@@ -287,7 +287,7 @@ both the wire layer and the driver projection, with one canonical label set.
| Severity | Low | | Severity | Low |
| Category | Code organization & conventions | | Category | Code organization & conventions |
| Location | `IFocasClient.cs:275-287` (`FocasAlarmType`), `FocasAlarmProjection.cs:149-175` | | Location | `IFocasClient.cs:275-287` (`FocasAlarmType`), `FocasAlarmProjection.cs:149-175` |
| Status | Open | | Status | Resolved |
**Description:** `FocasAlarmType` declares its constants as `public const int`, but the **Description:** `FocasAlarmType` declares its constants as `public const int`, but the
only consumers - `FocasAlarmProjection.MapAlarmType(short type)` and only consumers - `FocasAlarmProjection.MapAlarmType(short type)` and
@@ -301,7 +301,7 @@ expected by `ReadAlarmsAsync`.
**Recommendation:** Declare the `FocasAlarmType` constants as `short` (or make it an **Recommendation:** Declare the `FocasAlarmType` constants as `short` (or make it an
`enum : short`) so the type matches the wire field width and the projection signatures. `enum : short`) so the type matches the wire field width and the projection signatures.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — every `FocasAlarmType` constant (`All`, `Parameter`, `PulseCode`, `Overtravel`, `Overheat`, `Servo`, `DataIo`, `MemoryCheck`, `MacroAlarm`) is now typed `short`, matching the wire field width on `cnc_rdalmmsg2` and the `switch (short type)` arms in `FocasAlarmProjection.MapAlarmType` / `MapSeverity`. Regression test `FocasAlarmType_constants_are_typed_short` uses reflection to guarantee the type is preserved against future drift.
### Driver.FOCAS-012 ### Driver.FOCAS-012
code-reviews/Driver.Galaxy/findings.md
+9 -9
View File
@@ -7,7 +7,7 @@
| Review date | 2026-05-22 | | Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` | | Commit reviewed | `76d35d1` |
| Status | Reviewed | | Status | Reviewed |
| Open findings | 4 | | Open findings | 0 |
## Checklist coverage ## Checklist coverage
@@ -93,13 +93,13 @@
| Severity | Low | | Severity | Low |
| Category | OtOpcUa conventions | | Category | OtOpcUa conventions |
| Location | `Runtime/EventPump.cs:81-88` | | Location | `Runtime/EventPump.cs:81-88` |
| Status | Open | | Status | Resolved |
**Description:** The `BoundedChannelOptions` comment states "Newest-dropped policy: when full, the producer's TryWrite returns false ... We do this manually rather than relying on `BoundedChannelFullMode.DropWrite`" — but the option is then set to `FullMode = BoundedChannelFullMode.Wait`. With `Wait`, `TryWrite` returning `false` on a full channel is correct behaviour, so the code works, but the comment naming the mode and the actual mode disagree, which is confusing for a maintainer deciding whether the policy is `Wait`, `DropWrite`, or `DropNewest`. **Description:** The `BoundedChannelOptions` comment states "Newest-dropped policy: when full, the producer's TryWrite returns false ... We do this manually rather than relying on `BoundedChannelFullMode.DropWrite`" — but the option is then set to `FullMode = BoundedChannelFullMode.Wait`. With `Wait`, `TryWrite` returning `false` on a full channel is correct behaviour, so the code works, but the comment naming the mode and the actual mode disagree, which is confusing for a maintainer deciding whether the policy is `Wait`, `DropWrite`, or `DropNewest`.
**Recommendation:** Either reword the comment to say "we use `Wait` mode but never call the awaitable `WriteAsync` — `TryWrite` gives us synchronous newest-dropped semantics", or switch to `BoundedChannelFullMode.DropWrite` and keep the manual drop count. Make the comment and the mode consistent. **Recommendation:** Either reword the comment to say "we use `Wait` mode but never call the awaitable `WriteAsync` — `TryWrite` gives us synchronous newest-dropped semantics", or switch to `BoundedChannelFullMode.DropWrite` and keep the manual drop count. Make the comment and the mode consistent.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — reworded the `BoundedChannelOptions` comment to say "we use FullMode.Wait but never call the awaitable WriteAsync — only synchronous TryWrite, which returns false immediately on a full channel and lets us account for drops on the EventsDropped counter". Also explains why we deliberately do NOT use `BoundedChannelFullMode.DropWrite` (it would silently discard without surfacing on the counter). Comment and `FullMode` value now agree.
### Driver.Galaxy-006 ### Driver.Galaxy-006
@@ -168,13 +168,13 @@
| Severity | Low | | Severity | Low |
| Category | Security | | Category | Security |
| Location | `GalaxyDriver.cs:311-341` | | Location | `GalaxyDriver.cs:311-341` |
| Status | Open | | Status | Resolved |
**Description:** `ResolveApiKey` supports an `env:`/`file:` indirection and otherwise treats the config string as the literal API key ("Anything else — used as the literal API key. Convenient for dev"). `GalaxyGatewayOptions`' own XML doc claims "the API key never appears in cleartext config". The literal-key fallback silently permits a plaintext API key in the `DriverConfig` JSON column of the central config DB, contradicting the documented contract. There is no warning logged when the literal path is taken. **Description:** `ResolveApiKey` supports an `env:`/`file:` indirection and otherwise treats the config string as the literal API key ("Anything else — used as the literal API key. Convenient for dev"). `GalaxyGatewayOptions`' own XML doc claims "the API key never appears in cleartext config". The literal-key fallback silently permits a plaintext API key in the `DriverConfig` JSON column of the central config DB, contradicting the documented contract. There is no warning logged when the literal path is taken.
**Recommendation:** Log a startup warning when `ResolveApiKey` falls through to the literal arm so an operator who accidentally committed a cleartext key sees it, and update the `GalaxyGatewayOptions` doc comment so it no longer over-promises. Consider gating the literal arm behind an explicit `dev:`-style prefix so a cleartext key cannot be used by accident. **Recommendation:** Log a startup warning when `ResolveApiKey` falls through to the literal arm so an operator who accidentally committed a cleartext key sees it, and update the `GalaxyGatewayOptions` doc comment so it no longer over-promises. Consider gating the literal arm behind an explicit `dev:`-style prefix so a cleartext key cannot be used by accident.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — (a) added a logger-aware `ResolveApiKey(string, ILogger?)` overload that emits a `Warning` when the back-compat literal arm is taken, and wired the `BuildClientOptions` call site to pass `_logger`; (b) added an explicit `dev:KEY` prefix that returns the literal value without warning, so dev rigs / parity tests can opt-in deliberately; (c) rewrote the `GalaxyGatewayOptions.ApiKeySecretRef` XML doc so it no longer claims "the API key never appears in cleartext config" — it now documents all four supported forms (`env:`, `file:`, `dev:`, and the warning-on-literal back-compat path). Regression coverage in `GalaxyDriverApiKeyResolverTests` (`Literal_string_emits_warning_when_logger_supplied`, `Dev_prefix_returns_literal_without_warning`, `Env_prefix_does_not_emit_literal_warning`).
### Driver.Galaxy-011 ### Driver.Galaxy-011
@@ -198,13 +198,13 @@
| Severity | Low | | Severity | Low |
| Category | Performance & resource management | | Category | Performance & resource management |
| Location | `Runtime/SubscriptionRegistry.cs:65-67`, `GalaxyDriver.cs:538`, `GalaxyDriver.cs:675` | | Location | `Runtime/SubscriptionRegistry.cs:65-67`, `GalaxyDriver.cs:538`, `GalaxyDriver.cs:675` |
| Status | Open | | Status | Resolved |
**Description:** Several hot paths are O(n^2) per call. `SubscriptionRegistry.ResolveSubscribers` does `entry.Bindings.FirstOrDefault(b => b.ItemHandle == itemHandle)` — a linear scan of the whole binding list for every event dispatch; at 50k tags this is 50k-element scans on the 1Hz fan-out path. `GalaxyDriver.SubscribeAsync` and `ReadViaSubscribeOnceAsync` correlate results to references with `results.FirstOrDefault(r => string.Equals(...))` inside a `for` loop over all references — O(n^2) over the subscribe batch. `SubscriptionRegistry.Remove` rebuilds a `ConcurrentBag` from a LINQ filter on every unsubscribe. **Description:** Several hot paths are O(n^2) per call. `SubscriptionRegistry.ResolveSubscribers` does `entry.Bindings.FirstOrDefault(b => b.ItemHandle == itemHandle)` — a linear scan of the whole binding list for every event dispatch; at 50k tags this is 50k-element scans on the 1Hz fan-out path. `GalaxyDriver.SubscribeAsync` and `ReadViaSubscribeOnceAsync` correlate results to references with `results.FirstOrDefault(r => string.Equals(...))` inside a `for` loop over all references — O(n^2) over the subscribe batch. `SubscriptionRegistry.Remove` rebuilds a `ConcurrentBag` from a LINQ filter on every unsubscribe.
**Recommendation:** Index `SubscriptionEntry` bindings by item handle (a `Dictionary<int, string>` per entry) so `ResolveSubscribers` is O(1) per subscriber. Project the `SubscribeResult` list into a `Dictionary<string, SubscribeResult>` (OrdinalIgnoreCase) once before the correlation loop. These matter on the documented 50k-tag soak path. **Recommendation:** Index `SubscriptionEntry` bindings by item handle (a `Dictionary<int, string>` per entry) so `ResolveSubscribers` is O(1) per subscriber. Project the `SubscribeResult` list into a `Dictionary<string, SubscribeResult>` (OrdinalIgnoreCase) once before the correlation loop. These matter on the documented 50k-tag soak path.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — three changes: (a) `SubscriptionEntry` now carries a `FullRefByItemHandle` `Dictionary<int, string>` built once at construction; `ResolveSubscribers` does O(1) lookups per subscriber instead of a `FirstOrDefault` linear scan of the binding list. (b) Reverse map `_subscribersByItemHandle` swapped from `ConcurrentBag<long>` to `ImmutableHashSet<long>` — `Remove`/`Rebind` use `set.Remove(id)` (O(log n)) instead of "rebuild a new bag from a LINQ filter on every unsubscribe", and reads remain lock-free via atomic publication through `ConcurrentDictionary.AddOrUpdate`. (c) `GalaxyDriver.SubscribeAsync` + `ReadViaSubscribeOnceAsync` now index the `SubscribeResult` list once via the existing `BuildResultIndex` helper (already used by `ReplayAsync`) so per-reference correlation is O(1). Regression coverage in `SubscriptionRegistryTests.ResolveSubscribers_LargeBindingSet_DispatchesCorrectly`.
### Driver.Galaxy-013 ### Driver.Galaxy-013
@@ -213,13 +213,13 @@
| Severity | Low | | Severity | Low |
| Category | Design-document adherence | | Category | Design-document adherence |
| Location | `GalaxyDriver.cs:14-27`, `GalaxyDriver.cs:374-382`, `Config/GalaxyDriverOptions.cs:84-86` | | Location | `GalaxyDriver.cs:14-27`, `GalaxyDriver.cs:374-382`, `Config/GalaxyDriverOptions.cs:84-86` |
| Status | Open | | Status | Resolved |
**Description:** Multiple doc comments are stale relative to the shipped code. `GalaxyDriver`'s class summary still describes the file as "the project skeleton with `IDriver` bodies that wire to a future `IGalaxyGatewayClient` abstraction. Capability interfaces ... land in PRs 4.1-4.7" and references the legacy `GalaxyProxyDriver` coexisting "until PR 7.2" — but PR 7.2 already deleted the legacy Galaxy projects and the capability interfaces are all implemented. `ReinitializeAsync` is still a stub ("for the skeleton we just refresh health") that ignores `driverConfigJson` entirely — a config reapply silently does nothing. `GalaxyReconnectOptions.ReplayOnSessionLost` is defined and documented but never read anywhere in the driver (`ReplayAsync` always replays). **Description:** Multiple doc comments are stale relative to the shipped code. `GalaxyDriver`'s class summary still describes the file as "the project skeleton with `IDriver` bodies that wire to a future `IGalaxyGatewayClient` abstraction. Capability interfaces ... land in PRs 4.1-4.7" and references the legacy `GalaxyProxyDriver` coexisting "until PR 7.2" — but PR 7.2 already deleted the legacy Galaxy projects and the capability interfaces are all implemented. `ReinitializeAsync` is still a stub ("for the skeleton we just refresh health") that ignores `driverConfigJson` entirely — a config reapply silently does nothing. `GalaxyReconnectOptions.ReplayOnSessionLost` is defined and documented but never read anywhere in the driver (`ReplayAsync` always replays).
**Recommendation:** Refresh the `GalaxyDriver` class and `ReinitializeAsync` doc comments to describe the shipped state, implement or explicitly reject `ReinitializeAsync` config reapply, and either honour `ReplayOnSessionLost` or remove it from `GalaxyReconnectOptions`. **Recommendation:** Refresh the `GalaxyDriver` class and `ReinitializeAsync` doc comments to describe the shipped state, implement or explicitly reject `ReinitializeAsync` config reapply, and either honour `ReplayOnSessionLost` or remove it from `GalaxyReconnectOptions`.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — three fixes: (a) rewrote the `GalaxyDriver` class summary to describe the shipped capability surface (`ITagDiscovery`, `IReadable`, `IWritable`, `ISubscribable`, `IRediscoverable`, `IHostConnectivityProbe`, `IAlarmSource`) and removed the stale "PR 4.0 skeleton" / "legacy `GalaxyProxyDriver` coexists until PR 7.2" wording — PR 7.2 already retired the legacy projects. (b) `ReinitializeAsync` now parses the incoming `driverConfigJson` through the factory pipeline and compares the result to `_options`; an equivalent reapply refreshes health, a non-equivalent change throws `NotSupportedException` so a config swap never silently no-ops. (c) `ReplayAsync` now honours `_options.Reconnect.ReplayOnSessionLost` — when false it restarts the EventPump but skips the per-tag SubscribeBulk fan-out, delegating to gateway session-level replay. Regression coverage in `GalaxyDriverInfrastructureTests` (`ReinitializeAsync_RejectsNonEquivalentConfigChange`, `ReinitializeAsync_AcceptsEquivalentConfig`, `ReplayOnSessionLost_False_SkipsResubscribeBulk`, `ReplayOnSessionLost_True_RunsResubscribeBulk`). Updated `GalaxyDriverFactoryTests.ReinitializeAsync_RefreshesHealth_WhenConfigIsEquivalent` to use an equivalent config JSON.
### Driver.Galaxy-014 ### Driver.Galaxy-014
code-reviews/Driver.S7/findings.md
+55 -11
View File
@@ -7,7 +7,7 @@
| Review date | 2026-05-22 | | Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` | | Commit reviewed | `76d35d1` |
| Status | Reviewed | | Status | Reviewed |
| Open findings | 5 | | Open findings | 0 |
## Checklist coverage ## Checklist coverage
@@ -89,7 +89,7 @@ correct the comment so the lossiness of UInt32 is documented.
| Severity | Low | | Severity | Low |
| Category | Correctness & logic bugs | | Category | Correctness & logic bugs |
| Location | `S7Driver.cs:172`, `S7Driver.cs:255` | | Location | `S7Driver.cs:172`, `S7Driver.cs:255` |
| Status | Open | | Status | Resolved |
**Description:** ReadAsync and WriteAsync dereference fullReferences.Count / **Description:** ReadAsync and WriteAsync dereference fullReferences.Count /
writes.Count with no null guard. A null argument throws NullReferenceException writes.Count with no null guard. A null argument throws NullReferenceException
@@ -101,7 +101,13 @@ inconsistent with it.
**Recommendation:** Add ArgumentNullException.ThrowIfNull for the list parameters **Recommendation:** Add ArgumentNullException.ThrowIfNull for the list parameters
at the top of ReadAsync and WriteAsync. at the top of ReadAsync and WriteAsync.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — added `ArgumentNullException.ThrowIfNull`
at the top of both `ReadAsync` and `WriteAsync`, placed BEFORE `RequirePlc()` so
a null argument produces a typed `ArgumentNullException` (consistent with
`DiscoverAsync`) rather than either an NRE on `.Count` or the "not initialized"
`InvalidOperationException` from `RequirePlc`. Regression tests
`ReadAsync_with_null_fullReferences_throws_ArgumentNullException` and
`WriteAsync_with_null_writes_throws_ArgumentNullException`.
### Driver.S7-004 ### Driver.S7-004
@@ -133,7 +139,7 @@ and swallowed poll-loop / shutdown exceptions.
| Severity | Low | | Severity | Low |
| Category | OtOpcUa conventions | | Category | OtOpcUa conventions |
| Location | `S7Driver.cs:33`, `S7Driver.cs:433` | | Location | `S7Driver.cs:33`, `S7Driver.cs:433` |
| Status | Open | | Status | Resolved |
**Description:** System.Collections.Concurrent.ConcurrentDictionary is written **Description:** System.Collections.Concurrent.ConcurrentDictionary is written
out with a fully-qualified namespace at the field declarations instead of a out with a fully-qualified namespace at the field declarations instead of a
@@ -145,7 +151,11 @@ S7Driver.cs despite the file-top using S7.Net.
**Recommendation:** Add using System.Collections.Concurrent and drop the **Recommendation:** Add using System.Collections.Concurrent and drop the
redundant global::S7.Net. qualifiers where using S7.Net already covers them. redundant global::S7.Net. qualifiers where using S7.Net already covers them.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `using System.Collections.Concurrent` was
already added by an earlier finding fix; this resolution removes the remaining
`global::S7.Net.Plc` qualifiers from the `ReadOneAsync` and `WriteOneAsync`
signatures, now using the unqualified `Plc` type (the file-top `using S7.Net`
already covers it). House style restored.
### Driver.S7-006 ### Driver.S7-006
@@ -250,7 +260,7 @@ status, and update _health to Degraded on transport failures.
| Severity | Low | | Severity | Low |
| Category | Error handling & resilience | | Category | Error handling & resilience |
| Location | `S7Driver.cs:392` | | Location | `S7Driver.cs:392` |
| Status | Open | | Status | Resolved |
**Description:** The subscription poll loop never reflects sustained polling **Description:** The subscription poll loop never reflects sustained polling
failure anywhere an operator can see it. PollLoopAsync swallows every failure anywhere an operator can see it. PollLoopAsync swallows every
@@ -266,7 +276,19 @@ Interval indefinitely on a hard failure.
apply a capped backoff after consecutive errors; at minimum log the swallowed apply a capped backoff after consecutive errors; at minimum log the swallowed
exception (see Driver.S7-004). exception (see Driver.S7-004).
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `PollLoopAsync` now tracks
`consecutiveFailures`, calls new `HandlePollFailure` which both logs (with the
failure count) AND degrades `_health` to `Degraded` once
`PollFailureHealthThreshold` (1) consecutive failures have accumulated, and
applies a capped exponential backoff via new `ComputeBackoffDelay` (doubles the
wait each consecutive failure up to a 30 s `PollBackoffCap`). A healthy tick
resets the counter so the cadence snaps back to the configured Interval.
`HandlePollFailure` refuses to downgrade a `Faulted` state (reserved for
permanent config faults like PUT/GET-denied). Regression test
`PollLoop_against_uninitialized_driver_degrades_health` proves the health
surface now reflects sustained failure; `PollLoop_applies_capped_backoff_after_consecutive_failures`
proves shutdown still completes inside the drain window even under a fault
storm.
### Driver.S7-010 ### Driver.S7-010
@@ -275,7 +297,7 @@ exception (see Driver.S7-004).
| Severity | Low | | Severity | Low |
| Category | Performance & resource management | | Category | Performance & resource management |
| Location | `S7Driver.cs:504` | | Location | `S7Driver.cs:504` |
| Status | Open | | Status | Resolved |
**Description:** Dispose() is implemented as **Description:** Dispose() is implemented as
DisposeAsync().AsTask().GetAwaiter().GetResult() - sync-over-async. Inside the DisposeAsync().AsTask().GetAwaiter().GetResult() - sync-over-async. Inside the
@@ -288,7 +310,16 @@ blocking wrap is unnecessary risk.
perform the teardown directly (cancel CTSs, close Plc, dispose _gate) without perform the teardown directly (cancel CTSs, close Plc, dispose _gate) without
round-tripping through the async path. round-tripping through the async path.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `Dispose()` now performs teardown
directly via a new private `SynchronousTeardown` method that mirrors
`ShutdownAsync` but uses `Task.WhenAll(...).Wait(DrainTimeout)` instead of
`await Task.WhenAll(...).WaitAsync(...)`. Probe + poll Tasks are still drained
with the bounded 5 s timeout (so a wedged loop cannot hang `Dispose` indefinitely),
but the sync path no longer round-trips through `DisposeAsync().AsTask().GetAwaiter().GetResult()`.
`DisposeAsync` keeps its existing implementation for callers that opt into the
async dispose pattern. Regression tests
`Dispose_completes_synchronously_without_sync_over_async_round_trip` and
`Dispose_is_idempotent`.
### Driver.S7-011 ### Driver.S7-011
@@ -358,7 +389,7 @@ ReadStatusAsync-based probe.
| Severity | Low | | Severity | Low |
| Category | Code organization & conventions | | Category | Code organization & conventions |
| Location | `S7DriverOptions.cs:90`, `S7Driver.cs:300` | | Location | `S7DriverOptions.cs:90`, `S7Driver.cs:300` |
| Status | Open | | Status | Resolved |
**Description:** S7TagDefinition.StringLength is a public configured/JSON-bound **Description:** S7TagDefinition.StringLength is a public configured/JSON-bound
parameter (default 254) but is dead: S7DataType.String reads and writes both parameter (default 254) but is dead: S7DataType.String reads and writes both
@@ -376,7 +407,20 @@ StringLength) at InitializeAsync / factory validation with a clear "not yet
supported" error, so a partially-implemented type cannot be configured into a supported" error, so a partially-implemented type cannot be configured into a
live address space. live address space.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-23 — `InitializeAsync` now runs new
`RejectUnsupportedTagDataTypes`, which throws `NotSupportedException` for any
tag whose `DataType` is in the `UnimplementedDataTypes` set (`Int64`, `UInt64`,
`Float64`, `String`, `DateTime`). The half-implemented types can no longer leak
into the live address space — a site that configures one fails fast at init
rather than seeing a node that returns `BadNotSupported` on every access.
Entries should be removed from `UnimplementedDataTypes` as each type is wired
through; the comment on `RejectUnsupportedTagDataTypes` makes it a single grep
target for that follow-up. `StringLength` remains in `S7TagDefinition` because
removing it would be a breaking change to existing config JSON; once `String`
is implemented it will be consumed without further config changes. Regression
tests `Initialize_rejects_not_yet_implemented_data_type_with_NotSupportedException`
(Theory, 5 types) and `Initialize_accepts_implemented_data_types` (Theory, 7
types) prove the guard is targeted.
### Driver.S7-014 ### Driver.S7-014
code-reviews/README.md
+27 -27
View File
@@ -22,21 +22,21 @@ Each module's `findings.md` is the source of truth; this file is generated from
| [Core.ScriptedAlarms](Core.ScriptedAlarms/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | | [Core.ScriptedAlarms](Core.ScriptedAlarms/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
| [Core.Scripting](Core.Scripting/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 | | [Core.Scripting](Core.Scripting/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 |
| [Core.VirtualTags](Core.VirtualTags/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 | | [Core.VirtualTags](Core.VirtualTags/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 |
| [Driver.AbCip](Driver.AbCip/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 15 | | [Driver.AbCip](Driver.AbCip/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 15 |
| [Driver.AbCip.Cli](Driver.AbCip.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 8 | | [Driver.AbCip.Cli](Driver.AbCip.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 8 |
| [Driver.AbLegacy](Driver.AbLegacy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 3 | 13 | | [Driver.AbLegacy](Driver.AbLegacy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 |
| [Driver.AbLegacy.Cli](Driver.AbLegacy.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 7 | | [Driver.AbLegacy.Cli](Driver.AbLegacy.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 7 |
| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 2 | 6 | | [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 2 | 6 |
| [Driver.FOCAS](Driver.FOCAS/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 12 | | [Driver.FOCAS](Driver.FOCAS/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
| [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 5 | | [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 5 |
| [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 4 | 14 | | [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 14 |
| [Driver.Historian.Wonderware](Driver.Historian.Wonderware/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 7 | 12 | | [Driver.Historian.Wonderware](Driver.Historian.Wonderware/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 7 | 12 |
| [Driver.Historian.Wonderware.Client](Driver.Historian.Wonderware.Client/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 10 | | [Driver.Historian.Wonderware.Client](Driver.Historian.Wonderware.Client/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 10 |
| [Driver.Modbus](Driver.Modbus/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 7 | 12 | | [Driver.Modbus](Driver.Modbus/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 7 | 12 |
| [Driver.Modbus.Addressing](Driver.Modbus.Addressing/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 3 | 9 | | [Driver.Modbus.Addressing](Driver.Modbus.Addressing/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 3 | 9 |
| [Driver.Modbus.Cli](Driver.Modbus.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 8 | | [Driver.Modbus.Cli](Driver.Modbus.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 8 |
| [Driver.OpcUaClient](Driver.OpcUaClient/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 2 | 15 | | [Driver.OpcUaClient](Driver.OpcUaClient/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 2 | 15 |
| [Driver.S7](Driver.S7/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 14 | | [Driver.S7](Driver.S7/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 14 |
| [Driver.S7.Cli](Driver.S7.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 4 | 7 | | [Driver.S7.Cli](Driver.S7.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 4 | 7 |
| [Driver.TwinCAT](Driver.TwinCAT/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 16 | | [Driver.TwinCAT](Driver.TwinCAT/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 16 |
| [Driver.TwinCAT.Cli](Driver.TwinCAT.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 7 | 7 | | [Driver.TwinCAT.Cli](Driver.TwinCAT.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 7 | 7 |
@@ -67,20 +67,12 @@ Findings with status `Open` or `In Progress`, ordered by severity.
| Client.UI-009 | Low | Design-document adherence | `ViewModels/HistoryViewModel.cs:44-54` | `HistoryViewModel.AggregateTypes` exposes eight entries: `null` (Raw) plus Average, Minimum, Maximum, Count, Start, End, and `StandardDeviation`. `docs/Client.UI.md` ("Query Options" table) lists only "Raw (default), Average, Minimum, Maxi… | | Client.UI-009 | Low | Design-document adherence | `ViewModels/HistoryViewModel.cs:44-54` | `HistoryViewModel.AggregateTypes` exposes eight entries: `null` (Raw) plus Average, Minimum, Maximum, Count, Start, End, and `StandardDeviation`. `docs/Client.UI.md` ("Query Options" table) lists only "Raw (default), Average, Minimum, Maxi… |
| Client.UI-010 | Low | Code organization & conventions | `Controls/DateTimeRangePicker.axaml.cs:33-37`, `Controls/DateTimeRangePicker.axaml.cs:70-80` | `DateTimeRangePicker` declares `MinDateTimeProperty` / `MaxDateTimeProperty` styled properties with public CLR accessors, but neither is read anywhere in the control. `TryParseDateTime`, `OnStartLostFocus`, and `OnEndLostFocus` never clamp… | | Client.UI-010 | Low | Code organization & conventions | `Controls/DateTimeRangePicker.axaml.cs:33-37`, `Controls/DateTimeRangePicker.axaml.cs:70-80` | `DateTimeRangePicker` declares `MinDateTimeProperty` / `MaxDateTimeProperty` styled properties with public CLR accessors, but neither is read anywhere in the control. `TryParseDateTime`, `OnStartLostFocus`, and `OnEndLostFocus` never clamp… |
| Client.UI-011 | Low | Documentation & comments | `Views/MainWindow.axaml:81`, `Services/JsonSettingsService.cs:11-15` | The certificate-store-path `TextBox` watermark reads `(default: AppData/LmxOpcUaClient/pki)`, referencing the legacy pre-task-#208 folder name. Per `CLAUDE.md` / `docs/Client.UI.md` the canonical path is now `{LocalAppData}/OtOpcUaClient/`… | | Client.UI-011 | Low | Documentation & comments | `Views/MainWindow.axaml:81`, `Services/JsonSettingsService.cs:11-15` | The certificate-store-path `TextBox` watermark reads `(default: AppData/LmxOpcUaClient/pki)`, referencing the legacy pre-task-#208 folder name. Per `CLAUDE.md` / `docs/Client.UI.md` the canonical path is now `{LocalAppData}/OtOpcUaClient/`… |
| Driver.AbCip-007 | Low | OtOpcUa conventions | `AbCipDriver.cs` (whole file), `AbCipAlarmProjection.cs`, `LibplctagTagRuntime.cs` | `CLAUDE.md` Library Preferences mandate Serilog with a rolling daily file sink. The driver has no logging at all: no `ILogger`/Serilog dependency is injected or used. Failure paths instead swallow exceptions into the `_health` string (`Rea… |
| Driver.AbCip-011 | Low | Error handling & resilience | `AbCipDriver.cs:144-152`, `AbCipDriverOptions.cs:131-143` | `InitializeAsync` only starts probe loops when `_options.Probe.Enabled` is true AND `Probe.ProbeTagPath` is non-blank. When `Probe.Enabled` is true (the default) but `ProbeTagPath` is null (also the default; the doc comment says "PR 8 wire… |
| Driver.AbCip-012 | Low | Performance & resource management | `LibplctagTemplateReader.cs:15-35`, `AbCipDriver.cs:88-92` | `LibplctagTemplateReader` is created per `FetchUdtShapeAsync` call, and each call constructs a fresh libplctag `Tag` for the @udt pseudo-tag, initializes it (a CIP connection handshake), reads, and disposes it. There is no reuse of the `Ta… |
| Driver.AbCip-013 | Low | Design-document adherence | `AbCipDriverOptions.cs:70-73`, `PlcFamilies/AbCipPlcFamilyProfile.cs:13-19`, `LibplctagTagRuntime.cs:16-27` | `driver-specs.md` specifies the AB CIP per-device connection settings as discrete fields: Host, Path, PlcType, TimeoutMs, AllowPacking, ConnectionSize. The implementation instead collapses host + path into a single opaque ab:// URL string… |
| Driver.AbCip-015 | Low | Documentation & comments | `AbCipDriver.cs:9-11`, `PlcTagHandle.cs:23-27,53-58`, `AbCipTemplateCache.cs:12-15`, `IAbCipTagEnumerator.cs:6-11`, `AbCipDriverOptions.cs:21` | Numerous comments are stale relative to the commit under review. `AbCipDriver.cs:9-11` says the driver "Implements IDriver only for now" with capabilities shipping "in subsequent PRs (3-8)" while the class already implements all of them. `… |
| Driver.AbCip.Cli-003 | Low | Concurrency & thread safety | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:50-56,60-61` | The `OnDataChange` handler writes change lines to `console.Output` (a `TextWriter`) from the driver's poll-engine callback thread, while the command's main flow concurrently writes the "Subscribed to ... Ctrl+C to stop." line on the CLI th… | | Driver.AbCip.Cli-003 | Low | Concurrency & thread safety | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:50-56,60-61` | The `OnDataChange` handler writes change lines to `console.Output` (a `TextWriter`) from the driver's poll-engine callback thread, while the command's main flow concurrently writes the "Subscribed to ... Ctrl+C to stop." line on the CLI th… |
| Driver.AbCip.Cli-004 | Low | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:28,58`; `AbCipCommandBase.cs:26-34` | `--interval-ms` (`IntervalMs`) is taken verbatim and passed as `TimeSpan.FromMilliseconds(IntervalMs)` to `SubscribeAsync` with no validation. A zero or negative value produces a non-positive `TimeSpan`; the option description claims "Poll… | | Driver.AbCip.Cli-004 | Low | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:28,58`; `AbCipCommandBase.cs:26-34` | `--interval-ms` (`IntervalMs`) is taken verbatim and passed as `TimeSpan.FromMilliseconds(IntervalMs)` to `SubscribeAsync` with no validation. A zero or negative value produces a non-positive `TimeSpan`; the option description claims "Poll… |
| Driver.AbCip.Cli-005 | Low | Performance & resource management | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` | `ConfigureLogging` assigns a freshly created Serilog logger to the process-global `Log.Logger` but never calls `Log.CloseAndFlush()`. For a short-lived one-shot command (`probe`, `read`, `write`) the process exit flushes the console sink,… | | Driver.AbCip.Cli-005 | Low | Performance & resource management | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` | `ConfigureLogging` assigns a freshly created Serilog logger to the process-global `Log.Logger` but never calls `Log.CloseAndFlush()`. For a short-lived one-shot command (`probe`, `read`, `write`) the process exit flushes the console sink,… |
| Driver.AbCip.Cli-006 | Low | Design-document adherence | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs:29-34` | `AbCipCommandBase` overrides the abstract `DriverCommandBase.Timeout` property with a getter derived from `TimeoutMs` and an empty `init` body (`init { /* driven by TimeoutMs */ }`). Because the override has no `[CommandOption]` attribute,… | | Driver.AbCip.Cli-006 | Low | Design-document adherence | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs:29-34` | `AbCipCommandBase` overrides the abstract `DriverCommandBase.Timeout` property with a getter derived from `TimeoutMs` and an empty `init` body (`init { /* driven by TimeoutMs */ }`). Because the override has no `[CommandOption]` attribute,… |
| Driver.AbCip.Cli-007 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs` | The only test file covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName` — both pure static helpers. There is no coverage for `AbCipCommandBase.BuildOptions` (the flag-to-`AbCipDriverOptions` mapping that all four commands d… | | Driver.AbCip.Cli-007 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs` | The only test file covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName` — both pure static helpers. There is no coverage for `AbCipCommandBase.BuildOptions` (the flag-to-`AbCipDriverOptions` mapping that all four commands d… |
| Driver.AbCip.Cli-008 | Low | Documentation & comments | `docs/Driver.AbCip.Cli.md:8-9` | `docs/Driver.AbCip.Cli.md` opens with "Second of four driver test-client CLIs (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT)." The count "four" contradicts the chain that follows it (five names) and contradicts `docs/DriverClis.md`, whic… | | Driver.AbCip.Cli-008 | Low | Documentation & comments | `docs/Driver.AbCip.Cli.md:8-9` | `docs/Driver.AbCip.Cli.md` opens with "Second of four driver test-client CLIs (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT)." The count "four" contradicts the chain that follows it (five names) and contradicts `docs/DriverClis.md`, whic… |
| Driver.AbLegacy-005 | Low | OtOpcUa conventions | `AbLegacyDriver.cs` (whole file) | The driver uses no `ILogger`/Serilog at all. Probe-loop failures, runtime initialisation failures, libplctag non-zero statuses, and read/write exceptions are folded into `DriverHealth.Detail` strings but never logged. CLAUDE.md names Seril… |
| Driver.AbLegacy-011 | Low | Performance & resource management | `AbLegacyDriver.cs:440` | `Dispose()` is implemented as `DisposeAsync().AsTask().GetAwaiter().GetResult()` - sync-over-async. `ShutdownAsync` awaits `_poll.DisposeAsync()` (which completes synchronously) and does no other real async work, so a deadlock is unlikely… |
| Driver.AbLegacy-013 | Low | Code organization & conventions | `AbLegacyDriver.cs:340-345`, `AbLegacyDriver.cs:238-264` | Two minor organisational issues: 1. `ResolveHost` returns `_options.Devices.FirstOrDefault()?.HostAddress ?? DriverInstanceId` when the reference is unknown and no devices are configured. `DriverInstanceId` is not a host address (ab://...)… |
| Driver.AbLegacy.Cli-002 | Low | Correctness & logic bugs | `Commands/WriteCommand.cs:27-29`, `Program.cs:6-9` | The `--value` option help text states "booleans accept true/false/1/0", but `ParseBool` (`WriteCommand.cs:74-80`) and the error message also accept `on/off` and `yes/no`, and `DriverClis.md` documents the full `true/false/1/0/yes/no/on/off… | | Driver.AbLegacy.Cli-002 | Low | Correctness & logic bugs | `Commands/WriteCommand.cs:27-29`, `Program.cs:6-9` | The `--value` option help text states "booleans accept true/false/1/0", but `ParseBool` (`WriteCommand.cs:74-80`) and the error message also accept `on/off` and `yes/no`, and `DriverClis.md` documents the full `true/false/1/0/yes/no/on/off… |
| Driver.AbLegacy.Cli-003 | Low | Concurrency & thread safety | `Commands/SubscribeCommand.cs:47-53` | The `OnDataChange` handler calls `console.Output.WriteLine(line)` (the synchronous overload) directly from the `PollGroupEngine` poll thread. The poll engine raises change events from a background timer/loop thread, so two ticks that fire… | | Driver.AbLegacy.Cli-003 | Low | Concurrency & thread safety | `Commands/SubscribeCommand.cs:47-53` | The `OnDataChange` handler calls `console.Output.WriteLine(line)` (the synchronous overload) directly from the `PollGroupEngine` poll thread. The poll engine raises change events from a background timer/loop thread, so two ticks that fire… |
| Driver.AbLegacy.Cli-004 | Low | Error handling & resilience | `Commands/ProbeCommand.cs:37-56`, `Commands/ReadCommand.cs:39-50`, `Commands/WriteCommand.cs:48-59`, `Commands/SubscribeCommand.cs:41-76` | Every command does `await using var driver = new AbLegacyDriver(...)` *and* an explicit `await driver.ShutdownAsync(...)` in the `finally`. `AbLegacyDriver` `DisposeAsync` itself calls `ShutdownAsync`, so the driver is shut down twice on t… | | Driver.AbLegacy.Cli-004 | Low | Error handling & resilience | `Commands/ProbeCommand.cs:37-56`, `Commands/ReadCommand.cs:39-50`, `Commands/WriteCommand.cs:48-59`, `Commands/SubscribeCommand.cs:41-76` | Every command does `await using var driver = new AbLegacyDriver(...)` *and* an explicit `await driver.ShutdownAsync(...)` in the `finally`. `AbLegacyDriver` `DisposeAsync` itself calls `ShutdownAsync`, so the driver is shut down twice on t… |
@@ -89,20 +81,11 @@ Findings with status `Open` or `In Progress`, ordered by severity.
| Driver.AbLegacy.Cli-007 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs` | The only test file in the CLI test project covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. Two behaviours that are pure logic (testable without a device) are uncovered: (1) `AbLegacyCommandBase.BuildOptions` — that it… | | Driver.AbLegacy.Cli-007 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs` | The only test file in the CLI test project covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. Two behaviours that are pure logic (testable without a device) are uncovered: (1) `AbLegacyCommandBase.BuildOptions` — that it… |
| Driver.Cli.Common-004 | Low | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` | `FormatTable` calls `rows.Max(r => r.Tag.Length)` (and the same for the value and status columns) without guarding against empty input. When `tagNames` and `snapshots` are both empty (equal length, so the mismatch check at line 56 passes),… | | Driver.Cli.Common-004 | Low | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` | `FormatTable` calls `rows.Max(r => r.Tag.Length)` (and the same for the value and status columns) without guarding against empty input. When `tagNames` and `snapshots` are both empty (equal length, so the mismatch check at line 56 passes),… |
| Driver.Cli.Common-006 | Low | Documentation & comments | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:71`, `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:9` | Two minor doc inaccuracies. (1) The comment at `SnapshotFormatter.cs:71` states the "source-time column is fixed-width (ISO-8601 to ms) so no max-measurement needed" — true only when every snapshot has a non-null `SourceTimestampUtc`. `For… | | Driver.Cli.Common-006 | Low | Documentation & comments | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:71`, `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:9` | Two minor doc inaccuracies. (1) The comment at `SnapshotFormatter.cs:71` states the "source-time column is fixed-width (ISO-8601 to ms) so no max-measurement needed" — true only when every snapshot has a non-null `SourceTimestampUtc`. `For… |
| Driver.FOCAS-007 | Low | Error handling & resilience | `FocasDriver.cs:140-148`, `FocasDriver.cs:478-484`, `FocasDriver.cs:529-533`, `FocasAlarmProjection.cs:61-63` | Numerous `try { ... } catch {}` blocks swallow every exception with no logging - `ShutdownAsync` (CTS cancel/dispose), `RecycleLoopAsync` (`DisposeClient`), `FixedTreeLoopAsync` transient catches, `ProbeLoopAsync`, and the alarm projection… |
| Driver.FOCAS-008 | Low | Performance & resource management | `FocasDriver.cs:201`, `FocasDriver.cs:253` | `ReadAsync` and `WriteAsync` call `FocasAddress.TryParse(def.Address)` on every operation, even though `InitializeAsync` already parsed and validated every tag address. On a subscription hot path (each poll tick re-enters `ReadAsync`) this… |
| Driver.FOCAS-009 | Low | Design-document adherence | `FocasDriverOptions.cs:110-115`, `FocasDriver.cs:468-486`, `FocasDriverFactoryExtensions.cs:75-80` | `FocasProbeOptions.Timeout` is parsed by the factory (`FocasProbeDto.TimeoutMs` to `FocasProbeOptions.Timeout`) but never consumed. `ProbeLoopAsync` calls `client.ProbeAsync(ct)` with only the probe-loop cancellation token; no per-probe ti… |
| Driver.FOCAS-010 | Low | Code organization & conventions | `IFocasClient.cs:210-227` (`FocasOpMode`), `FocasConstants.cs:42-78` (`FocasOperationMode`) | There are two parallel operation-mode-to-text mappings with divergent labels. `FocasOpMode.ToText` (used by the driver fixed-tree `OperationMode/ModeText` node) yields `"TJOG"`, `"TEACH_IN_HANDLE"`; `FocasOperationModeExtensions.ToText` (i… |
| Driver.FOCAS-011 | Low | Code organization & conventions | `IFocasClient.cs:275-287` (`FocasAlarmType`), `FocasAlarmProjection.cs:149-175` | `FocasAlarmType` declares its constants as `public const int`, but the only consumers - `FocasAlarmProjection.MapAlarmType(short type)` and `MapSeverity(short type)` - take a `short` and `switch` against these `int` constants. It compiles… |
| Driver.FOCAS.Cli-001 | Low | Error handling & resilience | `Commands/WriteCommand.cs:58-68` | `WriteCommand.ParseValue` parses the numeric `--value` types (`Byte`/`Int16`/`Int32`/`Float32`/`Float64`) with `sbyte.Parse` / `short.Parse` / etc. These throw raw `FormatException` or `OverflowException` for malformed or out-of-range inpu… | | Driver.FOCAS.Cli-001 | Low | Error handling & resilience | `Commands/WriteCommand.cs:58-68` | `WriteCommand.ParseValue` parses the numeric `--value` types (`Byte`/`Int16`/`Int32`/`Float32`/`Float64`) with `sbyte.Parse` / `short.Parse` / etc. These throw raw `FormatException` or `OverflowException` for malformed or out-of-range inpu… |
| Driver.FOCAS.Cli-002 | Low | Concurrency & thread safety | `Commands/SubscribeCommand.cs:45-51` | The `subscribe` command attaches an `OnDataChange` handler that calls the synchronous `console.Output.WriteLine`. `OnDataChange` is raised from the driver's `PollGroupEngine` tick thread, while the command's main flow writes the "Subscribe… | | Driver.FOCAS.Cli-002 | Low | Concurrency & thread safety | `Commands/SubscribeCommand.cs:45-51` | The `subscribe` command attaches an `OnDataChange` handler that calls the synchronous `console.Output.WriteLine`. `OnDataChange` is raised from the driver's `PollGroupEngine` tick thread, while the command's main flow writes the "Subscribe… |
| Driver.FOCAS.Cli-003 | Low | Error handling & resilience | `FocasCommandBase.cs:19` (`CncPort`), `FocasCommandBase.cs:27` (`TimeoutMs`), `Commands/SubscribeCommand.cs:23` (`IntervalMs`) | The numeric command options `--cnc-port`, `--timeout-ms`, and `--interval-ms` are accepted without range validation. A zero or negative `--cnc-port` produces an invalid `focas://host:<n>` string; `--timeout-ms 0` yields a zero `TimeSpan` o… | | Driver.FOCAS.Cli-003 | Low | Error handling & resilience | `FocasCommandBase.cs:19` (`CncPort`), `FocasCommandBase.cs:27` (`TimeoutMs`), `Commands/SubscribeCommand.cs:23` (`IntervalMs`) | The numeric command options `--cnc-port`, `--timeout-ms`, and `--interval-ms` are accepted without range validation. A zero or negative `--cnc-port` produces an invalid `focas://host:<n>` string; `--timeout-ms 0` yields a zero `TimeSpan` o… |
| Driver.FOCAS.Cli-004 | Low | Performance & resource management | `Commands/ProbeCommand.cs:37,54`; `Commands/ReadCommand.cs:37,46`; `Commands/WriteCommand.cs:45,54`; `Commands/SubscribeCommand.cs:39,73` | Every command declares `await using var driver = new FocasDriver(...)` | | Driver.FOCAS.Cli-004 | Low | Performance & resource management | `Commands/ProbeCommand.cs:37,54`; `Commands/ReadCommand.cs:37,46`; `Commands/WriteCommand.cs:45,54`; `Commands/SubscribeCommand.cs:39,73` | Every command declares `await using var driver = new FocasDriver(...)` |
| Driver.FOCAS.Cli-005 | Low | Design-document adherence | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) | `docs/Driver.FOCAS.Cli.md` documents `BadDeviceFailure` and `BadCommunicationError` as the key diagnostic signals an operator reads off `probe` / `write` output ("A `BadCommunicationError` means ... `BadDeviceFailure` after a successful co… | | Driver.FOCAS.Cli-005 | Low | Design-document adherence | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) | `docs/Driver.FOCAS.Cli.md` documents `BadDeviceFailure` and `BadCommunicationError` as the key diagnostic signals an operator reads off `probe` / `write` output ("A `BadCommunicationError` means ... `BadDeviceFailure` after a successful co… |
| Driver.Galaxy-005 | Low | OtOpcUa conventions | `Runtime/EventPump.cs:81-88` | The `BoundedChannelOptions` comment states "Newest-dropped policy: when full, the producer's TryWrite returns false ... We do this manually rather than relying on `BoundedChannelFullMode.DropWrite`" — but the option is then set to `FullMod… |
| Driver.Galaxy-010 | Low | Security | `GalaxyDriver.cs:311-341` | `ResolveApiKey` supports an `env:`/`file:` indirection and otherwise treats the config string as the literal API key ("Anything else — used as the literal API key. Convenient for dev"). `GalaxyGatewayOptions`' own XML doc claims "the API k… |
| Driver.Galaxy-012 | Low | Performance & resource management | `Runtime/SubscriptionRegistry.cs:65-67`, `GalaxyDriver.cs:538`, `GalaxyDriver.cs:675` | Several hot paths are O(n^2) per call. `SubscriptionRegistry.ResolveSubscribers` does `entry.Bindings.FirstOrDefault(b => b.ItemHandle == itemHandle)` — a linear scan of the whole binding list for every event dispatch; at 50k tags this is… |
| Driver.Galaxy-013 | Low | Design-document adherence | `GalaxyDriver.cs:14-27`, `GalaxyDriver.cs:374-382`, `Config/GalaxyDriverOptions.cs:84-86` | Multiple doc comments are stale relative to the shipped code. `GalaxyDriver`'s class summary still describes the file as "the project skeleton with `IDriver` bodies that wire to a future `IGalaxyGatewayClient` abstraction. Capability inter… |
| Driver.Historian.Wonderware-004 | Low | Correctness and logic bugs | `Backend/SdkAlarmHistorianWriteBackend.cs:198-201` | `ToHistorianEvent` only assigns `historianEvent.Id` when `Guid.TryParse(dto.EventId, ...)` succeeds. If `EventId` is not a parseable GUID (or is empty), `Id` stays `Guid.Empty` and the event is written to the historian with an all-zeros id… | | Driver.Historian.Wonderware-004 | Low | Correctness and logic bugs | `Backend/SdkAlarmHistorianWriteBackend.cs:198-201` | `ToHistorianEvent` only assigns `historianEvent.Id` when `Guid.TryParse(dto.EventId, ...)` succeeds. If `EventId` is not a parseable GUID (or is empty), `Id` stays `Guid.Empty` and the event is written to the historian with an all-zeros id… |
| Driver.Historian.Wonderware-005 | Low | Concurrency and thread safety | `Backend/HistorianDataSource.cs:124`, `:126-127` | `GetHealthSnapshot` reads `_activeProcessNode` and `_activeEventNode` inside `_healthLock`, but those two fields are written under `_connectionLock` / `_eventConnectionLock` (lines 183, 243, 209-210, 266-269) — a different lock. The health… | | Driver.Historian.Wonderware-005 | Low | Concurrency and thread safety | `Backend/HistorianDataSource.cs:124`, `:126-127` | `GetHealthSnapshot` reads `_activeProcessNode` and `_activeEventNode` inside `_healthLock`, but those two fields are written under `_connectionLock` / `_eventConnectionLock` (lines 183, 243, 209-210, 266-269) — a different lock. The health… |
| Driver.Historian.Wonderware-007 | Low | Error handling and resilience | `Ipc/PipeServer.cs:70-75` | When `VerifyCaller` rejects the peer SID, the server logs the reason and calls `_current.Disconnect()` with no `HelloAck` frame sent. The shared-secret-mismatch and major-version-mismatch paths below it both send a rejecting `HelloAck` so… | | Driver.Historian.Wonderware-007 | Low | Error handling and resilience | `Ipc/PipeServer.cs:70-75` | When `VerifyCaller` rejects the peer SID, the server logs the reason and calls `_current.Disconnect()` with no `HelloAck` frame sent. The shared-secret-mismatch and major-version-mismatch paths below it both send a rejecting `HelloAck` so… |
@@ -133,11 +116,6 @@ Findings with status `Open` or `In Progress`, ordered by severity.
| Driver.Modbus.Cli-008 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/` | The test project covers only the two pure-function seams: `ReadCommand.SynthesiseTagName` and `WriteCommand.ParseValue`. There is no coverage for `WriteCommand`'s read-only-region rejection (`Region is not (Coils or HoldingRegisters)`), no… | | Driver.Modbus.Cli-008 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/` | The test project covers only the two pure-function seams: `ReadCommand.SynthesiseTagName` and `WriteCommand.ParseValue`. There is no coverage for `WriteCommand`'s read-only-region rejection (`Region is not (Coils or HoldingRegisters)`), no… |
| Driver.OpcUaClient-011 | Low | Documentation & comments | `OpcUaClientDriver.cs:783-784` | The comment on the isArray computation states "-1 = scalar; 1+ = array dimensions; 0 = one-dimensional array". This is inaccurate against OPC UA ValueRank semantics: -3 is ScalarOrOneDimension, -2 is Any, -1 is Scalar, and 0 is OneOrMoreDi… | | Driver.OpcUaClient-011 | Low | Documentation & comments | `OpcUaClientDriver.cs:783-784` | The comment on the isArray computation states "-1 = scalar; 1+ = array dimensions; 0 = one-dimensional array". This is inaccurate against OPC UA ValueRank semantics: -3 is ScalarOrOneDimension, -2 is Any, -1 is Scalar, and 0 is OneOrMoreDi… |
| Driver.OpcUaClient-014 | Low | Performance & resource management | `OpcUaClientDriver.cs:904`, `:1035` | `MonitoredItem.Notification += (mi, args) => ...` (and the alarm-event equivalent) attaches a closure-capturing lambda to each monitored item's event. The lambda is never detached. When UnsubscribeAsync removes a subscription it calls Subs… | | Driver.OpcUaClient-014 | Low | Performance & resource management | `OpcUaClientDriver.cs:904`, `:1035` | `MonitoredItem.Notification += (mi, args) => ...` (and the alarm-event equivalent) attaches a closure-capturing lambda to each monitored item's event. The lambda is never detached. When UnsubscribeAsync removes a subscription it calls Subs… |
| Driver.S7-003 | Low | Correctness & logic bugs | `S7Driver.cs:172`, `S7Driver.cs:255` | ReadAsync and WriteAsync dereference fullReferences.Count / writes.Count with no null guard. A null argument throws NullReferenceException rather than ArgumentNullException, and the NRE escapes before the _gate is taken so it is not wrappe… |
| Driver.S7-005 | Low | OtOpcUa conventions | `S7Driver.cs:33`, `S7Driver.cs:433` | System.Collections.Concurrent.ConcurrentDictionary is written out with a fully-qualified namespace at the field declarations instead of a using System.Collections.Concurrent directive. ImplicitUsings is enabled and the rest of the codebase… |
| Driver.S7-009 | Low | Error handling & resilience | `S7Driver.cs:392` | The subscription poll loop never reflects sustained polling failure anywhere an operator can see it. PollLoopAsync swallows every non-cancellation exception with an empty catch and the comment claims "the health surface reflects it" - but… |
| Driver.S7-010 | Low | Performance & resource management | `S7Driver.cs:504` | Dispose() is implemented as DisposeAsync().AsTask().GetAwaiter().GetResult() - sync-over-async. Inside the generic host this is currently safe (no captured SynchronizationContext), but it is a known deadlock pattern. The only async work be… |
| Driver.S7-013 | Low | Code organization & conventions | `S7DriverOptions.cs:90`, `S7Driver.cs:300` | S7TagDefinition.StringLength is a public configured/JSON-bound parameter (default 254) but is dead: S7DataType.String reads and writes both throw NotSupportedException ("...land in a follow-up PR"), so StringLength is never consumed. Likew… |
| Driver.S7.Cli-004 | Low | Performance & resource management | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs:36,53`, `Commands/ReadCommand.cs:45,54`, `Commands/WriteCommand.cs:51,60`, `Commands/SubscribeCommand.cs:39,73` | Every command declares the driver with `await using var driver = new S7Driver(...)` and *also* calls `await driver.ShutdownAsync(...)` in a `finally` block. `S7Driver.DisposeAsync` itself calls `ShutdownAsync`, so shutdown runs twice per c… | | Driver.S7.Cli-004 | Low | Performance & resource management | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs:36,53`, `Commands/ReadCommand.cs:45,54`, `Commands/WriteCommand.cs:51,60`, `Commands/SubscribeCommand.cs:39,73` | Every command declares the driver with `await using var driver = new S7Driver(...)` and *also* calls `await driver.ShutdownAsync(...)` in a `finally` block. `S7Driver.DisposeAsync` itself calls `ShutdownAsync`, so shutdown runs twice per c… |
| Driver.S7.Cli-005 | Low | Code organization & conventions | `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` | A stale directory `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` exists containing only an `obj/` folder — no `.csproj`, no source. The real test project lives at `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/`. The empty direct… | | Driver.S7.Cli-005 | Low | Code organization & conventions | `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` | A stale directory `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` exists containing only an `obj/` folder — no `.csproj`, no source. The real test project lives at `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/`. The empty direct… |
| Driver.S7.Cli-006 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs` | The only test file covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. `S7CommandBase.BuildOptions` — which maps the host / port / CPU / rack / slot / timeout flags onto an `S7DriverOptions` and forces `Probe.Enabled = fa… | | Driver.S7.Cli-006 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs` | The only test file covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. `S7CommandBase.BuildOptions` — which maps the host / port / CPU / rack / slot / timeout flags onto an `S7DriverOptions` and forces `Probe.Enabled = fa… |
@@ -384,6 +362,28 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
| Core.VirtualTags-010 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/ITagUpstreamSource.cs:18`, `VirtualTagContext.cs:30`, `VirtualTagDefinition.cs:28` | | Core.VirtualTags-010 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/ITagUpstreamSource.cs:18`, `VirtualTagContext.cs:30`, `VirtualTagDefinition.cs:28` |
| Core.VirtualTags-011 | Low | Resolved | Code organization & conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:404-409` | | Core.VirtualTags-011 | Low | Resolved | Code organization & conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:404-409` |
| Core.VirtualTags-013 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs:266-270` | | Core.VirtualTags-013 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs:266-270` |
| Driver.AbCip-007 | Low | Resolved | OtOpcUa conventions | `AbCipDriver.cs` (whole file), `AbCipAlarmProjection.cs`, `LibplctagTagRuntime.cs` |
| Driver.AbCip-011 | Low | Resolved | Error handling & resilience | `AbCipDriver.cs:144-152`, `AbCipDriverOptions.cs:131-143` |
| Driver.AbCip-012 | Low | Resolved | Performance & resource management | `LibplctagTemplateReader.cs:15-35`, `AbCipDriver.cs:88-92` |
| Driver.AbCip-013 | Low | Resolved | Design-document adherence | `AbCipDriverOptions.cs:70-73`, `PlcFamilies/AbCipPlcFamilyProfile.cs:13-19`, `LibplctagTagRuntime.cs:16-27` |
| Driver.AbCip-015 | Low | Resolved | Documentation & comments | `AbCipDriver.cs:9-11`, `PlcTagHandle.cs:23-27,53-58`, `AbCipTemplateCache.cs:12-15`, `IAbCipTagEnumerator.cs:6-11`, `AbCipDriverOptions.cs:21` |
| Driver.AbLegacy-005 | Low | Resolved | OtOpcUa conventions | `AbLegacyDriver.cs` (whole file) |
| Driver.AbLegacy-011 | Low | Resolved | Performance & resource management | `AbLegacyDriver.cs:440` |
| Driver.AbLegacy-013 | Low | Resolved | Code organization & conventions | `AbLegacyDriver.cs:340-345`, `AbLegacyDriver.cs:238-264` |
| Driver.FOCAS-007 | Low | Resolved | Error handling & resilience | `FocasDriver.cs:140-148`, `FocasDriver.cs:478-484`, `FocasDriver.cs:529-533`, `FocasAlarmProjection.cs:61-63` |
| Driver.FOCAS-008 | Low | Resolved | Performance & resource management | `FocasDriver.cs:201`, `FocasDriver.cs:253` |
| Driver.FOCAS-009 | Low | Resolved | Design-document adherence | `FocasDriverOptions.cs:110-115`, `FocasDriver.cs:468-486`, `FocasDriverFactoryExtensions.cs:75-80` |
| Driver.FOCAS-010 | Low | Resolved | Code organization & conventions | `IFocasClient.cs:210-227` (`FocasOpMode`), `FocasConstants.cs:42-78` (`FocasOperationMode`) |
| Driver.FOCAS-011 | Low | Resolved | Code organization & conventions | `IFocasClient.cs:275-287` (`FocasAlarmType`), `FocasAlarmProjection.cs:149-175` |
| Driver.Galaxy-005 | Low | Resolved | OtOpcUa conventions | `Runtime/EventPump.cs:81-88` |
| Driver.Galaxy-010 | Low | Resolved | Security | `GalaxyDriver.cs:311-341` |
| Driver.Galaxy-012 | Low | Resolved | Performance & resource management | `Runtime/SubscriptionRegistry.cs:65-67`, `GalaxyDriver.cs:538`, `GalaxyDriver.cs:675` |
| Driver.Galaxy-013 | Low | Resolved | Design-document adherence | `GalaxyDriver.cs:14-27`, `GalaxyDriver.cs:374-382`, `Config/GalaxyDriverOptions.cs:84-86` |
| Driver.S7-003 | Low | Resolved | Correctness & logic bugs | `S7Driver.cs:172`, `S7Driver.cs:255` |
| Driver.S7-005 | Low | Resolved | OtOpcUa conventions | `S7Driver.cs:33`, `S7Driver.cs:433` |
| Driver.S7-009 | Low | Resolved | Error handling & resilience | `S7Driver.cs:392` |
| Driver.S7-010 | Low | Resolved | Performance & resource management | `S7Driver.cs:504` |
| Driver.S7-013 | Low | Resolved | Code organization & conventions | `S7DriverOptions.cs:90`, `S7Driver.cs:300` |
| Server-004 | Low | Resolved | OtOpcUa conventions | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OtOpcUaServer.cs:187-200` | | Server-004 | Low | Resolved | OtOpcUa conventions | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OtOpcUaServer.cs:187-200` |
| Server-006 | Low | Resolved | Concurrency & thread safety | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:478-482, 1342-1348` | | Server-006 | Low | Resolved | Concurrency & thread safety | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:478-482, 1342-1348` |
| Server-008 | Low | Resolved | Error handling & resilience | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:736` | | Server-008 | Low | Resolved | Error handling & resilience | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:736` |
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipAlarmProjection.cs
+13 -2
View File
@@ -1,3 +1,5 @@
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip;
@@ -32,14 +34,16 @@ internal sealed class AbCipAlarmProjection : IAsyncDisposable
{ {
private readonly AbCipDriver _driver; private readonly AbCipDriver _driver;
private readonly TimeSpan _pollInterval; private readonly TimeSpan _pollInterval;
private readonly ILogger _logger;
private readonly Dictionary<long, Subscription> _subs = new(); private readonly Dictionary<long, Subscription> _subs = new();
private readonly Lock _subsLock = new(); private readonly Lock _subsLock = new();
private long _nextId; private long _nextId;
public AbCipAlarmProjection(AbCipDriver driver, TimeSpan pollInterval) public AbCipAlarmProjection(AbCipDriver driver, TimeSpan pollInterval, ILogger? logger = null)
{ {
_driver = driver; _driver = driver;
_pollInterval = pollInterval; _pollInterval = pollInterval;
_logger = logger ?? NullLogger.Instance;
} }
public async Task<IAlarmSubscriptionHandle> SubscribeAsync( public async Task<IAlarmSubscriptionHandle> SubscribeAsync(
@@ -158,7 +162,14 @@ internal sealed class AbCipAlarmProjection : IAsyncDisposable
Tick(sub, results); Tick(sub, results);
} }
catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; } catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; }
catch { /* per-tick failures are non-fatal; next tick retries */ } catch (Exception ex)
{
// Per-tick failures are non-fatal; next tick retries. Log at debug because a
// wedged controller produces one exception per tick and the operator already
// sees the failed-read warning from ReadAsync below this layer; this log just
// confirms the alarm projection loop is still running.
_logger.LogDebug(ex, "AbCip alarm-projection poll tick failed (will retry)");
}
try { await Task.Delay(_pollInterval, ct).ConfigureAwait(false); } try { await Task.Delay(_pollInterval, ct).ConfigureAwait(false); }
catch (OperationCanceledException) { break; } catch (OperationCanceledException) { break; }
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDataType.cs
+2 -1
View File
@@ -5,7 +5,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip;
/// <summary> /// <summary>
/// Logix atomic + string data types, plus a <see cref="Structure"/> marker used when a tag /// Logix atomic + string data types, plus a <see cref="Structure"/> marker used when a tag
/// references a UDT / predefined structure (Timer, Counter, Control). The concrete UDT /// references a UDT / predefined structure (Timer, Counter, Control). The concrete UDT
/// shape is resolved via the CIP Template Object at discovery time (PR 5 / PR 6). /// shape is resolved via the CIP Template Object at discovery time (see
/// <see cref="CipTemplateObjectDecoder"/> + <see cref="AbCipTemplateCache"/>).
/// </summary> /// </summary>
/// <remarks> /// <remarks>
/// Mirrors the shape of <c>ModbusDataType</c>. Atomic Logix names (BOOL / SINT / INT / DINT / /// Mirrors the shape of <c>ModbusDataType</c>. Atomic Logix names (BOOL / SINT / INT / DINT /
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs
+95 -45
View File
@@ -1,3 +1,5 @@
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.AbCip.PlcFamilies; using ZB.MOM.WW.OtOpcUa.Driver.AbCip.PlcFamilies;
@@ -34,6 +36,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
private readonly PollGroupEngine _poll; private readonly PollGroupEngine _poll;
private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, AbCipTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, AbCipTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase);
private readonly ILogger<AbCipDriver> _logger;
private AbCipAlarmProjection _alarmProjection; private AbCipAlarmProjection _alarmProjection;
private DriverHealth _health = new(DriverState.Unknown, null, null); private DriverHealth _health = new(DriverState.Unknown, null, null);
@@ -47,7 +50,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
public AbCipDriver(AbCipDriverOptions options, string driverInstanceId, public AbCipDriver(AbCipDriverOptions options, string driverInstanceId,
IAbCipTagFactory? tagFactory = null, IAbCipTagFactory? tagFactory = null,
IAbCipTagEnumeratorFactory? enumeratorFactory = null, IAbCipTagEnumeratorFactory? enumeratorFactory = null,
IAbCipTemplateReaderFactory? templateReaderFactory = null) IAbCipTemplateReaderFactory? templateReaderFactory = null,
ILogger<AbCipDriver>? logger = null)
{ {
ArgumentNullException.ThrowIfNull(options); ArgumentNullException.ThrowIfNull(options);
_options = options; _options = options;
@@ -55,11 +59,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
_tagFactory = tagFactory ?? new LibplctagTagFactory(); _tagFactory = tagFactory ?? new LibplctagTagFactory();
_enumeratorFactory = enumeratorFactory ?? new LibplctagTagEnumeratorFactory(); _enumeratorFactory = enumeratorFactory ?? new LibplctagTagEnumeratorFactory();
_templateReaderFactory = templateReaderFactory ?? new LibplctagTemplateReaderFactory(); _templateReaderFactory = templateReaderFactory ?? new LibplctagTemplateReaderFactory();
_logger = logger ?? NullLogger<AbCipDriver>.Instance;
_poll = new PollGroupEngine( _poll = new PollGroupEngine(
reader: ReadAsync, reader: ReadAsync,
onChange: (handle, tagRef, snapshot) => onChange: (handle, tagRef, snapshot) =>
OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot))); OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)));
_alarmProjection = new AbCipAlarmProjection(this, _options.AlarmPollInterval); _alarmProjection = new AbCipAlarmProjection(this, _options.AlarmPollInterval, _logger);
} }
/// <summary> /// <summary>
@@ -77,13 +82,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
if (!_devices.TryGetValue(deviceHostAddress, out var device)) return null; if (!_devices.TryGetValue(deviceHostAddress, out var device)) return null;
var deviceParams = new AbCipTagCreateParams( var deviceParams = device.BuildCreateParams($"@udt/{templateInstanceId}", _options.Timeout);
Gateway: device.ParsedAddress.Gateway,
Port: device.ParsedAddress.Port,
CipPath: device.ParsedAddress.CipPath,
LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute,
TagName: $"@udt/{templateInstanceId}",
Timeout: _options.Timeout);
try try
{ {
@@ -95,16 +94,23 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
return shape; return shape;
} }
catch (OperationCanceledException) { throw; } catch (OperationCanceledException) { throw; }
catch catch (Exception ex)
{ {
// Template read failure — log via the driver's health surface so operators see it, // Template read failure — surface via the driver's health surface AND a warning
// but don't propagate since callers should fall back to declaration-driven UDT // log so operators see it; don't propagate since callers should fall back to
// semantics rather than failing the whole discovery run. // declaration-driven UDT semantics rather than failing the whole discovery run.
_logger.LogWarning(ex,
"AbCip driver {DriverInstanceId} failed to read UDT template {TemplateInstanceId} from device {Device}; " +
"falling back to declaration-driven UDT semantics",
_driverInstanceId, templateInstanceId, deviceHostAddress);
return null; return null;
} }
} }
/// <summary>Shared UDT template cache. Exposed for PR 6 (UDT reader) + diagnostics.</summary> /// <summary>
/// Shared UDT template cache populated by <see cref="FetchUdtShapeAsync"/>. Exposed
/// internally so tests + diagnostics can inspect cached shapes.
/// </summary>
internal AbCipTemplateCache TemplateCache => _templateCache; internal AbCipTemplateCache TemplateCache => _templateCache;
public string DriverInstanceId => _driverInstanceId; public string DriverInstanceId => _driverInstanceId;
@@ -132,7 +138,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
if (parsed.Devices.Count > 0 || parsed.Tags.Count > 0) if (parsed.Devices.Count > 0 || parsed.Tags.Count > 0)
{ {
_options = parsed; _options = parsed;
_alarmProjection = new AbCipAlarmProjection(this, _options.AlarmPollInterval); _alarmProjection = new AbCipAlarmProjection(this, _options.AlarmPollInterval, _logger);
} }
} }
@@ -192,11 +198,24 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct); state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct);
} }
} }
else if (_options.Probe.Enabled && _devices.Count > 0)
{
// Driver.AbCip-011: probe is Enabled but no ProbeTagPath is configured. Without a
// tag path the loop has nothing to read, so HostState would stay Unknown forever
// and GetHostStatuses() would report every device as Unknown with no warning.
// Log a warning so the misconfiguration is visible in the rolling Serilog file.
_logger.LogWarning(
"AbCip probe is enabled but no ProbeTagPath is configured for driver {DriverInstanceId} — " +
"host connectivity probe loops were NOT started; GetHostStatuses() will report every device " +
"as Unknown until a ProbeTagPath is set or Probe.Enabled is set to false.",
_driverInstanceId);
}
_health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null);
} }
catch (Exception ex) catch (Exception ex)
{ {
_health = new DriverHealth(DriverState.Faulted, null, ex.Message); _health = new DriverHealth(DriverState.Faulted, null, ex.Message);
_logger.LogError(ex, "AbCip driver {DriverInstanceId} failed to initialize", _driverInstanceId);
throw; throw;
} }
return Task.CompletedTask; return Task.CompletedTask;
@@ -307,13 +326,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
private async Task ProbeLoopAsync(DeviceState state, CancellationToken ct) private async Task ProbeLoopAsync(DeviceState state, CancellationToken ct)
{ {
var probeParams = new AbCipTagCreateParams( var probeParams = state.BuildCreateParams(_options.Probe.ProbeTagPath!, _options.Probe.Timeout);
Gateway: state.ParsedAddress.Gateway,
Port: state.ParsedAddress.Port,
CipPath: state.ParsedAddress.CipPath,
LibplctagPlcAttribute: state.Profile.LibplctagPlcAttribute,
TagName: _options.Probe.ProbeTagPath!,
Timeout: _options.Probe.Timeout);
IAbCipTagRuntime? probeRuntime = null; IAbCipTagRuntime? probeRuntime = null;
while (!ct.IsCancellationRequested) while (!ct.IsCancellationRequested)
@@ -336,9 +349,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
{ {
break; break;
} }
catch catch (Exception ex)
{ {
// Wire / init error — tear down the probe runtime so the next tick re-creates it. // Wire / init error — tear down the probe runtime so the next tick re-creates it.
// Log at debug because a wedged device produces one per tick; the
// OnHostStatusChanged event is the persistent record once the state transitions.
_logger.LogDebug(ex,
"AbCip probe tick failed for driver {DriverInstanceId} device {Device}",
_driverInstanceId, state.Options.HostAddress);
try { probeRuntime?.Dispose(); } catch { } try { probeRuntime?.Dispose(); } catch { }
probeRuntime = null; probeRuntime = null;
state.ProbeInitialized = false; state.ProbeInitialized = false;
@@ -402,7 +420,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
// Task #194 — plan the batch: members of the same parent UDT get collapsed into one // Task #194 — plan the batch: members of the same parent UDT get collapsed into one
// whole-UDT read + in-memory member decode; every other reference falls back to the // whole-UDT read + in-memory member decode; every other reference falls back to the
// per-tag path that's been here since PR 3. Planner is a pure function over the // per-tag read path. Planner is a pure function over the
// current tag map; BOOL/String/Structure members stay on the fallback path because // current tag map; BOOL/String/Structure members stay on the fallback path because
// declaration-only offsets can't place them under Logix alignment rules. Whole-UDT // declaration-only offsets can't place them under Logix alignment rules. Whole-UDT
// grouping is itself gated behind EnableDeclarationOnlyUdtGrouping — Studio 5000 may // grouping is itself gated behind EnableDeclarationOnlyUdtGrouping — Studio 5000 may
@@ -460,6 +478,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
AbCipStatusMapper.MapLibplctagStatus(status), null, now); AbCipStatusMapper.MapLibplctagStatus(status), null, now);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead,
$"libplctag status {status} reading {reference}"); $"libplctag status {status} reading {reference}");
_logger.LogWarning(
"AbCip read returned non-zero libplctag status {LibplctagStatus} for tag {Tag} on device {Device}; " +
"evicting cached runtime so next call re-creates it",
status, reference, def.DeviceHostAddress);
return; return;
} }
@@ -480,6 +502,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
results[fb.OriginalIndex] = new DataValueSnapshot(null, results[fb.OriginalIndex] = new DataValueSnapshot(null,
AbCipStatusMapper.BadCommunicationError, null, now); AbCipStatusMapper.BadCommunicationError, null, now);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message);
_logger.LogWarning(ex,
"AbCip read transport exception for tag {Tag} on device {Device}",
reference, def.DeviceHostAddress);
} }
} }
@@ -514,6 +539,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
StampGroupStatus(group, results, now, mapped); StampGroupStatus(group, results, now, mapped);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead,
$"libplctag status {status} reading UDT {group.ParentName}"); $"libplctag status {status} reading UDT {group.ParentName}");
_logger.LogWarning(
"AbCip whole-UDT read returned non-zero libplctag status {LibplctagStatus} for parent {Parent} " +
"on device {Device}; {MemberCount} member values stamped with mapped status",
status, group.ParentName, parent.DeviceHostAddress, group.Members.Count);
return; return;
} }
@@ -533,6 +562,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
EvictRuntime(device, parent.Name); // Driver.AbCip-010 EvictRuntime(device, parent.Name); // Driver.AbCip-010
StampGroupStatus(group, results, now, AbCipStatusMapper.BadCommunicationError); StampGroupStatus(group, results, now, AbCipStatusMapper.BadCommunicationError);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message);
_logger.LogWarning(ex,
"AbCip whole-UDT read transport exception for parent {Parent} on device {Device}",
group.ParentName, parent.DeviceHostAddress);
} }
} }
@@ -605,6 +637,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
{ {
EvictRuntime(device, def.Name); // Driver.AbCip-010 EvictRuntime(device, def.Name); // Driver.AbCip-010
results[i] = new WriteResult(AbCipStatusMapper.MapLibplctagStatus(status)); results[i] = new WriteResult(AbCipStatusMapper.MapLibplctagStatus(status));
_logger.LogWarning(
"AbCip write returned non-zero libplctag status {LibplctagStatus} for tag {Tag} on device {Device}; " +
"evicting cached runtime so next call re-creates it",
status, w.FullReference, def.DeviceHostAddress);
} }
else else
{ {
@@ -621,22 +657,34 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
// Type/protocol error — not a transport fault; don't evict the handle. // Type/protocol error — not a transport fault; don't evict the handle.
results[i] = new WriteResult(AbCipStatusMapper.BadNotSupported); results[i] = new WriteResult(AbCipStatusMapper.BadNotSupported);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message);
_logger.LogWarning(nse,
"AbCip write not supported for tag {Tag} on device {Device}",
w.FullReference, def.DeviceHostAddress);
} }
catch (FormatException fe) catch (FormatException fe)
{ {
// Value conversion error — not a transport fault; don't evict. // Value conversion error — not a transport fault; don't evict.
results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, fe.Message); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, fe.Message);
_logger.LogWarning(fe,
"AbCip write value-conversion error for tag {Tag} on device {Device}",
w.FullReference, def.DeviceHostAddress);
} }
catch (InvalidCastException ice) catch (InvalidCastException ice)
{ {
results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ice.Message); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ice.Message);
_logger.LogWarning(ice,
"AbCip write type-cast error for tag {Tag} on device {Device}",
w.FullReference, def.DeviceHostAddress);
} }
catch (OverflowException oe) catch (OverflowException oe)
{ {
results[i] = new WriteResult(AbCipStatusMapper.BadOutOfRange); results[i] = new WriteResult(AbCipStatusMapper.BadOutOfRange);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, oe.Message); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, oe.Message);
_logger.LogWarning(oe,
"AbCip write value out of range for tag {Tag} on device {Device}",
w.FullReference, def.DeviceHostAddress);
} }
catch (Exception ex) catch (Exception ex)
{ {
@@ -644,6 +692,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
EvictRuntime(device, def.Name); // Driver.AbCip-010 EvictRuntime(device, def.Name); // Driver.AbCip-010
results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError); results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message);
_logger.LogWarning(ex,
"AbCip write transport exception for tag {Tag} on device {Device}",
w.FullReference, def.DeviceHostAddress);
} }
} }
@@ -698,13 +749,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
{ {
if (device.ParentRuntimes.TryGetValue(parentTagName, out var existing)) return existing; if (device.ParentRuntimes.TryGetValue(parentTagName, out var existing)) return existing;
var runtime = _tagFactory.Create(new AbCipTagCreateParams( var runtime = _tagFactory.Create(device.BuildCreateParams(parentTagName, _options.Timeout));
Gateway: device.ParsedAddress.Gateway,
Port: device.ParsedAddress.Port,
CipPath: device.ParsedAddress.CipPath,
LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute,
TagName: parentTagName,
Timeout: _options.Timeout));
try try
{ {
await runtime.InitializeAsync(ct).ConfigureAwait(false); await runtime.InitializeAsync(ct).ConfigureAwait(false);
@@ -736,13 +781,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
?? throw new InvalidOperationException( ?? throw new InvalidOperationException(
$"AbCip tag '{def.Name}' has malformed TagPath '{def.TagPath}'."); $"AbCip tag '{def.Name}' has malformed TagPath '{def.TagPath}'.");
var runtime = _tagFactory.Create(new AbCipTagCreateParams( var runtime = _tagFactory.Create(device.BuildCreateParams(parsed.ToLibplctagName(), _options.Timeout));
Gateway: device.ParsedAddress.Gateway,
Port: device.ParsedAddress.Port,
CipPath: device.ParsedAddress.CipPath,
LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute,
TagName: parsed.ToLibplctagName(),
Timeout: _options.Timeout));
try try
{ {
await runtime.InitializeAsync(ct).ConfigureAwait(false); await runtime.InitializeAsync(ct).ConfigureAwait(false);
@@ -850,13 +889,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
if (_options.EnableControllerBrowse && _devices.TryGetValue(device.HostAddress, out var state)) if (_options.EnableControllerBrowse && _devices.TryGetValue(device.HostAddress, out var state))
{ {
using var enumerator = _enumeratorFactory.Create(); using var enumerator = _enumeratorFactory.Create();
var deviceParams = new AbCipTagCreateParams( var deviceParams = state.BuildCreateParams("@tags", _options.Timeout);
Gateway: state.ParsedAddress.Gateway,
Port: state.ParsedAddress.Port,
CipPath: state.ParsedAddress.CipPath,
LibplctagPlcAttribute: state.Profile.LibplctagPlcAttribute,
TagName: "@tags",
Timeout: _options.Timeout);
IAddressSpaceBuilder? discoveredFolder = null; IAddressSpaceBuilder? discoveredFolder = null;
await foreach (var discovered in enumerator.EnumerateAsync(deviceParams, cancellationToken) await foreach (var discovered in enumerator.EnumerateAsync(deviceParams, cancellationToken)
@@ -966,6 +999,23 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
public SemaphoreSlim GetRmwLock(string parentTagName) => public SemaphoreSlim GetRmwLock(string parentTagName) =>
_rmwLocks.GetOrAdd(parentTagName, _ => new SemaphoreSlim(1, 1)); _rmwLocks.GetOrAdd(parentTagName, _ => new SemaphoreSlim(1, 1));
/// <summary>
/// Driver.AbCip-013 — compute the effective <see cref="AbCipTagCreateParams"/> for a
/// tag on this device. Combines the per-device options
/// (<see cref="AbCipDeviceOptions.AllowPacking"/>,
/// <see cref="AbCipDeviceOptions.ConnectionSize"/>) with the family profile defaults
/// so the wire layer sees one place that resolves both.
/// </summary>
public AbCipTagCreateParams BuildCreateParams(string tagName, TimeSpan timeout) => new(
Gateway: ParsedAddress.Gateway,
Port: ParsedAddress.Port,
CipPath: ParsedAddress.CipPath,
LibplctagPlcAttribute: Profile.LibplctagPlcAttribute,
TagName: tagName,
Timeout: timeout,
AllowPacking: Options.AllowPacking ?? Profile.SupportsRequestPacking,
ConnectionSize: Options.ConnectionSize ?? Profile.DefaultConnectionSize);
public void DisposeHandles() public void DisposeHandles()
{ {
foreach (var r in Runtimes.Values) r.Dispose(); foreach (var r in Runtimes.Values) r.Dispose();
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs
+5 -1
View File
@@ -51,7 +51,9 @@ public static class AbCipDriverFactoryExtensions
$"AB CIP config for '{driverInstanceId}' has a device missing HostAddress"), $"AB CIP config for '{driverInstanceId}' has a device missing HostAddress"),
PlcFamily: ParseEnum<AbCipPlcFamily>(d.PlcFamily, "device", driverInstanceId, "PlcFamily", PlcFamily: ParseEnum<AbCipPlcFamily>(d.PlcFamily, "device", driverInstanceId, "PlcFamily",
fallback: AbCipPlcFamily.ControlLogix), fallback: AbCipPlcFamily.ControlLogix),
DeviceName: d.DeviceName))] DeviceName: d.DeviceName,
AllowPacking: d.AllowPacking,
ConnectionSize: d.ConnectionSize))]
: [], : [],
Tags = dto.Tags is { Count: > 0 } Tags = dto.Tags is { Count: > 0 }
? [.. dto.Tags.Select(t => BuildTag(t, driverInstanceId))] ? [.. dto.Tags.Select(t => BuildTag(t, driverInstanceId))]
@@ -133,6 +135,8 @@ public static class AbCipDriverFactoryExtensions
public string? HostAddress { get; init; } public string? HostAddress { get; init; }
public string? PlcFamily { get; init; } public string? PlcFamily { get; init; }
public string? DeviceName { get; init; } public string? DeviceName { get; init; }
public bool? AllowPacking { get; init; }
public int? ConnectionSize { get; init; }
} }
internal sealed class AbCipTagDto internal sealed class AbCipTagDto
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverOptions.cs
+28 -7
View File
@@ -18,7 +18,11 @@ public sealed class AbCipDriverOptions
/// </summary> /// </summary>
public IReadOnlyList<AbCipDeviceOptions> Devices { get; init; } = []; public IReadOnlyList<AbCipDeviceOptions> Devices { get; init; } = [];
/// <summary>Pre-declared tag map across all devices — AB discovery lands in PR 5.</summary> /// <summary>
/// Pre-declared tag map across all devices. Pre-declared tags always emit during
/// discovery; opt in to controller-side discovery via
/// <see cref="EnableControllerBrowse"/>.
/// </summary>
public IReadOnlyList<AbCipTagDefinition> Tags { get; init; } = []; public IReadOnlyList<AbCipTagDefinition> Tags { get; init; } = [];
/// <summary>Per-device probe settings. Falls back to defaults when omitted.</summary> /// <summary>Per-device probe settings. Falls back to defaults when omitted.</summary>
@@ -78,13 +82,28 @@ public sealed class AbCipDriverOptions
/// initialization rather than silently connecting to nothing. /// initialization rather than silently connecting to nothing.
/// </summary> /// </summary>
/// <param name="HostAddress">Canonical <c>ab://gateway[:port]/cip-path</c> string.</param> /// <param name="HostAddress">Canonical <c>ab://gateway[:port]/cip-path</c> string.</param>
/// <param name="PlcFamily">Which per-family profile to apply. Determines ConnectionSize, /// <param name="PlcFamily">Which per-family profile to apply. Determines the family
/// request-packing support, unconnected-only hint, and other quirks.</param> /// <c>AllowPacking</c> default, <c>ConnectionSize</c> default, unconnected-only hint, and
/// other quirks; per-device overrides via <see cref="AllowPacking"/> and
/// <see cref="ConnectionSize"/> take precedence when set.</param>
/// <param name="DeviceName">Optional display label for Admin UI. Falls back to <see cref="HostAddress"/>.</param> /// <param name="DeviceName">Optional display label for Admin UI. Falls back to <see cref="HostAddress"/>.</param>
/// <param name="AllowPacking">Driver.AbCip-013 — per-device override for CIP request-packing
/// (firmware 20+). <c>null</c> (the default) inherits the family profile's
/// <c>SupportsRequestPacking</c>; set explicitly to opt a single device in or out without
/// touching every other device on the same family.</param>
/// <param name="ConnectionSize">Driver.AbCip-013 — per-device override for the Forward Open
/// ConnectionSize (Large Forward Open packet size in bytes). <c>null</c> inherits the family
/// profile's <c>DefaultConnectionSize</c>. Honoured by the driver layer; the underlying
/// libplctag 1.5.2 wrapper has no direct <c>ConnectionSize</c> property, so the value is
/// plumbed through <see cref="AbCipTagCreateParams"/> for forward-compat with future wrapper
/// versions or a custom tag-attribute path; current builds use the family profile default at
/// the wire layer regardless.</param>
public sealed record AbCipDeviceOptions( public sealed record AbCipDeviceOptions(
string HostAddress, string HostAddress,
AbCipPlcFamily PlcFamily = AbCipPlcFamily.ControlLogix, AbCipPlcFamily PlcFamily = AbCipPlcFamily.ControlLogix,
string? DeviceName = null); string? DeviceName = null,
bool? AllowPacking = null,
int? ConnectionSize = null);
/// <summary> /// <summary>
/// One AB-backed OPC UA variable. Mirrors the <c>ModbusTagDefinition</c> shape. /// One AB-backed OPC UA variable. Mirrors the <c>ModbusTagDefinition</c> shape.
@@ -149,9 +168,11 @@ public sealed class AbCipProbeOptions
public TimeSpan Timeout { get; init; } = TimeSpan.FromSeconds(2); public TimeSpan Timeout { get; init; } = TimeSpan.FromSeconds(2);
/// <summary> /// <summary>
/// Tag path used for the probe. If null, the driver attempts to read a default /// Tag path used for the probe. When <see cref="Enabled"/> is <c>true</c> but this is
/// system tag (PR 8 wires this up — the choice is family-dependent, e.g. /// <c>null</c>/blank, the driver logs a warning and runs no probe loops (Driver.AbCip-011);
/// <c>@raw_cpu_type</c> on ControlLogix or a user-configured probe tag on Micro800). /// <c>GetHostStatuses()</c> will then report every device as <c>Unknown</c>. A family-default
/// system-tag fallback (e.g. <c>@raw_cpu_type</c> on ControlLogix) is a deferred follow-up;
/// today an operator opting into the probe must supply a tag path explicitly.
/// </summary> /// </summary>
public string? ProbeTagPath { get; init; } public string? ProbeTagPath { get; init; }
} }
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagPath.cs
+3 -2
View File
@@ -9,8 +9,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip;
/// attribute consumes. /// attribute consumes.
/// </summary> /// </summary>
/// <remarks> /// <remarks>
/// Scope + members + subscripts are captured structurally so PR 6 (UDT support) can walk /// Scope + members + subscripts are captured structurally so UDT support can walk the
/// the path against a cached template without re-parsing. <see cref="BitIndex"/> is /// path against a cached template (see <see cref="AbCipTemplateCache"/>) without
/// re-parsing. <see cref="BitIndex"/> is
/// non-null only when the trailing segment is a decimal integer between 0 and 31 that /// non-null only when the trailing segment is a decimal integer between 0 and 31 that
/// parses as a bit-selector — this is the <c>.N</c> syntax documented in the Logix 5000 /// parses as a bit-selector — this is the <c>.N</c> syntax documented in the Logix 5000
/// General Instructions Reference §Tags, and it applies only to DINT-typed parents. The /// General Instructions Reference §Tags, and it applies only to DINT-typed parents. The
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTemplateCache.cs
+6 -5
View File
@@ -9,9 +9,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip;
/// <c>ReinitializeAsync</c>. /// <c>ReinitializeAsync</c>.
/// </summary> /// </summary>
/// <remarks> /// <remarks>
/// Template shape read (CIP Template Object class 0x6C, <c>GetAttributeList</c> + /// Templates are decoded by <see cref="CipTemplateObjectDecoder"/> (CIP Template Object
/// <c>Read Template</c>) lands with PR 6. This class ships the cache surface so PR 6 can /// class 0x6C, <c>GetAttributeList</c> + <c>Read Template</c>); the live reader is
/// drop the decoder in without reshaping any caller code. /// <see cref="LibplctagTemplateReader"/>, and <see cref="AbCipDriver.FetchUdtShapeAsync"/>
/// populates this cache on first fetch.
/// </remarks> /// </remarks>
public sealed class AbCipTemplateCache public sealed class AbCipTemplateCache
{ {
@@ -36,8 +37,8 @@ public sealed class AbCipTemplateCache
/// <summary> /// <summary>
/// Decoded shape of one Logix UDT — member list + each member's offset + type. Populated /// Decoded shape of one Logix UDT — member list + each member's offset + type. Populated
/// by PR 6's Template Object reader. At PR 5 time this is the cache's value type only; /// by <see cref="CipTemplateObjectDecoder.Decode"/> from a Template Object response buffer
/// no reader writes to it yet. /// read via <see cref="LibplctagTemplateReader"/>.
/// </summary> /// </summary>
/// <param name="TypeName">UDT name as reported by the Template Object.</param> /// <param name="TypeName">UDT name as reported by the Template Object.</param>
/// <param name="TotalSize">Bytes the UDT occupies in a whole-UDT read buffer.</param> /// <param name="TotalSize">Bytes the UDT occupies in a whole-UDT read buffer.</param>
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagEnumerator.cs
+9 -9
View File
@@ -3,11 +3,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip;
/// <summary> /// <summary>
/// Swappable scanner that walks a controller's symbol table (via libplctag's /// Swappable scanner that walks a controller's symbol table (via libplctag's
/// <c>@tags</c> pseudo-tag or the CIP Symbol Object class 0x6B) and yields the tags it /// <c>@tags</c> pseudo-tag or the CIP Symbol Object class 0x6B) and yields the tags it
/// finds. Defaults to <see cref="EmptyAbCipTagEnumeratorFactory"/> which returns no /// finds. Production default is <see cref="LibplctagTagEnumeratorFactory"/> which speaks
/// controller-side tags — the full <c>@tags</c> decoder lands as a follow-up PR once /// to the live controller; <see cref="EmptyAbCipTagEnumeratorFactory"/> is available for
/// libplctag 1.5.2 either gains <c>TagInfoPlcMapper</c> upstream or we ship our own /// tests / strict-config deployments where only pre-declared tags should appear.
/// <c>IPlcMapper</c> for the Symbol Object byte layout (tracked via follow-up task; PR 5
/// ships the abstraction + pre-declared-tag emission).
/// </summary> /// </summary>
public interface IAbCipTagEnumerator : IDisposable public interface IAbCipTagEnumerator : IDisposable
{ {
@@ -30,7 +28,8 @@ public interface IAbCipTagEnumeratorFactory
/// <param name="Name">Logix symbolic name as returned by the Symbol Object.</param> /// <param name="Name">Logix symbolic name as returned by the Symbol Object.</param>
/// <param name="ProgramScope">Program name if the tag is program-scoped; <c>null</c> for controller scope.</param> /// <param name="ProgramScope">Program name if the tag is program-scoped; <c>null</c> for controller scope.</param>
/// <param name="DataType">Detected data type; <see cref="AbCipDataType.Structure"/> when the tag /// <param name="DataType">Detected data type; <see cref="AbCipDataType.Structure"/> when the tag
/// is UDT-typed — the UDT shape lookup + per-member expansion ship with PR 6.</param> /// is UDT-typed — per-member expansion runs against the cached
/// <see cref="AbCipUdtShape"/> via <see cref="AbCipTemplateCache"/>.</param>
/// <param name="ReadOnly"><c>true</c> when the Symbol Object's External Access attribute forbids writes.</param> /// <param name="ReadOnly"><c>true</c> when the Symbol Object's External Access attribute forbids writes.</param>
/// <param name="IsSystemTag">Hint from the enumerator that this is a system / infrastructure tag; /// <param name="IsSystemTag">Hint from the enumerator that this is a system / infrastructure tag;
/// the driver applies <see cref="AbCipSystemTagFilter"/> on top so the enumerator is not the /// the driver applies <see cref="AbCipSystemTagFilter"/> on top so the enumerator is not the
@@ -43,9 +42,10 @@ public sealed record AbCipDiscoveredTag(
bool IsSystemTag = false); bool IsSystemTag = false);
/// <summary> /// <summary>
/// Default production enumerator — currently returns an empty sequence. The real <c>@tags</c> /// No-op enumerator returning an empty sequence. Useful for tests + strict-config
/// walk lands as a follow-up PR. Documented in <c>driver-specs.md §3</c> as the gap the /// deployments where <see cref="AbCipDriverOptions.EnableControllerBrowse"/> is set but the
/// Symbol Object walker closes. /// operator wants only pre-declared tags to surface. Production drivers use
/// <see cref="LibplctagTagEnumeratorFactory"/> instead.
/// </summary> /// </summary>
internal sealed class EmptyAbCipTagEnumerator : IAbCipTagEnumerator internal sealed class EmptyAbCipTagEnumerator : IAbCipTagEnumerator
{ {
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs
+10 -1
View File
@@ -65,10 +65,19 @@ public interface IAbCipTagFactory
/// <param name="LibplctagPlcAttribute">libplctag <c>plc=...</c> attribute, per family profile.</param> /// <param name="LibplctagPlcAttribute">libplctag <c>plc=...</c> attribute, per family profile.</param>
/// <param name="TagName">Logix symbolic tag name as emitted by <see cref="AbCipTagPath.ToLibplctagName"/>.</param> /// <param name="TagName">Logix symbolic tag name as emitted by <see cref="AbCipTagPath.ToLibplctagName"/>.</param>
/// <param name="Timeout">libplctag operation timeout (applies to Initialize / Read / Write).</param> /// <param name="Timeout">libplctag operation timeout (applies to Initialize / Read / Write).</param>
/// <param name="AllowPacking">CIP request-packing flag — combines the per-device override (if
/// any) with the family profile's <c>SupportsRequestPacking</c>. Forwarded to the libplctag
/// <c>Tag.AllowPacking</c> property (Driver.AbCip-013).</param>
/// <param name="ConnectionSize">Forward Open ConnectionSize — combines the per-device override
/// (if any) with the family profile's <c>DefaultConnectionSize</c>. libplctag 1.5.2 has no
/// direct <c>ConnectionSize</c> property; the value is plumbed for forward-compat with future
/// wrappers / a custom tag-attribute path (Driver.AbCip-013).</param>
public sealed record AbCipTagCreateParams( public sealed record AbCipTagCreateParams(
string Gateway, string Gateway,
int Port, int Port,
string CipPath, string CipPath,
string LibplctagPlcAttribute, string LibplctagPlcAttribute,
string TagName, string TagName,
TimeSpan Timeout); TimeSpan Timeout,
bool AllowPacking = true,
int ConnectionSize = 4002);
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagEnumerator.cs
+3 -3
View File
@@ -13,9 +13,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip;
/// tag name is <c>@tags</c>. The decoder walks the concatenated entries + emits /// tag name is <c>@tags</c>. The decoder walks the concatenated entries + emits
/// <see cref="AbCipDiscoveredTag"/> records matching our driver surface.</para> /// <see cref="AbCipDiscoveredTag"/> records matching our driver surface.</para>
/// ///
/// <para>Task #178 closed the stub gap from PR 5 — <see cref="EmptyAbCipTagEnumerator"/> /// <para><see cref="EmptyAbCipTagEnumerator"/> remains available for tests that don't want
/// is still available for tests that don't want to touch the native library, but the /// to touch the native library; the production factory default
/// production factory default now wires this implementation in.</para> /// (<see cref="LibplctagTagEnumeratorFactory"/>) wires this implementation in.</para>
/// </remarks> /// </remarks>
internal sealed class LibplctagTagEnumerator : IAbCipTagEnumerator internal sealed class LibplctagTagEnumerator : IAbCipTagEnumerator
{ {
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs
+13 -1
View File
@@ -23,7 +23,15 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime
Protocol = Protocol.ab_eip, Protocol = Protocol.ab_eip,
Name = p.TagName, Name = p.TagName,
Timeout = p.Timeout, Timeout = p.Timeout,
// Driver.AbCip-013 — honour the per-device or family-default AllowPacking knob so
// operators can disable CIP request-packing for older firmware or a single device.
AllowPacking = p.AllowPacking,
}; };
// ConnectionSize is captured on AbCipTagCreateParams for forward-compat (driver-specs.md
// exposes it as a per-device option) but libplctag.NET 1.5.2 has no direct Tag property
// for it. Until the wrapper exposes one (or we ship a custom tag-attribute path), the
// family profile DefaultConnectionSize is what the underlying CIP Forward Open
// negotiates with — Driver.AbCip-013.
} }
public Task InitializeAsync(CancellationToken cancellationToken) => _tag.InitializeAsync(cancellationToken); public Task InitializeAsync(CancellationToken cancellationToken) => _tag.InitializeAsync(cancellationToken);
@@ -108,7 +116,11 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime
_tag.SetInt32(0, Convert.ToInt32(value)); _tag.SetInt32(0, Convert.ToInt32(value));
break; break;
case AbCipDataType.Structure: case AbCipDataType.Structure:
throw new NotSupportedException("Whole-UDT writes land in PR 6."); // Whole-UDT writes are not implemented — operators address individual UDT
// members via dotted tag paths, which route per-member through the atomic
// encode cases above. A whole-UDT writer is a deferred follow-up.
throw new NotSupportedException(
"Whole-UDT writes are not supported — address individual member paths instead.");
default: default:
throw new NotSupportedException($"AbCipDataType {type} not writable."); throw new NotSupportedException($"AbCipDataType {type} not writable.");
} }
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTemplateReader.cs
+13
View File
@@ -8,6 +8,19 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip;
/// internally via a normal read call, + returns the raw byte buffer so /// internally via a normal read call, + returns the raw byte buffer so
/// <see cref="CipTemplateObjectDecoder"/> can decode it. /// <see cref="CipTemplateObjectDecoder"/> can decode it.
/// </summary> /// </summary>
/// <remarks>
/// Driver.AbCip-012 — by design each <c>FetchUdtShapeAsync</c> call creates one fresh
/// <see cref="Tag"/>, pays one CIP connection handshake, reads, and disposes. Per-type
/// connect cost is accepted because (a) template reads are a low-frequency discovery path
/// (one-shot per UDT type, then the decoded shape is cached in
/// <see cref="AbCipTemplateCache"/>), (b) libplctag pools its underlying CIP connections per
/// gateway+path so the underlying TCP/EIP session is reused even when individual
/// <see cref="Tag"/> instances are torn down, and (c) pooling at the wrapper layer here would
/// buy a single Forward Open per device per discovery run — small relative to the rest of a
/// bulk-tag-walk discovery. If telemetry ever shows discovery latency dominated by
/// template-read connects, revisit by holding one <c>@udt</c>-capable <see cref="Tag"/> per
/// device for the duration of a discovery run.
/// </remarks>
internal sealed class LibplctagTemplateReader : IAbCipTemplateReader internal sealed class LibplctagTemplateReader : IAbCipTemplateReader
{ {
private Tag? _tag; private Tag? _tag;
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/PlcFamilies/AbCipPlcFamilyProfile.cs
+2 -1
View File
@@ -8,7 +8,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.PlcFamilies;
/// <remarks> /// <remarks>
/// Mirrors the shape of the Modbus driver's per-family profiles (DL205, Siemens S7, /// Mirrors the shape of the Modbus driver's per-family profiles (DL205, Siemens S7,
/// Mitsubishi MELSEC). ControlLogix is the baseline; each subsequent family is a delta. /// Mitsubishi MELSEC). ControlLogix is the baseline; each subsequent family is a delta.
/// Family-specific wire tests ship in PRs 9–12. /// Family-specific behaviour (ControlLogix / CompactLogix / Micro800 / GuardLogix) is
/// covered by the unit tests in <c>AbCipPlcFamilyTests</c>.
/// </remarks> /// </remarks>
public sealed record AbCipPlcFamilyProfile( public sealed record AbCipPlcFamilyProfile(
string LibplctagPlcAttribute, string LibplctagPlcAttribute,
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs
+101 -2
View File
@@ -1,3 +1,5 @@
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies;
@@ -14,6 +16,7 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
private readonly AbLegacyDriverOptions _options; private readonly AbLegacyDriverOptions _options;
private readonly string _driverInstanceId; private readonly string _driverInstanceId;
private readonly IAbLegacyTagFactory _tagFactory; private readonly IAbLegacyTagFactory _tagFactory;
private readonly ILogger<AbLegacyDriver> _logger;
private readonly PollGroupEngine _poll; private readonly PollGroupEngine _poll;
private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, AbLegacyTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, AbLegacyTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase);
@@ -29,12 +32,14 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
public event EventHandler<HostStatusChangedEventArgs>? OnHostStatusChanged; public event EventHandler<HostStatusChangedEventArgs>? OnHostStatusChanged;
public AbLegacyDriver(AbLegacyDriverOptions options, string driverInstanceId, public AbLegacyDriver(AbLegacyDriverOptions options, string driverInstanceId,
IAbLegacyTagFactory? tagFactory = null) IAbLegacyTagFactory? tagFactory = null,
ILogger<AbLegacyDriver>? logger = null)
{ {
ArgumentNullException.ThrowIfNull(options); ArgumentNullException.ThrowIfNull(options);
_options = options; _options = options;
_driverInstanceId = driverInstanceId; _driverInstanceId = driverInstanceId;
_tagFactory = tagFactory ?? new LibplctagLegacyTagFactory(); _tagFactory = tagFactory ?? new LibplctagLegacyTagFactory();
_logger = logger ?? NullLogger<AbLegacyDriver>.Instance;
_poll = new PollGroupEngine( _poll = new PollGroupEngine(
reader: ReadAsync, reader: ReadAsync,
onChange: (handle, tagRef, snapshot) => onChange: (handle, tagRef, snapshot) =>
@@ -92,6 +97,11 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
catch (Exception ex) catch (Exception ex)
{ {
_health = new DriverHealth(DriverState.Faulted, null, ex.Message); _health = new DriverHealth(DriverState.Faulted, null, ex.Message);
// Driver.AbLegacy-005 — structured log of the init failure so a field operator sees
// the exception in the rolling Serilog file rather than only as a transient Detail
// string on DriverHealth.
_logger.LogError(ex,
"AbLegacy driver initialise failed. Driver={DriverInstanceId}", _driverInstanceId);
// Tear down any probe loops and cached state that were created before the failure so // Tear down any probe loops and cached state that were created before the failure so
// that a caller who catches and abandons (rather than retrying via ReinitializeAsync) // that a caller who catches and abandons (rather than retrying via ReinitializeAsync)
// doesn't leave orphaned background tasks, CancellationTokenSources, and libplctag // doesn't leave orphaned background tasks, CancellationTokenSources, and libplctag
@@ -192,8 +202,23 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
AbLegacyStatusMapper.MapLibplctagStatus(status), null, now); AbLegacyStatusMapper.MapLibplctagStatus(status), null, now);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead,
$"libplctag status {status} reading {reference}"); $"libplctag status {status} reading {reference}");
// Driver.AbLegacy-005 — log the FIRST non-zero libplctag status per device so
// a field operator can correlate a comms problem with a structured log
// entry. Detail on DriverHealth is overwritten by the very next read; the
// log entry persists. Subsequent occurrences on the same device stay quiet so
// a permanently-bad PLC doesn't flood the rolling file.
if (!device.FirstNonZeroStatusLogged)
{
device.FirstNonZeroStatusLogged = true;
_logger.LogWarning(
"AbLegacy non-zero libplctag status. Driver={DriverInstanceId} Device={DeviceHostAddress} Reference={Reference} Status={Status}",
_driverInstanceId, def.DeviceHostAddress, reference, status);
}
continue; continue;
} }
// Healthy read — re-arm the per-device first-failure log so a future non-zero
// status logs again rather than being suppressed by an old flag from a prior outage.
device.FirstNonZeroStatusLogged = false;
results[i] = new DataValueSnapshot(value, AbLegacyStatusMapper.Good, now, now); results[i] = new DataValueSnapshot(value, AbLegacyStatusMapper.Good, now, now);
_health = new DriverHealth(DriverState.Healthy, now, null); _health = new DriverHealth(DriverState.Healthy, now, null);
@@ -313,6 +338,14 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
string.Equals(t.DeviceHostAddress, device.HostAddress, StringComparison.OrdinalIgnoreCase)); string.Equals(t.DeviceHostAddress, device.HostAddress, StringComparison.OrdinalIgnoreCase));
foreach (var tag in tagsForDevice) foreach (var tag in tagsForDevice)
{ {
// Driver.AbLegacy-013 (tracked follow-up) — PCCC files are inherently arrays of
// elements (a single N7 file is up to 256 words), but the current tag-definition
// surface only addresses one element. IsArray/ArrayDim are hard-wired false/null
// until multi-element addressing lands; tags that genuinely span a range have to
// be enumerated one element at a time today. This is consistent with the
// PR-staged scope documented in docs/v2/driver-specs.md (AbLegacy ships with thin
// array coverage); when array support is added, ArrayCount on the tag definition
// will flow through here as it already does on the Modbus driver.
deviceFolder.Variable(tag.Name, tag.Name, new DriverAttributeInfo( deviceFolder.Variable(tag.Name, tag.Name, new DriverAttributeInfo(
FullName: tag.Name, FullName: tag.Name,
DriverDataType: tag.DataType.ToDriverDataType(), DriverDataType: tag.DataType.ToDriverDataType(),
@@ -397,12 +430,38 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
state.HostState = newState; state.HostState = newState;
state.HostStateChangedUtc = DateTime.UtcNow; state.HostStateChangedUtc = DateTime.UtcNow;
} }
// Driver.AbLegacy-005 — structured log of every probe-driven transition. Operators can
// grep the rolling Serilog file for the device address to see when a PLC was last
// reachable. Downgrades to Stopped log as Warning; recoveries log as Information.
if (newState == HostState.Stopped)
_logger.LogWarning(
"AbLegacy probe transition. Driver={DriverInstanceId} Device={DeviceHostAddress} From={Old} To={New}",
_driverInstanceId, state.Options.HostAddress, old, newState);
else
_logger.LogInformation(
"AbLegacy probe transition. Driver={DriverInstanceId} Device={DeviceHostAddress} From={Old} To={New}",
_driverInstanceId, state.Options.HostAddress, old, newState);
OnHostStatusChanged?.Invoke(this, OnHostStatusChanged?.Invoke(this,
new HostStatusChangedEventArgs(state.Options.HostAddress, old, newState)); new HostStatusChangedEventArgs(state.Options.HostAddress, old, newState));
} }
// ---- IPerCallHostResolver ---- // ---- IPerCallHostResolver ----
/// <summary>
/// Map a full reference to the host string used as the resilience-pipeline breaker key.
/// Driver.AbLegacy-013 — the contract on <see cref="IPerCallHostResolver"/> requires that
/// implementations never throw on an unknown reference. The fallback chain is therefore:
/// <list type="number">
/// <item>Known tag → its <c>DeviceHostAddress</c>.</item>
/// <item>Unknown reference but devices configured → the first device's host address
/// (multi-device drivers degrade to single-host behaviour rather than failing).</item>
/// <item>Unknown reference and no devices configured → the driver instance id, which
/// the dispatch layer treats as the single-host key per the interface
/// documentation. Reaching this branch indicates a misconfigured driver (no
/// devices) so callers that want to surface that should validate
/// <see cref="DeviceCount"/> before relying on per-tag routing.</item>
/// </list>
/// </summary>
public string ResolveHost(string fullReference) public string ResolveHost(string fullReference)
{ {
if (_tagsByName.TryGetValue(fullReference, out var def)) if (_tagsByName.TryGetValue(fullReference, out var def))
@@ -549,7 +608,36 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
} }
} }
public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult(); /// <summary>
/// Driver.AbLegacy-011 — synchronous teardown. Mirrors the body of
/// <see cref="ShutdownAsync"/> but never wraps the async path in
/// <c>.AsTask().GetAwaiter().GetResult()</c>. The poll engine's <c>DisposeAsync</c> is
/// drained with a <c>ConfigureAwait(false)</c> awaiter so a captured single-threaded
/// <see cref="SynchronizationContext"/> can never be the resumption target — the
/// classic sync-over-async deadlock cannot occur. Any other awaitable cleanup is
/// translated to direct synchronous calls (cancel probe CTSs, dispose runtimes).
/// </summary>
public void Dispose()
{
// ValueTask.ConfigureAwait(false).GetAwaiter().GetResult() — drains the poll engine
// shutdown on the current thread without capturing the SynchronizationContext. The
// engine cancels every loop CTS up-front then either completes immediately (no
// subscriptions, common case for the test fixture) or awaits a bounded WhenAll on the
// already-shutting-down loop tasks. We swallow exceptions so a buggy poll-loop teardown
// can't poison the rest of the disposal chain.
try { _poll.DisposeAsync().ConfigureAwait(false).GetAwaiter().GetResult(); } catch { }
foreach (var state in _devices.Values)
{
try { state.ProbeCts?.Cancel(); } catch { }
state.ProbeCts?.Dispose();
state.ProbeCts = null;
state.DisposeRuntimes();
}
_devices.Clear();
_tagsByName.Clear();
_health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null);
}
public async ValueTask DisposeAsync() => await ShutdownAsync(CancellationToken.None).ConfigureAwait(false); public async ValueTask DisposeAsync() => await ShutdownAsync(CancellationToken.None).ConfigureAwait(false);
internal sealed class DeviceState( internal sealed class DeviceState(
@@ -626,6 +714,17 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
public CancellationTokenSource? ProbeCts { get; set; } public CancellationTokenSource? ProbeCts { get; set; }
public bool ProbeInitialized { get; set; } public bool ProbeInitialized { get; set; }
/// <summary>
/// Driver.AbLegacy-005 — per-device latch for the structured "first non-zero
/// libplctag status" log. Reset to <see langword="false"/> on a successful read so a
/// future outage re-fires the warning rather than being suppressed by a stale flag.
/// Concurrent readers on the same device may race the unlatched check + set, but the
/// worst case is a small finite number of duplicate warnings at outage onset (one per
/// racing reader) — which is preferable to either silently losing the first warning
/// or paying lock contention on the hot read path.
/// </summary>
public bool FirstNonZeroStatusLogged { get; set; }
public void DisposeRuntimes() public void DisposeRuntimes()
{ {
foreach (var r in Runtimes.Values) r.Dispose(); foreach (var r in Runtimes.Values) r.Dispose();
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs
+17 -3
View File
@@ -1,5 +1,6 @@
using System.Text.Json; using System.Text.Json;
using System.Text.Json.Serialization; using System.Text.Json.Serialization;
using Microsoft.Extensions.Logging;
using ZB.MOM.WW.OtOpcUa.Core.Hosting; using ZB.MOM.WW.OtOpcUa.Core.Hosting;
using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies;
@@ -15,13 +16,23 @@ public static class AbLegacyDriverFactoryExtensions
{ {
public const string DriverTypeName = "AbLegacy"; public const string DriverTypeName = "AbLegacy";
public static void Register(DriverFactoryRegistry registry) /// <summary>
/// Register the AbLegacy factory with the driver registry. The optional
/// <paramref name="loggerFactory"/> is captured at registration time and used to
/// construct an <see cref="ILogger{AbLegacyDriver}"/> per driver instance — without it,
/// the driver runs with the null logger (existing tests and standalone callers stay
/// unchanged). Mirrors the Modbus driver registration pattern.
/// </summary>
public static void Register(DriverFactoryRegistry registry, ILoggerFactory? loggerFactory = null)
{ {
ArgumentNullException.ThrowIfNull(registry); ArgumentNullException.ThrowIfNull(registry);
registry.Register(DriverTypeName, CreateInstance); registry.Register(DriverTypeName, (id, json) => CreateInstance(id, json, loggerFactory));
} }
internal static AbLegacyDriver CreateInstance(string driverInstanceId, string driverConfigJson) internal static AbLegacyDriver CreateInstance(string driverInstanceId, string driverConfigJson)
=> CreateInstance(driverInstanceId, driverConfigJson, loggerFactory: null);
internal static AbLegacyDriver CreateInstance(string driverInstanceId, string driverConfigJson, ILoggerFactory? loggerFactory)
{ {
ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId);
ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson);
@@ -63,7 +74,10 @@ public static class AbLegacyDriverFactoryExtensions
Timeout = TimeSpan.FromMilliseconds(dto.TimeoutMs ?? 2_000), Timeout = TimeSpan.FromMilliseconds(dto.TimeoutMs ?? 2_000),
}; };
return new AbLegacyDriver(options, driverInstanceId); return new AbLegacyDriver(
options, driverInstanceId,
tagFactory: null,
logger: loggerFactory?.CreateLogger<AbLegacyDriver>());
} }
private static T ParseEnum<T>(string? raw, string driverInstanceId, string field, private static T ParseEnum<T>(string? raw, string driverInstanceId, string field,
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasAlarmProjection.cs
+18 -6
View File
@@ -1,3 +1,5 @@
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS; namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS;
@@ -21,14 +23,16 @@ internal sealed class FocasAlarmProjection : IAsyncDisposable
{ {
private readonly FocasDriver _driver; private readonly FocasDriver _driver;
private readonly TimeSpan _pollInterval; private readonly TimeSpan _pollInterval;
private readonly ILogger _logger;
private readonly Dictionary<long, Subscription> _subs = new(); private readonly Dictionary<long, Subscription> _subs = new();
private readonly Lock _subsLock = new(); private readonly Lock _subsLock = new();
private long _nextId; private long _nextId;
public FocasAlarmProjection(FocasDriver driver, TimeSpan pollInterval) public FocasAlarmProjection(FocasDriver driver, TimeSpan pollInterval, ILogger? logger = null)
{ {
_driver = driver; _driver = driver;
_pollInterval = pollInterval; _pollInterval = pollInterval;
_logger = logger ?? NullLogger.Instance;
} }
public Task<IAlarmSubscriptionHandle> SubscribeAsync( public Task<IAlarmSubscriptionHandle> SubscribeAsync(
@@ -58,8 +62,10 @@ internal sealed class FocasAlarmProjection : IAsyncDisposable
{ {
if (!_subs.Remove(h.Id, out sub)) return; if (!_subs.Remove(h.Id, out sub)) return;
} }
try { sub.Cts.Cancel(); } catch { } try { sub.Cts.Cancel(); }
try { await sub.Loop.ConfigureAwait(false); } catch { } catch (Exception ex) { _logger.LogDebug(ex, "Cancelling alarm-subscription CTS failed"); }
try { await sub.Loop.ConfigureAwait(false); }
catch (Exception ex) { _logger.LogDebug(ex, "Awaiting alarm-subscription loop failed during unsubscribe"); }
sub.Cts.Dispose(); sub.Cts.Dispose();
} }
@@ -78,8 +84,10 @@ internal sealed class FocasAlarmProjection : IAsyncDisposable
lock (_subsLock) { snap = [.. _subs.Values]; _subs.Clear(); } lock (_subsLock) { snap = [.. _subs.Values]; _subs.Clear(); }
foreach (var sub in snap) foreach (var sub in snap)
{ {
try { sub.Cts.Cancel(); } catch { } try { sub.Cts.Cancel(); }
try { await sub.Loop.ConfigureAwait(false); } catch { } catch (Exception ex) { _logger.LogDebug(ex, "Cancelling alarm-subscription CTS during dispose failed"); }
try { await sub.Loop.ConfigureAwait(false); }
catch (Exception ex) { _logger.LogDebug(ex, "Awaiting alarm-subscription loop during dispose failed"); }
sub.Cts.Dispose(); sub.Cts.Dispose();
} }
} }
@@ -136,7 +144,11 @@ internal sealed class FocasAlarmProjection : IAsyncDisposable
} }
} }
catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; } catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; }
catch { /* per-tick failures are non-fatal — next tick retries */ } catch (Exception ex)
{
/* per-tick failures are non-fatal — next tick retries */
_logger.LogDebug(ex, "FOCAS alarm-projection poll tick failed");
}
try { await Task.Delay(_pollInterval, ct).ConfigureAwait(false); } try { await Task.Delay(_pollInterval, ct).ConfigureAwait(false); }
catch (OperationCanceledException) { break; } catch (OperationCanceledException) { break; }
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs
+118 -20
View File
@@ -1,3 +1,5 @@
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS; namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS;
@@ -22,8 +24,14 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
private readonly string _driverInstanceId; private readonly string _driverInstanceId;
private readonly IFocasClientFactory _clientFactory; private readonly IFocasClientFactory _clientFactory;
private readonly PollGroupEngine _poll; private readonly PollGroupEngine _poll;
private readonly ILogger<FocasDriver> _logger;
private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, FocasTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, FocasTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase);
// Per-tag-name cache of the FocasAddress parsed once at InitializeAsync. ReadAsync /
// WriteAsync look up the pre-parsed value instead of re-parsing tag.Address on every hot
// call — resolves Driver.FOCAS-008.
private readonly Dictionary<string, FocasAddress> _parsedAddressesByTagName =
new(StringComparer.OrdinalIgnoreCase);
private FocasAlarmProjection? _alarmProjection; private FocasAlarmProjection? _alarmProjection;
// _health is read/written from multiple threads (ReadAsync, WriteAsync, ProbeLoopAsync). // _health is read/written from multiple threads (ReadAsync, WriteAsync, ProbeLoopAsync).
// Volatile.Read/Write ensures every thread sees the latest reference without a lock — the // Volatile.Read/Write ensures every thread sees the latest reference without a lock — the
@@ -35,12 +43,14 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
public event EventHandler<AlarmEventArgs>? OnAlarmEvent; public event EventHandler<AlarmEventArgs>? OnAlarmEvent;
public FocasDriver(FocasDriverOptions options, string driverInstanceId, public FocasDriver(FocasDriverOptions options, string driverInstanceId,
IFocasClientFactory? clientFactory = null) IFocasClientFactory? clientFactory = null,
ILogger<FocasDriver>? logger = null)
{ {
ArgumentNullException.ThrowIfNull(options); ArgumentNullException.ThrowIfNull(options);
_options = options; _options = options;
_driverInstanceId = driverInstanceId; _driverInstanceId = driverInstanceId;
_clientFactory = clientFactory ?? new Wire.WireFocasClientFactory(); _clientFactory = clientFactory ?? new Wire.WireFocasClientFactory();
_logger = logger ?? NullLogger<FocasDriver>.Instance;
_poll = new PollGroupEngine( _poll = new PollGroupEngine(
reader: ReadAsync, reader: ReadAsync,
onChange: (handle, tagRef, snapshot) => onChange: (handle, tagRef, snapshot) =>
@@ -82,6 +92,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
$"FOCAS tag '{tag.Name}' ({tag.Address}) rejected by capability matrix: {reason}"); $"FOCAS tag '{tag.Name}' ({tag.Address}) rejected by capability matrix: {reason}");
} }
_tagsByName[tag.Name] = tag; _tagsByName[tag.Name] = tag;
// Cache the parsed FocasAddress so ReadAsync / WriteAsync don't re-parse on every
// hot-path call (Driver.FOCAS-008). The address string has already been validated
// by FocasAddress.TryParse above; reusing the parsed record avoids per-tick allocs
// on subscription pollers.
_parsedAddressesByTagName[tag.Name] = parsed;
} }
if (_options.Probe.Enabled) if (_options.Probe.Enabled)
@@ -105,7 +120,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
} }
if (_options.AlarmProjection.Enabled) if (_options.AlarmProjection.Enabled)
_alarmProjection = new FocasAlarmProjection(this, _options.AlarmProjection.PollInterval); _alarmProjection = new FocasAlarmProjection(this, _options.AlarmProjection.PollInterval, _logger);
if (_options.FixedTree.Enabled) if (_options.FixedTree.Enabled)
{ {
@@ -143,19 +158,26 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
} }
foreach (var state in _devices.Values) foreach (var state in _devices.Values)
{ {
try { state.ProbeCts?.Cancel(); } catch { } // Cancel-then-dispose can race in tight shutdown loops; swallowing is intentional
// but we now log the cause so a noisy shutdown leaves a Debug trace
// (Driver.FOCAS-007).
try { state.ProbeCts?.Cancel(); }
catch (Exception ex) { _logger.LogDebug(ex, "Cancelling probe CTS for {Host} failed", state.Options.HostAddress); }
state.ProbeCts?.Dispose(); state.ProbeCts?.Dispose();
state.ProbeCts = null; state.ProbeCts = null;
try { state.RecycleCts?.Cancel(); } catch { } try { state.RecycleCts?.Cancel(); }
catch (Exception ex) { _logger.LogDebug(ex, "Cancelling recycle CTS for {Host} failed", state.Options.HostAddress); }
state.RecycleCts?.Dispose(); state.RecycleCts?.Dispose();
state.RecycleCts = null; state.RecycleCts = null;
try { state.FixedTreeCts?.Cancel(); } catch { } try { state.FixedTreeCts?.Cancel(); }
catch (Exception ex) { _logger.LogDebug(ex, "Cancelling fixed-tree CTS for {Host} failed", state.Options.HostAddress); }
state.FixedTreeCts?.Dispose(); state.FixedTreeCts?.Dispose();
state.FixedTreeCts = null; state.FixedTreeCts = null;
state.DisposeClient(); state.DisposeClient();
} }
_devices.Clear(); _devices.Clear();
_tagsByName.Clear(); _tagsByName.Clear();
_parsedAddressesByTagName.Clear();
Volatile.Write(ref _health, new DriverHealth(DriverState.Unknown, Volatile.Read(ref _health).LastSuccessfulRead, null)); Volatile.Write(ref _health, new DriverHealth(DriverState.Unknown, Volatile.Read(ref _health).LastSuccessfulRead, null));
} }
@@ -206,8 +228,14 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
try try
{ {
var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false); var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false);
var parsed = FocasAddress.TryParse(def.Address) // Parsed at InitializeAsync — defensive fallback re-parse only if the tag was
?? throw new InvalidOperationException($"FOCAS tag '{def.Name}' has malformed Address '{def.Address}'."); // somehow not seeded (shouldn't happen, but keeps the call total).
if (!_parsedAddressesByTagName.TryGetValue(def.Name, out var parsed))
{
parsed = FocasAddress.TryParse(def.Address)
?? throw new InvalidOperationException(
$"FOCAS tag '{def.Name}' has malformed Address '{def.Address}'.");
}
var (value, status) = await client.ReadAsync(parsed, def.DataType, cancellationToken).ConfigureAwait(false); var (value, status) = await client.ReadAsync(parsed, def.DataType, cancellationToken).ConfigureAwait(false);
results[i] = new DataValueSnapshot(value, status, now, now); results[i] = new DataValueSnapshot(value, status, now, now);
@@ -260,8 +288,12 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
try try
{ {
var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false); var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false);
var parsed = FocasAddress.TryParse(def.Address) if (!_parsedAddressesByTagName.TryGetValue(def.Name, out var parsed))
?? throw new InvalidOperationException($"FOCAS tag '{def.Name}' has malformed Address '{def.Address}'."); {
parsed = FocasAddress.TryParse(def.Address)
?? throw new InvalidOperationException(
$"FOCAS tag '{def.Name}' has malformed Address '{def.Address}'.");
}
var status = await client.WriteAsync(parsed, def.DataType, w.Value, cancellationToken).ConfigureAwait(false); var status = await client.WriteAsync(parsed, def.DataType, w.Value, cancellationToken).ConfigureAwait(false);
results[i] = new WriteResult(status); results[i] = new WriteResult(status);
} }
@@ -489,10 +521,35 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
try try
{ {
var client = await EnsureConnectedAsync(state, ct).ConfigureAwait(false); var client = await EnsureConnectedAsync(state, ct).ConfigureAwait(false);
success = await client.ProbeAsync(ct).ConfigureAwait(false); // Apply Probe.Timeout so a hung CNC socket gets cancelled at the configured
// budget rather than blocking until the OS TCP timeout (Driver.FOCAS-009).
// TimeSpan.Zero / negative means "no per-probe timeout" — fall back to the loop
// cancellation token unmodified.
var probeTimeout = _options.Probe.Timeout;
if (probeTimeout > TimeSpan.Zero)
{
using var linked = CancellationTokenSource.CreateLinkedTokenSource(ct);
linked.CancelAfter(probeTimeout);
success = await client.ProbeAsync(linked.Token).ConfigureAwait(false);
}
else
{
success = await client.ProbeAsync(ct).ConfigureAwait(false);
}
} }
catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; } catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; }
catch { /* connect-failure path already disposed + cleared the client */ } catch (OperationCanceledException ex)
{
// Per-probe timeout fired — the loop is still alive. Treat as a failed probe so
// the host state transitions to Stopped, and log so silent timeouts are visible.
_logger.LogDebug(ex, "FOCAS probe timed out for {Host} after {Timeout}",
state.Options.HostAddress, _options.Probe.Timeout);
}
catch (Exception ex)
{
/* connect-failure path already disposed + cleared the client */
_logger.LogDebug(ex, "FOCAS probe failed for {Host}", state.Options.HostAddress);
}
TransitionDeviceState(state, success ? HostState.Running : HostState.Stopped); TransitionDeviceState(state, success ? HostState.Running : HostState.Stopped);
@@ -542,8 +599,10 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
sys, [.. axes], [.. spindles], [.. spindleMaxRpms], caps); sys, [.. axes], [.. spindles], [.. spindleMaxRpms], caps);
} }
catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; } catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; }
catch catch (Exception ex)
{ {
_logger.LogDebug(ex, "FOCAS fixed-tree bootstrap failed for {Host} — retrying",
state.Options.HostAddress);
try { await Task.Delay(TimeSpan.FromSeconds(2), ct).ConfigureAwait(false); } try { await Task.Delay(TimeSpan.FromSeconds(2), ct).ConfigureAwait(false); }
catch (OperationCanceledException) { return; } catch (OperationCanceledException) { return; }
} }
@@ -559,7 +618,13 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
var loads = await client2.GetSpindleLoadsAsync(ct).ConfigureAwait(false); var loads = await client2.GetSpindleLoadsAsync(ct).ConfigureAwait(false);
for (var i = 0; i < loads.Count; i++) state.LastSpindleLoads[i] = loads[i]; for (var i = 0; i < loads.Count; i++) state.LastSpindleLoads[i] = loads[i];
} }
catch { /* first-tick poll will retry */ } catch (Exception ex)
{
/* first-tick poll will retry */
_logger.LogDebug(ex,
"FOCAS bootstrap spindle-loads prime failed for {Host} — first poll tick will retry",
state.Options.HostAddress);
}
} }
var programPollDue = DateTime.MinValue; var programPollDue = DateTime.MinValue;
@@ -591,7 +656,12 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
var loads = await client.GetServoLoadsAsync(ct).ConfigureAwait(false); var loads = await client.GetServoLoadsAsync(ct).ConfigureAwait(false);
PublishServoLoads(state, loads); PublishServoLoads(state, loads);
} }
catch { /* transient — next tick retries */ } catch (Exception ex)
{
/* transient — next tick retries */
_logger.LogDebug(ex, "FOCAS servo-loads poll failed for {Host}",
state.Options.HostAddress);
}
} }
if (cache.Capabilities.SpindleLoad) if (cache.Capabilities.SpindleLoad)
{ {
@@ -600,7 +670,12 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
var loads = await client.GetSpindleLoadsAsync(ct).ConfigureAwait(false); var loads = await client.GetSpindleLoadsAsync(ct).ConfigureAwait(false);
for (var i = 0; i < loads.Count; i++) state.LastSpindleLoads[i] = loads[i]; for (var i = 0; i < loads.Count; i++) state.LastSpindleLoads[i] = loads[i];
} }
catch { /* transient */ } catch (Exception ex)
{
/* transient */
_logger.LogDebug(ex, "FOCAS spindle-loads poll failed for {Host}",
state.Options.HostAddress);
}
} }
// Program-info poll runs on its own cadence — much slower than the axis // Program-info poll runs on its own cadence — much slower than the axis
@@ -615,7 +690,12 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
state.LastProgramInfo = program; state.LastProgramInfo = program;
if (firstAxisSnap is { } s) state.LastProgramAxisRef = s; if (firstAxisSnap is { } s) state.LastProgramAxisRef = s;
} }
catch { /* transient — next tick retries */ } catch (Exception ex)
{
/* transient — next tick retries */
_logger.LogDebug(ex, "FOCAS program-info poll failed for {Host}",
state.Options.HostAddress);
}
programPollDue = DateTime.UtcNow + programInterval; programPollDue = DateTime.UtcNow + programInterval;
} }
@@ -631,13 +711,23 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
var t = await client.GetTimerAsync(kind, ct).ConfigureAwait(false); var t = await client.GetTimerAsync(kind, ct).ConfigureAwait(false);
state.LastTimers[kind] = t; state.LastTimers[kind] = t;
} }
catch { /* per-kind failures are non-fatal */ } catch (Exception ex)
{
/* per-kind failures are non-fatal */
_logger.LogDebug(ex, "FOCAS timer poll failed for {Host} kind={Kind}",
state.Options.HostAddress, kind);
}
} }
timerPollDue = DateTime.UtcNow + timerInterval; timerPollDue = DateTime.UtcNow + timerInterval;
} }
} }
catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; } catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; }
catch { /* next tick retries — transient blips are expected */ } catch (Exception ex)
{
/* next tick retries — transient blips are expected */
_logger.LogDebug(ex, "FOCAS fixed-tree poll tick failed for {Host}",
state.Options.HostAddress);
}
try { await Task.Delay(_options.FixedTree.PollInterval, ct).ConfigureAwait(false); } try { await Task.Delay(_options.FixedTree.PollInterval, ct).ConfigureAwait(false); }
catch (OperationCanceledException) { break; } catch (OperationCanceledException) { break; }
@@ -801,7 +891,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
// reconnect because the goal is just to release the FWLIB handle slot; a // reconnect because the goal is just to release the FWLIB handle slot; a
// readable tick one probe cycle later is an acceptable cost. // readable tick one probe cycle later is an acceptable cost.
try { state.DisposeClient(); } try { state.DisposeClient(); }
catch { /* already disposed or race — next EnsureConnected recovers */ } catch (Exception ex)
{
/* already disposed or race — next EnsureConnected recovers */
_logger.LogDebug(ex, "FOCAS handle-recycle dispose failed for {Host}", state.Options.HostAddress);
}
} }
} }
@@ -858,7 +952,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
result.Add((state.Options.HostAddress, alarms)); result.Add((state.Options.HostAddress, alarms));
} }
catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; } catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; }
catch { /* surface a device-local fault on the next tick */ } catch (Exception ex)
{
/* surface a device-local fault on the next tick */
_logger.LogDebug(ex, "FOCAS alarm poll failed for {Host}", state.Options.HostAddress);
}
} }
return result; return result;
} }
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/IFocasClient.cs
+15 -9
View File
@@ -272,16 +272,22 @@ public sealed class UnimplementedFocasClientFactory : IFocasClientFactory
/// Well-known FOCAS alarm types from <c>fwlib32.h</c> <c>ALM_TYPE_*</c>. Narrow subset — /// Well-known FOCAS alarm types from <c>fwlib32.h</c> <c>ALM_TYPE_*</c>. Narrow subset —
/// the full list is ~15 types per model; these cover the universally-present categories. /// the full list is ~15 types per model; these cover the universally-present categories.
/// </summary> /// </summary>
/// <remarks>
/// Constants are typed <see cref="short"/> so they match the wire field width on
/// <c>cnc_rdalmmsg2</c> (and so <see cref="FocasAlarmProjection"/>'s <c>switch (short)</c>
/// statements compile against a matching type rather than relying on implicit int→short
/// narrowing on the constants).
/// </remarks>
public static class FocasAlarmType public static class FocasAlarmType
{ {
/// <summary>Pass to <see cref="IFocasClient.ReadAlarmsAsync"/>-equivalent to mean "any type".</summary> /// <summary>Pass to <see cref="IFocasClient.ReadAlarmsAsync"/>-equivalent to mean "any type".</summary>
public const int All = -1; public const short All = -1;
public const int Parameter = 0; // ALM_P public const short Parameter = 0; // ALM_P
public const int PulseCode = 1; // ALM_Y (servo) public const short PulseCode = 1; // ALM_Y (servo)
public const int Overtravel = 2; // ALM_O public const short Overtravel = 2; // ALM_O
public const int Overheat = 3; // ALM_H public const short Overheat = 3; // ALM_H
public const int Servo = 4; // ALM_S public const short Servo = 4; // ALM_S
public const int DataIo = 5; // ALM_T public const short DataIo = 5; // ALM_T
public const int MemoryCheck = 6; // ALM_M public const short MemoryCheck = 6; // ALM_M
public const int MacroAlarm = 13; // ALM_MC — used by #3006 etc. public const short MacroAlarm = 13; // ALM_MC — used by #3006 etc.
} }
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasConstants.cs
+6 -16
View File
@@ -58,23 +58,13 @@ public static class FocasOperationModeExtensions
{ {
/// <summary> /// <summary>
/// Canonical operator-facing label for an operation mode (e.g. <c>"AUTO"</c>, /// Canonical operator-facing label for an operation mode (e.g. <c>"AUTO"</c>,
/// <c>"EDIT"</c>). Unknown codes fall back to the raw numeric value as a string /// <c>"EDIT"</c>). Delegates to <see cref="FocasOpMode.ToText"/> so the wire layer
/// so the UI still shows something interpretable. /// and the fixed-tree projection render identical labels — historically these two
/// surfaces diverged ("TJOG" vs "T-JOG", "TEACH_IN_HANDLE" vs "TEACH-IN-HANDLE",
/// and different unknown-code fallbacks). Resolved by Driver.FOCAS-010.
/// </summary> /// </summary>
public static string ToText(this FocasOperationMode mode) => mode switch public static string ToText(this FocasOperationMode mode) =>
{ FocasOpMode.ToText((short)mode);
FocasOperationMode.Mdi => "MDI",
FocasOperationMode.Auto => "AUTO",
FocasOperationMode.TJog => "T-JOG",
FocasOperationMode.Edit => "EDIT",
FocasOperationMode.Handle => "HANDLE",
FocasOperationMode.Jog => "JOG",
FocasOperationMode.TeachInHandle => "TEACH-IN-HANDLE",
FocasOperationMode.Reference => "REFERENCE",
FocasOperationMode.Remote => "REMOTE",
FocasOperationMode.Test => "TEST",
_ => ((short)mode).ToString(),
};
} }
/// <summary> /// <summary>
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs
+35 -2
View File
@@ -1,3 +1,5 @@
using Microsoft.Extensions.Logging;
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire; namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire;
/// <summary> /// <summary>
@@ -14,9 +16,25 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire;
/// </remarks> /// </remarks>
public sealed class WireFocasClient : IFocasClient public sealed class WireFocasClient : IFocasClient
{ {
private readonly FocasWireClient _wire = new(); private readonly FocasWireClient _wire;
private FocasHostAddress? _address; private FocasHostAddress? _address;
/// <summary>
/// Default constructor — wire client without logger. Selected by the legacy
/// no-arg <see cref="WireFocasClientFactory.Create"/> path.
/// </summary>
public WireFocasClient() : this(logger: null) { }
/// <summary>
/// Construct with an optional logger. Threaded through to
/// <see cref="FocasWireClient"/> so the per-response Debug entries actually reach
/// the host's logging pipeline (Driver.FOCAS-007).
/// </summary>
public WireFocasClient(ILogger<FocasWireClient>? logger)
{
_wire = new FocasWireClient(logger);
}
public bool IsConnected => _wire.IsConnected; public bool IsConnected => _wire.IsConnected;
public async Task ConnectAsync(FocasHostAddress address, TimeSpan timeout, CancellationToken cancellationToken) public async Task ConnectAsync(FocasHostAddress address, TimeSpan timeout, CancellationToken cancellationToken)
@@ -339,5 +357,20 @@ public sealed class WireFocasClient : IFocasClient
/// <summary>Factory producing <see cref="WireFocasClient"/> instances — one per configured device.</summary> /// <summary>Factory producing <see cref="WireFocasClient"/> instances — one per configured device.</summary>
public sealed class WireFocasClientFactory : IFocasClientFactory public sealed class WireFocasClientFactory : IFocasClientFactory
{ {
public IFocasClient Create() => new WireFocasClient(); private readonly ILogger<FocasWireClient>? _logger;
public WireFocasClientFactory() : this(logger: null) { }
/// <summary>
/// Construct the factory with a logger that every created <see cref="WireFocasClient"/>
/// forwards to its <see cref="FocasWireClient"/>. Resolves Driver.FOCAS-007 — the wire
/// client already emits Debug entries per FOCAS response, but the previous no-arg
/// factory path discarded them.
/// </summary>
public WireFocasClientFactory(ILogger<FocasWireClient>? logger)
{
_logger = logger;
}
public IFocasClient Create() => new WireFocasClient(_logger);
} }
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Config/GalaxyDriverOptions.cs
+14 -3
View File
@@ -18,9 +18,20 @@ public sealed record GalaxyDriverOptions(
GalaxyReconnectOptions Reconnect); GalaxyReconnectOptions Reconnect);
/// <summary> /// <summary>
/// Connection details for the MxAccess gateway. <see cref="ApiKeySecretRef"/> resolves /// Connection details for the MxAccess gateway. <see cref="ApiKeySecretRef"/> is
/// through the server-side secret store (DPAPI for production, environment override for /// resolved by <c>GalaxyDriver.ResolveApiKey</c> at InitializeAsync time. Four forms
/// dev) — the API key never appears in cleartext config. /// supported, in priority order:
/// <list type="bullet">
/// <item><c>env:NAME</c> — read from an environment variable (recommended for
/// production; the central config DB holds only the indirection, not the key).</item>
/// <item><c>file:PATH</c> — read from an ACL'd file outside the repo.</item>
/// <item><c>dev:KEY</c> — explicit cleartext opt-in for dev rigs / parity tests;
/// no startup warning.</item>
/// <item>Anything else — treated as a literal cleartext API key for back-compat.
/// The resolver emits a <c>Warning</c> at startup so an operator who accidentally
/// committed a cleartext key sees it (Driver.Galaxy-010); production should
/// migrate to <c>env:</c> or <c>file:</c>.</item>
/// </list>
/// </summary> /// </summary>
// PR 6.5 tuning notes: // PR 6.5 tuning notes:
// ConnectTimeoutSeconds = 10 — cold-start network path comfort margin; soak runs // ConnectTimeoutSeconds = 10 — cold-start network path comfort margin; soak runs
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs
+145 -23
View File
@@ -11,19 +11,29 @@ using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime;
namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy; namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy;
/// <summary> /// <summary>
/// In-process .NET 10 Galaxy driver — the v2 replacement for the Galaxy.Host / /// In-process .NET 10 Galaxy driver — the only Galaxy backend since PR 7.2 retired
/// Galaxy.Proxy pair. PR 4.0 ships the project skeleton with <see cref="IDriver"/> /// the legacy <c>Galaxy.Host</c> / <c>Galaxy.Proxy</c> / <c>Galaxy.Shared</c>
/// bodies that wire to a future <c>IGalaxyGatewayClient</c> abstraction. Capability /// projects and the <c>OtOpcUaGalaxyHost</c> Windows service. Implements the full
/// interfaces (browse, read, write, subscribe, history routing, host probes) land in /// capability surface: <see cref="ITagDiscovery"/>, <see cref="IReadable"/>,
/// PRs 4.1–4.7; the wiring sequence keeps every intermediate state buildable so the /// <see cref="IWritable"/>, <see cref="ISubscribable"/>, <see cref="IRediscoverable"/>,
/// <c>Galaxy:Backend</c> flag (PR 4.W) can flip between legacy-host and mxgateway /// <see cref="IHostConnectivityProbe"/>, and <see cref="IAlarmSource"/>. Galaxy
/// for parity testing. /// access flows through the in-process driver over gRPC to the separately
/// installed <c>mxaccessgw</c> gateway (sibling repo), which owns the MXAccess
/// COM apartment server-side.
/// </summary> /// </summary>
/// <remarks> /// <remarks>
/// This driver is registered as a Tier A in-process driver alongside Modbus / S7 / etc. /// <para>
/// The legacy <c>GalaxyProxyDriver</c> (Driver.Galaxy.Proxy) coexists until PR 7.2; /// Registered as a Tier A in-process driver alongside Modbus / S7 / etc. via
/// <see cref="GalaxyDriverFactoryExtensions"/> registers under driver-type name /// <see cref="GalaxyDriverFactoryExtensions"/> under driver-type name
/// "GalaxyMxGateway" so both paths can be live simultaneously during parity testing. /// "GalaxyMxGateway".
/// </para>
/// <para>
/// Tests inject capability seams (<see cref="IGalaxyHierarchySource"/>,
/// <see cref="IGalaxyDataReader"/>, <see cref="IGalaxyDataWriter"/>,
/// <see cref="IGalaxySubscriber"/>, <see cref="IGalaxyAlarmAcknowledger"/>,
/// <see cref="IGalaxyAlarmFeed"/>) via the internal ctor so capability flow
/// can be exercised without a real gw round-trip.
/// </para>
/// </remarks> /// </remarks>
public sealed class GalaxyDriver public sealed class GalaxyDriver
: IDriver, ITagDiscovery, IReadable, IWritable, ISubscribable, IRediscoverable, IHostConnectivityProbe, IAlarmSource, IDisposable, IAsyncDisposable : IDriver, ITagDiscovery, IReadable, IWritable, ISubscribable, IRediscoverable, IHostConnectivityProbe, IAlarmSource, IDisposable, IAsyncDisposable
@@ -171,6 +181,16 @@ public sealed class GalaxyDriver
/// <summary>Test-visible options snapshot.</summary> /// <summary>Test-visible options snapshot.</summary>
internal GalaxyDriverOptions Options => _options; internal GalaxyDriverOptions Options => _options;
/// <summary>
/// Test-visible entry into <see cref="ReplayAsync"/>. The supervisor's
/// <see cref="ReconnectSupervisor.ReportTransportFailure"/> drives this on a
/// background task in production; tests prefer to invoke it directly so the
/// <see cref="GalaxyReconnectOptions.ReplayOnSessionLost"/> branch can be
/// asserted deterministically (Driver.Galaxy-013).
/// </summary>
internal Task InvokeReplayForTestAsync(CancellationToken cancellationToken) =>
ReplayAsync(cancellationToken);
/// <inheritdoc /> /// <inheritdoc />
public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken)
{ {
@@ -275,6 +295,20 @@ public sealed class GalaxyDriver
var entries = _subscriptions.SnapshotEntries(); var entries = _subscriptions.SnapshotEntries();
if (entries.Count == 0) return; if (entries.Count == 0) return;
// Driver.Galaxy-013: honor ReplayOnSessionLost. When operators opt out (false)
// we skip the per-tag SubscribeBulk fan-out — they're delegating to the
// gateway's session-level ReplaySubscriptions or accept post-reconnect tag
// loss. We still restart the EventPump so a future Subscribe call lands on
// a live consumer.
if (!_options.Reconnect.ReplayOnSessionLost)
{
RestartEventPumpForReplay();
_logger.LogInformation(
"GalaxyDriver {InstanceId} reconnect replay skipped — ReplayOnSessionLost=false ({SubCount} subscriptions tracked)",
_driverInstanceId, entries.Count);
return;
}
// The stream-fault that triggered this recovery left the old pump's RunAsync loop // The stream-fault that triggered this recovery left the old pump's RunAsync loop
// exited and its channel completed; EventPump.Start() is a no-op on a non-null but // exited and its channel completed; EventPump.Start() is a no-op on a non-null but
// completed loop. Recreate the pump so the replayed subscriptions have a consumer. // completed loop. Recreate the pump so the replayed subscriptions have a consumer.
@@ -380,7 +414,7 @@ public sealed class GalaxyDriver
} }
/// <summary> /// <summary>
/// Resolves <c>Gateway.ApiKeySecretRef</c> to the actual API-key bytes. Three /// Resolves <c>Gateway.ApiKeySecretRef</c> to the actual API-key bytes. Four
/// forms supported, evaluated in order: /// forms supported, evaluated in order:
/// <list type="number"> /// <list type="number">
/// <item><c>env:NAME</c> — reads <c>Environment.GetEnvironmentVariable(NAME)</c>. /// <item><c>env:NAME</c> — reads <c>Environment.GetEnvironmentVariable(NAME)</c>.
@@ -389,13 +423,26 @@ public sealed class GalaxyDriver
/// <item><c>file:PATH</c> — reads UTF-8 text from <c>PATH</c>, trimming /// <item><c>file:PATH</c> — reads UTF-8 text from <c>PATH</c>, trimming
/// whitespace. Lets operators stash the key in an ACL'd file outside the /// whitespace. Lets operators stash the key in an ACL'd file outside the
/// repo (the same pattern as the legacy <c>.local/galaxy-host-secret.txt</c>).</item> /// repo (the same pattern as the legacy <c>.local/galaxy-host-secret.txt</c>).</item>
/// <item>Anything else — used as the literal API key. Convenient for dev, /// <item><c>dev:KEY</c> — explicit cleartext literal. The <c>dev:</c> prefix
/// and avoids breaking existing configs that pre-date this resolver.</item> /// is a deliberate opt-in signal (dev box, parity rig) so the resolver
/// doesn't emit a warning; production should never use this arm.</item>
/// <item>Anything else — used as the literal API key for back-compat with
/// configs that pre-date this resolver. When a logger is supplied the
/// resolver emits a startup warning so an operator who accidentally
/// committed a cleartext key sees it (Driver.Galaxy-010).</item>
/// </list> /// </list>
/// A future PR can swap any of these arms for a DPAPI-backed lookup without /// A future PR can swap any of these arms for a DPAPI-backed lookup without
/// changing the call site. /// changing the call site.
/// </summary> /// </summary>
internal static string ResolveApiKey(string secretRef) internal static string ResolveApiKey(string secretRef) => ResolveApiKey(secretRef, logger: null);
/// <summary>
/// Logger-aware overload. Emits a <see cref="LogLevel.Warning"/> if the secret
/// ref falls through to the back-compat literal arm (an unprefixed cleartext
/// API key in <c>DriverConfig</c> JSON). The <c>dev:</c> prefix is the explicit
/// opt-in path that doesn't warn.
/// </summary>
internal static string ResolveApiKey(string secretRef, ILogger? logger)
{ {
ArgumentException.ThrowIfNullOrEmpty(secretRef); ArgumentException.ThrowIfNullOrEmpty(secretRef);
@@ -424,13 +471,30 @@ public sealed class GalaxyDriver
$"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' file '{path}' is empty."); $"Galaxy.Gateway.ApiKeySecretRef='{secretRef}' file '{path}' is empty.");
} }
if (secretRef.StartsWith("dev:", StringComparison.OrdinalIgnoreCase))
{
// Explicit dev opt-in — no warning, the operator deliberately chose a
// cleartext literal (dev box, parity rig).
return secretRef[4..];
}
// Back-compat literal arm. An unprefixed string is treated as the literal
// API key — but emit a warning so an operator who accidentally committed a
// cleartext key into DriverConfig sees it at startup. Use the dev: prefix
// to suppress this warning when the literal is intentional.
logger?.LogWarning(
"Galaxy.Gateway.ApiKeySecretRef is being treated as a literal cleartext API key. " +
"Prefer env:NAME, file:PATH, or the explicit dev:KEY prefix for dev rigs — " +
"a literal key in DriverConfig JSON is stored in cleartext in the central config DB.");
return secretRef; return secretRef;
} }
private static MxGatewayClientOptions BuildClientOptions(GalaxyGatewayOptions gw) => new() private MxGatewayClientOptions BuildClientOptions(GalaxyGatewayOptions gw) => new()
{ {
Endpoint = new Uri(gw.Endpoint, UriKind.Absolute), Endpoint = new Uri(gw.Endpoint, UriKind.Absolute),
ApiKey = ResolveApiKey(gw.ApiKeySecretRef), // Driver.Galaxy-010: pass the logger so the literal-arm cleartext fallback
// surfaces a startup warning rather than silently shipping the key.
ApiKey = ResolveApiKey(gw.ApiKeySecretRef, _logger),
UseTls = gw.UseTls, UseTls = gw.UseTls,
CaCertificatePath = gw.CaCertificatePath, CaCertificatePath = gw.CaCertificatePath,
ConnectTimeout = TimeSpan.FromSeconds(gw.ConnectTimeoutSeconds), ConnectTimeout = TimeSpan.FromSeconds(gw.ConnectTimeoutSeconds),
@@ -463,15 +527,64 @@ public sealed class GalaxyDriver
} }
/// <inheritdoc /> /// <inheritdoc />
/// <remarks>
/// <para>
/// In-place config reapply. The driver does not currently support
/// hot-swapping <see cref="GalaxyDriverOptions"/> at runtime — changing the
/// gateway endpoint, MxAccess client name, or reconnect policy requires
/// tearing down the gw session, supervisor, event pump, and address space.
/// The host stack handles that via DriverInstance restart, so this method
/// only accepts an equivalent config (no meaningful change) and refreshes
/// health; a non-equivalent reapply throws <see cref="NotSupportedException"/>
/// so the caller knows the change wasn't applied (Driver.Galaxy-013:
/// previously the method silently ignored <c>driverConfigJson</c>).
/// </para>
/// </remarks>
public Task ReinitializeAsync(string driverConfigJson, CancellationToken cancellationToken) public Task ReinitializeAsync(string driverConfigJson, CancellationToken cancellationToken)
{ {
// In-place config reapply. PR 4.5's reconnect supervisor will swap the
// gateway-client options under the lock; for the skeleton we just refresh health.
ObjectDisposedException.ThrowIf(_disposed, this); ObjectDisposedException.ThrowIf(_disposed, this);
if (!string.IsNullOrWhiteSpace(driverConfigJson))
{
// Materialise the incoming config and compare against the live options. We
// refuse any change that would require a session teardown rather than
// pretending to apply it.
GalaxyDriverOptions incoming;
try
{
// Reuse the factory's parse pipeline so any missing-required-field
// error surfaces with the same diagnostic text as InitializeAsync.
var transient = GalaxyDriverFactoryExtensions.CreateInstance(_driverInstanceId, driverConfigJson);
incoming = transient.Options;
// The transient instance never started a runtime — disposing is cheap.
transient.Dispose();
}
catch (Exception ex) when (ex is not NotSupportedException and not ObjectDisposedException)
{
throw new NotSupportedException(
$"GalaxyDriver.ReinitializeAsync could not parse the incoming DriverConfig JSON for '{_driverInstanceId}': {ex.Message}",
ex);
}
if (!OptionsAreEquivalent(_options, incoming))
{
throw new NotSupportedException(
"GalaxyDriver.ReinitializeAsync does not support hot-swapping driver options at runtime " +
"(gateway endpoint, MxAccess client name, reconnect policy, etc.). Restart the DriverInstance " +
"through the host stack to apply a config change.");
}
}
_health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null);
return Task.CompletedTask; return Task.CompletedTask;
} }
/// <summary>
/// Compare two <see cref="GalaxyDriverOptions"/> for runtime equivalence — every
/// field that drives gw session shape, address space, or reconnect behaviour
/// must match. Records get value-equality from the language, so a direct
/// equality check is enough.
/// </summary>
private static bool OptionsAreEquivalent(GalaxyDriverOptions a, GalaxyDriverOptions b) => a == b;
/// <inheritdoc /> /// <inheritdoc />
public Task ShutdownAsync(CancellationToken cancellationToken) public Task ShutdownAsync(CancellationToken cancellationToken)
{ {
@@ -637,12 +750,18 @@ public sealed class GalaxyDriver
} }
// Register bindings so the pump knows to dispatch events for these handles. // Register bindings so the pump knows to dispatch events for these handles.
// Driver.Galaxy-012: index the SubscribeBulk results once and correlate to
// references in O(1) instead of FirstOrDefault per element (O(n²) over the
// batch). On the 50k-tag soak path this turns a 2.5G-comparison loop into
// a single Dictionary build + linear scan.
var resultIndex = BuildResultIndex(results);
var bindings = new List<TagBinding>(fullReferences.Count); var bindings = new List<TagBinding>(fullReferences.Count);
for (var i = 0; i < fullReferences.Count; i++) for (var i = 0; i < fullReferences.Count; i++)
{ {
var fullRef = fullReferences[i]; var fullRef = fullReferences[i];
var match = results.FirstOrDefault(r => string.Equals(r.TagAddress, fullRef, StringComparison.OrdinalIgnoreCase)); var itemHandle = resultIndex.TryGetValue(fullRef, out var match) && match is { WasSuccessful: true }
var itemHandle = match is { WasSuccessful: true } ? match.ItemHandle : 0; ? match.ItemHandle
: 0;
bindings.Add(new TagBinding(fullRef, itemHandle)); bindings.Add(new TagBinding(fullRef, itemHandle));
// Tags the gw rejected up front — complete with Bad status now so the // Tags the gw rejected up front — complete with Bad status now so the
@@ -774,12 +893,15 @@ public sealed class GalaxyDriver
// recorded with a non-positive ItemHandle so the caller can detect partial failure // recorded with a non-positive ItemHandle so the caller can detect partial failure
// by inspecting the returned handle's diagnostic context — full per-tag error // by inspecting the returned handle's diagnostic context — full per-tag error
// surface lands in PR 5.3's parity tests. // surface lands in PR 5.3's parity tests.
// Driver.Galaxy-012: index results once, correlate in O(1) per reference rather
// than FirstOrDefault inside the loop (O(n²) on the 50k-tag path).
var resultIndex = BuildResultIndex(results);
var bindings = new List<TagBinding>(fullReferences.Count); var bindings = new List<TagBinding>(fullReferences.Count);
for (var i = 0; i < fullReferences.Count; i++) for (var i = 0; i < fullReferences.Count; i++)
{ {
var fullRef = fullReferences[i]; var fullRef = fullReferences[i];
var match = results.FirstOrDefault(r => string.Equals(r.TagAddress, fullRef, StringComparison.OrdinalIgnoreCase)); var hasMatch = resultIndex.TryGetValue(fullRef, out var match);
var itemHandle = match is { WasSuccessful: true } ? match.ItemHandle : 0; var itemHandle = hasMatch && match is { WasSuccessful: true } ? match.ItemHandle : 0;
bindings.Add(new TagBinding(fullRef, itemHandle)); bindings.Add(new TagBinding(fullRef, itemHandle));
if (match is null || !match.WasSuccessful) if (match is null || !match.WasSuccessful)
{ {
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs
+8 -3
View File
@@ -85,9 +85,14 @@ internal sealed class EventPump : IAsyncDisposable
} }
_channel = Channel.CreateBounded<MxEvent>(new BoundedChannelOptions(channelCapacity) _channel = Channel.CreateBounded<MxEvent>(new BoundedChannelOptions(channelCapacity)
{ {
// Newest-dropped policy: when full, the producer's TryWrite returns false // Newest-dropped semantics: we use FullMode.Wait but never call the
// and we account for the drop. We do this manually rather than relying on // awaitable WriteAsync — only the synchronous TryWrite below in
// BoundedChannelFullMode.DropWrite so we can count drops without polling. // RunAsync. With Wait + TryWrite, a full channel makes TryWrite return
// false immediately, which we account for via the EventsDropped counter.
// We deliberately do NOT use BoundedChannelFullMode.DropWrite — that
// would silently discard the new event inside Channel<T> without
// surfacing the drop on a counter (Driver.Galaxy-005: keep the comment
// and the FullMode value consistent).
FullMode = BoundedChannelFullMode.Wait, FullMode = BoundedChannelFullMode.Wait,
SingleReader = true, SingleReader = true,
SingleWriter = true, SingleWriter = true,
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs
+52 -17
View File
@@ -1,4 +1,5 @@
using System.Collections.Concurrent; using System.Collections.Concurrent;
using System.Collections.Immutable;
namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime; namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime;
@@ -18,7 +19,11 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime;
internal sealed class SubscriptionRegistry internal sealed class SubscriptionRegistry
{ {
private readonly ConcurrentDictionary<long, SubscriptionEntry> _bySubscriptionId = new(); private readonly ConcurrentDictionary<long, SubscriptionEntry> _bySubscriptionId = new();
private readonly ConcurrentDictionary<int, ConcurrentBag<long>> _subscribersByItemHandle = new(); // Driver.Galaxy-012: use ImmutableHashSet<long> for the reverse map so removal is
// O(log n) instead of "rebuild the entire ConcurrentBag from a LINQ filter on every
// unsubscribe"; reads are lock-free because the immutable snapshot is published
// atomically via ConcurrentDictionary AddOrUpdate.
private readonly ConcurrentDictionary<int, ImmutableHashSet<long>> _subscribersByItemHandle = new();
private long _nextSubscriptionId; private long _nextSubscriptionId;
public int TrackedSubscriptionCount => _bySubscriptionId.Count; public int TrackedSubscriptionCount => _bySubscriptionId.Count;
@@ -42,7 +47,7 @@ internal sealed class SubscriptionRegistry
_subscribersByItemHandle.AddOrUpdate( _subscribersByItemHandle.AddOrUpdate(
binding.ItemHandle, binding.ItemHandle,
_ => [subscriptionId], _ => [subscriptionId],
(_, bag) => { bag.Add(subscriptionId); return bag; }); (_, set) => set.Add(subscriptionId));
} }
} }
@@ -57,12 +62,10 @@ internal sealed class SubscriptionRegistry
foreach (var binding in entry.Bindings) foreach (var binding in entry.Bindings)
{ {
if (binding.ItemHandle <= 0) continue; if (binding.ItemHandle <= 0) continue;
if (!_subscribersByItemHandle.TryGetValue(binding.ItemHandle, out var bag)) continue; // Driver.Galaxy-012: ImmutableHashSet.Remove is O(log n) and the result is
// published atomically — no need to rebuild from a LINQ filter.
// Filter the bag to drop this subscription id. ConcurrentBag has no Remove — if (!_subscribersByItemHandle.TryGetValue(binding.ItemHandle, out var set)) continue;
// rebuild it from the remaining entries. The contention here is bounded by var remaining = set.Remove(subscriptionId);
// the number of tags in the dropped subscription.
var remaining = new ConcurrentBag<long>(bag.Where(id => id != subscriptionId));
if (remaining.IsEmpty) _subscribersByItemHandle.TryRemove(binding.ItemHandle, out _); if (remaining.IsEmpty) _subscribersByItemHandle.TryRemove(binding.ItemHandle, out _);
else _subscribersByItemHandle[binding.ItemHandle] = remaining; else _subscribersByItemHandle[binding.ItemHandle] = remaining;
} }
@@ -74,18 +77,23 @@ internal sealed class SubscriptionRegistry
/// Look up the (subscription id, full reference) pairs that should receive an /// Look up the (subscription id, full reference) pairs that should receive an
/// OnDataChange for the given gw item handle. Returns empty when nobody subscribes. /// OnDataChange for the given gw item handle. Returns empty when nobody subscribes.
/// </summary> /// </summary>
/// <remarks>
/// Driver.Galaxy-012: O(1) per subscriber via the per-entry
/// <c>FullRefByItemHandle</c> index, rather than a <c>FirstOrDefault</c> linear
/// scan of the binding list. At 50k tags / 1Hz this turns each dispatch from a
/// 50k-element scan into a single dictionary lookup.
/// </remarks>
public IReadOnlyList<(long SubscriptionId, string FullReference)> ResolveSubscribers(int itemHandle) public IReadOnlyList<(long SubscriptionId, string FullReference)> ResolveSubscribers(int itemHandle)
{ {
if (!_subscribersByItemHandle.TryGetValue(itemHandle, out var bag)) return []; if (!_subscribersByItemHandle.TryGetValue(itemHandle, out var bag)) return [];
// Each subscription may include the tag once. Walk every active subscription that // Each subscription may include the tag once. Walk every active subscription that
// claims this handle and pull the full ref from its binding list. // claims this handle and pull the full ref from its index in O(1).
var result = new List<(long, string)>(); var result = new List<(long, string)>();
foreach (var subId in bag.Distinct()) foreach (var subId in bag.Distinct())
{ {
if (!_bySubscriptionId.TryGetValue(subId, out var entry)) continue; if (!_bySubscriptionId.TryGetValue(subId, out var entry)) continue;
var binding = entry.Bindings.FirstOrDefault(b => b.ItemHandle == itemHandle); if (entry.FullRefByItemHandle.TryGetValue(itemHandle, out var fullRef))
if (binding is { FullReference: { } fullRef })
result.Add((subId, fullRef)); result.Add((subId, fullRef));
} }
return result; return result;
@@ -113,14 +121,14 @@ internal sealed class SubscriptionRegistry
{ {
if (!_bySubscriptionId.TryGetValue(subscriptionId, out var oldEntry)) return; if (!_bySubscriptionId.TryGetValue(subscriptionId, out var oldEntry)) return;
// Drop this subscription from every reverse-map bag it currently appears in. The // Drop this subscription from every reverse-map set it currently appears in. The
// pre-reconnect item handles are stale once the gw re-issues fresh ones. // pre-reconnect item handles are stale once the gw re-issues fresh ones.
// Driver.Galaxy-012: ImmutableHashSet.Remove is O(log n) — no LINQ rebuild.
foreach (var binding in oldEntry.Bindings) foreach (var binding in oldEntry.Bindings)
{ {
if (binding.ItemHandle <= 0) continue; if (binding.ItemHandle <= 0) continue;
if (!_subscribersByItemHandle.TryGetValue(binding.ItemHandle, out var bag)) continue; if (!_subscribersByItemHandle.TryGetValue(binding.ItemHandle, out var set)) continue;
var remaining = set.Remove(subscriptionId);
var remaining = new ConcurrentBag<long>(bag.Where(id => id != subscriptionId));
if (remaining.IsEmpty) _subscribersByItemHandle.TryRemove(binding.ItemHandle, out _); if (remaining.IsEmpty) _subscribersByItemHandle.TryRemove(binding.ItemHandle, out _);
else _subscribersByItemHandle[binding.ItemHandle] = remaining; else _subscribersByItemHandle[binding.ItemHandle] = remaining;
} }
@@ -132,11 +140,38 @@ internal sealed class SubscriptionRegistry
_subscribersByItemHandle.AddOrUpdate( _subscribersByItemHandle.AddOrUpdate(
binding.ItemHandle, binding.ItemHandle,
_ => [subscriptionId], _ => [subscriptionId],
(_, bag) => { bag.Add(subscriptionId); return bag; }); (_, set) => set.Add(subscriptionId));
} }
} }
private sealed record SubscriptionEntry(long SubscriptionId, IReadOnlyList<TagBinding> Bindings); /// <summary>
/// Per-subscription bookkeeping. <see cref="FullRefByItemHandle"/> is an index
/// over <see cref="Bindings"/> keyed by item handle so <c>ResolveSubscribers</c>
/// is O(1) per subscriber instead of a linear scan of every binding
/// (Driver.Galaxy-012). Failed bindings (item handle ≤ 0) are excluded from the
/// index because the EventPump only dispatches for positive handles.
/// </summary>
private sealed class SubscriptionEntry
{
public long SubscriptionId { get; }
public IReadOnlyList<TagBinding> Bindings { get; }
public IReadOnlyDictionary<int, string> FullRefByItemHandle { get; }
public SubscriptionEntry(long subscriptionId, IReadOnlyList<TagBinding> bindings)
{
SubscriptionId = subscriptionId;
Bindings = bindings;
var index = new Dictionary<int, string>(bindings.Count);
foreach (var binding in bindings)
{
if (binding.ItemHandle <= 0) continue; // failed gw subscribe — no events expected
// Last-write-wins on duplicates; the driver doesn't double-register a handle
// within a single subscription, but be defensive.
index[binding.ItemHandle] = binding.FullReference;
}
FullRefByItemHandle = index;
}
}
} }
/// <summary> /// <summary>
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs
+187 -9
View File
@@ -121,6 +121,15 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I
// read (Driver.S7-001). Drop this guard when Timer/Counter reads are wired through. // read (Driver.S7-001). Drop this guard when Timer/Counter reads are wired through.
RejectUnsupportedTagAddresses(); RejectUnsupportedTagAddresses();
// S7DataType values that ReadOneAsync / WriteOneAsync currently throw
// NotSupportedException for (Int64, UInt64, Float64, String, DateTime) must also
// be rejected at init — without this guard a site can configure e.g. a Float64
// tag, see the node appear in the address space via DiscoverAsync, and get
// BadNotSupported on every access. Half-implemented types must not leak into the
// configurable surface (Driver.S7-013). Drop entries from the set as each data
// type is wired through.
RejectUnsupportedTagDataTypes();
var plc = new Plc(_options.CpuType, _options.Host, _options.Port, _options.Rack, _options.Slot); var plc = new Plc(_options.CpuType, _options.Host, _options.Port, _options.Rack, _options.Slot);
// S7netplus writes timeouts into the underlying TcpClient via Plc.WriteTimeout / // S7netplus writes timeouts into the underlying TcpClient via Plc.WriteTimeout /
// Plc.ReadTimeout (milliseconds). Set before OpenAsync so the handshake itself // Plc.ReadTimeout (milliseconds). Set before OpenAsync so the handshake itself
@@ -262,6 +271,44 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I
} }
} }
/// <summary>
/// Rejects tags configured with an <see cref="S7DataType"/> that
/// <see cref="ReinterpretRawValue"/> / <see cref="BoxValueForWrite"/> still throw
/// <see cref="NotSupportedException"/> for. Without this guard those tags create live
/// OPC UA nodes via <see cref="DiscoverAsync"/> but every Read/Write returns
/// <c>BadNotSupported</c> — code-review finding Driver.S7-013. Drop entries from
/// <see cref="UnimplementedDataTypes"/> as each type is wired through.
/// </summary>
private void RejectUnsupportedTagDataTypes()
{
foreach (var t in _options.Tags)
{
if (UnimplementedDataTypes.Contains(t.DataType))
{
throw new NotSupportedException(
$"S7 tag '{t.Name}' uses data type '{t.DataType}' which is not yet " +
"supported by the S7 driver — Read/Write would return BadNotSupported. " +
"Remove the tag or use Bool/Byte/Int16/UInt16/Int32/UInt32/Float32 until " +
$"{t.DataType} is wired through.");
}
}
}
/// <summary>
/// S7DataType members that the read/write helpers throw NotSupportedException for.
/// Kept here (rather than reflecting over <see cref="ReinterpretRawValue"/>) so
/// <see cref="RejectUnsupportedTagDataTypes"/> is a single grep target for the
/// follow-up PR that wires each through.
/// </summary>
private static readonly HashSet<S7DataType> UnimplementedDataTypes = new()
{
S7DataType.Int64,
S7DataType.UInt64,
S7DataType.Float64,
S7DataType.String,
S7DataType.DateTime,
};
public DriverHealth GetHealth() => _health; public DriverHealth GetHealth() => _health;
/// <summary> /// <summary>
@@ -278,6 +325,10 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I
public async Task<IReadOnlyList<DataValueSnapshot>> ReadAsync( public async Task<IReadOnlyList<DataValueSnapshot>> ReadAsync(
IReadOnlyList<string> fullReferences, CancellationToken cancellationToken) IReadOnlyList<string> fullReferences, CancellationToken cancellationToken)
{ {
// Validate the list before RequirePlc() so a null argument produces an
// ArgumentNullException (consistent with DiscoverAsync) rather than an
// InvalidOperationException from the not-initialized check — Driver.S7-003.
ArgumentNullException.ThrowIfNull(fullReferences);
var plc = RequirePlc(); var plc = RequirePlc();
var now = DateTime.UtcNow; var now = DateTime.UtcNow;
var results = new DataValueSnapshot[fullReferences.Count]; var results = new DataValueSnapshot[fullReferences.Count];
@@ -333,7 +384,7 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I
return results; return results;
} }
private async Task<object> ReadOneAsync(global::S7.Net.Plc plc, S7TagDefinition tag, CancellationToken ct) private async Task<object> ReadOneAsync(Plc plc, S7TagDefinition tag, CancellationToken ct)
{ {
var addr = _parsedByName[tag.Name]; var addr = _parsedByName[tag.Name];
// S7.Net's string-based ReadAsync returns object where the boxed .NET type depends on // S7.Net's string-based ReadAsync returns object where the boxed .NET type depends on
@@ -381,6 +432,9 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I
public async Task<IReadOnlyList<WriteResult>> WriteAsync( public async Task<IReadOnlyList<WriteResult>> WriteAsync(
IReadOnlyList<WriteRequest> writes, CancellationToken cancellationToken) IReadOnlyList<WriteRequest> writes, CancellationToken cancellationToken)
{ {
// Same as ReadAsync — validate before RequirePlc() so a null argument is a
// typed argument error, not the "not initialized" surface (Driver.S7-003).
ArgumentNullException.ThrowIfNull(writes);
var plc = RequirePlc(); var plc = RequirePlc();
var results = new WriteResult[writes.Count]; var results = new WriteResult[writes.Count];
@@ -446,7 +500,7 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I
return results; return results;
} }
private async Task WriteOneAsync(global::S7.Net.Plc plc, S7TagDefinition tag, object? value, CancellationToken ct) private async Task WriteOneAsync(Plc plc, S7TagDefinition tag, object? value, CancellationToken ct)
{ {
// S7.Net's Plc.WriteAsync(string address, object value) expects the boxed value to // S7.Net's Plc.WriteAsync(string address, object value) expects the boxed value to
// match the address's size-suffix type: DBX=bool, DBB=byte, DBW=ushort, DBD=uint. // match the address's size-suffix type: DBX=bool, DBB=byte, DBW=ushort, DBD=uint.
@@ -574,32 +628,103 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I
return Task.CompletedTask; return Task.CompletedTask;
} }
/// <summary>
/// Upper bound on the poll-loop backoff window. After enough consecutive failures the
/// loop waits this long between retries instead of <see cref="SubscriptionState.Interval"/>,
/// so a subscription against a dropped / uninitialised driver doesn't spin (Driver.S7-009).
/// </summary>
private static readonly TimeSpan PollBackoffCap = TimeSpan.FromSeconds(30);
/// <summary>
/// Number of consecutive poll failures before the loop transitions the driver's
/// health to <see cref="DriverState.Degraded"/>. One stray failure can be transient;
/// a sustained run indicates the operator should see it. Threshold of 1 because the
/// first failure already lives in the LastError surface — see Driver.S7-009.
/// </summary>
private const int PollFailureHealthThreshold = 1;
private async Task PollLoopAsync(SubscriptionState state, CancellationToken ct) private async Task PollLoopAsync(SubscriptionState state, CancellationToken ct)
{ {
var consecutiveFailures = 0;
// Initial-data push per OPC UA Part 4 convention. // Initial-data push per OPC UA Part 4 convention.
try { await PollOnceAsync(state, forceRaise: true, ct).ConfigureAwait(false); } try
{
await PollOnceAsync(state, forceRaise: true, ct).ConfigureAwait(false);
consecutiveFailures = 0;
}
catch (OperationCanceledException) { return; } catch (OperationCanceledException) { return; }
catch (Exception ex) catch (Exception ex)
{ {
// First-read error — polling continues; log so the operator has an event trail. // First-read error — polling continues; log so the operator has an event trail.
_logger.LogWarning(ex, "S7 poll initial-read failed. Driver={DriverInstanceId}", driverInstanceId); consecutiveFailures++;
HandlePollFailure(ex, consecutiveFailures, initial: true);
} }
while (!ct.IsCancellationRequested) while (!ct.IsCancellationRequested)
{ {
try { await Task.Delay(state.Interval, ct).ConfigureAwait(false); } // Capped exponential backoff: Interval, 2×, 4×, ... up to PollBackoffCap. Healthy
// ticks reset consecutiveFailures back to 0 so the cadence snaps back to Interval.
var delay = ComputeBackoffDelay(state.Interval, consecutiveFailures);
try { await Task.Delay(delay, ct).ConfigureAwait(false); }
catch (OperationCanceledException) { return; } catch (OperationCanceledException) { return; }
try { await PollOnceAsync(state, forceRaise: false, ct).ConfigureAwait(false); } try
{
await PollOnceAsync(state, forceRaise: false, ct).ConfigureAwait(false);
consecutiveFailures = 0;
}
catch (OperationCanceledException) { return; } catch (OperationCanceledException) { return; }
catch (Exception ex) catch (Exception ex)
{ {
// Transient polling error — loop continues; log so the operator has an event trail. // Sustained polling error — loop continues with backoff; log + update health.
_logger.LogWarning(ex, "S7 poll tick failed. Driver={DriverInstanceId}", driverInstanceId); consecutiveFailures++;
HandlePollFailure(ex, consecutiveFailures, initial: false);
} }
} }
} }
/// <summary>
/// Logs the swallowed poll exception and, once <see cref="PollFailureHealthThreshold"/>
/// consecutive failures have accumulated, degrades the driver health so the failure
/// surfaces on the dashboard — see Driver.S7-009. The probe loop owns Running/Stopped
/// transitions for the host-connectivity surface, so we touch <see cref="_health"/>
/// rather than the probe state.
/// </summary>
private void HandlePollFailure(Exception ex, int consecutiveFailures, bool initial)
{
if (initial)
_logger.LogWarning(ex, "S7 poll initial-read failed. Driver={DriverInstanceId} ConsecutiveFailures={Count}",
driverInstanceId, consecutiveFailures);
else
_logger.LogWarning(ex, "S7 poll tick failed. Driver={DriverInstanceId} ConsecutiveFailures={Count}",
driverInstanceId, consecutiveFailures);
if (consecutiveFailures >= PollFailureHealthThreshold)
{
// Don't downgrade a Faulted state (e.g. PUT/GET-denied set by ReadAsync) — Faulted
// is a stronger signal than Degraded and is reserved for permanent config faults.
if (_health.State != DriverState.Faulted)
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message);
}
}
/// <summary>
/// Capped exponential backoff. <c>consecutiveFailures == 0</c> returns the configured
/// <paramref name="interval"/>; each subsequent failure doubles the wait up to
/// <see cref="PollBackoffCap"/>. Computed in ticks to avoid overflow at large counts.
/// </summary>
internal static TimeSpan ComputeBackoffDelay(TimeSpan interval, int consecutiveFailures)
{
if (consecutiveFailures <= 0) return interval;
// Cap the shift to avoid overflow — at 30 the result already saturates PollBackoffCap
// for any reasonable Interval.
var shift = Math.Min(consecutiveFailures - 1, 30);
var ticks = interval.Ticks << shift;
if (ticks <= 0 || ticks > PollBackoffCap.Ticks) return PollBackoffCap;
return TimeSpan.FromTicks(ticks);
}
private async Task PollOnceAsync(SubscriptionState state, bool forceRaise, CancellationToken ct) private async Task PollOnceAsync(SubscriptionState state, bool forceRaise, CancellationToken ct)
{ {
var snapshots = await ReadAsync(state.TagReferences, ct).ConfigureAwait(false); var snapshots = await ReadAsync(state.TagReferences, ct).ConfigureAwait(false);
@@ -702,7 +827,18 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I
OnHostStatusChanged?.Invoke(this, new HostStatusChangedEventArgs(HostName, old, newState)); OnHostStatusChanged?.Invoke(this, new HostStatusChangedEventArgs(HostName, old, newState));
} }
public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult(); public void Dispose()
{
// Driver.S7-010: avoid the sync-over-async DisposeAsync().AsTask().GetAwaiter().GetResult()
// pattern (a known deadlock surface even when currently safe here). ShutdownAsync's
// body is effectively synchronous apart from waiting on probe/poll Tasks; do the same
// teardown directly, blocking only on the drain — and only with a bounded timeout so
// a wedged loop can't hang Dispose() indefinitely.
if (_disposed) return;
_disposed = true;
SynchronousTeardown();
_gate.Dispose();
}
public async ValueTask DisposeAsync() public async ValueTask DisposeAsync()
{ {
@@ -712,4 +848,46 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I
catch { /* disposal is best-effort */ } catch { /* disposal is best-effort */ }
_gate.Dispose(); _gate.Dispose();
} }
/// <summary>
/// Synchronous teardown — mirrors <see cref="ShutdownAsync"/> but blocks (with a bounded
/// timeout) on the probe + poll Tasks instead of awaiting them. Used by the sync
/// <see cref="Dispose"/> path so we don't sync-over-async <see cref="DisposeAsync"/>
/// (Driver.S7-010).
/// </summary>
private void SynchronousTeardown()
{
var drain = new List<Task>();
var probeCts = _probeCts;
var probeTask = _probeTask;
try { probeCts?.Cancel(); } catch { }
if (probeTask is not null) drain.Add(probeTask);
var subscriptions = _subscriptions.Values.ToArray();
_subscriptions.Clear();
foreach (var state in subscriptions)
{
try { state.Cts.Cancel(); } catch { }
drain.Add(state.PollTask);
}
if (drain.Count > 0)
{
try { Task.WhenAll(drain).Wait(DrainTimeout); }
catch { /* timeouts/loop faults are tolerated — teardown continues */ }
}
probeCts?.Dispose();
_probeCts = null;
_probeTask = null;
foreach (var state in subscriptions)
{
try { state.Cts.Dispose(); } catch { }
}
try { Plc?.Close(); } catch { /* best-effort — tearing down anyway */ }
Plc = null;
_health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null);
}
} }
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLoggingTests.cs
+197
View File
@@ -0,0 +1,197 @@
using Microsoft.Extensions.Logging;
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.AbCip;
namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests;
/// <summary>
/// Regression coverage for Driver.AbCip-007 — the driver previously swallowed every
/// exception in its read / write / probe / template-read / alarm-poll paths with no
/// logging at all, leaving operators blind when a PLC was silently failing every tick.
/// Driver.AbCip-011 — when the probe is Enabled but no ProbeTagPath is configured, the
/// driver used to silently leave every device's HostState=Unknown forever; the fix logs a
/// warning at init time so the operator notices.
/// </summary>
[Trait("Category", "Unit")]
public sealed class AbCipLoggingTests
{
private const string Device = "ab://10.0.0.5/1,0";
[Fact]
public void Constructor_accepts_an_ILogger()
{
// Constructor signature must allow an ILogger<AbCipDriver> so the host can wire one
// through Microsoft.Extensions.DependencyInjection. The driver code project already
// pulls in Microsoft.Extensions.Logging.Abstractions transitively via Core.
var logger = new CapturingLogger<AbCipDriver>();
var drv = new AbCipDriver(
new AbCipDriverOptions { Probe = new AbCipProbeOptions { Enabled = false } },
"drv-1",
logger: logger);
drv.ShouldNotBeNull();
}
[Fact]
public async Task ProbeLoop_logs_when_an_exception_is_swallowed()
{
var logger = new CapturingLogger<AbCipDriver>();
var factory = new FakeAbCipTagFactory
{
// Force the probe to throw on initialize so the swallow path runs every tick.
Customise = p => new FakeAbCipTag(p)
{
ThrowOnInitialize = true,
Exception = new InvalidOperationException("simulated probe init failure"),
},
};
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Device)],
Probe = new AbCipProbeOptions
{
Enabled = true,
ProbeTagPath = "ProbeTag",
Interval = TimeSpan.FromMilliseconds(20),
Timeout = TimeSpan.FromMilliseconds(50),
},
}, "drv-log", factory, logger: logger);
await drv.InitializeAsync("{}", CancellationToken.None);
// Give the probe loop a couple of ticks to log.
await Task.Delay(200);
await drv.ShutdownAsync(CancellationToken.None);
// We expect at least one log entry that mentions the probe loop or carries the
// simulated exception. Without it there is no record of a wedged probe — exactly the
// gap the finding called out.
logger.Entries.ShouldNotBeEmpty();
logger.Entries.Any(e => e.Message.Contains("probe", StringComparison.OrdinalIgnoreCase)
|| (e.Exception?.Message.Contains("simulated probe init failure") ?? false))
.ShouldBeTrue("at least one log entry should reference the probe loop or surface the swallowed exception");
}
[Fact]
public async Task ReadFailure_logs_at_warning_level()
{
// A non-zero libplctag status used to be silently classified into BadCommunicationError
// with no log. After the fix the driver logs a warning so operators can correlate the
// status code with the affected tag.
var logger = new CapturingLogger<AbCipDriver>();
var factory = new FakeAbCipTagFactory
{
Customise = p => new FakeAbCipTag(p) { Status = (int)libplctag.Status.ErrorBadConnection },
};
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Device)],
Tags = [new AbCipTagDefinition("Speed", Device, "Speed", AbCipDataType.DInt)],
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-1", factory, logger: logger);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.ReadAsync(["Speed"], CancellationToken.None);
logger.Entries.Any(e => e.Level >= LogLevel.Warning
&& e.Message.Contains("Speed", StringComparison.OrdinalIgnoreCase))
.ShouldBeTrue("read failure on tag 'Speed' should be logged at warning level or above");
}
[Fact]
public async Task ReadException_logs_at_warning_level()
{
// A transport-level exception used to be silently mapped to BadCommunicationError with
// no log. After the fix the driver logs a warning carrying the exception so operators
// can see the root cause.
var logger = new CapturingLogger<AbCipDriver>();
var factory = new FakeAbCipTagFactory
{
Customise = p => new FakeAbCipTag(p)
{
ThrowOnRead = true,
Exception = new InvalidOperationException("simulated wire failure"),
},
};
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Device)],
Tags = [new AbCipTagDefinition("Speed", Device, "Speed", AbCipDataType.DInt)],
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-1", factory, logger: logger);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.ReadAsync(["Speed"], CancellationToken.None);
logger.Entries.Any(e => e.Level >= LogLevel.Warning
&& (e.Exception?.Message.Contains("simulated wire failure") ?? false))
.ShouldBeTrue("read transport exception should be logged at warning level with the inner exception attached");
}
[Fact]
public async Task InitializeAsync_warns_when_probe_is_enabled_but_ProbeTagPath_is_blank()
{
// Driver.AbCip-011: when Probe.Enabled is true but ProbeTagPath is null/blank, the
// driver used to start no probe loop and leave the device's HostState=Unknown forever.
// The fix logs a warning so the operator sees the misconfiguration instead of getting
// a silently inert health surface.
var logger = new CapturingLogger<AbCipDriver>();
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Device)],
Probe = new AbCipProbeOptions
{
Enabled = true,
ProbeTagPath = null, // explicitly inert
},
}, "drv-1", logger: logger);
await drv.InitializeAsync("{}", CancellationToken.None);
logger.Entries.Any(e => e.Level == LogLevel.Warning
&& (e.Message.Contains("probe", StringComparison.OrdinalIgnoreCase)
&& e.Message.Contains("ProbeTagPath", StringComparison.OrdinalIgnoreCase)))
.ShouldBeTrue("probe-enabled-but-inert configuration should be logged at warning level");
}
[Fact]
public async Task InitializeAsync_does_not_warn_when_probe_is_disabled()
{
// No warning when the operator explicitly opted out.
var logger = new CapturingLogger<AbCipDriver>();
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Device)],
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-1", logger: logger);
await drv.InitializeAsync("{}", CancellationToken.None);
logger.Entries.Any(e => e.Level == LogLevel.Warning
&& e.Message.Contains("ProbeTagPath", StringComparison.OrdinalIgnoreCase))
.ShouldBeFalse("no probe warning expected when Probe.Enabled is false");
}
internal sealed class CapturingLogger<T> : ILogger<T>
{
public List<(LogLevel Level, string Message, Exception? Exception)> Entries { get; } = new();
public IDisposable BeginScope<TState>(TState state) where TState : notnull => NullScope.Instance;
public bool IsEnabled(LogLevel logLevel) => true;
public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception,
Func<TState, Exception?, string> formatter)
{
Entries.Add((logLevel, formatter(state, exception), exception));
}
private sealed class NullScope : IDisposable
{
public static NullScope Instance { get; } = new();
public void Dispose() { }
}
}
}
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipPerDeviceConnectionOptionsTests.cs
+134
View File
@@ -0,0 +1,134 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Driver.AbCip;
namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests;
/// <summary>
/// Regression coverage for Driver.AbCip-013 — driver-specs.md §3 lists per-device
/// <c>AllowPacking</c> and <c>ConnectionSize</c> as configurable connection settings, but
/// the implementation previously hard-coded both from the family profile and never wired
/// them through to the libplctag <c>Tag</c>. The fix exposes them as per-device options on
/// <see cref="AbCipDeviceOptions"/>, plumbs them through <see cref="AbCipTagCreateParams"/>,
/// and applies <c>AllowPacking</c> to the live <c>Tag</c>. <c>ConnectionSize</c> is captured
/// for forward-compat (libplctag 1.5.2 does not expose a direct property; the value is
/// plumbed through the create-params so future wrappers / a custom tag-attribute path can
/// consume it without a config-shape change).
/// </summary>
[Trait("Category", "Unit")]
public sealed class AbCipPerDeviceConnectionOptionsTests
{
private const string Device = "ab://10.0.0.5/1,0";
[Fact]
public async Task Device_AllowPacking_override_is_forwarded_to_tag_create_params()
{
// Driver.AbCip-013 — operator opts out of request packing on a specific device.
var factory = new FakeAbCipTagFactory();
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Device, AllowPacking: false)],
Tags = [new AbCipTagDefinition("Speed", Device, "Speed", AbCipDataType.DInt)],
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-1", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.ReadAsync(["Speed"], CancellationToken.None);
factory.Tags["Speed"].CreationParams.AllowPacking.ShouldBeFalse();
}
[Fact]
public async Task Device_AllowPacking_default_inherits_from_family_profile()
{
// No per-device override — default falls back to the family profile's value (true for
// ControlLogix, false for Micro800).
var factory = new FakeAbCipTagFactory();
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Device)],
Tags = [new AbCipTagDefinition("Speed", Device, "Speed", AbCipDataType.DInt)],
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-1", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.ReadAsync(["Speed"], CancellationToken.None);
// ControlLogix profile has SupportsRequestPacking = true.
factory.Tags["Speed"].CreationParams.AllowPacking.ShouldBeTrue();
}
[Fact]
public async Task Micro800_default_AllowPacking_is_false_from_family_profile()
{
const string micro = "ab://10.0.0.6/";
var factory = new FakeAbCipTagFactory();
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(micro, AbCipPlcFamily.Micro800)],
Tags = [new AbCipTagDefinition("X", micro, "X", AbCipDataType.DInt)],
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-1", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.ReadAsync(["X"], CancellationToken.None);
// Micro800 profile defaults SupportsRequestPacking = false.
factory.Tags["X"].CreationParams.AllowPacking.ShouldBeFalse();
}
[Fact]
public async Task Device_ConnectionSize_override_is_forwarded_to_tag_create_params()
{
var factory = new FakeAbCipTagFactory();
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Device, ConnectionSize: 504)],
Tags = [new AbCipTagDefinition("Speed", Device, "Speed", AbCipDataType.DInt)],
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-1", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.ReadAsync(["Speed"], CancellationToken.None);
factory.Tags["Speed"].CreationParams.ConnectionSize.ShouldBe(504);
}
[Fact]
public async Task Device_ConnectionSize_default_inherits_from_family_profile()
{
var factory = new FakeAbCipTagFactory();
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Device)],
Tags = [new AbCipTagDefinition("Speed", Device, "Speed", AbCipDataType.DInt)],
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-1", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.ReadAsync(["Speed"], CancellationToken.None);
// ControlLogix family default ConnectionSize is 4002 (Large Forward Open).
factory.Tags["Speed"].CreationParams.ConnectionSize.ShouldBe(4002);
}
[Fact]
public void AbCipDriverFactoryExtensions_ParseOptions_round_trips_AllowPacking_and_ConnectionSize()
{
const string json = """
{
"Devices": [ {
"HostAddress": "ab://10.0.0.5/1,0",
"PlcFamily": "ControlLogix",
"AllowPacking": false,
"ConnectionSize": 504
} ]
}
""";
var opts = AbCipDriverFactoryExtensions.ParseOptions("drv-1", json);
opts.Devices.Single().AllowPacking.ShouldBe(false);
opts.Devices.Single().ConnectionSize.ShouldBe(504);
}
}
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyDisposeAndResolveHostTests.cs
+158
View File
@@ -0,0 +1,158 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies;
namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests;
/// <summary>
/// Driver.AbLegacy-011 — synchronous <see cref="IDisposable.Dispose"/> must perform real
/// synchronous teardown rather than blocking via <c>DisposeAsync().AsTask().GetAwaiter().GetResult()</c>.
/// Driver.AbLegacy-013 — <see cref="AbLegacyDriver.ResolveHost"/> fallback when the reference
/// is unknown and no devices are configured is the documented single-host fallback per the
/// <c>IPerCallHostResolver</c> contract.
/// </summary>
[Trait("Category", "Unit")]
public sealed class AbLegacyDisposeAndResolveHostTests
{
// ---- Driver.AbLegacy-011 ----
[Fact]
public async Task Dispose_runs_teardown_without_blocking_on_async_wait()
{
// Build a driver with a real device + tag so InitializeAsync registers state, then Dispose.
// The teardown must clear the device dictionary just like ShutdownAsync would, but without
// round-tripping through AsTask().GetAwaiter().GetResult() (which would deadlock under a
// single-threaded synchronization context).
var factory = new FakeAbLegacyTagFactory();
var drv = new AbLegacyDriver(new AbLegacyDriverOptions
{
Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0", AbLegacyPlcFamily.Slc500)],
Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)],
Probe = new AbLegacyProbeOptions { Enabled = false },
}, "drv-dispose", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
// Materialise one runtime so DisposeRuntimes has work to do.
await drv.ReadAsync(["X"], CancellationToken.None);
factory.Tags["N7:0"].Disposed.ShouldBeFalse();
drv.Dispose();
// The cached libplctag tag must be disposed and the device map cleared.
factory.Tags["N7:0"].Disposed.ShouldBeTrue();
drv.DeviceCount.ShouldBe(0);
drv.GetHealth().State.ShouldBe(DriverState.Unknown);
}
[Fact]
public async Task Dispose_is_idempotent()
{
var drv = new AbLegacyDriver(new AbLegacyDriverOptions
{
Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")],
Probe = new AbLegacyProbeOptions { Enabled = false },
}, "drv-1");
await drv.InitializeAsync("{}", CancellationToken.None);
drv.Dispose();
Should.NotThrow(() => drv.Dispose());
}
[Fact]
public async Task Dispose_under_single_threaded_sync_context_does_not_deadlock()
{
// The legacy sync-over-async pattern (DisposeAsync().AsTask().GetAwaiter().GetResult())
// can deadlock if any awaited continuation marshals back to a captured single-threaded
// context. Drive a single-threaded SynchronizationContext + Dispose() and ensure it
// returns within a short timeout.
var factory = new FakeAbLegacyTagFactory();
var drv = new AbLegacyDriver(new AbLegacyDriverOptions
{
Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")],
Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)],
Probe = new AbLegacyProbeOptions { Enabled = false },
}, "drv-1", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.ReadAsync(["X"], CancellationToken.None);
using var ctx = new SingleThreadSynchronizationContext();
var prior = SynchronizationContext.Current;
SynchronizationContext.SetSynchronizationContext(ctx);
try
{
var disposed = new ManualResetEventSlim(false);
ctx.Post(_ => { drv.Dispose(); disposed.Set(); }, null);
ctx.RunUntil(disposed);
disposed.IsSet.ShouldBeTrue("Dispose must return without blocking on the single-threaded context");
}
finally
{
SynchronizationContext.SetSynchronizationContext(prior);
}
}
/// <summary>
/// Minimal cooperative single-threaded SynchronizationContext for the deadlock-regression
/// test. The thread that calls <see cref="RunUntil"/> pumps queued callbacks until the
/// stop event is set.
/// </summary>
private sealed class SingleThreadSynchronizationContext : SynchronizationContext, IDisposable
{
private readonly System.Collections.Concurrent.BlockingCollection<(SendOrPostCallback, object?)> _queue = new();
public override void Post(SendOrPostCallback d, object? state) => _queue.Add((d, state));
public override void Send(SendOrPostCallback d, object? state) => d(state);
public void RunUntil(ManualResetEventSlim stop)
{
while (!stop.IsSet)
{
if (_queue.TryTake(out var item, TimeSpan.FromSeconds(2)))
item.Item1(item.Item2);
else
throw new TimeoutException("Dispose did not complete — likely sync-over-async deadlock");
}
}
public void Dispose() => _queue.Dispose();
}
// ---- Driver.AbLegacy-013 ----
[Fact]
public void ResolveHost_known_reference_returns_tag_device()
{
var drv = new AbLegacyDriver(new AbLegacyDriverOptions
{
Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")],
Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)],
}, "drv-1");
drv.ResolveHost("X").ShouldBe("ab://10.0.0.5/1,0");
}
[Fact]
public void ResolveHost_unknown_reference_with_devices_returns_first_device()
{
// Multi-device fallback: an unknown reference returns the first configured device so the
// resilience pipeline keys on a real ab:// host rather than the instance id.
var drv = new AbLegacyDriver(new AbLegacyDriverOptions
{
Devices =
[
new AbLegacyDeviceOptions("ab://10.0.0.5/1,0"),
new AbLegacyDeviceOptions("ab://10.0.0.6/"),
],
}, "drv-1");
drv.ResolveHost("unknown").ShouldBe("ab://10.0.0.5/1,0");
}
[Fact]
public void ResolveHost_unknown_reference_no_devices_returns_driver_instance_id()
{
// Per IPerCallHostResolver: implementations MUST NOT throw on an unknown reference; they
// must return the driver's default-host string. With no devices configured the driver
// instance id is the documented single-host fallback.
var drv = new AbLegacyDriver(new AbLegacyDriverOptions(), "drv-singleton");
drv.ResolveHost("anything").ShouldBe("drv-singleton");
}
}
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyLoggerInjectionTests.cs
+90
View File
@@ -0,0 +1,90 @@
using Microsoft.Extensions.Logging;
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests;
/// <summary>
/// Driver.AbLegacy-005 — verifies the driver accepts and uses an optional
/// <see cref="ILogger{AbLegacyDriver}"/>. Probe transitions, init failures, and the first
/// non-zero libplctag status per device must be logged (rather than only folded into the
/// transient <see cref="DriverHealth.Detail"/> string) so a field operator can correlate a
/// PCCC comms problem with a structured log entry.
/// </summary>
[Trait("Category", "Unit")]
public sealed class AbLegacyLoggerInjectionTests
{
private sealed class CapturingLogger : ILogger<AbLegacyDriver>
{
public readonly List<(LogLevel Level, string Message)> Entries = new();
public IDisposable BeginScope<TState>(TState state) where TState : notnull => NullScope.Instance;
public bool IsEnabled(LogLevel logLevel) => true;
public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception,
Func<TState, Exception?, string> formatter)
=> Entries.Add((logLevel, formatter(state, exception)));
private sealed class NullScope : IDisposable
{
public static readonly NullScope Instance = new();
public void Dispose() { }
}
}
[Fact]
public void Driver_accepts_optional_logger_parameter()
{
// Constructor must accept an ILogger<AbLegacyDriver> as an optional named arg, matching
// the Modbus/S7/Galaxy driver pattern. The driver runs with NullLogger when omitted.
var logger = new CapturingLogger();
var drv = new AbLegacyDriver(new AbLegacyDriverOptions(), "drv-logged", tagFactory: null, logger: logger);
drv.ShouldNotBeNull();
}
[Fact]
public async Task InitializeAsync_failure_emits_error_log()
{
var logger = new CapturingLogger();
var drv = new AbLegacyDriver(new AbLegacyDriverOptions
{
Devices = [new AbLegacyDeviceOptions("not-a-valid-address")],
}, "drv-logged", tagFactory: null, logger: logger);
await Should.ThrowAsync<InvalidOperationException>(
() => drv.InitializeAsync("{}", CancellationToken.None));
var errors = logger.Entries.Where(e => e.Level >= LogLevel.Error).ToList();
errors.ShouldNotBeEmpty("init failure must surface as an Error-level log");
errors[0].Message.ShouldContain("drv-logged");
}
[Fact]
public async Task First_nonzero_libplctag_status_per_device_is_logged()
{
// The driver should log the first occurrence of a non-zero libplctag status per device,
// so that a field operator can correlate a comms problem with a structured log entry
// even though Detail is overwritten by the next read.
var factory = new FakeAbLegacyTagFactory();
var logger = new CapturingLogger();
var drv = new AbLegacyDriver(new AbLegacyDriverOptions
{
Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")],
Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)],
Probe = new AbLegacyProbeOptions { Enabled = false },
}, "drv-logged", factory, logger);
await drv.InitializeAsync("{}", CancellationToken.None);
factory.Customise = p => new FakeAbLegacyTag(p) { Status = -32 }; // timeout
await drv.ReadAsync(["X"], CancellationToken.None);
var warnings = logger.Entries.Where(e => e.Level == LogLevel.Warning).ToList();
warnings.ShouldNotBeEmpty("first non-zero libplctag status must emit a structured warning");
warnings.Any(w => w.Message.Contains("ab://10.0.0.5/1,0") || w.Message.Contains("drv-logged"))
.ShouldBeTrue("warning must identify the device or driver instance");
// Second read with the same status — the per-device de-dupe should suppress.
var warningCountAfterFirst = warnings.Count;
await drv.ReadAsync(["X"], CancellationToken.None);
var newWarnings = logger.Entries.Where(e => e.Level == LogLevel.Warning).ToList();
newWarnings.Count.ShouldBe(warningCountAfterFirst, "subsequent same-device non-zero status stays quiet");
}
}
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasLoggingTests.cs
+101
View File
@@ -0,0 +1,101 @@
using Microsoft.Extensions.Logging;
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Driver.FOCAS;
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests;
/// <summary>
/// Regression coverage for Driver.FOCAS-007 — the driver previously swallowed every
/// exception in its poll / probe / recycle / fixed-tree loops with no logging at all,
/// leaving operators blind when a CNC was silently failing every tick.
/// </summary>
[Trait("Category", "Unit")]
public sealed class FocasLoggingTests
{
[Fact]
public void Constructor_accepts_an_ILogger()
{
// Constructor signature must allow an ILogger<FocasDriver> so the host can wire one
// through Microsoft.Extensions.DependencyInjection. The driver code project already
// references Microsoft.Extensions.Logging.Abstractions.
var logger = new CapturingLogger<FocasDriver>();
var drv = new FocasDriver(
new FocasDriverOptions { Probe = new FocasProbeOptions { Enabled = false } },
"drv-1",
new FakeFocasClientFactory(),
logger);
drv.ShouldNotBeNull();
}
[Fact]
public async Task ProbeLoop_logs_when_an_exception_is_swallowed()
{
var logger = new CapturingLogger<FocasDriver>();
var factory = new FakeFocasClientFactory
{
Customise = () => new FakeFocasClient
{
// Make ProbeAsync throw — the probe loop swallows it but must log.
ThrowOnConnect = false,
ProbeResult = true, // not used because the underlying probe path throws
},
};
// Force the probe to throw by making the client throw on connect.
factory.Customise = () => new FakeFocasClient
{
ThrowOnConnect = true,
Exception = new InvalidOperationException("simulated probe failure"),
};
var drv = new FocasDriver(
new FocasDriverOptions
{
Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")],
Probe = new FocasProbeOptions
{
Enabled = true,
Interval = TimeSpan.FromMilliseconds(50),
Timeout = TimeSpan.FromMilliseconds(100),
},
},
"drv-log",
factory,
logger);
await drv.InitializeAsync("{}", CancellationToken.None);
// Give the probe loop one tick or two to log.
await Task.Delay(250);
await drv.ShutdownAsync(CancellationToken.None);
// We expect at least one log entry at Debug / Warning that mentions the simulated
// failure or the probe loop. Without logging there's literally no record on a wedged
// CNC — exactly the gap the finding called out.
logger.Entries.ShouldNotBeEmpty();
logger.Entries.Any(e => e.Message.Contains("probe", StringComparison.OrdinalIgnoreCase)
|| (e.Exception?.Message.Contains("simulated probe failure") ?? false))
.ShouldBeTrue("at least one log entry should reference the probe loop or surface the swallowed exception");
}
private sealed class CapturingLogger<T> : ILogger<T>
{
public List<(LogLevel Level, string Message, Exception? Exception)> Entries { get; } = new();
public IDisposable BeginScope<TState>(TState state) where TState : notnull => NullScope.Instance;
public bool IsEnabled(LogLevel logLevel) => true;
public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception,
Func<TState, Exception?, string> formatter)
{
Entries.Add((logLevel, formatter(state, exception), exception));
}
private sealed class NullScope : IDisposable
{
public static NullScope Instance { get; } = new();
public void Dispose() { }
}
}
}
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasLowFindingsTests.cs
+218
View File
@@ -0,0 +1,218 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.FOCAS;
using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire;
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests;
/// <summary>
/// Regression coverage for the Low-severity code-review findings:
/// <list type="bullet">
/// <item>Driver.FOCAS-008 — parsed FocasAddress is cached at init, not re-parsed per read/write</item>
/// <item>Driver.FOCAS-009 — Probe.Timeout is actually applied around ProbeAsync</item>
/// <item>Driver.FOCAS-010 — operation-mode → text mapping is consolidated</item>
/// <item>Driver.FOCAS-011 — FocasAlarmType constants are typed as short</item>
/// </list>
/// </summary>
[Trait("Category", "Unit")]
public sealed class FocasLowFindingsTests
{
// ---- Driver.FOCAS-008 — parsed FocasAddress cached at init ----
[Fact]
public async Task ReadAsync_uses_cached_FocasAddress_when_tag_definition_has_a_malformed_address_after_init()
{
// After InitializeAsync succeeds with a well-formed address, the driver must rely on the
// cached parse — *not* re-parse `FocasTagDefinition.Address` on every read. We can prove
// the cache is used by stuffing a malformed Address onto a tag *post-init* through the
// internal cache surface — but that's brittle. Instead, prove no re-parse by counting:
// we monkey-patch the FakeFocasClient.ReadAsync to capture the FocasAddress reference
// it receives and assert it's the *same* instance across two consecutive reads.
var captured = new List<FocasAddress>();
var factory = new FakeFocasClientFactory
{
Customise = () => new CapturingFakeFocasClient(captured)
{
Values = { ["R100"] = (sbyte)1 },
},
};
var drv = new FocasDriver(new FocasDriverOptions
{
Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")],
Tags = [new FocasTagDefinition("X", "focas://10.0.0.5:8193", "R100", FocasDataType.Byte)],
Probe = new FocasProbeOptions { Enabled = false },
}, "drv-1", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.ReadAsync(["X"], CancellationToken.None);
await drv.ReadAsync(["X"], CancellationToken.None);
captured.Count.ShouldBe(2);
ReferenceEquals(captured[0], captured[1])
.ShouldBeTrue("ReadAsync must reuse the FocasAddress parsed at init, not re-parse per read");
}
[Fact]
public async Task WriteAsync_uses_cached_FocasAddress_too()
{
var captured = new List<FocasAddress>();
var factory = new FakeFocasClientFactory
{
Customise = () => new CapturingFakeFocasClient(captured)
{
WriteStatuses = { ["R100"] = FocasStatusMapper.Good },
},
};
var drv = new FocasDriver(new FocasDriverOptions
{
Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")],
Tags =
[
new FocasTagDefinition(
"X", "focas://10.0.0.5:8193", "R100", FocasDataType.Byte, Writable: true),
],
Probe = new FocasProbeOptions { Enabled = false },
}, "drv-1", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
await drv.WriteAsync(
[new WriteRequest("X", (sbyte)1)],
CancellationToken.None);
await drv.WriteAsync(
[new WriteRequest("X", (sbyte)2)],
CancellationToken.None);
captured.Count.ShouldBe(2);
ReferenceEquals(captured[0], captured[1])
.ShouldBeTrue("WriteAsync must reuse the FocasAddress parsed at init");
}
// ---- Driver.FOCAS-009 — Probe.Timeout applies to ProbeAsync ----
[Fact]
public async Task ProbeLoop_cancels_a_slow_ProbeAsync_at_Probe_Timeout()
{
// The probe loop must apply Probe.Timeout — a hung CNC socket should be cancelled at the
// configured timeout rather than blocking until the OS TCP timeout. We prove the timeout
// is applied by making ProbeAsync wait indefinitely and asserting it observes
// cancellation before the normal probe Interval would tick again.
var hangSignal = new TaskCompletionSource();
var factory = new FakeFocasClientFactory
{
Customise = () => new HangingProbeFakeClient(hangSignal),
};
var probeTimeout = TimeSpan.FromMilliseconds(100);
var drv = new FocasDriver(new FocasDriverOptions
{
Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")],
Probe = new FocasProbeOptions
{
Enabled = true,
Interval = TimeSpan.FromSeconds(10),
Timeout = probeTimeout,
},
}, "drv-1", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
// Wait up to a generous bound for the probe to observe cancellation. Without the
// timeout fix, this never completes (Probe runs forever).
var cancelled = await Task.WhenAny(
hangSignal.Task,
Task.Delay(TimeSpan.FromSeconds(2))) == hangSignal.Task;
await drv.ShutdownAsync(CancellationToken.None);
cancelled.ShouldBeTrue(
"ProbeAsync must be cancelled at Probe.Timeout when it does not complete; otherwise a hung CNC blocks the probe loop indefinitely");
}
// ---- Driver.FOCAS-010 — operation-mode → text mapping is consolidated ----
[Theory]
[InlineData(0, "MDI")]
[InlineData(1, "AUTO")]
[InlineData(2, "TJOG")] // canonical FocasOpMode label, matches both surfaces post-fix
[InlineData(3, "EDIT")]
[InlineData(4, "HANDLE")]
[InlineData(5, "JOG")]
[InlineData(6, "TEACH_IN_HANDLE")]
[InlineData(7, "REFERENCE")]
[InlineData(8, "REMOTE")]
[InlineData(9, "TEST")]
public void OpMode_ToText_yields_the_same_label_in_both_namespaces(int code, string expected)
{
// Driver fixed-tree path (FocasOpMode.ToText) and wire layer (FocasOperationModeExtensions.ToText)
// must yield the same canonical label so dashboard rendering doesn't vary by code path.
FocasOpMode.ToText(code).ShouldBe(expected);
((FocasOperationMode)(short)code).ToText().ShouldBe(expected);
}
[Fact]
public void OpMode_ToText_fallback_label_is_consistent()
{
// Unknown codes must fall back to the same shape from both call sites — previously
// FocasOpMode used "Mode{n}" while FocasOperationModeExtensions used the bare number.
const int unknown = 99;
FocasOpMode.ToText(unknown).ShouldBe(
((FocasOperationMode)(short)unknown).ToText(),
"unknown-mode fallback must agree across both surfaces");
}
// ---- Driver.FOCAS-011 — FocasAlarmType constants typed as short ----
[Fact]
public void FocasAlarmType_constants_are_typed_short()
{
// The downstream switches in FocasAlarmProjection.MapAlarmType / MapSeverity take a short.
// Declaring the constants as int (the old shape) compiled by accident because the values
// fit short range; making them short makes the type match the wire width.
// We assert this at runtime via reflection so the test fails if a future contributor
// demotes them back to int.
var fields = typeof(FocasAlarmType).GetFields(
System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Static);
foreach (var f in fields)
{
f.FieldType.ShouldBe(typeof(short),
$"FocasAlarmType.{f.Name} must be typed `short` so it matches the wire field width " +
"and the FocasAlarmProjection switch arm types");
}
}
// ---- helpers ----
private sealed class CapturingFakeFocasClient(List<FocasAddress> captured) : FakeFocasClient
{
public override Task<(object? value, uint status)> ReadAsync(
FocasAddress address, FocasDataType type, CancellationToken ct)
{
captured.Add(address);
return base.ReadAsync(address, type, ct);
}
public override Task<uint> WriteAsync(
FocasAddress address, FocasDataType type, object? value, CancellationToken ct)
{
captured.Add(address);
return base.WriteAsync(address, type, value, ct);
}
}
private sealed class HangingProbeFakeClient(TaskCompletionSource cancelledSignal) : FakeFocasClient
{
public override async Task<bool> ProbeAsync(CancellationToken ct)
{
try
{
await Task.Delay(Timeout.InfiniteTimeSpan, ct).ConfigureAwait(false);
return true;
}
catch (OperationCanceledException)
{
cancelledSignal.TrySetResult();
throw;
}
}
}
}
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverApiKeyResolverTests.cs
+56
View File
@@ -1,3 +1,4 @@
using Microsoft.Extensions.Logging;
using Shouldly; using Shouldly;
using Xunit; using Xunit;
@@ -69,6 +70,61 @@ public sealed class GalaxyDriverApiKeyResolverTests
ex.Message.ShouldContain("doesn't exist"); ex.Message.ShouldContain("doesn't exist");
} }
// ===== Driver.Galaxy-010 regression: literal arm warns + dev: prefix path =====
[Fact]
public void Literal_string_emits_warning_when_logger_supplied()
{
// A literal API key on a production deployment means the cleartext key sits
// in the DriverConfig JSON. The resolver must surface a warning so an
// operator who committed one by accident sees it at startup.
var logger = new CaptureLogger();
var key = GalaxyDriver.ResolveApiKey("plain-text-key", logger);
key.ShouldBe("plain-text-key");
logger.Entries.ShouldContain(e =>
e.Level == LogLevel.Warning && e.Message.Contains("literal", StringComparison.OrdinalIgnoreCase));
}
[Fact]
public void Dev_prefix_returns_literal_without_warning()
{
// An explicit dev: prefix signals the operator knowingly opted into a literal
// key (dev / parity rig). The resolver must accept it AND suppress the
// warning so production logs aren't polluted on a deliberate dev choice.
var logger = new CaptureLogger();
var key = GalaxyDriver.ResolveApiKey("dev:plain-text-key", logger);
key.ShouldBe("plain-text-key");
logger.Entries.ShouldNotContain(e => e.Level == LogLevel.Warning);
}
[Fact]
public void Env_prefix_does_not_emit_literal_warning()
{
const string name = "OTOPCUA_TEST_GALAXY_API_KEY_NOWARN";
Environment.SetEnvironmentVariable(name, "v");
try
{
var logger = new CaptureLogger();
GalaxyDriver.ResolveApiKey($"env:{name}", logger);
logger.Entries.ShouldNotContain(e => e.Level == LogLevel.Warning);
}
finally
{
Environment.SetEnvironmentVariable(name, null);
}
}
private sealed class CaptureLogger : ILogger
{
public List<(LogLevel Level, string Message)> Entries { get; } = new();
public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
public bool IsEnabled(LogLevel logLevel) => true;
public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func<TState, Exception?, string> formatter)
=> Entries.Add((logLevel, formatter(state, exception)));
}
[Fact] [Fact]
public void File_prefix_empty_file_throws() public void File_prefix_empty_file_throws()
{ {
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverFactoryTests.cs
+15 -3
View File
@@ -141,17 +141,29 @@ public sealed class GalaxyDriverFactoryTests
} }
[Fact] [Fact]
public async Task ReinitializeAsync_RefreshesHealth() public async Task ReinitializeAsync_RefreshesHealth_WhenConfigIsEquivalent()
{ {
// Driver.Galaxy-013: ReinitializeAsync now compares the incoming JSON to the
// live options. An equivalent config is accepted and refreshes health; a
// non-equivalent reapply throws NotSupportedException (covered in
// GalaxyDriverInfrastructureTests.ReinitializeAsync_RejectsNonEquivalentConfigChange).
// Build a config JSON whose parsed shape equals BuildOptions() so the
// equivalence check passes.
const string equivalentConfig = """
{
"Gateway": { "Endpoint": "https://mxgw.test:5001", "ApiKeySecretRef": "key" },
"MxAccess": { "ClientName": "OtOpcUa-A" }
}
""";
using var driver = new GalaxyDriver( using var driver = new GalaxyDriver(
"galaxy-x", BuildOptions(), hierarchySource: null, dataReader: null, "galaxy-x", BuildOptions(), hierarchySource: null, dataReader: null,
dataWriter: null, subscriber: new NoopSubscriber()); dataWriter: null, subscriber: new NoopSubscriber());
await driver.InitializeAsync(MinimalConfig, CancellationToken.None); await driver.InitializeAsync(equivalentConfig, CancellationToken.None);
var firstStamp = driver.GetHealth().LastSuccessfulRead!.Value; var firstStamp = driver.GetHealth().LastSuccessfulRead!.Value;
// Force a measurable clock delta so the comparison is stable on fast machines. // Force a measurable clock delta so the comparison is stable on fast machines.
await Task.Delay(20); await Task.Delay(20);
await driver.ReinitializeAsync(MinimalConfig, CancellationToken.None); await driver.ReinitializeAsync(equivalentConfig, CancellationToken.None);
driver.GetHealth().State.ShouldBe(DriverState.Healthy); driver.GetHealth().State.ShouldBe(DriverState.Healthy);
driver.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(firstStamp); driver.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(firstStamp);
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/GalaxyDriverInfrastructureTests.cs
+115
View File
@@ -85,6 +85,121 @@ public sealed class GalaxyDriverInfrastructureTests
await Should.NotThrowAsync(async () => await driver.DisposeAsync()); await Should.NotThrowAsync(async () => await driver.DisposeAsync());
} }
// ===== Driver.Galaxy-013 regression: ReplayOnSessionLost gates the replay step =====
[Fact]
public async Task ReplayOnSessionLost_False_SkipsResubscribeBulk()
{
// ReplayOnSessionLost was a dangling option — defined + documented but never
// read. After the fix, setting it to false makes the reconnect replay path
// skip SubscribeBulk (operator opts out of replay; the gateway's session-level
// ReplaySubscriptions handles state restoration).
var sub = new ReplayCountingSubscriber();
var opts = new GalaxyDriverOptions(
new GalaxyGatewayOptions("https://mxgw.test:5001", "key"),
new GalaxyMxAccessOptions("InfraTest"),
new GalaxyRepositoryOptions(WatchDeployEvents: false),
new GalaxyReconnectOptions(ReplayOnSessionLost: false));
using var driver = new GalaxyDriver("drv-1", opts, null, null, null, sub);
// Establish a subscription so the replay path has something to walk.
await driver.SubscribeAsync(["Tag.A", "Tag.B"], TimeSpan.Zero, CancellationToken.None);
sub.SubscribeCalls.ShouldBe(1);
// Invoke the replay path directly via the internal test seam — the supervisor's
// ReportTransportFailure spins it up async; for a deterministic assertion we
// call the helper that ReplayAsync is wired against.
await driver.InvokeReplayForTestAsync(CancellationToken.None);
sub.SubscribeCalls.ShouldBe(1,
"ReplayOnSessionLost=false must skip the re-SubscribeBulk fan-out on reconnect");
}
[Fact]
public async Task ReplayOnSessionLost_True_RunsResubscribeBulk()
{
var sub = new ReplayCountingSubscriber();
var opts = new GalaxyDriverOptions(
new GalaxyGatewayOptions("https://mxgw.test:5001", "key"),
new GalaxyMxAccessOptions("InfraTest"),
new GalaxyRepositoryOptions(WatchDeployEvents: false),
new GalaxyReconnectOptions(ReplayOnSessionLost: true));
using var driver = new GalaxyDriver("drv-1", opts, null, null, null, sub);
await driver.SubscribeAsync(["Tag.A"], TimeSpan.Zero, CancellationToken.None);
sub.SubscribeCalls.ShouldBe(1);
await driver.InvokeReplayForTestAsync(CancellationToken.None);
sub.SubscribeCalls.ShouldBe(2,
"default ReplayOnSessionLost=true must re-issue SubscribeBulk after a transport drop");
}
private sealed class ReplayCountingSubscriber : IGalaxySubscriber
{
private readonly Channel<MxEvent> _stream = Channel.CreateUnbounded<MxEvent>();
private int _nextHandle = 1;
public int SubscribeCalls;
public Task<IReadOnlyList<SubscribeResult>> SubscribeBulkAsync(
IReadOnlyList<string> fullReferences, int bufferedUpdateIntervalMs, CancellationToken cancellationToken)
{
Interlocked.Increment(ref SubscribeCalls);
var results = fullReferences.Select(r => new SubscribeResult
{
TagAddress = r,
ItemHandle = Interlocked.Increment(ref _nextHandle),
WasSuccessful = true,
}).ToList();
return Task.FromResult<IReadOnlyList<SubscribeResult>>(results);
}
public Task UnsubscribeBulkAsync(IReadOnlyList<int> itemHandles, CancellationToken cancellationToken)
=> Task.CompletedTask;
public IAsyncEnumerable<MxEvent> StreamEventsAsync(CancellationToken cancellationToken)
=> _stream.Reader.ReadAllAsync(cancellationToken);
}
// ===== Driver.Galaxy-013 regression: ReinitializeAsync rejects unsupported reapply =====
[Fact]
public async Task ReinitializeAsync_RejectsNonEquivalentConfigChange()
{
// ReinitializeAsync was previously a silent no-op that ignored driverConfigJson.
// After the fix it either applies an equivalent config (no-op) or throws
// NotSupportedException so a config change isn't silently dropped.
var sub = new NoOpSubscriber();
using var driver = new GalaxyDriver("drv-1", Opts(), null, null, null, sub);
const string newConfig = "{\"Gateway\":{\"Endpoint\":\"https://other.test:5001\",\"ApiKeySecretRef\":\"dev:other\"}}";
// The driver must NOT pretend the change was applied — either no-op equivalence
// or an explicit rejection is acceptable. Silently dropping the new config
// (the previous behaviour) is not.
await Should.ThrowAsync<NotSupportedException>(async () =>
await driver.ReinitializeAsync(newConfig, CancellationToken.None));
}
[Fact]
public async Task ReinitializeAsync_AcceptsEquivalentConfig()
{
var sub = new NoOpSubscriber();
using var driver = new GalaxyDriver("drv-1", Opts(), null, null, null, sub);
// An empty / null-equivalent config reapply (no field changes) must not throw —
// it's a legitimate "refresh health" path. Pass a JSON object that round-trips
// to the driver's current options.
var json = "{\"Gateway\":{\"Endpoint\":\"https://mxgw.test:5001\",\"ApiKeySecretRef\":\"key\"}," +
"\"MxAccess\":{\"ClientName\":\"InfraTest\"}," +
"\"Repository\":{\"WatchDeployEvents\":false}," +
"\"Reconnect\":{}}";
await Should.NotThrowAsync(async () =>
await driver.ReinitializeAsync(json, CancellationToken.None));
}
// ===== Minimal IGalaxySubscriber fake that returns empty results for subscribe calls ===== // ===== Minimal IGalaxySubscriber fake that returns empty results for subscribe calls =====
private sealed class NoOpSubscriber : IGalaxySubscriber private sealed class NoOpSubscriber : IGalaxySubscriber
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/SubscriptionRegistryTests.cs
+30
View File
@@ -183,6 +183,36 @@ public sealed class SubscriptionRegistryTests
registry.ResolveSubscribers(0).ShouldBeEmpty(); registry.ResolveSubscribers(0).ShouldBeEmpty();
} }
// ===== Driver.Galaxy-012 regression: ResolveSubscribers is O(1) per binding =====
[Fact]
public void ResolveSubscribers_LargeBindingSet_DispatchesCorrectly()
{
// 5000-tag subscription. ResolveSubscribers must still return the right
// full-reference for any item handle without a linear scan of the entire
// binding list — the old FirstOrDefault(b => b.ItemHandle == h) was O(n)
// per dispatch, so 50k tags × 1Hz fan-out was 50k linear scans per second.
var registry = new SubscriptionRegistryAccess();
const int tagCount = 5000;
var bindings = new List<TagBindingAccess>(tagCount);
for (var i = 0; i < tagCount; i++)
{
bindings.Add(new TagBindingAccess($"Tag.{i}", 1000 + i));
}
registry.Register(1, bindings);
// Pull the last entry — the worst case for a linear scan.
var subs = registry.ResolveSubscribers(1000 + tagCount - 1);
subs.Count.ShouldBe(1);
subs[0].FullReference.ShouldBe($"Tag.{tagCount - 1}");
// Mid-range entry too — proves the index isn't position-dependent.
var mid = registry.ResolveSubscribers(1000 + tagCount / 2);
mid.Count.ShouldBe(1);
mid[0].FullReference.ShouldBe($"Tag.{tagCount / 2}");
}
// Internal types are accessed via friend assembly (InternalsVisibleTo); these // Internal types are accessed via friend assembly (InternalsVisibleTo); these
// wrapper aliases keep the test code readable. // wrapper aliases keep the test code readable.
private sealed class SubscriptionRegistryAccess private sealed class SubscriptionRegistryAccess
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs
+191
View File
@@ -0,0 +1,191 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests;
/// <summary>
/// Regression tests for the remaining code-review findings closed against the S7 driver:
/// Driver.S7-003 (Read/WriteAsync null-arg validation), Driver.S7-009 (poll-loop health
/// update + backoff), Driver.S7-010 (Dispose without sync-over-async), and Driver.S7-013
/// (reject not-yet-implemented S7DataType values at init).
/// </summary>
[Trait("Category", "Unit")]
public sealed class S7DriverCodeReviewFixTests2
{
// ── Driver.S7-003 — Read/WriteAsync must throw ArgumentNullException, not NRE ─────────
[Fact]
public async Task ReadAsync_with_null_fullReferences_throws_ArgumentNullException()
{
// The driver must validate its inputs consistently with DiscoverAsync (which already
// uses ArgumentNullException.ThrowIfNull). A NullReferenceException escaping the entry
// point bypasses the gate and gives the caller a non-actionable stack.
using var drv = new S7Driver(new S7DriverOptions { Host = "192.0.2.1" }, "s7-null-read");
await Should.ThrowAsync<ArgumentNullException>(async () =>
await drv.ReadAsync(null!, TestContext.Current.CancellationToken));
}
[Fact]
public async Task WriteAsync_with_null_writes_throws_ArgumentNullException()
{
using var drv = new S7Driver(new S7DriverOptions { Host = "192.0.2.1" }, "s7-null-write");
await Should.ThrowAsync<ArgumentNullException>(async () =>
await drv.WriteAsync(null!, TestContext.Current.CancellationToken));
}
// ── Driver.S7-009 — Poll loop must update health on sustained failure ────────────────
[Fact]
public async Task PollLoop_against_uninitialized_driver_degrades_health()
{
// Subscribing without InitializeAsync means RequirePlc() throws on every poll tick.
// Previously the empty catch swallowed everything and the dashboard reported Healthy
// / Unknown indefinitely. With the fix the poll loop must surface the failure on the
// health surface so an operator can see it (driver-stability convention).
var opts = new S7DriverOptions
{
Host = "192.0.2.1",
Probe = new S7ProbeOptions { Enabled = false },
};
var drv = new S7Driver(opts, "s7-poll-health");
await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken);
// Wait long enough for several poll ticks. With backoff the second tick should arrive
// within ~100-200 ms; allow generous slack.
for (var i = 0; i < 40 && drv.GetHealth().State is DriverState.Unknown or DriverState.Initializing; i++)
await Task.Delay(50, TestContext.Current.CancellationToken);
drv.GetHealth().State.ShouldBe(DriverState.Degraded,
"sustained poll failure must surface on the health state — see Driver.S7-009");
await drv.ShutdownAsync(CancellationToken.None);
await drv.DisposeAsync();
}
[Fact]
public async Task PollLoop_applies_capped_backoff_after_consecutive_failures()
{
// After repeated poll errors the loop must back off rather than burn CPU re-polling
// every Interval — Driver.S7-009 calls for a capped backoff. Inspect the ratio of
// observed tick count to the floor count we would have seen WITHOUT backoff over a
// short window. Without backoff, an Interval=50 ms loop with sub-ms ReadAsync
// RequirePlc-throw would tick ~20 times in 1 s. With capped backoff it ticks far
// fewer; we use a generous upper bound that still proves "something is throttling".
var opts = new S7DriverOptions
{
Host = "192.0.2.1",
Probe = new S7ProbeOptions { Enabled = false },
};
var drv = new S7Driver(opts, "s7-poll-backoff");
var ticks = 0;
drv.OnDataChange += (_, _) => Interlocked.Increment(ref ticks);
await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken);
// OnDataChange is never raised (every poll fails) so we can't count via the event.
// Instead, time-bound the test and rely on the health-degradation test above to prove
// the catch ran; here we just confirm the driver doesn't deadlock or spin so hot it
// refuses to shut down. ShutdownAsync must complete within the drain window.
await Task.Delay(500, TestContext.Current.CancellationToken);
var sw = System.Diagnostics.Stopwatch.StartNew();
await drv.ShutdownAsync(CancellationToken.None);
sw.Stop();
sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(6),
"shutdown must complete inside the drain timeout — a runaway backoff would block it");
await drv.DisposeAsync();
_ = ticks; // silence unused
}
// ── Driver.S7-010 — Dispose() must not deadlock via sync-over-async ──────────────────
[Fact]
public void Dispose_completes_synchronously_without_sync_over_async_round_trip()
{
// The sync Dispose() path must perform the teardown directly rather than blocking on
// DisposeAsync().AsTask().GetAwaiter().GetResult(). The current sync-over-async pattern
// is a known deadlock risk even when the wrapped Task.Run paths happen to be safe.
// We assert by measuring Dispose() runtime on a no-subscription, no-init driver: it
// should complete in microseconds, not the ~5 s drain window that a hung sync-over-async
// would burn waiting on a never-completing continuation.
var drv = new S7Driver(new S7DriverOptions { Host = "192.0.2.1" }, "s7-dispose-sync");
var sw = System.Diagnostics.Stopwatch.StartNew();
drv.Dispose();
sw.Stop();
sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(1),
"Dispose() must teardown directly — see Driver.S7-010");
}
[Fact]
public void Dispose_is_idempotent()
{
// After the rewrite Dispose() must remain safe to call twice — disposal is
// best-effort and the second call must not throw.
var drv = new S7Driver(new S7DriverOptions { Host = "192.0.2.1" }, "s7-dispose-twice");
drv.Dispose();
Should.NotThrow(() => drv.Dispose());
}
// ── Driver.S7-013 — Reject not-yet-implemented S7DataType values at init ─────────────
[Theory]
[InlineData(S7DataType.Int64)]
[InlineData(S7DataType.UInt64)]
[InlineData(S7DataType.Float64)]
[InlineData(S7DataType.String)]
[InlineData(S7DataType.DateTime)]
public async Task Initialize_rejects_not_yet_implemented_data_type_with_NotSupportedException(S7DataType dt)
{
// A tag declared with one of the not-yet-wired data types parses cleanly and creates
// an OPC UA node via DiscoverAsync — then every Read/Write of it returns BadNotSupported.
// The half-implemented type must be rejected at init so a site can't deploy a config
// that produces dead nodes (Driver.S7-013).
var opts = new S7DriverOptions
{
Host = "192.0.2.1",
Timeout = TimeSpan.FromMilliseconds(250),
// Use a DB.DBD address — the parser accepts it for every data type. The init guard
// must fault on the data-type rather than on the address.
Tags = [new S7TagDefinition("X", "DB1.DBD0", dt)],
};
using var drv = new S7Driver(opts, $"s7-bad-dt-{dt}");
var ex = await Should.ThrowAsync<NotSupportedException>(async () =>
await drv.InitializeAsync("{}", TestContext.Current.CancellationToken));
ex.Message.ShouldContain(dt.ToString());
var health = drv.GetHealth();
health.State.ShouldBe(DriverState.Faulted);
}
[Theory]
[InlineData(S7DataType.Bool, "DB1.DBX0.0")]
[InlineData(S7DataType.Byte, "DB1.DBB0")]
[InlineData(S7DataType.Int16, "DB1.DBW0")]
[InlineData(S7DataType.UInt16, "DB1.DBW0")]
[InlineData(S7DataType.Int32, "DB1.DBD0")]
[InlineData(S7DataType.UInt32, "DB1.DBD0")]
[InlineData(S7DataType.Float32, "DB1.DBD0")]
public async Task Initialize_accepts_implemented_data_types(S7DataType dt, string addr)
{
// Sanity check the guard is targeted — implemented data types must still pass. The
// TCP connect still fails (reserved host); the failure must NOT be a NotSupportedException
// from the data-type guard.
var opts = new S7DriverOptions
{
Host = "192.0.2.1",
Timeout = TimeSpan.FromMilliseconds(250),
Tags = [new S7TagDefinition("X", addr, dt)],
};
using var drv = new S7Driver(opts, $"s7-good-dt-{dt}");
var ex = await Should.ThrowAsync<Exception>(async () =>
await drv.InitializeAsync("{}", TestContext.Current.CancellationToken));
ex.ShouldNotBeOfType<NotSupportedException>(
"implemented data types must pass the init guard — the failure must be the connect");
}
}