dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(driver-focas): resolve Low code-review findings (Driver.FOCAS-007,008,009,010,011)

Browse Source 
- Driver.FOCAS-007: optional ILogger<FocasDriver> + alarm-projection
  logger; log Debug around every formerly-empty catch (probe / shutdown
  / fixed-tree / recycle / alarms-read / projection).
- Driver.FOCAS-008: cache the parsed FocasAddress per tag at
  InitializeAsync; Read/WriteAsync look it up instead of re-parsing on
  every call.
- Driver.FOCAS-009: ProbeLoopAsync now wraps client.ProbeAsync in a
  linked CTS honouring Probe.Timeout so a hung CNC socket can't block
  past the configured limit.
- Driver.FOCAS-010: FocasOperationModeExtensions.ToText delegates to
  FocasOpMode.ToText — single canonical op-mode label surface.
- Driver.FOCAS-011: FocasAlarmType constants are typed short to match
  the cnc_rdalmmsg2 wire field and the projection switch arms.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 07:45:38 -04:00
parent f7e3e9885e
commit 6575c6e5f6
8 changed files with 522 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
code-reviews/Driver.FOCAS/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 5 |
| Open findings | 0 |
## Checklist coverage
@@ -200,7 +200,7 @@ stale object.
| Severity | Low |
| Category | Error handling & resilience |
| Location | `FocasDriver.cs:140-148`, `FocasDriver.cs:478-484`, `FocasDriver.cs:529-533`, `FocasAlarmProjection.cs:61-63` |
| Status | Open |
| Status | Resolved |
**Description:** Numerous `try { ... } catch {}` blocks swallow every exception with no
logging - `ShutdownAsync` (CTS cancel/dispose), `RecycleLoopAsync` (`DisposeClient`),
@@ -215,7 +215,7 @@ solely on `GetHealth()`.
poll/probe/recycle loops at `Debug`/`Warning`. Pass a logger into `FocasWireClient` so
the per-response `Debug` entries it already emits are actually captured.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `FocasDriver` now takes an optional `ILogger<FocasDriver>` (defaulting to `NullLogger`) and every previously-empty `catch { }` in `ShutdownAsync` / `ProbeLoopAsync` / `FixedTreeLoopAsync` / `RecycleLoopAsync` / `ReadActiveAlarmsAcrossDevicesAsync` now logs at `Debug` with the host address + context. `FocasAlarmProjection` also accepts an optional `ILogger` (forwarded by the driver) so its unsubscribe / dispose / per-tick poll swallows log. `WireFocasClientFactory` gained a logger-accepting overload that threads through to `FocasWireClient`, so its per-response `Debug` entries actually reach the host pipeline.
### Driver.FOCAS-008
@@ -224,7 +224,7 @@ the per-response `Debug` entries it already emits are actually captured.
| Severity | Low |
| Category | Performance & resource management |
| Location | `FocasDriver.cs:201`, `FocasDriver.cs:253` |
| Status | Open |
| Status | Resolved |
**Description:** `ReadAsync` and `WriteAsync` call `FocasAddress.TryParse(def.Address)`
on every operation, even though `InitializeAsync` already parsed and validated every
@@ -235,7 +235,7 @@ re-parses and allocates a `FocasAddress` record per tag per tick unnecessarily.
parsed `FocasAddress` on `FocasTagDefinition` (or in a side dictionary), so the runtime
read/write paths use the cached value.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `FocasDriver` now holds a `_parsedAddressesByTagName` side dictionary populated at `InitializeAsync`. `ReadAsync` and `WriteAsync` look up the cached `FocasAddress` instance; the defensive fallback `TryParse` only fires if a tag was somehow not seeded. The cache is cleared on `ShutdownAsync`. Regression test `ReadAsync_uses_cached_FocasAddress_when_tag_definition_has_a_malformed_address_after_init` (and the matching `WriteAsync` variant) asserts the same `FocasAddress` instance is reused across calls.
### Driver.FOCAS-009
@@ -244,7 +244,7 @@ read/write paths use the cached value.
| Severity | Low |
| Category | Design-document adherence |
| Location | `FocasDriverOptions.cs:110-115`, `FocasDriver.cs:468-486`, `FocasDriverFactoryExtensions.cs:75-80` |
| Status | Open |
| Status | Resolved |
**Description:** `FocasProbeOptions.Timeout` is parsed by the factory
(`FocasProbeDto.TimeoutMs` to `FocasProbeOptions.Timeout`) but never consumed.
@@ -257,7 +257,7 @@ until the OS TCP timeout rather than the configured `Probe.Timeout`.
around the `ProbeAsync` call, or remove the dead `Timeout` field from
`FocasProbeOptions` / `FocasProbeDto` if it is genuinely not intended.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `FocasDriver.ProbeLoopAsync` now wraps `client.ProbeAsync` in a linked `CancellationTokenSource` that fires after `Probe.Timeout` (skipped when the timeout is `<= TimeSpan.Zero`). On timeout the loop logs the cancellation at Debug and surfaces it as a failed probe, so a hung CNC socket transitions the host to `Stopped` at the configured budget instead of blocking on the OS TCP timeout. Regression test `ProbeLoop_cancels_a_slow_ProbeAsync_at_Probe_Timeout` asserts the cancellation reaches the fake `ProbeAsync` within the configured 100 ms.
### Driver.FOCAS-010
@@ -266,7 +266,7 @@ around the `ProbeAsync` call, or remove the dead `Timeout` field from
| Severity | Low |
| Category | Code organization & conventions |
| Location | `IFocasClient.cs:210-227` (`FocasOpMode`), `FocasConstants.cs:42-78` (`FocasOperationMode`) |
| Status | Open |
| Status | Resolved |
**Description:** There are two parallel operation-mode-to-text mappings with divergent
labels. `FocasOpMode.ToText` (used by the driver fixed-tree `OperationMode/ModeText`
@@ -278,7 +278,7 @@ inconsistent results depending on which path renders it.
**Recommendation:** Consolidate to a single op-mode enum + `ToText` helper shared by
both the wire layer and the driver projection, with one canonical label set.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `FocasOperationModeExtensions.ToText` now delegates to `FocasOpMode.ToText((short)mode)`, so the wire layer and the driver fixed-tree projection render identical labels. `FocasOpMode` keeps its existing labels (`TJOG`, `TEACH_IN_HANDLE`, `Mode{n}` fallback), which are now the single canonical surface. Regression theory `OpMode_ToText_yields_the_same_label_in_both_namespaces` cross-checks every defined code; `OpMode_ToText_fallback_label_is_consistent` covers the unknown-code path.
### Driver.FOCAS-011
@@ -287,7 +287,7 @@ both the wire layer and the driver projection, with one canonical label set.
| Severity | Low |
| Category | Code organization & conventions |
| Location | `IFocasClient.cs:275-287` (`FocasAlarmType`), `FocasAlarmProjection.cs:149-175` |
| Status | Open |
| Status | Resolved |
**Description:** `FocasAlarmType` declares its constants as `public const int`, but the
only consumers - `FocasAlarmProjection.MapAlarmType(short type)` and
@@ -301,7 +301,7 @@ expected by `ReadAlarmsAsync`.
**Recommendation:** Declare the `FocasAlarmType` constants as `short` (or make it an
`enum : short`) so the type matches the wire field width and the projection signatures.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — every `FocasAlarmType` constant (`All`, `Parameter`, `PulseCode`, `Overtravel`, `Overheat`, `Servo`, `DataIo`, `MemoryCheck`, `MacroAlarm`) is now typed `short`, matching the wire field width on `cnc_rdalmmsg2` and the `switch (short type)` arms in `FocasAlarmProjection.MapAlarmType` / `MapSeverity`. Regression test `FocasAlarmType_constants_are_typed_short` uses reflection to guarantee the type is preserved against future drift.
### Driver.FOCAS-012