From 988a7a938fdf92fc88ea0a77b4696b8abdb601d2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 17 Jun 2026 06:31:41 -0400 Subject: [PATCH] fix(s7): UInt64 box cast + Timer/Counter transient-write returns BadNotWritable (final review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M1: add missing (object) cast to UInt64 arm of DecodeScalarBlock switch expression, matching the Int64 arm style and the comment that each arm is boxed explicitly. M2: short-circuit Timer/Counter writes in WriteAsync to BadNotWritable before WriteOneAsync, so transient equipment-tag refs (Writable=true from parser) return the same status code as authored tags rejected at init — documented in the docs. Adds 6 pure unit tests pinning the area-detection precondition the guard relies on. EncodeScalarBlock Timer/Counter throws remain as the defensive backstop. --- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 18 ++++++++++- .../S7ScalarBlockTests.cs | 32 ++++++++++++++++++- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index fa7d22c9..96cf059c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -670,7 +670,7 @@ public sealed class S7Driver return tag.DataType switch { S7DataType.Int64 => (object)System.Buffers.Binary.BinaryPrimitives.ReadInt64BigEndian(block), - S7DataType.UInt64 => System.Buffers.Binary.BinaryPrimitives.ReadUInt64BigEndian(block), + S7DataType.UInt64 => (object)System.Buffers.Binary.BinaryPrimitives.ReadUInt64BigEndian(block), S7DataType.Float64 => System.Buffers.Binary.BinaryPrimitives.ReadDoubleBigEndian(block), // S7 classic STRING: [maxLen byte][curLen byte][chars…]. S7.Net's S7String.FromByteArray @@ -949,6 +949,22 @@ public sealed class S7Driver results[i] = new WriteResult(StatusBadNotWritable); continue; } + // Timer/Counter transient-write guard: a transient equipment-tag ref is always + // resolved with Writable=true (S7EquipmentTagParser hardcodes it, and node-level + // auth governs writes), so Timer/Counter addresses bypass the !Writable gate above + // and would otherwise reach EncodeScalarBlock, which throws NotSupportedException → + // BadNotSupported. The docs say Timer/Counter writes return BadNotWritable. Detect + // the area here before the call so BOTH authored (caught by guard-d at init) and + // transient Timer/Counter writes consistently return BadNotWritable. + // TryParse (not Parse) so a malformed address falls through to the existing + // error path rather than throwing here. + if (_parsedByName.TryGetValue(tag.Name, out var tcParsed) + ? tcParsed.Area is S7Area.Timer or S7Area.Counter + : (S7AddressParser.TryParse(tag.Address, out tcParsed) && tcParsed.Area is S7Area.Timer or S7Area.Counter)) + { + results[i] = new WriteResult(StatusBadNotWritable); + continue; + } try { await WriteOneAsync(plc, tag, w.Value, cancellationToken).ConfigureAwait(false); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ScalarBlockTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ScalarBlockTests.cs index f2672bf6..583d658e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ScalarBlockTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ScalarBlockTests.cs @@ -498,7 +498,10 @@ public sealed class S7ScalarBlockTests // ── EncodeScalarBlock — Timer/Counter are read-only this phase ───────────────────────── - /// Verifies a Timer write throws NotSupportedException — Timer/Counter are read-only. + /// Verifies a Timer write throws NotSupportedException — Timer/Counter are read-only. + /// This backstop is still exercised even though now + /// short-circuits to BadNotWritable before reaching EncodeScalarBlock — a mis-route (e.g. + /// a future refactor that bypasses the WriteAsync guard) must still fail loudly here. [Fact] public void EncodeScalarBlock_Timer_throws_read_only() => Should.Throw(() => S7Driver.EncodeScalarBlock(TimerTag(), 1.0)); @@ -507,4 +510,31 @@ public sealed class S7ScalarBlockTests [Fact] public void EncodeScalarBlock_Counter_throws_read_only() => Should.Throw(() => S7Driver.EncodeScalarBlock(CounterTag(), 42)); + + // ── WriteAsync guard — Timer/Counter area precondition ──────────────────────────────── + + /// + /// Pins the precondition for the Timer/Counter + /// short-circuit: on a Timer address must + /// yield and on a Counter address must yield + /// . The WriteAsync guard detects the area via TryParse (for + /// transient equipment-tag refs not in _parsedByName) and returns BadNotWritable without + /// reaching EncodeScalarBlock. This test verifies the area detection that gate depends on + /// — a pure, no-PLC assertion. ( itself requires a connected Plc + /// and is therefore integration-tested only; the EncodeScalarBlock_Timer/Counter_throws + /// tests above cover the defensive backstop if the guard is ever bypassed.) + /// + [Theory] + [InlineData("T0", S7Area.Timer)] + [InlineData("T5", S7Area.Timer)] + [InlineData("T15", S7Area.Timer)] + [InlineData("C0", S7Area.Counter)] + [InlineData("C3", S7Area.Counter)] + [InlineData("C10", S7Area.Counter)] + public void AddressParser_yields_Timer_or_Counter_area_for_TC_addresses(string address, S7Area expectedArea) + { + S7AddressParser.TryParse(address, out var parsed).ShouldBeTrue(); + parsed.Area.ShouldBe(expectedArea, + $"WriteAsync BadNotWritable guard relies on TryParse('{address}').Area == {expectedArea}"); + } }