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 84852e1..eb360c1 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 a3b55d7..cc6ce45 100644 --- a/src/Z21.Client/Core/Z21CommandStation.cs +++ b/src/Z21.Client/Core/Z21CommandStation.cs @@ -33,8 +33,9 @@ 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; - private bool _disposed; /// /// IPv4 safe MTU for payload according to specification. @@ -179,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) @@ -283,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); @@ -305,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 { @@ -316,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, @@ -365,9 +402,11 @@ 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); } } diff --git a/version.json b/version.json index a59cefc..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", + "version": "7.1", "publicReleaseRefs": [ "^refs/heads/main$" ]