From 97020d052759829dd7a9ac8df3788230ba206302 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 12:22:53 -0400 Subject: [PATCH] review(Driver.FOCAS.Contracts): first review; keep Writable=true default (re-triaged) First review at 7286d320. -001 re-triaged Won't-Fix: the 'FOCAS is read-only' premise was WRONG (FOCAS supports PMC/data writes); flipping Writable default to false broke 6 consuming write tests -> reverted to true, doc corrected. -003/-004 doc fixes resolved. -002 (WriteIdempotent not threaded, Driver.FOCAS) Open. --- .../Driver.FOCAS.Contracts/findings.md | 104 ++++++++++++++++++ .../FocasDriverOptions.cs | 31 ++++++ 2 files changed, 135 insertions(+) create mode 100644 code-reviews/Driver.FOCAS.Contracts/findings.md diff --git a/code-reviews/Driver.FOCAS.Contracts/findings.md b/code-reviews/Driver.FOCAS.Contracts/findings.md new file mode 100644 index 00000000..4f927b84 --- /dev/null +++ b/code-reviews/Driver.FOCAS.Contracts/findings.md @@ -0,0 +1,104 @@ +# Code Review — Driver.FOCAS.Contracts + + + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | +| 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 | 1 finding (Driver.FOCAS.Contracts-001) | +| 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 | 1 finding (Driver.FOCAS.Contracts-002) | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | No test project — findings recorded, deferred by task scope | +| 10 | Documentation & comments | 2 findings (Driver.FOCAS.Contracts-003, Driver.FOCAS.Contracts-004) | + +## Findings + + + +### Driver.FOCAS.Contracts-001 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs:144` | +| Status | Won't Fix | + +**Description:** `FocasTagDefinition`'s positional parameter `Writable` defaults to `true`. The original review hypothesised that FOCAS is read-only by design (claiming `WireFocasClient.WriteAsync` always returns `BadNotWritable` and the `def.Writable` gate is dead code) and that the default should therefore be `false`. **That premise was wrong** — FOCAS supports PMC/data writes, the `def.Writable` gate is live, and six `Driver.FOCAS.Tests` write tests (`FocasPmcBitRmwTests`, `FocasReadWriteTests`) rely on `Writable` defaulting to `true`. + +**Recommendation:** Leave the default at `true`. (The original recommendation to flip it to `false` is withdrawn.) + +**Resolution:** Won't Fix (re-triaged 2026-06-19). The "FOCAS is read-only" premise was incorrect — the consuming `Driver.FOCAS.Tests` write suites failed when the default was flipped to `false`. The default `Writable = true` is intentional and correct; the change was reverted. The accompanying `` doc was kept but corrected to describe the live write path. (Caught by the orchestrator's consuming-driver-test safety net, which the no-test-project Contracts build could not exercise.) + +--- + +### Driver.FOCAS.Contracts-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Design-document adherence | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs:139-145` | +| Status | Open | + +**Description:** `FocasTagDefinition` carries a `WriteIdempotent` field (default `false`) that the FOCAS driver never reads anywhere — neither in `DiscoverAsync`, `ReadAsync`, nor `WriteAsync`. `DiscoverAsync` always passes `WriteIdempotent: false` to `DriverAttributeInfo` (hardcoded), so the field has no runtime effect. `docs/drivers/FOCAS.md` does not mention `WriteIdempotent` in its configuration tables, making its purpose undiscoverable to operators. + +**Recommendation:** Either (a) thread `FocasTagDefinition.WriteIdempotent` through to `DriverAttributeInfo` in `DiscoverAsync` so the field has runtime effect and matches the pattern of other drivers, or (b) remove it from the record and the `FocasDriverConfigDto` wire DTO. Option (a) is the safer fix and matches the design of Modbus and other drivers. This is deferred because the fix touches `FocasDriver.DiscoverAsync` outside this module and requires a driver-level test change. + +**Resolution:** _(empty until closed)_ + +--- + +### Driver.FOCAS.Contracts-003 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs:147-155` | +| Status | Resolved | + +**Description:** `FocasProbeOptions` is the only class in this module without a class-level XML `` comment. All other options classes (`FocasDriverOptions`, `FocasFixedTreeOptions`, `FocasHandleRecycleOptions`, `FocasAlarmProjectionOptions`) have class-level summaries. This inconsistency causes IDE tooltips and XML-doc generators to produce a blank description for `FocasProbeOptions`. + +**Recommendation:** Add a class-level `` consistent with the pattern of the other options types: `Controls periodic connectivity probing. One cnc_rdcncstat call per configured device per tick; transitions fire OnHostStatusChanged.` + +**Resolution:** Fixed in the same commit as this review. Added XML `` to `FocasProbeOptions`. Verified by build (no test project). + +--- + +### Driver.FOCAS.Contracts-004 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs:107-131` | +| Status | Resolved | + +**Description:** `FocasDeviceOptions`'s class-level `` references `` but there is no corresponding `` documentation block. Only `PositionDecimalPlaces` has a `` entry; `HostAddress`, `DeviceName`, and `Series` are all undocumented at the parameter level. This leaves the expected URI form of `HostAddress` (`focas://{ip}[:{port}]`) and the valid values of `Series` undiscoverable from the XML doc. + +**Recommendation:** Add `` entries for `HostAddress`, `DeviceName`, and `Series` documenting the expected URI form for `HostAddress` and referring to `FocasCncSeries.Unknown` as the permissive default for `Series`. + +**Resolution:** Fixed in the same commit as this review. Added `` doc entries for `HostAddress`, `DeviceName`, and `Series` to `FocasDeviceOptions`. Verified by build (no test project). diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs index c133937b..61e5e3ad 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasDriverOptions.cs @@ -109,6 +109,20 @@ public sealed class FocasAlarmProjectionOptions /// address validation at FocasDriver.InitializeAsync; leave as /// to skip validation (legacy behaviour). /// +/// +/// FOCAS TCP endpoint in focas://{ip}[:{port}] form, e.g. +/// focas://10.20.30.40:8193. The port defaults to 8193 when omitted. +/// Parsed by FocasHostAddress.TryParse at FocasDriver.InitializeAsync. +/// +/// +/// Optional human-readable label shown in the OPC UA address-space folder for this +/// device. Defaults to when null. +/// +/// +/// CNC controller series. Used by to gate which +/// FOCAS addresses are valid. Leave as (the +/// default) to skip range validation and preserve legacy behaviour. +/// /// /// Axis positions returned by cnc_rddynamic2 are scaled integers. The driver /// divides AbsolutePosition / MachinePosition / RelativePosition / DistanceToGo by @@ -136,6 +150,17 @@ public sealed record FocasDeviceOptions( /// address string that parses via FocasAddress.TryParse — /// X0.0 / R100 / PARAM:1815/0 / MACRO:500. /// +/// +/// Whether the tag is declared writable in config. Reflected at the +/// DiscoverAsync address-space advertisement seam and gates the driver's +/// write path — FOCAS supports PMC/data writes (see FocasPmcBitRmwTests / +/// FocasReadWriteTests). Defaults to true. +/// +/// +/// Whether repeated writes of the same value are safe. Carried for parity; not yet +/// threaded through to DriverAttributeInfo in DiscoverAsync (see +/// Driver.FOCAS.Contracts-002). Defaults to false. +/// public sealed record FocasTagDefinition( string Name, string DeviceHostAddress, @@ -144,6 +169,12 @@ public sealed record FocasTagDefinition( bool Writable = true, bool WriteIdempotent = false); +/// +/// Controls periodic connectivity probing. One cnc_rdcncstat call per +/// configured device per tick; transitions fire OnHostStatusChanged. +/// Enabled by default so the driver surfaces a fast host-state transition when +/// a CNC goes offline between data reads. +/// public sealed class FocasProbeOptions { /// Gets or sets a value indicating whether probing is enabled.