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

feat(s7): unblock wide-type/Timer-Counter init guards + fix Int64/UInt64 node mapping

Browse Source
This commit is contained in:
Joseph Doherty
2026-06-17 05:24:48 -04:00
parent b1256bcbf2
commit 06b858eb02
3 changed files with 284 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs
+93 -43
View File
@@ -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
}
/// <summary>
/// Rejects tag addresses the read path cannot serve. Timer (<c>T{n}</c>) and Counter
/// (<c>C{n}</c>) addresses parse cleanly via <see cref="S7AddressParser"/> but
/// <see cref="ReadOneAsync"/> has no decode case for them and <see cref="S7DataType"/>
/// has no Timer/Counter member — left unguarded they fail fast init's promise and throw
/// a misleading type-mismatch on every read instead (code-review finding Driver.S7-001).
/// S7 data types addressed by START BYTE (the <c>B</c> suffix) rather than by an
/// access-width suffix. Phase 4d decodes these from a contiguous byte buffer
/// (<c>DB{n}.DBB{offset}</c> / <c>MB</c> / <c>IB</c> / <c>QB{offset}</c>), so a wide tag
/// authored against a non-byte suffix (DBW/DBD/…) is a config error — see
/// <see cref="RejectUnsupportedTagConfigs"/>.
/// </summary>
private void RejectUnsupportedTagAddresses()
private static readonly HashSet<S7DataType> ByteAnchoredDataTypes = new()
{
S7DataType.Int64,
S7DataType.UInt64,
S7DataType.Float64,
S7DataType.String,
S7DataType.DateTime,
};
/// <summary>
/// Combined init-time config guard (Phase 4d, supersedes the old Timer/Counter-only
/// <c>RejectUnsupportedTagAddresses</c>). Timer/Counter tags and the wide/structured
/// types (Int64/UInt64/Float64/String/DateTime) are now <i>supported</i> — 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:
/// <list type="bullet">
/// <item>(a) a wide/structured type declared as an array — wide-type arrays are out of scope this phase.</item>
/// <item>(b) a wide/structured type on a DB/M/I/Q area whose address isn't byte-anchored (<c>DBB</c>/<c>MB</c>/<c>IB</c>/<c>QB</c>).</item>
/// <item>(c) a Timer tag not typed <c>Float64</c>, or a Counter tag not typed <c>Int32</c>.</item>
/// </list>
/// Addresses that don't parse are left to the existing <see cref="S7AddressParser.Parse"/>
/// pass in <see cref="InitializeAsync"/> so the parse error isn't double-handled.
/// </summary>
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).");
}
}
}
/// <summary>
/// Rejects tags configured with an <see cref="S7DataType"/> that
/// <see cref="ReinterpretRawValue"/> / <see cref="BoxValueForWrite"/> still throw
/// <see cref="NotSupportedException"/> for. Without this guard those tags create live
/// OPC UA nodes via <see cref="DiscoverAsync"/> but every Read/Write returns
/// <c>BadNotSupported</c> — code-review finding Driver.S7-013. Drop entries from
/// <see cref="UnimplementedDataTypes"/> as each type is wired through.
/// Rejects tags configured with an <see cref="S7DataType"/> still on the
/// <see cref="UnimplementedDataTypes"/> list — types <see cref="ReinterpretRawValue"/> /
/// <see cref="BoxValueForWrite"/> throw <see cref="NotSupportedException"/> for, which
/// would otherwise create live OPC UA nodes via <see cref="DiscoverAsync"/> that return
/// <c>BadNotSupported</c> on every access (code-review finding Driver.S7-013).
/// <b>The set is now empty</b> (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.
/// </summary>
private void RejectUnsupportedTagDataTypes()
{
@@ -343,18 +398,12 @@ public sealed class S7Driver
/// <summary>
/// S7DataType members that the read/write helpers throw NotSupportedException for.
/// Kept here (rather than reflecting over <see cref="ReinterpretRawValue"/>) so
/// <see cref="RejectUnsupportedTagDataTypes"/> is a single grep target for the
/// follow-up PR that wires each through.
/// <b>Now empty</b> — 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 <see cref="ReinterpretRawValue"/>) as
/// the single grep target / future seam should a data type ever need re-gating at init.
/// </summary>
private static readonly HashSet<S7DataType> UnimplementedDataTypes = new()
{
S7DataType.Int64,
S7DataType.UInt64,
S7DataType.Float64,
S7DataType.String,
S7DataType.DateTime,
};
private static readonly HashSet<S7DataType> UnimplementedDataTypes = new();
/// <summary>
/// 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,
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs
+108
View File
@@ -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).
/// <summary>Verifies that a valid byte-addressed wide scalar (Float64 at DB1.DBB0) passes the init guard.</summary>
[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<Exception>(async () =>
await drv.InitializeAsync("{}", TestContext.Current.CancellationToken));
ex.ShouldNotBeOfType<NotSupportedException>(
"a byte-addressed wide scalar must pass the init guard — the failure must be the connect");
ex.ShouldNotBeOfType<FormatException>(
"DB1.DBB0 parses cleanly — the failure must be the connect, not an address-parse error");
}
/// <summary>Verifies that a wide-type array (Int64 + ArrayCount) is rejected as out-of-scope this phase.</summary>
[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<NotSupportedException>(async () =>
await drv.InitializeAsync("{}", TestContext.Current.CancellationToken));
ex.Message.ShouldContain("Batch64");
ex.Message.ShouldContain("array", Case.Insensitive);
drv.GetHealth().State.ShouldBe(DriverState.Faulted);
}
/// <summary>Verifies that a wide scalar with a non-byte address (Float64 at DB1.DBW0) is rejected with a byte-address message.</summary>
[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<NotSupportedException>(async () =>
await drv.InitializeAsync("{}", TestContext.Current.CancellationToken));
ex.Message.ShouldContain("WideWord");
ex.Message.ShouldContain("byte", Case.Insensitive);
drv.GetHealth().State.ShouldBe(DriverState.Faulted);
}
/// <summary>Verifies that a Timer tag with the wrong DataType (Int32, not Float64) is rejected.</summary>
[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<NotSupportedException>(async () =>
await drv.InitializeAsync("{}", TestContext.Current.CancellationToken));
ex.Message.ShouldContain("Timer5");
ex.Message.ShouldContain("Float64");
drv.GetHealth().State.ShouldBe(DriverState.Faulted);
}
/// <summary>Verifies that a Counter tag with the wrong DataType (Float32, not Int32) is rejected.</summary>
[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<NotSupportedException>(async () =>
await drv.InitializeAsync("{}", TestContext.Current.CancellationToken));
ex.Message.ShouldContain("Counter3");
ex.Message.ShouldContain("Int32");
drv.GetHealth().State.ShouldBe(DriverState.Faulted);
}
}
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TypeMappingTests.cs
+83
View File
@@ -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<OverflowException>(() => 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();
/// <summary>Records the folder and returns this builder for chaining.</summary>
/// <param name="browseName">The browse name of the folder.</param>
/// <param name="displayName">The display name of the folder.</param>
/// <returns>This builder instance.</returns>
public IAddressSpaceBuilder Folder(string browseName, string displayName) => this;
/// <summary>Records the variable's name + attribute info.</summary>
/// <param name="browseName">The browse name of the variable.</param>
/// <param name="displayName">The display name of the variable.</param>
/// <param name="attributeInfo">The attribute information for the variable.</param>
/// <returns>A stub handle.</returns>
public IVariableHandle Variable(string browseName, string displayName, DriverAttributeInfo attributeInfo)
{
Variables.Add((browseName, attributeInfo));
return new StubHandle();
}
/// <summary>No-op property sink.</summary>
/// <param name="browseName">The browse name of the property.</param>
/// <param name="dataType">The data type of the property.</param>
/// <param name="value">The initial value of the property.</param>
public void AddProperty(string browseName, DriverDataType dataType, object? value) { }
private sealed class StubHandle : IVariableHandle
{
/// <summary>Gets the full reference of the variable.</summary>
public string FullReference => "stub";
/// <summary>Marks this variable as an alarm condition.</summary>
/// <param name="info">The alarm condition information.</param>
/// <returns>An alarm condition sink.</returns>
public IAlarmConditionSink MarkAsAlarmCondition(AlarmConditionInfo info)
=> throw new NotImplementedException("S7 driver never calls this");
}
}
/// <summary>Verifies that an Int64 tag discovers a node with DriverDataType.Int64 (no longer lossily mapped to Int32).</summary>
[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");
}
/// <summary>Verifies that a UInt64 tag discovers a node with DriverDataType.UInt64.</summary>
[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");
}
}