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 367bc665..a7056144 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -145,19 +145,16 @@ public sealed class S7Driver 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(); + // 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(); - // S7DataType values that ReadOneAsync / WriteOneAsync currently throw - // NotSupportedException for (Int64, UInt64, Float64, String, DateTime) must also - // be rejected at init — without this guard a site can configure e.g. a Float64 - // tag, see the node appear in the address space via DiscoverAsync, and get - // BadNotSupported on every access. Half-implemented types must not leak into the - // configurable surface (Driver.S7-013). Drop entries from the set as each data - // type is wired through. + // Legacy data-type seam (Driver.S7-013). 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(); var plc = new Plc(S7CpuTypeMap.ToS7Net(_options.CpuType), _options.Host, _options.Port, _options.Rack, _options.Slot); @@ -297,34 +294,92 @@ public sealed class S7Driver } /// - /// Rejects tag addresses the read path cannot serve. Timer (T{n}) and Counter - /// (C{n}) addresses parse cleanly via but - /// has no decode case for them and - /// 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). + /// 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 + /// . /// - private void RejectUnsupportedTagAddresses() + private static readonly HashSet ByteAnchoredDataTypes = new() + { + S7DataType.Int64, + S7DataType.UInt64, + S7DataType.Float64, + S7DataType.String, + S7DataType.DateTime, + }; + + /// + /// 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 _options.Tags) { - if (S7AddressParser.TryParse(t.Address, out var parsed) - && parsed.Area is S7Area.Timer or S7Area.Counter) + 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}' 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."); + $"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)."); } } } /// - /// Rejects tags configured with an that - /// / still throw - /// for. Without this guard those tags create live - /// OPC UA nodes via but every Read/Write returns - /// BadNotSupported — code-review finding Driver.S7-013. Drop entries from - /// as each type is wired through. + /// 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 (code-review finding Driver.S7-013). + /// 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() { @@ -343,18 +398,12 @@ public sealed class S7Driver /// /// S7DataType members that the read/write helpers throw NotSupportedException for. - /// Kept here (rather than reflecting over ) so - /// is a single grep target for the - /// follow-up PR that wires each through. + /// Now empty — Phase 4d (S7 wide types + Timer/Counter) unblocks Int64, UInt64, + /// Float64, String, and DateTime at init; the codec wires them through in follow-up + /// tasks. 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() - { - S7DataType.Int64, - S7DataType.UInt64, - S7DataType.Float64, - S7DataType.String, - S7DataType.DateTime, - }; + private static readonly HashSet UnimplementedDataTypes = new(); /// /// Approximate memory footprint. The Plc instance + one 240-960 byte PDU buffer is @@ -816,10 +865,11 @@ public sealed class S7Driver 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. + // Int32. That residual lossy note now applies ONLY to UInt32 — Int64/UInt64 map to + // their own DriverDataType members below (Phase 4d), so they are no longer lossy. S7DataType.Int16 or S7DataType.UInt16 or S7DataType.Int32 or S7DataType.UInt32 => DriverDataType.Int32, - S7DataType.Int64 or S7DataType.UInt64 => DriverDataType.Int32, // lossy for values > 2^31-1; tracked for follow-up + S7DataType.Int64 => DriverDataType.Int64, + S7DataType.UInt64 => DriverDataType.UInt64, S7DataType.Float32 => DriverDataType.Float32, S7DataType.Float64 => DriverDataType.Float64, S7DataType.String => DriverDataType.String, 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 b0e4b431..3f96608c 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 @@ -68,4 +68,112 @@ public sealed class S7DriverScaffoldTests health.State.ShouldBe(DriverState.Faulted, "unreachable host must flip the driver to Faulted so operators see it"); health.LastError.ShouldNotBeNull(); } + + // ── Phase 4d T1 — wide-type / Timer-Counter init guards ────────────────────────────── + // + // The init flow runs RejectUnsupportedTagConfigs BEFORE plc.OpenAsync, so a guard + // rejection throws before any TCP connect. The reserved-for-documentation host (192.0.2.1) + // means a config that PASSES the guard still throws on connect — so a positive case + // asserts the failure is NOT the guard's NotSupportedException/FormatException (the same + // shape used by S7DriverCodeReviewFixTests2.Initialize_accepts_implemented_data_types). + + /// Verifies that a valid byte-addressed wide scalar (Float64 at DB1.DBB0) passes the init guard. + [Fact] + public async Task Initialize_accepts_byte_addressed_wide_scalar_Float64() + { + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Tags = [new S7TagDefinition("LReal", "DB1.DBB0", S7DataType.Float64)], + }; + using var drv = new S7Driver(opts, "s7-wide-ok"); + + var ex = await Should.ThrowAsync(async () => + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + ex.ShouldNotBeOfType( + "a byte-addressed wide scalar must pass the init guard — the failure must be the connect"); + ex.ShouldNotBeOfType( + "DB1.DBB0 parses cleanly — the failure must be the connect, not an address-parse error"); + } + + /// Verifies that a wide-type array (Int64 + ArrayCount) is rejected as out-of-scope this phase. + [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), + Tags = [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); + } + + /// 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), + Tags = [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); + } + + /// 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), + Tags = [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); + } + + /// 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), + Tags = [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); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TypeMappingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TypeMappingTests.cs index c61f0c09..81f45ba0 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TypeMappingTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TypeMappingTests.cs @@ -1,5 +1,6 @@ using Shouldly; using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; @@ -190,4 +191,86 @@ public sealed class S7TypeMappingTests { Should.Throw(() => S7Driver.BoxValueForWrite(S7DataType.UInt16, 65_536)); } + + // ── MapDataType (via DiscoverAsync) — Int64/UInt64 now map to their own members ─────── + + // MapDataType is private; reach it through DiscoverAsync with a capturing builder — the + // same indirection S7DiscoveryAndSubscribeTests uses. T1 split the formerly-lossy + // Int64/UInt64 → Int32 line so 64-bit tags surface as the matching DriverDataType. + + private sealed class CapturingBuilder : IAddressSpaceBuilder + { + public readonly List<(string Name, DriverAttributeInfo Attr)> Variables = new(); + + /// Records the folder and returns this builder for chaining. + /// The browse name of the folder. + /// The display name of the folder. + /// This builder instance. + public IAddressSpaceBuilder Folder(string browseName, string displayName) => this; + + /// Records the variable's name + attribute info. + /// The browse name of the variable. + /// The display name of the variable. + /// The attribute information for the variable. + /// A stub handle. + public IVariableHandle Variable(string browseName, string displayName, DriverAttributeInfo attributeInfo) + { + Variables.Add((browseName, attributeInfo)); + return new StubHandle(); + } + + /// No-op property sink. + /// The browse name of the property. + /// The data type of the property. + /// The initial value of the property. + public void AddProperty(string browseName, DriverDataType dataType, object? value) { } + + private sealed class StubHandle : IVariableHandle + { + /// Gets the full reference of the variable. + public string FullReference => "stub"; + + /// Marks this variable as an alarm condition. + /// The alarm condition information. + /// An alarm condition sink. + public IAlarmConditionSink MarkAsAlarmCondition(AlarmConditionInfo info) + => throw new NotImplementedException("S7 driver never calls this"); + } + } + + /// Verifies that an Int64 tag discovers a node with DriverDataType.Int64 (no longer lossily mapped to Int32). + [Fact] + public async Task DiscoverAsync_Int64_tag_maps_to_DriverDataType_Int64() + { + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Tags = [new S7TagDefinition("Counter64", "DB1.DBB0", S7DataType.Int64)], + }; + using var drv = new S7Driver(opts, "s7-i64-map"); + + var builder = new CapturingBuilder(); + await drv.DiscoverAsync(builder, TestContext.Current.CancellationToken); + + builder.Variables.Single(v => v.Name == "Counter64").Attr.DriverDataType + .ShouldBe(DriverDataType.Int64, "Int64 now maps to its own DriverDataType member, not lossy Int32"); + } + + /// Verifies that a UInt64 tag discovers a node with DriverDataType.UInt64. + [Fact] + public async Task DiscoverAsync_UInt64_tag_maps_to_DriverDataType_UInt64() + { + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Tags = [new S7TagDefinition("Total64", "DB1.DBB0", S7DataType.UInt64)], + }; + using var drv = new S7Driver(opts, "s7-u64-map"); + + var builder = new CapturingBuilder(); + await drv.DiscoverAsync(builder, TestContext.Current.CancellationToken); + + builder.Variables.Single(v => v.Name == "Total64").Attr.DriverDataType + .ShouldBe(DriverDataType.UInt64, "UInt64 now maps to its own DriverDataType member, not lossy Int32"); + } }