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

review(Driver.FOCAS): add byte-level wire-protocol test coverage

Browse Source 
Re-review at 7286d320. -013 (Medium, testing): the managed FOCAS/2 wire-decode layer
(BuildPdu/ParseResponseBlocks, incl. cnc_getfigure stride) had zero byte-level tests; added
15 (no decode bug found). -014 (spindle-load truncation heuristic) deferred bench-gated.
Note: runtime read path is now pure-managed TCP (no P/Invoke except the probe handshake).
This commit is contained in:
Joseph Doherty
2026-06-19 11:47:11 -04:00
parent 22f7d92b72
commit e07a4fbf52
2 changed files with 378 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Driver.FOCAS/findings.md
+116 -2
View File
@@ -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).
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasWireProtocolTests.cs
+262
View File
@@ -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;
/// <summary>
/// Offline byte-level coverage for the managed FOCAS/2 wire-protocol decode layer
/// (<see cref="FocasWireProtocol"/>) — 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 <c>cnc_getfigure</c> figure stride) ran without a
/// single byte-level test; every other test drives the <c>IFocasClient</c> seam through the
/// Fake, which bypasses all framing. Recorded as Driver.FOCAS-013.
/// </summary>
[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<ArgumentOutOfRangeException>(() =>
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<FocasWireException>(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<FocasWireException>(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<byte>()));
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<FocasWireException>(() => 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<FocasWireException>(() => 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<int>();
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 ----
/// <summary>Build a response body in the 0x21 response-block shape ParseResponseBlocks expects.</summary>
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(); }
}
}