fix(driver-modbus-addressing): resolve Low code-review findings (Driver.Modbus.Addressing-006,007,009)
- Driver.Modbus.Addressing-006: broaden the catch in TryParseFamilyNative so a future helper throwing a non-Argument/Overflow type still satisfies the try-parse contract. - Driver.Modbus.Addressing-007: document that the address grammar does not carry ModbusStringByteOrder (the structured-tag path does); add a 'Grammar scope' bullet to docs/v2/dl205.md. - Driver.Modbus.Addressing-009: reword the ModbusModiconAddress comments so they don't imply a leading-digit invariant the parser doesn't enforce. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 3 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -157,7 +157,7 @@ overwrite it.
|
||||
| Severity | Low |
|
||||
| Category | Error handling & resilience |
|
||||
| Location | `ModbusAddressParser.cs:297-301` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `TryParseFamilyNative` catches only `ArgumentException` and `OverflowException`.
|
||||
The current helpers throw only those (including `ArgumentOutOfRangeException`, which derives from
|
||||
@@ -171,7 +171,13 @@ depend on.
|
||||
narrow catch, or broaden to a general catch-all that records the message — a try-parse method
|
||||
should never throw.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — broadened the `catch` filter in
|
||||
`ModbusAddressParser.TryParseFamilyNative` from `ArgumentException or OverflowException` to a
|
||||
general `catch (Exception ex)` so any future helper exception type is converted to a structured
|
||||
`(false, error)` rather than escaping the `TryParse` method. Added `DL205_TryParse_NeverThrows`
|
||||
and `MELSEC_TryParse_NeverThrows` parameterised regression tests in
|
||||
`ModbusAddressEdgeCaseTests` covering ~20 pathological inputs (empty prefixes, octal/hex digit
|
||||
violations, overflow inputs, unknown prefixes) to pin the defensive contract.
|
||||
|
||||
### Driver.Modbus.Addressing-007
|
||||
|
||||
@@ -180,7 +186,7 @@ should never throw.
|
||||
| Severity | Low |
|
||||
| Category | Design-document adherence |
|
||||
| Location | `ModbusDataType.cs:91-95`, `docs/v2/dl205.md` section Strings |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `ModbusStringByteOrder` (HighByteFirst / LowByteFirst) is defined in this
|
||||
assembly and documented as the DL205 low-byte-first string-packing knob, but `ParsedModbusAddress`
|
||||
@@ -193,7 +199,18 @@ unreachable from the parser, so the grammar cannot represent a known, documented
|
||||
token for it, or document explicitly that DL205 string byte order is only configurable via the
|
||||
structured tag form and is intentionally out of grammar scope.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — chose the "document the limitation" branch of the
|
||||
recommendation rather than adding a grammar token: the 3rd field slot is the multi-register
|
||||
word/byte order and the 4th is the array count, so a 5th `:<order>` suffix would conflict with
|
||||
the existing count-shape disambiguation; `ModbusStringByteOrder` is already plumbed through the
|
||||
structured tag form (`ModbusDriverFactoryExtensions.ModbusTagDto.StringByteOrder` →
|
||||
`ModbusTagDefinition.StringByteOrder`) which is the canonical config path. Added an explicit
|
||||
"Grammar scope" remarks block to `ModbusStringByteOrder` and to the `ModbusAddressParser`
|
||||
`<remarks>` block stating that string byte order is configurable only via the structured tag
|
||||
form. Added a corresponding bullet to `docs/v2/dl205.md` §Strings. Added two regression tests
|
||||
(`Parser_STR_grammar_does_not_carry_StringByteOrder` reflecting on `ParsedModbusAddress`, and
|
||||
`Parser_rejects_unknown_string_byte_order_token_in_grammar`) pinning the contract so a future
|
||||
grammar change can't quietly add a conflicting token.
|
||||
|
||||
### Driver.Modbus.Addressing-008
|
||||
|
||||
@@ -226,7 +243,7 @@ finding -001.
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `ModbusModiconAddress.cs:55-64`, `ModbusModiconAddress.cs:104-110` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The comments on `ModbusModiconAddress.TryParse` are slightly inaccurate. The
|
||||
remark that 5-digit Modicon is always exactly 5 chars (40001..49999) and 6-digit is exactly 6
|
||||
@@ -238,4 +255,11 @@ says the 5-digit form caps at 9999 by construction while the adjacent code path
|
||||
**Recommendation:** Reword the range examples to cover all four region digits and drop the
|
||||
caps-at-9999 aside or restate it as a precise statement about trailing-digit count.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — reworded the up-front range-check comment to describe all
|
||||
four region digits (0/1/3/4) and give examples covering each region (coils 00001..09999 /
|
||||
000001..065536, holding registers 40001..49999 / 400001..465536). Reworded the lower
|
||||
`> 65536` comment to drop the misleading "5-digit form caps at 9999 by construction" framing and
|
||||
state precisely that the check is reached only by the 6-digit form in practice, but applied to
|
||||
both for safety rather than relying on the digit-count invariant. Pure documentation change —
|
||||
no behavioural change; the existing `ModbusModiconAddressTests` already pin the cross-region
|
||||
5-digit ranges (00001..09999 / 10001..19999 / 30001..39999 / 40001..49999).
|
||||
|
||||
@@ -43,6 +43,15 @@ that a naive Modbus client will byte-swap [1][2].
|
||||
really "read 10 consecutive holding registers starting at the Modbus address
|
||||
that V2000 translates to (see next section), unpack each register low-byte
|
||||
then high-byte, stop at the first `0x00`."
|
||||
- **Grammar scope** (Driver.Modbus.Addressing-007): the
|
||||
`ModbusStringByteOrder` knob (HighByteFirst / LowByteFirst) is **not**
|
||||
expressible through the `ModbusAddressParser` grammar string — the 3rd grammar
|
||||
field is the multi-register word/byte order (ABCD/CDAB/BADC/DCBA) and the 4th
|
||||
is the array count, so there is no token slot for the per-string byte order.
|
||||
Tags that need low-byte-first packing on DL205 must set
|
||||
`ModbusTagDefinition.StringByteOrder = LowByteFirst` via the structured tag
|
||||
form (the driver config DTO). The grammar default produces high-byte-first
|
||||
strings (matches Ignition / Kepware default behaviour).
|
||||
|
||||
Test names:
|
||||
`DL205_String_low_byte_first_within_register`,
|
||||
|
||||
@@ -29,6 +29,15 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus;
|
||||
/// <item><c>C100</c> — Coils[99] (mnemonic).</item>
|
||||
/// </list>
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Grammar scope — out of band (Driver.Modbus.Addressing-007):</b> per-string byte
|
||||
/// order (<see cref="ModbusStringByteOrder"/>) is NOT expressible through this grammar.
|
||||
/// The DL205 low-byte-first string-packing knob is configurable only via the structured
|
||||
/// tag form (the driver's <c>ModbusTagDefinition.StringByteOrder</c> field). The 3rd
|
||||
/// grammar field is the multi-register word/byte order (ABCD/CDAB/BADC/DCBA), not the
|
||||
/// per-string byte order — adding a 5th token would conflict with the array-count slot.
|
||||
/// See <see cref="ModbusStringByteOrder"/> and <c>docs/v2/dl205.md</c> §Strings.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public static class ModbusAddressParser
|
||||
{
|
||||
@@ -341,8 +350,15 @@ public static class ModbusAddressParser
|
||||
return false;
|
||||
}
|
||||
}
|
||||
catch (Exception ex) when (ex is ArgumentException or OverflowException)
|
||||
catch (Exception ex)
|
||||
{
|
||||
// Driver.Modbus.Addressing-006: a try-parse method must never throw, so any helper
|
||||
// exception is converted to a structured error. The current helpers throw only
|
||||
// ArgumentException (incl. ArgumentOutOfRangeException) and OverflowException, but
|
||||
// catching narrowly would silently break the TryParse contract if a helper ever
|
||||
// switches to e.g. FormatException from a ushort.Parse swap. Config-bind hot-path
|
||||
// callers depend on TryParse returning a structured (false, error) rather than
|
||||
// throwing an unhandled exception that escapes their TryParse wrapper.
|
||||
error = $"Family-native parse for {family} failed on '{text}': {ex.Message}";
|
||||
return false;
|
||||
}
|
||||
|
||||
@@ -88,6 +88,25 @@ public enum ModbusByteOrder
|
||||
/// each register. Word ordering across multiple registers is always ascending address for
|
||||
/// strings — only the byte order inside each register flips.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>Grammar scope (Driver.Modbus.Addressing-007):</b> this enum is intentionally NOT
|
||||
/// expressible through the <see cref="ModbusAddressParser"/> grammar string. The grammar
|
||||
/// has no token form for it, and <see cref="ParsedModbusAddress"/> has no field for it —
|
||||
/// a DL205 string tag parsed from the grammar always carries the driver's default order.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The string byte order is configurable only via the structured tag definition (the
|
||||
/// driver's <c>ModbusTagDefinition.StringByteOrder</c> field). Adding a grammar token
|
||||
/// was explicitly considered and rejected: the 3rd-field slot is the multi-register
|
||||
/// word/byte order (ABCD/CDAB/BADC/DCBA) and the 4th-field slot is the array count, so
|
||||
/// a fifth <c>:<order></c> suffix would conflict with the count-shape disambiguation.
|
||||
/// Sites that need per-tag low-byte-first strings must use the structured form. The
|
||||
/// default high-byte-first matches the Modbus spec and Ignition / Kepware default
|
||||
/// behaviour.
|
||||
/// </para>
|
||||
/// <para>See <c>docs/v2/dl205.md</c> §Strings for the DL205-specific rationale.</para>
|
||||
/// </remarks>
|
||||
public enum ModbusStringByteOrder
|
||||
{
|
||||
HighByteFirst,
|
||||
|
||||
@@ -52,10 +52,14 @@ public static class ModbusModiconAddress
|
||||
return false;
|
||||
}
|
||||
|
||||
// Range check up-front — keeps the rest of the parser straight-line. 5-digit Modicon
|
||||
// is always exactly 5 chars (40001..49999, with the lead digit selecting region), and
|
||||
// 6-digit is exactly 6 (400001..465536-shaped). Anything else is unambiguously
|
||||
// malformed so we reject before doing the per-character work.
|
||||
// Range check up-front — keeps the rest of the parser straight-line. Modicon addresses
|
||||
// are exactly 5 or 6 characters: a leading region digit (0/1/3/4 — coils, discrete
|
||||
// inputs, input registers, holding registers respectively) followed by 4 (5-digit form)
|
||||
// or 5 (6-digit form) trailing digits encoding the 1-based register number. The
|
||||
// 5-digit form covers 1..9999 per region (e.g. coils 00001..09999, holding registers
|
||||
// 40001..49999); the 6-digit form covers the full 1..65536 wire range (e.g. coils
|
||||
// 000001..065536, holding 400001..465536). Anything else is unambiguously malformed so
|
||||
// we reject before doing the per-character work.
|
||||
var s = address.Trim();
|
||||
if (s.Length is not (5 or 6))
|
||||
{
|
||||
@@ -100,9 +104,10 @@ public static class ModbusModiconAddress
|
||||
return false;
|
||||
}
|
||||
|
||||
// 5-digit form caps at 9999 by construction (4 trailing digits); reject if the parsed
|
||||
// value exceeds the wire-protocol maximum of 65536 (i.e. PDU offset 65535). 6-digit
|
||||
// form can address the full 65535-offset range.
|
||||
// Wire-protocol maximum is register number 65536 (PDU offset 65535). The 5-digit form's
|
||||
// 4 trailing digits can only encode up to 9999, so this check is reached only by the
|
||||
// 6-digit form in practice — but it is applied to both for safety / simplicity rather
|
||||
// than relying on the digit-count invariant.
|
||||
if (registerNumber > 65536)
|
||||
{
|
||||
error = $"Modicon register number {registerNumber} exceeds the wire maximum (65536 / PDU offset 65535)";
|
||||
|
||||
@@ -146,4 +146,94 @@ public sealed class ModbusAddressEdgeCaseTests
|
||||
// D0 with a bank base that itself overflows: base 65535 + D1 = 65536.
|
||||
Should.Throw<OverflowException>(() => MelsecAddress.DRegisterToHolding("D1", dBankBase: 65535));
|
||||
}
|
||||
|
||||
// ── TryParse never throws (Driver.Modbus.Addressing-006) ─────────────────────────────────
|
||||
//
|
||||
// The TryParse contract is that it converts every parse failure into a structured (false,
|
||||
// error) return — config-bind hot paths depend on this. The family-native catch was previously
|
||||
// narrow (ArgumentException / OverflowException only); any future helper change that threw a
|
||||
// different exception type (e.g. FormatException from a ushort.Parse swap) would escape as an
|
||||
// unhandled exception out of a TryParse method. These tests assert the defensive contract
|
||||
// across a broad set of pathological inputs.
|
||||
|
||||
[Theory]
|
||||
[InlineData("V")] // V prefix with no digits
|
||||
[InlineData("V99999999999999")] // overflow in user V-memory octal decode
|
||||
[InlineData("V200000")] // overflow in user V-memory octal decode
|
||||
[InlineData("V77777777")] // octal way past 0xFFFF in system bank
|
||||
[InlineData("Y")] // Y prefix with no digits
|
||||
[InlineData("Y8888")] // non-octal digit
|
||||
[InlineData("Y174000")] // octal offset overflows YOutputBaseCoil + value
|
||||
[InlineData("C")] // C prefix alone
|
||||
[InlineData("C99999999")] // overflow in C-relay
|
||||
[InlineData("X")] // X prefix alone
|
||||
[InlineData("X8")] // non-octal digit
|
||||
[InlineData("SP")] // SP prefix alone
|
||||
[InlineData("SP9")] // non-octal digit
|
||||
[InlineData("Z123")] // unknown DL205 prefix
|
||||
public void DL205_TryParse_NeverThrows_ReturnsStructuredError(string addr)
|
||||
{
|
||||
// Defensive contract: any helper failure must surface as (false, non-null error), never
|
||||
// as an unhandled exception out of TryParse.
|
||||
var ok = ModbusAddressParser.TryParse(addr, ModbusFamily.DL205, MelsecFamily.Q_L_iQR, out var result, out var error);
|
||||
ok.ShouldBeFalse();
|
||||
result.ShouldBeNull();
|
||||
error.ShouldNotBeNullOrEmpty();
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData("D")] // D prefix alone — no digits
|
||||
[InlineData("D-1")] // negative — would fail ushort.TryParse, must not throw
|
||||
[InlineData("D65536")] // overflow
|
||||
[InlineData("DABC")] // non-decimal digits in D
|
||||
[InlineData("MABC")] // non-decimal digits in M
|
||||
[InlineData("X10000")] // hex overflow (Q-family)
|
||||
[InlineData("XZZZZ")] // non-hex digit (Q-family)
|
||||
[InlineData("Y10000")] // hex overflow (Q-family)
|
||||
public void MELSEC_TryParse_NeverThrows_ReturnsStructuredError(string addr)
|
||||
{
|
||||
var ok = ModbusAddressParser.TryParse(addr, ModbusFamily.MELSEC, MelsecFamily.Q_L_iQR, out var result, out var error);
|
||||
ok.ShouldBeFalse();
|
||||
result.ShouldBeNull();
|
||||
error.ShouldNotBeNullOrEmpty();
|
||||
}
|
||||
|
||||
// ── ModbusStringByteOrder is grammar-out-of-scope (Driver.Modbus.Addressing-007) ────────
|
||||
//
|
||||
// ModbusStringByteOrder (HighByteFirst / LowByteFirst) is the DL205 low-byte-first packing
|
||||
// knob. It is intentionally NOT expressible through the address grammar — there is no token
|
||||
// form to set it and ParsedModbusAddress has no field for it. The string byte order is
|
||||
// configurable only via the structured tag form (ModbusTagDefinition.StringByteOrder), which
|
||||
// is the canonical config path. These tests pin that contract so a future grammar change
|
||||
// can't quietly add a token that conflicts with the array-count slot.
|
||||
|
||||
[Fact]
|
||||
public void Parser_STR_grammar_does_not_carry_StringByteOrder()
|
||||
{
|
||||
// STR20 parses fine — but the result has no StringByteOrder field (the property does
|
||||
// not exist on ParsedModbusAddress). The string byte order must be set on the structured
|
||||
// tag definition, not the grammar string.
|
||||
var ok = ModbusAddressParser.TryParse("40001:STR20", out var result, out _);
|
||||
ok.ShouldBeTrue();
|
||||
result!.DataType.ShouldBe(ModbusDataType.String);
|
||||
result.StringLength.ShouldBe((ushort)20);
|
||||
// Compile-time assertion: ParsedModbusAddress does not expose StringByteOrder.
|
||||
// Searching for a property by reflection would let us assert "no such field":
|
||||
typeof(ParsedModbusAddress)
|
||||
.GetProperty("StringByteOrder")
|
||||
.ShouldBeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Parser_rejects_unknown_string_byte_order_token_in_grammar()
|
||||
{
|
||||
// A user trying to express low-byte-first via a grammar suffix like "LOWB" or "HIGH" in
|
||||
// the byte-order slot gets the standard "Unknown byte order" diagnostic — the parser is
|
||||
// explicit that field 3 is the multi-register word/byte order, not the per-string byte
|
||||
// order. The structured tag form is the only configuration path for ModbusStringByteOrder.
|
||||
var ok = ModbusAddressParser.TryParse("40001:STR20:LOWB", out _, out var error);
|
||||
ok.ShouldBeFalse();
|
||||
error.ShouldNotBeNullOrEmpty();
|
||||
error!.ShouldContain("byte order", Case.Insensitive);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user