diff --git a/code-reviews/Driver.Modbus.Cli/findings.md b/code-reviews/Driver.Modbus.Cli/findings.md index 3da7507..3eb3792 100644 --- a/code-reviews/Driver.Modbus.Cli/findings.md +++ b/code-reviews/Driver.Modbus.Cli/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 8 | +| Open findings | 6 | ## Checklist coverage @@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:43-51` | -| Status | Open | +| Status | Resolved | **Description:** `SubscribeCommand` synthesises its `ModbusTagDefinition` with only `Name`, `Region`, `Address`, `DataType`, `Writable`, and `ByteOrder` — it never @@ -54,7 +54,7 @@ options to `SubscribeCommand` (mirroring `ReadCommand`) and pass them into the actually supports and reject `BitInRegister` / `String` at command entry with a clear message. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `--bit-index`, `--string-length`, and `--string-byte-order` options to `SubscribeCommand`, mirroring `ReadCommand`, and passed them through to `ModbusTagDefinition` so `BitInRegister` and `String` types subscribe correctly. ### Driver.Modbus.Cli-002 @@ -63,7 +63,7 @@ clear message. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs:54-89` | -| Status | Open | +| Status | Resolved | **Description:** `WriteCommand` rejects read-only regions (`DiscreteInputs` / `InputRegisters`) but does not validate that `--type` is meaningful for the @@ -78,7 +78,7 @@ region only supports `Bool`-style boolean values. combined with any non-boolean `--type` (anything other than `Bool`), with a message explaining coils carry a single bit. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added a `Region == Coils && DataType != Bool` check immediately after the read-only-region guard, throwing `CommandException` with a message explaining that coils carry a single bit and only `--type Bool` is valid. ### Driver.Modbus.Cli-003 diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs index b536351..5f9168a 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs @@ -35,6 +35,21 @@ public sealed class SubscribeCommand : ModbusCommandBase "BigEndian (default) or WordSwap.")] public ModbusByteOrder ByteOrder { get; init; } = ModbusByteOrder.BigEndian; + // Driver.Modbus.Cli-001: subscribe previously lacked these three options that read and + // write both expose. Without them, BitInRegister always watches bit 0 and String runs with + // StringLength=0, silently producing wrong results for any subscriber using those types. + [CommandOption("bit-index", Description = + "For type=BitInRegister: which bit of the holding register (0-15, LSB-first).")] + public byte BitIndex { get; init; } + + [CommandOption("string-length", Description = + "For type=String: character count (2 per register, rounded up).")] + public ushort StringLength { get; init; } + + [CommandOption("string-byte-order", Description = + "For type=String: HighByteFirst (standard) or LowByteFirst (DirectLOGIC).")] + public ModbusStringByteOrder StringByteOrder { get; init; } = ModbusStringByteOrder.HighByteFirst; + public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); @@ -47,7 +62,10 @@ public sealed class SubscribeCommand : ModbusCommandBase Address: Address, DataType: DataType, Writable: false, - ByteOrder: ByteOrder); + ByteOrder: ByteOrder, + BitIndex: BitIndex, + StringLength: StringLength, + StringByteOrder: StringByteOrder); var options = BuildOptions([tag]); await using var driver = new ModbusDriver(options, DriverInstanceId);