diff --git a/src/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs b/src/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs index 9fd71d5..7ab8721 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs +++ b/src/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs @@ -409,7 +409,19 @@ public sealed class ModbusDriver /// Cleared by ReinitializeAsync (operator restart) or by an explicit re-probe API /// (not yet shipped). /// - private readonly Dictionary<(byte Unit, ModbusRegion Region, ushort Start, ushort End), DateTime> _autoProhibited = new(); + /// + /// #150 — per-prohibition state. SplitPending drives the re-probe loop's + /// bisection: when true and the range spans > 1 register, the next re-probe + /// tries the two halves separately to narrow the actual offending register(s). + /// Single-register prohibitions can't be split further; they stay re-probed as-is. + /// + private sealed class ProhibitionState + { + public DateTime LastProbedUtc; + public bool SplitPending; + } + + private readonly Dictionary<(byte Unit, ModbusRegion Region, ushort Start, ushort End), ProhibitionState> _autoProhibited = new(); private readonly object _autoProhibitedLock = new(); private CancellationTokenSource? _reprobeCts; @@ -431,7 +443,16 @@ public sealed class ModbusDriver private void RecordAutoProhibition(byte unit, ModbusRegion region, ushort start, ushort end) { - lock (_autoProhibitedLock) _autoProhibited[(unit, region, start, end)] = DateTime.UtcNow; + lock (_autoProhibitedLock) + { + // Multi-register prohibitions enter the bisection workflow on the next re-probe; + // single-register prohibitions are already minimal and skip bisection. + _autoProhibited[(unit, region, start, end)] = new ProhibitionState + { + LastProbedUtc = DateTime.UtcNow, + SplitPending = end > start, + }; + } } /// Test/diagnostic accessor — returns the current auto-prohibited range count. @@ -441,80 +462,121 @@ public sealed class ModbusDriver } /// - /// #151 — periodic re-probe loop. Wakes every AutoProhibitReprobeInterval and - /// retries each auto-prohibited range with a one-shot coalesced read. Successful - /// re-probes drop the prohibition; failed ones leave it in place + bump the - /// last-probed timestamp so the next attempt waits another full interval. - /// Lives for the driver lifetime; cancelled by ShutdownAsync. + /// #151 — periodic re-probe loop, augmented in #150 with bisection-style narrowing. + /// Each tick processes every prohibition: split-pending multi-register ranges get + /// bisected (try left + right halves; replace with whichever halves still fail), + /// single-register or non-split-pending ranges get a straight re-probe. Lives for + /// the driver lifetime; cancelled by ShutdownAsync. /// private async Task ReprobeLoopAsync(CancellationToken ct) { var interval = _options.AutoProhibitReprobeInterval!.Value; - var transport = _transport; while (!ct.IsCancellationRequested) { try { await Task.Delay(interval, ct).ConfigureAwait(false); } catch (OperationCanceledException) { return; } - - if (transport is null) continue; - - // Snapshot the prohibition set so we can release the lock during the wire calls. - (byte Unit, ModbusRegion Region, ushort Start, ushort End)[] candidates; - lock (_autoProhibitedLock) - candidates = _autoProhibited.Keys.ToArray(); - - foreach (var p in candidates) - { - if (ct.IsCancellationRequested) return; - var fc = p.Region == ModbusRegion.HoldingRegisters ? (byte)0x03 : (byte)0x04; - var qty = (ushort)(p.End - p.Start + 1); - try - { - using var probeCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - probeCts.CancelAfter(_options.Timeout); - _ = await ReadRegisterBlockAsync(transport, p.Unit, fc, p.Start, qty, probeCts.Token).ConfigureAwait(false); - // Range is healthy now — drop the prohibition. Next data scan re-coalesces normally. - lock (_autoProhibitedLock) _autoProhibited.Remove(p); - } - catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; } - catch - { - // Still bad. Bump the timestamp so it shows up on diagnostics as recently - // re-probed — the prohibition stays in place. - lock (_autoProhibitedLock) - { - if (_autoProhibited.ContainsKey(p)) - _autoProhibited[p] = DateTime.UtcNow; - } - } - } + try { await RunReprobeOnceForTestAsync(ct).ConfigureAwait(false); } + catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; } } } - /// Test/diagnostic accessor — fires one re-probe pass synchronously for tests. + /// + /// One re-probe pass. Public-but-internal so tests can drive it synchronously rather + /// than wait on the background timer. Iterates a snapshot of the prohibition set; for + /// each entry decides between bisection (multi-register + SplitPending) or straight + /// retry (single-register or already-narrowed). + /// internal async Task RunReprobeOnceForTestAsync(CancellationToken ct) { var transport = _transport ?? throw new InvalidOperationException("Transport not connected"); - (byte Unit, ModbusRegion Region, ushort Start, ushort End)[] candidates; - lock (_autoProhibitedLock) candidates = _autoProhibited.Keys.ToArray(); - foreach (var p in candidates) + + ((byte Unit, ModbusRegion Region, ushort Start, ushort End) Key, bool SplitPending)[] candidates; + lock (_autoProhibitedLock) + candidates = _autoProhibited + .Select(kv => (Key: kv.Key, SplitPending: kv.Value.SplitPending)) + .ToArray(); + + foreach (var (key, splitPending) in candidates) { - var fc = p.Region == ModbusRegion.HoldingRegisters ? (byte)0x03 : (byte)0x04; - var qty = (ushort)(p.End - p.Start + 1); - try - { - _ = await ReadRegisterBlockAsync(transport, p.Unit, fc, p.Start, qty, ct).ConfigureAwait(false); - lock (_autoProhibitedLock) _autoProhibited.Remove(p); - } - catch - { - lock (_autoProhibitedLock) - if (_autoProhibited.ContainsKey(p)) - _autoProhibited[p] = DateTime.UtcNow; - } + if (ct.IsCancellationRequested) return; + if (splitPending && key.End > key.Start) + await BisectAndReprobeAsync(transport, key, ct).ConfigureAwait(false); + else + await StraightReprobeAsync(transport, key, ct).ConfigureAwait(false); } } + private async Task StraightReprobeAsync(IModbusTransport transport, + (byte Unit, ModbusRegion Region, ushort Start, ushort End) key, CancellationToken ct) + { + var fc = key.Region == ModbusRegion.HoldingRegisters ? (byte)0x03 : (byte)0x04; + var qty = (ushort)(key.End - key.Start + 1); + try + { + _ = await ReadRegisterBlockAsync(transport, key.Unit, fc, key.Start, qty, ct).ConfigureAwait(false); + lock (_autoProhibitedLock) _autoProhibited.Remove(key); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } + catch + { + lock (_autoProhibitedLock) + if (_autoProhibited.TryGetValue(key, out var st)) st.LastProbedUtc = DateTime.UtcNow; + } + } + + /// + /// #150 — bisect a multi-register prohibition. Removes the parent entry and re-adds + /// whichever halves still fail. Over multiple re-probe ticks the prohibition narrows + /// log2(span) times until it pinpoints the actual protected register(s). + /// + private async Task BisectAndReprobeAsync(IModbusTransport transport, + (byte Unit, ModbusRegion Region, ushort Start, ushort End) key, CancellationToken ct) + { + var fc = key.Region == ModbusRegion.HoldingRegisters ? (byte)0x03 : (byte)0x04; + var mid = (ushort)((key.Start + key.End) / 2); + var leftEnd = mid; + var rightStart = (ushort)(mid + 1); + + var leftFailed = await ProbeFailsAsync(transport, fc, key.Unit, key.Start, leftEnd, ct).ConfigureAwait(false); + var rightFailed = await ProbeFailsAsync(transport, fc, key.Unit, rightStart, key.End, ct).ConfigureAwait(false); + + lock (_autoProhibitedLock) + { + _autoProhibited.Remove(key); + if (leftFailed) + { + _autoProhibited[(key.Unit, key.Region, key.Start, leftEnd)] = new ProhibitionState + { + LastProbedUtc = DateTime.UtcNow, + SplitPending = leftEnd > key.Start, + }; + } + if (rightFailed) + { + _autoProhibited[(key.Unit, key.Region, rightStart, key.End)] = new ProhibitionState + { + LastProbedUtc = DateTime.UtcNow, + SplitPending = key.End > rightStart, + }; + } + // Both halves succeeded → entry is just removed. The parent prohibition is gone + // and the next normal scan can re-coalesce across the whole original range. + } + } + + private async Task ProbeFailsAsync(IModbusTransport transport, byte fc, byte unit, + ushort start, ushort end, CancellationToken ct) + { + var qty = (ushort)(end - start + 1); + try + { + _ = await ReadRegisterBlockAsync(transport, unit, fc, start, qty, ct).ConfigureAwait(false); + return false; + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } + catch { return true; } + } + /// /// #143 block-read coalescing planner. Groups eligible tags by (UnitId, Region), sorts /// by start address, and merges adjacent / near-adjacent (gap ≤ MaxReadGap) into single diff --git a/tests/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingBisectionTests.cs b/tests/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingBisectionTests.cs new file mode 100644 index 0000000..7eb21c6 --- /dev/null +++ b/tests/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingBisectionTests.cs @@ -0,0 +1,171 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// #150 — bisection-style range narrowing for coalescing prohibitions. After a coalesced +/// read fails, the re-probe loop bisects the prohibited range over multiple ticks until +/// it pinpoints the actual protected register(s). Healthy halves get cleared as the +/// bisection narrows. +/// +[Trait("Category", "Unit")] +public sealed class ModbusCoalescingBisectionTests +{ + /// + /// Programmable transport like the one in ModbusCoalescingAutoRecoveryTests but local + /// to keep this test file standalone — having the protection model live next to the + /// bisection assertions makes the test intent easier to read. + /// + private sealed class ProtectedHoleTransport : IModbusTransport + { + public ushort ProtectedAddress { get; set; } = ushort.MaxValue; + public Task ConnectAsync(CancellationToken ct) => Task.CompletedTask; + public Task SendAsync(byte unitId, byte[] pdu, CancellationToken ct) + { + var addr = (ushort)((pdu[1] << 8) | pdu[2]); + var qty = (ushort)((pdu[3] << 8) | pdu[4]); + if (pdu[0] is 0x03 or 0x04 && ProtectedAddress >= addr && ProtectedAddress < addr + qty) + return Task.FromException(new ModbusException(pdu[0], 0x02, "IllegalDataAddress")); + switch (pdu[0]) + { + case 0x03: case 0x04: + { + var resp = new byte[2 + qty * 2]; + resp[0] = pdu[0]; resp[1] = (byte)(qty * 2); + return Task.FromResult(resp); + } + default: return Task.FromResult(new byte[] { pdu[0], 0, 0 }); + } + } + public ValueTask DisposeAsync() => ValueTask.CompletedTask; + } + + [Fact] + public async Task Bisection_Narrows_Multi_Register_Prohibition_Per_Reprobe() + { + var fake = new ProtectedHoleTransport { ProtectedAddress = 105 }; + // 11 tags 100..110 with MaxReadGap=10 → coalesce into one block 100..110. The protected + // register is in the middle (105). After the first failure the planner records the + // full 100..110 range as split-pending. Each subsequent re-probe bisects until the + // prohibition is pinned at register 105. + var tags = Enumerable.Range(100, 11) + .Select(i => new ModbusTagDefinition($"T{i}", ModbusRegion.HoldingRegisters, (ushort)i, ModbusDataType.Int16)) + .ToArray(); + var opts = new ModbusDriverOptions { Host = "f", Tags = tags, MaxReadGap = 10, + AutoProhibitReprobeInterval = TimeSpan.FromMilliseconds(100), + Probe = new ModbusProbeOptions { Enabled = false } }; + var drv = new ModbusDriver(opts, "m1", _ => fake); + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.ReadAsync(tags.Select(t => t.Name).ToArray(), CancellationToken.None); + // Initial prohibition: full 100..110 range, split-pending. + drv.AutoProhibitedRangeCount.ShouldBe(1); + + // Re-probe pass 1: bisect 100..110 → mid=105 → left=100..105 (fails because 105 is + // protected), right=106..110 (succeeds). Result: prohibition collapses to 100..105. + await drv.RunReprobeOnceForTestAsync(CancellationToken.None); + drv.AutoProhibitedRangeCount.ShouldBe(1, "after pass 1 the prohibition narrows but doesn't disappear"); + + // Re-probe pass 2: bisect 100..105 → mid=102 → left=100..102 (succeeds), right=103..105 (fails). + // Result: prohibition collapses to 103..105. + await drv.RunReprobeOnceForTestAsync(CancellationToken.None); + + // Re-probe pass 3: bisect 103..105 → mid=104 → left=103..104 (succeeds), right=105..105 (fails). + // Result: prohibition collapses to 105..105 (single register, no longer split-pending). + await drv.RunReprobeOnceForTestAsync(CancellationToken.None); + drv.AutoProhibitedRangeCount.ShouldBe(1, "single-register prohibition stays after bisection terminates"); + + // Re-probe pass 4: 105..105 is single-register; straight-retry path. Still fails; + // prohibition stays. + await drv.RunReprobeOnceForTestAsync(CancellationToken.None); + drv.AutoProhibitedRangeCount.ShouldBe(1); + + await drv.ShutdownAsync(CancellationToken.None); + } + + [Fact] + public async Task Bisection_Clears_When_Both_Halves_Are_Healthy() + { + // Transient failure scenario: range failed once, but by the next re-probe the PLC has + // unlocked it. Bisection of (100..110) returns success on both halves → entry removed + // entirely. + var fake = new ProtectedHoleTransport { ProtectedAddress = 105 }; + var tags = Enumerable.Range(100, 11) + .Select(i => new ModbusTagDefinition($"T{i}", ModbusRegion.HoldingRegisters, (ushort)i, ModbusDataType.Int16)) + .ToArray(); + var opts = new ModbusDriverOptions { Host = "f", Tags = tags, MaxReadGap = 10, + AutoProhibitReprobeInterval = TimeSpan.FromMilliseconds(100), + Probe = new ModbusProbeOptions { Enabled = false } }; + var drv = new ModbusDriver(opts, "m1", _ => fake); + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.ReadAsync(tags.Select(t => t.Name).ToArray(), CancellationToken.None); + drv.AutoProhibitedRangeCount.ShouldBe(1); + + // Operator unlocks the protected register before the re-probe runs. + fake.ProtectedAddress = ushort.MaxValue; + + await drv.RunReprobeOnceForTestAsync(CancellationToken.None); + drv.AutoProhibitedRangeCount.ShouldBe(0, "both bisected halves succeed → parent prohibition cleared entirely"); + + await drv.ShutdownAsync(CancellationToken.None); + } + + [Fact] + public async Task Bisection_Splits_Into_Two_When_Both_Halves_Still_Fail() + { + // Two protected registers in the same coalesced range: 102 and 108. After bisection, + // both halves of the original (100..110) range still contain a protected address + // (left=100..105 contains 102, right=106..110 contains 108). The prohibition replaces + // the parent with TWO smaller split-pending entries. + var fake = new ProtectedHoleTransport(); + // Build a more elaborate transport that protects two addresses. + var twoHole = new TwoHoleTransport { ProtectedAddresses = { 102, 108 } }; + var tags = Enumerable.Range(100, 11) + .Select(i => new ModbusTagDefinition($"T{i}", ModbusRegion.HoldingRegisters, (ushort)i, ModbusDataType.Int16)) + .ToArray(); + var opts = new ModbusDriverOptions { Host = "f", Tags = tags, MaxReadGap = 10, + AutoProhibitReprobeInterval = TimeSpan.FromMilliseconds(100), + Probe = new ModbusProbeOptions { Enabled = false } }; + var drv = new ModbusDriver(opts, "m1", _ => twoHole); + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.ReadAsync(tags.Select(t => t.Name).ToArray(), CancellationToken.None); + drv.AutoProhibitedRangeCount.ShouldBe(1); + + // Re-probe: bisect 100..110 at mid=105 → left=100..105 (contains 102, fails), + // right=106..110 (contains 108, fails). Result: TWO entries in place of the parent. + await drv.RunReprobeOnceForTestAsync(CancellationToken.None); + drv.AutoProhibitedRangeCount.ShouldBe(2, "both halves still fail → prohibition splits into two"); + + await drv.ShutdownAsync(CancellationToken.None); + } + + private sealed class TwoHoleTransport : IModbusTransport + { + public readonly HashSet ProtectedAddresses = new(); + public Task ConnectAsync(CancellationToken ct) => Task.CompletedTask; + public Task SendAsync(byte unitId, byte[] pdu, CancellationToken ct) + { + var addr = (ushort)((pdu[1] << 8) | pdu[2]); + var qty = (ushort)((pdu[3] << 8) | pdu[4]); + if (pdu[0] is 0x03 or 0x04) + for (var i = 0; i < qty; i++) + if (ProtectedAddresses.Contains((ushort)(addr + i))) + return Task.FromException(new ModbusException(pdu[0], 0x02, "IllegalDataAddress")); + switch (pdu[0]) + { + case 0x03: case 0x04: + { + var resp = new byte[2 + qty * 2]; + resp[0] = pdu[0]; resp[1] = (byte)(qty * 2); + return Task.FromResult(resp); + } + default: return Task.FromResult(new byte[] { pdu[0], 0, 0 }); + } + } + public ValueTask DisposeAsync() => ValueTask.CompletedTask; + } +}