diff --git a/code-reviews/Driver.Modbus.Addressing/findings.md b/code-reviews/Driver.Modbus.Addressing/findings.md index 5890017..2bb822d 100644 --- a/code-reviews/Driver.Modbus.Addressing/findings.md +++ b/code-reviews/Driver.Modbus.Addressing/findings.md @@ -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 `:` 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` +`` 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). diff --git a/docs/v2/dl205.md b/docs/v2/dl205.md index b5ff16d..2169daf 100644 --- a/docs/v2/dl205.md +++ b/docs/v2/dl205.md @@ -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`, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusAddressParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusAddressParser.cs index 9276f7d..dddadda 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusAddressParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusAddressParser.cs @@ -29,6 +29,15 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; /// C100 — Coils[99] (mnemonic). /// /// +/// +/// Grammar scope — out of band (Driver.Modbus.Addressing-007): per-string byte +/// order () 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 ModbusTagDefinition.StringByteOrder 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 and docs/v2/dl205.md §Strings. +/// /// 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; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusDataType.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusDataType.cs index 2fce635..5e14dd9 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusDataType.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusDataType.cs @@ -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. /// +/// +/// +/// Grammar scope (Driver.Modbus.Addressing-007): this enum is intentionally NOT +/// expressible through the grammar string. The grammar +/// has no token form for it, and has no field for it — +/// a DL205 string tag parsed from the grammar always carries the driver's default order. +/// +/// +/// The string byte order is configurable only via the structured tag definition (the +/// driver's ModbusTagDefinition.StringByteOrder 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 :<order> 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. +/// +/// See docs/v2/dl205.md §Strings for the DL205-specific rationale. +/// public enum ModbusStringByteOrder { HighByteFirst, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusModiconAddress.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusModiconAddress.cs index 1ec6f37..3301533 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusModiconAddress.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusModiconAddress.cs @@ -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)"; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/ModbusAddressEdgeCaseTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/ModbusAddressEdgeCaseTests.cs index 25c28bc..4e0c3e3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/ModbusAddressEdgeCaseTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/ModbusAddressEdgeCaseTests.cs @@ -146,4 +146,94 @@ public sealed class ModbusAddressEdgeCaseTests // D0 with a bank base that itself overflows: base 65535 + D1 = 65536. Should.Throw(() => 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); + } }