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 6dd2a123..82f8516b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -544,6 +544,9 @@ public sealed class S7Driver RequireInitialized(); var now = DateTime.UtcNow; var results = new DataValueSnapshot[fullReferences.Count]; + // Health is decided ONCE, at the end of the pass, from every outcome the pass observed — + // never per item. See ReadPassOutcome. + var outcome = new ReadPassOutcome(); await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); try @@ -612,7 +615,7 @@ public sealed class S7Driver perTag.Add(span.Members[0].OriginalIndex); continue; } - await ReadSpanAsync(plc, span, results, now, cancellationToken).ConfigureAwait(false); + await ReadSpanAsync(plc, span, results, now, outcome, cancellationToken).ConfigureAwait(false); } perTag.AddRange(plan.LeftoverIndexes); } @@ -635,7 +638,7 @@ public sealed class S7Driver if (_options.SpanReadMaxGapBytes >= 0) { perTag = await ReadLeftoversByMultiVarAsync( - plc, resolved, perTag, results, now, cancellationToken).ConfigureAwait(false); + plc, resolved, perTag, results, now, outcome, cancellationToken).ConfigureAwait(false); } foreach (var i in perTag) @@ -644,7 +647,7 @@ public sealed class S7Driver { var value = await ReadOneAsync(plc, resolved[i]!, cancellationToken).ConfigureAwait(false); results[i] = new DataValueSnapshot(value, 0u, now, now); - _health = new DriverHealth(DriverState.Healthy, now, null); + outcome.RecordSuccess(); } catch (OperationCanceledException) { @@ -660,14 +663,90 @@ public sealed class S7Driver } catch (Exception ex) { - results[i] = new DataValueSnapshot(null, RecordReadFailure(ex), null, now); + results[i] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome), null, now); } } + + // THE single health write of the pass — worst-state-wins over every outcome above. + // Deliberately the last statement of the try body rather than a finally: the three OCE + // arms abandon the batch mid-pass, and a partially-observed pass must not stamp a + // verdict. LastSuccessfulRead advances whenever ANYTHING read, even into a + // Degraded/Faulted state — see ReadPassOutcome for why that is load-bearing. + _health = new DriverHealth( + outcome.WorstFailure, + outcome.AnySuccess ? now : _health.LastSuccessfulRead, + outcome.WorstFailure == DriverState.Healthy ? null : outcome.FirstError); } finally { _gate.Release(); } return results; } + /// + /// The outcomes ONE pass observed, accumulated so the pass writes + /// exactly once — worst state wins. + /// + /// + /// + /// Health used to be stamped per item, in three separate read loops, so the pass + /// reported whatever its LAST item happened to be: a mixed pass read Healthy, and + /// the access-denied Faulted config alert — usually a minority of the batch — was + /// erased by any later good tag. Ranking (Healthy < Degraded < Faulted) and + /// deciding once at the end makes the verdict a function of the whole pass rather than + /// of its item ordering. + /// + /// + /// is tracked separately from the state on purpose. The + /// comms-loss watchdog trips only on a non-Healthy state AND a frozen + /// LastSuccessfulRead; several drivers latch Degraded off a single unreadable + /// address, so a pass that lands Degraded must still advance the timestamp when + /// anything at all was read, or an entirely reachable CPU would be reconnected every + /// 90 s forever. + /// + /// + /// Not thread-safe, and does not need to be: a pass runs wholly under + /// and every read loop inside it is sequential. + /// + /// + private sealed class ReadPassOutcome + { + /// True once any tag in the pass read successfully. + public bool AnySuccess { get; private set; } + + /// + /// The worst failure state recorded so far; means no + /// failure has been recorded at all. + /// + public DriverState WorstFailure { get; private set; } = DriverState.Healthy; + + /// + /// The FIRST failure's message — the causal one. A later failure is as likely to be a + /// consequence of the first as an independent fault, so it does not overwrite it. + /// + public string? FirstError { get; private set; } + + /// Records that one tag read successfully. + public void RecordSuccess() => AnySuccess = true; + + /// Records one failed tag, keeping the worst state and the first message. + /// The state this failure would have stamped on its own. + /// The failure's operator-facing message. + public void RecordFailure(DriverState state, string message) + { + if (Rank(state) > Rank(WorstFailure)) WorstFailure = state; + FirstError ??= message; + } + + /// Severity order the pass ranks by: Healthy (none) < Degraded < Faulted. + /// The state to rank. + /// Its severity, higher being worse. + private static int Rank(DriverState state) => state switch + { + DriverState.Faulted => 2, + DriverState.Degraded => 1, + _ => 0, + }; + } + /// /// Items one multi-var request may carry. The caller owns chunking — S7.Net does NOT /// split a multi-var read, it throws when the request or the projected response exceeds @@ -743,6 +822,7 @@ public sealed class S7Driver /// Indexes the span pass left for the per-tag path, ascending. /// The batch's result array, written in place at each tag's original index. /// The batch timestamp, shared by every tag in the batch. + /// The pass's outcome accumulator; every decoded item records into it. /// Cancellation token for the reads. /// The indexes still owed an individual read, ascending. private async Task> ReadLeftoversByMultiVarAsync( @@ -751,6 +831,7 @@ public sealed class S7Driver List leftovers, DataValueSnapshot[] results, DateTime now, + ReadPassOutcome outcome, CancellationToken ct) { var stillPerTag = new List(); @@ -825,13 +906,13 @@ public sealed class S7Driver ?? throw new System.IO.InvalidDataException( $"S7.Net returned null multi-var value for '{tag.Address}'"); results[index] = new DataValueSnapshot(ReinterpretRawValue(tag, addr, raw), 0u, now, now); - _health = new DriverHealth(DriverState.Healthy, now, null); + outcome.RecordSuccess(); } catch (Exception ex) { // Decoding is pure and synchronous — no cancellation can surface here, so there // is deliberately no OperationCanceledException arm to shadow the request one. - results[index] = new DataValueSnapshot(null, RecordReadFailure(ex), null, now); + results[index] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome), null, now); } } } @@ -902,10 +983,12 @@ public sealed class S7Driver /// The planned block read. /// The batch's result array, written in place at each member's original index. /// 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( - IS7Plc plc, S7ReadSpanPlanner.Span span, DataValueSnapshot[] results, DateTime now, CancellationToken ct) + IS7Plc plc, S7ReadSpanPlanner.Span span, DataValueSnapshot[] results, DateTime now, + ReadPassOutcome outcome, CancellationToken ct) { byte[] block; try @@ -926,7 +1009,7 @@ public sealed class S7Driver } catch (Exception ex) { - var status = RecordReadFailure(ex); + var status = RecordReadFailure(ex, outcome); foreach (var member in span.Members) results[member.OriginalIndex] = new DataValueSnapshot(null, status, null, now); return; @@ -938,13 +1021,14 @@ public sealed class S7Driver { var value = DecodeMember(member, block); results[member.OriginalIndex] = new DataValueSnapshot(value, 0u, now, now); - _health = new DriverHealth(DriverState.Healthy, now, null); + outcome.RecordSuccess(); } catch (Exception ex) { // Decoding is pure and synchronous — no cancellation can surface here, so there is // deliberately no OperationCanceledException arm to shadow the block-read one above. - results[member.OriginalIndex] = new DataValueSnapshot(null, RecordReadFailure(ex), null, now); + results[member.OriginalIndex] = + new DataValueSnapshot(null, RecordReadFailure(ex, outcome), null, now); } } } @@ -998,17 +1082,29 @@ public sealed class S7Driver /// /// The read-failure classification ladder, shared by the per-tag path and the span path so /// a failed span stamps every member with exactly the status the same exception would have - /// produced on a single tag. Applies the health / dead-handle side effects and returns the - /// status code the caller must stamp. + /// produced on a single tag. Records the failure into the pass's outcome, applies the + /// dead-handle side effect, and returns the status code the caller must stamp. /// /// - /// is deliberately NOT handled here: it is control - /// flow rather than a per-tag status (it marks the handle dead and propagates), and - /// rethrowing it from a helper would rewrite the stack. Both call sites catch it first. + /// + /// The health state is recorded, not applied. It goes into + /// so the pass can write once at the + /// end, worst-state-wins — writing it here is what let a later good tag erase this + /// failure. The _plcDead / side effects + /// stay IMMEDIATE: they govern whether the very next read reopens the handle, which + /// cannot wait for the pass to finish. + /// + /// + /// is deliberately NOT handled here: it is + /// control flow rather than a per-tag status (it marks the handle dead and propagates), + /// and rethrowing it from a helper would rewrite the stack. Both call sites catch it + /// first. + /// /// /// The exception observed on a read. + /// The pass's outcome accumulator this failure is recorded into. /// The OPC UA status code every affected tag must be stamped with. - private uint RecordReadFailure(Exception ex) + private uint RecordReadFailure(Exception ex, ReadPassOutcome outcome) { switch (ex) { @@ -1020,7 +1116,7 @@ public sealed class S7Driver // fault, NOT a transient one. Blind retry is wasted effort, so map it to // BadNotSupported and flag the driver as a config alert (Faulted) rather than // Degraded — per driver-specs.md §5. - _health = new DriverHealth(DriverState.Faulted, _health.LastSuccessfulRead, + outcome.RecordFailure(DriverState.Faulted, "S7 access denied — enable PUT/GET communication in TIA Portal " + $"(Protection & Security) for this CPU. PLC reported: {pex.Message}"); return StatusBadNotSupported; @@ -1031,14 +1127,14 @@ public sealed class S7Driver // PlcException (socket drop / ConnectionError) additionally marks the handle dead // so the next read reopens it. MarkConnectionDeadIfFatal(pex); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, pex.Message); + outcome.RecordFailure(DriverState.Degraded, pex.Message); return StatusBadDeviceFailure; default: // Socket / timeout / transport fault — mark the handle dead when it's a connection // loss so EnsureConnectedAsync reopens on the next call. MarkConnectionDeadIfFatal(ex); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + outcome.RecordFailure(DriverState.Degraded, ex.Message); return StatusBadCommunicationError; } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs index 73f661b1..b55761b5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs @@ -387,10 +387,14 @@ public sealed class S7SpanReadParityTests /// failed REQUEST, and firing it here would re-read tags that already have good values. /// /// - /// Health ends Healthy rather than Degraded because the decode loop stamps - /// health per item in chunk order, so the good item after the bad one re-stamps it. That is - /// the same last-writer-wins behaviour the per-tag loop has always had; it is pinned here so - /// a change to it is a deliberate one. + /// Health ends Degraded, NOT Healthy. This pin used to assert the opposite: + /// the decode loop stamped health per item in chunk order, so the good item after the bad + /// one re-stamped it and a pass with failures reported Healthy — last-writer-wins. + /// That was deliberately replaced (perf follow-ups round 3) by worst-state-wins: the + /// whole read pass accumulates its outcomes and writes health ONCE at the end, so a mixed + /// pass can no longer report Healthy. Only the driver-level health claim changed — the + /// per-tag statuses below are exactly as they were, because a per-ITEM decode fault still + /// spoils only its own tag. /// [Fact] public async Task One_undecodable_multi_var_item_stamps_only_its_own_tag_Bad() @@ -420,7 +424,135 @@ public sealed class S7SpanReadParityTests factory.MultiVarCalls.ShouldBe(1); factory.StringReadCalls.ShouldBe(0); factory.ReadBytesCalls.ShouldBe(0); - driver.GetHealth().State.ShouldBe(DriverState.Healthy); + driver.GetHealth().State.ShouldBe(DriverState.Degraded); + } + + // ── Worst-state-wins read-pass health ──────────────────────────────────────────────── + + /// + /// A pass with BOTH failures and successes reports Degraded — the failure is not + /// erased by the later success — AND still advances LastSuccessfulRead, because + /// something genuinely was read. + /// + /// + /// Advancing the timestamp on a Degraded pass is load-bearing, not cosmetic: the + /// comms-loss watchdog (CLAUDE.md §"Comms-loss detection") trips on non-Healthy state + /// AND a frozen LastSuccessfulRead. Several drivers latch Degraded off one + /// unreadable address, so freezing the timestamp on any non-Healthy pass would reconnect a + /// perfectly reachable PLC every 90 s forever. + /// + [Fact] + public async Task A_mixed_read_pass_reports_Degraded_and_still_advances_LastSuccessfulRead() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ImageS7PlcFactory(Images()) { NullItemIndex = 1 }; + using var driver = new S7Driver(Options(ScatteredWords(3), maxGap: 2), "s7-mixed-pass", factory); + await driver.InitializeAsync("{}", ct); + + // The connect stamped a LastSuccessfulRead; the pass must move it PAST that value, so a + // frozen timestamp is distinguishable from a merely non-null one. + var before = driver.GetHealth().LastSuccessfulRead; + before.ShouldNotBeNull(); + + var results = await driver.ReadAsync(ScatteredRefs(3), ct); + + results[0].StatusCode.ShouldBe(0u); + results[1].StatusCode.ShouldBe(StatusBadCommunicationError); + results[2].StatusCode.ShouldBe(0u); + + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Degraded, "a pass that had a failure must not report Healthy"); + health.LastSuccessfulRead.ShouldNotBeNull(); + health.LastSuccessfulRead!.Value.ShouldBeGreaterThan( + before!.Value, "two of the three tags read fine — the watchdog's evidence must advance"); + } + + /// + /// The SPAN path obeys the same rule: one member that fails to decode leaves the pass + /// Degraded even though a later member of the same block decoded fine. + /// + /// + /// The block read itself succeeds — this is the per-member decode guard, the span-path + /// sibling of the multi-var per-item guard above. The poisoned member is an S7 STRING laid + /// over the image's filler pattern, whose two-byte header declares a current length larger + /// than its capacity; S7String.FromByteArray rejects that. It is deliberately the + /// FIRST member (lower offset) so the good member decodes after it — the exact ordering + /// last-writer-wins used to erase. + /// + [Fact] + public async Task One_undecodable_span_member_leaves_the_pass_Degraded() + { + var ct = TestContext.Current.CancellationToken; + // The STRING occupies DB1 bytes 100..111 (StringLength + 2), so the word at 112 is + // adjacent and lands in the SAME span. + var tags = S7RawTags.Entries( + new S7TagDefinition("BadStr", "DB1.DBB100", S7DataType.String, Writable: false, StringLength: 10), + new S7TagDefinition("GoodWord", "DB1.DBW112", S7DataType.UInt16, Writable: false)); + var factory = new ImageS7PlcFactory(Images()); + using var driver = new S7Driver(Options(tags, maxGap: 16), "s7-span-decode-fault", factory); + await driver.InitializeAsync("{}", ct); + + var before = driver.GetHealth().LastSuccessfulRead; + var results = await driver.ReadAsync(["BadStr", "GoodWord"], ct); + + // ONE block read served both members — this really is the span path, not two per-tag reads. + factory.ReadBytesCalls.ShouldBe(1); + factory.StringReadCalls.ShouldBe(0); + + // Only the poisoned member is Bad; its block-mate keeps its value. + results[0].StatusCode.ShouldNotBe(0u); + results[0].Value.ShouldBeNull(); + results[1].StatusCode.ShouldBe(0u); + results[1].Value.ShouldNotBeNull(); + + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Degraded, "a span member that failed to decode must not be erased"); + health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(before!.Value); + } + + /// + /// Faulted outranks BOTH Degraded and Healthy: an access-denied read (the permanent PUT/GET + /// configuration fault) on one tag survives a later device-fault tag AND a later tag that + /// read successfully, all in the same pass. + /// + /// + /// The ordering is Healthy < Degraded < Faulted, so the config alert — the one state + /// an operator must act on — is precisely the one last-writer-wins was most likely to + /// erase, since the denied tag is usually a minority of the batch. Both later outcomes are + /// present deliberately: the success falsifies "any success clears the failure", the + /// Degraded falsifies "the last failure wins". LastSuccessfulRead still advances, + /// because one tag really was read. + /// + [Fact] + public async Task An_access_denied_tag_keeps_the_pass_Faulted_even_after_a_later_tag_succeeds() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ImageS7PlcFactory(Images()); + // A: "not allowed" is what IsAccessDenied matches — PUT/GET disabled on an S7-1200/1500, + // which is the Faulted config alert. B: an ordinary device-layer fault, merely Degraded, + // observed AFTER it. C: no fault at all. + factory.PerTagFaults["DB1.DBW0"] = new PlcException(ErrorCode.ReadData, "Accessing object not allowed"); + factory.PerTagFaults["DB1.DBW2"] = new PlcException(ErrorCode.ReadData, "S7 CPU reported a device fault"); + // maxGap -1 turns grouping off entirely, so every tag takes the per-tag read path and the + // faults can be aimed at individual addresses. + using var driver = new S7Driver(Options(NarrowTags(), maxGap: -1), "s7-access-denied", factory); + await driver.InitializeAsync("{}", ct); + + var before = driver.GetHealth().LastSuccessfulRead; + var results = await driver.ReadAsync(["A", "B", "C"], ct); + + results[0].StatusCode.ShouldNotBe(0u); + results[1].StatusCode.ShouldBe(StatusBadDeviceFailure); + results[2].StatusCode.ShouldBe(0u); + + var health = driver.GetHealth(); + health.State.ShouldBe( + DriverState.Faulted, "a config alert must outrank a later device fault AND a later success"); + health.LastError.ShouldNotBeNull(); + // The FIRST failure is the causal one — the later device fault does not overwrite it. + health.LastError!.ShouldContain("PUT/GET"); + health.LastSuccessfulRead!.Value.ShouldBeGreaterThan( + before!.Value, "one tag read fine, so the watchdog's evidence must still advance"); } /// @@ -611,6 +743,14 @@ public sealed class S7SpanReadParityTests /// When set, only the MULTI-VAR read throws this — the per-tag fallback still serves the image. public Exception? MultiVarFault { get; init; } + /// + /// Per-address read faults, keyed by the address string (e.g. DB1.DBW0): only + /// those addresses throw, every other one still serves the image. Lets a test put + /// faults of DIFFERENT severities and successes in ONE pass, which + /// (all-or-nothing) cannot express. + /// + public Dictionary PerTagFaults { get; } = new(StringComparer.OrdinalIgnoreCase); + /// /// When true, ReadBytesAsync hangs until its token is cancelled, so cancellation is /// the only way out of a block read (STAB-15a). Settable so a test can turn it back off @@ -745,6 +885,7 @@ public sealed class S7SpanReadParityTests public Task ReadAsync(string address, CancellationToken ct) { Interlocked.Increment(ref owner.StringReadCalls); + if (owner.PerTagFaults.TryGetValue(address, out var aimed)) throw aimed; if (owner.ReadFault is { } fault) throw fault; var addr = S7AddressParser.Parse(address); var image = owner.Images[(ToS7NetArea(addr.Area), addr.DbNumber)];