diff --git a/code-reviews/Driver.FOCAS/findings.md b/code-reviews/Driver.FOCAS/findings.md index fbb4ccc8..0ce1a2d8 100644 --- a/code-reviews/Driver.FOCAS/findings.md +++ b/code-reviews/Driver.FOCAS/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `04e0877b` (re-review; prior `76d35d1`) | | Status | Reviewed | | Open findings | 0 | @@ -328,3 +328,117 @@ three opt-in sections and assert the options reach the driver; add a fake client mid-session and asserts recovery. **Resolution:** Resolved 2026-05-22 — Added `FocasDriverMediumFindingsTests.cs` covering: unknown-DeviceHostAddress init throw (003), ViewOnly enforcement for all tags (004), Volatile `_health` under concurrent reads (005), reconnect-after-external-dispose recovery (006), and a factory full-round-trip test for all three opt-in config sections (012). + +## Re-review 2026-06-19 (commit 04e0877b) + +Re-review of the FOCAS driver at HEAD (`04e0877b`, which descends from `7286d320`). +Since the prior review at `76d35d1` the driver gained ~1,200 lines, most notably the +pure-managed FOCAS/2 Ethernet **wire backend** (`Wire/FocasWireClient.cs`, +`Wire/FocasWireProtocol.cs`, `Wire/WireFocasClient.cs`, `Wire/FocasConstants.cs`) and +the `cnc_getfigure` per-axis position auto-scale path (`ReadPositionFiguresAsync` + +`AxisFactor`). NB: there is **no P/Invoke** in the runtime read path — the wire backend +speaks the Fanuc binary protocol directly over TCP. The only P/Invoke is the +`cnc_allclibhndl3` / `cnc_freelibhndl` Test-Connect handshake in `FocasDriverProbe.cs` +(marshalling reviewed below). + +All 12 prior findings remain Resolved. Two new findings recorded below. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.FOCAS-014 (deferred) — otherwise no issues found | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No new issues found (see note) | +| 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 | Driver.FOCAS-013 | +| 10 | Documentation & comments | No issues found | + +**Interop / marshalling note (P/Invoke probe).** `FocasDriverProbe.NativeFwlib` declares +`cnc_allclibhndl3([MarshalAs(LPStr)] string ipaddr, ushort port, int timeout, out ushort handle)` +with `CallingConvention.Cdecl` + `CharSet.Ansi`. This matches the published FWLIB +signature (`short cnc_allclibhndl3(const char*, unsigned short, long, unsigned short*)`). +The `out ushort handle` is freed best-effort in a `finally` even on a timeout race, and the +whole call is wrapped so a `DllNotFoundException` / load failure degrades to TCP-only — sound. +The one residual subtlety (FWLIB `long` is 32-bit on Win32 → `int` is correct on the +typical x86 worker; on an LP64 Linux `libfwlib32.so` the C `long` is 64-bit and `int` +would mis-marshal) is **bench-gated** (no FWLIB on this dev/CI host) and is recorded as +context only, not a separate finding, because the project ships no Linux-FWLIB path today. + +**Concurrency note (not a new finding).** `FocasDriver.DeviceState.Client` is a plain +auto-property read/written without a lock from `EnsureConnectedAsync` (Read/Write/probe/ +fixed-tree threads) and `RecycleLoopAsync`/`DisposeClient`. A `HandleRecycle`-enabled +config can therefore race a dispose against an in-flight read's captured client reference. +This is the same lifecycle class as the already-Resolved Driver.FOCAS-006; `HandleRecycle` +is disabled by default and the wire client's own `_lifetimeGate`/`_requestGate` plus the +"reconnect on next call" recovery make the window self-correcting. Left as-is (no new +finding) — hardening it to a per-device lock is a larger change with no offline-observable +defect. + +#### Driver.FOCAS-013 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Testing coverage | +| Location | `Wire/FocasWireProtocol.cs`, `Wire/FocasWireClient.cs` (`ReadPositionFiguresAsync`, `ParseAlarms`, name/sysinfo decode) | +| Status | Resolved | + +**Description:** The entire managed wire-protocol decode layer — the byte-offset-fragile +code that is this driver's analogue of P/Invoke struct marshalling — has **zero** unit +tests. No test in `Driver.FOCAS.Tests` references `FocasWireProtocol`, `FocasWireClient`, +`ParseResponseBlocks`, `BuildPdu`/`BuildRequestBody`, `ReadAscii`/`ReadNameRecord`, or the +new `cnc_getfigure` figure decode (`ReadPositionFiguresAsync`, added since the prior review). +Every existing test drives the `IFocasClient` seam through `FakeFocasClient`, which bypasses +all wire framing and big-endian decode. A regression in a block-envelope offset, a PDU +length field, the ASCII NUL/space trimming, or the figure `short` stride would not be caught +by any test — exactly the corruption class the review brief calls out for marshalling code. + +**Recommendation:** Add offline (no-socket) unit tests for the public/internal static decode +primitives: `BuildPdu` header layout + `ReadPduAsync` round-trip; `BuildRequestBody` block +count/stride; `ParseResponseBlocks` (command/RC/payload extraction, truncation guards); +`ReadAscii` (NUL stop + trailing-space/NUL trim) and `ReadNameRecord`; and a +`ResponseBlock`-shaped payload that exercises the `cnc_getfigure` figure `short` stride. + +**Resolution:** Resolved 2026-06-19 — Added `FocasWireProtocolTests.cs` (15 tests) covering +`BuildPdu` header layout + magic/version/length, the `BuildPdu`→`ReadPdu` round-trip, +`BuildRequestBody` block framing, `ParseResponseBlocks` (multi-block command/RC/payload +extraction + the truncated-length and bad-block-length guards), `ReadAscii` NUL-stop and +trailing space/NUL trimming, `ReadNameRecord` 2-byte extraction, and a response-block payload +decoded into the per-axis `short` figure sequence that the `cnc_getfigure` path relies on. + +#### Driver.FOCAS-014 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `Wire/WireFocasClient.cs:318-325` (`ReadSpindleMetricAsync` trailing-zero truncation) | +| Status | Deferred | + +**Description:** `WireFocasClient.ReadSpindleMetricAsync` (the shared decode for +`GetSpindleLoadsAsync` / `GetSpindleMaxRpmsAsync`) stops accumulating at the first zero value +after a non-zero one: `if (m.Value == 0 && list.Count > 0) break;`. The intent (per the inline +comment) is to drop Fanuc's trailing zero-padding of unused spindle slots. But a spindle that +is legitimately at **0% load** (stopped) sitting *between* two running spindles truncates the +list at that point, dropping every subsequent spindle. Because the consumer in +`FocasDriver.FixedTreeLoopAsync` index-aligns the returned list to the spindle order +(`state.LastSpindleLoads[i] = loads[i]`) and the read path looks up `Load` by that index, a +dropped middle element **misaligns all later spindles' Load values**. The truncation is correct +only if the CNC always returns a contiguous run of active spindles followed by zero padding — +which holds for single-spindle and trailing-stopped layouts but not for a stopped middle +spindle. + +**Recommendation:** Distinguish "trailing padding" from "a real zero in the middle". Either read +the actual spindle count from `cnc_rdspdlname` / sysinfo and decode exactly that many fixed-width +slots (no zero-based truncation), or keep zeros and trim only a trailing run of zeros after +decoding the full payload. + +**Resolution:** Deferred — the correct slot-count/padding semantics of `cnc_rdspload` / +`cnc_rdspmaxrpm` depend on the real Fanuc wire response shape, which is bench-CNC-gated (this +managed backend's binary shapes are validated only against the in-tree `focas_mock` sim, per the +`Wire/FocasWireClient.cs` remarks). Fixing the truncation heuristic without a real CNC's padding +behaviour risks substituting one wrong assumption for another. Waiting on a bench-CNC verification +pass (the same gate that covers the `cnc_getfigure` binary shape). diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasWireProtocolTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasWireProtocolTests.cs new file mode 100644 index 00000000..1bf7b0e9 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasWireProtocolTests.cs @@ -0,0 +1,262 @@ +using System.Buffers.Binary; +using System.Net.Sockets; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// Offline byte-level coverage for the managed FOCAS/2 wire-protocol decode layer +/// () — the big-endian framing + block-envelope decode that +/// is this driver's analogue of P/Invoke struct marshalling. Before this suite the entire +/// wire decode surface (including the cnc_getfigure figure stride) ran without a +/// single byte-level test; every other test drives the IFocasClient seam through the +/// Fake, which bypasses all framing. Recorded as Driver.FOCAS-013. +/// +[Trait("Category", "Unit")] +public sealed class FocasWireProtocolTests +{ + // ---- BuildPdu header layout ---- + + [Fact] + public void BuildPdu_writes_magic_version_type_direction_and_length() + { + var body = new byte[] { 0xde, 0xad, 0xbe, 0xef }; + var pdu = FocasWireProtocol.BuildPdu( + FocasWireProtocol.TypeData, FocasWireProtocol.DirectionRequest, body); + + pdu.Length.ShouldBe(10 + body.Length); + pdu[0].ShouldBe((byte)0xa0); + pdu[1].ShouldBe((byte)0xa0); + pdu[2].ShouldBe((byte)0xa0); + pdu[3].ShouldBe((byte)0xa0); + BinaryPrimitives.ReadUInt16BigEndian(pdu.AsSpan(4, 2)).ShouldBe(FocasWireProtocol.Version); + pdu[6].ShouldBe(FocasWireProtocol.TypeData); + pdu[7].ShouldBe(FocasWireProtocol.DirectionRequest); + BinaryPrimitives.ReadUInt16BigEndian(pdu.AsSpan(8, 2)).ShouldBe((ushort)body.Length); + pdu.AsSpan(10).ToArray().ShouldBe(body); + } + + [Fact] + public void BuildPdu_rejects_a_body_larger_than_ushort_max() + { + var tooBig = new byte[ushort.MaxValue + 1]; + Should.Throw(() => + FocasWireProtocol.BuildPdu(FocasWireProtocol.TypeData, FocasWireProtocol.DirectionRequest, tooBig)); + } + + [Fact] + public void BuildInitiateBody_writes_the_socket_index_big_endian() + { + var body = FocasWireProtocol.BuildInitiateBody(2); + body.Length.ShouldBe(2); + BinaryPrimitives.ReadUInt16BigEndian(body).ShouldBe((ushort)2); + } + + // ---- BuildPdu -> ReadPduAsync round-trip over a real socket pair ---- + + [Fact] + public async Task BuildPdu_round_trips_through_ReadPduAsync() + { + var body = new byte[] { 1, 2, 3, 4, 5 }; + var pdu = FocasWireProtocol.BuildPdu( + FocasWireProtocol.TypeData, FocasWireProtocol.DirectionResponse, body); + + var (client, server) = await ConnectedPairAsync(); + try + { + await server.GetStream().WriteAsync(pdu); + var read = await FocasWireProtocol.ReadPduAsync(client.GetStream(), CancellationToken.None); + + read.Type.ShouldBe(FocasWireProtocol.TypeData); + read.Direction.ShouldBe(FocasWireProtocol.DirectionResponse); + read.Body.ShouldBe(body); + } + finally { client.Dispose(); server.Dispose(); } + } + + [Fact] + public async Task ReadPduAsync_rejects_bad_magic() + { + var bad = new byte[10]; + bad[0] = 0x00; // not 0xa0 + var (client, server) = await ConnectedPairAsync(); + try + { + await server.GetStream().WriteAsync(bad); + await Should.ThrowAsync(async () => + await FocasWireProtocol.ReadPduAsync(client.GetStream(), CancellationToken.None)); + } + finally { client.Dispose(); server.Dispose(); } + } + + [Fact] + public async Task ReadPduAsync_rejects_unsupported_version() + { + var header = new byte[10]; + new byte[] { 0xa0, 0xa0, 0xa0, 0xa0 }.CopyTo(header, 0); + BinaryPrimitives.WriteUInt16BigEndian(header.AsSpan(4, 2), 99); // unsupported version + var (client, server) = await ConnectedPairAsync(); + try + { + await server.GetStream().WriteAsync(header); + await Should.ThrowAsync(async () => + await FocasWireProtocol.ReadPduAsync(client.GetStream(), CancellationToken.None)); + } + finally { client.Dispose(); server.Dispose(); } + } + + // ---- BuildRequestBody framing ---- + + [Fact] + public void BuildRequestBody_prefixes_the_block_count_and_concatenates_blocks() + { + var blocks = new[] + { + new RequestBlock(0x0018, PathId: 1), + new RequestBlock(0x0019, Arg1: 7, PathId: 1), + }; + var body = FocasWireProtocol.BuildRequestBody(blocks); + + BinaryPrimitives.ReadUInt16BigEndian(body.AsSpan(0, 2)).ShouldBe((ushort)2); + // First block sits right after the 2-byte count; its declared block length is at +0. + var firstLen = BinaryPrimitives.ReadUInt16BigEndian(body.AsSpan(2, 2)); + firstLen.ShouldBe((ushort)0x1c); // no extra payload → header-only block + BinaryPrimitives.ReadUInt16BigEndian(body.AsSpan(2 + 6, 2)).ShouldBe((ushort)0x0018); // command @ +6 + } + + // ---- ParseResponseBlocks: command / RC / payload extraction ---- + + [Fact] + public void ParseResponseBlocks_decodes_command_rc_and_payload() + { + var payload = new byte[] { 0xAA, 0xBB, 0xCC, 0xDD }; + var body = BuildResponseBody( + (command: (ushort)0x0018, rc: (short)0, payload: payload), + (command: (ushort)0x0019, rc: (short)6 /* EW_NOOPT */, payload: Array.Empty())); + + var blocks = FocasWireProtocol.ParseResponseBlocks(body); + + blocks.Count.ShouldBe(2); + blocks[0].Command.ShouldBe((ushort)0x0018); + blocks[0].Rc.ShouldBe((short)0); + blocks[0].Payload.ShouldBe(payload); + blocks[1].Command.ShouldBe((ushort)0x0019); + blocks[1].Rc.ShouldBe((short)6); + blocks[1].Payload.ShouldBeEmpty(); + } + + [Fact] + public void ParseResponseBlocks_returns_empty_for_a_body_shorter_than_the_count_field() + { + FocasWireProtocol.ParseResponseBlocks(new byte[] { 0x00 }).ShouldBeEmpty(); + } + + [Fact] + public void ParseResponseBlocks_throws_on_a_truncated_block_length_field() + { + // count says 1 block, but only one stray byte follows the count. + var body = new byte[] { 0x00, 0x01, 0x00 }; + Should.Throw(() => FocasWireProtocol.ParseResponseBlocks(body)); + } + + [Fact] + public void ParseResponseBlocks_throws_when_the_declared_block_length_is_below_the_header() + { + var body = new byte[4]; + BinaryPrimitives.WriteUInt16BigEndian(body.AsSpan(0, 2), 1); // 1 block + BinaryPrimitives.WriteUInt16BigEndian(body.AsSpan(2, 2), 0x08); // block length < 0x10 + Should.Throw(() => FocasWireProtocol.ParseResponseBlocks(body)); + } + + // ---- The cnc_getfigure figure stride: decode a response block into shorts ---- + + [Fact] + public void ParseResponseBlocks_yields_a_payload_that_decodes_as_a_sequence_of_figure_shorts() + { + // cnc_getfigure (command 0x00d3) returns one big-endian short per axis. Build the + // response block and assert the payload decodes to the [3, 1, 0] figure sequence the + // driver's auto-scale path reads. + var figurePayload = new byte[6]; + BinaryPrimitives.WriteInt16BigEndian(figurePayload.AsSpan(0, 2), 3); + BinaryPrimitives.WriteInt16BigEndian(figurePayload.AsSpan(2, 2), 1); + BinaryPrimitives.WriteInt16BigEndian(figurePayload.AsSpan(4, 2), 0); + + var body = BuildResponseBody((command: (ushort)0x00d3, rc: (short)0, payload: figurePayload)); + var block = FocasWireProtocol.ParseResponseBlocks(body).Single(); + + var figures = new List(); + for (var offset = 0; offset + 2 <= block.Payload.Length; offset += 2) + figures.Add(BinaryPrimitives.ReadInt16BigEndian(block.Payload.AsSpan(offset, 2))); + + figures.ShouldBe(new[] { 3, 1, 0 }); + } + + // ---- ReadAscii / ReadNameRecord ---- + + [Fact] + public void ReadAscii_stops_at_the_first_NUL_and_trims_trailing_space() + { + var bytes = "30i \0junkafternul"u8.ToArray(); + FocasWireProtocol.ReadAscii(bytes).ShouldBe("30i"); + } + + [Fact] + public void ReadAscii_trims_trailing_spaces_without_a_NUL() + { + FocasWireProtocol.ReadAscii("A1.0 "u8.ToArray()).ShouldBe("A1.0"); + } + + [Fact] + public void ReadNameRecord_reads_two_bytes_and_strips_padding() + { + FocasWireProtocol.ReadNameRecord("X \0\0"u8.ToArray()).ShouldBe("X"); + FocasWireProtocol.ReadNameRecord("XY"u8.ToArray()).ShouldBe("XY"); + FocasWireProtocol.ReadNameRecord(new byte[] { 0x58 }).ShouldBe(string.Empty); // < 2 bytes + } + + // ---- helpers ---- + + /// Build a response body in the 0x21 response-block shape ParseResponseBlocks expects. + private static byte[] BuildResponseBody(params (ushort command, short rc, byte[] payload)[] blocks) + { + var encoded = blocks.Select(b => + { + var blockLen = 0x10 + b.payload.Length; + var block = new byte[blockLen]; + BinaryPrimitives.WriteUInt16BigEndian(block.AsSpan(0, 2), (ushort)blockLen); + BinaryPrimitives.WriteUInt16BigEndian(block.AsSpan(6, 2), b.command); + BinaryPrimitives.WriteInt16BigEndian(block.AsSpan(8, 2), b.rc); + BinaryPrimitives.WriteUInt16BigEndian(block.AsSpan(14, 2), (ushort)b.payload.Length); + b.payload.CopyTo(block.AsSpan(16)); + return block; + }).ToArray(); + + var body = new byte[2 + encoded.Sum(b => b.Length)]; + BinaryPrimitives.WriteUInt16BigEndian(body.AsSpan(0, 2), (ushort)encoded.Length); + var offset = 2; + foreach (var block in encoded) + { + block.CopyTo(body.AsSpan(offset)); + offset += block.Length; + } + return body; + } + + private static async Task<(TcpClient client, TcpClient server)> ConnectedPairAsync() + { + var listener = new TcpListener(System.Net.IPAddress.Loopback, 0); + listener.Start(); + try + { + var port = ((System.Net.IPEndPoint)listener.LocalEndpoint).Port; + var client = new TcpClient(); + var connect = client.ConnectAsync(System.Net.IPAddress.Loopback, port); + var server = await listener.AcceptTcpClientAsync(); + await connect; + return (client, server); + } + finally { listener.Stop(); } + } +}