diff --git a/code-reviews/Driver.TwinCAT/findings.md b/code-reviews/Driver.TwinCAT/findings.md index 53267c2..2afc0ff 100644 --- a/code-reviews/Driver.TwinCAT/findings.md +++ b/code-reviews/Driver.TwinCAT/findings.md @@ -134,7 +134,7 @@ date/time semantics are intended to be exposed properly, track a follow-up to de | Severity | Medium | | Category | OtOpcUa conventions | | Location | `TwinCATDriver.cs` (whole file), `AdsTwinCATClient.cs` (whole file) | -| Status | Open | +| Status | Resolved | **Description:** The driver performs no logging. `CLAUDE.md` Library Preferences mandate Serilog with a rolling daily file sink. Connect failures, ADS error codes, symbol-browse @@ -147,7 +147,7 @@ success/failure per device, ADS errors with code, symbol-browse fallback (the `D catch), native-notification registration failures, and host state transitions (`TransitionDeviceState`). -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added optional `ILogger` constructor parameter (defaults to `NullLogger`); logs connect success/failure in `EnsureConnectedAsync`, ADS read errors in `ReadAsync`, symbol-browse fallback in `DiscoverAsync`, notification-registration failures in `SubscribeAsync`, and host-state transitions in `TransitionDeviceState`. ### Driver.TwinCAT-006 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs index ba02e32..b4035c9 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -1,4 +1,6 @@ using System.Collections.Concurrent; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; @@ -16,6 +18,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery private TwinCATDriverOptions _options; private readonly string _driverInstanceId; private readonly ITwinCATClientFactory _clientFactory; + private readonly ILogger _logger; private readonly PollGroupEngine _poll; private readonly Dictionary _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary _tagsByName = new(StringComparer.OrdinalIgnoreCase); @@ -26,12 +29,14 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery public event EventHandler? OnRediscoveryNeeded; public TwinCATDriver(TwinCATDriverOptions options, string driverInstanceId, - ITwinCATClientFactory? clientFactory = null) + ITwinCATClientFactory? clientFactory = null, + ILogger? logger = null) { ArgumentNullException.ThrowIfNull(options); _options = options; _driverInstanceId = driverInstanceId; _clientFactory = clientFactory ?? new AdsTwinCATClientFactory(); + _logger = logger ?? NullLogger.Instance; _poll = new PollGroupEngine( reader: ReadAsync, onChange: (handle, tagRef, snapshot) => @@ -155,8 +160,13 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery if (status == TwinCATStatusMapper.Good) _health = new DriverHealth(DriverState.Healthy, now, null); else + { + _logger.LogWarning( + "TwinCAT driver '{DriverInstanceId}' ADS read error 0x{Status:X8} for '{Reference}'", + _driverInstanceId, status, reference); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, $"ADS status {status:X8} reading {reference}"); + } } catch (OperationCanceledException) { throw; } catch (Exception ex) @@ -286,10 +296,14 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } } catch (OperationCanceledException) { throw; } - catch + catch (Exception ex) { // Symbol-loader failure is non-fatal to discovery — pre-declared tags already - // shipped + operators see the failure in driver health on next read. + // shipped. Log so operators can correlate the partial discovery. + _logger.LogWarning(ex, + "TwinCAT driver '{DriverInstanceId}' symbol browse failed for device " + + "'{HostAddress}'; falling back to pre-declared tags only", + _driverInstanceId, device.HostAddress); } } } @@ -339,11 +353,15 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery registrations.Add(reg); } } - catch + catch (Exception ex) { // On any registration failure, tear down everything we got so far + rethrow. Leaves // the subscription in a clean "never existed" state rather than a half-registered // state the caller has to clean up. + _logger.LogWarning(ex, + "TwinCAT driver '{DriverInstanceId}' native-notification registration failed; " + + "tearing down {Count} partial registrations", + _driverInstanceId, registrations.Count); foreach (var r in registrations) { try { r.Dispose(); } catch { } } throw; } @@ -411,6 +429,9 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery state.HostState = newState; state.HostStateChangedUtc = DateTime.UtcNow; } + _logger.LogInformation( + "TwinCAT driver '{DriverInstanceId}' device '{HostAddress}' state: {OldState} → {NewState}", + _driverInstanceId, state.Options.HostAddress, old, newState); OnHostStatusChanged?.Invoke(this, new HostStatusChangedEventArgs(state.Options.HostAddress, old, newState)); } @@ -456,9 +477,15 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { await client.ConnectAsync(device.ParsedAddress, _options.Timeout, ct) .ConfigureAwait(false); + _logger.LogInformation( + "TwinCAT driver '{DriverInstanceId}' connected to {HostAddress}", + _driverInstanceId, device.Options.HostAddress); } - catch + catch (Exception ex) { + _logger.LogWarning(ex, + "TwinCAT driver '{DriverInstanceId}' failed to connect to {HostAddress}", + _driverInstanceId, device.Options.HostAddress); client.OnSymbolVersionChanged -= HandleSymbolVersionChanged; client.Dispose(); throw;