From a92fb81f00c2467b483c729268744de11fec1b16 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 21:26:26 -0400 Subject: [PATCH] =?UTF-8?q?fix(twincat):=20review=20=E2=80=94=20a=20connec?= =?UTF-8?q?t=20failure=20no=20longer=20burns=20the=20sum-chunk=20warning;?= =?UTF-8?q?=20doc=20precision?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ReadBucketAsync: the once-per-device "sum-read chunk failed" Warning fires only after a successful connect; a connect/backoff failure is the connect path's to report. TwinCATSumChunkWarningTests pins it (no warning across connect failures, exactly one across two later failing sum polls; red without the guard) plus a source guard that ReadGroupAsync builds its SumRead only via CreateSumRead. - Docs: diagnostics live-gated via the driver harness (/hosts not exercised); TC2 costs 8 reads on the first tick and after each reconnect, then <= 4; read counts attributed to the unit tests. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- CLAUDE.md | 7 +- docs/drivers/TwinCAT.md | 15 ++- .../TwinCATDriver.cs | 6 +- .../TwinCATSumChunkWarningTests.cs | 99 +++++++++++++++++++ 4 files changed, 118 insertions(+), 9 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSumChunkWarningTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index c92bf72e..25397bac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1151,9 +1151,10 @@ host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT fa (`CheckNotification`) rides a batch; value-converted pairs (DInt over INT, Real over LREAL/DINT) read singly. Only overall ADS 1793/1794 latch the device to single reads; any other non-comms overall code (1792/1795/1797/1798...) reads that chunk singly for the poll without latching, with a once-per-device Warning; a comms failure neither. Opt-in `diagnosticsEnabled` (default off) puts ADS state/app/online-change/cycle/exec/`HeadroomMs` on the `/hosts` - `IDriverDiagnostics` bag (up to 4 extra reads per device per probe tick; TC2 falls back to `.SYSTEMINFO` / - `.SYSTEMTASKINFOARR[1]`). Live-gated read-only 2026-10-08 on Z35474 (TC3) + Z34834 (TC2, which answers sum - commands); the gate found ⚠️ Beckhoff's `SumRead` ctor throws for a `List<>` of entities (only an array survives + `IDriverDiagnostics` bag (≤ 4 extra reads per device per probe tick once settled; TC2 falls back to `.SYSTEMINFO` / + `.SYSTEMTASKINFOARR[1]`, costing 8 on the first tick and after each reconnect). Live-gated read-only 2026-10-08 on + Z35474 (TC3) + Z34834 (TC2, which answers sum commands) via a harness driving `TwinCATDriver` — `/hosts` end-to-end + not run (a stock host has no AMS router); the gate found ⚠️ Beckhoff's `SumRead` ctor throws for a `List<>` of entities (only an array survives its internal cast) — `AdsTwinCATClient.CreateSumRead` owns it. Flat browse stays (VirtualTree is a strict subset on TC3). Sum-write not ported. - **AbLegacy (#636):** `AbLegacyAddress` (in `AbLegacy.Contracts`) is family-aware: PLC-5 `I:`/`O:` indices are OCTAL diff --git a/docs/drivers/TwinCAT.md b/docs/drivers/TwinCAT.md index ec85d5de..22477d55 100644 --- a/docs/drivers/TwinCAT.md +++ b/docs/drivers/TwinCAT.md @@ -224,12 +224,16 @@ command is unverified, hence opt-in. ### `/hosts` diagnostics (`IDriverDiagnostics`, opt-in) -Behind `diagnosticsEnabled` (default **off** — not yet live-gated). The driver implements the optional +Behind `diagnosticsEnabled` (default **off**). Live-gated read-only 2026-10-08 on Z35474 (TC3) and Z34834 (TC2) by +driving `TwinCATDriver` from a scratch harness with an in-process AMS router; the `/hosts` page end-to-end was +**not** exercised (a stock host has no AMS router). The driver implements the optional `IDriverDiagnostics` bag (the S7 `NegotiatedPduSize` precedent, #640), deliberately not synthetic raw tags. When enabled, after each healthy probe tick **and after the host-state transition** it reads up to four `TwinCAT_SystemInfoVarList` symbols per device — `_AppInfo.AppName`, `_AppInfo.OnlineChangeCnt`, -`_TaskInfo[1].CycleTime`, `_TaskInfo[1].LastExecTime` (100 ns ticks) — so up to **4 extra read requests per -device per probe interval** (default 5 s). The ADS state is the one `ProbeAsync` already read; no second +`_TaskInfo[1].CycleTime`, `_TaskInfo[1].LastExecTime` (100 ns ticks) — so at most **4 extra read requests per +device per probe interval** (default 5 s) once settled. On **TwinCAT 2** (fallback below) the first tick, and the +first tick after every reconnect, costs **8** (the four TC3 names answer "not found", then the four TC2 names); +after that ≤ 4. The ADS state is the one `ProbeAsync` already read; no second ReadState. One device: flat keys `TwinCAT.AdsState`, `.AppName`, `.OnlineChangeCnt`, `.CycleTimeMs`, `.LastExecTimeMs`, `.HeadroomMs` (cycle − last exec; negative = the task overran; this is spare time, not jitter). Several devices: one line per device, `TwinCAT.` = @@ -239,8 +243,9 @@ best-effort: a runtime that does not export one keeps the others, and a symbol a found" is not re-asked until a new client is installed (reconnect). **TwinCAT 2** has no `TwinCAT_SystemInfoVarList`; once a TC3 name answers "not found" the driver reads the TC2 globals instead — `.SYSTEMINFO.PROJECTNAME` (AppName), `.SYSTEMINFO.ONLINECHANGECOUNT` (UINT), `.SYSTEMTASKINFOARR[1].CYCLETIME` / -`.LASTEXECTIME` (same 100 ns units). Live on TC2 2.11: 8 reads on the first tick, 4 after; a TC3 target never -reads the TC2 names. +`.LASTEXECTIME` (same 100 ns units). The read counts — 8 on the first tick and after each reconnect, 4 after, and +none of the TC2 names on a TC3 target — are pinned by `TwinCATDiagnosticsTests`; the live run proved the values +(all six keys on TC2 2.11), not the counts. ## Capability surface diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs index 649a6a76..c2d57855 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -386,9 +386,13 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery continue; } + // Set once the connect succeeded: a connect / backoff failure is the connect path's to report (it + // logs its own Warning) and must not consume the once-per-device sum-chunk-failure Warning below. + var connected = false; try { var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false); + connected = true; var items = new TwinCATBulkReadItem[chunk.Count]; for (var k = 0; k < chunk.Count; k++) items[k] = new TwinCATBulkReadItem(chunk[k].SymbolName, chunk[k].Def.DataType, chunk[k].BitIndex, chunk[k].Def.ArrayLength); @@ -429,7 +433,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); // Once per device: the health message is overwritten by the next good read in the same poll, so // without this a sum-read path failing on every poll left no trace (hw2 live gate, 2026-10-08). - if (!device.SumChunkFailureWarned) + if (connected && !device.SumChunkFailureWarned) { device.SumChunkFailureWarned = true; _logger.LogWarning(ex, diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSumChunkWarningTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSumChunkWarningTests.cs new file mode 100644 index 00000000..948705b3 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSumChunkWarningTests.cs @@ -0,0 +1,99 @@ +using System.Text.RegularExpressions; +using Microsoft.Extensions.Logging; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests; + +/// +/// hw2 review of the live-gate fix: the once-per-device "sum-read chunk failed" Warning exists to surface a +/// sum-read path that fails on every poll (the gate's silent failure). A device that is merely unreachable must +/// not consume it — the connect path reports that itself — or the later real defect is logged nowhere. +/// +[Trait("Category", "Unit")] +public sealed class TwinCATSumChunkWarningTests +{ + private const string Host = "ads://5.23.91.23.1.1:851"; + private const string ChunkWarning = "an ADS sum-read chunk"; + + [Fact] + public async Task A_connect_failure_does_not_burn_the_warning_and_a_later_real_chunk_failure_warns_once() + { + var connectFails = true; + var factory = new FakeTwinCATClientFactory + { + Customise = () => new FakeTwinCATClient + { + ThrowOnConnect = connectFails, + Exception = new InvalidOperationException("device down"), + SumReadException = new InvalidOperationException("sum path broken"), + }, + }; + var logger = new CapturingLogger(); + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host)], + RawTags = TwinCATRawTags.Entries([ + new TwinCATTagDefinition("A", Host, "MAIN.A", TwinCATDataType.DInt), + new TwinCATTagDefinition("B", Host, "MAIN.B", TwinCATDataType.DInt), + ]), + Probe = new TwinCATProbeOptions { Enabled = false }, + UseSumRead = true, + SumReadMaxItems = 10, + }, "drv-warn", factory, logger); + await drv.InitializeAsync("{}", CancellationToken.None); + + // Unreachable: the connect attempt fails, then the backoff window rejects further attempts. + for (var i = 0; i < 3; i++) + (await drv.ReadAsync(["A", "B"], CancellationToken.None)) + .ShouldAllBe(s => s.StatusCode == TwinCATStatusMapper.BadCommunicationError); + logger.Warnings.Count(w => w.Contains(ChunkWarning)).ShouldBe(0); + + // Device comes back, but the sum path itself fails: wait out the 1 s connect backoff, then two polls. + connectFails = false; + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(5); + while (!factory.Clients.Any(c => c.IsConnected) && DateTime.UtcNow < deadline) + { + await drv.ReadAsync(["A", "B"], CancellationToken.None); + await Task.Delay(50); + } + factory.Clients.Any(c => c.IsConnected).ShouldBeTrue("the device must reconnect for the second half to mean anything"); + await drv.ReadAsync(["A", "B"], CancellationToken.None); + await drv.ReadAsync(["A", "B"], CancellationToken.None); + + logger.Warnings.Count(w => w.Contains(ChunkWarning)).ShouldBe(1); + } + + /// Source guard: the driver's only Beckhoff SumRead construction is inside + /// CreateSumRead, so re-inlining a List-based construction in ReadGroupAsync goes red. + [Fact] + public void ReadGroupAsync_builds_its_sum_command_through_CreateSumRead_only() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx"))) dir = dir.Parent; + dir.ShouldNotBeNull("repo root not found"); + var src = File.ReadAllText(Path.Combine(dir!.FullName, + "src", "Drivers", "ZB.MOM.WW.OtOpcUa.Driver.TwinCAT", "AdsTwinCATClient.cs")); + + Regex.Matches(src, @"new\s+SumRead\s*\(").Count.ShouldBe(1); + var create = src.IndexOf("internal static SumRead CreateSumRead(", StringComparison.Ordinal); + create.ShouldBeGreaterThan(0); + var ctor = Regex.Match(src, @"new\s+SumRead\s*\(").Index; + ctor.ShouldBeGreaterThan(create); + src.IndexOf("private const uint SymbolValueByHandle", StringComparison.Ordinal).ShouldBeGreaterThan(ctor); + Regex.IsMatch(src, @"await\s+CreateSumRead\(_client").ShouldBeTrue(); + } + + private sealed class CapturingLogger : ILogger + { + public List Warnings { get; } = []; + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + { + if (logLevel == LogLevel.Warning) lock (Warnings) Warnings.Add(formatter(state, exception)); + } + } +}