diff --git a/code-reviews/Driver.AbCip/findings.md b/code-reviews/Driver.AbCip/findings.md index 610ed54..ba5f840 100644 --- a/code-reviews/Driver.AbCip/findings.md +++ b/code-reviews/Driver.AbCip/findings.md @@ -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` / `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. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipAlarmProjection.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipAlarmProjection.cs index 17a8f97..b817add 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipAlarmProjection.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipAlarmProjection.cs @@ -1,3 +1,5 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; @@ -32,14 +34,16 @@ internal sealed class AbCipAlarmProjection : IAsyncDisposable { private readonly AbCipDriver _driver; private readonly TimeSpan _pollInterval; + private readonly ILogger _logger; private readonly Dictionary _subs = new(); private readonly Lock _subsLock = new(); private long _nextId; - public AbCipAlarmProjection(AbCipDriver driver, TimeSpan pollInterval) + public AbCipAlarmProjection(AbCipDriver driver, TimeSpan pollInterval, ILogger? logger = null) { _driver = driver; _pollInterval = pollInterval; + _logger = logger ?? NullLogger.Instance; } public async Task SubscribeAsync( @@ -158,7 +162,14 @@ internal sealed class AbCipAlarmProjection : IAsyncDisposable Tick(sub, results); } 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); } catch (OperationCanceledException) { break; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDataType.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDataType.cs index b0c4e89..ed553f3 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDataType.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDataType.cs @@ -5,7 +5,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// /// Logix atomic + string data types, plus a marker used when a tag /// 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 +/// + ). /// /// /// Mirrors the shape of ModbusDataType. Atomic Logix names (BOOL / SINT / INT / DINT / diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index b25ed40..a90fbb9 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -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.Driver.AbCip.PlcFamilies; @@ -34,6 +36,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, private readonly PollGroupEngine _poll; private readonly Dictionary _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary _tagsByName = new(StringComparer.OrdinalIgnoreCase); + private readonly ILogger _logger; private AbCipAlarmProjection _alarmProjection; 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, IAbCipTagFactory? tagFactory = null, IAbCipTagEnumeratorFactory? enumeratorFactory = null, - IAbCipTemplateReaderFactory? templateReaderFactory = null) + IAbCipTemplateReaderFactory? templateReaderFactory = null, + ILogger? logger = null) { ArgumentNullException.ThrowIfNull(options); _options = options; @@ -55,11 +59,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, _tagFactory = tagFactory ?? new LibplctagTagFactory(); _enumeratorFactory = enumeratorFactory ?? new LibplctagTagEnumeratorFactory(); _templateReaderFactory = templateReaderFactory ?? new LibplctagTemplateReaderFactory(); + _logger = logger ?? NullLogger.Instance; _poll = new PollGroupEngine( reader: ReadAsync, onChange: (handle, tagRef, snapshot) => OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot))); - _alarmProjection = new AbCipAlarmProjection(this, _options.AlarmPollInterval); + _alarmProjection = new AbCipAlarmProjection(this, _options.AlarmPollInterval, _logger); } /// @@ -77,13 +82,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, if (!_devices.TryGetValue(deviceHostAddress, out var device)) return null; - var deviceParams = new AbCipTagCreateParams( - Gateway: device.ParsedAddress.Gateway, - Port: device.ParsedAddress.Port, - CipPath: device.ParsedAddress.CipPath, - LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute, - TagName: $"@udt/{templateInstanceId}", - Timeout: _options.Timeout); + var deviceParams = device.BuildCreateParams($"@udt/{templateInstanceId}", _options.Timeout); try { @@ -95,16 +94,23 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, return shape; } catch (OperationCanceledException) { throw; } - catch + catch (Exception ex) { - // Template read failure — log via the driver's health surface so operators see it, - // but don't propagate since callers should fall back to declaration-driven UDT - // semantics rather than failing the whole discovery run. + // Template read failure — surface via the driver's health surface AND a warning + // log so operators see it; don't propagate since callers should fall back to + // 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; } } - /// Shared UDT template cache. Exposed for PR 6 (UDT reader) + diagnostics. + /// + /// Shared UDT template cache populated by . Exposed + /// internally so tests + diagnostics can inspect cached shapes. + /// internal AbCipTemplateCache TemplateCache => _templateCache; 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) { _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); } } + 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); } catch (Exception ex) { _health = new DriverHealth(DriverState.Faulted, null, ex.Message); + _logger.LogError(ex, "AbCip driver {DriverInstanceId} failed to initialize", _driverInstanceId); throw; } return Task.CompletedTask; @@ -307,13 +326,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, private async Task ProbeLoopAsync(DeviceState state, CancellationToken ct) { - var probeParams = new AbCipTagCreateParams( - Gateway: state.ParsedAddress.Gateway, - Port: state.ParsedAddress.Port, - CipPath: state.ParsedAddress.CipPath, - LibplctagPlcAttribute: state.Profile.LibplctagPlcAttribute, - TagName: _options.Probe.ProbeTagPath!, - Timeout: _options.Probe.Timeout); + var probeParams = state.BuildCreateParams(_options.Probe.ProbeTagPath!, _options.Probe.Timeout); IAbCipTagRuntime? probeRuntime = null; while (!ct.IsCancellationRequested) @@ -336,9 +349,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { break; } - catch + catch (Exception ex) { // 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 { } probeRuntime = null; 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 // 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 // declaration-only offsets can't place them under Logix alignment rules. Whole-UDT // 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); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, $"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; } @@ -480,6 +502,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadCommunicationError, null, now); _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); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, $"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; } @@ -533,6 +562,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, EvictRuntime(device, parent.Name); // Driver.AbCip-010 StampGroupStatus(group, results, now, AbCipStatusMapper.BadCommunicationError); _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 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 { @@ -621,22 +657,34 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Type/protocol error — not a transport fault; don't evict the handle. results[i] = new WriteResult(AbCipStatusMapper.BadNotSupported); _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) { // Value conversion error — not a transport fault; don't evict. results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); _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) { results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); _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) { results[i] = new WriteResult(AbCipStatusMapper.BadOutOfRange); _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) { @@ -644,6 +692,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, EvictRuntime(device, def.Name); // Driver.AbCip-010 results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError); _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; - var runtime = _tagFactory.Create(new AbCipTagCreateParams( - Gateway: device.ParsedAddress.Gateway, - Port: device.ParsedAddress.Port, - CipPath: device.ParsedAddress.CipPath, - LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute, - TagName: parentTagName, - Timeout: _options.Timeout)); + var runtime = _tagFactory.Create(device.BuildCreateParams(parentTagName, _options.Timeout)); try { await runtime.InitializeAsync(ct).ConfigureAwait(false); @@ -736,13 +781,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, ?? throw new InvalidOperationException( $"AbCip tag '{def.Name}' has malformed TagPath '{def.TagPath}'."); - var runtime = _tagFactory.Create(new AbCipTagCreateParams( - Gateway: device.ParsedAddress.Gateway, - Port: device.ParsedAddress.Port, - CipPath: device.ParsedAddress.CipPath, - LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute, - TagName: parsed.ToLibplctagName(), - Timeout: _options.Timeout)); + var runtime = _tagFactory.Create(device.BuildCreateParams(parsed.ToLibplctagName(), _options.Timeout)); try { 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)) { using var enumerator = _enumeratorFactory.Create(); - var deviceParams = new AbCipTagCreateParams( - Gateway: state.ParsedAddress.Gateway, - Port: state.ParsedAddress.Port, - CipPath: state.ParsedAddress.CipPath, - LibplctagPlcAttribute: state.Profile.LibplctagPlcAttribute, - TagName: "@tags", - Timeout: _options.Timeout); + var deviceParams = state.BuildCreateParams("@tags", _options.Timeout); IAddressSpaceBuilder? discoveredFolder = null; 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) => _rmwLocks.GetOrAdd(parentTagName, _ => new SemaphoreSlim(1, 1)); + /// + /// Driver.AbCip-013 — compute the effective for a + /// tag on this device. Combines the per-device options + /// (, + /// ) with the family profile defaults + /// so the wire layer sees one place that resolves both. + /// + 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() { foreach (var r in Runtimes.Values) r.Dispose(); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs index bd94291..9c1c626 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs @@ -51,7 +51,9 @@ public static class AbCipDriverFactoryExtensions $"AB CIP config for '{driverInstanceId}' has a device missing HostAddress"), PlcFamily: ParseEnum(d.PlcFamily, "device", driverInstanceId, "PlcFamily", fallback: AbCipPlcFamily.ControlLogix), - DeviceName: d.DeviceName))] + DeviceName: d.DeviceName, + AllowPacking: d.AllowPacking, + ConnectionSize: d.ConnectionSize))] : [], Tags = dto.Tags is { Count: > 0 } ? [.. dto.Tags.Select(t => BuildTag(t, driverInstanceId))] @@ -133,6 +135,8 @@ public static class AbCipDriverFactoryExtensions public string? HostAddress { get; init; } public string? PlcFamily { get; init; } public string? DeviceName { get; init; } + public bool? AllowPacking { get; init; } + public int? ConnectionSize { get; init; } } internal sealed class AbCipTagDto diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverOptions.cs index 175892e..4eccf31 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverOptions.cs @@ -18,7 +18,11 @@ public sealed class AbCipDriverOptions /// public IReadOnlyList Devices { get; init; } = []; - /// Pre-declared tag map across all devices — AB discovery lands in PR 5. + /// + /// Pre-declared tag map across all devices. Pre-declared tags always emit during + /// discovery; opt in to controller-side discovery via + /// . + /// public IReadOnlyList Tags { get; init; } = []; /// Per-device probe settings. Falls back to defaults when omitted. @@ -78,13 +82,28 @@ public sealed class AbCipDriverOptions /// initialization rather than silently connecting to nothing. /// /// Canonical ab://gateway[:port]/cip-path string. -/// Which per-family profile to apply. Determines ConnectionSize, -/// request-packing support, unconnected-only hint, and other quirks. +/// Which per-family profile to apply. Determines the family +/// AllowPacking default, ConnectionSize default, unconnected-only hint, and +/// other quirks; per-device overrides via and +/// take precedence when set. /// Optional display label for Admin UI. Falls back to . +/// Driver.AbCip-013 — per-device override for CIP request-packing +/// (firmware 20+). null (the default) inherits the family profile's +/// SupportsRequestPacking; set explicitly to opt a single device in or out without +/// touching every other device on the same family. +/// Driver.AbCip-013 — per-device override for the Forward Open +/// ConnectionSize (Large Forward Open packet size in bytes). null inherits the family +/// profile's DefaultConnectionSize. Honoured by the driver layer; the underlying +/// libplctag 1.5.2 wrapper has no direct ConnectionSize property, so the value is +/// plumbed through 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. public sealed record AbCipDeviceOptions( string HostAddress, AbCipPlcFamily PlcFamily = AbCipPlcFamily.ControlLogix, - string? DeviceName = null); + string? DeviceName = null, + bool? AllowPacking = null, + int? ConnectionSize = null); /// /// One AB-backed OPC UA variable. Mirrors the ModbusTagDefinition shape. @@ -149,9 +168,11 @@ public sealed class AbCipProbeOptions public TimeSpan Timeout { get; init; } = TimeSpan.FromSeconds(2); /// - /// Tag path used for the probe. If null, the driver attempts to read a default - /// system tag (PR 8 wires this up — the choice is family-dependent, e.g. - /// @raw_cpu_type on ControlLogix or a user-configured probe tag on Micro800). + /// Tag path used for the probe. When is true but this is + /// null/blank, the driver logs a warning and runs no probe loops (Driver.AbCip-011); + /// GetHostStatuses() will then report every device as Unknown. A family-default + /// system-tag fallback (e.g. @raw_cpu_type on ControlLogix) is a deferred follow-up; + /// today an operator opting into the probe must supply a tag path explicitly. /// public string? ProbeTagPath { get; init; } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagPath.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagPath.cs index 0891664..1533e95 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagPath.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagPath.cs @@ -9,8 +9,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// attribute consumes. /// /// -/// Scope + members + subscripts are captured structurally so PR 6 (UDT support) can walk -/// the path against a cached template without re-parsing. is +/// Scope + members + subscripts are captured structurally so UDT support can walk the +/// path against a cached template (see ) without +/// re-parsing. is /// non-null only when the trailing segment is a decimal integer between 0 and 31 that /// parses as a bit-selector — this is the .N syntax documented in the Logix 5000 /// General Instructions Reference §Tags, and it applies only to DINT-typed parents. The diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTemplateCache.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTemplateCache.cs index 3641b6d..386a7d4 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTemplateCache.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTemplateCache.cs @@ -9,9 +9,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// ReinitializeAsync. /// /// -/// Template shape read (CIP Template Object class 0x6C, GetAttributeList + -/// Read Template) lands with PR 6. This class ships the cache surface so PR 6 can -/// drop the decoder in without reshaping any caller code. +/// Templates are decoded by (CIP Template Object +/// class 0x6C, GetAttributeList + Read Template); the live reader is +/// , and +/// populates this cache on first fetch. /// public sealed class AbCipTemplateCache { @@ -36,8 +37,8 @@ public sealed class AbCipTemplateCache /// /// 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; -/// no reader writes to it yet. +/// by from a Template Object response buffer +/// read via . /// /// UDT name as reported by the Template Object. /// Bytes the UDT occupies in a whole-UDT read buffer. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagEnumerator.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagEnumerator.cs index 1847a05..fdaa7d7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagEnumerator.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagEnumerator.cs @@ -3,11 +3,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// /// Swappable scanner that walks a controller's symbol table (via libplctag's /// @tags pseudo-tag or the CIP Symbol Object class 0x6B) and yields the tags it -/// finds. Defaults to which returns no -/// controller-side tags — the full @tags decoder lands as a follow-up PR once -/// libplctag 1.5.2 either gains TagInfoPlcMapper upstream or we ship our own -/// IPlcMapper for the Symbol Object byte layout (tracked via follow-up task; PR 5 -/// ships the abstraction + pre-declared-tag emission). +/// finds. Production default is which speaks +/// to the live controller; is available for +/// tests / strict-config deployments where only pre-declared tags should appear. /// public interface IAbCipTagEnumerator : IDisposable { @@ -30,7 +28,8 @@ public interface IAbCipTagEnumeratorFactory /// Logix symbolic name as returned by the Symbol Object. /// Program name if the tag is program-scoped; null for controller scope. /// Detected data type; when the tag -/// is UDT-typed — the UDT shape lookup + per-member expansion ship with PR 6. +/// is UDT-typed — per-member expansion runs against the cached +/// via . /// true when the Symbol Object's External Access attribute forbids writes. /// Hint from the enumerator that this is a system / infrastructure tag; /// the driver applies on top so the enumerator is not the @@ -43,9 +42,10 @@ public sealed record AbCipDiscoveredTag( bool IsSystemTag = false); /// -/// Default production enumerator — currently returns an empty sequence. The real @tags -/// walk lands as a follow-up PR. Documented in driver-specs.md §3 as the gap the -/// Symbol Object walker closes. +/// No-op enumerator returning an empty sequence. Useful for tests + strict-config +/// deployments where is set but the +/// operator wants only pre-declared tags to surface. Production drivers use +/// instead. /// internal sealed class EmptyAbCipTagEnumerator : IAbCipTagEnumerator { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs index bce13a5..a37fda1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs @@ -65,10 +65,19 @@ public interface IAbCipTagFactory /// libplctag plc=... attribute, per family profile. /// Logix symbolic tag name as emitted by . /// libplctag operation timeout (applies to Initialize / Read / Write). +/// CIP request-packing flag — combines the per-device override (if +/// any) with the family profile's SupportsRequestPacking. Forwarded to the libplctag +/// Tag.AllowPacking property (Driver.AbCip-013). +/// Forward Open ConnectionSize — combines the per-device override +/// (if any) with the family profile's DefaultConnectionSize. libplctag 1.5.2 has no +/// direct ConnectionSize property; the value is plumbed for forward-compat with future +/// wrappers / a custom tag-attribute path (Driver.AbCip-013). public sealed record AbCipTagCreateParams( string Gateway, int Port, string CipPath, string LibplctagPlcAttribute, string TagName, - TimeSpan Timeout); + TimeSpan Timeout, + bool AllowPacking = true, + int ConnectionSize = 4002); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagEnumerator.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagEnumerator.cs index 77476f6..45bc3ac 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagEnumerator.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagEnumerator.cs @@ -13,9 +13,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// tag name is @tags. The decoder walks the concatenated entries + emits /// records matching our driver surface. /// -/// Task #178 closed the stub gap from PR 5 — -/// is still available for tests that don't want to touch the native library, but the -/// production factory default now wires this implementation in. +/// remains available for tests that don't want +/// to touch the native library; the production factory default +/// () wires this implementation in. /// internal sealed class LibplctagTagEnumerator : IAbCipTagEnumerator { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs index 946b351..1f78d8f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs @@ -23,7 +23,15 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime Protocol = Protocol.ab_eip, Name = p.TagName, 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); @@ -108,7 +116,11 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime _tag.SetInt32(0, Convert.ToInt32(value)); break; 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: throw new NotSupportedException($"AbCipDataType {type} not writable."); } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTemplateReader.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTemplateReader.cs index 009660e..f5fc70c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTemplateReader.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTemplateReader.cs @@ -8,6 +8,19 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// internally via a normal read call, + returns the raw byte buffer so /// can decode it. /// +/// +/// Driver.AbCip-012 — by design each FetchUdtShapeAsync call creates one fresh +/// , 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 +/// ), (b) libplctag pools its underlying CIP connections per +/// gateway+path so the underlying TCP/EIP session is reused even when individual +/// 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 @udt-capable per +/// device for the duration of a discovery run. +/// internal sealed class LibplctagTemplateReader : IAbCipTemplateReader { private Tag? _tag; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/PlcFamilies/AbCipPlcFamilyProfile.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/PlcFamilies/AbCipPlcFamilyProfile.cs index 56ab5f3..1d64c79 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/PlcFamilies/AbCipPlcFamilyProfile.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/PlcFamilies/AbCipPlcFamilyProfile.cs @@ -8,7 +8,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.PlcFamilies; /// /// 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. -/// Family-specific wire tests ship in PRs 9–12. +/// Family-specific behaviour (ControlLogix / CompactLogix / Micro800 / GuardLogix) is +/// covered by the unit tests in AbCipPlcFamilyTests. /// public sealed record AbCipPlcFamilyProfile( string LibplctagPlcAttribute, diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLoggingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLoggingTests.cs new file mode 100644 index 0000000..12d23b4 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLoggingTests.cs @@ -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; + +/// +/// 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. +/// +[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 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(); + 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(); + 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(); + 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(); + 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(); + 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(); + 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 : ILogger + { + public List<(LogLevel Level, string Message, Exception? Exception)> Entries { get; } = new(); + + public IDisposable BeginScope(TState state) where TState : notnull => NullScope.Instance; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + { + Entries.Add((logLevel, formatter(state, exception), exception)); + } + + private sealed class NullScope : IDisposable + { + public static NullScope Instance { get; } = new(); + public void Dispose() { } + } + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipPerDeviceConnectionOptionsTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipPerDeviceConnectionOptionsTests.cs new file mode 100644 index 0000000..37fdda2 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipPerDeviceConnectionOptionsTests.cs @@ -0,0 +1,134 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Regression coverage for Driver.AbCip-013 — driver-specs.md §3 lists per-device +/// AllowPacking and ConnectionSize as configurable connection settings, but +/// the implementation previously hard-coded both from the family profile and never wired +/// them through to the libplctag Tag. The fix exposes them as per-device options on +/// , plumbs them through , +/// and applies AllowPacking to the live Tag. ConnectionSize 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). +/// +[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); + } +}