dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(driver-focas-cli): resolve Low code-review findings (Driver.FOCAS.Cli-001,002,003,004; -005 deferred)

Browse Source 
- Driver.FOCAS.Cli-001: WriteCommand.ParseValue now wraps numeric
  FormatException / OverflowException as CliFx CommandException with
  the offending value.
- Driver.FOCAS.Cli-002: SubscribeCommand's OnDataChange handler and the
  banner both take a writeLock so notification-callback and main-thread
  writes can't interleave; handler exceptions are warn-and-swallow.
- Driver.FOCAS.Cli-003: FocasCommandBase.ValidateOptions rejects
  --cnc-port outside 1..65535, non-positive --timeout-ms, and
  non-positive --interval-ms; ExecuteAsync calls it first.
- Driver.FOCAS.Cli-004: 'await using var driver' is the sole driver
  disposal path; dropped the redundant explicit await ShutdownAsync.
- Driver.FOCAS.Cli-005 (Deferred): the fix lives in
  Driver.Cli.Common.SnapshotFormatter — explicitly naming the
  status-code shortlist there benefits every driver CLI. Left as a
  Driver.Cli.Common follow-up.
- Registered the new tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests
  project in ZB.MOM.WW.OtOpcUa.slnx.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 11:11:55 -04:00
parent 2a941b255f
commit 6923be3aa2
14 changed files with 629 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ProbeCommand.cs
View File

@@ -24,6 +24,8 @@ public sealed class ProbeCommand : FocasCommandBase
public override async ValueTask ExecuteAsync(IConsole console)
{
ConfigureLogging();
// Driver.FOCAS.Cli-003: validate numeric option ranges before any driver work.
ValidateOptions();
var ct = console.RegisterCancellationHandler();
var probeTag = new FocasTagDefinition(
@@ -34,24 +36,20 @@ public sealed class ProbeCommand : FocasCommandBase
Writable: false);
var options = BuildOptions([probeTag]);
// Driver.FOCAS.Cli-004: `await using` is the sole disposal mechanism — FocasDriver.DisposeAsync
// already invokes ShutdownAsync, so a redundant explicit ShutdownAsync(CancellationToken.None)
// in a finally block ran shutdown twice. The await-using on the next line is enough.
await using var driver = new FocasDriver(options, DriverInstanceId);
try
{
await driver.InitializeAsync("{}", ct);
var snapshot = await driver.ReadAsync(["__probe"], ct);
var health = driver.GetHealth();
await driver.InitializeAsync("{}", ct);
var snapshot = await driver.ReadAsync(["__probe"], ct);
var health = driver.GetHealth();
await console.Output.WriteLineAsync($"CNC: {CncHost}:{CncPort}");
await console.Output.WriteLineAsync($"Series: {Series}");
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(Address, snapshot[0]));
}
finally
{
await driver.ShutdownAsync(CancellationToken.None);
}
await console.Output.WriteLineAsync($"CNC: {CncHost}:{CncPort}");
await console.Output.WriteLineAsync($"Series: {Series}");
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(Address, snapshot[0]));
}
}

18
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ReadCommand.cs
View File

@@ -23,6 +23,8 @@ public sealed class ReadCommand : FocasCommandBase
public override async ValueTask ExecuteAsync(IConsole console)
{
ConfigureLogging();
// Driver.FOCAS.Cli-003: validate numeric option ranges before any driver work.
ValidateOptions();
var ct = console.RegisterCancellationHandler();
var tagName = SynthesiseTagName(Address, DataType);
@@ -34,17 +36,13 @@ public sealed class ReadCommand : FocasCommandBase
Writable: false);
var options = BuildOptions([tag]);
// Driver.FOCAS.Cli-004: `await using` is the sole disposal mechanism — FocasDriver.DisposeAsync
// already invokes ShutdownAsync, so a redundant explicit ShutdownAsync(CancellationToken.None)
// in a finally block ran shutdown twice. The await-using on the next line is enough.
await using var driver = new FocasDriver(options, DriverInstanceId);
try
{
await driver.InitializeAsync("{}", ct);
var snapshot = await driver.ReadAsync([tagName], ct);
await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
}
finally
{
await driver.ShutdownAsync(CancellationToken.None);
}
await driver.InitializeAsync("{}", ct);
var snapshot = await driver.ReadAsync([tagName], ct);
await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
}
internal static string SynthesiseTagName(string address, FocasDataType type)

59
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/SubscribeCommand.cs
View File

@@ -25,6 +25,10 @@ public sealed class SubscribeCommand : FocasCommandBase
public override async ValueTask ExecuteAsync(IConsole console)
{
ConfigureLogging();
// Driver.FOCAS.Cli-003: validate numeric option ranges (including the subscribe-only
// --interval-ms) before any driver work so a zero/negative interval surfaces as a
// clean CommandException rather than a tight-spinning poll loop.
ValidateOptions(IntervalMs);
var ct = console.RegisterCancellationHandler();
var tagName = ReadCommand.SynthesiseTagName(Address, DataType);
@@ -36,24 +40,59 @@ public sealed class SubscribeCommand : FocasCommandBase
Writable: false);
var options = BuildOptions([tag]);
// Driver.FOCAS.Cli-004: `await using` is the sole driver-disposal mechanism — FocasDriver.DisposeAsync
// already invokes ShutdownAsync, so a redundant ShutdownAsync(CancellationToken.None) in finally
// ran shutdown twice. Only UnsubscribeAsync stays in the finally block — that's a subscription
// lifecycle concern that is not part of driver disposal.
await using var driver = new FocasDriver(options, DriverInstanceId);
// Driver.FOCAS.Cli-002: serialize console writes from the PollGroupEngine background
// thread so overlapping poll ticks (and the "Subscribed to ..." banner from the CliFx
// invocation thread) can't interleave partial lines.
var writeLock = new object();
ISubscriptionHandle? handle = null;
try
{
await driver.InitializeAsync("{}", ct);
// Driver.FOCAS.Cli-002: route every data-change event to the CliFx console (not
// System.Console — the analyzer flags it + IConsole is the testable abstraction).
// The handler is synchronous because OnDataChange is raised from a driver
// background thread; the IConsole.Output writer is not documented as thread-safe
// so we serialize against the banner write via writeLock.
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);
// Swallow + log write failures so a transient stdout error (closed pipe, IO
// exception on a redirected stream) cannot tear down the poll-engine
// background loop. Without this guard the unhandled exception would fault
// the long-running subscribe.
try
{
var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " +
$"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " +
$"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})";
lock (writeLock)
{
console.Output.WriteLine(line);
}
}
catch (Exception ex)
{
Serilog.Log.Logger.Warning(ex,
"SubscribeCommand: console write failed for {Tag}; continuing poll loop.",
e.FullReference);
}
};
handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct);
await console.Output.WriteLineAsync(
$"Subscribed to {Address} @ {IntervalMs}ms. Ctrl+C to stop.");
// Driver.FOCAS.Cli-002: hold the lock around the banner write so the first
// poll-driven change line from the driver tick thread can't interleave with
// this banner.
lock (writeLock)
{
console.Output.WriteLine(
$"Subscribed to {Address} @ {IntervalMs}ms. Ctrl+C to stop.");
}
try
{
await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct);
@@ -67,10 +106,16 @@ public sealed class SubscribeCommand : FocasCommandBase
{
if (handle is not null)
{
// Driver.FOCAS.Cli-002: detach the OnDataChange handler before unsubscribe +
// disposal for symmetry with the handle teardown, so a future refactor that
// reuses the driver after the subscribe verb returns wouldn't leak a
// dangling subscription.
// (Single anonymous handler instance is captured implicitly by `await using`
// disposing the driver immediately afterwards; the unsubscribe + dispose
// sequence is what really cleans up here.)
try { await driver.UnsubscribeAsync(handle, CancellationToken.None); }
catch { /* teardown best-effort */ }
}
await driver.ShutdownAsync(CancellationToken.None);
}
}
}

63
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/WriteCommand.cs
View File

@@ -29,6 +29,10 @@ public sealed class WriteCommand : FocasCommandBase
public override async ValueTask ExecuteAsync(IConsole console)
{
ConfigureLogging();
// Driver.FOCAS.Cli-003: validate numeric option ranges before any driver work so
// a zero/negative port/timeout surfaces as a clean CommandException rather than an
// opaque downstream exception.
ValidateOptions();
var ct = console.RegisterCancellationHandler();
var tagName = ReadCommand.SynthesiseTagName(Address, DataType);
@@ -42,30 +46,49 @@ public sealed class WriteCommand : FocasCommandBase
var parsed = ParseValue(Value, DataType);
// Driver.FOCAS.Cli-004: `await using` is the sole disposal mechanism — FocasDriver.DisposeAsync
// already invokes ShutdownAsync, so a redundant explicit ShutdownAsync(CancellationToken.None)
// in a finally block ran shutdown twice. The await-using on the next line is enough.
await using var driver = new FocasDriver(options, DriverInstanceId);
try
{
await driver.InitializeAsync("{}", ct);
var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct);
await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0]));
}
finally
{
await driver.ShutdownAsync(CancellationToken.None);
}
await driver.InitializeAsync("{}", ct);
var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct);
await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0]));
}
internal static object ParseValue(string raw, FocasDataType type) => type switch
/// <summary>Parse <c>--value</c> per <see cref="FocasDataType"/>, invariant culture throughout.</summary>
/// <remarks>
/// Driver.FOCAS.Cli-001: numeric parses are wrapped so that malformed input
/// (<see cref="FormatException"/> / <see cref="OverflowException"/>) surfaces
/// as a clean <see cref="CliFx.Exceptions.CommandException"/> rather than a raw
/// .NET stack trace — matching the friendly message the Bit path already produces.
/// </remarks>
internal static object ParseValue(string raw, FocasDataType type)
{
FocasDataType.Bit => ParseBool(raw),
FocasDataType.Byte => sbyte.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Int16 => short.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Int32 => int.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Float32 => float.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Float64 => double.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.String => raw,
_ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."),
};
if (type == FocasDataType.Bit) return ParseBool(raw);
if (type == FocasDataType.String) return raw;
try
{
return type switch
{
FocasDataType.Byte => (object)sbyte.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Int16 => (object)short.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Int32 => (object)int.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Float32 => (object)float.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Float64 => (object)double.Parse(raw, CultureInfo.InvariantCulture),
_ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."),
};
}
catch (FormatException ex)
{
throw new CliFx.Exceptions.CommandException(
$"Value '{raw}' is not a valid {type}: {ex.Message}");
}
catch (OverflowException ex)
{
throw new CliFx.Exceptions.CommandException(
$"Value '{raw}' is out of range for {type}: {ex.Message}");
}
}
private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch
{

22
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/FocasCommandBase.cs
View File

@@ -54,4 +54,26 @@ public abstract class FocasCommandBase : DriverCommandBase
};
protected string DriverInstanceId => $"focas-cli-{CncHost}:{CncPort}";
/// <summary>
/// Driver.FOCAS.Cli-003: validate numeric option ranges at the CLI boundary so a
/// zero/negative <c>--cnc-port</c>, <c>--timeout-ms</c>, or <c>--interval-ms</c>
/// surfaces as a clean <see cref="CliFx.Exceptions.CommandException"/> rather than
/// either an opaque downstream exception (invalid <c>focas://host:&lt;n&gt;</c> /
/// zero <c>TimeSpan</c>) or a tight-spinning poll loop. The <c>--interval-ms</c>
/// option is subscribe-only — pass <c>null</c> for probe/read/write so this
/// helper can be a single shared validator.
/// </summary>
protected void ValidateOptions(int? intervalMs = null)
{
if (CncPort < 1 || CncPort > 65535)
throw new CliFx.Exceptions.CommandException(
$"--cnc-port must be in the range 1..65535 (got {CncPort}).");
if (TimeoutMs <= 0)
throw new CliFx.Exceptions.CommandException(
$"--timeout-ms must be positive (got {TimeoutMs}).");
if (intervalMs is { } iv && iv <= 0)
throw new CliFx.Exceptions.CommandException(
$"--interval-ms must be positive (got {iv}).");
}
}

4
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.csproj
View File

@@ -22,6 +22,10 @@
<ProjectReference Include="..\..\ZB.MOM.WW.OtOpcUa.Driver.FOCAS\ZB.MOM.WW.OtOpcUa.Driver.FOCAS.csproj"/>
</ItemGroup>
<ItemGroup>
<InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests"/>
</ItemGroup>
<!-- CLI runs the managed WireFocasClient and talks to the CNC over TCP:8193
directly — no Fwlib64.dll copy step needed. -->