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

fix(driver-s7): resolve High code-review findings (Driver.S7-001, -006, -007, -011)

Browse Source 
Driver.S7-001: Timer (T{n}) / Counter (C{n}) addresses parsed cleanly but
the read path had no S7DataType or decode case for them, so a Timer/Counter
tag passed fail-fast init and then threw a misleading type-mismatch on every
read. InitializeAsync now runs RejectUnsupportedTagAddresses, throwing a clear
NotSupportedException ("not yet supported", echoing tag name + address) so the
config error fails fast at init.

Driver.S7-006: ShutdownAsync cancelled the probe/poll CTSs but did not await
the fire-and-forget loop tasks before DisposeAsync disposed _gate, letting a
loop iteration mid-semaphore race a disposed object. The probe task is now
tracked in _probeTask and each poll task in SubscriptionState.PollTask;
ShutdownAsync cancels every CTS, awaits Task.WhenAll of those handles with a
bounded 5 s DrainTimeout, then disposes the CTSs and gate. Task.Run is passed
CancellationToken.None so the handle is always awaitable.

Driver.S7-007: a PUT/GET-disabled fault (permanent misconfiguration) was
mapped identically to a transient PlcException — both BadDeviceFailure +
Degraded. ReadAsync/WriteAsync now split the catch via an IsAccessDenied
filter (S7.Net exposes no typed code for AccessingObjectNotAllowed, so the
inner-exception chain is inspected for the "not allowed" marker). Access-denied
now maps to BadNotSupported and Faulted with a config-alert message pointing
at the TIA Portal PUT/GET toggle; genuine device faults stay BadDeviceFailure.

Driver.S7-011: S7Driver ignored driverConfigJson on Initialize/Reinitialize,
so a config change delivered through ReinitializeAsync (the only Core-initiated
in-process recovery path) was silently discarded. Config parsing was factored
into S7DriverFactoryExtensions.ParseOptions; InitializeAsync now re-parses
driverConfigJson and rebuilds _options whenever the document has a real body.
An empty / placeholder document keeps the constructor options.

Adds S7DriverCodeReviewFixTests covering Timer/Counter rejection, config-json
application on Initialize/Reinitialize, and shutdown-drain with active
subscriptions. All 68 S7 driver tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 06:38:01 -04:00
parent d89be2a011
commit 090d2a4b44
4 changed files with 398 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

192
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs
View File

@@ -1,3 +1,4 @@
using System.Collections.Concurrent;
using S7.Net;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
@@ -30,13 +31,27 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
{
// ---- ISubscribable + IHostConnectivityProbe state ----
private readonly System.Collections.Concurrent.ConcurrentDictionary<long, SubscriptionState> _subscriptions = new();
private readonly ConcurrentDictionary<long, SubscriptionState> _subscriptions = new();
private long _nextSubscriptionId;
private readonly object _probeLock = new();
private HostState _hostState = HostState.Unknown;
private DateTime _hostStateChangedUtc = DateTime.UtcNow;
private CancellationTokenSource? _probeCts;
/// <summary>
/// Handle to the in-flight probe loop. Tracked (rather than fire-and-forget) so
/// <see cref="ShutdownAsync"/> can await it after cancelling — otherwise a probe
/// iteration still inside the <see cref="_gate"/> would race a disposed semaphore.
/// See code-review finding Driver.S7-006.
/// </summary>
private Task? _probeTask;
/// <summary>
/// Bounded grace window <see cref="ShutdownAsync"/> waits for the probe + poll loops to
/// observe cancellation and exit before it disposes the shared semaphore / CTS objects.
/// </summary>
private static readonly TimeSpan DrainTimeout = TimeSpan.FromSeconds(5);
public event EventHandler<DataChangeEventArgs>? OnDataChange;
public event EventHandler<HostStatusChangedEventArgs>? OnHostStatusChanged;
@@ -50,13 +65,20 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
private const uint StatusBadInternalError = 0x80020000u;
/// <summary>OPC UA StatusCode used for socket / timeout / protocol-layer faults.</summary>
private const uint StatusBadCommunicationError = 0x80050000u;
/// <summary>OPC UA StatusCode used when S7 returns <c>ErrorCode.WrongCPU</c> / PUT/GET disabled.</summary>
/// <summary>OPC UA StatusCode used for a genuine device fault (CPU error, hardware fault).</summary>
private const uint StatusBadDeviceFailure = 0x80550000u;
private readonly Dictionary<string, S7TagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, S7ParsedAddress> _parsedByName = new(StringComparer.OrdinalIgnoreCase);
private readonly S7DriverOptions _options = options;
/// <summary>
/// Active driver configuration. Seeded from the constructor argument, then replaced by
/// whatever <see cref="InitializeAsync"/> / <see cref="ReinitializeAsync"/> parse out of
/// the supplied <c>driverConfigJson</c> — see code-review finding Driver.S7-011. The
/// constructor value is the fallback used when the caller passes an empty / placeholder
/// JSON document (e.g. the <c>"{}"</c> some unit tests pass).
/// </summary>
private S7DriverOptions _options = options;
private readonly SemaphoreSlim _gate = new(1, 1);
/// <summary>
@@ -84,6 +106,18 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
_health = new DriverHealth(DriverState.Initializing, null, null);
try
{
// Re-parse the supplied DriverConfig JSON so a config change delivered through the
// IDriver contract is honoured (Driver.S7-011). An empty / placeholder document
// (e.g. the "{}" some unit tests pass) keeps the constructor-supplied options.
if (HasConfigBody(driverConfigJson))
_options = S7DriverFactoryExtensions.ParseOptions(driverInstanceId, driverConfigJson);
// Timer (T{n}) / Counter (C{n}) addresses parse cleanly but the read path has no
// S7DataType for them and no decode case — reject them here so a config typo
// fails fast at init instead of throwing a misleading type-mismatch on every
// read (Driver.S7-001). Drop this guard when Timer/Counter reads are wired through.
RejectUnsupportedTagAddresses();
var plc = new Plc(_options.CpuType, _options.Host, _options.Port, _options.Rack, _options.Slot);
// S7netplus writes timeouts into the underlying TcpClient via Plc.WriteTimeout /
// Plc.ReadTimeout (milliseconds). Set before OpenAsync so the handshake itself
@@ -117,7 +151,11 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
if (_options.Probe.Enabled)
{
_probeCts = new CancellationTokenSource();
_ = Task.Run(() => ProbeLoopAsync(_probeCts.Token), _probeCts.Token);
// Track the probe Task (not fire-and-forget) so ShutdownAsync can await it
// before disposing _gate / _probeCts (Driver.S7-006). Pass None to Task.Run so
// the delegate always runs and the handle is always awaitable; the loop's own
// token check handles cancellation.
_probeTask = Task.Run(() => ProbeLoopAsync(_probeCts.Token), CancellationToken.None);
}
}
catch (Exception ex)
@@ -133,27 +171,89 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
public async Task ReinitializeAsync(string driverConfigJson, CancellationToken cancellationToken)
{
// InitializeAsync re-parses driverConfigJson, so a config change delivered here is
// applied in place rather than silently discarded (Driver.S7-011).
await ShutdownAsync(cancellationToken).ConfigureAwait(false);
await InitializeAsync(driverConfigJson, cancellationToken).ConfigureAwait(false);
}
public Task ShutdownAsync(CancellationToken cancellationToken)
public async Task ShutdownAsync(CancellationToken cancellationToken)
{
try { _probeCts?.Cancel(); } catch { }
_probeCts?.Dispose();
_probeCts = null;
// Signal cancellation to the probe + poll loops first, collect their Task handles,
// then await all of them with a bounded timeout BEFORE disposing the shared semaphore
// and CTS objects. Without the drain, a loop iteration mid-_gate would call Release()
// on (or WaitAsync against) a disposed semaphore — see code-review finding Driver.S7-006.
var drain = new List<Task>();
foreach (var state in _subscriptions.Values)
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 { }
state.Cts.Dispose();
drain.Add(state.PollTask);
}
_subscriptions.Clear();
if (drain.Count > 0)
{
try
{
await Task.WhenAll(drain).WaitAsync(DrainTimeout, CancellationToken.None)
.ConfigureAwait(false);
}
catch (TimeoutException) { /* a wedged loop — proceed; better than leaking the teardown */ }
catch { /* loop faults are already surfaced via health; teardown continues */ }
}
// Loops have now observed cancellation and released _gate — safe to dispose the CTSs.
probeCts?.Dispose();
_probeCts = null;
_probeTask = null;
foreach (var state in subscriptions)
state.Cts.Dispose();
try { Plc?.Close(); } catch { /* best-effort — tearing down anyway */ }
Plc = null;
_health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null);
return Task.CompletedTask;
}
/// <summary>
/// True when <paramref name="driverConfigJson"/> carries a real config body. The
/// bootstrapper always passes a populated document; some unit tests pass <c>"{}"</c> or
/// an empty string to exercise lifecycle shape without a config — those keep the
/// constructor-supplied <see cref="_options"/>.
/// </summary>
private static bool HasConfigBody(string? driverConfigJson)
{
if (string.IsNullOrWhiteSpace(driverConfigJson)) return false;
var trimmed = driverConfigJson.Trim();
return trimmed is not "{}" and not "[]";
}
/// <summary>
/// Rejects tag addresses the read path cannot serve. Timer (<c>T{n}</c>) and Counter
/// (<c>C{n}</c>) addresses parse cleanly via <see cref="S7AddressParser"/> but
/// <see cref="ReadOneAsync"/> has no decode case for them and <see cref="S7DataType"/>
/// has no Timer/Counter member — left unguarded they fail fast init's promise and throw
/// a misleading type-mismatch on every read instead (code-review finding Driver.S7-001).
/// </summary>
private void RejectUnsupportedTagAddresses()
{
foreach (var t in _options.Tags)
{
if (S7AddressParser.TryParse(t.Address, out var parsed)
&& parsed.Area is S7Area.Timer or S7Area.Counter)
{
throw new NotSupportedException(
$"S7 tag '{t.Name}' uses a {parsed.Area} address ('{t.Address}'); " +
"Timer/Counter tags are not yet supported by the S7 driver. " +
"Remove the tag or use a DB/M/I/Q address until Timer/Counter reads are wired through.");
}
}
}
public DriverHealth GetHealth() => _health;
@@ -197,12 +297,22 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
{
results[i] = new DataValueSnapshot(null, StatusBadNotSupported, null, now);
}
catch (global::S7.Net.PlcException pex)
catch (PlcException pex) when (IsAccessDenied(pex))
{
// S7.Net's PlcException carries an ErrorCode; PUT/GET-disabled on
// S7-1200/1500 surfaces here. Map to BadDeviceFailure so operators see a
// device-config problem (toggle PUT/GET in TIA Portal) rather than a
// transient fault — per driver-specs.md §5.
// PUT/GET-disabled (S7-1200/1500) / access-protection — a permanent
// configuration fault, NOT a transient one. Blind retry is wasted effort,
// so map it to BadNotSupported and flag the driver as a config alert
// (Faulted) rather than Degraded — per driver-specs.md §5 and
// code-review finding Driver.S7-007.
results[i] = new DataValueSnapshot(null, StatusBadNotSupported, null, now);
_health = new DriverHealth(DriverState.Faulted, _health.LastSuccessfulRead,
"S7 access denied — enable PUT/GET communication in TIA Portal " +
$"(Protection & Security) for this CPU. PLC reported: {pex.Message}");
}
catch (PlcException pex)
{
// A genuine device-layer fault (CPU error, hardware fault) — transient
// enough to keep retrying; report BadDeviceFailure and degrade health.
results[i] = new DataValueSnapshot(null, StatusBadDeviceFailure, null, now);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, pex.Message);
}
@@ -283,7 +393,17 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
{
results[i] = new WriteResult(StatusBadNotSupported);
}
catch (global::S7.Net.PlcException)
catch (PlcException pex) when (IsAccessDenied(pex))
{
// PUT/GET-disabled / access-protection on write — same permanent
// configuration fault as on read (Driver.S7-007). BadNotSupported +
// a config-alert health state, not a transient device failure.
results[i] = new WriteResult(StatusBadNotSupported);
_health = new DriverHealth(DriverState.Faulted, _health.LastSuccessfulRead,
"S7 access denied — enable PUT/GET communication in TIA Portal " +
$"(Protection & Security) for this CPU. PLC reported: {pex.Message}");
}
catch (PlcException)
{
results[i] = new WriteResult(StatusBadDeviceFailure);
}
@@ -323,9 +443,31 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
await plc.WriteAsync(tag.Address, boxed, ct).ConfigureAwait(false);
}
private global::S7.Net.Plc RequirePlc() =>
private Plc RequirePlc() =>
Plc ?? throw new InvalidOperationException("S7Driver not initialized");
/// <summary>
/// Detects an S7 PUT/GET-disabled / access-protection fault inside an S7.Net
/// <see cref="PlcException"/>. S7.Net's read/write paths wrap every PLC-side error in a
/// <c>PlcException</c> with <see cref="ErrorCode.ReadData"/> / <see cref="ErrorCode.WriteData"/>;
/// the response-code validator throws a plain <see cref="Exception"/> for the S7
/// <c>AccessingObjectNotAllowed</c> status, which lands as the inner exception. There is
/// no typed error code for it, so the inner message is the only discriminator
/// S7.Net exposes — see code-review finding Driver.S7-007.
/// </summary>
private static bool IsAccessDenied(PlcException pex)
{
for (Exception? e = pex; e is not null; e = e.InnerException)
{
if (e.Message.Contains("Accessing object not allowed", StringComparison.OrdinalIgnoreCase)
|| e.Message.Contains("not allowed", StringComparison.OrdinalIgnoreCase))
{
return true;
}
}
return false;
}
// ---- ITagDiscovery ----
public Task DiscoverAsync(IAddressSpaceBuilder builder, CancellationToken cancellationToken)
@@ -375,7 +517,9 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
var handle = new S7SubscriptionHandle(id);
var state = new SubscriptionState(handle, [.. fullReferences], interval, cts);
_subscriptions[id] = state;
_ = Task.Run(() => PollLoopAsync(state, cts.Token), cts.Token);
// Track the poll Task so ShutdownAsync can await it after cancelling — a poll
// iteration mid-_gate would otherwise race the semaphore's disposal (Driver.S7-006).
state.PollTask = Task.Run(() => PollLoopAsync(state, cts.Token), CancellationToken.None);
return Task.FromResult<ISubscriptionHandle>(handle);
}
@@ -430,8 +574,14 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
TimeSpan Interval,
CancellationTokenSource Cts)
{
public System.Collections.Concurrent.ConcurrentDictionary<string, DataValueSnapshot> LastValues { get; }
public ConcurrentDictionary<string, DataValueSnapshot> LastValues { get; }
= new(StringComparer.OrdinalIgnoreCase);
/// <summary>
/// Handle to this subscription's poll loop. Tracked so <see cref="ShutdownAsync"/>
/// can await it after cancelling — see code-review finding Driver.S7-006.
/// </summary>
public Task PollTask { get; set; } = Task.CompletedTask;
}
private sealed record S7SubscriptionHandle(long Id) : ISubscriptionHandle