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

fix(driver-modbus-cli): resolve Low code-review findings (Driver.Modbus.Cli-003,004,005,006,007,008)

Browse Source 
- Driver.Modbus.Cli-003: ModbusCommandBase.ValidateEndpoint rejects
  --port outside 1..65535, non-positive --timeout-ms, and --unit-id
  outside 1..247.
- Driver.Modbus.Cli-004: wrapped SubscribeCommand's OnDataChange handler
  body in a try/catch (warn-and-swallow) and serialised the console
  write through a lock.
- Driver.Modbus.Cli-005: Probe / Read / Write now catch the
  cancellation-during-init OperationCanceledException and print
  'Cancelled.' instead of dumping a stack trace.
- Driver.Modbus.Cli-006: ProbeCommand.ComputeVerdict derives the headline
  from BOTH the driver state and the probe snapshot's OPC UA quality
  class so the headline can't disagree with the wire result.
- Driver.Modbus.Cli-007: docs/Driver.Modbus.Cli.md carries an explicit
  'CLI scope' callout — the address-string grammar is a DriverConfig
  JSON feature; the CLI takes the structured triple only.
- Driver.Modbus.Cli-008: pinned BuildOptions, ValidateEndpoint, the
  region-validation guards, ComputeVerdict, and the cancellation-during-
  initialize paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 08:35:05 -04:00
parent f2ee027145
commit 80ef8806e0
11 changed files with 533 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs
View File

@@ -1,5 +1,6 @@
using CliFx.Attributes;
using CliFx.Infrastructure;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common;
namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands;
@@ -21,6 +22,7 @@ public sealed class ProbeCommand : ModbusCommandBase
public override async ValueTask ExecuteAsync(IConsole console)
{
ConfigureLogging();
ValidateEndpoint();
var ct = console.RegisterCancellationHandler();
// Build with one probe tag + Probe.Enabled=false so InitializeAsync connects the
@@ -39,17 +41,59 @@ public sealed class ProbeCommand : ModbusCommandBase
var snapshot = await driver.ReadAsync(["__probe"], ct);
var health = driver.GetHealth();
// Driver.Modbus.Cli-006: derive the headline verdict from BOTH the driver state
// AND the probe-read StatusCode so the operator never sees the previous
// contradictory pair (`Health: Healthy` over a Bad snapshot line). The bare
// driver state is still printed below for diagnostics, but the verdict is what
// the operator scans first.
var verdict = ComputeVerdict(health.State, snapshot[0].StatusCode);
await console.Output.WriteLineAsync($"Host: {Host}:{Port} (unit {UnitId})");
await console.Output.WriteLineAsync($"Health: {health.State}");
await console.Output.WriteLineAsync($"Verdict: {verdict}");
await console.Output.WriteLineAsync($"Driver state: {health.State}");
if (health.LastError is { } err)
await console.Output.WriteLineAsync($"Last error: {err}");
await console.Output.WriteLineAsync();
await console.Output.WriteLineAsync(
SnapshotFormatter.Format($"HR[{ProbeAddress}]", snapshot[0]));
}
catch (OperationCanceledException) when (ct.IsCancellationRequested)
{
// Driver.Modbus.Cli-005: Ctrl+C during InitializeAsync — exit quietly so CliFx
// does not render a full stack trace for a user-initiated cancellation.
await console.Output.WriteLineAsync("Cancelled.");
}
finally
{
await driver.ShutdownAsync(CancellationToken.None);
}
}
/// <summary>
/// Driver.Modbus.Cli-006: combine the driver-side <see cref="DriverState"/> with the
/// probe snapshot's OPC UA <c>StatusCode</c> into a single headline verdict. Order
/// of precedence:
/// <list type="number">
/// <item><c>FAIL</c> — driver did not reach <see cref="DriverState.Healthy"/>
/// (Faulted / Reconnecting / Unknown) OR the snapshot reports Bad quality.</item>
/// <item><c>DEGRADED</c> — driver Healthy but the snapshot quality is Uncertain.</item>
/// <item><c>OK</c> — driver Healthy and snapshot Good.</item>
/// </list>
/// </summary>
public static string ComputeVerdict(DriverState state, uint statusCode)
{
// OPC UA StatusCode top 2 bits encode the quality class:
// 0x00xxxxxx → Good, 0x40xxxxxx → Uncertain, 0x80xxxxxx / 0xC0xxxxxx → Bad.
var qualityClass = statusCode & 0xC0000000u;
var snapshotGood = qualityClass == 0x00000000u;
var snapshotUncertain = qualityClass == 0x40000000u;
if (state != DriverState.Healthy || !snapshotGood && !snapshotUncertain)
return $"FAIL (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})";
if (snapshotUncertain)
return $"DEGRADED (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})";
return $"OK (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})";
}
}

7
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs
View File

@@ -46,6 +46,7 @@ public sealed class ReadCommand : ModbusCommandBase
public override async ValueTask ExecuteAsync(IConsole console)
{
ConfigureLogging();
ValidateEndpoint();
var ct = console.RegisterCancellationHandler();
var tagName = SynthesiseTagName(Region, Address, DataType);
@@ -68,6 +69,12 @@ public sealed class ReadCommand : ModbusCommandBase
var snapshot = await driver.ReadAsync([tagName], ct);
await console.Output.WriteLineAsync(SnapshotFormatter.Format(tagName, snapshot[0]));
}
catch (OperationCanceledException) when (ct.IsCancellationRequested)
{
// Driver.Modbus.Cli-005: Ctrl+C during driver connect/read — exit quietly so
// CliFx does not render a full stack trace for a user-initiated cancellation.
await console.Output.WriteLineAsync("Cancelled.");
}
finally
{
await driver.ShutdownAsync(CancellationToken.None);

28
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs
View File

@@ -53,6 +53,7 @@ public sealed class SubscribeCommand : ModbusCommandBase
public override async ValueTask ExecuteAsync(IConsole console)
{
ConfigureLogging();
ValidateEndpoint();
var ct = console.RegisterCancellationHandler();
var tagName = ReadCommand.SynthesiseTagName(Region, Address, DataType);
@@ -69,6 +70,9 @@ public sealed class SubscribeCommand : ModbusCommandBase
var options = BuildOptions([tag]);
await using var driver = new ModbusDriver(options, DriverInstanceId);
// Driver.Modbus.Cli-004: serialize console writes from the PollGroupEngine background
// thread so overlapping poll ticks can't interleave partial lines.
var writeLock = new object();
ISubscriptionHandle? handle = null;
try
{
@@ -78,10 +82,26 @@ public sealed class SubscribeCommand : ModbusCommandBase
// analyzer flags it + IConsole is the testable abstraction).
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.Modbus.Cli-004: 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);

7
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs
View File

@@ -54,6 +54,7 @@ public sealed class WriteCommand : ModbusCommandBase
public override async ValueTask ExecuteAsync(IConsole console)
{
ConfigureLogging();
ValidateEndpoint();
var ct = console.RegisterCancellationHandler();
if (Region is not (ModbusRegion.Coils or ModbusRegion.HoldingRegisters))
@@ -92,6 +93,12 @@ public sealed class WriteCommand : ModbusCommandBase
var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct);
await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(tagName, results[0]));
}
catch (OperationCanceledException) when (ct.IsCancellationRequested)
{
// Driver.Modbus.Cli-005: Ctrl+C during driver connect/write — exit quietly so
// CliFx does not render a full stack trace for a user-initiated cancellation.
await console.Output.WriteLineAsync("Cancelled.");
}
finally
{
await driver.ShutdownAsync(CancellationToken.None);

30
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs
View File

@@ -1,4 +1,5 @@
using CliFx.Attributes;
using CliFx.Exceptions;
using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common;
namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli;
@@ -57,4 +58,33 @@ public abstract class ModbusCommandBase : DriverCommandBase
/// multiple endpoints in parallel can distinguish the logs.
/// </summary>
protected string DriverInstanceId => $"modbus-cli-{Host}:{Port}";
/// <summary>
/// Driver.Modbus.Cli-003: validate the endpoint flags at parse time so the operator
/// gets a clear CliFx error instead of an opaque socket / argument exception thrown
/// deep inside the driver. Ranges:
/// <list type="bullet">
/// <item><c>--port</c>: 1..65535 (IANA TCP port space, excludes the
/// "any" sentinel 0 and rejects negative / overflowed values).</item>
/// <item><c>--timeout-ms</c>: strictly positive — a non-positive
/// <see cref="TimeSpan"/> would propagate as an immediate-timeout into the
/// driver and surface as a confusing TimeoutException.</item>
/// <item><c>--unit-id</c>: 1..247 — the Modbus spec unicast unit-id range.
/// 0 is the broadcast address and not valid for read/write requests; 248-255
/// are reserved. (Documented in <c>docs/Driver.Modbus.Cli.md</c>.)</item>
/// </list>
/// </summary>
protected void ValidateEndpoint()
{
if (Port < 1 || Port > 65535)
throw new CommandException(
$"--port must be in 1..65535 (got {Port}).");
if (TimeoutMs <= 0)
throw new CommandException(
$"--timeout-ms must be strictly positive (got {TimeoutMs}).");
if (UnitId < 1 || UnitId > 247)
throw new CommandException(
$"--unit-id must be in 1..247 per the Modbus spec (got {UnitId}); " +
$"0 is the broadcast address, 248-255 are reserved.");
}
}