From 5a9c4591b9ea3e13f002651b4ad2c79a4070c3b1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 23 May 2026 15:14:08 -0400 Subject: [PATCH] fix(cli-common): name native-driver-emitted status codes in SnapshotFormatter Driver.FOCAS.Cli-005 follow-up: extend the SnapshotFormatter.FormatStatus shortlist with the five Bad* codes the native-protocol mappers (FOCAS, AbCip, AbLegacy) emit but which the shortlist previously left unnamed, so they rendered only as severity-class 'Bad' instead of the documented 'BadDeviceFailure' / 'BadNotWritable' / ... names operators are told to read off probe/write output. Added entries: 0x80020000 BadInternalError 0x803B0000 BadNotWritable 0x803C0000 BadOutOfRange 0x803D0000 BadNotSupported 0x80550000 BadDeviceFailure (BadTimeout 0x800A0000 was already added under Driver.Cli.Common-001.) Tests: SnapshotFormatterTests gains a new [Theory] FormatStatus_names_native_driver_emitted_codes covering the five names, and the existing well-known [Theory] is extended with the same entries to enforce exact '0x... (Name)' rendering. Suite now 47 green (was 42). Flips Driver.FOCAS.Cli-005 from Deferred to Resolved; README regenerated. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.FOCAS.Cli/findings.md | 28 +++++++++++-------- code-reviews/README.md | 2 +- .../SnapshotFormatter.cs | 5 ++++ .../SnapshotFormatterTests.cs | 21 ++++++++++++++ 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/code-reviews/Driver.FOCAS.Cli/findings.md b/code-reviews/Driver.FOCAS.Cli/findings.md index 9e4e1ce..2f76854 100644 --- a/code-reviews/Driver.FOCAS.Cli/findings.md +++ b/code-reviews/Driver.FOCAS.Cli/findings.md @@ -187,7 +187,7 @@ the source-level convention against regression. | Severity | Low | | Category | Design-document adherence | | Location | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) | -| Status | Deferred | +| Status | Resolved | **Description:** `docs/Driver.FOCAS.Cli.md` documents `BadDeviceFailure` and `BadCommunicationError` as the key diagnostic signals an operator reads off @@ -214,14 +214,18 @@ actually emit — at minimum `BadNotWritable`, `BadOutOfRange`, `BadNotSupported because the gap defeats this module documented `probe`/`write` diagnostic workflow; cross-reference the `Driver.Cli.Common` review. -**Resolution:** Deferred 2026-05-23 — the recommended fix lives in -`SnapshotFormatter.FormatStatus` inside the `Driver.Cli.Common` shared module, -which is outside this module's edit scope. Driver.Cli.Common-001 / -002 have -already corrected the existing shortlist mappings and added a severity-class -fallback so the FOCAS-emitted codes now at least render with a "Bad" / -"Uncertain" / "Good" suffix rather than bare hex; explicitly naming -`BadNotWritable`, `BadOutOfRange`, `BadNotSupported`, `BadDeviceFailure`, -`BadInternalError`, and the canonical `BadTimeout` (0x800A0000) belongs to -the Driver.Cli.Common review's follow-up (and benefits every driver CLI, not -just FOCAS). Re-open here only if Driver.Cli.Common declines to extend the -shortlist. +**Resolution:** Resolved 2026-05-23 — the cross-CLI fix landed in `Driver.Cli.Common`: +`SnapshotFormatter.FormatStatus` now names `BadInternalError` (0x80020000), +`BadNotWritable` (0x803B0000), `BadOutOfRange` (0x803C0000), `BadNotSupported` +(0x803D0000), and `BadDeviceFailure` (0x80550000) — the five codes the FOCAS / +AbCip / AbLegacy native-protocol mappers all emit but the shortlist previously +left unnamed (the canonical `BadTimeout` 0x800A0000 was already added under +Driver.Cli.Common-001). FOCAS `probe` / `write` against a non-writable parameter, +out-of-range address, unsupported function, busy device, or CNC-handle failure +now renders with the named status the `docs/Driver.FOCAS.Cli.md` workflow +promises, restoring parity between the docs and the shipped behaviour. Regression +`[Theory]` `FormatStatus_names_native_driver_emitted_codes` added to +`SnapshotFormatterTests` so the five names can't silently drop out of the +shortlist again; the existing well-known shortlist `[Theory]` was extended with +the same five entries to enforce the exact `0x... (Name)` rendering. Suite now +47 green (was 42). diff --git a/code-reviews/README.md b/code-reviews/README.md index 7e3924d..4711e69 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -327,7 +327,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Driver.FOCAS.Cli-002 | Low | Resolved | Concurrency & thread safety | `Commands/SubscribeCommand.cs:45-51` | | Driver.FOCAS.Cli-003 | Low | Resolved | Error handling & resilience | `FocasCommandBase.cs:19` (`CncPort`), `FocasCommandBase.cs:27` (`TimeoutMs`), `Commands/SubscribeCommand.cs:23` (`IntervalMs`) | | Driver.FOCAS.Cli-004 | Low | Resolved | Performance & resource management | `Commands/ProbeCommand.cs:37,54`; `Commands/ReadCommand.cs:37,46`; `Commands/WriteCommand.cs:45,54`; `Commands/SubscribeCommand.cs:39,73` | -| Driver.FOCAS.Cli-005 | Low | Deferred | Design-document adherence | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) | +| Driver.FOCAS.Cli-005 | Low | Resolved | Design-document adherence | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) | | Driver.Galaxy-005 | Low | Resolved | OtOpcUa conventions | `Runtime/EventPump.cs:81-88` | | Driver.Galaxy-010 | Low | Resolved | Security | `GalaxyDriver.cs:311-341` | | Driver.Galaxy-012 | Low | Resolved | Performance & resource management | `Runtime/SubscriptionRegistry.cs:65-67`, `GalaxyDriver.cs:538`, `GalaxyDriver.cs:675` | diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs index 1a6c3e3..1005e56 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs @@ -116,12 +116,17 @@ public static class SnapshotFormatter { 0x00000000u => "Good", 0x80000000u => "Bad", + 0x80020000u => "BadInternalError", 0x80050000u => "BadCommunicationError", 0x800A0000u => "BadTimeout", 0x80310000u => "BadNoCommunication", 0x80320000u => "BadWaitingForInitialData", 0x80340000u => "BadNodeIdUnknown", 0x80330000u => "BadNodeIdInvalid", + 0x803B0000u => "BadNotWritable", + 0x803C0000u => "BadOutOfRange", + 0x803D0000u => "BadNotSupported", + 0x80550000u => "BadDeviceFailure", 0x80740000u => "BadTypeMismatch", 0x40000000u => "Uncertain", _ => null, diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs index 4bfe303..f6d1d52 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs @@ -29,12 +29,17 @@ public sealed class SnapshotFormatterTests // Numeric codes are the canonical OPC Foundation Opc.Ua.StatusCodes values. [InlineData(0x00000000u, "Good")] [InlineData(0x80000000u, "Bad")] + [InlineData(0x80020000u, "BadInternalError")] [InlineData(0x80050000u, "BadCommunicationError")] [InlineData(0x800A0000u, "BadTimeout")] [InlineData(0x80310000u, "BadNoCommunication")] [InlineData(0x80320000u, "BadWaitingForInitialData")] [InlineData(0x80340000u, "BadNodeIdUnknown")] [InlineData(0x80330000u, "BadNodeIdInvalid")] + [InlineData(0x803B0000u, "BadNotWritable")] + [InlineData(0x803C0000u, "BadOutOfRange")] + [InlineData(0x803D0000u, "BadNotSupported")] + [InlineData(0x80550000u, "BadDeviceFailure")] [InlineData(0x80740000u, "BadTypeMismatch")] [InlineData(0x40000000u, "Uncertain")] public void FormatStatus_names_well_known_status_codes(uint status, string expectedName) @@ -42,6 +47,22 @@ public sealed class SnapshotFormatterTests SnapshotFormatter.FormatStatus(status).ShouldBe($"0x{status:X8} ({expectedName})"); } + [Theory] + // Driver.FOCAS.Cli-005 follow-up (Driver.Cli.Common): the FOCAS / AbCip / AbLegacy mappers + // emit these five codes — they previously rendered only as severity-class "Bad" because the + // shortlist did not name them, defeating the operator workflow that docs/Driver.*.Cli.md + // promise (read the BadDeviceFailure / BadNotWritable / ... name straight off probe/write + // output). Pin them named so the docs stay accurate. + [InlineData(0x80020000u, "BadInternalError")] + [InlineData(0x803B0000u, "BadNotWritable")] + [InlineData(0x803C0000u, "BadOutOfRange")] + [InlineData(0x803D0000u, "BadNotSupported")] + [InlineData(0x80550000u, "BadDeviceFailure")] + public void FormatStatus_names_native_driver_emitted_codes(uint status, string expectedName) + { + SnapshotFormatter.FormatStatus(status).ShouldContain($"({expectedName})"); + } + [Theory] // Regression for Driver.Cli.Common-001: these codes were previously mapped to the // wrong names. The hex values below are what the buggy shortlist used; they must