diff --git a/code-reviews/Driver.Modbus.Addressing/findings.md b/code-reviews/Driver.Modbus.Addressing/findings.md index e6ffadf..5890017 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 | 8 | +| Open findings | 3 | ## Checklist coverage @@ -66,7 +66,7 @@ assertion was corrected from 16640 to 0x2100 with system-bank regression cases a | Severity | Medium | | Category | Correctness & logic bugs | | Location | `ModbusAddressParser.cs:86-94` | -| Status | Open | +| Status | Resolved | **Description:** In the 3-field disambiguation, an empty 3rd field (`40001:F:`) reaches `parts[2].All(char.IsDigit)`. `Enumerable.All` returns true for an empty sequence, so the empty @@ -79,7 +79,7 @@ colon gets no diagnostic. **Recommendation:** Reject an empty 3rd field explicitly, or guard the `All(char.IsDigit)` branch with `parts[2].Length > 0`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added an explicit `parts[2].Length == 0` check before the `All(char.IsDigit)` branch that returns a descriptive error, so a trailing colon typo produces a diagnostic instead of silently parsing as a scalar. ### Driver.Modbus.Addressing-003 @@ -88,7 +88,7 @@ with `parts[2].Length > 0`. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `ModbusAddressParser.cs:405-406`, `ModbusAddressParser.cs:128` | -| Status | Open | +| Status | Resolved | **Description:** `LooksLikeByteOrderToken` classifies any 4-letter token as a byte-order token. A 3-field address whose 3rd field is a 4-letter type-like token (e.g. `40001:S:BOOL`) is routed @@ -101,7 +101,7 @@ byte order, so the diagnostic actively misdirects. the error message to mention that field 3 is a byte order and field 2 is the type, or attempt a type-parse fallback before emitting the byte-order error. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — in the 3-field disambiguation error path, a 4-letter alphanumeric token that looks like a type code now produces a diagnostic explicitly stating that field 3 is the byte-order slot and field 2 is the type slot, directing the user to the correct fix. ### Driver.Modbus.Addressing-004 @@ -110,7 +110,7 @@ type-parse fallback before emitting the byte-order error. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `ModbusAddressParser.cs:182-194` | -| Status | Open | +| Status | Resolved | **Description:** The bit suffix is stripped using `text.IndexOf('.')` — the first dot. An input such as `40001.5.3` produces a bit text of "5.3", rejected by `byte.TryParse` with the generic @@ -124,7 +124,7 @@ asserting it, and the diagnostics for these malformed inputs are inconsistent. region/offset segment is non-empty and dot-free after the strip so malformed inputs get a precise diagnostic. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — switched to `LastIndexOf('.')`, added a non-empty guard for the address segment before the dot, and added a check that the address segment itself contains no dot (diagnosing multi-dot inputs with "contains multiple dots" rather than a confusing bit-index error). ### Driver.Modbus.Addressing-005 @@ -133,7 +133,7 @@ diagnostic. | Severity | Medium | | Category | Error handling & resilience | | Location | `ModbusAddressParser.cs:200-213` | -| Status | Open | +| Status | Resolved | **Description:** `TryParseRegionAndOffset` tries family-native, then mnemonic, then Modicon. When all three fail it returns false with whatever error the Modicon parser last wrote (comment: "the @@ -148,7 +148,7 @@ Modicon "must be 5 or 6 digits" error, hiding the real cause (e.g. "contains non prefix, prefer and preserve the family-native error rather than letting the Modicon fallback overwrite it. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — the family-native error is now captured in `familyNativeError` and, after all three branches fail, preferred over the Modicon fallback error when it is non-null (indicating the address matched a family prefix but failed deep inside the helper). ### Driver.Modbus.Addressing-006 @@ -202,7 +202,7 @@ structured tag form and is intentionally out of grammar scope. | Severity | Medium | | Category | Testing coverage | | Location | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/` | -| Status | Open | +| Status | Resolved | **Description:** Several edge cases of the address arithmetic are untested or asserted wrong: (a) DL205 system V-memory mapping is tested only with the incorrect expected value @@ -217,7 +217,7 @@ are exactly the high-risk surface this module owns, and they are the least cover and for the parser count/bit/field edge cases. Correct the V40400 assertion as part of fixing finding -001. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `ModbusAddressEdgeCaseTests.cs` covering: empty 3rd-field rejection, multi-dot input rejection, `UserVMemoryToPdu` overflow, `AddOctalOffset` overflow via Y and C helpers, `SystemVMemoryToPdu` base/overflow, `MelsecAddress.ParseHex` overflow, `DRegisterToHolding` and `MRelayToCoil` bank-base overflow. ### Driver.Modbus.Addressing-009 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 c571735..dd6efd8 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 @@ -85,11 +85,26 @@ public static class ModbusAddressParser // else surfaces a clear error in whichever slot it lands. if (parts.Length == 3) { + // Driver.Modbus.Addressing-002: reject an empty 3rd field (e.g. "40001:F:") rather + // than silently dropping it. Enumerable.All returns true for an empty sequence, so + // without this guard the empty string would be classified as a valid array count and + // then quietly ignored, leaving the user with no diagnostic for a typo'd trailing colon. + if (parts[2].Length == 0) + { + error = $"3rd field is empty in '{address}' — use 4-field form '40001:F::5' to specify an array count with default byte order, or remove the trailing ':'"; + return false; + } if (LooksLikeByteOrderToken(parts[2])) orderPart = parts[2]; else if (parts[2].All(char.IsDigit)) countPart = parts[2]; else { - error = $"3rd field '{parts[2]}' must be a 4-letter byte order (ABCD/CDAB/BADC/DCBA) or a positive integer array count in '{address}'"; + // Driver.Modbus.Addressing-003: when TryParseByteOrder would fail on a 4-letter + // token that looks like a type code (e.g. BOOL), improve the diagnostic so the + // user knows field 3 is a byte order and field 2 is the type. + var mightBeTypeCode = parts[2].Length == 4 && parts[2].All(char.IsLetterOrDigit); + error = mightBeTypeCode + ? $"3rd field '{parts[2]}' looks like a type code — type belongs in field 2 (e.g. '40001:BOOL'), not field 3. Field 3 must be a 4-letter byte order (ABCD/CDAB/BADC/DCBA) or a positive integer array count in '{address}'" + : $"3rd field '{parts[2]}' must be a 4-letter byte order (ABCD/CDAB/BADC/DCBA) or a positive integer array count in '{address}'"; return false; } } @@ -180,10 +195,26 @@ public static class ModbusAddressParser } // Optional bit suffix: '.N' at the end, N in 0..15. Strip before parsing region/offset. - var dotIdx = text.IndexOf('.'); + // Driver.Modbus.Addressing-004: use LastIndexOf so a multi-dot input like "40001.5.3" + // produces a descriptive error ("bit index '5.3' must be 0..15") rather than silently + // parsing "5" as the bit and leaving ".3" as part of the address text. Also validate + // the address segment is non-empty (a leading dot like ".5" is not a valid Modbus addr). + var dotIdx = text.LastIndexOf('.'); var addrText = dotIdx < 0 ? text : text[..dotIdx]; if (dotIdx >= 0) { + if (addrText.Length == 0) + { + error = $"Region/offset segment is empty before bit suffix '.{text[(dotIdx + 1)..]}' in '{text}'"; + return false; + } + // Assert exactly one dot: if the remaining address still contains a dot the + // user typed something like "400.01.5" — give a precise "multiple dots" diagnostic. + if (addrText.Contains('.')) + { + error = $"Address segment '{addrText}' contains multiple dots; expected at most one '.bit' suffix in '{text}'"; + return false; + } var bitText = text[(dotIdx + 1)..]; if (!byte.TryParse(bitText, NumberStyles.None, CultureInfo.InvariantCulture, out var bitVal) || bitVal > 15) { @@ -197,8 +228,15 @@ public static class ModbusAddressParser // syntax first. Successful native parse wins; failure falls through to Modicon / mnemonic. // The order matters for cross-family ambiguity: DL205 'C100' is a control relay, not a // Modicon coil, when the user has explicitly selected DL205. - if (family != ModbusFamily.Generic && TryParseFamilyNative(addrText, family, melsecSubFamily, out region, out offset, out error)) - return true; + string? familyNativeError = null; + if (family != ModbusFamily.Generic) + { + if (TryParseFamilyNative(addrText, family, melsecSubFamily, out region, out offset, out familyNativeError)) + { + error = null; + return true; + } + } // Try mnemonic prefix first (HR, IR, C, DI). Cheaper than the digit branch and // unambiguous when present. @@ -209,7 +247,14 @@ public static class ModbusAddressParser if (ModbusModiconAddress.TryParse(addrText, out region, out offset, out error)) return true; - // Both branches failed; the Modicon error is the more specific diagnostic. + // Driver.Modbus.Addressing-005: when a non-Generic family was configured and the + // family-native parser set a specific error (meaning the address matched a recognised + // family prefix but the value was invalid, e.g. "contains non-octal digit"), prefer + // that error over the generic Modicon fallback diagnostic, which otherwise says + // "must be 5 or 6 digits" for something the user clearly intended as a V-address. + if (familyNativeError is not null) + error = familyNativeError; + return false; }