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
This commit is contained in:
Joseph Doherty
2026-10-22 15:05:25 -04:00
parent 530f1b2f0b
commit ae20519e9d
3 changed files with 128 additions and 12 deletions
@@ -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<ModbusTagDefinition> _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<string, object> _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<string, (object Value, uint Status)> _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
/// <summary>Format a per-slave host string. Multi-slave deployments distinguish breakers by this string.</summary>
private string BuildSlaveHostName(byte unitId) => $"{_options.Host}:{_options.Port}/unit{unitId}";
private bool ShouldPublish(string tagRef, DataValueSnapshot snapshot)
/// <summary>
/// Deadband filter for the subscribe path. The baseline is keyed on <b>(value, status)</b>, 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 (<c>|10 − 10| &lt; deadband</c>) swallowed the Good recovery and left
/// the node Bad until the value drifted past the deadband — the #527 trap.
/// </summary>
/// <param name="tagRef">The subscribed reference.</param>
/// <param name="snapshot">The snapshot the poll engine is about to raise.</param>
/// <returns><see langword="true"/> when the snapshot must be published.</returns>
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;
}
/// <summary>OPC UA severity bits 31..30 = 10 ⇒ Bad.</summary>
private static bool StatusCodeIsBad(uint statusCode) => (statusCode & 0xC0000000u) == 0x80000000u;
/// <inheritdoc />
public string DriverInstanceId => _driverInstanceId;
/// <inheritdoc />
@@ -85,4 +85,47 @@ public sealed class CommsRecoveryTests(ModbusSimulatorFixture sim)
recovered[0].Value.ShouldBe((ushort)5);
driver.GetHealth().State.ShouldBe(DriverState.Healthy);
}
/// <summary>
/// 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.
/// </summary>
[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<DataValueSnapshot>();
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);
}
}
}
@@ -0,0 +1,53 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests;
/// <summary>
/// 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.
/// </summary>
[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<ModbusDriver> 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");
}
}