From 1f7ab80b814402a9bf93b6999b5848bd294da5c7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:30:12 -0400 Subject: [PATCH] fix(s7): a refused span falls back per member and is not regrouped (R07:P2) A span's block is the union of its members' ranges, so one member that runs past the DB's real end (or over-declares a STRING) made the CPU refuse the whole block, and every neighbour within it was Bad on every poll. The planner's stated invariant (a grouped read cannot fault where per-tag reads would succeed) was false; the multi-var path already fell back, spans did not. - A refused multi-member span hands its members back; they are read one at a time for that pass, bypassing the multi-var pass. - _noGroup remembers the members refused on their own (or every member when none was) and keeps them out of later plans, so the rest group again. It is cleared by every initialize publish. Non-refusal span failures are unchanged (every member stamped). - The once-per-tag refusal Warning memo (R07:H4) is now also cleared by the publish, and shutdown resets the write-side Degraded owner, as documented. - Live gate: MW10 + MD254 (2 bytes past the 256-byte M area) merged into one refused block; MW10 keeps reading 4097 on every pass. Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/S7.md | 7 + .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 74 +++++++- .../S7ReadSpanPlanner.cs | 10 +- .../S7_1500/S7_1500SpanRefusalTests.cs | 49 ++++++ .../S7SpanRefusalFallbackTests.cs | 161 ++++++++++++++++++ 5 files changed, 295 insertions(+), 6 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanRefusalTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 9491c93c..207d26b1 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -344,6 +344,13 @@ to one doomed per-tag read per member. Without this an op that ignores its cance one full deadline per remaining item under the gate, and `Degraded` (and so the comms-loss watchdog's clock) only became visible after the whole pass. +**A refused span falls back per member (R07:P2).** A grouped block read covers the union of its members' +ranges, so one member the CPU refuses (running past the DB's real end, an over-declared `stringLength`) +used to make every neighbour in the block Bad on every poll. When the CPU refuses a multi-member span the +driver re-reads its members one at a time for that pass and remembers which tags must not be grouped — +the members refused on their own, or every member when none was — so later passes group only the rest. +The memo lasts until the next initialize. A span failure that is not a refusal still stamps every member. + **Connect backoff.** A dead PLC costs one connect attempt per backoff window (1 s doubling to a 30 s cap), not one per read / poll tick / probe tick: inside the window `EnsureConnectedAsync` fails fast (`BadCommunicationError`, `Degraded`) without a connect. A successful connect clears the window, so a 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 e413125c..9b0ce86b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -221,6 +221,14 @@ public sealed class S7Driver /// by every initialize publish. /// private readonly ConcurrentDictionary _refusalWarned = new(StringComparer.Ordinal); + + /// + /// Tags the span planner must not group (R07:P2): members of a span the CPU refused as a block — + /// the ones that were refused on their own, or every member when none was. They are read + /// individually for the rest of the generation; a re-initialize (a corrected DB, a fixed tag) + /// groups everything again. Guarded by . + /// + private readonly HashSet _noGroup = new(StringComparer.Ordinal); private bool _disposed; /// @@ -338,6 +346,8 @@ public sealed class S7Driver _options = options; Volatile.Write(ref _tables, new TagTables(tags, parsed)); + _refusalWarned.Clear(); + _noGroup.Clear(); _skippedTags.BeginRebuild(options.RawTags.Count); foreach (var (rawPath, reason) in skipped) { @@ -453,6 +463,7 @@ public sealed class S7Driver { if (_generation != gen) return; // a newer initialize owns the state now _writeFault = null; + _writeOwnedError = null; _connectBackoff.RecordSuccess(); Health = new DriverHealth(DriverState.Unknown, Health.LastSuccessfulRead, null); } @@ -732,6 +743,13 @@ public sealed class S7Driver // Indexes the per-tag path must serve: everything, when grouping is off; otherwise the // planner's leftovers (Timer/Counter) plus any tag whose address won't parse. var perTag = new List(); + // Indexes that must take the per-tag path WITHOUT the multi-var pass: tags the planner may + // not group (R07:P2) and the members of a span refused this pass. Batching them into a + // multi-var request would just refuse the whole chunk again. + List? forcedPerTag = null; + // Members of each span refused this pass, and which of them were then refused on their own. + List>? refusedSpans = null; + HashSet? refusedAlone = null; if (_options.SpanReadMaxGapBytes >= 0) { @@ -739,6 +757,11 @@ public sealed class S7Driver for (var i = 0; i < fullReferences.Count; i++) { if (resolved[i] is not { } tag) continue; + if (_noGroup.Contains(tag.Name)) + { + (forcedPerTag ??= []).Add(i); + continue; + } if (!TryGetParsedAddress(tag, out var addr)) { // Defensive: every tag pre-parses at init. A tag that somehow didn't goes @@ -769,7 +792,12 @@ public sealed class S7Driver foreach (var member in span.Members) results[member.OriginalIndex] = DeadHandleSnapshot(now); continue; } - await ReadSpanAsync(plc, span, results, now, outcome, cancellationToken).ConfigureAwait(false); + if (await ReadSpanAsync(plc, span, results, now, outcome, cancellationToken).ConfigureAwait(false) + is { } refusedMembers) + { + (refusedSpans ??= []).Add(refusedMembers); + (forcedPerTag ??= []).AddRange(refusedMembers); + } } perTag.AddRange(plan.LeftoverIndexes); } @@ -794,6 +822,11 @@ public sealed class S7Driver perTag = await ReadLeftoversByMultiVarAsync( plc, resolved, perTag, results, now, outcome, cancellationToken).ConfigureAwait(false); } + if (forcedPerTag is not null) + { + perTag.AddRange(forcedPerTag); + perTag.Sort(); + } foreach (var i in perTag) { @@ -823,9 +856,25 @@ public sealed class S7Driver } catch (Exception ex) { + if (refusedSpans is not null && IsRefusal(ex)) (refusedAlone ??= []).Add(i); results[i] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome, [resolved[i]!.Name]), null, now); } } + + // Remember what must not be grouped again (R07:P2). Skipped when the handle died mid-pass: + // the per-member reads then proved nothing about which member the CPU refuses. + if (refusedSpans is not null && !_plcDead) + { + foreach (var members in refusedSpans) + { + var bad = members.Where(m => refusedAlone?.Contains(m) == true).ToList(); + var memo = bad.Count > 0 ? bad : [.. members]; + foreach (var m in memo) _noGroup.Add(resolved[m]!.Name); + _logger.LogInformation( + "S7 CPU refused a grouped read of {MemberCount} tag(s); reading {Tags} individually from now on. Driver={DriverInstanceId}", + members.Count, string.Join(", ", memo.Select(m => resolved[m]!.Name)), _driverInstanceId); + } + } } finally { @@ -1222,8 +1271,12 @@ public sealed class S7Driver /// The batch timestamp, shared by every tag in the batch. /// The pass's outcome accumulator; every member records into it. /// Cancellation token for the read. - /// A task that completes once every member of the span has been stamped. - private async Task ReadSpanAsync( + /// + /// once every member has been stamped; or, when the CPU REFUSED the block + /// of a multi-member span (R07:P2 — one member past the DB's end refuses them all), the members' + /// original indexes, unstamped, for the caller to read one at a time. + /// + private async Task?> ReadSpanAsync( IS7Plc plc, S7ReadSpanPlanner.Span span, DataValueSnapshot[] results, DateTime now, ReadPassOutcome outcome, CancellationToken ct) { @@ -1244,12 +1297,24 @@ public sealed class S7Driver _plcDead = true; throw; } + catch (Exception ex) when (span.Members.Count > 1 && IsRefusal(ex)) + { + // The planner's old invariant ("a grouped read cannot fault where the per-tag reads would + // have succeeded") is false: the block is the UNION of the members' ranges, so one member + // running past the DB's real end, or over-declaring a STRING, makes the CPU refuse every + // neighbour with it. Hand the members back for individual reads. + _logger.LogDebug(ex, + "S7 driver {DriverInstanceId}: CPU refused the block read of {Area} db{Db} @{Start}+{Length}; " + + "re-reading its {Count} member(s) individually", _driverInstanceId, span.Area, span.DbNumber, + span.StartByte, span.LengthBytes, span.Members.Count); + return [.. span.Members.Select(m => m.OriginalIndex)]; + } catch (Exception ex) { var status = RecordReadFailure(ex, outcome, span.Members.Select(m => m.Tag.Name)); foreach (var member in span.Members) results[member.OriginalIndex] = new DataValueSnapshot(null, status, null, now); - return; + return null; } foreach (var member in span.Members) @@ -1268,6 +1333,7 @@ public sealed class S7Driver new DataValueSnapshot(null, RecordReadFailure(ex, outcome, [member.Tag.Name]), null, now); } } + return null; } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ReadSpanPlanner.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ReadSpanPlanner.cs index 96f7770a..dfe05b77 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ReadSpanPlanner.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ReadSpanPlanner.cs @@ -11,8 +11,14 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.S7; /// The safety invariant is that a span never extends past its last member's end. A /// span may deliberately read bytes nobody asked for BETWEEN its members (that is what the /// gap budget buys), but every byte it requests lies inside a range some member was going -/// to read anyway. A grouped read therefore cannot fault (area overrun, DB overrun) where -/// the individual per-tag reads would have succeeded. +/// to read anyway. +/// +/// +/// That does NOT make a grouped read as safe as the per-tag reads (R07:P2): the block is +/// the union of its members' ranges, so ONE member the CPU refuses (running past the DB's real +/// end, an over-declared STRING) makes it refuse the whole block, neighbours included. +/// owns that case: a refused multi-member span is re-read one +/// member at a time, and the members that must not be grouped are left out of later plans. /// /// /// Timer and Counter tags are never grouped. Their diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanRefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanRefusalTests.cs new file mode 100644 index 00000000..96dde491 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanRefusalTests.cs @@ -0,0 +1,49 @@ +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:P2 over real S7comm. The fixture's M area is 256 bytes. MW10 is seeded +/// (4097); MD254 runs two bytes past the end of the area. With the gap budget opened wide +/// the planner merges them into ONE block MB10..258, which the CPU refuses — before the fix +/// that made the perfectly readable MW10 Bad (and the driver Degraded) on every poll. +/// +[Collection(Snap7ServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Device", "S7_1500")] +public sealed class S7_1500SpanRefusalTests(Snap7ServerFixture sim) +{ + /// The good neighbour of an over-running member keeps reading its seeded value, every pass. + [Fact] + public async Task A_member_past_the_area_end_does_not_spoil_its_span_neighbour() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + var ct = TestContext.Current.CancellationToken; + + var options = new S7DriverOptions + { + Host = sim.Host, Port = sim.Port, CpuType = S7CpuType.S71500, + Timeout = TimeSpan.FromSeconds(5), + Probe = new S7ProbeOptions { Enabled = false }, + // Wide enough that MW10 and MD254 land in one planned span. + SpanReadMaxGapBytes = 256, + SpanReadMaxSpanBytes = 256, + RawTags = S7RawTags.Entries( + new S7TagDefinition("Mk/Word10", "MW10", S7DataType.UInt16, Writable: false), + new S7TagDefinition("Mk/PastEnd", "MD254", S7DataType.Int32, Writable: false)), + }; + await using var drv = new S7Driver(options, driverInstanceId: "s7-span-refusal-live"); + await drv.InitializeAsync("{}", ct); + + for (var pass = 0; pass < 3; pass++) + { + var r = await drv.ReadAsync(["Mk/Word10", "Mk/PastEnd"], ct); + r[0].StatusCode.ShouldBe(0u, $"pass {pass}: the in-range neighbour must be served"); + Convert.ToInt32(r[0].Value).ShouldBe(4097); + r[1].StatusCode.ShouldNotBe(0u, "the over-running member is refused on its own"); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a refusal is not a comms fault"); + } + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs new file mode 100644 index 00000000..ce6ff0ca --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs @@ -0,0 +1,161 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// R07:P2 — a span's block is the union of its members' ranges, so ONE member that runs past the +/// DB's real end (or over-declares a STRING) made the CPU refuse the whole block and every +/// neighbour inside it went Bad on every poll. A refused span now re-reads its members one at a +/// time for that pass and remembers which tags must not be grouped, so later passes group only the +/// members that can be grouped. +/// +[Trait("Category", "Unit")] +public sealed class S7SpanRefusalFallbackTests +{ + private const uint BadDeviceFailure = 0x808B0000u; + + /// DB1 is 4 bytes long on this fake CPU: anything reaching past byte 4 is refused. + private const int DbLength = 4; + + private static Exception OutOfRange() => new("Received error from PLC: Address out of range."); + + private static (S7Driver Driver, ScriptedS7PlcFactory Factory, Func BlockReads, Func TagReads) Start( + params S7TagDefinition[] tags) + { + var factory = new ScriptedS7PlcFactory(); + var blockReads = 0; + var tagReads = 0; + factory.ReadBytes = (_, _, start, count, _) => + { + Interlocked.Increment(ref blockReads); + if (start + count > DbLength) throw OutOfRange(); + var b = new byte[count]; + for (var i = 0; i < count; i++) b[i] = (byte)(start + i + 1); + return Task.FromResult(b); + }; + factory.Read = (addr, _) => + { + Interlocked.Increment(ref tagReads); + var offset = int.Parse(addr[(addr.LastIndexOf('W') + 1)..], System.Globalization.CultureInfo.InvariantCulture); + if (offset + 2 > DbLength) throw OutOfRange(); + return Task.FromResult((ushort)(((offset + 1) << 8) | (offset + 2))); + }; + var drv = new S7Driver(new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries(tags), + }, "s7-span-fallback", factory); + return (drv, factory, () => Volatile.Read(ref blockReads), () => Volatile.Read(ref tagReads)); + } + + private static S7TagDefinition Word(string name, int offset) => + new(name, $"DB1.DBW{offset}", S7DataType.UInt16, Writable: false); + + /// + /// The over-running member is refused on its own; its neighbours are served from the very first + /// pass, and from the second pass on the neighbours are grouped again WITHOUT the bad member. + /// + [Fact] + public async Task A_refused_span_falls_back_per_member_and_stops_grouping_the_bad_member() + { + var ct = TestContext.Current.CancellationToken; + var (drv, _, blockReads, tagReads) = Start(Word("A", 0), Word("B", 2), Word("C", 4)); + using var _ = drv; + await drv.InitializeAsync("{}", ct); + string[] refs = ["A", "B", "C"]; + + // Pass 1: the 6-byte span is refused, then each member is read on its own. + var r1 = await drv.ReadAsync(refs, ct); + r1[0].StatusCode.ShouldBe(0u); + r1[0].Value.ShouldBe((ushort)0x0102); + r1[1].StatusCode.ShouldBe(0u); + r1[1].Value.ShouldBe((ushort)0x0304); + r1[2].StatusCode.ShouldBe(BadDeviceFailure); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + blockReads().ShouldBe(1); + tagReads().ShouldBe(3); + + // Pass 2: A+B ride ONE block read again (the planner no longer sees C); C reads alone. + var r2 = await drv.ReadAsync(refs, ct); + r2[0].Value.ShouldBe((ushort)0x0102); + r2[1].Value.ShouldBe((ushort)0x0304); + r2[2].StatusCode.ShouldBe(BadDeviceFailure); + blockReads().ShouldBe(2, "A+B grouped again, without the refused member"); + tagReads().ShouldBe(4, "only C took the per-tag path on pass 2"); + } + + /// + /// When the block is refused but every member reads fine alone, the span itself is the problem: + /// all of its members stop being grouped (and stop re-trying the refused block every pass). + /// + [Fact] + public async Task A_refused_span_whose_members_all_read_alone_is_not_grouped_again() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ScriptedS7PlcFactory(); + var blockReads = 0; + factory.ReadBytes = (_, _, _, _, _) => { Interlocked.Increment(ref blockReads); throw OutOfRange(); }; + factory.Read = (_, _) => Task.FromResult((ushort)7); + using var drv = new S7Driver(new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries(Word("A", 0), Word("B", 2)), + }, "s7-span-fallback-all", factory); + await drv.InitializeAsync("{}", ct); + + (await drv.ReadAsync(["A", "B"], ct)).ShouldAllBe(s => s.StatusCode == 0u); + (await drv.ReadAsync(["A", "B"], ct)).ShouldAllBe(s => s.StatusCode == 0u); + Volatile.Read(ref blockReads).ShouldBe(1, "the refused block is not re-issued on later passes"); + } + + /// + /// Positive control: a span failure that is NOT a refusal (a device-layer fault) does not fall + /// back — re-reading N members one by one against a failing device would only multiply the fault. + /// + [Fact] + public async Task A_non_refusal_span_failure_does_not_fall_back() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ScriptedS7PlcFactory + { + ReadBytes = (_, _, _, _, _) => throw new global::S7.Net.PlcException(global::S7.Net.ErrorCode.SendData, "device fault"), + }; + using var drv = new S7Driver(new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries(Word("A", 0), Word("B", 2)), + }, "s7-span-nofallback", factory); + await drv.InitializeAsync("{}", ct); + + var before = factory.WireOps; + var r = await drv.ReadAsync(["A", "B"], ct); + r.ShouldAllBe(s => s.StatusCode == BadDeviceFailure); + (factory.WireOps - before).ShouldBe(1); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + } + + /// The memo is per generation: a re-initialize (e.g. a corrected DB) groups everything again. + [Fact] + public async Task Reinitialize_clears_the_do_not_group_memo() + { + var ct = TestContext.Current.CancellationToken; + var (drv, _, blockReads, tagReads) = Start(Word("A", 0), Word("B", 2), Word("C", 4)); + using var _ = drv; + await drv.InitializeAsync("{}", ct); + await drv.ReadAsync(["A", "B", "C"], ct); + blockReads().ShouldBe(1); + tagReads().ShouldBe(3); + + await drv.InitializeAsync("{}", ct); + await drv.ReadAsync(["A", "B", "C"], ct); + // With the memo still in force pass 2 would be one A+B block read plus ONE per-tag read (C). + // Cleared, the full A..C span is tried again, refused again, and all three re-read alone. + blockReads().ShouldBe(2); + tagReads().ShouldBe(6, "after re-init the full span is tried again (and refused again)"); + } +}