Joseph Doherty
0c1d5f7f88
First review at 7286d320. -001 (Low): ReadArrayLength doc corrected (positive int32 via
TryGetInt32, not uint). -002 (Structure datatype doc) Open. Consistent with the sibling
Driver.TwinCAT-017 ArrayLength flow.
112 lines
5.2 KiB
Markdown
112 lines
5.2 KiB
Markdown
# Code Review — Driver.TwinCAT.Contracts
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts` |
|
|
| Reviewer | Claude Code |
|
|
| Review date | 2026-06-19 |
|
|
| Commit reviewed | `a19b0f86` |
|
|
| Status | Reviewed |
|
|
| Open findings | 1 |
|
|
|
|
## 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.TwinCAT.Contracts-001 (Resolved) |
|
|
| 2 | OtOpcUa conventions | No issues found |
|
|
| 3 | Concurrency & thread safety | No issues found |
|
|
| 4 | Error handling & resilience | No issues found |
|
|
| 5 | Security | No issues found |
|
|
| 6 | Performance & resource management | No issues found |
|
|
| 7 | Design-document adherence | No issues found |
|
|
| 8 | Code organization & conventions | No issues found |
|
|
| 9 | Testing coverage | No issues found (no test project; thin contracts module) |
|
|
| 10 | Documentation & comments | Driver.TwinCAT.Contracts-002 |
|
|
|
|
## Cross-module consistency (Driver.TwinCAT-017 context)
|
|
|
|
Driver.TwinCAT-017 added `int? ArrayLength` to `TwinCATTagDto` in the sibling driver project,
|
|
fixing the pre-declared-tag authoring gap. The Contracts module's two consumers of
|
|
`TwinCATTagDefinition.ArrayLength` are both consistent:
|
|
|
|
- `TwinCATEquipmentTagParser.ReadArrayLength` reads `isArray`+`arrayLength` from the
|
|
equipment-tag TagConfig JSON (the AdminUI TagModal's `TagArrayConfig` layer). Field names,
|
|
guard logic (isArray must be the JSON literal `true`; length must be a positive number), and
|
|
the return type (`int?`) all match the driver's consumption in `DiscoverAsync` /
|
|
`ReadValueAsync`. No gap.
|
|
- `TwinCATDriverOptions.TwinCATTagDefinition` (the record) receives `ArrayLength` from both
|
|
`BuildTag` (pre-declared JSON path, fixed by -017) and `TwinCATEquipmentTagParser.TryParse`
|
|
(equipment-tag JSON path). Both paths are consistent. No gap introduced by -017.
|
|
|
|
One minor comment inaccuracy found and fixed below (Driver.TwinCAT.Contracts-001).
|
|
|
|
## Findings
|
|
|
|
### Driver.TwinCAT.Contracts-001
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Correctness & logic bugs |
|
|
| Location | `TwinCATEquipmentTagParser.cs:56-66` (`ReadArrayLength`) |
|
|
| Status | Resolved |
|
|
|
|
**Description:** The XML doc comment for `ReadArrayLength` describes `arrayLength` as "a
|
|
positive uint", but the implementation calls `lEl.TryGetInt32(out var len)`. If `arrayLength`
|
|
in the TagConfig JSON holds a value greater than `int.MaxValue` (2 147 483 647), `TryGetInt32`
|
|
returns `false` and the method silently returns `null` — treating the tag as scalar instead of
|
|
returning an error. The comment implies uint semantics but the code applies int semantics,
|
|
creating a mismatch that would mislead a maintainer trying to understand the accepted range.
|
|
|
|
In practice the `TagArrayConfig` layer (AdminUI) writes `arrayLength` as a `uint`, so values
|
|
above `int.MaxValue` are theoretically possible in JSON (though no real PLC array would have
|
|
two billion elements). The discrepancy is documentation-level rather than a real data-loss risk.
|
|
|
|
**Recommendation:** Correct the comment to say "a positive int32 (values above `int.MaxValue`
|
|
are treated as absent/scalar)" to match the actual `TryGetInt32` call. The simplest safe fix
|
|
is the comment correction; the behaviour is acceptable in practice.
|
|
|
|
**Resolution:** Resolved 2026-06-19 — corrected the `ReadArrayLength` XML doc comment to say
|
|
"a positive int32 (values above `int.MaxValue` are treated as absent/scalar)" so it matches
|
|
the actual `TryGetInt32` call; no behaviour change. Verified by build: 0 errors, 0 warnings.
|
|
|
|
---
|
|
|
|
### Driver.TwinCAT.Contracts-002
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Documentation & comments |
|
|
| Location | `TwinCATDataType.cs:33` (`Structure` member) |
|
|
| Status | Open |
|
|
|
|
**Description:** The `Structure` enum member's XML summary says "UDT / FB instance. Resolved
|
|
per member at discovery time." This is accurate for the discovery path, but the comment omits
|
|
that `Structure` is **rejected at pre-declaration time** by
|
|
`TwinCATDriverFactoryExtensions.BuildTag` (with a clear `InvalidOperationException`), and that
|
|
it is also **not parseable as a valid data type by `TwinCATEquipmentTagParser`** (the parser
|
|
falls through to the `DInt` fallback because `Structure` is never written by the AdminUI typed
|
|
editor). A maintainer reading only the Contracts project will infer that `Structure` is a
|
|
usable user-facing data type, when in fact it is an internal-discovery-only sentinel that must
|
|
not appear in operator-authored config.
|
|
|
|
**Recommendation:** Expand the `Structure` XML doc to reflect its actual contract:
|
|
|
|
```csharp
|
|
/// <summary>
|
|
/// UDT / FB instance. Used internally by <c>DiscoverAsync</c> when browsing controller
|
|
/// symbols — members are resolved to atomic types and emitted individually. Must not be
|
|
/// used in pre-declared tags (rejected at initialisation by the factory with
|
|
/// <see cref="InvalidOperationException"/>) or in AdminUI equipment-tag configs (the typed
|
|
/// editor does not offer it; the parser falls back to <c>DInt</c>).
|
|
/// </summary>
|
|
Structure,
|
|
```
|
|
|
|
**Resolution:** _(empty until closed)_
|