diff --git a/code-reviews/Driver.TwinCAT.Cli/findings.md b/code-reviews/Driver.TwinCAT.Cli/findings.md index 45ae46c..182db7a 100644 --- a/code-reviews/Driver.TwinCAT.Cli/findings.md +++ b/code-reviews/Driver.TwinCAT.Cli/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 7 | +| Open findings | 0 | ## Checklist coverage @@ -40,7 +40,7 @@ a category produced nothing rather than leaving it blank. | Severity | Low | | Category | Correctness & logic bugs | | Location | `TwinCATCommandBase.cs:23-24`, `Commands/SubscribeCommand.cs:23-24`, `Commands/BrowseCommand.cs:21-24` | -| Status | Open | +| Status | Resolved | **Description:** Numeric command options are accepted without range validation. `--timeout-ms` feeds `Timeout => TimeSpan.FromMilliseconds(TimeoutMs)`; passing `--timeout-ms 0` or a negative @@ -56,7 +56,16 @@ failure mode should be a readable up-front rejection. shared helper on `TwinCATCommandBase`) and throw `CliFx.Exceptions.CommandException` with a clear message when `TimeoutMs <= 0`, `IntervalMs <= 0`, or `AmsPort` falls outside `1..65535`. -**Resolution:** _(open)_ +**Resolution:** Added a virtual `Validate()` helper on `TwinCATCommandBase` that rejects +`TimeoutMs <= 0` and `AmsPort` outside `1..65535` with a clean +`CliFx.Exceptions.CommandException` carrying the offending value. `SubscribeCommand` overrides +`Validate()` to add the `IntervalMs > 0` check. Each `ExecuteAsync` calls `Validate()` first +so the CLI surfaces "bad argument" up-front instead of letting the driver fail with an opaque +transport error. Covered by `TwinCATCommandBaseTests.Validate_rejects_zero_timeout`, +`Validate_rejects_negative_timeout`, `Validate_rejects_out_of_range_ams_port` (theory, 4 +cases), `Validate_accepts_in_range_ams_port` (theory, 4 cases), +`SubscribeCommand_validate_rejects_zero_interval`, +`SubscribeCommand_validate_rejects_negative_interval`. ### Driver.TwinCAT.Cli-002 @@ -65,7 +74,7 @@ clear message when `TimeoutMs <= 0`, `IntervalMs <= 0`, or `AmsPort` falls outsi | Severity | Low | | Category | Concurrency & thread safety | | Location | `Commands/SubscribeCommand.cs:46-58` | -| Status | Open | +| Status | Resolved | **Description:** The `OnDataChange` handler calls `console.Output.WriteLine(line)` synchronously. In native ADS-notification mode the event is raised from the `Beckhoff.TwinCAT.Ads` @@ -83,7 +92,12 @@ serialised on one poll loop; the TwinCAT native path has no such serialisation. `TextWriter.Synchronized`. At minimum, gate it so the banner is written before the subscription is registered (it already is) and lock the per-event writes against each other. -**Resolution:** _(open)_ +**Resolution:** Introduced a per-execution `writeLock` object inside +`SubscribeCommand.ExecuteAsync`. Both the `OnDataChange` handler's `WriteLine` and the +post-subscription "Subscribed to ..." banner take the lock, so notification-callback writes +cannot interleave with the main-thread banner or with each other. Lock is local to the +command so parallel process instances do not contend with one another (each owns its own +console). ### Driver.TwinCAT.Cli-003 @@ -92,7 +106,7 @@ subscription is registered (it already is) and lock the per-event writes against | Severity | Low | | Category | Error handling & resilience | | Location | `Commands/SubscribeCommand.cs:56-58` | -| Status | Open | +| Status | Resolved | **Description:** The subscribe banner reports the mechanism purely from the `--poll-only` flag (`var mode = PollOnly ? "polling" : "ADS notification"`). The doc (`docs/Driver.TwinCAT.Cli.md`) @@ -108,7 +122,15 @@ returned `ISubscriptionHandle.DiagnosticId`, which is `twincat-native-sub-*` for path vs the `PollGroupEngine` handle for poll mode) or soften the wording to "(requested: ADS notification)" so it does not over-claim. -**Resolution:** _(open)_ +**Resolution:** Added internal static +`SubscribeCommand.DescribeMechanism(ISubscriptionHandle)` that maps +`DiagnosticId.StartsWith("twincat-native-sub-", Ordinal)` to `"ADS notification"` and +anything else to `"polling"`. The banner now reads from the handle the driver actually +returned, so the line cannot disagree with what the driver did even if a future fallback +lands the subscription somewhere unexpected. Covered by +`SubscribeCommandMechanismTests.DescribeMechanism_returns_ADS_notification_for_native_handle` +(theory, 3 cases) and `DescribeMechanism_returns_polling_for_anything_else` (theory, 4 cases +including an ordinal case-sensitivity guard). ### Driver.TwinCAT.Cli-004 @@ -117,7 +139,7 @@ ADS notification)" so it does not over-claim. | Severity | Low | | Category | Design-document adherence | | Location | `TwinCATCommandBase.cs:26-29`, `Commands/BrowseCommand.cs` | -| Status | Open | +| Status | Resolved | **Description:** `--poll-only` is declared on `TwinCATCommandBase`, so it is inherited by `browse`. `BrowseCommand` only ever calls `DiscoverAsync` — it never subscribes — so @@ -131,7 +153,15 @@ disagree. flag) onto an intermediate base shared by only `probe`/`read`/`subscribe`, or override/hide it for `browse`. Alternatively document explicitly that the flag is a no-op for `browse`. -**Resolution:** _(open)_ +**Resolution:** Introduced an intermediate `TwinCATTagCommandBase : TwinCATCommandBase` that +hosts the `--poll-only` flag and the `BuildOptions(...)` helper. `ProbeCommand`, +`ReadCommand`, `WriteCommand`, and `SubscribeCommand` inherit from this intermediate (they all +build a tag-list `TwinCATDriverOptions`). `BrowseCommand` keeps inheriting from +`TwinCATCommandBase` directly, so `--poll-only` no longer surfaces in `browse --help`. Browse +sets `UseNativeNotifications = true` on its inline options (irrelevant either way for the +discover-only path, but matches production wiring). Covered by +`TwinCATCommandBaseTests.BrowseCommand_does_not_expose_poll_only_flag` and +`ProbeCommand_still_exposes_poll_only_flag` (both reflect over the public property surface). ### Driver.TwinCAT.Cli-005 @@ -140,7 +170,7 @@ for `browse`. Alternatively document explicitly that the flag is a no-op for `br | Severity | Low | | Category | Code organization & conventions | | Location | `Commands/ProbeCommand.cs:23`, `Commands/ReadCommand.cs:20`, `Commands/WriteCommand.cs:20`, `Commands/SubscribeCommand.cs:18` | -| Status | Open | +| Status | Resolved | **Description:** The `--type` option is declared with the short alias `-t` on `read`, `write`, and `subscribe`, but `ProbeCommand` declares `[CommandOption("type", ...)]` with no short @@ -151,7 +181,10 @@ take the same `TwinCATDataType` option. **Recommendation:** Add the `'t'` short alias to `ProbeCommand`'s `--type` option to match the other three commands. -**Resolution:** _(open)_ +**Resolution:** Added the `'t'` short alias on `ProbeCommand.DataType`'s `[CommandOption]`, +matching read/write/subscribe so muscle memory carries between the four verbs. Covered by +`TwinCATCommandBaseTests.ProbeCommand_type_option_carries_short_alias_t` which asserts the +`CommandOptionAttribute.ShortName` is `'t'`. ### Driver.TwinCAT.Cli-006 @@ -160,7 +193,7 @@ other three commands. | Severity | Low | | Category | Testing coverage | | Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs` | -| Status | Open | +| Status | Resolved | **Description:** The only test file covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. Other deterministic, router-independent logic is untested: @@ -177,7 +210,20 @@ module's scope but worth flagging to whoever owns the test tree. `BuildOptions` field wiring, and for the `CollectingAddressSpaceBuilder` prefix/max filtering and access-classification logic. -**Resolution:** _(open)_ +**Resolution:** Added two new test classes. `TwinCATCommandBaseTests` covers the Gateway +canonical form, round-trip through `TwinCATAmsAddress.TryParse`, DriverInstanceId +composition, Timeout projection, BuildOptions field wiring (devices, tags, timeout, probe +disabled, controller-browse disabled, UseNativeNotifications default true), and the PollOnly +toggle flipping UseNativeNotifications. `BrowseCommandFilterTests` covers +`CollectingAddressSpaceBuilder` (records variables in call order, treats `Folder` as a +same-builder pass-through), `BrowseCommand.FilterByPrefix` (empty/null prefix passes +everything, case-sensitive ordinal match), `BrowseCommand.PrintLimit` (max <= 0 = unbounded, +caps when matched > max, no padding when matched < max), and `BrowseCommand.AccessTag` +(ViewOnly -> RO, every other classification -> RW, theory over all 6 non-ViewOnly values). +`BrowseCommand.CollectingAddressSpaceBuilder` made `internal` (was `private`) so the test +project can construct it directly via the existing `InternalsVisibleTo` hook. Total tests +for this assembly went from 27 to 69. The stale empty sibling test directory mention is +left out of scope as noted. ### Driver.TwinCAT.Cli-007 @@ -186,7 +232,7 @@ and access-classification logic. | Severity | Low | | Category | Documentation & comments | | Location | `TwinCATCommandBase.cs:31-36` | -| Status | Open | +| Status | Resolved | **Description:** The `Timeout` override has an empty `init` accessor with the comment `/* driven by TimeoutMs */`. Because the base `DriverCommandBase.Timeout` is declared @@ -199,4 +245,9 @@ gives no hint of the deliberate no-op. This is a maintainability/clarity nit, no computed projection of `--timeout-ms` and the `init` accessor is intentionally a no-op, so the design intent survives refactoring. -**Resolution:** _(open)_ +**Resolution:** Replaced the `` on `TwinCATCommandBase.Timeout` with an explicit +`` documenting that `Timeout` is projected from `TimeoutMs`, that the `init` +accessor required by the abstract base property is intentionally a no-op, and that adding a +backing field would cause the two to drift on every refactor. The inner-block comment was +tightened to point at the XML summary so the design intent survives whichever doc surface a +future maintainer reads first. diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/BrowseCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/BrowseCommand.cs index 02313b0..2064fd8 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/BrowseCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/BrowseCommand.cs @@ -10,6 +10,12 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands; /// when EnableControllerBrowse = true — structured UDTs / function-block instances /// won't appear because the driver filters to the supported primitive surface. /// +/// +/// Inherits from rather than +/// so the --poll-only flag does NOT surface in +/// browse --help: browse never subscribes, the flag would be a no-op, and the help +/// text would mislead users (Driver.TwinCAT.Cli-004). +/// [Command("browse", Description = "Enumerate controller symbols via the driver's DiscoverAsync walk.")] public sealed class BrowseCommand : TwinCATCommandBase { @@ -25,18 +31,21 @@ public sealed class BrowseCommand : TwinCATCommandBase public override async ValueTask ExecuteAsync(IConsole console) { + Validate(); ConfigureLogging(); var ct = console.RegisterCancellationHandler(); // Browse-only — no declared tags. EnableControllerBrowse=true flips DiscoverAsync's - // symbol-walk on so every recognized primitive surfaces through the builder. + // symbol-walk on so every recognized primitive surfaces through the builder. Native + // ADS notifications are irrelevant here (DiscoverAsync never subscribes); leave the + // default on so the options record matches the production wiring. var options = new TwinCATDriverOptions { Devices = [new TwinCATDeviceOptions(Gateway, $"cli-{AmsNetId}:{AmsPort}")], Tags = [], Timeout = Timeout, Probe = new TwinCATProbeOptions { Enabled = false }, - UseNativeNotifications = !PollOnly, + UseNativeNotifications = true, EnableControllerBrowse = true, }; @@ -52,10 +61,8 @@ public sealed class BrowseCommand : TwinCATCommandBase await driver.ShutdownAsync(CancellationToken.None); } - var matched = builder.Variables - .Where(v => string.IsNullOrEmpty(Prefix) || v.BrowseName.StartsWith(Prefix, StringComparison.Ordinal)) - .ToList(); - var printLimit = Max <= 0 ? matched.Count : Math.Min(Max, matched.Count); + var matched = FilterByPrefix(builder.Variables, Prefix); + var printLimit = PrintLimit(matched.Count, Max); await console.Output.WriteLineAsync($"AMS: {AmsNetId}:{AmsPort}"); await console.Output.WriteLineAsync( @@ -64,8 +71,7 @@ public sealed class BrowseCommand : TwinCATCommandBase foreach (var v in matched.Take(printLimit)) { - var access = v.Info.SecurityClass == SecurityClassification.ViewOnly ? "RO" : "RW"; - await console.Output.WriteLineAsync($" [{access}] {v.Info.DriverDataType,-8} {v.BrowseName}"); + await console.Output.WriteLineAsync($" [{AccessTag(v.Info)}] {v.Info.DriverDataType,-8} {v.BrowseName}"); } if (matched.Count > printLimit) @@ -73,7 +79,35 @@ public sealed class BrowseCommand : TwinCATCommandBase $" … {matched.Count - printLimit} more — raise --max or tighten --prefix"); } - private sealed class CollectingAddressSpaceBuilder : IAddressSpaceBuilder + /// + /// Case-sensitive prefix filter. A null/empty prefix keeps everything; otherwise we + /// keep symbols whose browse name starts with under + /// — TwinCAT identifiers are case-sensitive on + /// the wire, so a relaxed match would be misleading. + /// + internal static List<(string BrowseName, DriverAttributeInfo Info)> FilterByPrefix( + IReadOnlyList<(string BrowseName, DriverAttributeInfo Info)> source, string? prefix) + => source + .Where(v => string.IsNullOrEmpty(prefix) || v.BrowseName.StartsWith(prefix, StringComparison.Ordinal)) + .ToList(); + + /// + /// Cap-to-max projection. <= 0 means unbounded, otherwise the + /// min of and . + /// + internal static int PrintLimit(int matchedCount, int max) + => max <= 0 ? matchedCount : Math.Min(max, matchedCount); + + /// + /// Coarse RO/RW label used in the browse output. + /// is the only classification that is unconditionally read-only; everything else can be + /// written from at least one ACL tier, so the CLI labels it RW. The real per-tier + /// authorization is enforced server-side. + /// + internal static string AccessTag(DriverAttributeInfo info) + => info.SecurityClass == SecurityClassification.ViewOnly ? "RO" : "RW"; + + internal sealed class CollectingAddressSpaceBuilder : IAddressSpaceBuilder { public List<(string BrowseName, DriverAttributeInfo Info)> Variables { get; } = []; diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ProbeCommand.cs index 19d6811..b18a75b 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ProbeCommand.cs @@ -11,7 +11,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands; /// server near the endpoint. /// [Command("probe", Description = "Verify the TwinCAT runtime is reachable and a sample symbol reads.")] -public sealed class ProbeCommand : TwinCATCommandBase +public sealed class ProbeCommand : TwinCATTagCommandBase { [CommandOption("symbol", 's', Description = "Symbol path to probe. System-global examples: " + @@ -20,11 +20,14 @@ public sealed class ProbeCommand : TwinCATCommandBase IsRequired = true)] public string SymbolPath { get; init; } = default!; - [CommandOption("type", Description = "Data type (default DInt — TwinCAT DINT maps to int32).")] + [CommandOption("type", 't', Description = + "Bool / SInt / USInt / Int / UInt / DInt / UDInt / LInt / ULInt / Real / LReal / " + + "String / WString / Time / Date / DateTime / TimeOfDay (default DInt).")] public TwinCATDataType DataType { get; init; } = TwinCATDataType.DInt; public override async ValueTask ExecuteAsync(IConsole console) { + Validate(); ConfigureLogging(); var ct = console.RegisterCancellationHandler(); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs index 9222c73..ed7a1c1 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs @@ -9,7 +9,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands; /// member list into individual reads if you need them. /// [Command("read", Description = "Read a single TwinCAT symbol.")] -public sealed class ReadCommand : TwinCATCommandBase +public sealed class ReadCommand : TwinCATTagCommandBase { [CommandOption("symbol", 's', Description = "Symbol path. Program scope: 'MAIN.bStart'. Global: 'GVL.Counter'. " + @@ -24,6 +24,7 @@ public sealed class ReadCommand : TwinCATCommandBase public override async ValueTask ExecuteAsync(IConsole console) { + Validate(); ConfigureLogging(); var ct = console.RegisterCancellationHandler(); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/SubscribeCommand.cs index f186fdb..2accb5d 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/SubscribeCommand.cs @@ -1,4 +1,5 @@ using CliFx.Attributes; +using CliFx.Exceptions; using CliFx.Infrastructure; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; @@ -10,7 +11,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands; /// pushes on its own cycle); pass --poll-only to fall through to PollGroupEngine. /// [Command("subscribe", Description = "Watch a TwinCAT symbol via ADS notification or poll, until Ctrl+C.")] -public sealed class SubscribeCommand : TwinCATCommandBase +public sealed class SubscribeCommand : TwinCATTagCommandBase { [CommandOption("symbol", 's', Description = "Symbol path — same format as `read`.", IsRequired = true)] public string SymbolPath { get; init; } = default!; @@ -23,8 +24,17 @@ public sealed class SubscribeCommand : TwinCATCommandBase [CommandOption("interval-ms", 'i', Description = "Publishing interval ms (default 1000).")] public int IntervalMs { get; init; } = 1000; + protected override void Validate() + { + base.Validate(); + if (IntervalMs <= 0) + throw new CommandException( + $"--interval-ms must be greater than 0 (got {IntervalMs})."); + } + public override async ValueTask ExecuteAsync(IConsole console) { + Validate(); ConfigureLogging(); var ct = console.RegisterCancellationHandler(); @@ -43,19 +53,39 @@ public sealed class SubscribeCommand : TwinCATCommandBase { await driver.InitializeAsync("{}", ct); + // Native ADS notifications fire OnDataChange from the Beckhoff.TwinCAT.Ads + // notification callback thread — unlike the poll-mode path (which serialises on a + // single PollGroupEngine loop), the native callback can interleave with the banner + // write below and with subsequent change events if the PLC pushes faster than a + // single console write completes. A TextWriter is not guaranteed thread-safe, so + // we serialise every write through a lock to keep output clean for + // screen-recorded bug-report timelines (Driver.TwinCAT.Cli-002). + var writeLock = new object(); + 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); + lock (writeLock) + { + console.Output.WriteLine(line); + } }; handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct); - var mode = PollOnly ? "polling" : "ADS notification"; - await console.Output.WriteLineAsync( - $"Subscribed to {SymbolPath} @ {IntervalMs}ms ({mode}). Ctrl+C to stop."); + // Driver.TwinCAT.Cli-003: derive the banner mechanism from the actual subscription + // handle the driver returned, not from --poll-only. The native ADS path tags its + // handle with a "twincat-native-sub-*" DiagnosticId; anything else means we landed + // on the shared PollGroupEngine. That way the line cannot disagree with what the + // driver actually did (e.g. a future fallback inside SubscribeAsync). + var mode = DescribeMechanism(handle); + lock (writeLock) + { + console.Output.WriteLine( + $"Subscribed to {SymbolPath} @ {IntervalMs}ms ({mode}). Ctrl+C to stop."); + } try { await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct); @@ -75,4 +105,16 @@ public sealed class SubscribeCommand : TwinCATCommandBase await driver.ShutdownAsync(CancellationToken.None); } } + + /// + /// Maps the returned subscription handle's + /// to the banner label. The TwinCAT driver tags native ADS subscriptions with + /// twincat-native-sub-* and the shared PollGroupEngine handle uses a + /// different format — anything else means we landed on the poll loop. Internal so the + /// test assembly can cover the mapping without spinning a real driver. + /// + internal static string DescribeMechanism(ISubscriptionHandle handle) => + handle.DiagnosticId.StartsWith("twincat-native-sub-", StringComparison.Ordinal) + ? "ADS notification" + : "polling"; } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs index 83e208d..4b3d4c9 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs @@ -11,7 +11,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands; /// JSON for those. /// [Command("write", Description = "Write a single TwinCAT symbol.")] -public sealed class WriteCommand : TwinCATCommandBase +public sealed class WriteCommand : TwinCATTagCommandBase { [CommandOption("symbol", 's', Description = "Symbol path — same format as `read`.", IsRequired = true)] @@ -29,6 +29,7 @@ public sealed class WriteCommand : TwinCATCommandBase public override async ValueTask ExecuteAsync(IConsole console) { + Validate(); ConfigureLogging(); var ct = console.RegisterCancellationHandler(); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/TwinCATCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/TwinCATCommandBase.cs index 17deb44..42c9782 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/TwinCATCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/TwinCATCommandBase.cs @@ -1,13 +1,15 @@ using CliFx.Attributes; +using CliFx.Exceptions; using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli; /// /// Base for every TwinCAT CLI command. Carries the AMS target options -/// (--ams-net-id + --ams-port) + the notification-mode toggle that the -/// driver itself takes. Exposes so each command can build a -/// single-device / single-tag from flag input. +/// (--ams-net-id + --ams-port) + the per-call timeout. Commands that build +/// a single-device / single-tag from flag input inherit +/// from instead — that intermediate adds the +/// --poll-only flag and the BuildOptions helper. /// public abstract class TwinCATCommandBase : DriverCommandBase { @@ -23,16 +25,19 @@ public abstract class TwinCATCommandBase : DriverCommandBase [CommandOption("timeout-ms", Description = "Per-operation timeout in ms (default 5000).")] public int TimeoutMs { get; init; } = 5000; - [CommandOption("poll-only", Description = - "Disable native ADS notifications and fall through to the shared PollGroupEngine " + - "(same as setting UseNativeNotifications=false in a real driver config).")] - public bool PollOnly { get; init; } - - /// + /// + /// The per-operation timeout, projected from . The CliFx + /// init accessor required by the abstract base property is intentionally a + /// no-op: is the only source of truth, so any value an + /// `init` initialiser supplies to directly is silently + /// dropped. Do NOT add a backing field "fixing" the empty body — it would diverge + /// from and the two would drift on every refactor + /// (Driver.TwinCAT.Cli-007). + /// public override TimeSpan Timeout { get => TimeSpan.FromMilliseconds(TimeoutMs); - init { /* driven by TimeoutMs */ } + init { /* see XML summary — driven by TimeoutMs */ } } /// @@ -41,22 +46,29 @@ public abstract class TwinCATCommandBase : DriverCommandBase /// protected string Gateway => $"ads://{AmsNetId}:{AmsPort}"; - /// - /// Build a with the AMS target this base collected + - /// the tag list a subclass supplies. Probe disabled, controller-browse disabled, - /// native notifications toggled by . - /// - protected TwinCATDriverOptions BuildOptions(IReadOnlyList tags) => new() - { - Devices = [new TwinCATDeviceOptions( - HostAddress: Gateway, - DeviceName: $"cli-{AmsNetId}:{AmsPort}")], - Tags = tags, - Timeout = Timeout, - Probe = new TwinCATProbeOptions { Enabled = false }, - UseNativeNotifications = !PollOnly, - EnableControllerBrowse = false, - }; - protected string DriverInstanceId => $"twincat-cli-{AmsNetId}:{AmsPort}"; + + /// + /// Validates the numeric options every TwinCAT CLI command shares (timeout + AMS port). + /// Subclasses override and call base.Validate() first to add their own range + /// checks. Throwing here surfaces a clean CliFx one-line error before the driver gets + /// a chance to fail with an opaque transport error (Driver.TwinCAT.Cli-001). + /// + protected virtual void Validate() + { + if (TimeoutMs <= 0) + throw new CommandException( + $"--timeout-ms must be greater than 0 (got {TimeoutMs})."); + if (AmsPort is <= 0 or > 65535) + throw new CommandException( + $"--ams-port must be in the range 1..65535 (got {AmsPort})."); + } + + // ---- Test hooks ---- + // Protected members are exposed to the test assembly through these internal accessors so the + // test project can cover Gateway / DriverInstanceId composition + range validation without + // needing reflection on every assertion (Driver.TwinCAT.Cli-006). + internal string GatewayForTest => Gateway; + internal string DriverInstanceIdForTest => DriverInstanceId; + internal void ValidateForTest() => Validate(); } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/TwinCATTagCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/TwinCATTagCommandBase.cs new file mode 100644 index 0000000..308a551 --- /dev/null +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/TwinCATTagCommandBase.cs @@ -0,0 +1,40 @@ +using CliFx.Attributes; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli; + +/// +/// Intermediate base for the four TwinCAT CLI commands that build a single-device / +/// single-tag — probe, read, write, +/// subscribe. Adds the --poll-only flag (relevant only when the driver is +/// about to register native ADS notifications, which is why it does NOT live on the +/// browse command — Driver.TwinCAT.Cli-004) and the BuildOptions helper that +/// assembles the driver-side options record. +/// +public abstract class TwinCATTagCommandBase : TwinCATCommandBase +{ + [CommandOption("poll-only", Description = + "Disable native ADS notifications and fall through to the shared PollGroupEngine " + + "(same as setting UseNativeNotifications=false in a real driver config).")] + public bool PollOnly { get; init; } + + /// + /// Build a with the AMS target this base collected + + /// the tag list a subclass supplies. Probe disabled, controller-browse disabled, + /// native notifications toggled by . + /// + protected TwinCATDriverOptions BuildOptions(IReadOnlyList tags) => new() + { + Devices = [new TwinCATDeviceOptions( + HostAddress: Gateway, + DeviceName: $"cli-{AmsNetId}:{AmsPort}")], + Tags = tags, + Timeout = Timeout, + Probe = new TwinCATProbeOptions { Enabled = false }, + UseNativeNotifications = !PollOnly, + EnableControllerBrowse = false, + }; + + // ---- Test hook ---- + internal TwinCATDriverOptions BuildOptionsForTest(IReadOnlyList tags) + => BuildOptions(tags); +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/BrowseCommandFilterTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/BrowseCommandFilterTests.cs new file mode 100644 index 0000000..d4fdebf --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/BrowseCommandFilterTests.cs @@ -0,0 +1,123 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests; + +/// +/// Driver.TwinCAT.Cli-006: covers the prefix / max filtering and the RO/RW classification +/// logic inside . The collecting builder + the filter pipeline +/// are pure — no ADS contact required — so they unit-test cleanly. +/// +[Trait("Category", "Unit")] +public sealed class BrowseCommandFilterTests +{ + private static DriverAttributeInfo Info(SecurityClassification cls, DriverDataType dt = DriverDataType.Int32) + => new( + FullName: "ignored", + DriverDataType: dt, + IsArray: false, + ArrayDim: null, + SecurityClass: cls, + IsHistorized: false); + + [Fact] + public void Collector_records_each_variable_in_call_order() + { + var builder = new BrowseCommand.CollectingAddressSpaceBuilder(); + builder.Variable("GVL.A", "GVL.A", Info(SecurityClassification.Operate)); + builder.Variable("GVL.B", "GVL.B", Info(SecurityClassification.ViewOnly)); + + builder.Variables.Count.ShouldBe(2); + builder.Variables[0].BrowseName.ShouldBe("GVL.A"); + builder.Variables[1].BrowseName.ShouldBe("GVL.B"); + } + + [Fact] + public void Folder_returns_same_builder_so_nested_variables_land_in_one_flat_list() + { + // BrowseCommand expects a flat list — TwinCAT's flat-mode symbol walk doesn't nest + // into sub-folders, but DiscoverAsync may still call Folder() before Variable(). + var builder = new BrowseCommand.CollectingAddressSpaceBuilder(); + var nested = builder.Folder("Discovered", "Discovered"); + nested.Variable("GVL.X", "GVL.X", Info(SecurityClassification.Operate)); + + builder.Variables.Count.ShouldBe(1); + builder.Variables[0].BrowseName.ShouldBe("GVL.X"); + } + + [Fact] + public void FilterAndLimit_empty_prefix_returns_everything_up_to_max() + { + var symbols = new List<(string BrowseName, DriverAttributeInfo Info)> + { + ("GVL_A.x", Info(SecurityClassification.Operate)), + ("GVL_B.y", Info(SecurityClassification.ViewOnly)), + ("MAIN.z", Info(SecurityClassification.Operate)), + }; + + var matched = BrowseCommand.FilterByPrefix(symbols, prefix: null); + matched.Count.ShouldBe(3); + } + + [Fact] + public void FilterAndLimit_prefix_is_case_sensitive() + { + var symbols = new List<(string BrowseName, DriverAttributeInfo Info)> + { + ("GVL_Fixture.x", Info(SecurityClassification.Operate)), + ("gvl_fixture.y", Info(SecurityClassification.Operate)), + ("MAIN.z", Info(SecurityClassification.Operate)), + }; + + var matched = BrowseCommand.FilterByPrefix(symbols, prefix: "GVL_Fixture"); + + matched.Count.ShouldBe(1); + matched[0].BrowseName.ShouldBe("GVL_Fixture.x"); + } + + [Fact] + public void FilterAndLimit_zero_max_means_unbounded() + { + var symbols = Enumerable.Range(0, 10) + .Select(i => ($"GVL.S{i}", Info(SecurityClassification.Operate))) + .ToList(); + + var limit = BrowseCommand.PrintLimit(symbols.Count, max: 0); + limit.ShouldBe(10); + } + + [Fact] + public void FilterAndLimit_caps_to_max_when_more_matched() + { + BrowseCommand.PrintLimit(matchedCount: 1000, max: 50).ShouldBe(50); + } + + [Fact] + public void FilterAndLimit_does_not_pad_to_max_when_fewer_matched() + { + BrowseCommand.PrintLimit(matchedCount: 3, max: 50).ShouldBe(3); + } + + [Fact] + public void AccessTag_returns_RO_for_ViewOnly_attribute() + { + BrowseCommand.AccessTag(Info(SecurityClassification.ViewOnly)).ShouldBe("RO"); + } + + [Theory] + [InlineData(SecurityClassification.FreeAccess)] + [InlineData(SecurityClassification.Operate)] + [InlineData(SecurityClassification.SecuredWrite)] + [InlineData(SecurityClassification.VerifiedWrite)] + [InlineData(SecurityClassification.Tune)] + [InlineData(SecurityClassification.Configure)] + public void AccessTag_returns_RW_for_anything_except_ViewOnly(SecurityClassification cls) + { + // BrowseCommand's display logic flips ViewOnly = RO, everything else = RW. The real + // ACL is enforced server-side from the SecurityClassification — the CLI label is just + // a coarse "is this writable from any tier" indicator. + BrowseCommand.AccessTag(Info(cls)).ShouldBe("RW"); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/SubscribeCommandMechanismTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/SubscribeCommandMechanismTests.cs new file mode 100644 index 0000000..29816ea --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/SubscribeCommandMechanismTests.cs @@ -0,0 +1,37 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests; + +/// +/// Driver.TwinCAT.Cli-003: the subscribe banner mechanism label is derived from the +/// the driver actually returned, not from +/// the --poll-only flag. That way the banner cannot disagree with what the driver +/// did even if a future fallback path lands the subscription somewhere unexpected. +/// +[Trait("Category", "Unit")] +public sealed class SubscribeCommandMechanismTests +{ + private sealed record StubHandle(string DiagnosticId) : ISubscriptionHandle; + + [Theory] + [InlineData("twincat-native-sub-1")] + [InlineData("twincat-native-sub-42")] + [InlineData("twincat-native-sub-9223372036854775807")] + public void DescribeMechanism_returns_ADS_notification_for_native_handle(string diagId) + { + SubscribeCommand.DescribeMechanism(new StubHandle(diagId)).ShouldBe("ADS notification"); + } + + [Theory] + [InlineData("pollgroup-1")] + [InlineData("modbus-poll-7")] + [InlineData("")] + [InlineData("TWINCAT-NATIVE-SUB-1")] // ordinal comparison — uppercase prefix does NOT match. + public void DescribeMechanism_returns_polling_for_anything_else(string diagId) + { + SubscribeCommand.DescribeMechanism(new StubHandle(diagId)).ShouldBe("polling"); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/TwinCATCommandBaseTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/TwinCATCommandBaseTests.cs new file mode 100644 index 0000000..f7e2327 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/TwinCATCommandBaseTests.cs @@ -0,0 +1,238 @@ +using System.Reflection; +using CliFx.Attributes; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests; + +/// +/// Covers / wiring: +/// the canonical gateway string, the driver instance id, the BuildOptions field projection +/// (Driver.TwinCAT.Cli-006), and the up-front range validation guards +/// (Driver.TwinCAT.Cli-001). +/// +[Trait("Category", "Unit")] +public sealed class TwinCATCommandBaseTests +{ + [Fact] + public void Gateway_uses_canonical_ads_scheme_with_port() + { + var cmd = new ProbeCommand + { + AmsNetId = "192.168.1.40.1.1", + AmsPort = 851, + SymbolPath = "MAIN.bRunning", + }; + cmd.GatewayForTest.ShouldBe("ads://192.168.1.40.1.1:851"); + } + + [Fact] + public void Gateway_round_trips_through_TwinCATAmsAddress_TryParse() + { + // Driver.TwinCAT.Cli-006: a regression in the Gateway string breaks every command + // because the driver's TwinCATAmsAddress.TryParse refuses anything not shaped + // ads://{netId}:{port}. + var cmd = new ProbeCommand + { + AmsNetId = "5.23.91.23.1.1", + AmsPort = 852, + SymbolPath = "MAIN.x", + }; + var parsed = TwinCAT.TwinCATAmsAddress.TryParse(cmd.GatewayForTest); + parsed.ShouldNotBeNull(); + parsed!.NetId.ShouldBe("5.23.91.23.1.1"); + parsed.Port.ShouldBe(852); + } + + [Fact] + public void DriverInstanceId_includes_ams_target() + { + var cmd = new ProbeCommand + { + AmsNetId = "127.0.0.1.1.1", + AmsPort = 851, + SymbolPath = "MAIN.x", + }; + cmd.DriverInstanceIdForTest.ShouldBe("twincat-cli-127.0.0.1.1.1:851"); + } + + [Fact] + public void Timeout_is_projection_of_TimeoutMs_and_init_is_noop() + { + var cmd = new ProbeCommand + { + AmsNetId = "127.0.0.1.1.1", + TimeoutMs = 7777, + SymbolPath = "MAIN.x", + }; + cmd.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7777)); + } + + [Fact] + public void BuildOptions_wires_device_tags_timeout_and_disables_probe() + { + // Driver.TwinCAT.Cli-006: cover the property-by-property wiring that the four runtime + // commands depend on. Probe must be disabled (CLI is one-shot — the probe loop would + // race the operator's own reads) and controller-browse must stay off. + var cmd = new ProbeCommand + { + AmsNetId = "10.0.0.1.1.1", + AmsPort = 851, + TimeoutMs = 4321, + SymbolPath = "MAIN.x", + }; + var tag = new TwinCAT.TwinCATTagDefinition( + Name: "n1", + DeviceHostAddress: cmd.GatewayForTest, + SymbolPath: "MAIN.x", + DataType: TwinCAT.TwinCATDataType.DInt, + Writable: false); + + var options = cmd.BuildOptionsForTest([tag]); + + options.Devices.Count.ShouldBe(1); + options.Devices[0].HostAddress.ShouldBe("ads://10.0.0.1.1.1:851"); + options.Devices[0].DeviceName.ShouldBe("cli-10.0.0.1.1.1:851"); + options.Tags.ShouldBe([tag]); + options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(4321)); + options.Probe.Enabled.ShouldBeFalse(); + options.EnableControllerBrowse.ShouldBeFalse(); + // Default UseNativeNotifications = true (no --poll-only). + options.UseNativeNotifications.ShouldBeTrue(); + } + + [Fact] + public void BuildOptions_PollOnly_flips_UseNativeNotifications_off() + { + var cmd = new ProbeCommand + { + AmsNetId = "10.0.0.1.1.1", + SymbolPath = "MAIN.x", + PollOnly = true, + }; + cmd.BuildOptionsForTest([]).UseNativeNotifications.ShouldBeFalse(); + } + + // ---- Driver.TwinCAT.Cli-001 (range validation) ---- + + [Fact] + public void Validate_rejects_zero_timeout() + { + var cmd = new ProbeCommand + { + AmsNetId = "127.0.0.1.1.1", + SymbolPath = "MAIN.x", + TimeoutMs = 0, + }; + var ex = Should.Throw(() => cmd.ValidateForTest()); + ex.Message.ShouldContain("--timeout-ms"); + } + + [Fact] + public void Validate_rejects_negative_timeout() + { + var cmd = new ProbeCommand + { + AmsNetId = "127.0.0.1.1.1", + SymbolPath = "MAIN.x", + TimeoutMs = -1, + }; + Should.Throw(() => cmd.ValidateForTest()); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(65536)] + [InlineData(100000)] + public void Validate_rejects_out_of_range_ams_port(int port) + { + var cmd = new ProbeCommand + { + AmsNetId = "127.0.0.1.1.1", + SymbolPath = "MAIN.x", + AmsPort = port, + }; + var ex = Should.Throw(() => cmd.ValidateForTest()); + ex.Message.ShouldContain("--ams-port"); + } + + [Theory] + [InlineData(1)] + [InlineData(801)] + [InlineData(851)] + [InlineData(65535)] + public void Validate_accepts_in_range_ams_port(int port) + { + var cmd = new ProbeCommand + { + AmsNetId = "127.0.0.1.1.1", + SymbolPath = "MAIN.x", + AmsPort = port, + }; + Should.NotThrow(() => cmd.ValidateForTest()); + } + + [Fact] + public void SubscribeCommand_validate_rejects_zero_interval() + { + var cmd = new SubscribeCommand + { + AmsNetId = "127.0.0.1.1.1", + SymbolPath = "MAIN.x", + IntervalMs = 0, + }; + var ex = Should.Throw(() => cmd.ValidateForTest()); + ex.Message.ShouldContain("--interval-ms"); + } + + [Fact] + public void SubscribeCommand_validate_rejects_negative_interval() + { + var cmd = new SubscribeCommand + { + AmsNetId = "127.0.0.1.1.1", + SymbolPath = "MAIN.x", + IntervalMs = -100, + }; + Should.Throw(() => cmd.ValidateForTest()); + } + + // ---- Driver.TwinCAT.Cli-004 (PollOnly off BrowseCommand surface) ---- + + [Fact] + public void BrowseCommand_does_not_expose_poll_only_flag() + { + // Driver.TwinCAT.Cli-004: the flag has no observable effect on browse — surfacing it + // misleads users. After the refactor, PollOnly lives on an intermediate base shared + // only by the commands that actually consume native ADS notifications. + var props = typeof(BrowseCommand) + .GetProperties(BindingFlags.Public | BindingFlags.Instance); + props.ShouldNotContain(p => p.Name == "PollOnly"); + } + + [Fact] + public void ProbeCommand_still_exposes_poll_only_flag() + { + // Probe / Read / Write / Subscribe all build TwinCATDriverOptions and so still take + // the --poll-only toggle. + var props = typeof(ProbeCommand) + .GetProperties(BindingFlags.Public | BindingFlags.Instance); + props.ShouldContain(p => p.Name == "PollOnly"); + } + + // ---- Driver.TwinCAT.Cli-005 (probe --type short alias) ---- + + [Fact] + public void ProbeCommand_type_option_carries_short_alias_t() + { + // Driver.TwinCAT.Cli-005: --type on read/write/subscribe takes the -t short alias; + // probe must match so muscle memory works the same way across all four verbs. + var dataTypeProp = typeof(ProbeCommand).GetProperty("DataType"); + dataTypeProp.ShouldNotBeNull(); + var attr = dataTypeProp!.GetCustomAttribute(); + attr.ShouldNotBeNull(); + attr!.ShortName.ShouldBe('t'); + } +}