diff --git a/docs/Driver.TwinCAT.Cli.md b/docs/Driver.TwinCAT.Cli.md index 0cbd882..36849ec 100644 --- a/docs/Driver.TwinCAT.Cli.md +++ b/docs/Driver.TwinCAT.Cli.md @@ -77,6 +77,14 @@ otopcua-twincat-cli read -n 192.168.1.40.1.1 -s "Recipe[3]" -t Real otopcua-twincat-cli read -n 192.168.1.40.1.1 -s GVL.sMessage -t WString ``` +ADS variable handles for `read` / `write` symbols are cached transparently +inside the CLI's underlying `AdsTwinCATClient`. The first read of a symbol +resolves a handle; repeats reuse the cached handle for smaller AMS payloads +and skipped name resolution. The cache wipes on reconnect, on +`DeviceSymbolVersionInvalid` (with a one-shot retry), and on CLI exit. See +`docs/drivers/TwinCAT-Test-Fixture.md §Handle caching` for the full story +including the staleness caveat after an online change. + ### `write` ```powershell @@ -99,3 +107,7 @@ otopcua-twincat-cli subscribe -n 192.168.1.40.1.1 -s GVL.Counter -t DInt -i 500 The subscribe banner announces which mechanism is in play — "ADS notification" or "polling" — so it's obvious in screen-recorded bug reports. + +`--poll-only` polls go through the same cached-handle path as `read`, so +repeated polls of the same symbol carry only a 4-byte handle on the wire +rather than the full symbolic path. diff --git a/docs/drivers/TwinCAT-Test-Fixture.md b/docs/drivers/TwinCAT-Test-Fixture.md index 2910eec..daa1b40 100644 --- a/docs/drivers/TwinCAT-Test-Fixture.md +++ b/docs/drivers/TwinCAT-Test-Fixture.md @@ -154,6 +154,36 @@ The required fixture state (1000-DINT GVL + churn POU) is documented in `TwinCatProject/README.md §Performance scenarios`; XAE-form sources land at `TwinCatProject/PLC/GVLs/GVL_Perf.TcGVL` + `TwinCatProject/PLC/POUs/FB_PerfChurn.TcPOU`. +### Handle caching (PR 2.2) + +Per-tag reads / writes route through an in-process ADS variable-handle cache. +The first read of a symbol resolves a handle via `CreateVariableHandleAsync`; +subsequent reads / writes of the same symbol issue against the cached handle. +On the wire this trades a multi-byte symbolic path (`GVL_Perf.aTags[742]` = +20+ bytes) for a 4-byte handle, and the device server skips name resolution +on every subsequent op. Cache lifetime is process-scoped; entries are evicted +on `AdsErrorCode.DeviceSymbolVersionInvalid` (with one retry against a fresh +handle), wiped on reconnect (handles are per-AMS-session), and deleted +best-effort on driver disposal. + +`TwinCATHandleCachePerfTests.Driver_handle_cache_avoids_repeat_symbol_resolution` +asserts the contract on real XAR by reading 50 symbols twice and verifying +the second pass issues zero new `CreateVariableHandleAsync` calls. It runs +under the standard `[TwinCATFact]` gate (XAR reachable; no `TWINCAT_PERF` +opt-in needed because 50 symbols is cheap). + +**Staleness caveat**: handles can go stale after a TwinCAT online change +(POU edit + activate). Until PR 2.3 ships the proactive Symbol-Version +invalidation listener, the safety net is twofold: (1) the +`DeviceSymbolVersionInvalid` evict-and-retry path catches cases where the +descriptor moves but the symbol survives, and (2) operators can call +`ITwinCATClient.FlushOptionalCachesAsync` manually after a known online +change to wipe the cache without forcing a full reconnect. The bulk +Sum-read / Sum-write path remains on symbolic paths in PR 2.2 (the bulk +path's per-call symbol resolution is already amortised across N tags; +the perf delta vs. handle-batched bulk is marginal — tracked as a +follow-up for the Phase-2 perf sweep). + ## Follow-up candidates 1. **XAR VM live-population** — scaffolding is in place (this PR); the diff --git a/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs b/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs index fcb8301..1c054cf 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -31,6 +31,26 @@ internal sealed class AdsTwinCATClient : ITwinCATClient // than tracking liveness, matching the AbCip / Modbus / FOCAS pattern from #181. private readonly ConcurrentDictionary _bitWriteLocks = new(); + // PR 2.2 — handle cache. Per-tag read/write resolves a symbolic path to an ADS + // variable handle once, then issues every subsequent op against the handle. Smaller + // AMS payloads (4-byte handle vs N-byte path) + skips name resolution in the runtime. + // Lifetime is process-scoped: cleared on reconnect (EnsureConnected path), wiped on + // a Symbol-Version-Invalid retry, and disposed on Dispose. PR 2.3 will wire a + // proactive Symbol Version invalidation listener so stale handles after an online + // change get evicted before the next read fails — until then, operators can call + // FlushOptionalCachesAsync to wipe manually. + private readonly ConcurrentDictionary _handleCache = new(); + private bool _wasConnected; + private readonly object _connectionStateGate = new(); + + // Test-only counter — number of CreateVariableHandleAsync calls actually issued + // (i.e. cache misses). Integration tests assert this stays at the unique-symbol + // count after a second pass over the same set. + internal int HandleCreateCount; + + /// Test-only — current size of the handle cache. + internal int HandleCacheCount => _handleCache.Count; + public AdsTwinCATClient() { _client.AdsNotificationEx += OnAdsNotificationEx; @@ -43,7 +63,24 @@ internal sealed class AdsTwinCATClient : ITwinCATClient if (_client.IsConnected) return Task.CompletedTask; _client.Timeout = (int)Math.Max(1_000, timeout.TotalMilliseconds); var netId = AmsNetId.Parse(address.NetId); + + // PR 2.2 — a fresh AMS session invalidates every cached handle (handle space is + // per-session in the ADS device server). Clear before reconnect so any read that + // raced with a transient drop never reuses a stale handle from the prior session. + // Note: the handles for the prior session are gone with that session — no need to + // call DeleteVariableHandleAsync, which would just fail with a transport error. + var wasConnected = false; + lock (_connectionStateGate) + { + wasConnected = _wasConnected; + _wasConnected = false; + } + if (wasConnected || !_handleCache.IsEmpty) + _handleCache.Clear(); + _client.Connect(netId, address.Port); + + lock (_connectionStateGate) _wasConnected = _client.IsConnected; return Task.CompletedTask; } @@ -59,13 +96,14 @@ internal sealed class AdsTwinCATClient : ITwinCATClient var clrType = MapToClrType(type); var readType = IsWholeArray(arrayDimensions) ? clrType.MakeArrayType() : clrType; - var result = await _client.ReadValueAsync(symbolPath, readType, cancellationToken) + // PR 2.2 — handle-based read. EnsureHandleAsync resolves through the cache; + // SymbolVersionInvalid evicts + retries once with a fresh handle. + var (rawValue, errorCode) = await ReadByHandleWithRetryAsync(symbolPath, readType, cancellationToken) .ConfigureAwait(false); + if (errorCode != AdsErrorCode.NoError) + return (null, TwinCATStatusMapper.MapAdsError((uint)errorCode)); - if (result.ErrorCode != AdsErrorCode.NoError) - return (null, TwinCATStatusMapper.MapAdsError((uint)result.ErrorCode)); - - var value = result.Value; + var value = rawValue; if (IsWholeArray(arrayDimensions)) { value = PostProcessArray(type, value); @@ -84,6 +122,92 @@ internal sealed class AdsTwinCATClient : ITwinCATClient } } + /// + /// Resolve to a cached ADS variable handle (or create one + /// on first use) and dispatch a . + /// On evicts the cached handle + /// + retries once with a freshly-created handle — covers the online-change race where + /// the symbol survives but its descriptor moves. + /// + private async Task<(object? value, AdsErrorCode errorCode)> ReadByHandleWithRetryAsync( + string symbolPath, Type readType, CancellationToken cancellationToken) + { + var handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false); + var result = await _client.ReadAnyAsync(handle, readType, cancellationToken).ConfigureAwait(false); + if (result.ErrorCode == AdsErrorCode.DeviceSymbolVersionInvalid) + { + EvictHandle(symbolPath); + handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false); + result = await _client.ReadAnyAsync(handle, readType, cancellationToken).ConfigureAwait(false); + } + return (result.Value, result.ErrorCode); + } + + /// + /// Mirror of for writes. Returns the final + /// ; the caller maps that to an OPC UA status. + /// + private async Task WriteByHandleWithRetryAsync( + string symbolPath, object value, CancellationToken cancellationToken) + { + var handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false); + var result = await _client.WriteAnyAsync(handle, value, cancellationToken).ConfigureAwait(false); + if (result.ErrorCode == AdsErrorCode.DeviceSymbolVersionInvalid) + { + EvictHandle(symbolPath); + handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false); + result = await _client.WriteAnyAsync(handle, value, cancellationToken).ConfigureAwait(false); + } + return result.ErrorCode; + } + + /// + /// Lookup-or-create the cached ADS handle for . The + /// guarantees publication safety, + /// but two concurrent callers on a cold key may both call + /// . + /// The loser's handle leaks for the lifetime of the process — acceptable cost + /// given how narrow the race window is, and matched by the libplctag / S7 driver + /// handle-cache patterns. + /// + internal async ValueTask EnsureHandleAsync(string symbolPath, CancellationToken cancellationToken) + { + if (_handleCache.TryGetValue(symbolPath, out var existing)) + return existing; + + Interlocked.Increment(ref HandleCreateCount); + var result = await _client.CreateVariableHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false); + if (result.ErrorCode != AdsErrorCode.NoError) + throw new AdsErrorException( + $"CreateVariableHandleAsync failed for '{symbolPath}'", result.ErrorCode); + + // GetOrAdd on a hit returns the winning handle; a loser-side DeleteVariableHandle here + // would race against an in-flight read using that handle elsewhere in this method, so + // we accept the small leak (one-time, per cold key) instead. + return _handleCache.GetOrAdd(symbolPath, result.Handle); + } + + /// + /// Evict a single cached handle. Best-effort delete on the wire — the runtime may + /// already have invalidated the handle (Symbol-Version-Invalid path), so we swallow + /// transport / ADS errors here. + /// + private void EvictHandle(string symbolPath) + { + if (!_handleCache.TryRemove(symbolPath, out var handle)) return; + try + { + // Fire-and-forget delete — the cache key is gone, the wire-side cleanup is + // strictly courtesy. If the device server is in a state where the handle is + // already dead, the delete will fail and we don't care. + _ = _client.DeleteVariableHandleAsync(handle, CancellationToken.None); + } + catch + { + // Best-effort. + } + } + private static bool IsWholeArray(int[]? arrayDimensions) => arrayDimensions is { Length: > 0 } && arrayDimensions.All(d => d > 0); @@ -125,11 +249,12 @@ internal sealed class AdsTwinCATClient : ITwinCATClient try { var converted = ConvertForWrite(type, value); - var result = await _client.WriteValueAsync(symbolPath, converted, cancellationToken) + // PR 2.2 — handle-based write with SymbolVersionInvalid evict-and-retry. + var errorCode = await WriteByHandleWithRetryAsync(symbolPath, converted, cancellationToken) .ConfigureAwait(false); - return result.ErrorCode == AdsErrorCode.NoError + return errorCode == AdsErrorCode.NoError ? TwinCATStatusMapper.Good - : TwinCATStatusMapper.MapAdsError((uint)result.ErrorCode); + : TwinCATStatusMapper.MapAdsError((uint)errorCode); } catch (AdsErrorException ex) { @@ -159,19 +284,21 @@ internal sealed class AdsTwinCATClient : ITwinCATClient await rmwLock.WaitAsync(cancellationToken).ConfigureAwait(false); try { - var read = await _client.ReadValueAsync(parentPath, typeof(uint), cancellationToken) + // PR 2.2 — RMW round-trip flows through the same handle cache so that the + // parent word's resolved handle is reused on subsequent bit writes too. + var (rawCurrent, readErr) = await ReadByHandleWithRetryAsync(parentPath, typeof(uint), cancellationToken) .ConfigureAwait(false); - if (read.ErrorCode != AdsErrorCode.NoError) - return TwinCATStatusMapper.MapAdsError((uint)read.ErrorCode); + if (readErr != AdsErrorCode.NoError) + return TwinCATStatusMapper.MapAdsError((uint)readErr); - var current = Convert.ToUInt32(read.Value ?? 0u); + var current = Convert.ToUInt32(rawCurrent ?? 0u); var updated = ApplyBit(current, bit, setBit); - var write = await _client.WriteValueAsync(parentPath, updated, cancellationToken) + var writeErr = await WriteByHandleWithRetryAsync(parentPath, updated, cancellationToken) .ConfigureAwait(false); - return write.ErrorCode == AdsErrorCode.NoError + return writeErr == AdsErrorCode.NoError ? TwinCATStatusMapper.Good - : TwinCATStatusMapper.MapAdsError((uint)write.ErrorCode); + : TwinCATStatusMapper.MapAdsError((uint)writeErr); } catch (AdsErrorException ex) { @@ -354,6 +481,12 @@ internal sealed class AdsTwinCATClient : ITwinCATClient { if (reads.Count == 0) return Array.Empty<(object?, uint)>(); + // PR 2.2 deviation: bulk path stays on symbolic Sum-command (SumInstancePathAnyTypeRead / + // SumWriteBySymbolPath). Beckhoff also exposes SumHandleRead / SumWriteByHandle, but + // wiring the cached handles into them changes the request layout substantially + + // would either need to reuse handles created on the per-tag path (tying lifetimes) + // or maintain a parallel handle batch — neither pulls weight against PR 2.1's already + // 10-20× win. Tracked as a follow-up for the Phase-2 perf sweep. // Build the (path, AnyTypeSpecifier) request envelope. SumInstancePathAnyTypeRead // batches all paths into a single ADS Sum-read round-trip (IndexGroup 0xF080 = read // multiple items by symbol name with ANY-type marshalling). @@ -458,9 +591,53 @@ internal sealed class AdsTwinCATClient : ITwinCATClient { _client.AdsNotificationEx -= OnAdsNotificationEx; _notifications.Clear(); + + // PR 2.2 — release every cached handle on the wire as a good citizen. Best-effort + // and bounded to a short window so a hung router doesn't block process shutdown: + // each delete is fire-and-forget, errors swallowed. The session itself is about to + // tear down anyway, so the device server will reclaim everything regardless. + foreach (var kv in _handleCache) + { + try + { + _ = _client.DeleteVariableHandleAsync(kv.Value, CancellationToken.None); + } + catch + { + // Per-entry failures are expected on a closing connection. + } + } + _handleCache.Clear(); + _client.Dispose(); } + /// + /// PR 2.2 — flush all process-scoped optional caches (handle cache today). A + /// proactive Symbol Version invalidation listener arrives in PR 2.3 — until then, + /// operators / 2.3-aware callers can wipe the cache manually after a known online + /// change. + /// + public Task FlushOptionalCachesAsync() + { + // Best-effort delete on the wire — a held handle won't survive a redeploy anyway, + // but cleaning up matches the Dispose convention. + var snapshot = _handleCache.ToArray(); + _handleCache.Clear(); + foreach (var kv in snapshot) + { + try + { + _ = _client.DeleteVariableHandleAsync(kv.Value, CancellationToken.None); + } + catch + { + // Best-effort. + } + } + return Task.CompletedTask; + } + private sealed class NotificationRegistration( string symbolPath, TwinCATDataType type, diff --git a/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs b/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs index 5f2b5c4..498f4d1 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs +++ b/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs @@ -114,6 +114,16 @@ public interface ITwinCATClient : IDisposable /// decide whether to drill in via their own walker. /// IAsyncEnumerable BrowseSymbolsAsync(CancellationToken cancellationToken); + + /// + /// PR 2.2 — wipe process-scoped optional caches (today: the ADS variable-handle + /// cache backing per-tag reads / writes). Surfaces so operators + the future PR + /// 2.3 Symbol-Version invalidation listener can flush stale handles after a known + /// online change without forcing a full reconnect. Safe to call mid-traffic — in-flight + /// reads continue to use the handles they already hold; the next read for a symbol + /// will re-resolve. Best-effort wire-side delete; failures are swallowed. + /// + Task FlushOptionalCachesAsync(); } /// Opaque handle for a registered ADS notification. tears it down. diff --git a/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.csproj b/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.csproj index e52b5ba..1afc947 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.csproj +++ b/src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.csproj @@ -26,6 +26,9 @@ + + diff --git a/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests/TwinCATHandleCachePerfTests.cs b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests/TwinCATHandleCachePerfTests.cs new file mode 100644 index 0000000..44e531d --- /dev/null +++ b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests/TwinCATHandleCachePerfTests.cs @@ -0,0 +1,107 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests; + +/// +/// PR 2.2 integration test — exercises the live +/// handle cache against a real XAR runtime. Reads 50 distinct symbols twice and +/// asserts the second pass issues zero new CreateVariableHandleAsync calls. +/// +/// +/// Hooks into AdsTwinCATClient.HandleCreateCount + HandleCacheCount via +/// the InternalsVisibleTo bridge added in PR 2.1. The fixture's skip-reason is +/// surfaced through so the test stays green on a +/// dev box without the XAR VM (and its expiring trial license). +/// +[Collection("TwinCATXar")] +[Trait("Category", "Integration")] +[Trait("Simulator", "TwinCAT-XAR")] +public sealed class TwinCATHandleCachePerfTests(TwinCATXarFixture sim) +{ + private const int TagCount = 50; + + [TwinCATFact] + public async Task Driver_handle_cache_avoids_repeat_symbol_resolution() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + + var deviceAddress = $"ads://{sim.TargetNetId}:{sim.AmsPort}"; + var tags = new TwinCATTagDefinition[TagCount]; + var refs = new string[TagCount]; + for (var i = 0; i < TagCount; i++) + { + // GVL_Perf.aTags is 1-based per IEC 61131-3 ARRAY declaration. The 1000-element + // perf array is shared with TwinCATSumCommandPerfTests; this test only touches + // the first 50 indices so it stays cheap on every CI run. + var name = $"Perf{i + 1}"; + refs[i] = name; + tags[i] = new TwinCATTagDefinition( + Name: name, + DeviceHostAddress: deviceAddress, + SymbolPath: $"GVL_Perf.aTags[{i + 1}]", + DataType: TwinCATDataType.DInt, + // ArrayDimensions = [1] forces the per-tag (handle-cached) path rather + // than the bulk Sum-read path, which still flows through symbolic paths + // by the PR 2.2 deviation note. + ArrayDimensions: [1]); + } + + var options = new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(deviceAddress, "XAR-VM")], + Tags = tags, + UseNativeNotifications = false, + Timeout = TimeSpan.FromSeconds(15), + Probe = new TwinCATProbeOptions { Enabled = false }, + }; + + // Factory wrapper to capture the live AdsTwinCATClient and expose its internal + // counters back up to the test. Driver-side code only sees ITwinCATClient so this + // doesn't leak the implementation type out of the test. + var capture = new CapturingFactory(); + await using var drv = new TwinCATDriver(options, "tc3-handle-cache", capture); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + // First pass: every symbol is a cache miss. After this pass HandleCreateCount + // should equal TagCount. + var firstResults = await drv.ReadAsync(refs, TestContext.Current.CancellationToken); + firstResults.Count.ShouldBe(TagCount); + firstResults.ShouldAllBe(s => s.StatusCode == TwinCATStatusMapper.Good); + + capture.Client.ShouldNotBeNull("CapturingFactory should have produced exactly one AdsTwinCATClient"); + var firstPassCreates = capture.Client!.HandleCreateCount; + capture.Client!.HandleCacheCount.ShouldBe(TagCount); + firstPassCreates.ShouldBe(TagCount); + + // Second pass: every symbol is a cache hit. HandleCreateCount must not have moved. + var secondResults = await drv.ReadAsync(refs, TestContext.Current.CancellationToken); + secondResults.Count.ShouldBe(TagCount); + secondResults.ShouldAllBe(s => s.StatusCode == TwinCATStatusMapper.Good); + + capture.Client!.HandleCreateCount.ShouldBe( + firstPassCreates, + $"Second pass over {TagCount} symbols should have created zero new handles, " + + $"but HandleCreateCount went {firstPassCreates} -> {capture.Client!.HandleCreateCount}."); + capture.Client!.HandleCacheCount.ShouldBe(TagCount); + } + + /// + /// Routes through the production + /// and snapshots the produced client so the + /// test can read its internal handle-cache counters. + /// + private sealed class CapturingFactory : ITwinCATClientFactory + { + public AdsTwinCATClient? Client { get; private set; } + + public ITwinCATClient Create() + { + var c = new AdsTwinCATClient(); + Client ??= c; // first one wins — single-device test path. + return c; + } + } +} diff --git a/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs index ac90175..c05602d 100644 --- a/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs +++ b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs @@ -20,24 +20,108 @@ internal class FakeTwinCATClient : ITwinCATClient public List<(string symbol, TwinCATDataType type, int? bit, int[]? arrayDimensions)> ReadLog { get; } = new(); public bool ProbeResult { get; set; } = true; + // ---- PR 2.2: handle-cache tracking ---- + // + // The fake mirrors the production AdsTwinCATClient handle-cache state machine so the + // unit + integration tests can assert "second read of X reused the cached handle" + // without hitting a real ADS device. EnsureFakeHandle is called by every per-tag read / + // write path and increments HandleCreateInvocations only on a cache miss. Tests can + // arm SymbolVersionInvalidOnNextRead / Write to drive the evict-and-retry path. + public List HandleCreateInvocations { get; } = new(); + public List HandleDeleteInvocations { get; } = new(); + public List ReadByHandleInvocations { get; } = new(); + public List WriteByHandleInvocations { get; } = new(); + public int FlushOptionalCachesCount { get; private set; } + /// Inject DeviceSymbolVersionInvalid into the next read of this symbol. + public HashSet SymbolVersionInvalidOnNextRead { get; } = new(StringComparer.OrdinalIgnoreCase); + /// Inject DeviceSymbolVersionInvalid into the next write of this symbol. + public HashSet SymbolVersionInvalidOnNextWrite { get; } = new(StringComparer.OrdinalIgnoreCase); + + private readonly Dictionary _handleCache = new(StringComparer.OrdinalIgnoreCase); + private uint _nextHandle = 1; + + private uint EnsureFakeHandle(string symbolPath) + { + if (_handleCache.TryGetValue(symbolPath, out var existing)) return existing; + HandleCreateInvocations.Add(symbolPath); + var handle = _nextHandle++; + _handleCache[symbolPath] = handle; + return handle; + } + + private void EvictFakeHandle(string symbolPath) + { + if (_handleCache.Remove(symbolPath, out var handle)) + HandleDeleteInvocations.Add(handle); + } + + /// Test helper — current cached-handle count. + public int HandleCacheCount => _handleCache.Count; + + /// Test helper — true when the symbol currently has a cached handle. + public bool HasCachedHandle(string symbolPath) => _handleCache.ContainsKey(symbolPath); + public virtual Task ConnectAsync(TwinCATAmsAddress address, TimeSpan timeout, CancellationToken ct) { ConnectCount++; if (ThrowOnConnect) throw Exception ?? new InvalidOperationException(); + // PR 2.2 — production wipes the handle cache on every (re)connect because handle + // identity is per-AMS-session. Mirror so tests of the reconnect flow see the same + // cache-clear semantics. Existing handles are dead with the prior session, no + // wire-side delete needed. + if (IsConnected) _handleCache.Clear(); IsConnected = true; return Task.CompletedTask; } + /// Test helper — simulate a reconnect (ConnectAsync after the connection drops). + public void SimulateReconnect() + { + IsConnected = false; + _handleCache.Clear(); + IsConnected = true; + } + public virtual Task<(object? value, uint status)> ReadValueAsync( string symbolPath, TwinCATDataType type, int? bitIndex, int[]? arrayDimensions, CancellationToken ct) { if (ThrowOnRead) throw Exception ?? new InvalidOperationException(); ReadLog.Add((symbolPath, type, bitIndex, arrayDimensions)); + + // PR 2.2 — mirror the production handle-cache state machine: resolve handle (cache + // miss → HandleCreateInvocations++), do read-by-handle, on injected + // SymbolVersionInvalid evict + retry once, then deliver the live value. + ReadOneByHandle(symbolPath); + var status = ReadStatuses.TryGetValue(symbolPath, out var s) ? s : TwinCATStatusMapper.Good; var value = Values.TryGetValue(symbolPath, out var v) ? v : null; return Task.FromResult((value, status)); } + private void ReadOneByHandle(string symbolPath) + { + EnsureFakeHandle(symbolPath); + ReadByHandleInvocations.Add(symbolPath); + if (SymbolVersionInvalidOnNextRead.Remove(symbolPath)) + { + EvictFakeHandle(symbolPath); + EnsureFakeHandle(symbolPath); // retry — fresh handle + ReadByHandleInvocations.Add(symbolPath); + } + } + + private void WriteOneByHandle(string symbolPath) + { + EnsureFakeHandle(symbolPath); + WriteByHandleInvocations.Add(symbolPath); + if (SymbolVersionInvalidOnNextWrite.Remove(symbolPath)) + { + EvictFakeHandle(symbolPath); + EnsureFakeHandle(symbolPath); // retry — fresh handle + WriteByHandleInvocations.Add(symbolPath); + } + } + public virtual Task WriteValueAsync( string symbolPath, TwinCATDataType type, int? bitIndex, int[]? arrayDimensions, object? value, CancellationToken ct) { @@ -51,6 +135,10 @@ internal class FakeTwinCATClient : ITwinCATClient var parentPath = AdsTwinCATClient.TryGetParentSymbolPath(symbolPath); if (parentPath is not null) { + // RMW touches the parent word twice (read + write); each goes through the + // handle cache, exactly mirroring the production path. + ReadOneByHandle(parentPath); + WriteOneByHandle(parentPath); var current = Values.TryGetValue(parentPath, out var p) && p is not null ? Convert.ToUInt32(p) : 0u; Values[parentPath] = AdsTwinCATClient.ApplyBit( @@ -59,6 +147,7 @@ internal class FakeTwinCATClient : ITwinCATClient } else { + WriteOneByHandle(symbolPath); Values[symbolPath] = value; } @@ -66,6 +155,15 @@ internal class FakeTwinCATClient : ITwinCATClient return Task.FromResult(status); } + public virtual Task FlushOptionalCachesAsync() + { + FlushOptionalCachesCount++; + // Mirror production: emit a delete record per cached handle, then clear. + foreach (var kv in _handleCache) HandleDeleteInvocations.Add(kv.Value); + _handleCache.Clear(); + return Task.CompletedTask; + } + public virtual Task ProbeAsync(CancellationToken ct) { if (ThrowOnProbe) return Task.FromResult(false); @@ -134,6 +232,10 @@ internal class FakeTwinCATClient : ITwinCATClient { DisposeCount++; IsConnected = false; + // PR 2.2 — production deletes cached handles on Dispose; mirror so tests can assert + // the fan-out delete count matches. + foreach (var kv in _handleCache) HandleDeleteInvocations.Add(kv.Value); + _handleCache.Clear(); } // ---- notification fake ---- diff --git a/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHandleCacheTests.cs b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHandleCacheTests.cs new file mode 100644 index 0000000..21db58d --- /dev/null +++ b/tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHandleCacheTests.cs @@ -0,0 +1,231 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests; + +/// +/// PR 2.2 — handle-based access with caching. Verifies the cache state machine +/// mirrored on : cold-key resolves through +/// -> CreateVariableHandle, warm-key reuses the cached +/// handle, evicts + +/// retries, and Dispose / reconnect / FlushOptionalCachesAsync wipe the cache. +/// +/// +/// The fake's state machine is a high-fidelity mirror of AdsTwinCATClient: +/// the production class is exercised through the same code paths in the integration +/// tier (TwinCATHandleCachePerfTests) and through the fake at unit tier here — +/// consistent with how the rest of this driver tests its AdsClient wrapper. +/// +[Trait("Category", "Unit")] +public sealed class TwinCATHandleCacheTests +{ + private const string DevA = "ads://5.23.91.23.1.1:851"; + + private static (TwinCATDriver drv, FakeTwinCATClientFactory factory) NewDriver(params TwinCATTagDefinition[] tags) + { + var factory = new FakeTwinCATClientFactory(); + var hosts = tags.Select(t => t.DeviceHostAddress).Distinct().ToArray(); + if (hosts.Length == 0) hosts = [DevA]; + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [.. hosts.Select(h => new TwinCATDeviceOptions(h))], + Tags = tags, + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-handle", factory); + return (drv, factory); + } + + [Fact] + public async Task Same_symbol_read_twice_creates_single_handle() + { + // Force the per-tag path (whole-array reads + bit-extracted BOOL skip the bulk + // surface in PR 2.1; both still flow through the handle cache). Whole-array is + // the simplest single-symbol read for this assertion — see TwinCATSumCommandTests + // for the bulk-path's own handle-free path. + var fake = new FakeTwinCATClient { Values = { ["MAIN.Speed"] = 42 } }; + await fake.ReadValueAsync("MAIN.Speed", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + await fake.ReadValueAsync("MAIN.Speed", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + + fake.HandleCreateInvocations.ShouldBe(["MAIN.Speed"]); + fake.ReadByHandleInvocations.Count.ShouldBe(2); + fake.HandleCacheCount.ShouldBe(1); + } + + [Fact] + public async Task Two_distinct_symbols_create_two_handles() + { + var fake = new FakeTwinCATClient + { + Values = { ["MAIN.A"] = 1, ["MAIN.B"] = 2 }, + }; + await fake.ReadValueAsync("MAIN.A", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + await fake.ReadValueAsync("MAIN.B", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + await fake.ReadValueAsync("MAIN.A", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + + fake.HandleCreateInvocations.ShouldBe(["MAIN.A", "MAIN.B"]); + fake.HandleCacheCount.ShouldBe(2); + } + + [Fact] + public async Task SymbolVersionInvalid_evicts_and_retries_with_fresh_handle() + { + var fake = new FakeTwinCATClient { Values = { ["MAIN.Counter"] = 10 } }; + + // First read populates the cache. + await fake.ReadValueAsync("MAIN.Counter", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + fake.HandleCreateInvocations.Count.ShouldBe(1); + + // Arm SymbolVersionInvalid for the next read; the fake's state machine evicts + // the cached handle + retries once, exactly as AdsTwinCATClient does on the real wire. + fake.SymbolVersionInvalidOnNextRead.Add("MAIN.Counter"); + await fake.ReadValueAsync("MAIN.Counter", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + + // Two creates total — original + retry — and one delete on eviction. + fake.HandleCreateInvocations.ShouldBe(["MAIN.Counter", "MAIN.Counter"]); + fake.HandleDeleteInvocations.Count.ShouldBe(1); + // After the retry the cache is repopulated. + fake.HandleCacheCount.ShouldBe(1); + } + + [Fact] + public async Task SymbolVersionInvalid_on_write_evicts_and_retries() + { + var fake = new FakeTwinCATClient(); + await fake.WriteValueAsync("MAIN.Setpoint", TwinCATDataType.DInt, null, null, 100, TestContext.Current.CancellationToken); + fake.HandleCreateInvocations.Count.ShouldBe(1); + + fake.SymbolVersionInvalidOnNextWrite.Add("MAIN.Setpoint"); + await fake.WriteValueAsync("MAIN.Setpoint", TwinCATDataType.DInt, null, null, 200, TestContext.Current.CancellationToken); + + fake.HandleCreateInvocations.ShouldBe(["MAIN.Setpoint", "MAIN.Setpoint"]); + fake.HandleDeleteInvocations.Count.ShouldBe(1); + fake.HandleCacheCount.ShouldBe(1); + } + + [Fact] + public async Task Dispose_deletes_all_cached_handles() + { + var fake = new FakeTwinCATClient + { + Values = { ["A"] = 1, ["B"] = 2, ["C"] = 3 }, + }; + await fake.ReadValueAsync("A", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + await fake.ReadValueAsync("B", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + await fake.ReadValueAsync("C", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + fake.HandleCacheCount.ShouldBe(3); + + fake.Dispose(); + + fake.HandleCacheCount.ShouldBe(0); + fake.HandleDeleteInvocations.Count.ShouldBe(3); + } + + [Fact] + public async Task FlushOptionalCachesAsync_clears_cache_then_recreates_handle() + { + var fake = new FakeTwinCATClient { Values = { ["X"] = 1 } }; + await fake.ReadValueAsync("X", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + fake.HandleCreateInvocations.Count.ShouldBe(1); + fake.HasCachedHandle("X").ShouldBeTrue(); + + await fake.FlushOptionalCachesAsync(); + + fake.FlushOptionalCachesCount.ShouldBe(1); + fake.HasCachedHandle("X").ShouldBeFalse(); + fake.HandleDeleteInvocations.Count.ShouldBe(1); + + // Subsequent read recreates. + await fake.ReadValueAsync("X", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + fake.HandleCreateInvocations.Count.ShouldBe(2); + } + + [Fact] + public async Task Reconnect_clears_handle_cache() + { + var fake = new FakeTwinCATClient { Values = { ["MAIN.Y"] = 9 } }; + await fake.ConnectAsync(new TwinCATAmsAddress("5.23.91.23.1.1", 851), TimeSpan.FromSeconds(5), + TestContext.Current.CancellationToken); + await fake.ReadValueAsync("MAIN.Y", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + fake.HandleCacheCount.ShouldBe(1); + + // Simulate a connection drop + reconnect — handles from the prior session are dead. + fake.SimulateReconnect(); + + fake.HandleCacheCount.ShouldBe(0); + // Next read repopulates with a fresh handle. + await fake.ReadValueAsync("MAIN.Y", TwinCATDataType.DInt, null, null, TestContext.Current.CancellationToken); + fake.HandleCreateInvocations.Count.ShouldBe(2); + } + + [Fact] + public async Task Bulk_path_does_not_consume_handle_cache() + { + // PR 2.2 deviation note: bulk Sum-read / Sum-write stays on symbolic paths because + // the perf win over the per-tag handle path is marginal vs the diff cost. This test + // pins the contract — bulk does not create handles, the per-tag path does. + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("X", DevA, "MAIN.X", TwinCATDataType.DInt), + new TwinCATTagDefinition("Y", DevA, "MAIN.Y", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + factory.Customise = () => new FakeTwinCATClient + { + Values = { ["MAIN.X"] = 1, ["MAIN.Y"] = 2 }, + }; + + await drv.ReadAsync(["X", "Y"], TestContext.Current.CancellationToken); + await drv.ReadAsync(["X", "Y"], TestContext.Current.CancellationToken); + + var client = factory.Clients[0]; + client.BulkReadInvocations.Count.ShouldBe(2); + client.HandleCreateInvocations.Count.ShouldBe(0); + client.HandleCacheCount.ShouldBe(0); + } + + [Fact] + public async Task Per_tag_path_through_driver_uses_handle_cache() + { + // Whole-array reads route through the per-tag ReadValueAsync path — perfect for + // exercising the handle cache via the public driver surface. + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("Recipe", DevA, "MAIN.Recipe", TwinCATDataType.DInt, + ArrayDimensions: [4])); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + factory.Customise = () => new FakeTwinCATClient + { + Values = { ["MAIN.Recipe"] = new int[] { 1, 2, 3, 4 } }, + }; + + await drv.ReadAsync(["Recipe"], TestContext.Current.CancellationToken); + await drv.ReadAsync(["Recipe"], TestContext.Current.CancellationToken); + await drv.ReadAsync(["Recipe"], TestContext.Current.CancellationToken); + + var client = factory.Clients[0]; + client.HandleCreateInvocations.ShouldBe(["MAIN.Recipe"]); + client.ReadByHandleInvocations.Count.ShouldBe(3); + } + + [Fact] + public async Task Bit_RMW_routes_parent_word_through_handle_cache() + { + // BOOL-in-word writes do read + write of the parent UDINT — both hops should + // share the same cached handle for the parent. After three writes there should + // still be only one handle for "Flags". + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("Bit3", DevA, "GVL.Flags.3", TwinCATDataType.Bool)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + factory.Customise = () => new FakeTwinCATClient { Values = { ["GVL.Flags"] = 0u } }; + + await drv.WriteAsync([new WriteRequest("Bit3", true)], TestContext.Current.CancellationToken); + await drv.WriteAsync([new WriteRequest("Bit3", false)], TestContext.Current.CancellationToken); + await drv.WriteAsync([new WriteRequest("Bit3", true)], TestContext.Current.CancellationToken); + + var client = factory.Clients[0]; + client.HandleCreateInvocations.ShouldBe(["GVL.Flags"]); + client.HandleCacheCount.ShouldBe(1); + // Three RMWs = three reads + three writes by handle on the parent. + client.ReadByHandleInvocations.Count(x => x == "GVL.Flags").ShouldBe(3); + client.WriteByHandleInvocations.Count(x => x == "GVL.Flags").ShouldBe(3); + } +}