From fdd6b332fe3c1ce12247df103fa094e739246436 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 17 Jun 2026 20:22:45 -0400 Subject: [PATCH] fix(twincat): update BrowseSymbolsAsync doc + cache adapter fields + Flat-mode note (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ITwinCATClient.BrowseSymbolsAsync XML doc updated: states the implementation now expands struct/UDT/FB symbols into atomic member leaves via TwinCATSymbolExpander; callers receive only atomic/array leaves with full InstancePaths, never struct containers. - AdsSymbolNode: cache IsStruct, Mapped, Children, ReadOnly as readonly fields computed once in the ctor so repeated property access during recursive expansion doesn't re-materialize or re-invoke MapSymbolType/IsSymbolWritable. - BrowseSymbolsAsync: add operator-gated live risk note next to SymbolsLoadMode.Flat warning that a real TC3 target may not populate SubSymbols in Flat mode, with guidance to switch to VirtualTree if members don't surface — do not change mode now. - TwinCATSymbolExpanderTests: simplify confusing `new string('.', 0)` no-op to `""`. --- .../AdsTwinCATClient.cs | 42 +++++++++++++++---- .../ITwinCATClient.cs | 13 +++--- .../TwinCATSymbolExpanderTests.cs | 2 +- 3 files changed, 42 insertions(+), 15 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs index 3a3c600c..16ca0f5f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -322,6 +322,14 @@ internal sealed class AdsTwinCATClient : ITwinCATClient // SymbolLoaderFactory downloads the symbol-info blob once then iterates locally — the // async surface on this interface is for our callers, not for the underlying call which // is effectively sync on top of the already-open AdsClient. + // + // LIVE RISK (operator-gated, not yet verified against a real TC3 target): Flat mode + // may not populate ISymbol.SubSymbols on struct/UDT/FB symbols on some TC3 firmware + // versions — in that case TwinCATSymbolExpander sees no children and drops those + // symbols silently. If struct members don't surface on a real target, switch to + // SymbolsLoadMode.VirtualTree which is documented to always populate SubSymbols via + // the hierarchy. Do NOT change the mode here until confirmed against live ADS — Flat is + // required for Phase 4c array discovery to work correctly. var settings = new SymbolLoaderSettings(SymbolsLoadMode.Flat); var loader = SymbolLoaderFactory.Create(_client, settings); await Task.Yield(); // honors the async surface; pragmatic given the loader itself is sync @@ -350,20 +358,36 @@ internal sealed class AdsTwinCATClient : ITwinCATClient /// a local operation). Reuses the existing + /// so atomic-type mapping and access-rights handling stay identical to the pre-expansion path. /// - private sealed class AdsSymbolNode(ISymbol symbol) : ITwinCATSymbolNode + private sealed class AdsSymbolNode : ITwinCATSymbolNode { - public string InstancePath => symbol.InstancePath; + // Computed once in the ctor so repeated property access during recursive expansion + // doesn't re-materialize Children or re-invoke MapSymbolType / IsSymbolWritable. + private readonly ISymbol _symbol; + private readonly bool _isStruct; + private readonly (TwinCATDataType? Type, int? ArrayLength) _mapped; + private readonly IReadOnlyList _children; + private readonly bool _readOnly; - public bool IsStruct => symbol.DataType?.Category == DataTypeCategory.Struct; - - public (TwinCATDataType? Type, int? ArrayLength) Mapped => MapSymbolType(symbol.DataType); - - public IReadOnlyList Children => - symbol.SubSymbols is { } subs + public AdsSymbolNode(ISymbol symbol) + { + _symbol = symbol; + _isStruct = symbol.DataType?.Category == DataTypeCategory.Struct; + _mapped = MapSymbolType(symbol.DataType); + _children = symbol.SubSymbols is { } subs ? subs.Select(s => (ITwinCATSymbolNode)new AdsSymbolNode(s)).ToList() : []; + _readOnly = !IsSymbolWritable(symbol); + } - public bool ReadOnly => !IsSymbolWritable(symbol); + public string InstancePath => _symbol.InstancePath; + + public bool IsStruct => _isStruct; + + public (TwinCATDataType? Type, int? ArrayLength) Mapped => _mapped; + + public IReadOnlyList Children => _children; + + public bool ReadOnly => _readOnly; } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs index f76bbb9e..03c59232 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs @@ -99,11 +99,14 @@ public interface ITwinCATClient : IDisposable CancellationToken cancellationToken); /// - /// Walk the target's symbol table via the TwinCAT SymbolLoaderFactory (flat mode). - /// Yields each top-level symbol the PLC exposes — global variables, program-scope locals, - /// function-block instance fields. Filters for our atomic type surface; structured / - /// UDT / function-block typed symbols surface with DataType = null so callers can - /// decide whether to drill in via their own walker. + /// Walk the target's symbol table via the TwinCAT SymbolLoaderFactory (flat mode) + /// and expand all struct / UDT / function-block symbols to their atomic member leaves. + /// The implementation uses to recursively + /// descend each struct/UDT/FB-instance symbol into its addressable atomic members, + /// yielding only atomic leaves (scalars or 1-D arrays with a non-null + /// ) and dropping struct/UDT/FB containers. + /// Callers receive fully-qualified InstancePaths (e.g. MAIN.Motor1.Speed) + /// and never see struct-typed entries — expansion is performed here, not in the caller. /// /// Cancellation token for the enumeration operation. IAsyncEnumerable BrowseSymbolsAsync(CancellationToken cancellationToken); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSymbolExpanderTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSymbolExpanderTests.cs index 47262bd6..9f037119 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSymbolExpanderTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATSymbolExpanderTests.cs @@ -104,7 +104,7 @@ public sealed class TwinCATSymbolExpanderTests // atomic leaf at the very bottom. The leaf sits at depth == number-of-struct-levels. // Place the leaf at depth MaxDepth (== one past the deepest descended level) so it is dropped, // and a sibling leaf at a shallow depth so we prove the walk itself still works. - ITwinCATSymbolNode deep = Atomic("Root" + new string('.', 0) + "Leaf.Deep", TwinCATDataType.DInt); + ITwinCATSymbolNode deep = Atomic("RootLeaf.Deep", TwinCATDataType.DInt); // Wrap the atomic in MaxDepth struct levels so the atomic ends up at depth == MaxDepth. for (var i = 0; i < TwinCATSymbolExpander.MaxDepth; i++) deep = Struct($"Level{i}", deep);