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
This commit is contained in:
Joseph Doherty
2026-10-22 15:04:19 -04:00
parent 7bcb6331ed
commit 87a7414df7
19 changed files with 598 additions and 368 deletions
@@ -48,8 +48,23 @@ public static class S7TagDefinitionFactory
/// <param name="def">The mapped definition when this returns <see langword="true"/>.</param>
/// <returns><see langword="true"/> when <paramref name="tagConfig"/> is a valid S7 address object.</returns>
public static bool FromTagConfig(string tagConfig, string rawPath, out S7TagDefinition def)
=> FromTagConfig(tagConfig, rawPath, out def, out _);
/// <summary>
/// <see cref="FromTagConfig(string, string, out S7TagDefinition)"/>, also reporting WHY a structurally
/// valid blob was refused by <see cref="S7TagShapeRules"/> (an unparseable address, or a shape the
/// driver cannot serve). <paramref name="shapeRejection"/> stays <see langword="null"/> for a JSON-level
/// refusal (missing keys, bad enum), which <see cref="Inspect"/> explains on its own.
/// </summary>
/// <param name="tagConfig">The authored equipment-tag TagConfig JSON.</param>
/// <param name="rawPath">The tag's RawPath — becomes the definition's identity (<c>Name</c>).</param>
/// <param name="def">The mapped definition when this returns <see langword="true"/>.</param>
/// <param name="shapeRejection">The shape/address rejection reason, when that is why it refused.</param>
/// <returns><see langword="true"/> when the driver can serve the tag.</returns>
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
/// <b>reject</b> — a structurally unparseable or non-object document, an absent / non-string /
/// blank <c>address</c>, an <b>absent</b> <c>dataType</c> (see <see cref="DeclaresDataType"/>), a
/// present-but-invalid <c>dataType</c> (the runtime enum read is STRICT, so a
/// typo rejects the tag), or an out-of-range <c>stringLength</c> on a <c>String</c> tag. Such a tag
/// typo rejects the tag), an out-of-range <c>stringLength</c> on a <c>String</c> tag, or a shape
/// <see cref="S7TagShapeRules"/> 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 <c>BadNodeIdUnknown</c> forever. Empty when the mapper accepts
/// the blob, when it is blank, or when it is not an equipment-tag object. Never throws.
/// </summary>
@@ -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
{
@@ -0,0 +1,149 @@
namespace ZB.MOM.WW.OtOpcUa.Driver.S7;
/// <summary>
/// <b>The single authority on which S7 tag shapes the driver can serve.</b> Consulted by
/// <see cref="S7TagDefinitionFactory.FromTagConfig"/>, so the AdminUI editor, the deploy gate
/// (<see cref="S7TagDefinitionFactory.Inspect"/>) and the driver's own init all refuse the same
/// tags with the same words.
/// </summary>
/// <remarks>
/// <para>
/// These rules used to be init-time guards in <c>S7Driver</c> that THREW. One mis-authored tag
/// (<c>DB1.DBZ0</c>, <c>DB1.DBW0</c> typed <c>Float64</c>, <c>T5</c> typed <c>Int32</c>) 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
/// <c>BadNodeIdUnknown</c>) and the deploy gate warns about it.
/// </para>
/// <para>
/// 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.
/// </para>
/// </remarks>
public static class S7TagShapeRules
{
/// <summary>
/// S7 data types addressed by START BYTE (the <c>B</c> 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.
/// </summary>
private static readonly HashSet<S7DataType> 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,
];
/// <summary>True when <paramref name="dataType"/> is a wide/structured type read from a byte-anchored block.</summary>
/// <param name="dataType">The declared data type.</param>
/// <returns><see langword="true"/> for the byte-anchored types.</returns>
public static bool IsByteAnchored(S7DataType dataType) => ByteAnchoredDataTypes.Contains(dataType);
/// <summary>
/// Judges one tag definition. Returns <see langword="null"/> when the driver can serve it, or
/// the operator-facing reason it cannot.
/// </summary>
/// <param name="def">The mapped tag definition.</param>
/// <param name="parsed">The parsed address when this returns <see langword="null"/>.</param>
/// <returns>The rejection reason, or <see langword="null"/> when the tag is servable.</returns>
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)",
};
}
@@ -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 "[]";
}
/// <summary>
/// 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>
/// <summary>
/// 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<string, string>();
}
private static readonly HashSet<S7DataType> 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,
};
/// <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 _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.");
}
}
}
/// <summary>
/// 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.
/// <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()
{
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.");
}
}
}
/// <summary>
/// S7DataType members that the read/write helpers throw NotSupportedException for.
/// <b>Now empty</b> — 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
/// <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();
// ---- IReadable ----
// The batch is served in two passes under ONE gate acquisition. First S7ReadSpanPlanner groups
@@ -1378,7 +1220,7 @@ public sealed class S7Driver
/// <returns><c>true</c> if the tag routes through the byte-buffer codec.</returns>
internal static bool IsBufferType(S7TagDefinition tag, S7ParsedAddress addr) =>
addr.Area is S7Area.Timer or S7Area.Counter
|| ByteAnchoredDataTypes.Contains(tag.DataType);
|| S7TagShapeRules.IsByteAnchored(tag.DataType);
/// <summary>
/// Reads a wide/structured scalar as ONE contiguous byte block via S7.Net's