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

fix(driver-s7): resolve Medium code-review findings (Driver.S7-002, S7-004, S7-008)

Browse Source 
S7-002: add inline comment documenting the UInt32→Int32 lossiness in MapDataType,
consistent with the Int64/UInt64 note. Tracked for a follow-up that adds unsigned
DriverDataType members.

S7-004: inject ILogger<S7Driver> (optional, defaults to NullLogger); add structured
log calls for connect success/failure, probe Running/Stopped transitions, and
swallowed poll-loop exceptions, so operators have an event trail via Serilog.

S7-008: restructure WriteAsync catch ladder to mirror ReadAsync — OperationCanceledException
re-throws, NotSupportedException → BadNotSupported, PUT/GET-disabled PlcException →
BadNotSupported/Faulted, genuine PlcException → BadDeviceFailure/Degraded, all
others → BadCommunicationError/Degraded. Health is now updated on every write failure.
Also factor ReadOneAsync reinterpret into internal ReinterpretRawValue and
WriteOneAsync boxing into internal BoxValueForWrite for testability (Driver.S7-014).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 10:17:08 -04:00
parent b827b0c0a2
commit 909490622d
1 changed files with 77 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -1,4 +1,6 @@
using System.Collections.Concurrent;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using S7.Net;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
@@ -26,9 +28,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.S7;
/// 8-64 connection-resource budget.
/// </para>
/// </remarks>
public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, ILogger<S7Driver>? logger = null)
: IDriver, ITagDiscovery, IReadable, IWritable, ISubscribable, IHostConnectivityProbe, IDisposable, IAsyncDisposable
{
private readonly ILogger<S7Driver> _logger = logger ?? NullLogger<S7Driver>.Instance;
// ---- ISubscribable + IHostConnectivityProbe state ----
private readonly ConcurrentDictionary<long, SubscriptionState> _subscriptions = new();
@@ -144,6 +147,8 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
}
_health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null);
_logger.LogInformation("S7Driver connected. Driver={DriverInstanceId} Host={Host} CPU={CpuType} Tags={TagCount}",
driverInstanceId, _options.Host, _options.CpuType, _options.Tags.Count);
// Kick off the probe loop once the connection is up. Initial HostState stays
// Unknown until the first probe tick succeeds — avoids broadcasting a premature
@@ -165,6 +170,7 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
try { Plc?.Close(); } catch { }
Plc = null;
_health = new DriverHealth(DriverState.Faulted, null, ex.Message);
_logger.LogError(ex, "S7Driver connect failed. Driver={DriverInstanceId} Host={Host}", driverInstanceId, _options.Host);
throw;
}
}
@@ -338,7 +344,18 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
var raw = await plc.ReadAsync(tag.Address, ct).ConfigureAwait(false)
?? throw new System.IO.InvalidDataException($"S7.Net returned null for '{tag.Address}'");
return (tag.DataType, addr.Size, raw) switch
return ReinterpretRawValue(tag, addr, raw);
}
/// <summary>
/// Pure reinterpret step — converts the boxed value that S7.Net returns (always an
/// unsigned type: <c>bool</c>, <c>byte</c>, <c>ushort</c>, <c>uint</c>) into the
/// SEMANTIC type declared by the tag's <see cref="S7DataType"/>. No network I/O.
/// Factored out of <see cref="ReadOneAsync"/> so it can be exercised in unit tests
/// without a live PLC (Driver.S7-014).
/// </summary>
internal static object ReinterpretRawValue(S7TagDefinition tag, S7ParsedAddress addr, object raw) =>
(tag.DataType, addr.Size, raw) switch
{
(S7DataType.Bool, S7Size.Bit, bool b) => b,
(S7DataType.Byte, S7Size.Byte, byte by) => by,
@@ -358,7 +375,6 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
$"S7 Read type-mismatch: tag '{tag.Name}' declared {tag.DataType} but address '{tag.Address}' " +
$"parsed as Size={addr.Size}; S7.Net returned {raw.GetType().Name}"),
};
}
// ---- IWritable ----
@@ -389,6 +405,12 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
await WriteOneAsync(plc, tag, w.Value, cancellationToken).ConfigureAwait(false);
results[i] = new WriteResult(0u);
}
catch (OperationCanceledException)
{
// Driver.S7-008: let cancellation propagate rather than turning it into
// a status code — the gate is still held so Release() runs in finally.
throw;
}
catch (NotSupportedException)
{
results[i] = new WriteResult(StatusBadNotSupported);
@@ -403,13 +425,20 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
"S7 access denied — enable PUT/GET communication in TIA Portal " +
$"(Protection & Security) for this CPU. PLC reported: {pex.Message}");
}
catch (PlcException)
catch (PlcException pex)
{
// Genuine device-layer fault — degrade health so a PLC-down-during-writes
// scenario is visible to the operator (previously health was never updated
// on write failure — Driver.S7-008).
results[i] = new WriteResult(StatusBadDeviceFailure);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, pex.Message);
}
catch (Exception)
catch (Exception ex)
{
results[i] = new WriteResult(StatusBadInternalError);
// Socket/timeout/conversion failure. Map to BadCommunicationError (not
// BadInternalError) for transport faults; degrade health — Driver.S7-008.
results[i] = new WriteResult(StatusBadCommunicationError);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message);
}
}
}
@@ -423,26 +452,35 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
// match the address's size-suffix type: DBX=bool, DBB=byte, DBW=ushort, DBD=uint.
// Our S7DataType lets the caller pass short/int/float; convert to the unsigned
// wire representation before handing off.
var boxed = tag.DataType switch
{
S7DataType.Bool => (object)Convert.ToBoolean(value),
S7DataType.Byte => (object)Convert.ToByte(value),
S7DataType.UInt16 => (object)Convert.ToUInt16(value),
S7DataType.Int16 => (object)unchecked((ushort)Convert.ToInt16(value)),
S7DataType.UInt32 => (object)Convert.ToUInt32(value),
S7DataType.Int32 => (object)unchecked((uint)Convert.ToInt32(value)),
S7DataType.Float32 => (object)BitConverter.SingleToUInt32Bits(Convert.ToSingle(value)),
S7DataType.Int64 => throw new NotSupportedException("S7 Int64 writes land in a follow-up PR"),
S7DataType.UInt64 => throw new NotSupportedException("S7 UInt64 writes land in a follow-up PR"),
S7DataType.Float64 => throw new NotSupportedException("S7 Float64 (LReal) writes land in a follow-up PR"),
S7DataType.String => throw new NotSupportedException("S7 STRING writes land in a follow-up PR"),
S7DataType.DateTime => throw new NotSupportedException("S7 DateTime writes land in a follow-up PR"),
_ => throw new InvalidOperationException($"Unknown S7DataType {tag.DataType}"),
};
var boxed = BoxValueForWrite(tag.DataType, value);
await plc.WriteAsync(tag.Address, boxed, ct).ConfigureAwait(false);
}
/// <summary>
/// Pure boxing step — converts the caller's value into the unsigned wire type that
/// S7.Net's <c>Plc.WriteAsync</c> expects for each address size (bool → bool, byte
/// → byte, short → ushort, int → uint, float → uint-bits). No network I/O.
/// Factored out of <see cref="WriteOneAsync"/> so it can be exercised in unit tests
/// without a live PLC (Driver.S7-014).
/// </summary>
internal static object BoxValueForWrite(S7DataType dataType, object? value) => dataType switch
{
S7DataType.Bool => (object)Convert.ToBoolean(value),
S7DataType.Byte => (object)Convert.ToByte(value),
S7DataType.UInt16 => (object)Convert.ToUInt16(value),
S7DataType.Int16 => (object)unchecked((ushort)Convert.ToInt16(value)),
S7DataType.UInt32 => (object)Convert.ToUInt32(value),
S7DataType.Int32 => (object)unchecked((uint)Convert.ToInt32(value)),
S7DataType.Float32 => (object)BitConverter.SingleToUInt32Bits(Convert.ToSingle(value)),
S7DataType.Int64 => throw new NotSupportedException("S7 Int64 writes land in a follow-up PR"),
S7DataType.UInt64 => throw new NotSupportedException("S7 UInt64 writes land in a follow-up PR"),
S7DataType.Float64 => throw new NotSupportedException("S7 Float64 (LReal) writes land in a follow-up PR"),
S7DataType.String => throw new NotSupportedException("S7 STRING writes land in a follow-up PR"),
S7DataType.DateTime => throw new NotSupportedException("S7 DateTime writes land in a follow-up PR"),
_ => throw new InvalidOperationException($"Unknown S7DataType {dataType}"),
};
private Plc RequirePlc() =>
Plc ?? throw new InvalidOperationException("S7Driver not initialized");
@@ -493,8 +531,11 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
{
S7DataType.Bool => DriverDataType.Boolean,
S7DataType.Byte => DriverDataType.Int32, // no 8-bit in DriverDataType yet
// Driver.S7-002: UInt32 values > int.MaxValue (2^31-1) wrap negative when surfaced as
// Int32. This is lossy in the same way as the Int64/UInt64 mapping below — both are
// acknowledged limitations until unsigned DriverDataType members ship.
S7DataType.Int16 or S7DataType.UInt16 or S7DataType.Int32 or S7DataType.UInt32 => DriverDataType.Int32,
S7DataType.Int64 or S7DataType.UInt64 => DriverDataType.Int32, // widens; lossy for >2^31-1
S7DataType.Int64 or S7DataType.UInt64 => DriverDataType.Int32, // lossy for values > 2^31-1; tracked for follow-up
S7DataType.Float32 => DriverDataType.Float32,
S7DataType.Float64 => DriverDataType.Float64,
S7DataType.String => DriverDataType.String,
@@ -538,7 +579,11 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
// Initial-data push per OPC UA Part 4 convention.
try { await PollOnceAsync(state, forceRaise: true, ct).ConfigureAwait(false); }
catch (OperationCanceledException) { return; }
catch { /* first-read error — polling continues */ }
catch (Exception ex)
{
// First-read error — polling continues; log so the operator has an event trail.
_logger.LogWarning(ex, "S7 poll initial-read failed. Driver={DriverInstanceId}", driverInstanceId);
}
while (!ct.IsCancellationRequested)
{
@@ -547,7 +592,11 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
try { await PollOnceAsync(state, forceRaise: false, ct).ConfigureAwait(false); }
catch (OperationCanceledException) { return; }
catch { /* transient polling error — loop continues, health surface reflects it */ }
catch (Exception ex)
{
// Transient polling error — loop continues; log so the operator has an event trail.
_logger.LogWarning(ex, "S7 poll tick failed. Driver={DriverInstanceId}", driverInstanceId);
}
}
}
@@ -648,6 +697,8 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId)
_hostState = newState;
_hostStateChangedUtc = DateTime.UtcNow;
}
_logger.LogInformation("S7 probe transition. Driver={DriverInstanceId} Host={Host} {OldState} → {NewState}",
driverInstanceId, HostName, old, newState);
OnHostStatusChanged?.Invoke(this, new HostStatusChangedEventArgs(HostName, old, newState));
}