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

fix(driver-abcip): resolve Low code-review findings (Driver.AbCip-007,011,012,013,015)

Browse Source 
- 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>
This commit is contained in:
Joseph Doherty
2026-05-23 07:45:19 -04:00
parent 9f7ae20995
commit 77b8686199
16 changed files with 544 additions and 89 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
code-reviews/Driver.AbCip/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 5 |
| Open findings | 0 |
## Checklist coverage
@@ -123,13 +123,13 @@
| Severity | Low |
| Category | OtOpcUa conventions |
| 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.
**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
@@ -183,13 +183,13 @@
| Severity | Low |
| Category | Error handling & resilience |
| 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.
**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
@@ -198,13 +198,13 @@
| Severity | Low |
| Category | Performance & resource management |
| 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`.
**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
@@ -213,13 +213,13 @@
| Severity | Low |
| Category | Design-document adherence |
| 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.
**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
@@ -243,10 +243,10 @@
| Severity | Low |
| 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` |
| 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.
**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.