diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs index 804ff1bc..c914d160 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs @@ -44,17 +44,21 @@ public sealed class ProbeCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync(["__probe"], ct); - var health = driver.GetHealth(); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync(["__probe"], ct); + var health = driver.GetHealth(); - await console.Output.WriteLineAsync($"Gateway: {Gateway}"); - await console.Output.WriteLineAsync($"Family: {Family}"); - await console.Output.WriteLineAsync($"Health: {health.State}"); - if (health.LastError is { } err) - await console.Output.WriteLineAsync($"Last error: {err}"); - await console.Output.WriteLineAsync(); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + await console.Output.WriteLineAsync($"Gateway: {Gateway}"); + await console.Output.WriteLineAsync($"Family: {Family}"); + await console.Output.WriteLineAsync($"Health: {health.State}"); + if (health.LastError is { } err) + await console.Output.WriteLineAsync($"Last error: {err}"); + await console.Output.WriteLineAsync(); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs index 0d0bbb80..0e4b5ad4 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs @@ -45,9 +45,13 @@ public sealed class ReadCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync([tagName], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync([tagName], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs index b266fa3d..399b3a0e 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs @@ -50,23 +50,22 @@ public sealed class SubscribeCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); ISubscriptionHandle? handle = null; + // The poll engine raises OnDataChange on a background thread: serialise its lines against each other and + // against this thread's own, and never let a failed console write fault the poll loop (review 07 U7). + var writeLock = new object(); try { await driver.InitializeAsync("{}", ct); - await console.Output.WriteLineAsync( - $"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop."); - - driver.OnDataChange += (_, e) => - { - var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + - $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + - $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; - console.Output.WriteLine(line); - }; + driver.OnDataChange += CreateChangeLineWriter(console.Output, writeLock); handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct); + lock (writeLock) + { + console.Output.WriteLine($"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop."); + } + try { await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct); @@ -76,6 +75,10 @@ public sealed class SubscribeCommand : AbCipCommandBase // Expected on Ctrl+C. } } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Ctrl+C while still connecting/subscribing: a clean exit, not a stack trace (review 07 U7). + } finally { if (handle is not null) diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs index e4dadd38..b4473e02 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs @@ -57,9 +57,13 @@ public sealed class WriteCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(TagPath, results[0])); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(TagPath, results[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs index 157caf2d..e96b1a75 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs @@ -32,27 +32,6 @@ public abstract class AbLegacyCommandBase : DriverCommandBase "LogixPccc unverified).")] public AbLegacyCliTransport Transport { get; init; } = AbLegacyCliTransport.Default; - /// - /// Runs a one-shot command's driver work so a Ctrl-C (cancellation of ) prints - /// Cancelled. instead of an unhandled stack trace (review 07 U7). - /// Any other exception — and an OCE the operator did not ask for — still propagates. - /// - /// The CLI console. - /// The console's Ctrl-C token. - /// The command body. - /// A task that completes when the body has finished or was cancelled. - internal static async ValueTask RunCancellableAsync(CliFx.Infrastructure.IConsole console, CancellationToken ct, Func body) - { - try - { - await body(); - } - catch (OperationCanceledException) when (ct.IsCancellationRequested) - { - await console.Error.WriteLineAsync("Cancelled."); - } - } - /// Gets the per-operation timeout in milliseconds. [CommandOption("timeout-ms", Description = "Per-operation timeout in ms (default 5000).")] public int TimeoutMs { get; init; } = 5000; diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs index d77bff85..d5a087fd 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs @@ -79,5 +79,61 @@ public abstract class DriverCommandBase : ICommand /// protected static void FlushLogging() => Log.CloseAndFlush(); + /// + /// Runs a one-shot command's driver work so a Ctrl-C (cancellation of ) prints + /// Cancelled. instead of an unhandled stack trace (review 07 U7). + /// Any other exception — and an OCE the operator did not ask for — still propagates. + /// + /// The CLI console. + /// The console's Ctrl-C token. + /// The command body. + /// A task that completes when the body has finished or was cancelled. + public static async ValueTask RunCancellableAsync(IConsole console, CancellationToken ct, Func body) + { + try + { + await body(); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + await console.Error.WriteLineAsync("Cancelled."); + } + } + + /// + /// Builds the OnDataChange handler a subscribe command attaches: one + /// [HH:mm:ss.fff] ref = value (status) line per change. Writes are serialised on + /// (the poll engine raises from a background thread, concurrently with the + /// command thread's own lines), and a failing console write (closed pipe, redirected stream error) is logged + /// and swallowed so it cannot fault the driver's poll loop (review 07 U7). + /// + /// The console output writer. + /// The lock every console write of the command takes. + /// The event handler. + public static EventHandler CreateChangeLineWriter( + TextWriter output, object writeLock) + { + ArgumentNullException.ThrowIfNull(output); + ArgumentNullException.ThrowIfNull(writeLock); + return (_, e) => + { + try + { + var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + + $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + + $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; + lock (writeLock) + { + output.WriteLine(line); + } + } + catch (Exception ex) + { + Log.Logger.Warning(ex, "subscribe: console write failed for {Tag}; continuing the poll loop.", + e.FullReference); + } + }; + } + private bool _loggingConfigured; } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs index 906fee8e..9a49acdb 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs @@ -101,7 +101,11 @@ public static class SnapshotFormatter return sb.ToString().TrimEnd(); } - /// Formats a value for console output, handling null, bool, string, and formattable types. + /// + /// Formats a value for console output, handling null, bool, string, formattable and array/list types. An + /// array renders its elements ([1, 2, 3], each formatted as a scalar) rather than its CLR type name + /// (System.Int32[]). + /// /// The value to format. /// A formatted string representation of the value. public static string FormatValue(object? value) => value switch @@ -110,9 +114,23 @@ public static class SnapshotFormatter bool b => b ? "true" : "false", string s => $"\"{s}\"", IFormattable f => f.ToString(null, CultureInfo.InvariantCulture), + System.Collections.IEnumerable items => FormatSequence(items), _ => value.ToString() ?? "", }; + private static string FormatSequence(System.Collections.IEnumerable items) + { + var sb = new System.Text.StringBuilder("["); + var first = true; + foreach (var item in items) + { + if (!first) sb.Append(", "); + sb.Append(FormatValue(item)); + first = false; + } + return sb.Append(']').ToString(); + } + /// Formats an OPC UA status code as a hexadecimal string with named severity classification. /// The OPC UA status code to format. /// A formatted status string like "0x00000000 (Good)" or "0x80050000 (BadCommunicationError)". diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs new file mode 100644 index 00000000..cad52821 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs @@ -0,0 +1,50 @@ +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests; + +/// +/// Review 07 U7: the AbCip probe/read/write commands had no arm, so a +/// Ctrl-C printed a stack trace. A command cancelled by the operator now exits cleanly with Cancelled.. +/// The gateway is TEST-NET (never answers), so nothing is sent to a real device. +/// +[Trait("Category", "Unit")] +public sealed class CancellationHandlingTests +{ + private const string Blackhole = "ab://192.0.2.1/1,0"; + + private static async Task RunCancelledAsync(AbCipCommandBase command) + { + using var console = new FakeInMemoryConsole(); + console.RequestCancellation(); + + await Should.NotThrowAsync(async () => await command.ExecuteAsync(console)); + return console.ReadErrorString(); + } + + /// A cancelled read prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_read_prints_Cancelled() + { + (await RunCancelledAsync(new ReadCommand { Gateway = Blackhole, TagPath = "Motor01" })) + .ShouldContain("Cancelled."); + } + + /// A cancelled probe prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_probe_prints_Cancelled() + { + (await RunCancelledAsync(new ProbeCommand { Gateway = Blackhole, TagPath = "Motor01" })) + .ShouldContain("Cancelled."); + } + + /// A cancelled write prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_write_prints_Cancelled() + { + (await RunCancelledAsync(new WriteCommand { Gateway = Blackhole, TagPath = "Motor01", Value = "1" })) + .ShouldContain("Cancelled."); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs new file mode 100644 index 00000000..ecdf93f5 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs @@ -0,0 +1,111 @@ +using System.Text; +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests; + +/// +/// Review 07 U7, shared half: arrays render their elements (not System.Int32[]), Ctrl-C prints +/// Cancelled., and the subscribe change-line writer is serialised and never throws into the poll loop. +/// +[Trait("Category", "Unit")] +public sealed class CliHelpersU7Tests +{ + private sealed record Handle(string DiagnosticId) : ISubscriptionHandle; + + private static DataChangeEventArgs Change(string reference, object? value) => + new(new Handle("h"), reference, new DataValueSnapshot(value, 0u, DateTime.UtcNow, DateTime.UtcNow)); + + /// An array value renders as its elements, each formatted like a scalar. + [Fact] + public void FormatValue_renders_array_elements() + { + SnapshotFormatter.FormatValue(new[] { 1, 2, 3 }).ShouldBe("[1, 2, 3]"); + SnapshotFormatter.FormatValue(new[] { 1.5f, -2f }).ShouldBe("[1.5, -2]"); + SnapshotFormatter.FormatValue(new[] { true, false }).ShouldBe("[true, false]"); + SnapshotFormatter.FormatValue(new[] { "a", "b" }).ShouldBe("[\"a\", \"b\"]"); + SnapshotFormatter.FormatValue(new List { 7 }).ShouldBe("[7]"); + SnapshotFormatter.FormatValue(Array.Empty()).ShouldBe("[]"); + } + + /// A string is still a scalar, not a sequence of chars (positive control for the array arm). + [Fact] + public void FormatValue_keeps_a_string_scalar() + { + SnapshotFormatter.FormatValue("abc").ShouldBe("\"abc\""); + } + + /// Operator Ctrl-C prints Cancelled. instead of a stack trace. + [Fact] + public async Task RunCancellableAsync_operator_cancellation_prints_Cancelled() + { + using var console = new FakeInMemoryConsole(); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await Should.NotThrowAsync(async () => + await DriverCommandBase.RunCancellableAsync(console, cts.Token, () => Task.FromCanceled(cts.Token))); + + console.ReadErrorString().ShouldContain("Cancelled."); + } + + /// A cancellation the operator did not request still propagates. + [Fact] + public async Task RunCancellableAsync_unrequested_cancellation_propagates() + { + using var console = new FakeInMemoryConsole(); + + await Should.ThrowAsync(async () => + await DriverCommandBase.RunCancellableAsync(console, CancellationToken.None, + () => throw new OperationCanceledException())); + } + + /// A change line carries the reference, the formatted value and the status. + [Fact] + public void ChangeLineWriter_writes_one_formatted_line_per_change() + { + var output = new StringWriter(); + var write = DriverCommandBase.CreateChangeLineWriter(output, new object()); + + write(this, Change("Tag:DInt", new[] { 4, 5 })); + + var line = output.ToString().TrimEnd(); + line.ShouldContain("Tag:DInt = [4, 5] (0x00000000 (Good))"); + } + + /// Concurrent poll-thread changes produce whole lines, never interleaved fragments. + [Fact] + public async Task ChangeLineWriter_serialises_concurrent_writes() + { + var output = new StringWriter(new StringBuilder()); + var write = DriverCommandBase.CreateChangeLineWriter(output, new object()); + + await Task.WhenAll(Enumerable.Range(0, 8).Select(t => Task.Run(() => + { + for (var i = 0; i < 200; i++) write(this, Change($"T{t}", i)); + }))); + + var lines = output.ToString().Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries); + lines.Length.ShouldBe(1600); + lines.ShouldAllBe(l => l.StartsWith('[') && l.EndsWith("(Good))")); + } + + /// A failing console (closed pipe) is swallowed so it cannot fault the driver's poll loop. + [Fact] + public void ChangeLineWriter_swallows_a_console_write_failure() + { + var write = DriverCommandBase.CreateChangeLineWriter(new ThrowingWriter(), new object()); + + Should.NotThrow(() => write(this, Change("T", 1))); + } + + private sealed class ThrowingWriter : TextWriter + { + public override Encoding Encoding => Encoding.UTF8; + public override void Write(char value) => throw new IOException("pipe closed"); + public override void WriteLine(string? value) => throw new IOException("pipe closed"); + } +}