From e7217fdc81ba7e6a70ffe6cae23c26cb090ad56c Mon Sep 17 00:00:00 2001 From: Jakob Date: Fri, 5 Jun 2026 23:36:32 +0200 Subject: [PATCH 1/4] Add safe retrying, deadline-bounded CV read/write methods --- .../CvOperationTimeoutException.cs | 24 ++ .../CvShortCircuitException.cs | 20 ++ .../IProgrammingControl.cs | 15 ++ .../Core/SafeCvProgrammingTest.cs | 215 ++++++++++++++++++ src/Z21.Client/Core/IZ21CommandStation.cs | 17 ++ src/Z21.Client/Core/Z21CommandStation.cs | 156 +++++++++++++ 6 files changed, 447 insertions(+) create mode 100644 src/CommandStation.Abstractions/CvOperationTimeoutException.cs create mode 100644 src/CommandStation.Abstractions/CvShortCircuitException.cs create mode 100644 src/Z21.Client.UnitTest/Core/SafeCvProgrammingTest.cs diff --git a/src/CommandStation.Abstractions/CvOperationTimeoutException.cs b/src/CommandStation.Abstractions/CvOperationTimeoutException.cs new file mode 100644 index 0000000..e6ec56f --- /dev/null +++ b/src/CommandStation.Abstractions/CvOperationTimeoutException.cs @@ -0,0 +1,24 @@ +using System; + +namespace CommandStation +{ + /// + /// Thrown when a safe CV programming operation does not complete within the caller-supplied timeout + /// (for example, the decoder never acknowledges so the operation keeps retrying until the deadline). + /// + public sealed class CvOperationTimeoutException : Exception + { + /// The 0-based CV address the operation targeted (0 = CV1). + public ushort CvAddress { get; } + + /// The timeout that elapsed before a result was received. + public TimeSpan Timeout { get; } + + public CvOperationTimeoutException(ushort cvAddress, TimeSpan timeout) + : base($"CV {cvAddress + 1} did not complete within {timeout.TotalSeconds:0.###}s.") + { + CvAddress = cvAddress; + Timeout = timeout; + } + } +} diff --git a/src/CommandStation.Abstractions/CvShortCircuitException.cs b/src/CommandStation.Abstractions/CvShortCircuitException.cs new file mode 100644 index 0000000..1922e8e --- /dev/null +++ b/src/CommandStation.Abstractions/CvShortCircuitException.cs @@ -0,0 +1,20 @@ +using System; + +namespace CommandStation +{ + /// + /// Thrown when a CV programming operation is aborted because the command station reported a short + /// circuit on the track. The operation is not retried. + /// + public sealed class CvShortCircuitException : Exception + { + /// The 0-based CV address the operation targeted (0 = CV1). + public ushort CvAddress { get; } + + public CvShortCircuitException(ushort cvAddress) + : base($"CV programming aborted: short circuit on the track (CV {cvAddress + 1}).") + { + CvAddress = cvAddress; + } + } +} diff --git a/src/CommandStation.Abstractions/IProgrammingControl.cs b/src/CommandStation.Abstractions/IProgrammingControl.cs index 9b85896..96ec3ff 100644 --- a/src/CommandStation.Abstractions/IProgrammingControl.cs +++ b/src/CommandStation.Abstractions/IProgrammingControl.cs @@ -14,6 +14,21 @@ public interface IProgrammingControl Task WriteCvAsync(ushort cvAddress, byte value); + /// + /// Reads a CV on the programming track, retrying while the decoder does not acknowledge, and + /// returns the value. Throws if no result arrives within + /// , or on a short circuit. + /// + Task ReadCvAsync(ushort cvAddress, TimeSpan timeout); + + /// + /// Writes a CV on the programming track, retrying while the decoder does not acknowledge, and + /// completes once the command station confirms the write. Throws + /// if not confirmed within , or + /// on a short circuit. + /// + Task WriteCvAsync(ushort cvAddress, byte value, TimeSpan timeout); + event EventHandler? CvReadCompleted; event EventHandler? CvProgrammingFailed; diff --git a/src/Z21.Client.UnitTest/Core/SafeCvProgrammingTest.cs b/src/Z21.Client.UnitTest/Core/SafeCvProgrammingTest.cs new file mode 100644 index 0000000..8af7b12 --- /dev/null +++ b/src/Z21.Client.UnitTest/Core/SafeCvProgrammingTest.cs @@ -0,0 +1,215 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Threading.Tasks; +using CommandStation; +using Moq; +using Z21.Core; +using Z21.Core.Codecs; +using Z21.Core.Command; +using Z21.Core.Framing; +using Z21.Core.Model.EventArgs; +using Z21.Core.ResponseHandler; +using Z21.Core.ResponseHandler.Driving; +using Z21.Core.ResponseHandler.FastClock; +using Z21.Core.ResponseHandler.Feedback; +using Z21.Core.ResponseHandler.Programming; +using Z21.Core.ResponseHandler.Switching; +using Z21.Core.ResponseHandler.SystemState; +using Z21.Core.ResponseHandler.SystemState.TrackPower; + +namespace Z21.UnitTest.Core +{ + public class SafeCvProgrammingTest + { + private FakeTransport _transport = null!; + private Mock _cvResult = null!; + private Mock _cvNack = null!; + private Mock _cvNackSc = null!; + private Z21CommandStation _station = null!; + + [SetUp] + public void SetUp() + { + _transport = new FakeTransport(); + Z21CommandFactory factory = new(new Z21FrameBuilder(), new AddressCodec(), new LocoSpeedCodec()); + _cvResult = new Mock(); + _cvNack = new Mock(); + _cvNackSc = new Mock(); + Z21ResponseHandler dispatcher = new(_transport, new Z21FrameReader(), new List()); + + _station = new Z21CommandStation( + _transport, + dispatcher, + factory, + new Z21Options(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + _cvResult.Object, + _cvNack.Object, + _cvNackSc.Object, + Mock.Of(), + Mock.Of()); + + _transport.SetConnected(true); + } + + [TearDown] + public void TearDown() => _station.Dispose(); + + private void RaiseResult(ushort cvAddress, byte value) => + _cvResult.Raise(h => h.OnCvResultReceived += null, new CvResultReceivedEventArgs(cvAddress, value)); + + private void RaiseNack() => _cvNack.Raise(h => h.OnCvNackReceived += null, EventArgs.Empty); + + private void RaiseShortCircuit() => _cvNackSc.Raise(h => h.OnCvNackShortCircuitReceived += null, EventArgs.Empty); + + private async Task WaitForSentAsync(int count) + { + Stopwatch stopwatch = Stopwatch.StartNew(); + while (_transport.Sent.Count < count) + { + if (stopwatch.Elapsed > TimeSpan.FromSeconds(2)) + throw new TimeoutException($"Expected {count} sent datagrams, saw {_transport.Sent.Count}."); + await Task.Delay(5); + } + } + + [Test] + public async Task ReadCvAsync_ResultReturnsValue() + { + Task task = _station.ReadCvAsync(5, TimeSpan.FromSeconds(2)); + RaiseResult(5, 42); + + byte value = await task; + Assert.Multiple(() => + { + Assert.That(value, Is.EqualTo(42)); + Assert.That(_transport.Sent, Has.Count.EqualTo(1)); + }); + } + + [Test] + public async Task ReadCvAsync_RetriesOnNackThenReturnsValue() + { + Task task = _station.ReadCvAsync(5, TimeSpan.FromSeconds(5)); + + RaiseNack(); + await WaitForSentAsync(2); + RaiseResult(5, 99); + + byte value = await task; + Assert.Multiple(() => + { + Assert.That(value, Is.EqualTo(99)); + Assert.That(_transport.Sent, Has.Count.EqualTo(2)); + }); + } + + [Test] + public void ReadCvAsync_ShortCircuitThrowsAndDoesNotRetry() + { + Task task = _station.ReadCvAsync(5, TimeSpan.FromSeconds(2)); + RaiseShortCircuit(); + + Assert.ThrowsAsync(async () => await task); + Assert.That(_transport.Sent, Has.Count.EqualTo(1)); + } + + [Test] + public void ReadCvAsync_NoResponseThrowsTimeout() + { + Task task = _station.ReadCvAsync(5, TimeSpan.FromMilliseconds(100)); + + Assert.ThrowsAsync(async () => await task); + } + + [Test] + public async Task WriteCvAsync_ResultCompletes() + { + Task task = _station.WriteCvAsync(7, 200, TimeSpan.FromSeconds(2)); + RaiseResult(7, 200); + + await task; + Assert.That(_transport.Sent, Has.Count.EqualTo(1)); + } + + [Test] + public async Task WriteCvAsync_RetriesOnNackThenCompletes() + { + Task task = _station.WriteCvAsync(7, 200, TimeSpan.FromSeconds(5)); + + RaiseNack(); + await WaitForSentAsync(2); + RaiseResult(7, 200); + + await task; + Assert.That(_transport.Sent, Has.Count.EqualTo(2)); + } + + [Test] + public void WriteCvAsync_NoResponseThrowsTimeout() + { + Task task = _station.WriteCvAsync(7, 200, TimeSpan.FromMilliseconds(100)); + + Assert.ThrowsAsync(async () => await task); + } + + [Test] + public async Task ReadPomCvAsync_ResultReturnsValue() + { + Task task = _station.ReadPomCvAsync(3, 5, TimeSpan.FromSeconds(2)); + RaiseResult(5, 17); + + Assert.That(await task, Is.EqualTo(17)); + } + + [Test] + public void ReadPomCvAsync_NoRailComReplyThrowsTimeout() + { + Task task = _station.ReadPomCvAsync(3, 5, TimeSpan.FromMilliseconds(100)); + + Assert.ThrowsAsync(async () => await task); + } + + [Test] + public async Task WritePomCvAsync_ReadBackMatchesCompletes() + { + Task task = _station.WritePomCvAsync(3, 5, 50, TimeSpan.FromSeconds(2)); + + await WaitForSentAsync(2); // POM write + POM read-back + RaiseResult(5, 50); + + await task; + Assert.That(_transport.Sent, Has.Count.EqualTo(2)); + } + + [Test] + public async Task WritePomCvAsync_RetriesUntilReadBackMatches() + { + Task task = _station.WritePomCvAsync(3, 5, 50, TimeSpan.FromSeconds(5)); + + await WaitForSentAsync(2); // write + read-back + RaiseResult(5, 13); // read-back mismatch -> rewrite + reread + await WaitForSentAsync(4); + RaiseResult(5, 50); // matches -> done + + await task; + Assert.That(_transport.Sent, Has.Count.EqualTo(4)); + } + + [Test] + public void WritePomCvAsync_NoReplyThrowsTimeout() + { + Task task = _station.WritePomCvAsync(3, 5, 50, TimeSpan.FromMilliseconds(100)); + + Assert.ThrowsAsync(async () => await task); + } + } +} diff --git a/src/Z21.Client/Core/IZ21CommandStation.cs b/src/Z21.Client/Core/IZ21CommandStation.cs index 8ff7e9b..e538365 100644 --- a/src/Z21.Client/Core/IZ21CommandStation.cs +++ b/src/Z21.Client/Core/IZ21CommandStation.cs @@ -1,3 +1,4 @@ +using System; using System.Threading.Tasks; using CommandStation; using Z21.Core.Command; @@ -19,5 +20,21 @@ public interface IZ21CommandStation : ICommandStation, ILocoControl, IAccessoryC /// Sends one or more raw commands in a single UDP packet. /// Task SendCommandsAsync(params IZ21Command[] commands); + + /// + /// Reads a CV of a locomotive decoder on the main track (POM), retrying while the decoder does not + /// acknowledge, and returns the value. Requires RailCom; without it the read times out. Throws + /// on timeout or on + /// a short circuit. + /// + Task ReadPomCvAsync(ushort locoAddress, ushort cvAddress, TimeSpan timeout); + + /// + /// Writes a CV of a locomotive decoder on the main track (POM). Because a POM write returns no + /// acknowledgement, this verifies by reading the CV back and retrying until the read-back matches + /// the written value (so it requires RailCom). Throws if + /// it cannot be confirmed within . + /// + Task WritePomCvAsync(ushort locoAddress, ushort cvAddress, byte value, TimeSpan timeout); } } diff --git a/src/Z21.Client/Core/Z21CommandStation.cs b/src/Z21.Client/Core/Z21CommandStation.cs index 4cc2b18..5d46359 100644 --- a/src/Z21.Client/Core/Z21CommandStation.cs +++ b/src/Z21.Client/Core/Z21CommandStation.cs @@ -1,5 +1,6 @@ using System; using System.Linq; +using System.Threading; using System.Threading.Tasks; using CommandStation; using CommandStation.Transport; @@ -31,6 +32,7 @@ public class Z21CommandStation : IZ21CommandStation, IProgrammingControl, IFeedb private readonly Z21ResponseHandler _dispatcher; private readonly Z21Options _options; private readonly DelayedAction _delayedKeepAliveAction; + private readonly SemaphoreSlim _cvLock = new(1, 1); private readonly ILogger? _logger; /// @@ -180,6 +182,159 @@ public Task SetExtAccessoryAsync(ushort accessoryAddress, byte payload) => public Task WriteCvAsync(ushort cvAddress, byte value) => SendCommandsAsync(Commands.Create(cvAddress, value)); + public async Task ReadCvAsync(ushort cvAddress, TimeSpan timeout) + { + using CancellationTokenSource deadline = new(timeout); + await AcquireCvLockAsync(cvAddress, timeout, deadline.Token); + try + { + return await AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(cvAddress)), deadline.Token, timeout); + } + finally + { + _cvLock.Release(); + } + } + + public async Task WriteCvAsync(ushort cvAddress, byte value, TimeSpan timeout) + { + using CancellationTokenSource deadline = new(timeout); + await AcquireCvLockAsync(cvAddress, timeout, deadline.Token); + try + { + await AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(cvAddress, value)), deadline.Token, timeout); + } + finally + { + _cvLock.Release(); + } + } + + public async Task ReadPomCvAsync(ushort locoAddress, ushort cvAddress, TimeSpan timeout) + { + using CancellationTokenSource deadline = new(timeout); + await AcquireCvLockAsync(cvAddress, timeout, deadline.Token); + try + { + return await AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(locoAddress, cvAddress)), deadline.Token, timeout); + } + finally + { + _cvLock.Release(); + } + } + + public async Task WritePomCvAsync(ushort locoAddress, ushort cvAddress, byte value, TimeSpan timeout) + { + using CancellationTokenSource deadline = new(timeout); + await AcquireCvLockAsync(cvAddress, timeout, deadline.Token); + try + { + while (true) + { + await SendCommandsAsync(Commands.Create(locoAddress, cvAddress, value)); + byte readBack = await AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(locoAddress, cvAddress)), deadline.Token, timeout); + if (readBack == value) + return; + if (deadline.IsCancellationRequested) + throw new CvOperationTimeoutException(cvAddress, timeout); + } + } + finally + { + _cvLock.Release(); + } + } + + private async Task AcquireCvLockAsync(ushort cvAddress, TimeSpan timeout, CancellationToken deadline) + { + try + { + await _cvLock.WaitAsync(deadline); + } + catch (OperationCanceledException) + { + throw new CvOperationTimeoutException(cvAddress, timeout); + } + } + + private async Task AwaitResultLoopAsync(ushort cvAddress, Func send, CancellationToken deadline, TimeSpan timeout) + { + while (true) + { + if (deadline.IsCancellationRequested) + throw new CvOperationTimeoutException(cvAddress, timeout); + + CvAttempt attempt; + try + { + attempt = await AwaitNextCvAsync(cvAddress, send, deadline); + } + catch (OperationCanceledException) + { + throw new CvOperationTimeoutException(cvAddress, timeout); + } + + switch (attempt.Kind) + { + case CvAttemptKind.Result: + return attempt.Value; + case CvAttemptKind.ShortCircuit: + throw new CvShortCircuitException(cvAddress); + default: + break; // missing acknowledgement -> retry until the deadline + } + } + } + + private async Task AwaitNextCvAsync(ushort cvAddress, Func send, CancellationToken deadline) + { + TaskCompletionSource completion = new(TaskCreationOptions.RunContinuationsAsynchronously); + + void OnResult(object? sender, CvValue value) + { + if (value.CvAddress == cvAddress) + completion.TrySetResult(new CvAttempt(CvAttemptKind.Result, value.Value)); + } + + void OnFailed(object? sender, CvProgrammingError error) => + completion.TrySetResult(new CvAttempt(error == CvProgrammingError.ShortCircuit ? CvAttemptKind.ShortCircuit : CvAttemptKind.Nack, 0)); + + CvReadCompleted += OnResult; + CvProgrammingFailed += OnFailed; + try + { + await send(); + using (deadline.Register(() => completion.TrySetCanceled(deadline))) + return await completion.Task; + } + finally + { + CvReadCompleted -= OnResult; + CvProgrammingFailed -= OnFailed; + } + } + + private enum CvAttemptKind + { + Result, + Nack, + ShortCircuit + } + + private readonly struct CvAttempt + { + public CvAttempt(CvAttemptKind kind, byte value) + { + Kind = kind; + Value = value; + } + + public CvAttemptKind Kind { get; } + + public byte Value { get; } + } + public Task RequestFeedbackAsync(byte groupIndex) => SendCommandsAsync(Commands.Create(groupIndex)); public Task RequestModelTimeAsync() => SendCommandsAsync(Commands.Create(FastClockAction.Read)); @@ -208,6 +363,7 @@ private async Task KeepAliveAsync() public void Dispose() { _delayedKeepAliveAction.Dispose(); + _cvLock.Dispose(); GC.SuppressFinalize(this); } } From 6124e4c11f29181366cab849d1f296f36450b26e Mon Sep 17 00:00:00 2001 From: Jakob Date: Fri, 5 Jun 2026 23:41:56 +0200 Subject: [PATCH 2/4] Bump version to 7.0.3 --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index a59cefc..7c48c6a 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/main/src/NerdBank.GitVersioning/version.schema.json", - "version": "7.0", + "version": "7.0.3", "publicReleaseRefs": [ "^refs/heads/main$" ] From 4a3f4e1ce19db9bdae2b9d174b8c06acb6d4e29d Mon Sep 17 00:00:00 2001 From: Jakob Date: Sat, 6 Jun 2026 00:16:44 +0200 Subject: [PATCH 3/4] Harden safe CV programming against races, unbounded sends and misuse --- .../IProgrammingControl.cs | 17 +- .../Core/SafeCvProgrammingTest.cs | 43 +++++ src/Z21.Client/Core/IZ21CommandStation.cs | 16 +- src/Z21.Client/Core/Z21CommandStation.cs | 170 +++++++++++------- src/Z21.Client/Core/Z21Options.cs | 8 + 5 files changed, 181 insertions(+), 73 deletions(-) diff --git a/src/CommandStation.Abstractions/IProgrammingControl.cs b/src/CommandStation.Abstractions/IProgrammingControl.cs index 96ec3ff..84dfb7d 100644 --- a/src/CommandStation.Abstractions/IProgrammingControl.cs +++ b/src/CommandStation.Abstractions/IProgrammingControl.cs @@ -16,17 +16,24 @@ public interface IProgrammingControl /// /// Reads a CV on the programming track, retrying while the decoder does not acknowledge, and - /// returns the value. Throws if no result arrives within - /// , or on a short circuit. + /// returns the value. A missing acknowledgement that never clears (for example an absent or + /// unreadable decoder) is reported as a timeout rather than a distinct error. Do not call the + /// fire-and-forget CV methods concurrently with this one on the same station. /// + /// No result arrived within . + /// The command station reported a short circuit. + /// is not a positive, in-range duration. Task ReadCvAsync(ushort cvAddress, TimeSpan timeout); /// /// Writes a CV on the programming track, retrying while the decoder does not acknowledge, and - /// completes once the command station confirms the write. Throws - /// if not confirmed within , or - /// on a short circuit. + /// completes once the command station acknowledges the write (LAN_X_CV_RESULT). A missing + /// acknowledgement that never clears is reported as a timeout. Do not call the fire-and-forget CV + /// methods concurrently with this one on the same station. /// + /// The write was not acknowledged within . + /// The command station reported a short circuit. + /// is not a positive, in-range duration. Task WriteCvAsync(ushort cvAddress, byte value, TimeSpan timeout); event EventHandler? CvReadCompleted; diff --git a/src/Z21.Client.UnitTest/Core/SafeCvProgrammingTest.cs b/src/Z21.Client.UnitTest/Core/SafeCvProgrammingTest.cs index 8af7b12..f4da533 100644 --- a/src/Z21.Client.UnitTest/Core/SafeCvProgrammingTest.cs +++ b/src/Z21.Client.UnitTest/Core/SafeCvProgrammingTest.cs @@ -211,5 +211,48 @@ public void WritePomCvAsync_NoReplyThrowsTimeout() Assert.ThrowsAsync(async () => await task); } + + [Test] + public void ReadCvAsync_NonPositiveTimeoutThrowsArgumentOutOfRange() => + Assert.ThrowsAsync(async () => await _station.ReadCvAsync(5, TimeSpan.Zero)); + + [Test] + public void WriteCvAsync_NegativeTimeoutThrowsArgumentOutOfRange() => + Assert.ThrowsAsync(async () => await _station.WriteCvAsync(5, 1, TimeSpan.FromSeconds(-1))); + + [Test] + public async Task FireAndForgetCv_WhileSafeOperationActive_Throws() + { + Task safe = _station.ReadCvAsync(5, TimeSpan.FromSeconds(5)); + await WaitForSentAsync(1); // safe op has acquired the CV lock and is awaiting a result + + Assert.Multiple(() => + { + Assert.Throws(() => _station.ReadCvAsync(9)); + Assert.Throws(() => _station.WriteCvAsync(9, 1)); + }); + + RaiseResult(5, 7); // let the safe op finish so the fixture tears down cleanly + Assert.That(await safe, Is.EqualTo(7)); + } + + [Test] + public async Task Dispose_DuringInFlightOperation_ThrowsObjectDisposed() + { + Task task = _station.ReadCvAsync(5, TimeSpan.FromSeconds(5)); + await WaitForSentAsync(1); + + _station.Dispose(); + + Assert.ThrowsAsync(async () => await task); + } + + [Test] + public void Operation_AfterDispose_ThrowsObjectDisposed() + { + _station.Dispose(); + + Assert.ThrowsAsync(async () => await _station.ReadCvAsync(5, TimeSpan.FromSeconds(2))); + } } } diff --git a/src/Z21.Client/Core/IZ21CommandStation.cs b/src/Z21.Client/Core/IZ21CommandStation.cs index e538365..fcd82f9 100644 --- a/src/Z21.Client/Core/IZ21CommandStation.cs +++ b/src/Z21.Client/Core/IZ21CommandStation.cs @@ -23,18 +23,24 @@ public interface IZ21CommandStation : ICommandStation, ILocoControl, IAccessoryC /// /// Reads a CV of a locomotive decoder on the main track (POM), retrying while the decoder does not - /// acknowledge, and returns the value. Requires RailCom; without it the read times out. Throws - /// on timeout or on - /// a short circuit. + /// acknowledge, and returns the value. Requires RailCom; without it (or for an absent decoder) the + /// read can only ever time out. The result is correlated by CV address only — the protocol's POM + /// result carries no loco address — so do not run other CV operations on this station concurrently. /// + /// No result arrived within . + /// The command station reported a short circuit. + /// is not a positive, in-range duration. Task ReadPomCvAsync(ushort locoAddress, ushort cvAddress, TimeSpan timeout); /// /// Writes a CV of a locomotive decoder on the main track (POM). Because a POM write returns no /// acknowledgement, this verifies by reading the CV back and retrying until the read-back matches - /// the written value (so it requires RailCom). Throws if - /// it cannot be confirmed within . + /// the written value (so it requires RailCom). A decoder that never reads back the written value is + /// reported as a timeout. Do not run other CV operations on this station concurrently. /// + /// The write could not be confirmed within . + /// The command station reported a short circuit. + /// is not a positive, in-range duration. Task WritePomCvAsync(ushort locoAddress, ushort cvAddress, byte value, TimeSpan timeout); } } diff --git a/src/Z21.Client/Core/Z21CommandStation.cs b/src/Z21.Client/Core/Z21CommandStation.cs index 5d46359..cc6ce45 100644 --- a/src/Z21.Client/Core/Z21CommandStation.cs +++ b/src/Z21.Client/Core/Z21CommandStation.cs @@ -33,6 +33,8 @@ public class Z21CommandStation : IZ21CommandStation, IProgrammingControl, IFeedb private readonly Z21Options _options; private readonly DelayedAction _delayedKeepAliveAction; private readonly SemaphoreSlim _cvLock = new(1, 1); + private readonly CancellationTokenSource _disposeCts = new(); + private volatile bool _disposed; private readonly ILogger? _logger; /// @@ -178,101 +180,90 @@ public Task SetExtAccessoryAsync(ushort accessoryAddress, byte payload) => public Task RequestStatusAsync() => SendCommandsAsync(Commands.Create()); - public Task ReadCvAsync(ushort cvAddress) => SendCommandsAsync(Commands.Create(cvAddress)); - - public Task WriteCvAsync(ushort cvAddress, byte value) => SendCommandsAsync(Commands.Create(cvAddress, value)); - - public async Task ReadCvAsync(ushort cvAddress, TimeSpan timeout) + public Task ReadCvAsync(ushort cvAddress) { - using CancellationTokenSource deadline = new(timeout); - await AcquireCvLockAsync(cvAddress, timeout, deadline.Token); - try - { - return await AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(cvAddress)), deadline.Token, timeout); - } - finally - { - _cvLock.Release(); - } + ThrowIfSafeCvOperationActive(); + return SendCommandsAsync(Commands.Create(cvAddress)); } - public async Task WriteCvAsync(ushort cvAddress, byte value, TimeSpan timeout) + public Task WriteCvAsync(ushort cvAddress, byte value) { - using CancellationTokenSource deadline = new(timeout); - await AcquireCvLockAsync(cvAddress, timeout, deadline.Token); - try - { - await AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(cvAddress, value)), deadline.Token, timeout); - } - finally - { - _cvLock.Release(); - } + ThrowIfSafeCvOperationActive(); + return SendCommandsAsync(Commands.Create(cvAddress, value)); } - public async Task ReadPomCvAsync(ushort locoAddress, ushort cvAddress, TimeSpan timeout) + public Task ReadCvAsync(ushort cvAddress, TimeSpan timeout) => + RunUnderCvLockAsync(cvAddress, timeout, + token => AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(cvAddress)), token, timeout)); + + public async Task WriteCvAsync(ushort cvAddress, byte value, TimeSpan timeout) => + await RunUnderCvLockAsync(cvAddress, timeout, + token => AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(cvAddress, value)), token, timeout)); + + public Task ReadPomCvAsync(ushort locoAddress, ushort cvAddress, TimeSpan timeout) => + RunUnderCvLockAsync(cvAddress, timeout, + token => AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(locoAddress, cvAddress)), token, timeout)); + + public async Task WritePomCvAsync(ushort locoAddress, ushort cvAddress, byte value, TimeSpan timeout) => + await RunUnderCvLockAsync(cvAddress, timeout, + token => WritePomCvCoreAsync(locoAddress, cvAddress, value, token, timeout)); + + private async Task WritePomCvCoreAsync(ushort locoAddress, ushort cvAddress, byte value, CancellationToken token, TimeSpan timeout) { - using CancellationTokenSource deadline = new(timeout); - await AcquireCvLockAsync(cvAddress, timeout, deadline.Token); - try - { - return await AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(locoAddress, cvAddress)), deadline.Token, timeout); - } - finally + while (true) { - _cvLock.Release(); + await SendCommandsAsync(Commands.Create(locoAddress, cvAddress, value)).WaitAsync(token); + byte readBack = await AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(locoAddress, cvAddress)), token, timeout); + if (readBack == value) + return value; + + await DelayBeforeRetryAsync(cvAddress, token, timeout); // read-back mismatch -> wait, then re-write } } - public async Task WritePomCvAsync(ushort locoAddress, ushort cvAddress, byte value, TimeSpan timeout) + private async Task RunUnderCvLockAsync(ushort cvAddress, TimeSpan timeout, Func> operation) { + ObjectDisposedException.ThrowIf(_disposed, this); + ValidateTimeout(timeout); + using CancellationTokenSource deadline = new(timeout); - await AcquireCvLockAsync(cvAddress, timeout, deadline.Token); + using CancellationTokenSource linked = CancellationTokenSource.CreateLinkedTokenSource(deadline.Token, _disposeCts.Token); + + await AcquireCvLockAsync(cvAddress, timeout, linked.Token); try { - while (true) - { - await SendCommandsAsync(Commands.Create(locoAddress, cvAddress, value)); - byte readBack = await AwaitResultLoopAsync(cvAddress, () => SendCommandsAsync(Commands.Create(locoAddress, cvAddress)), deadline.Token, timeout); - if (readBack == value) - return; - if (deadline.IsCancellationRequested) - throw new CvOperationTimeoutException(cvAddress, timeout); - } + return await operation(linked.Token); } finally { - _cvLock.Release(); + ReleaseCvLock(); } } - private async Task AcquireCvLockAsync(ushort cvAddress, TimeSpan timeout, CancellationToken deadline) + private async Task AcquireCvLockAsync(ushort cvAddress, TimeSpan timeout, CancellationToken token) { try { - await _cvLock.WaitAsync(deadline); + await _cvLock.WaitAsync(token); } - catch (OperationCanceledException) + catch (OperationCanceledException) when (token.IsCancellationRequested) { - throw new CvOperationTimeoutException(cvAddress, timeout); + throw MapCancellation(cvAddress, timeout); } } - private async Task AwaitResultLoopAsync(ushort cvAddress, Func send, CancellationToken deadline, TimeSpan timeout) + private async Task AwaitResultLoopAsync(ushort cvAddress, Func send, CancellationToken token, TimeSpan timeout) { while (true) { - if (deadline.IsCancellationRequested) - throw new CvOperationTimeoutException(cvAddress, timeout); - CvAttempt attempt; try { - attempt = await AwaitNextCvAsync(cvAddress, send, deadline); + attempt = await AwaitNextCvAsync(cvAddress, send, token); } - catch (OperationCanceledException) + catch (OperationCanceledException) when (token.IsCancellationRequested) { - throw new CvOperationTimeoutException(cvAddress, timeout); + throw MapCancellation(cvAddress, timeout); } switch (attempt.Kind) @@ -282,12 +273,13 @@ private async Task AwaitResultLoopAsync(ushort cvAddress, Func send, case CvAttemptKind.ShortCircuit: throw new CvShortCircuitException(cvAddress); default: - break; // missing acknowledgement -> retry until the deadline + await DelayBeforeRetryAsync(cvAddress, token, timeout); // missing acknowledgement -> back off, then retry until the deadline + break; } } } - private async Task AwaitNextCvAsync(ushort cvAddress, Func send, CancellationToken deadline) + private async Task AwaitNextCvAsync(ushort cvAddress, Func send, CancellationToken token) { TaskCompletionSource completion = new(TaskCreationOptions.RunContinuationsAsynchronously); @@ -304,9 +296,10 @@ void OnFailed(object? sender, CvProgrammingError error) => CvProgrammingFailed += OnFailed; try { - await send(); - using (deadline.Register(() => completion.TrySetCanceled(deadline))) - return await completion.Task; + // WaitAsync bounds both the send and the wait by the deadline, so a stalled transport cannot + // outlive the caller's timeout. + await send().WaitAsync(token); + return await completion.Task.WaitAsync(token); } finally { @@ -315,6 +308,51 @@ void OnFailed(object? sender, CvProgrammingError error) => } } + private async Task DelayBeforeRetryAsync(ushort cvAddress, CancellationToken token, TimeSpan timeout) + { + try + { + await Task.Delay(_options.CvRetryDelay, token); + } + catch (OperationCanceledException) when (token.IsCancellationRequested) + { + throw MapCancellation(cvAddress, timeout); + } + } + + private void ThrowIfSafeCvOperationActive() + { + if (_cvLock.CurrentCount == 0) + throw new InvalidOperationException( + "A safe (timeout-bounded) CV operation is in progress. Fire-and-forget CV commands cannot run " + + "concurrently with a safe CV operation, because CV NACKs carry no address and would be misattributed."); + } + + private void ValidateTimeout(TimeSpan timeout) + { + if (timeout <= TimeSpan.Zero) + throw new ArgumentOutOfRangeException(nameof(timeout), timeout, "The CV operation timeout must be greater than zero."); + if (timeout.TotalMilliseconds > int.MaxValue) + throw new ArgumentOutOfRangeException(nameof(timeout), timeout, $"The CV operation timeout must not exceed {int.MaxValue} milliseconds."); + } + + private System.Exception MapCancellation(ushort cvAddress, TimeSpan timeout) => + _disposeCts.IsCancellationRequested + ? new ObjectDisposedException(GetType().FullName) + : new CvOperationTimeoutException(cvAddress, timeout); + + private void ReleaseCvLock() + { + try + { + _cvLock.Release(); + } + catch (ObjectDisposedException exception) + { + _logger?.LogDebug(exception, "CV lock released after the station was disposed."); + } + } + private enum CvAttemptKind { Result, @@ -362,7 +400,13 @@ private async Task KeepAliveAsync() public void Dispose() { + if (_disposed) + return; + _disposed = true; + + _disposeCts.Cancel(); // unblock any in-flight safe CV operation; it surfaces as ObjectDisposedException _delayedKeepAliveAction.Dispose(); + _disposeCts.Dispose(); _cvLock.Dispose(); GC.SuppressFinalize(this); } diff --git a/src/Z21.Client/Core/Z21Options.cs b/src/Z21.Client/Core/Z21Options.cs index a83b083..dc4d834 100644 --- a/src/Z21.Client/Core/Z21Options.cs +++ b/src/Z21.Client/Core/Z21Options.cs @@ -22,5 +22,13 @@ public class Z21Options /// Interval after the last command before an automatic keep-alive request is sent. /// public TimeSpan KeepAliveInterval { get; set; } = TimeSpan.FromSeconds(45); + + /// + /// Delay between retries of a safe (timeout-bounded) CV operation after the decoder fails to + /// acknowledge (LAN_X_CV_NACK). A short, non-zero delay avoids hammering the command station + /// and repeatedly re-entering programming mode while a slow byte-wise read is in progress. The + /// caller-supplied timeout still bounds the overall operation. + /// + public TimeSpan CvRetryDelay { get; set; } = TimeSpan.FromMilliseconds(50); } } From 850692dcd8db4d42f24066b375bb0a3e41897bad Mon Sep 17 00:00:00 2001 From: Jakob Date: Sat, 6 Jun 2026 00:29:14 +0200 Subject: [PATCH 4/4] Bump version to 7.1 for the safe CV programming feature --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index 7c48c6a..475e1d5 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/main/src/NerdBank.GitVersioning/version.schema.json", - "version": "7.0.3", + "version": "7.1", "publicReleaseRefs": [ "^refs/heads/main$" ]