lmxopcua/code-reviews/Driver.Modbus.Addressing/findings.md

Code Review — Driver.Modbus.Addressing

Field	Value
Module	src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing
Reviewer	Claude Code
Review date	2026-05-22
Commit reviewed	76d35d1
Status	Reviewed
Open findings	3

Checklist coverage

A comprehensive review completes every category, recording "No issues found" where a category produced nothing rather than leaving it blank.

#	Category	Result
1	Correctness & logic bugs	Driver.Modbus.Addressing-001, -002, -003, -004
2	OtOpcUa conventions	No issues found
3	Concurrency & thread safety	No issues found
4	Error handling & resilience	Driver.Modbus.Addressing-005, -006
5	Security	No issues found
6	Performance & resource management	No issues found
7	Design-document adherence	Driver.Modbus.Addressing-001, -007
8	Code organization & conventions	No issues found
9	Testing coverage	Driver.Modbus.Addressing-008
10	Documentation & comments	Driver.Modbus.Addressing-009

Findings

Driver.Modbus.Addressing-001

Field	Value
Severity	High
Category	Correctness & logic bugs
Location	ModbusAddressParser.cs:230-235, DirectLogicAddress.cs:66-73
Status	Resolved

Description: The DL205 family-native branch routes every V-prefixed address through DirectLogicAddress.UserVMemoryToPdu, which is a plain octal-to-decimal conversion. DL205/DL260 system V-memory (V40400 and up) is NOT a simple octal decode — per docs/v2/dl205.md section V-Memory, V40400 must map to Modbus PDU 0x2100 (decimal 8448) on a factory-mode ECOM module. The parser instead octal-decodes V40400 to decimal 16640 (0x4100), the wrong register. The DirectLogicAddress.SystemVMemoryToPdu / SystemVMemoryBasePdu helper that exists to do this correctly is never called by the parser — it is dead code from the parser point of view. A tag spreadsheet that addresses any DL system register through the grammar string silently reads and writes the wrong PLC memory. The companion test ModbusFamilyParserTests.cs:20 bakes the wrong value (V40400 to 16640) into a passing assertion, so the regression is locked in.

Recommendation: Make the DL205 V branch detect the system bank (octal address >= 40400) and route it through SystemVMemoryToPdu, or explicitly reject system V-memory in the grammar string with a diagnostic pointing at the structured tag form. Either way, fix the V40400 test to assert the corrected mapping.

Resolution: Resolved 2026-05-22 — added DirectLogicAddress.VMemoryToPdu, which detects the system bank (octal >= V40400) and relocates it through SystemVMemoryToPdu to PDU 0x2100; the DL205 V branch in ModbusAddressParser now calls it, and the ModbusFamilyParserTests V40400 assertion was corrected from 16640 to 0x2100 with system-bank regression cases added.

Driver.Modbus.Addressing-002

Field	Value
Severity	Medium
Category	Correctness & logic bugs
Location	ModbusAddressParser.cs:86-94
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 string is classified as a valid-shaped array count, assigned to countPart, then silently dropped by the later string.IsNullOrEmpty(countPart) guard. The result is that 40001:F: parses successfully as a plain scalar with a dangling empty field rather than being rejected as malformed. The 4-field form 40001:F:: has the analogous effect. A user who mistypes a trailing colon gets no diagnostic.

Recommendation: Reject an empty 3rd field explicitly, or guard the All(char.IsDigit) branch with parts[2].Length > 0.

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

Field	Value
Severity	Medium
Category	Correctness & logic bugs
Location	ModbusAddressParser.cs:405-406, ModbusAddressParser.cs:128
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 into TryParseByteOrder, producing the misleading diagnostic "Unknown byte order BOOL" instead of telling the user the type belongs in field 2. The type code BOOL is exactly 4 letters and could only ever be intended as a type — the shape heuristic cannot tell a mistyped type from a byte order, so the diagnostic actively misdirects.

Recommendation: When TryParseByteOrder fails on a 4-letter token in the 3-field form, widen 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: 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

Field	Value
Severity	Medium
Category	Correctness & logic bugs
Location	ModbusAddressParser.cs:182-194
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 "Bit index must be 0..15" message. A Modicon-style decimal-point typo like 400.01 is silently treated as region/offset 400 plus bit 01; 400 then fails Modicon length validation, so the surfaced error is the Modicon length diagnostic rather than a bit-index diagnostic, because the bit was parsed first and 01 is a valid bit. The dot-handling assumes a single dot without asserting it, and the diagnostics for these malformed inputs are inconsistent.

Recommendation: Use LastIndexOf('.') or assert exactly one dot, and validate that the region/offset segment is non-empty and dot-free after the strip so malformed inputs get a precise diagnostic.

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

Field	Value
Severity	Medium
Category	Error handling & resilience
Location	ModbusAddressParser.cs:200-213
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 Modicon error is the more specific diagnostic"). For a non-Generic family this is misleading: TryParseFamilyNative returns false with error left null for any address that does not start with a recognised family prefix, and even for recognised prefixes it only sets error inside the catch. The subsequent mnemonic and Modicon attempts overwrite error. Net effect: a clearly family-native-shaped input that fails deep in the family helper can still surface a generic Modicon "must be 5 or 6 digits" error, hiding the real cause (e.g. "contains non-octal digit").

Recommendation: When a non-Generic family is configured and the input matches a family prefix, prefer and preserve the family-native error rather than letting the Modicon fallback overwrite it.

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

Field	Value
Severity	Low
Category	Error handling & resilience
Location	ModbusAddressParser.cs:297-301
Status	Open

Description: TryParseFamilyNative catches only ArgumentException and OverflowException. The current helpers throw only those (including ArgumentOutOfRangeException, which derives from ArgumentException), so today it is correct. But the parser intent is to convert helper exceptions into structured errors; any future helper change that throws a different exception type (e.g. a FormatException from a ushort.Parse swap) would escape as an unhandled exception out of a TryParse method, violating the try-parse contract that config-bind hot-path callers depend on.

Recommendation: Either document the exact exception contract of the helpers and keep the narrow catch, or broaden to a general catch-all that records the message — a try-parse method should never throw.

Resolution: (open)

Driver.Modbus.Addressing-007

Field	Value
Severity	Low
Category	Design-document adherence
Location	ModbusDataType.cs:91-95, docs/v2/dl205.md section Strings
Status	Open

Description: ModbusStringByteOrder (HighByteFirst / LowByteFirst) is defined in this assembly and documented as the DL205 low-byte-first string-packing knob, but ParsedModbusAddress has no field for it and ModbusAddressParser never produces or consumes it. The STR<n> grammar form cannot express the DL205 string byte order described in docs/v2/dl205.md — a DL205 string tag parsed from the grammar string always carries the default order. The enum is effectively unreachable from the parser, so the grammar cannot represent a known, documented device quirk.

Recommendation: Either add a StringByteOrder field to ParsedModbusAddress plus a grammar 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)

Driver.Modbus.Addressing-008

Field	Value
Severity	Medium
Category	Testing coverage
Location	tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/
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 (ModbusFamilyParserTests.cs:20, see finding -001); (b) there is no test for UserVMemoryToPdu or AddOctalOffset overflow (V200000, C200000) hitting the OverflowException path; (c) no test for the empty-trailing-field cases of finding -002; (d) MelsecAddress.ParseHex overflow and DRegisterToHolding / MRelayToCoil bank-base overflow are untested; (e) no test that SystemVMemoryToPdu is exercised at all. The address-arithmetic overflow and off-by-one paths are exactly the high-risk surface this module owns, and they are the least covered.

Recommendation: Add overflow/boundary tests for every PDU/coil/discrete translation helper and for the parser count/bit/field edge cases. Correct the V40400 assertion as part of fixing finding -001.

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

Field	Value
Severity	Low
Category	Documentation & comments
Location	ModbusModiconAddress.cs:55-64, ModbusModiconAddress.cs:104-110
Status	Open

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 (400001..465536-shaped) implies the leading digit is always 4, but the parser accepts leading 0/1/3 too — a 5-digit coil is 00001..09999, not 40001..49999. Separately, the line-106 comment says the 5-digit form caps at 9999 by construction while the adjacent code path applies the same > 65536 check to both forms; the comment describes an invariant the code does not rely on.

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)