From 87a7414df7b8446e38bb30662f80c1a0b4ff1609 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:04:19 -0400 Subject: [PATCH] fix(s7): refuse unservable tag shapes per tag at every gate (R07:H3, R07:U4) The init-time tag guards THREW: one mis-authored tag (DB1.DBZ0, DB1.DBW0 typed Float64, T5 typed Int32, a writable Bool array) faulted the whole driver and the actor retried the same init every 10 s forever, darkening every tag on the PLC. Neither the editor nor the deploy gate checked any of it. - S7AddressParser moves to S7.Contracts (same namespace) and a new S7TagShapeRules there is the single authority on servable shapes. - S7TagDefinitionFactory.FromTagConfig applies it, so the AdminUI editor, the deploy gate (Inspect, with the specific reason) and the driver init refuse the same tags. Init now skips a refused tag (ISkippedTagReporter) instead of throwing; the throwing RejectUnsupportedTagConfigs is gone. - New rules on top of the old guards: a narrow type at the wrong access width (every read failed) and a Bool array not starting at bit 0 (U4: DB1.DBX0.3 silently read bit 0 of bytes 0..n). - AdminUI test fixtures that used addresses the driver could never serve (DB1.DBS0, a browse "SomeAddress", Int32/Float32 on DBW) now use real ones. Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/S7.md | 17 +- .../S7AddressParser.cs | 0 .../S7TagDefinitionFactory.cs | 38 +++- .../S7TagShapeRules.cs | 149 ++++++++++++++ .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 192 ++---------------- .../Uns/TagEditors/S7TagConfigModel.cs | 5 +- .../S7_1500/S7_1500TagShapeSkipTests.cs | 57 ++++++ .../S7DriverCodeReviewFixTests.cs | 40 ++-- .../S7DriverCodeReviewFixTests2.cs | 20 +- .../S7DriverReadWriteTests.cs | 21 +- .../S7DriverScaffoldTests.cs | 114 ++--------- .../S7TagShapeGateTests.cs | 66 ++++++ .../S7TagSkipAssert.cs | 60 ++++++ .../S7ValueCodecTests.cs | 13 +- .../ScriptedS7Plc.cs | 142 +++++++++++++ .../Uns/CsvMergeEvidenceTests.cs | 2 +- .../Uns/RawTagConfigGateTests.cs | 6 +- .../Uns/S7TagConfigModelTests.cs | 6 +- .../Uns/TagConfigNonEnumParityTests.cs | 18 +- 19 files changed, 598 insertions(+), 368 deletions(-) rename src/Drivers/{ZB.MOM.WW.OtOpcUa.Driver.S7 => ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts}/S7AddressParser.cs (100%) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagShapeRules.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500TagShapeSkipTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagShapeGateTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagSkipAssert.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/ScriptedS7Plc.cs diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 02d27885..18e07821 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -103,9 +103,18 @@ Addresses use Siemens TIA-Portal / STEP 7 Classic syntax, parsed by | Timer | `T0` / `T15` | Timer area (read-only — see Wide types section) | | Counter | `C0` / `C10` | Counter area (read-only — see Wide types section) | -Parsing is strict and runs once at `InitializeAsync` so a config typo fails fast -at load instead of surfacing as `BadInternalError` on every read. Bit offsets -must be 0-7, byte offsets non-negative, DB numbers >= 1. +Parsing is strict. Bit offsets must be 0-7, byte offsets non-negative, DB numbers >= 1. + +**Tag shape rules are judged per tag, at every gate** (`S7TagShapeRules`, called by +`S7TagDefinitionFactory.FromTagConfig`): the AdminUI editor refuses the tag, the deploy gate +(`Inspect`) warns, and the driver's init **skips** it (reported through `ISkippedTagReporter`; it reads +`BadNodeIdUnknown`) while every other tag keeps working. Refused: an address that does not parse, a +wide/structured type that is an array or is not byte-addressed (`DBB`/`MB`/`IB`/`QB`), a narrow type at the +wrong access width (`Int32` on `DBW0`, `Bool` on `DBB0`...), a Timer not typed `Float64` or a Counter not typed +`Int32`, a writable or array Timer/Counter, a writable `Bool` array, and a `Bool` array that does not start +at bit 0 (a Bool array reads bit 0 of each byte, so `DB1.DBX0.3` used to silently return bit 0 of bytes +0..n). These used to be init guards that THREW, so one typo faulted every tag on the PLC and the actor +retried the same init every 10 s forever (R07:H3). ## Data types @@ -359,7 +368,7 @@ normal dev (the S7 sim is on the shared Docker host but currently offline). **Array writes (S7-016)** — a writable `Byte` / `Int16` / `UInt16` / `Int32` / `UInt32` / `Float32` array is written as ONE contiguous `WriteBytesAsync` of `count x element-bytes` (the inverse of the read decode), and the value must be an array of exactly `arrayLength` elements (else `BadTypeMismatch`, no -health impact). **Writable `Bool` arrays are refused at init** (`NotSupportedException`): the array +health impact). **Writable `Bool` arrays are refused per tag** (see the tag shape rules above): the array read is byte-granular, so a matching write would overwrite the 7 neighbouring bits of every byte it touches, and a safe bit-packed write needs read-modify-write. Mark Bool arrays `writable: false`. A `null` array element is rejected (`BadTypeMismatch`, nothing written) rather than coerced to 0. An array write larger than one PDU is split by S7.Net into several writes and is diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7AddressParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7AddressParser.cs similarity index 100% rename from src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7AddressParser.cs rename to src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7AddressParser.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs index a10c2c7d..94457fa5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs @@ -48,8 +48,23 @@ public static class S7TagDefinitionFactory /// The mapped definition when this returns . /// when is a valid S7 address object. public static bool FromTagConfig(string tagConfig, string rawPath, out S7TagDefinition def) + => FromTagConfig(tagConfig, rawPath, out def, out _); + + /// + /// , also reporting WHY a structurally + /// valid blob was refused by (an unparseable address, or a shape the + /// driver cannot serve). stays for a JSON-level + /// refusal (missing keys, bad enum), which explains on its own. + /// + /// The authored equipment-tag TagConfig JSON. + /// The tag's RawPath — becomes the definition's identity (Name). + /// The mapped definition when this returns . + /// The shape/address rejection reason, when that is why it refused. + /// when the driver can serve the tag. + public static bool FromTagConfig(string tagConfig, string rawPath, out S7TagDefinition def, out string? shapeRejection) { def = null!; + shapeRejection = null; if (string.IsNullOrWhiteSpace(tagConfig)) return false; try { @@ -78,7 +93,7 @@ public static class S7TagDefinitionFactory // "writable" defaults to true when absent (today's value); node-level authz still governs // writes. Honouring the key makes read-only equipment tags authorable. var writable = TagConfigJson.ReadWritable(root); - def = new S7TagDefinition( + var mapped = new S7TagDefinition( Name: rawPath, Address: address, DataType: dataType, @@ -86,6 +101,15 @@ public static class S7TagDefinitionFactory StringLength: stringLength == 0 ? MaxStringLength : stringLength, WriteIdempotent: false, ArrayCount: arrayCount); + // The address must parse and the shape must be one the driver can serve. These used to be + // init-time guards that THREW and faulted every tag on the PLC; refusing here skips only + // this tag, and the editor + deploy gate (which ask this method) refuse it too. + if (S7TagShapeRules.Validate(mapped, out _) is { } reason) + { + shapeRejection = reason; + return false; + } + def = mapped; return true; } catch (JsonException) { return false; } @@ -162,7 +186,9 @@ public static class S7TagDefinitionFactory /// reject — a structurally unparseable or non-object document, an absent / non-string / /// blank address, an absent dataType (see ), a /// present-but-invalid dataType (the runtime enum read is STRICT, so a - /// typo rejects the tag), or an out-of-range stringLength on a String tag. Such a tag + /// typo rejects the tag), an out-of-range stringLength on a String tag, or a shape + /// refuses (an unparseable address, a wrong access width, a mis-typed + /// Timer/Counter, an unsupported array). Such a tag /// is skipped by the driver and reads BadNodeIdUnknown forever. Empty when the mapper accepts /// the blob, when it is blank, or when it is not an equipment-tag object. Never throws. /// @@ -184,7 +210,13 @@ public static class S7TagDefinitionFactory // The accept gate: whatever the driver reads draws nothing. The RAW reference is handed to the // mapper — the driver reads the stored blob, not a trimmed copy of it. - if (FromTagConfig(reference, InspectionRawPath, out _)) return warnings; + if (FromTagConfig(reference, InspectionRawPath, out _, out var shapeRejection)) return warnings; + + if (shapeRejection is not null) + { + warnings.Add(shapeRejection + " The driver will skip this tag and it will read BadNodeIdUnknown."); + return warnings; + } try { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagShapeRules.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagShapeRules.cs new file mode 100644 index 00000000..17a9bf48 --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagShapeRules.cs @@ -0,0 +1,149 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.S7; + +/// +/// The single authority on which S7 tag shapes the driver can serve. Consulted by +/// , so the AdminUI editor, the deploy gate +/// () and the driver's own init all refuse the same +/// tags with the same words. +/// +/// +/// +/// These rules used to be init-time guards in S7Driver that THREW. One mis-authored tag +/// (DB1.DBZ0, DB1.DBW0 typed Float64, T5 typed Int32) faulted the +/// whole driver, and the actor retried that same init every 10 s forever, darkening every tag on +/// the PLC. The editor and the deploy gate checked none of it, and a CSV import or API blob +/// bypasses the editor entirely. A rejected tag is now skipped on its own (it reads +/// BadNodeIdUnknown) and the deploy gate warns about it. +/// +/// +/// Every rule rejects a tag that could never be served correctly. None of them can newly break +/// a tag that used to work: each one either faulted init, or failed (or silently mis-decoded) +/// every read of that tag. +/// +/// +public static class S7TagShapeRules +{ + /// + /// S7 data types addressed by START BYTE (the B suffix) rather than by an access-width + /// suffix. The driver decodes them from a contiguous byte buffer, so a wide tag authored against a + /// non-byte suffix (DBW/DBD/...) would mis-frame its value. + /// + private static readonly HashSet ByteAnchoredDataTypes = + [ + S7DataType.Int64, + S7DataType.UInt64, + S7DataType.Float64, + S7DataType.String, + S7DataType.DateTime, + S7DataType.WString, + S7DataType.Char, + S7DataType.WChar, + S7DataType.Dtl, + S7DataType.Date, + S7DataType.Tod, + S7DataType.Time, + S7DataType.S5Time, + ]; + + /// True when is a wide/structured type read from a byte-anchored block. + /// The declared data type. + /// for the byte-anchored types. + public static bool IsByteAnchored(S7DataType dataType) => ByteAnchoredDataTypes.Contains(dataType); + + /// + /// Judges one tag definition. Returns when the driver can serve it, or + /// the operator-facing reason it cannot. + /// + /// The mapped tag definition. + /// The parsed address when this returns . + /// The rejection reason, or when the tag is servable. + public static string? Validate(S7TagDefinition def, out S7ParsedAddress parsed) + { + ArgumentNullException.ThrowIfNull(def); + parsed = default; + + S7ParsedAddress addr; + try + { + addr = S7AddressParser.Parse(def.Address); + } + catch (FormatException ex) + { + return $"S7 address '{def.Address}' does not parse: {ex.Message}"; + } + + var isWide = IsByteAnchored(def.DataType); + var isArray = def.ArrayCount is >= 1; + + // Wide-type arrays have no decoder. + if (isWide && isArray) + return $"S7 {def.DataType} arrays are not supported. Use a scalar {def.DataType} tag, or an " + + "Int16/UInt16/Int32/UInt32/Float32/Byte/Bool element type for an array."; + + // Timer / Counter type constraints: a timer decodes to Float64 seconds, a counter to an Int32 count. + if (addr.Area is S7Area.Timer && def.DataType != S7DataType.Float64) + return $"S7 Timer address '{def.Address}' is typed {def.DataType}. Timer tags must be " + + "DataType=Float64 (decoded to seconds, read-only)."; + if (addr.Area is S7Area.Counter && def.DataType != S7DataType.Int32) + return $"S7 Counter address '{def.Address}' is typed {def.DataType}. Counter tags must be " + + "DataType=Int32 (decoded to a count, read-only)."; + if (addr.Area is S7Area.Timer or S7Area.Counter) + { + if (isArray) + return $"S7 Timer/Counter address '{def.Address}' cannot be an array."; + if (def.Writable) + return $"S7 Timer/Counter address '{def.Address}' is declared writable. Timer/Counter tags " + + "are read-only; set Writable=false."; + parsed = addr; + return null; + } + + // Wide/structured scalars on DB/M/I/Q must be byte-anchored (DBB/MB/IB/QB). + if (isWide) + { + if (addr.Size != S7Size.Byte) + return $"S7 {def.DataType} at '{def.Address}' is not byte-addressed. Wide/structured types " + + "must be byte-addressed (DBB/MB/IB/QB), e.g. 'DB1.DBB0', 'MB4'."; + parsed = addr; + return null; + } + + // Narrow types: the declared type must match the address's access width, or every read fails + // (or decodes garbage) with a type mismatch. + var required = def.DataType switch + { + S7DataType.Bool => S7Size.Bit, + S7DataType.Byte => S7Size.Byte, + S7DataType.Int16 or S7DataType.UInt16 => S7Size.Word, + _ => S7Size.DWord, // Int32 / UInt32 / Float32 + }; + if (addr.Size != required) + return $"S7 {def.DataType} at '{def.Address}' has the wrong access width ({addr.Size}). " + + $"A {def.DataType} tag needs a {SuffixHint(required)} address."; + + if (isArray && def.DataType == S7DataType.Bool) + { + // Element i of a Bool array is bit 0 of byte (start + i): the array is byte-granular. A + // non-zero bit offset used to be silently ignored (DB1.DBX0.3 returned bit 0 of bytes 0..n). + if (addr.BitOffset != 0) + return $"S7 Bool array at '{def.Address}' starts at bit {addr.BitOffset}. A Bool array reads " + + "one byte per element (bit 0 of each byte), so it must start at bit 0 (e.g. 'DB1.DBX0.0')."; + // A byte-granular write would overwrite the 7 neighbouring bits of every byte it touches. + if (def.Writable) + return $"S7 Bool array at '{def.Address}' is declared writable. Bool array writes are not " + + "supported (a byte-granular write would clobber neighbouring bits); set Writable=false " + + "or write the bits as individual scalar Bool tags."; + } + + parsed = addr; + return null; + } + + private static string SuffixHint(S7Size size) => size switch + { + S7Size.Bit => "bit (DBX/M/I/Q n.b)", + S7Size.Byte => "byte (DBB/MB/IB/QB)", + S7Size.Word => "word (DBW/MW/IW/QW)", + _ => "double-word (DBD/MD/ID/QD)", + }; +} 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 8659565a..347a0b19 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -239,41 +239,36 @@ public sealed class S7Driver if (HasConfigBody(driverConfigJson)) _options = S7DriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); - // v3: build the RawPath → definition table from the authored raw tags BEFORE the config - // guards run (they now iterate the mapped table). Each entry's TagConfig blob is mapped by - // the pure factory; the entry's WriteIdempotent flag is threaded onto the def (it lives on - // the RawTagEntry, not inside the blob). A mapper miss is skipped (logged), never thrown — - // one bad tag must not fail the whole driver init. + // v3: build the RawPath → definition table from the authored raw tags. Each entry's TagConfig + // blob is mapped by the pure factory, which also applies S7TagShapeRules (the address must + // parse; wide arrays, mis-typed Timer/Counter, wrong access widths, writable Timer/Counter or + // Bool arrays and offset Bool arrays are refused). A refused tag is SKIPPED (logged, reported + // through ISkippedTagReporter, reads BadNodeIdUnknown), never thrown: these used to be init + // guards that faulted every tag on the PLC and made the actor retry the same init forever. + // The entry's WriteIdempotent flag is threaded onto the def (it lives on the RawTagEntry). _tagsByRawPath.Clear(); _parsedByRawPath.Clear(); _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry _skippedTags.BeginRebuild(_options.RawTags.Count); foreach (var entry in _options.RawTags) { - if (S7TagDefinitionFactory.FromTagConfig(entry.TagConfig, entry.RawPath, out var def)) + if (S7TagDefinitionFactory.FromTagConfig(entry.TagConfig, entry.RawPath, out var def, out var shapeRejection) + && S7AddressParser.TryParse(def.Address, out var parsedAddress)) + { _tagsByRawPath[entry.RawPath] = def with { WriteIdempotent = entry.WriteIdempotent }; + _parsedByRawPath[entry.RawPath] = parsedAddress; + } else { - _skippedTags.Skip(entry.RawPath, "TagConfig did not map to a definition"); + var reason = shapeRejection ?? "TagConfig did not map to a definition"; + _skippedTags.Skip(entry.RawPath, reason); _logger.LogWarning( - "S7 tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath}", - _driverInstanceId, entry.RawPath); + "S7 tag skipped: {Reason} Driver={DriverInstanceId} RawPath={RawPath}", + reason, _driverInstanceId, entry.RawPath); } } _skippedTags.Commit(); - // Phase 4d combined config guard. Timer/Counter addresses and the wide/structured - // types (Int64/UInt64/Float64/String/DateTime) are now SUPPORTED, but only in - // specific shapes — this rejects the genuinely-unsupported configurations - // (wide-type arrays, non-byte-addressed wide scalars, wrong Timer/Counter DataType) - // so a config typo fails fast at init instead of as a misleading per-read fault. - RejectUnsupportedTagConfigs(); - - // Legacy data-type seam. The UnimplementedDataTypes set is now - // empty (Phase 4d unblocked all five wide types), so this rejects nothing — kept as - // the single grep target / future seam should a type ever need re-gating at init. - RejectUnsupportedTagDataTypes(); - // Build the connection through the factory (the same seam EnsureConnectedAsync's reopen // uses) so the read/write timeouts are baked in identically on connect and reconnect. var plc = _plcFactory.Create(_options); @@ -306,15 +301,6 @@ public sealed class S7Driver _connectBackoff.RecordSuccess(); Volatile.Write(ref _negotiatedPduSize, plc.MaxPduSize); - // Parse every tag's address once at init so config typos fail fast here instead - // of surfacing as BadInternalError on every Read against the bad tag. The parser - // also rejects bit-offset > 7, DB 0, unknown area letters, etc. Keyed by RawPath - // (== def.Name) so Read/Write can pick up the pre-parsed address. - foreach (var t in _tagsByRawPath.Values) - { - _parsedByRawPath[t.Name] = S7AddressParser.Parse(t.Address); // throws FormatException - } - _initialized = true; Health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); _logger.LogInformation("S7Driver connected. Driver={DriverInstanceId} Host={Host} CPU={CpuType} PduSize={PduSize} Tags={TagCount}", @@ -437,13 +423,6 @@ public sealed class S7Driver return trimmed is not "{}" and not "[]"; } - /// - /// S7 data types addressed by START BYTE (the B suffix) rather than by an - /// access-width suffix. Phase 4d decodes these from a contiguous byte buffer - /// (DB{n}.DBB{offset} / MB / IB / QB{offset}), so a wide tag - /// authored against a non-byte suffix (DBW/DBD/…) is a config error — see - /// . - /// /// /// PDU size the CPU negotiated on the most recent successful connect, in bytes; 0 before the first /// connect and after shutdown. Snapshotted at connect so it stays readable while the handle is @@ -474,143 +453,6 @@ public sealed class S7Driver : new Dictionary(); } - private static readonly HashSet ByteAnchoredDataTypes = new() - { - S7DataType.Int64, - S7DataType.UInt64, - S7DataType.Float64, - S7DataType.String, - S7DataType.DateTime, - S7DataType.WString, - S7DataType.Char, - S7DataType.WChar, - S7DataType.Dtl, - S7DataType.Date, - S7DataType.Tod, - S7DataType.Time, - S7DataType.S5Time, - }; - - /// - /// Combined init-time config guard (Phase 4d, supersedes the old Timer/Counter-only - /// RejectUnsupportedTagAddresses). Timer/Counter tags and the wide/structured - /// types (Int64/UInt64/Float64/String/DateTime) are now supported — but only in - /// specific shapes. This guard fails fast on the genuinely-unsupported configurations so - /// a config typo throws here at init rather than as a misleading per-read fault: - /// - /// (a) a wide/structured type declared as an array — wide-type arrays are out of scope this phase. - /// (b) a wide/structured type on a DB/M/I/Q area whose address isn't byte-anchored (DBB/MB/IB/QB). - /// (c) a Timer tag not typed Float64, or a Counter tag not typed Int32. - /// - /// Addresses that don't parse are left to the existing - /// pass in so the parse error isn't double-handled. - /// - private void RejectUnsupportedTagConfigs() - { - foreach (var t in _tagsByRawPath.Values) - { - var isWide = ByteAnchoredDataTypes.Contains(t.DataType); - - // (a) wide-type arrays — out of scope this phase. - if (isWide && t.ArrayCount is >= 1) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a {t.DataType} array, which the S7 driver does not yet " + - "support. Use a scalar wide tag, or an Int16/UInt16/Int32/UInt32/Float32/Byte/Bool " + - "element type for arrays."); - } - - // Parse the address; if it doesn't parse, leave it for the InitializeAsync - // S7AddressParser.Parse pass to throw the FormatException (no double-handling). - if (!S7AddressParser.TryParse(t.Address, out var parsed)) - continue; - - // (b) wide/structured scalar on DB/M/I/Q must be byte-anchored (Size == Byte). The - // codec decodes from a byte buffer at the start byte; a non-byte suffix (DBW/DBD/…) - // would mis-frame the value. - if (isWide - && parsed.Area is S7Area.DataBlock or S7Area.Memory or S7Area.Input or S7Area.Output - && parsed.Size != S7Size.Byte) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a {t.DataType} at '{t.Address}', which is not byte-addressed. " + - "Wide/structured types must be byte-addressed (DBB/MB/IB/QB) — e.g. 'DB1.DBB0', 'MB4'."); - } - - // (c) Timer/Counter type constraints — Timer decodes to a Float64 (seconds), Counter - // to an Int32 (count); both are read-only. - if (parsed.Area is S7Area.Timer && t.DataType != S7DataType.Float64) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a Timer address ('{t.Address}') but is typed {t.DataType}. " + - "Timer tags must be DataType=Float64 (decoded to seconds, read-only)."); - } - if (parsed.Area is S7Area.Counter && t.DataType != S7DataType.Int32) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a Counter address ('{t.Address}') but is typed {t.DataType}. " + - "Counter tags must be DataType=Int32 (decoded to count, read-only)."); - } - - // (d) Timer/Counter declared Writable — writes are read-only this phase; without this - // a node is discovered as Operate-writable but every write returns BadNotSupported. - if (parsed.Area is S7Area.Timer or S7Area.Counter && t.Writable) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a Timer/Counter ('{t.Address}') declared Writable — " + - "Timer/Counter are read-only this phase; set Writable=false."); - } - - // (e) Bool array declared Writable — element writes are byte-granular (one byte per - // element, mirroring the read), so a write would overwrite the 7 neighbouring bits of - // every byte it touches; a safe bit-packed write needs read-modify-write and is not - // implemented. Every other supported element type (Byte/Int16/UInt16/Int32/UInt32/ - // Float32) writes as ONE contiguous block (EncodeArrayBlock). Wide-type arrays - // are already rejected above (guard-a). - if (t.ArrayCount is >= 1 && t.Writable && t.DataType == S7DataType.Bool) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a Bool array (ArrayCount={t.ArrayCount}) declared Writable — " + - "Bool array writes are not supported (a byte-granular write would clobber the neighbouring " + - "bits). Set Writable=false, or write the bits as individual scalar Bool tags."); - } - } - } - - /// - /// Rejects tags configured with an still on the - /// list — types / - /// throw for, which - /// would otherwise create live OPC UA nodes via that return - /// BadNotSupported on every access. - /// The set is now empty (Phase 4d unblocked all five wide types), so this rejects - /// nothing — kept as the seam / grep target should a type ever need re-gating at init. - /// - private void RejectUnsupportedTagDataTypes() - { - foreach (var t in _tagsByRawPath.Values) - { - if (UnimplementedDataTypes.Contains(t.DataType)) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' uses data type '{t.DataType}' which is not yet " + - "supported by the S7 driver — Read/Write would return BadNotSupported. " + - "Remove the tag or use Bool/Byte/Int16/UInt16/Int32/UInt32/Float32 until " + - $"{t.DataType} is wired through."); - } - } - } - - /// - /// S7DataType members that the read/write helpers throw NotSupportedException for. - /// Now empty — Phase 4d (S7 wide types + Timer/Counter) wired Int64, UInt64, - /// Float64, String, and DateTime through the byte-buffer codec, and Timer/Counter read - /// through it on AREA. Kept here (rather than reflecting over - /// ) as the single grep target / future seam should a - /// data type ever need re-gating at init. - /// - private static readonly HashSet UnimplementedDataTypes = new(); - // ---- IReadable ---- // The batch is served in two passes under ONE gate acquisition. First S7ReadSpanPlanner groups @@ -1378,7 +1220,7 @@ public sealed class S7Driver /// true if the tag routes through the byte-buffer codec. internal static bool IsBufferType(S7TagDefinition tag, S7ParsedAddress addr) => addr.Area is S7Area.Timer or S7Area.Counter - || ByteAnchoredDataTypes.Contains(tag.DataType); + || S7TagShapeRules.IsByteAnchored(tag.DataType); /// /// Reads a wide/structured scalar as ONE contiguous byte block via S7.Net's diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs index 49b4e199..723cd369 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs @@ -112,8 +112,9 @@ public sealed class S7TagConfigModel + string.Join(", ", Enum.GetNames()) + "."; } - // Array shape is judged against the driver's INIT guards, which THROW (failing the whole - // driver's init, not just this tag) for a wide-type array and for a writable Bool array. The + // Array shape: the friendlier editor wording for two of the S7TagShapeRules refusals (a wide-type + // array, a writable Bool array). The reader below refuses them too (and the rest of the shape + // rules), so this is wording, not a second authority. The // isArray / arrayLength keys themselves belong to the TagModal's generic array controls // (TagArrayConfig) and are intentionally not fields of this model, so read them off the key bag. if (TagConfigJson.GetBool(_bag, "isArray")) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500TagShapeSkipTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500TagShapeSkipTests.cs new file mode 100644 index 00000000..17bc9a61 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500TagShapeSkipTests.cs @@ -0,0 +1,57 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests.S7_1500; + +/// +/// Live gate for R07:H3 — one mis-authored tag must not darken the PLC. Before the fix each of +/// these shapes threw at init, so the driver Faulted and served nothing (and the actor retried the +/// same init every 10 s forever). Now each is skipped on its own and the good tags keep reading +/// their seeded values over real S7comm. +/// +[Collection(Snap7ServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Device", "S7_1500")] +public sealed class S7_1500TagShapeSkipTests(Snap7ServerFixture sim) +{ + /// Unservable tags are skipped per tag; the seeded neighbours still read Good. + [Fact] + public async Task Mis_authored_tags_are_skipped_and_the_rest_of_the_PLC_still_reads() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + var ct = TestContext.Current.CancellationToken; + + var good = S7_1500Profile.BuildOptions(sim.Host, sim.Port); + var bad = new[] + { + new S7TagDefinition("Bad/Typo", "DB1.DBZ0", S7DataType.Int16), + new S7TagDefinition("Bad/WideOnWord", "DB1.DBW0", S7DataType.Float64), + new S7TagDefinition("Bad/TimerAsInt", "T5", S7DataType.Int32, Writable: false), + new S7TagDefinition("Bad/OffsetBoolArray", "DB1.DBX50.3", S7DataType.Bool, Writable: false, ArrayCount: 8), + }; + var options = new S7DriverOptions + { + Host = good.Host, Port = good.Port, CpuType = good.CpuType, Rack = good.Rack, Slot = good.Slot, + Timeout = good.Timeout, Probe = good.Probe, + RawTags = [.. good.RawTags, .. S7RawTags.Entries(bad)], + }; + + await using var drv = new S7Driver(options, driverInstanceId: "s7-shape-skip-live"); + await drv.InitializeAsync("{}", ct); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + drv.GetSkippedTags().ShouldNotBeNull().Sample.Select(s => s.RawPath) + .ShouldBe(bad.Select(b => b.Name), ignoreOrder: true); + + var r = await drv.ReadAsync( + [S7_1500Profile.ProbeTag, S7_1500Profile.SmokeI16Tag, S7_1500Profile.SmokeBoolTag, bad[0].Name], ct); + r[0].StatusCode.ShouldBe(0u); + Convert.ToInt32(r[0].Value).ShouldBe(S7_1500Profile.ProbeSeedValue); + r[1].StatusCode.ShouldBe(0u); + Convert.ToInt32(r[1].Value).ShouldBe((int)S7_1500Profile.SmokeI16SeedValue); + r[2].StatusCode.ShouldBe(0u); + Convert.ToBoolean(r[2].Value).ShouldBeTrue(); + r[3].StatusCode.ShouldBe(0x80340000u, "a skipped tag reads BadNodeIdUnknown"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs index 9c1c3a8a..6f0f12b8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs @@ -25,26 +25,10 @@ public sealed class S7DriverCodeReviewFixTests [InlineData("C10")] public async Task Initialize_rejects_timer_or_counter_tag_with_NotSupportedException(string address) { - // A Timer/Counter address parses cleanly but the read path has no decode case for it, - // so it must fail fast at init rather than throw a misleading type-mismatch on every - // read. The host is reserved-for-documentation so the TCP connect can never succeed — - // the unsupported-address guard runs before the connect, so the NotSupportedException - // is what surfaces. - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Quirk", address, S7DataType.Int16)), - }; - using var drv = new S7Driver(opts, "s7-tc"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain(address); - - var health = drv.GetHealth(); - health.State.ShouldBe(DriverState.Faulted, "an unsupported-address config error must Fault the driver"); - health.LastError.ShouldNotBeNull(); + // A Timer/Counter address typed anything but Float64 (Timer) / Int32 (Counter) cannot be decoded. + // It is refused PER TAG at every gate; it used to throw at init and fault every tag on the PLC. + await S7TagSkipAssert.RefusedPerTagAsync( + new S7TagDefinition("Quirk", address, S7DataType.Int16, Writable: false), address); } /// Verifies that initialize accepts DB and MIQ addresses without triggering the unsupported guard. @@ -102,19 +86,21 @@ public sealed class S7DriverCodeReviewFixTests [Fact] public async Task Initialize_rejects_a_timer_tag_supplied_only_through_driverConfigJson() { - // The Timer/Counter guard must run against the *re-parsed* config, not just the - // constructor options — proves Driver.S7-001 and Driver.S7-011 compose correctly. + // The shape gate must run against the *re-parsed* config, not just the constructor options — + // proves the per-tag skip and Driver.S7-011 compose correctly. var ctorOpts = new S7DriverOptions { Host = "192.0.2.1", Timeout = TimeSpan.FromMilliseconds(250) }; - using var drv = new S7Driver(ctorOpts, "s7-cfg-tc"); + using var drv = new S7Driver(ctorOpts, "s7-cfg-tc", new ScriptedS7PlcFactory()); const string json = """ - { "Host": "192.0.2.1", "TimeoutMs": 250, + { "Host": "192.0.2.1", "TimeoutMs": 250, "Probe": { "Enabled": false }, "RawTags": [ { "RawPath": "TimerTag", "TagConfig": "{\"address\":\"T5\",\"dataType\":\"Int16\"}", "WriteIdempotent": false } ] } """; - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync(json, TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("T5"); + await drv.InitializeAsync(json, TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + var skipped = drv.GetSkippedTags().ShouldNotBeNull().Sample.ShouldHaveSingleItem(); + skipped.RawPath.ShouldBe("TimerTag"); + skipped.Reason.ShouldContain("T5"); } /// Verifies that reinitialize applies a changed driverConfigJson. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs index 75731de5..59bb0922 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs @@ -158,23 +158,9 @@ public sealed class S7DriverCodeReviewFixTests2 [InlineData(S7DataType.DateTime)] public async Task Initialize_rejects_wide_type_at_non_byte_address_with_NotSupportedException(S7DataType dt) { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - // DB1.DBD0 is a DWord (non-byte) address — guard-(b) must fault on the address shape - // for a wide type. (At a byte address like DB1.DBB0 the 8-byte numerics round-trip; - // see S7ScalarBlockTests.) - RawTags = S7RawTags.Entries(new S7TagDefinition("X", "DB1.DBD0", dt)), - }; - using var drv = new S7Driver(opts, $"s7-wide-nonbyte-{dt}"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain(dt.ToString()); - - var health = drv.GetHealth(); - health.State.ShouldBe(DriverState.Faulted); + // DB1.DBD0 is a DWord (non-byte) address — a wide type there would mis-frame its value. (At a + // byte address like DB1.DBB0 the 8-byte numerics round-trip; see S7ScalarBlockTests.) + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("X", "DB1.DBD0", dt), dt.ToString(), "byte-addressed"); } /// Verifies that Initialize accepts implemented data types. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReadWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReadWriteTests.cs index 9f59d91c..d5bca500 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReadWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReadWriteTests.cs @@ -17,22 +17,11 @@ public sealed class S7DriverReadWriteTests [Fact] public async Task Initialize_rejects_invalid_tag_address_and_fails_fast() { - // Bad address at init time must throw; the alternative (deferring the parse to the - // first read) would surface the config bug as BadInternalError on every subsequent - // Read which is impossible for an operator to diagnose from the OPC UA client. - var opts = new S7DriverOptions - { - Host = "192.0.2.1", // reserved — will never complete TCP handshake - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("BadTag", "NOT-AN-S7-ADDRESS", S7DataType.Int16)), - }; - using var drv = new S7Driver(opts, "s7-bad-tag"); - - // Either the TCP connect fails first (Exception) or the parser fails (FormatException) - // — both are acceptable since both are init-time fail-fast. What matters is that we - // don't return a "healthy" driver with a latent bad tag. - await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + // A bad address is refused PER TAG at every gate (mapper/editor, deploy gate, driver init): + // it reads BadNodeIdUnknown while its neighbours keep working. It used to throw at init and + // fault every tag on the PLC, with the actor retrying the same init every 10 s forever. + await S7TagSkipAssert.RefusedPerTagAsync( + new S7TagDefinition("BadTag", "NOT-AN-S7-ADDRESS", S7DataType.Int16), "does not parse"); } /// Verifies that ReadAsync without initialize throws InvalidOperationException. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs index 6c1c6561..a8e4e52e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs @@ -101,80 +101,32 @@ public sealed class S7DriverScaffoldTests [Fact] public async Task Initialize_rejects_wide_type_array_with_clear_message() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Batch64", "DB1.DBB0", S7DataType.Int64, ArrayCount: 4)), - }; - using var drv = new S7Driver(opts, "s7-wide-array"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Batch64"); - ex.Message.ShouldContain("array", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Batch64", "DB1.DBB0", S7DataType.Int64, ArrayCount: 4), "Int64", "array"); } /// Verifies that a wide scalar with a non-byte address (Float64 at DB1.DBW0) is rejected with a byte-address message. [Fact] public async Task Initialize_rejects_wide_scalar_with_non_byte_address() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("WideWord", "DB1.DBW0", S7DataType.Float64)), - }; - using var drv = new S7Driver(opts, "s7-wide-nonbyte"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("WideWord"); - ex.Message.ShouldContain("byte", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("WideWord", "DB1.DBW0", S7DataType.Float64), "Float64", "byte-addressed"); } /// Verifies that a Timer tag with the wrong DataType (Int32, not Float64) is rejected. [Fact] public async Task Initialize_rejects_Timer_tag_with_wrong_data_type() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Timer5", "T5", S7DataType.Int32)), - }; - using var drv = new S7Driver(opts, "s7-timer-bad"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Timer5"); - ex.Message.ShouldContain("Float64"); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Timer5", "T5", S7DataType.Int32, Writable: false), "T5", "Float64"); } /// Verifies that a Counter tag with the wrong DataType (Float32, not Int32) is rejected. [Fact] public async Task Initialize_rejects_Counter_tag_with_wrong_data_type() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Counter3", "C3", S7DataType.Float32)), - }; - using var drv = new S7Driver(opts, "s7-counter-bad"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Counter3"); - ex.Message.ShouldContain("Int32"); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Counter3", "C3", S7DataType.Float32, Writable: false), "C3", "Int32"); } // ── Phase 4d bundle-review fix — Writable Timer/Counter guard ─────────────────────── @@ -188,42 +140,16 @@ public sealed class S7DriverScaffoldTests [Fact] public async Task Initialize_rejects_Timer_tag_declared_Writable() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Timer5", "T5", S7DataType.Float64, Writable: true)), - }; - using var drv = new S7Driver(opts, "s7-timer-writable"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Timer5"); - ex.Message.ShouldContain("Writable", Case.Insensitive); - ex.Message.ShouldContain("read-only", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Timer5", "T5", S7DataType.Float64, Writable: true), "T5", "writable", "read-only"); } /// Verifies that a Counter tag declared Writable=true is rejected at init with a Writable message. [Fact] public async Task Initialize_rejects_Counter_tag_declared_Writable() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Counter3", "C3", S7DataType.Int32, Writable: true)), - }; - using var drv = new S7Driver(opts, "s7-counter-writable"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Counter3"); - ex.Message.ShouldContain("Writable", Case.Insensitive); - ex.Message.ShouldContain("read-only", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Counter3", "C3", S7DataType.Int32, Writable: true), "C3", "writable", "read-only"); } // ── Driver.S7-015 / S7-016 — writable Bool array must fail fast at init ───────────── @@ -236,20 +162,8 @@ public sealed class S7DriverScaffoldTests [Fact] public async Task Initialize_rejects_writable_bool_array_tag_with_NotSupportedException() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("ArrTag", "DB1.DBX0.0", S7DataType.Bool, Writable: true, ArrayCount: 4)), - }; - using var drv = new S7Driver(opts, "s7-writable-bool-arr"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("ArrTag"); - ex.Message.ShouldContain("Bool array", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("ArrTag", "DB1.DBX0.0", S7DataType.Bool, Writable: true, ArrayCount: 4), "Bool array", "writable"); } /// Verifies that writable non-bool arrays pass the init guard — the failure diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagShapeGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagShapeGateTests.cs new file mode 100644 index 00000000..3bc47dca --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagShapeGateTests.cs @@ -0,0 +1,66 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// R07:H3 / R07:U4 — the tag shapes the driver cannot serve are refused per tag at the mapper, +/// the deploy gate and the driver's init (see ), instead of faulting +/// the whole driver at init or being silently mis-read. +/// +[Trait("Category", "Unit")] +public sealed class S7TagShapeGateTests +{ + /// A typo'd area/size letter is refused per tag (it used to throw FormatException at init). + [Fact] + public Task Unparseable_address_is_refused_per_tag() + => S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Typo", "DB1.DBZ0", S7DataType.Int16), "DB1.DBZ0", "does not parse"); + + /// + /// R07:U4 — a Bool array reads bit 0 of each byte, so a non-zero start bit was silently ignored + /// (DB1.DBX0.3 returned bit 0 of bytes 0..7). It is refused at authoring now. + /// + [Fact] + public Task Bool_array_starting_at_a_nonzero_bit_is_refused() + => S7TagSkipAssert.RefusedPerTagAsync( + new S7TagDefinition("OffsetBits", "DB1.DBX0.3", S7DataType.Bool, Writable: false, ArrayCount: 8), + "Bool array", "bit 3"); + + /// A narrow type at the wrong access width fails every read with a type mismatch; refused up front. + /// The address. + /// The declared type, which does not match the address's access width. + [Theory] + [InlineData("DB1.DBW0", S7DataType.Int32)] + [InlineData("DB1.DBD0", S7DataType.Int16)] + [InlineData("DB1.DBB0", S7DataType.Bool)] + [InlineData("M0.0", S7DataType.Byte)] + [InlineData("MW0", S7DataType.Float32)] + public Task Narrow_type_at_the_wrong_access_width_is_refused(string address, S7DataType dt) + => S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Mismatch", address, dt), "access width"); + + /// Positive control: the shapes the driver serves are all still accepted by the mapper and the gate. + /// The address. + /// The declared type. + /// The array count, or 0 for a scalar. + /// Whether the tag is writable. + [Theory] + [InlineData("DB1.DBX0.0", S7DataType.Bool, 8, false)] + [InlineData("DB1.DBX50.3", S7DataType.Bool, 0, true)] + [InlineData("DB1.DBB0", S7DataType.Byte, 4, true)] + [InlineData("DB1.DBW0", S7DataType.Int16, 0, true)] + [InlineData("MW2", S7DataType.UInt16, 3, true)] + [InlineData("DB1.DBD0", S7DataType.Float32, 0, true)] + [InlineData("QD4", S7DataType.Int32, 2, true)] + [InlineData("DB1.DBB0", S7DataType.Float64, 0, true)] + [InlineData("DB1.DBB0", S7DataType.String, 0, true)] + [InlineData("T5", S7DataType.Float64, 0, false)] + [InlineData("C3", S7DataType.Int32, 0, false)] + public void Servable_shapes_are_accepted(string address, S7DataType dt, int arrayCount, bool writable) + { + var def = new S7TagDefinition("Ok", address, dt, Writable: writable, ArrayCount: arrayCount == 0 ? null : arrayCount); + var blob = S7TagDefinitionFactory.ToTagConfig(def); + S7TagDefinitionFactory.FromTagConfig(blob, "Ok", out var mapped, out var reason).ShouldBeTrue(reason); + mapped.Address.ShouldBe(address); + S7TagDefinitionFactory.Inspect(blob).ShouldBeEmpty(); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagSkipAssert.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagSkipAssert.cs new file mode 100644 index 00000000..47f96de4 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagSkipAssert.cs @@ -0,0 +1,60 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// Shared assertion for an S7 tag shape the driver cannot serve. It must be refused PER TAG at +/// every gate, never fault the whole driver: the mapper (and so the AdminUI editor) refuses it with +/// the reason, the deploy gate () warns, and a driver +/// carrying it next to a good tag initializes Healthy, reports the bad one through +/// , and still serves the good one (the positive control). +/// +internal static class S7TagSkipAssert +{ + private const string GoodName = "Good"; + + /// Asserts is refused per tag at the mapper, the deploy gate and the driver. + /// The unservable tag. + /// Fragments the operator-facing rejection reason must contain. + /// A task that completes when every gate has been checked. + public static async Task RefusedPerTagAsync(S7TagDefinition bad, params string[] reasonFragments) + { + var ct = TestContext.Current.CancellationToken; + var blob = S7TagDefinitionFactory.ToTagConfig(bad); + + // Mapper (and therefore the AdminUI editor, which asks it) — refused, with the reason. + S7TagDefinitionFactory.FromTagConfig(blob, bad.Name, out _, out var reason).ShouldBeFalse(); + reason.ShouldNotBeNull(); + foreach (var f in reasonFragments) reason.ShouldContain(f, Case.Insensitive); + + // Deploy gate — warns with the same reason. + var warnings = S7TagDefinitionFactory.Inspect(blob); + warnings.ShouldHaveSingleItem().ShouldContain(reason); + + // Driver — initializes Healthy with the bad tag skipped and the good tag served. + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries(bad, new S7TagDefinition(GoodName, "DB1.DBW0", S7DataType.UInt16, Writable: false)), + }; + using var drv = new S7Driver(opts, "s7-skip-" + bad.Name, new ScriptedS7PlcFactory()); + await drv.InitializeAsync("{}", ct); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "one unservable tag must not fault the driver"); + var skipped = drv.GetSkippedTags().ShouldNotBeNull(); + skipped.Sample.ShouldHaveSingleItem().RawPath.ShouldBe(bad.Name); + skipped.Sample[0].Reason.ShouldBe(reason); + + var builder = new RecordingAddressSpaceBuilder(); + await drv.DiscoverAsync(builder, ct); + builder.Variables.Select(v => v.Name).ShouldBe([GoodName]); + + var results = await drv.ReadAsync([bad.Name, GoodName], ct); + results[0].StatusCode.ShouldBe(0x80340000u, "the skipped tag reads BadNodeIdUnknown"); + results[1].StatusCode.ShouldBe(0u, "the good neighbour is still served"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs index 506d5d67..95bba0b3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs @@ -318,16 +318,9 @@ public sealed class S7ValueCodecTests [Fact] public async Task New_types_declared_as_arrays_or_off_byte_addresses_fail_init() { - S7DriverOptions With(S7TagDefinition t) => new() - { - Host = "192.0.2.1", - Probe = new S7ProbeOptions { Enabled = false }, - RawTags = S7RawTags.Entries(t), - }; - using var arr = new S7Driver(With(Tag(S7DataType.Dtl) with { ArrayCount = 2 }), "a", new ConnectingFakeS7PlcFactory()); - await Should.ThrowAsync(() => arr.InitializeAsync("{}", TestContext.Current.CancellationToken)); - using var word = new S7Driver(With(Tag(S7DataType.Tod, address: "DB1.DBW0")), "b", new ConnectingFakeS7PlcFactory()); - await Should.ThrowAsync(() => word.InitializeAsync("{}", TestContext.Current.CancellationToken)); + // Refused PER TAG at every gate (they used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(Tag(S7DataType.Dtl) with { Name = "DtlArr", ArrayCount = 2 }, "Dtl", "array"); + await S7TagSkipAssert.RefusedPerTagAsync(Tag(S7DataType.Tod, address: "DB1.DBW0") with { Name = "TodWord" }, "Tod", "byte-addressed"); } // ---- TagConfig ---- diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/ScriptedS7Plc.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/ScriptedS7Plc.cs new file mode 100644 index 00000000..25d74961 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/ScriptedS7Plc.cs @@ -0,0 +1,142 @@ +using S7DataItem = global::S7.Net.Types.DataItem; +using S7NetDataType = global::S7.Net.DataType; +using VarType = global::S7.Net.VarType; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// A scriptable : every connection it creates delegates its wire +/// operations to the factory's handlers, so a test scripts the device once and observes every +/// connection (including reopened ones). Counts wire operations so a test can prove what did +/// NOT go to the wire without timing anything. +/// +internal sealed class ScriptedS7PlcFactory : IS7PlcFactory +{ + private int _wireOps; + + /// Every connection created, in order. + public List Created { get; } = []; + + /// Wire operations issued (reads, multi-var reads, writes; not opens or status probes). + public int WireOps => Volatile.Read(ref _wireOps); + + /// Hook run inside every ; the default opens immediately. + public Func Open { get; set; } = (_, _) => Task.CompletedTask; + + /// Per-address string read; the default returns (ushort)42. + public Func> Read { get; set; } = (_, _) => Task.FromResult((ushort)42); + + /// Block read; the default returns count zero bytes. + public Func> ReadBytes { get; set; } + = (_, _, _, count, _) => Task.FromResult(new byte[count]); + + /// Multi-var read; the default answers every item with a zero of its var type. + public Func, CancellationToken, Task>> ReadMulti { get; set; } + = (items, _) => + { + foreach (var i in items) + i.Value = i.VarType switch + { + VarType.Bit => false, + VarType.Byte => (byte)0, + VarType.Word => (ushort)0, + _ => (uint)0, + }; + return Task.FromResult>(items); + }; + + /// Per-address string write; the default succeeds. + public Func Write { get; set; } = (_, _, _) => Task.CompletedTask; + + /// Block write; the default succeeds. + public Func WriteBytes { get; set; } + = (_, _, _, _, _) => Task.CompletedTask; + + /// CPU-status probe; the default answers. + public Func ReadStatus { get; set; } = _ => Task.CompletedTask; + + /// + public IS7Plc Create(S7CpuType cpuType, string host, int port, short rack, short slot, TimeSpan timeout) + { + lock (Created) + { + var plc = new ScriptedS7Plc(this, Created.Count); + Created.Add(plc); + return plc; + } + } + + /// Counts one wire operation. + internal void CountWireOp() => Interlocked.Increment(ref _wireOps); +} + +/// One connection created by . +internal sealed class ScriptedS7Plc(ScriptedS7PlcFactory factory, int index) : IS7Plc +{ + /// Zero-based creation index. + public int Index { get; } = index; + + /// + public bool IsConnected { get; private set; } + + /// True once disposed. + public bool Disposed { get; private set; } + + /// + public int MaxPduSize => 240; + + /// + public async Task OpenAsync(CancellationToken cancellationToken) + { + await factory.Open(Index, cancellationToken).ConfigureAwait(false); + IsConnected = true; + } + + /// + public Task ReadAsync(string address, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.Read(address, cancellationToken); + } + + /// + public Task ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.ReadBytes(area, db, startByteAdr, count, cancellationToken); + } + + /// + public Task> ReadMultipleVarsAsync(List items, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.ReadMulti(items, cancellationToken); + } + + /// + public Task WriteAsync(string address, object value, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.Write(address, value, cancellationToken); + } + + /// + public Task WriteBytesAsync(S7NetDataType area, int db, int startByteAdr, byte[] value, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.WriteBytes(area, db, startByteAdr, value, cancellationToken); + } + + /// + public Task ReadStatusAsync(CancellationToken cancellationToken) => factory.ReadStatus(cancellationToken); + + /// + public void Close() => IsConnected = false; + + /// + public void Dispose() + { + Disposed = true; + IsConnected = false; + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CsvMergeEvidenceTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CsvMergeEvidenceTests.cs index 337c6a34..112f1287 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CsvMergeEvidenceTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CsvMergeEvidenceTests.cs @@ -142,7 +142,7 @@ public sealed class CsvMergeEvidenceTests var row = SingleRow( DriverTypeNames.S7, "Name,DataType,Address,S7DataType,TagConfigJson", - "t1,Int16,DB1.DBW0,Int32,\"{\"\"pollHint\"\":\"\"fast\"\"}\""); + "t1,Int16,DB1.DBD0,Int32,\"{\"\"pollHint\"\":\"\"fast\"\"}\""); var blob = Blob(row); blob["dataType"]!.GetValue().ShouldBe("Int32"); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs index 65930cf0..17703c6b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs @@ -346,7 +346,11 @@ public sealed class RawTagConfigGateTests public void A_browse_committed_config_passes_the_gate(string driverType) { var tagConfig = RawBrowseCommitMapper.BuildTagConfig( - driverType, driverType == DriverTypeNames.AbLegacy ? "N7:0" : "SomeAddress", BrowseFieldsFor(driverType)); + driverType, + // Drivers whose address grammar is validated by their reader need a real address: AbLegacy's + // file grammar, and S7's (S7TagShapeRules — an unparseable or wrong-width address is refused). + driverType == DriverTypeNames.AbLegacy ? "N7:0" : driverType == DriverTypeNames.S7 ? "DB1.DBW0" : "SomeAddress", + BrowseFieldsFor(driverType)); TagConfigValidator.Validate(driverType, tagConfig).ShouldBeNull( $"browse-commit for {driverType} must not author a config the import refuses"); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs index dbd8e61b..12cb121c 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs @@ -98,13 +98,13 @@ public sealed class S7TagConfigModelTests /// driver serves. (An absent key is deliberately not asserted: S7 alone hard-requires the key, an /// asymmetry that predates this change and that its own Inspect does not report either.) [Theory] - [InlineData("""{"address":"DB1.DBW0","dataType":"float32"}""")] - [InlineData("""{"address":"DB1.DBW0","dataType":"FLOAT32"}""")] + [InlineData("""{"address":"DB1.DBD0","dataType":"float32"}""")] + [InlineData("""{"address":"DB1.DBD0","dataType":"FLOAT32"}""")] [InlineData("""{"address":"DB1.DBW0","dataType":7}""")] public void Validate_accepts_every_dataType_the_driver_accepts(string json) => S7TagConfigModel.FromJson(json).Validate().ShouldBeNull(); - // ---- S7-016: array shape vs the driver's init guards (which fail the WHOLE driver init) ---- + // ---- S7-016: array shape vs S7TagShapeRules (refused per tag at every gate) ---- [Theory] [InlineData("Int64")] diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs index 019498e4..d71a00e0 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs @@ -160,9 +160,9 @@ public sealed class TagConfigNonEnumParityTests /// S7's remaining non-enum refusal: a String tag whose stringLength falls outside /// 0–254. The reader REJECTS it (it does not clamp), so the tag would never resolve. [Theory] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":255}""")] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":300}""")] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":-1}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":255}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":300}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":-1}""")] public void S7_refuses_a_string_length_its_reader_refuses(string blob) { RuntimeAccepts(DriverTypeNames.S7, blob).ShouldBeFalse( @@ -174,9 +174,9 @@ public sealed class TagConfigNonEnumParityTests /// stops this becoming an over-rejection — an out-of-range stringLength on a tag that is not a /// String at all, which the reader ignores entirely. [Theory] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":10}""")] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":254}""")] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":0}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":10}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":254}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":0}""")] [InlineData("""{"address":"DB1.DBW0","dataType":"Int16","stringLength":300}""")] public void S7_accepts_a_string_length_its_reader_accepts(string blob) { @@ -360,8 +360,8 @@ public sealed class TagConfigNonEnumParityTests (DriverTypeNames.S7, """{"address":"DB1.DBW0","dataType":"Int16"}"""), (DriverTypeNames.S7, """{"address":"DB1.DBW0"}"""), (DriverTypeNames.S7, """{"address":" ","dataType":"Int16"}"""), - (DriverTypeNames.S7, """{"address":"DB1.DBS0","dataType":"String","stringLength":300}"""), - (DriverTypeNames.S7, """{"address":"DB1.DBS0","dataType":"String","stringLength":24}"""), + (DriverTypeNames.S7, """{"address":"DB1.DBB0","dataType":"String","stringLength":300}"""), + (DriverTypeNames.S7, """{"address":"DB1.DBB0","dataType":"String","stringLength":24}"""), (DriverTypeNames.AbCip, "{}"), (DriverTypeNames.AbCip, """{"tagPath":"Program:Main.Speed"}"""), @@ -576,7 +576,7 @@ public sealed class TagConfigNonEnumParityTests data.Add(DriverTypeNames.Modbus, new ModbusTagConfigModel { Address = 1 }.ToJson()); data.Add(DriverTypeNames.S7, new S7TagConfigModel { Address = "DB1.DBW0" }.ToJson()); data.Add(DriverTypeNames.S7, - new S7TagConfigModel { Address = "DB1.DBS0", DataType = S7DataType.String, StringLength = 24 }.ToJson()); + new S7TagConfigModel { Address = "DB1.DBB0", DataType = S7DataType.String, StringLength = 24 }.ToJson()); data.Add(DriverTypeNames.AbCip, new AbCipTagConfigModel { TagPath = "Program:Main.Speed" }.ToJson()); data.Add(DriverTypeNames.AbLegacy, new AbLegacyTagConfigModel { Address = "N7:0" }.ToJson()); data.Add(DriverTypeNames.TwinCAT, new TwinCATTagConfigModel { SymbolPath = "MAIN.bStart" }.ToJson());