diff --git a/code-reviews/Driver.Galaxy/findings.md b/code-reviews/Driver.Galaxy/findings.md index 6598492..a964ca7 100644 --- a/code-reviews/Driver.Galaxy/findings.md +++ b/code-reviews/Driver.Galaxy/findings.md @@ -78,13 +78,13 @@ | Severity | Medium | | Category | Correctness & logic bugs | | Location | `GalaxyDriver.cs:901` | -| Status | Open | +| Status | Resolved | **Description:** `OnPumpDataChange` reconstructs a raw OPC DA quality byte from an OPC UA `StatusCode` for the probe watcher: it shifts `StatusCode >> 30` and maps `0->192, 1->64, _->0`. The `StatusCode` was itself produced upstream by `StatusCodeMap.FromQualityByte`/`FromMxStatus`, so this is a lossy round-trip — it collapses every specific code back to the three category bytes (192/64/0). That happens to satisfy `PerPlatformProbeWatcher.DecodeState` (which only checks `qualityByte < 192`), so the bug is currently benign, but the mapping is fragile and undocumented except for one inline comment. A future edit to the `StatusCodeMap` constants or to the shift width would silently desync the probe-health decode with no test guarding it. **Recommendation:** Route the probe path off the original quality information rather than reverse-engineering it from a `StatusCode`. Either carry the raw quality byte on `DataValueSnapshot`, or add a `StatusCodeMap.ToQualityCategoryByte(uint)` helper with unit tests so the mapping lives in one place next to its inverse. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `StatusCodeMap.ToQualityCategoryByte(uint)` helper that extracts top-two bits of the OPC UA StatusCode into the OPC DA category byte (Good=192, Uncertain=64, Bad=0); `GalaxyDriver.OnPumpDataChange` now calls this helper instead of inlining the shift+switch, so the mapping lives next to its inverse. Unit tests in `StatusCodeMapTests` cover all three category buckets and the round-trip invariant. ### Driver.Galaxy-005 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs index 9403426..f687b03 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs @@ -1010,15 +1010,11 @@ public sealed class GalaxyDriver if (_probeWatcher is not null && args.FullReference.EndsWith(PerPlatformProbeWatcher.ProbeSuffix, StringComparison.OrdinalIgnoreCase)) { - // The probe decoder takes a raw quality byte; recover it from the StatusCode - // top byte (Good=0x00 → byte 192, Uncertain=0x40 → byte 64, Bad=0x80 → byte 0). - var qualityByte = (byte)((args.Snapshot.StatusCode >> 30) & 0x3) switch - { - 0 => 192, - 1 => 64, - _ => 0, - }; - _probeWatcher.OnProbeValueChanged(args.FullReference, args.Snapshot.Value, (byte)qualityByte); + // The probe decoder takes a raw quality byte. Recover it via the canonical + // StatusCodeMap.ToQualityCategoryByte helper so the mapping lives in one + // place next to its inverse (FromQualityByte) and cannot desync silently. + var qualityByte = StatusCodeMap.ToQualityCategoryByte(args.Snapshot.StatusCode); + _probeWatcher.OnProbeValueChanged(args.FullReference, args.Snapshot.Value, qualityByte); } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs index 0b4058b..914a623 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs @@ -101,6 +101,25 @@ internal static class StatusCodeMap return Bad; } + /// + /// Convert an OPC UA uint back to the OPC DA quality category + /// byte — Good=192, Uncertain=64, Bad=0 — by extracting the top-two bits of the + /// high word. This is the inverse of the category-bucket arm of + /// . It is intentionally lossy (substatus bits are not + /// round-tripped) because the sole consumer + /// () + /// only tests qualityByte < 192 to distinguish Running from Stopped. Keeping + /// the round-trip in one place means a future change to the OPC UA bit layout cannot + /// silently desync the probe-health decode. + /// + public static byte ToQualityCategoryByte(uint statusCode) => + (byte)(((statusCode >> 30) & 0x3u) switch + { + 0u => 192u, // Good — top two bits 00b → OPC DA 0xC0 + 1u => 64u, // Uncertain — top two bits 01b → OPC DA 0x40 + _ => 0u, // Bad — top two bits 10b/11b → OPC DA 0x00 + }); + private static uint Categorize(byte q, ILogger? logger) { if (q >= 192) { Log(logger, q, "Good"); return Good; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/StatusCodeMapTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/StatusCodeMapTests.cs index e446273..6d44f45 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/StatusCodeMapTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/StatusCodeMapTests.cs @@ -112,4 +112,29 @@ public sealed class StatusCodeMapTests ((StatusCodeMap.BadNotConnected >> 30) & 0x3u).ShouldBe(2u); ((StatusCodeMap.BadOutOfService >> 30) & 0x3u).ShouldBe(2u); } + + // ===== Driver.Galaxy-004 regression: ToQualityCategoryByte lives next to its inverse ===== + + [Theory] + [InlineData(0x00000000u, (byte)192)] // Good + [InlineData(0x00D80000u, (byte)192)] // GoodLocalOverride — still Good category + [InlineData(0x40000000u, (byte)64)] // Uncertain + [InlineData(0x408F0000u, (byte)64)] // UncertainSubNormal — still Uncertain category + [InlineData(0x80000000u, (byte)0)] // Bad + [InlineData(0x808A0000u, (byte)0)] // BadNotConnected — still Bad category + [InlineData(0x80020000u, (byte)0)] // BadInternalError — still Bad category + public void ToQualityCategoryByte_ExtractsTopTwoBitsAsOpcDaByte(uint statusCode, byte expected) + { + StatusCodeMap.ToQualityCategoryByte(statusCode).ShouldBe(expected); + } + + [Fact] + public void ToQualityCategoryByte_IsRightInverseOfFromQualityByte_ForCategoryBytes() + { + // The category bytes the probe watcher uses are 0, 64, 192. Round-trip: a value + // that came FROM those bytes should map back to the same byte. + StatusCodeMap.ToQualityCategoryByte(StatusCodeMap.FromQualityByte(0)).ShouldBe((byte)0); + StatusCodeMap.ToQualityCategoryByte(StatusCodeMap.FromQualityByte(64)).ShouldBe((byte)64); + StatusCodeMap.ToQualityCategoryByte(StatusCodeMap.FromQualityByte(192)).ShouldBe((byte)192); + } }