From ae20519e9d676c9ca61ecd34eeb5811cc79c34c2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:05:25 -0400 Subject: [PATCH] fix(modbus): deadband baseline on (value, status) so the Good recovery publishes H2: a Bad/null publish drops the deadband baseline and any status change publishes, so a device that returns holding the identical value is no longer swallowed by |new - old| < deadband (the #527 value-only-dedup trap). Integration case drives a real drop/restore through a loopback relay. Refs #741 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../ModbusDriver.cs | 44 ++++++++++----- .../CommsRecoveryTests.cs | 43 +++++++++++++++ .../ModbusDeadbandRecoveryTests.cs | 53 +++++++++++++++++++ 3 files changed, 128 insertions(+), 12 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusDeadbandRecoveryTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs index 19e90946..fc85ddde 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs @@ -49,10 +49,10 @@ public sealed class ModbusDriver // authored _tagsByRawPath table (v3: the reference is always a RawPath; a miss is a miss). private readonly EquipmentTagRefResolver _resolver; - // Last-published value per tag, keyed by FullReference. Used by ShouldPublish to apply - // the deadband filter. Stored as object so all numeric types share one map; the comparison - // does a typed cast inside. - private readonly ConcurrentDictionary _lastPublishedByRef = new(StringComparer.OrdinalIgnoreCase); + // Last-published (value, status) per tag, keyed by FullReference. Used by ShouldPublish to apply + // the deadband filter. The value is stored as object so all numeric types share one map; the + // comparison does a typed cast inside. The status is part of the baseline — see ShouldPublish. + private readonly ConcurrentDictionary _lastPublishedByRef = new(StringComparer.OrdinalIgnoreCase); // Per-cycle buffer of the items this driver actually PUBLISHED (i.e. that passed ShouldPublish), // keyed by subscription handle. The poll engine collects its own batch list unconditionally — it @@ -233,31 +233,51 @@ public sealed class ModbusDriver /// Format a per-slave host string. Multi-slave deployments distinguish breakers by this string. private string BuildSlaveHostName(byte unitId) => $"{_options.Host}:{_options.Port}/unit{unitId}"; - private bool ShouldPublish(string tagRef, DataValueSnapshot snapshot) + /// + /// Deadband filter for the subscribe path. The baseline is keyed on (value, status), never + /// value alone: a Bad/null publish drops the baseline, and a status change always publishes. A + /// device that comes back after a comms drop usually comes back holding the SAME reading, so the + /// old value-only comparison (|10 − 10| < deadband) swallowed the Good recovery and left + /// the node Bad until the value drifted past the deadband — the #527 trap. + /// + /// The subscribed reference. + /// The snapshot the poll engine is about to raise. + /// when the snapshot must be published. + internal bool ShouldPublish(string tagRef, DataValueSnapshot snapshot) { if (!_resolver.TryResolve(tagRef, out var tag) || tag.Deadband is null) return true; - if (snapshot.Value is null) return true; // Deadband only applies to numeric scalar types — array / Bool / String publishes // unconditionally. Easier to special-case skip than to enumerate the supported types. if (tag.ArrayCount.HasValue || tag.DataType is ModbusDataType.Bool or ModbusDataType.BitInRegister or ModbusDataType.String) return true; - if (!_lastPublishedByRef.TryGetValue(tagRef, out var prev)) + if (snapshot.Value is null || StatusCodeIsBad(snapshot.StatusCode)) { - // First sample passes through unconditionally — the threshold can't be evaluated - // without a baseline. The publish lands and seeds the comparison. - _lastPublishedByRef[tagRef] = snapshot.Value; + // A Bad publish always passes AND drops the baseline, so the next Good sample is + // compared against nothing and publishes — whatever value it carries. + _lastPublishedByRef.TryRemove(tagRef, out _); + return true; + } + + if (!_lastPublishedByRef.TryGetValue(tagRef, out var prev) || prev.Status != snapshot.StatusCode) + { + // First sample, or a quality transition (Good ↔ Uncertain): passes unconditionally and + // (re)seeds the comparison. The threshold cannot be evaluated across a status change. + _lastPublishedByRef[tagRef] = (snapshot.Value, snapshot.StatusCode); return true; } var newD = Convert.ToDouble(snapshot.Value); - var oldD = Convert.ToDouble(prev); + var oldD = Convert.ToDouble(prev.Value); if (Math.Abs(newD - oldD) < tag.Deadband.Value) return false; - _lastPublishedByRef[tagRef] = snapshot.Value; + _lastPublishedByRef[tagRef] = (snapshot.Value, snapshot.StatusCode); return true; } + /// OPC UA severity bits 31..30 = 10 ⇒ Bad. + private static bool StatusCodeIsBad(uint statusCode) => (statusCode & 0xC0000000u) == 0x80000000u; + /// public string DriverInstanceId => _driverInstanceId; /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/CommsRecoveryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/CommsRecoveryTests.cs index 37192239..857ce3b7 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/CommsRecoveryTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/CommsRecoveryTests.cs @@ -85,4 +85,47 @@ public sealed class CommsRecoveryTests(ModbusSimulatorFixture sim) recovered[0].Value.ShouldBe((ushort)5); driver.GetHealth().State.ShouldBe(DriverState.Healthy); } + + /// + /// H2: a deadbanded tag whose device returns holding the SAME value must publish the Good recovery. + /// HR[5] is a constant 5 in the standard profile, so the recovery value is identical by construction. + /// Positive control: the Bad publish during the outage is observed first. + /// + [Fact] + public async Task Deadbanded_subscription_publishes_the_Good_recovery_with_an_identical_value() + { + SkipUnlessStandardLive(); + await using var relay = new TcpRelay(sim.Host, sim.Port); + var tag = new ModbusTagDefinition("HR5", ModbusRegion.HoldingRegisters, 5, ModbusDataType.UInt16, + Writable: false, Deadband: 10.0); + await using var driver = new ModbusDriver(Options(relay, tag), "modbus-h2"); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var published = new System.Collections.Concurrent.ConcurrentQueue(); + driver.OnDataChange += (_, e) => published.Enqueue(e.Snapshot); + var handle = await driver.SubscribeAsync(["HR5"], TimeSpan.FromMilliseconds(200), TestContext.Current.CancellationToken); + try + { + (await WaitFor(() => Task.FromResult(published.ToArray()), + p => p.Any(s => s.StatusCode == StatusGood), TimeSpan.FromSeconds(10))) + .ShouldContain(s => s.StatusCode == StatusGood, "initial Good publish"); + + relay.Cut(); + (await WaitFor(() => Task.FromResult(published.ToArray()), + p => p.Any(s => s.StatusCode != StatusGood), TimeSpan.FromSeconds(20))) + .ShouldContain(s => s.StatusCode != StatusGood, "the outage must publish Bad (positive control)"); + + var badCount = published.Count; + relay.Restore(); + var after = await WaitFor(() => Task.FromResult(published.Skip(badCount).ToArray()), + p => p.Any(s => s.StatusCode == StatusGood), TimeSpan.FromSeconds(30)); + var recovery = after.FirstOrDefault(s => s.StatusCode == StatusGood); + recovery.ShouldNotBeNull("the Good recovery with the same value must not be swallowed by the deadband"); + recovery.Value.ShouldBe((ushort)5); + } + finally + { + await driver.UnsubscribeAsync(handle, TestContext.Current.CancellationToken); + } + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusDeadbandRecoveryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusDeadbandRecoveryTests.cs new file mode 100644 index 00000000..43275c15 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusDeadbandRecoveryTests.cs @@ -0,0 +1,53 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// Review 07 H2: the deadband baseline is keyed on (value, status). A Good-after-Bad recovery carrying +/// the IDENTICAL value must publish — the value-only dedup left the node Bad until the value drifted. +/// +[Trait("Category", "Unit")] +public sealed class ModbusDeadbandRecoveryTests +{ + private const uint Good = 0u; + private const uint Uncertain = 0x40000000u; + private const uint BadComm = 0x80050000u; + + private static async Task NewDriverAsync() + { + var tag = new ModbusTagDefinition("Temp", ModbusRegion.HoldingRegisters, 0, ModbusDataType.Int16, Deadband: 5.0); + var opts = new ModbusDriverOptions + { + Host = "f", RawTags = ModbusRawTags.Entries([tag]), Probe = new ModbusProbeOptions { Enabled = false }, + }; + var drv = new ModbusDriver(opts, "m1", _ => new ModbusDriverTests.FakeTransport()); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + return drv; + } + + private static DataValueSnapshot Snap(object? v, uint status) => new(v, status, DateTime.UtcNow, DateTime.UtcNow); + + [Fact] + public async Task Good_after_Bad_with_the_same_value_publishes() + { + await using var drv = await NewDriverAsync(); + drv.ShouldPublish("Temp", Snap((short)10, Good)).ShouldBeTrue(); // baseline + drv.ShouldPublish("Temp", Snap(null, BadComm)).ShouldBeTrue(); // comms drop + drv.ShouldPublish("Temp", Snap((short)10, Good)).ShouldBeTrue("the recovery publish must not be swallowed"); + // Positive control: the deadband is still active after the recovery re-seeded the baseline. + drv.ShouldPublish("Temp", Snap((short)11, Good)).ShouldBeFalse(); + drv.ShouldPublish("Temp", Snap((short)16, Good)).ShouldBeTrue(); + } + + [Fact] + public async Task A_status_change_with_the_same_value_publishes() + { + await using var drv = await NewDriverAsync(); + drv.ShouldPublish("Temp", Snap((short)10, Good)).ShouldBeTrue(); + drv.ShouldPublish("Temp", Snap((short)10, Uncertain)).ShouldBeTrue("a quality change must publish"); + drv.ShouldPublish("Temp", Snap((short)12, Uncertain)).ShouldBeFalse("within deadband at the same quality"); + drv.ShouldPublish("Temp", Snap((short)10, Good)).ShouldBeTrue("back to Good must publish"); + } +}