diff --git a/archreview/plans/R3-02-stale-good-family-plan.md b/archreview/plans/R3-02-stale-good-family-plan.md index 686d5020..d51c252d 100644 --- a/archreview/plans/R3-02-stale-good-family-plan.md +++ b/archreview/plans/R3-02-stale-good-family-plan.md @@ -580,3 +580,23 @@ T17 anytime; T18 after all; T19 last ``` Overall effort: **~2 dev-days code+unit, plus the rig session**. The three HIGH-RISK data-plane slices (T3 MQTT stale, T8 value-node Bad, T13/T16 hold-on-Bad) each land behind their full suites; the two recovery contract tests (T10, T16's identical-value case) must each be seen to FAIL once against a reverted dedup before merge — that is the whole reason this family re-appeared. + +--- + +## Execution notes + +Branch `fix/r3-02-stale-good-family` (worktree `.claude/worktrees/agent-a696a39981023fa0b`), off master +`72329eab`. Executed in plan block order T1, B (T2–T5), C (T6–T10), D (T12–T14), E (T15–T16), T17, T18. +**T11 and T19 are deliberately NOT in this branch** — T11 waits for the sibling R3-03 branch to merge +(it is the only edit that would touch `DriverInstanceActor` lifecycle alongside R3-03's restart work), +and T19 is the operator rig gate. + +Red-before observations are recorded per task below: what was run, and the failure text that proves the +test could only pass once the production change landed. + +### T1 — `InputQuality.WorstOf` + +- RED: `dotnet test tests/Core/…Core.Abstractions.Tests --filter "FullyQualifiedName~InputQualityTests"` + ⇒ **build failure**, 8 × `error CS0103: The name 'InputQuality' does not exist in the current context`. +- GREEN: 13 passed. `Core.ScriptedAlarms.Tests` 94/94 green with `WorstInputStatus` re-pointed (the + never-pushed/null skips stay in the alarm engine — they are alarm policy, not the shared rule). diff --git a/archreview/plans/R3-02-stale-good-family-plan.md.tasks.json b/archreview/plans/R3-02-stale-good-family-plan.md.tasks.json index 20d2dce8..24c53f7e 100644 --- a/archreview/plans/R3-02-stale-good-family-plan.md.tasks.json +++ b/archreview/plans/R3-02-stale-good-family-plan.md.tasks.json @@ -5,7 +5,7 @@ { "id": "T1", "subject": "InputQuality.WorstOf shared helper (Core.Abstractions) + truth-table tests; re-point ScriptedAlarmEngine.WorstInputStatus", - "status": "pending", + "status": "done", "blockedBy": [] }, { diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/InputQuality.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/InputQuality.cs new file mode 100644 index 00000000..df51b4b8 --- /dev/null +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/InputQuality.cs @@ -0,0 +1,74 @@ +namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +/// +/// The one worst-of-input rule every derived-value seam uses — scripted alarms, virtual tags and +/// the Calculation driver. +/// +/// The contract it implements (archreview R3-02, "the stale-Good family"): a value +/// consumer never reports a quality better than its source's. A node derived from driver values +/// carries, at worst, the worst status among its inputs — a Bad input makes the derived node Bad +/// with that input's sub-code, an Uncertain input makes it Uncertain, and a source that has +/// stopped communicating is a Bad input for everything downstream of it. +/// +/// Why it lives here. Core.Abstractions is the one assembly all three seams already +/// reference (Commons, Runtime and the driver assemblies all sit above it), and +/// — the shape the rule reads — is declared beside it. Stating +/// the rule three times is how the three seams drifted apart in the first place. +/// +/// Severity, not equality. Comparison is on the OPC UA Part 4 severity bits (the top +/// two: 00 Good, 01 Uncertain, 10/11 Bad); the sub-code below them is +/// carried through verbatim because the sub-code is the diagnostic +/// (BadNoCommunication = go look at the device, BadWaitingForInitialData = it never +/// arrived, BadInternalError = the script). +/// +public static class InputQuality +{ + /// + /// No value has ever been received for this input — the status a fresh node is materialised + /// with, and what reports for a dependency slot that holds no snapshot + /// at all. Spelled here rather than taken from Commons.OpcUa.OpcUaStatus because + /// Core.Abstractions sits below Commons and deliberately references nothing. + /// + public const uint BadWaitingForInitialData = 0x80320000u; + + /// + /// The worst status among : the highest Part 4 severity, carrying + /// that input's own sub-code. A entry is a dependency that has never + /// been pushed and reports — reporting Good for it is + /// the laundering this helper exists to stop. An empty set is Good: there is nothing to be + /// worse than. + /// + /// The dependency snapshots, in a stable order. + /// The worst status code observed. + /// + /// Among equal severity the first entry seen wins, so a re-evaluation over the same + /// inputs reports the same sub-code instead of oscillating between two equally-bad codes. + /// + public static uint WorstOf(IEnumerable inputs) + { + ArgumentNullException.ThrowIfNull(inputs); + + var worst = 0u; + var worstSeverity = 0u; + foreach (var input in inputs) + { + var status = input?.StatusCode ?? BadWaitingForInitialData; + var severity = status >> 30; + if (severity <= worstSeverity) continue; + worstSeverity = severity; + worst = status; + } + + return worst; + } + + /// + /// True when a consumer may evaluate against a value carrying . + /// Good and Uncertain are usable — an Uncertain value is present and a script can make a + /// qualified decision from it; Bad is not, and a consumer holds instead (the alarm engine's + /// long-standing policy, generalised). + /// + /// The status code to classify. + /// for Good or Uncertain severity. + public static bool IsUsable(uint statusCode) => (statusCode >> 30) < 2; +} diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs index 2acbb9cd..f8c8e07d 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs @@ -919,26 +919,18 @@ public sealed class ScriptedAlarmEngine : IDisposable /// the case the skip was hiding was the case that matters most. The upstream source now stamps a /// never-pushed path and nothing else does, so the two are tellable /// apart. A null snapshot is still skipped: that is a cache the refill did not populate, not an - /// observation. + /// observation. + /// The comparison itself lives in (archreview R3-02) + /// — the same rule the virtual-tag and Calculation seams now use, so "worst input wins, carrying its + /// sub-code, first-seen on a severity tie" is stated once. What stays here is the alarm-specific + /// filter: this engine's two skips are deliberately NOT part of the shared rule, because the + /// other two seams treat a never-pushed input as Bad rather than as absent. private static uint WorstInputStatus(IReadOnlyDictionary cache) { - uint worst = 0u; - var worstSeverity = 0u; - foreach (var kv in cache) - { - // No entry at all — nothing was observed, so there is nothing to report on. - if (kv.Value is null) continue; - // Observed as "no data has ever arrived" — the cold-start placeholder, not a fault. - if (kv.Value.StatusCode == StatusNeverPushed) continue; - var status = kv.Value.StatusCode; - var severity = status >> 30; - if (severity > worstSeverity) - { - worstSeverity = severity; - worst = status; - } - } - return worst; + // No entry at all — nothing was observed, so there is nothing to report on. + // Observed as "no data has ever arrived" — the cold-start placeholder, not a fault. + var observed = cache.Values.Where(v => v is not null && v.StatusCode != StatusNeverPushed); + return InputQuality.WorstOf(observed); } /// Update the tracked worst-quality bucket for an alarm; return true iff the 3-state bucket diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/InputQualityTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/InputQualityTests.cs new file mode 100644 index 00000000..117a144e --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/InputQualityTests.cs @@ -0,0 +1,97 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests; + +/// +/// Truth table for — the one worst-of-input rule every derived-value +/// seam uses (scripted alarms, virtual tags, the Calculation driver). The quality contract +/// (archreview R3-02) is "a value consumer never reports a quality better than its source's", +/// and this type is the single place that decides what "worse" means, so a divergence here is a +/// divergence in every seam at once. +/// +public sealed class InputQualityTests +{ + private const uint Good = 0x00000000u; + private const uint Uncertain = 0x40000000u; + private const uint UncertainLastUsableValue = 0x40900000u; + private const uint BadNoCommunication = 0x80310000u; + private const uint BadInternalError = 0x80020000u; + private const uint BadWaitingForInitialData = 0x80320000u; + + private static DataValueSnapshot Snap(object? value, uint status) => + new(value, status, SourceTimestampUtc: DateTime.UtcNow, ServerTimestampUtc: DateTime.UtcNow); + + /// An empty input set reports Good — there is nothing to be worse than. + [Fact] + public void WorstOf_no_inputs_is_Good() + { + InputQuality.WorstOf([]).ShouldBe(Good); + } + + /// All-Good inputs report Good. + [Fact] + public void WorstOf_all_good_is_Good() + { + InputQuality.WorstOf([Snap(1, Good), Snap(2, Good)]).ShouldBe(Good); + } + + /// A single Uncertain input carries its own sub-code out, not the generic Uncertain. + [Fact] + public void WorstOf_propagates_the_uncertain_sub_code() + { + InputQuality.WorstOf([Snap(1, Good), Snap(2, UncertainLastUsableValue)]) + .ShouldBe(UncertainLastUsableValue); + } + + /// Bad beats Uncertain regardless of the order the inputs are seen in. + [Theory] + [InlineData(true)] + [InlineData(false)] + public void WorstOf_bad_beats_uncertain(bool badFirst) + { + DataValueSnapshot?[] inputs = badFirst + ? [Snap(null, BadNoCommunication), Snap(2, Uncertain)] + : [Snap(2, Uncertain), Snap(null, BadNoCommunication)]; + + InputQuality.WorstOf(inputs).ShouldBe(BadNoCommunication); + } + + /// + /// A null entry means "this dependency has never been pushed", which is + /// BadWaitingForInitialData — the same code the materialiser stamps on a fresh node. + /// Reporting Good for it would be the exact laundering this helper exists to stop. + /// + [Fact] + public void WorstOf_a_null_entry_is_BadWaitingForInitialData() + { + InputQuality.WorstOf([Snap(1, Good), null]).ShouldBe(BadWaitingForInitialData); + } + + /// + /// Among equal severity the FIRST seen wins, so a re-evaluation over the same inputs reports + /// the same sub-code rather than oscillating between two equally-bad codes. + /// + [Fact] + public void WorstOf_equal_severity_keeps_the_first_seen() + { + InputQuality.WorstOf([Snap(null, BadNoCommunication), Snap(null, BadInternalError)]) + .ShouldBe(BadNoCommunication); + InputQuality.WorstOf([Snap(null, BadInternalError), Snap(null, BadNoCommunication)]) + .ShouldBe(BadInternalError); + } + + /// Good and Uncertain are usable (a script may still inspect the value); Bad is not. + [Theory] + [InlineData(Good, true)] + [InlineData(Uncertain, true)] + [InlineData(UncertainLastUsableValue, true)] + [InlineData(BadNoCommunication, false)] + [InlineData(BadWaitingForInitialData, false)] + [InlineData(0xC0000000u, false)] + public void IsUsable_follows_Part4_severity(uint statusCode, bool expected) + { + InputQuality.IsUsable(statusCode).ShouldBe(expected); + } +}