From 909490622dcfa79743f87dbe53863480adc750e4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 10:17:08 -0400 Subject: [PATCH] fix(driver-s7): resolve Medium code-review findings (Driver.S7-002, S7-004, S7-008) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (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) --- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 103 +++++++++++++----- 1 file changed, 77 insertions(+), 26 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index e8616ee..eaab8ad 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -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. /// /// -public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) +public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, ILogger? logger = null) : IDriver, ITagDiscovery, IReadable, IWritable, ISubscribable, IHostConnectivityProbe, IDisposable, IAsyncDisposable { + private readonly ILogger _logger = logger ?? NullLogger.Instance; // ---- ISubscribable + IHostConnectivityProbe state ---- private readonly ConcurrentDictionary _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); + } + + /// + /// Pure reinterpret step — converts the boxed value that S7.Net returns (always an + /// unsigned type: bool, byte, ushort, uint) into the + /// SEMANTIC type declared by the tag's . No network I/O. + /// Factored out of so it can be exercised in unit tests + /// without a live PLC (Driver.S7-014). + /// + 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); } + /// + /// Pure boxing step — converts the caller's value into the unsigned wire type that + /// S7.Net's Plc.WriteAsync expects for each address size (bool → bool, byte + /// → byte, short → ushort, int → uint, float → uint-bits). No network I/O. + /// Factored out of so it can be exercised in unit tests + /// without a live PLC (Driver.S7-014). + /// + 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)); }