From 41a926a9290e2a608cf5c77b27b1790841938894 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Fri, 11 Sep 2026 11:31:33 +0200 Subject: [PATCH 1/9] Fix thread-unsafe LinkedList access in OpenSSLConnector pending buffer lists (GML-1187) --- .../OpenSSL/OpenSSLConnector.cs | 60 +++++++++++++++---- 1 file changed, 49 insertions(+), 11 deletions(-) diff --git a/src/S7CommPlusDriver/OpenSSL/OpenSSLConnector.cs b/src/S7CommPlusDriver/OpenSSL/OpenSSLConnector.cs index da21d9a..5f513a4 100644 --- a/src/S7CommPlusDriver/OpenSSL/OpenSSLConnector.cs +++ b/src/S7CommPlusDriver/OpenSSL/OpenSSLConnector.cs @@ -33,6 +33,8 @@ public class OpenSSLConnector : IDisposable private readonly byte[] m_buffer = new byte[4096]; private readonly DataBufferList m_pendingWriteList; private readonly DataBufferList m_pendingReadList; + private readonly object m_readListLock = new object(); + private readonly object m_writeListLock = new object(); private bool m_fatalError; private bool m_disposed; @@ -277,8 +279,16 @@ protected void RunSSL() while (!m_fatalError && ((!m_readRequired && dataToWrite) || dataToRead)) { - int pendingReadBefore = m_pendingReadList.Count; - int pendingWriteBefore = m_pendingWriteList.Count; + int pendingReadBefore; + int pendingWriteBefore; + lock (m_readListLock) + { + pendingReadBefore = m_pendingReadList.Count; + } + lock (m_writeListLock) + { + pendingWriteBefore = m_pendingWriteList.Count; + } bool readRequiredBefore = m_readRequired; var bioPendingBefore = Native.BIO_ctrl_pending(m_pBioOut); @@ -304,8 +314,18 @@ protected void RunSSL() GetPendingOperations(ref dataToRead, ref dataToWrite); var bioPendingAfter = Native.BIO_ctrl_pending(m_pBioOut); - if (pendingReadBefore == m_pendingReadList.Count && - pendingWriteBefore == m_pendingWriteList.Count && + int pendingReadAfter; + int pendingWriteAfter; + lock (m_readListLock) + { + pendingReadAfter = m_pendingReadList.Count; + } + lock (m_writeListLock) + { + pendingWriteAfter = m_pendingWriteList.Count; + } + if (pendingReadBefore == pendingReadAfter && + pendingWriteBefore == pendingWriteAfter && readRequiredBefore == m_readRequired && bioPendingBefore == bioPendingAfter) { @@ -341,8 +361,14 @@ public void ReadCompleted(byte[] pData, int dataLen) private void GetPendingOperations(ref bool dataToRead, ref bool dataToWrite) { - dataToRead = m_pendingReadList.Count > 0; - dataToWrite = m_pendingWriteList.Count > 0; + lock (m_readListLock) + { + dataToRead = m_pendingReadList.Count > 0; + } + lock (m_writeListLock) + { + dataToWrite = m_pendingWriteList.Count > 0; + } } private void PerformRead() @@ -437,23 +463,35 @@ private void UseData(DataBufferList list, DataBuffer pBuffer, int result) private void AppendBuffer(DataBufferList list, DataBuffer pBuffer) { - list.AddLast(pBuffer); + object lockObject = (list == m_pendingReadList) ? m_readListLock : m_writeListLock; + lock (lockObject) + { + list.AddLast(pBuffer); + } } DataBuffer GetNextBuffer(DataBufferList list) { DataBuffer head = null; - if (list.Count > 0) + object lockObject = (list == m_pendingReadList) ? m_readListLock : m_writeListLock; + lock (lockObject) { - head = list.First.Value; - list.RemoveFirst(); + if (list.Count > 0) + { + head = list.First.Value; + list.RemoveFirst(); + } } return head; } void PutbackBuffer(DataBufferList list, DataBuffer pBuffer) { - list.AddFirst(pBuffer); + object lockObject = (list == m_pendingReadList) ? m_readListLock : m_writeListLock; + lock (lockObject) + { + list.AddFirst(pBuffer); + } } /// From f65c8d93f9a5e70033d8df05feb493a1af9c5865 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Thu, 10 Sep 2026 12:53:53 +0200 Subject: [PATCH 2/9] Add PlcTag.GetValue/SetValue and POffsetInfoType.MaxStringLength Port net functional changes from fork package 01 (DataTypes extension / string array fix, commits 6aef271/519cdeb/ffe8daa): - PlcTag: add virtual GetValue()/SetValue(object) reflecting the concrete tag's public Value property, for generic tag value access. - POffsetInfoType: add virtual MaxStringLength(), returning 0 by default, UnspecifiedOffsetinfo1 for Array1Dim (string array max length), and Min(UnspecifiedOffsetinfo1, UnspecifiedOffsetinfo2) for String (fixes cases where firmware swaps both fields). All other changes from this fork package are already superseded by the target repo's own PlcTag/PlcTags implementation and are documented as excluded in package 16. --- src/S7CommPlusDriver/ClientApi/PlcTag.cs | 20 ++++++++++++++++++++ src/S7CommPlusDriver/Core/POffsetInfoType.cs | 10 ++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/S7CommPlusDriver/ClientApi/PlcTag.cs b/src/S7CommPlusDriver/ClientApi/PlcTag.cs index 7cdef54..c40b5c0 100644 --- a/src/S7CommPlusDriver/ClientApi/PlcTag.cs +++ b/src/S7CommPlusDriver/ClientApi/PlcTag.cs @@ -64,6 +64,26 @@ public virtual void ProcessWriteResult(UInt64 error) internal abstract PValue GetWriteValue(); + /// + /// Gets the tag's current typed value as a boxed via its public Value property. + /// + /// The boxed value, or when the concrete tag type has no public Value property. + public virtual object GetValue() + { + var valueProperty = GetType().GetProperty("Value", BindingFlags.Public | BindingFlags.Instance); + return valueProperty?.GetValue(this); + } + + /// + /// Sets the tag's current typed value from a boxed via its public Value property. + /// + /// The value to assign, which must be assignable to the concrete tag's Value property type. + public virtual void SetValue(object value) + { + var valueProperty = GetType().GetProperty("Value", BindingFlags.Public | BindingFlags.Instance); + valueProperty?.SetValue(this, value); + } + internal void SetTraceAddressMetadata(VarInfo variable) { if (variable == null) diff --git a/src/S7CommPlusDriver/Core/POffsetInfoType.cs b/src/S7CommPlusDriver/Core/POffsetInfoType.cs index 1972a65..3eefd3f 100644 --- a/src/S7CommPlusDriver/Core/POffsetInfoType.cs +++ b/src/S7CommPlusDriver/Core/POffsetInfoType.cs @@ -69,6 +69,12 @@ public enum OffsetInfoType public abstract bool Is1Dim(); public abstract bool IsMDim(); + /// + /// Gets the maximum string length for a String/WString offset info, or 0 when not applicable. + /// + /// The maximum length in characters, or 0 for non-string offset info types. + public virtual int MaxStringLength() { return 0; } + public static POffsetInfoType Deserialize(Stream buffer, int offsetinfotype, out int length) { switch ((OffsetInfoType)offsetinfotype) @@ -542,6 +548,8 @@ public class POffsetInfoType_Array1Dim : POffsetInfoType, IOffsetInfoType_1Dim public override bool HasRelation() { return false; } public override bool Is1Dim() { return true; } public override bool IsMDim() { return false; } + // For a string array, UnspecifiedOffsetinfo1 carries the maximum string length of the array elements. + public override int MaxStringLength() { return UnspecifiedOffsetinfo1; } public static POffsetInfoType_Array1Dim Deserialize(Stream buffer, out int length) { @@ -599,6 +607,8 @@ public class POffsetInfoType_String: POffsetInfoType public override bool HasRelation() { return false; } public override bool Is1Dim() { return false; } public override bool IsMDim() { return false; } + // Some firmware versions swap UnspecifiedOffsetinfo1/2, so take the smaller value as the max. string length. + public override int MaxStringLength() { return Math.Min(UnspecifiedOffsetinfo1, UnspecifiedOffsetinfo2); } public static POffsetInfoType_String Deserialize(Stream buffer, out int length) { From 46c1e577cbb22b82ea8586c0a551ecde69b3d9f9 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Mon, 14 Sep 2026 09:22:12 +0200 Subject: [PATCH 3/9] Fix TagFactory not forwarding declared PLC string length, causing STRING/WSTRING writes to fail --- .../PlcTagStringMaxLengthTests.cs | 91 +++++++++++++++++++ src/S7CommPlusDriver/ClientApi/Browser.cs | 7 ++ src/S7CommPlusDriver/ClientApi/PlcTag.cs | 13 +++ src/S7CommPlusDriver/ClientApi/PlcTags.cs | 27 +++++- .../S7CommPlusProtocolSession.cs | 15 ++- .../S7CommPlusTagAccessorCatalog.cs | 31 +++++-- 6 files changed, 166 insertions(+), 18 deletions(-) create mode 100644 src/S7CommPlusDriver.Tests/PlcTagStringMaxLengthTests.cs diff --git a/src/S7CommPlusDriver.Tests/PlcTagStringMaxLengthTests.cs b/src/S7CommPlusDriver.Tests/PlcTagStringMaxLengthTests.cs new file mode 100644 index 0000000..42921b2 --- /dev/null +++ b/src/S7CommPlusDriver.Tests/PlcTagStringMaxLengthTests.cs @@ -0,0 +1,91 @@ +using S7CommPlusDriver.ClientApi; +using Xunit; + +namespace S7CommPlusDriver.Tests +{ + /// + /// Verifies that forwards the PLC-declared maximum string length to + /// , , and , and that the resulting + /// write telegram matches the declared length instead of always falling back to the built-in default of 254. + /// + /// + /// Regression test for a bug where TagFactory never received the declared maximum string length, so every + /// String/WString tag was created with the built-in default of 254, regardless of its actual PLC + /// declaration (e.g. String[10]). Writing such a tag then sent a telegram of the wrong size and with the wrong + /// declared-length header byte, which the PLC rejected. + /// + public sealed class PlcTagStringMaxLengthTests + { + [Fact] + public void TagFactoryAppliesDeclaredMaxStringLengthToScalarStringTag() + { + var tag = PlcTags.TagFactory( + "DB.Text", + new ItemAddress("8A0E0001.F"), + Softdatatype.S7COMMP_SOFTDATATYPE_STRING, + Is1Dim: false, + maxStringLength: 10); + + var stringTag = Assert.IsType(tag); + stringTag.Value = "0123456789"; // exactly 10 characters, must not throw + + var writeValue = Assert.IsType(GetWriteValue(stringTag)); + var bytes = writeValue.GetValue(); + + Assert.Equal(12, bytes.Length); // declared max length (10) + 2 header bytes + Assert.Equal(10, bytes[0]); // declared max length, not the built-in default of 254 + Assert.Equal(10, bytes[1]); // actual length + } + + [Fact] + public void TagFactoryFallsBackToDefaultMaxStringLengthWhenNotSupplied() + { + var tag = PlcTags.TagFactory( + "DB.Text", + new ItemAddress("8A0E0001.F"), + Softdatatype.S7COMMP_SOFTDATATYPE_STRING); + + var stringTag = Assert.IsType(tag); + stringTag.Value = "Hello"; + + var writeValue = Assert.IsType(GetWriteValue(stringTag)); + var bytes = writeValue.GetValue(); + + Assert.Equal(256, bytes.Length); // built-in default of 254 + 2 header bytes + Assert.Equal(254, bytes[0]); + } + + [Fact] + public void TagFactoryAppliesDeclaredMaxStringLengthToStringArrayElementTag() + { + var tag = PlcTags.TagFactory( + "DB.Texts", + new ItemAddress("8A0E0001.F"), + Softdatatype.S7COMMP_SOFTDATATYPE_STRING, + Is1Dim: true, + maxStringLength: 5); + + Assert.IsType(tag); + Assert.Equal(5, tag.GetMaxStringLength()); + } + + [Fact] + public void TagFactoryAppliesDeclaredMaxStringLengthToWStringTag() + { + var tag = PlcTags.TagFactory( + "DB.UnicodeText", + new ItemAddress("8A0E0001.F"), + Softdatatype.S7COMMP_SOFTDATATYPE_WSTRING, + Is1Dim: false, + maxStringLength: 20); + + var wstringTag = Assert.IsType(tag); + Assert.Equal(20, wstringTag.GetMaxStringLength()); + } + + private static PValue GetWriteValue(PlcTag tag) + { + return tag.GetWriteValue(); + } + } +} diff --git a/src/S7CommPlusDriver/ClientApi/Browser.cs b/src/S7CommPlusDriver/ClientApi/Browser.cs index 8fc194e..d111bbd 100644 --- a/src/S7CommPlusDriver/ClientApi/Browser.cs +++ b/src/S7CommPlusDriver/ClientApi/Browser.cs @@ -174,6 +174,7 @@ private void AddFlatSubnodes( ArrayElementCount = GetArrayElementCount(node), ArrayDimensions = GetArrayDimensions(node), ContainsIndexedArray = containsIndexedArray, + MaxStringLength = node.Vte?.OffsetInfoType?.MaxStringLength() ?? 0, }; // If an Array element of basic datatype, the Vte is here from the parent array base element and offsets not valid here. if (node.NodeType == eNodeType.Array) @@ -940,6 +941,12 @@ public class VarInfo /// public IReadOnlyList ArrayDimensions = Array.Empty(); + /// + /// Gets the maximum string length declared in the PLC for String/WString values, or zero for every + /// other datatype. Required to build a correctly sized write telegram; see . + /// + public int MaxStringLength; + /// Retains the parsed symbol path so tag creation can reuse the exact CRC inputs. internal List SymbolCrcPath; diff --git a/src/S7CommPlusDriver/ClientApi/PlcTag.cs b/src/S7CommPlusDriver/ClientApi/PlcTag.cs index c40b5c0..790d148 100644 --- a/src/S7CommPlusDriver/ClientApi/PlcTag.cs +++ b/src/S7CommPlusDriver/ClientApi/PlcTag.cs @@ -64,6 +64,13 @@ public virtual void ProcessWriteResult(UInt64 error) internal abstract PValue GetWriteValue(); + /// + /// Gets the PLC-declared maximum string length for , , and + /// tags, or zero for every other tag type. Used to persist and recreate the exact + /// wire length required by (see ). + /// + internal virtual int GetMaxStringLength() => 0; + /// /// Gets the tag's current typed value as a boxed via its public Value property. /// @@ -1001,6 +1008,8 @@ public PlcTagString(string name, ItemAddress address, uint softdatatype, byte ma m_MaxLength = maxlength; } + internal override int GetMaxStringLength() => m_MaxLength; + public override void ProcessReadResult(object valueObj, ulong error) { LastReadError = error; @@ -1503,6 +1512,8 @@ public PlcTagWString(string name, ItemAddress address, uint softdatatype, ushort m_MaxLength = maxlength; } + internal override int GetMaxStringLength() => m_MaxLength; + public override void ProcessReadResult(object valueObj, ulong error) { LastReadError = error; @@ -2639,6 +2650,8 @@ public PlcTagStringArray(string name, ItemAddress address, uint softdatatype, by m_MaxLength = maxlength; } + internal override int GetMaxStringLength() => m_MaxLength; + public override void ProcessReadResult(object valueObj, ulong error) { LastReadError = error; diff --git a/src/S7CommPlusDriver/ClientApi/PlcTags.cs b/src/S7CommPlusDriver/ClientApi/PlcTags.cs index 09ad4e7..afd20a1 100644 --- a/src/S7CommPlusDriver/ClientApi/PlcTags.cs +++ b/src/S7CommPlusDriver/ClientApi/PlcTags.cs @@ -78,8 +78,13 @@ public static int WriteTags(this S7CommPlusProtocolSession conn, IEnumerable + /// + /// The PLC-declared maximum length for values, or zero when unknown. + /// Ignored for every other datatype. A value of zero falls back to the tag's built-in default of 254, matching the previous + /// behavior for callers that cannot supply this metadata. + /// /// The matching concrete tag implementation, or for an unsupported datatype. - public static PlcTag TagFactory(string name, ItemAddress address, uint softdatatype, bool Is1Dim = false) + public static PlcTag TagFactory(string name, ItemAddress address, uint softdatatype, bool Is1Dim = false, int maxStringLength = 0) { switch (softdatatype) { @@ -127,9 +132,15 @@ public static PlcTag TagFactory(string name, ItemAddress address, uint softdatat return new PlcTagDateAndTime(name, address, softdatatype); case Softdatatype.S7COMMP_SOFTDATATYPE_STRING: - if (Is1Dim) - return new PlcTagStringArray(name, address, softdatatype); - return new PlcTagString(name, address, softdatatype); + { + // A value of 0 (unknown/not supplied by the caller) keeps the tag's own built-in default (254). + var stringMaxLength = maxStringLength > 0 && maxStringLength <= byte.MaxValue + ? (byte)maxStringLength + : (byte)254; + if (Is1Dim) + return new PlcTagStringArray(name, address, softdatatype, stringMaxLength); + return new PlcTagString(name, address, softdatatype, stringMaxLength); + } case Softdatatype.S7COMMP_SOFTDATATYPE_POINTER: return new PlcTagPointer(name, address, softdatatype); @@ -178,7 +189,13 @@ public static PlcTag TagFactory(string name, ItemAddress address, uint softdatat case Softdatatype.S7COMMP_SOFTDATATYPE_WCHAR: return new PlcTagWChar(name, address, softdatatype); case Softdatatype.S7COMMP_SOFTDATATYPE_WSTRING: - return new PlcTagWString(name, address, softdatatype); + { + // A value of 0 (unknown/not supplied by the caller) keeps the tag's own built-in default (254). + var wstringMaxLength = maxStringLength > 0 && maxStringLength <= ushort.MaxValue + ? (ushort)maxStringLength + : (ushort)254; + return new PlcTagWString(name, address, softdatatype, wstringMaxLength); + } //case Softdatatype.S7COMMP_SOFTDATATYPE_VARIANT: //-> Variant isn't added inside of the instance-db as a variable! case Softdatatype.S7COMMP_SOFTDATATYPE_LTIME: diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs index 58e8b11..e3891bf 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs @@ -2067,7 +2067,8 @@ internal static bool IsAggregatePrimitiveArray(PVartypeListElement varType, stri private static PlcTag CreateResolvedPlcTag(VarInfo varInfo, PVartypeListElement varType, bool isAggregateArray) { var address = CreateItemAddress(varInfo); - var tag = PlcTags.TagFactory(varInfo.Name, address, varType.Softdatatype, isAggregateArray); + var maxStringLength = varType.OffsetInfoType?.MaxStringLength() ?? 0; + var tag = PlcTags.TagFactory(varInfo.Name, address, varType.Softdatatype, isAggregateArray, maxStringLength); tag?.SetTraceAddressMetadata(varInfo); if (!isAggregateArray || tag == null) { @@ -2081,7 +2082,7 @@ private static PlcTag CreateResolvedPlcTag(VarInfo varInfo, PVartypeListElement varInfo.AccessSequence, accessId, varType.OffsetInfoType.HasRelation()); - return PlcTags.TagFactory($"{varInfo.Name}[#{accessId}]", elementAddress, varType.Softdatatype); + return PlcTags.TagFactory($"{varInfo.Name}[#{accessId}]", elementAddress, varType.Softdatatype, false, maxStringLength); }) .Where(elementTag => elementTag != null) .ToList(); @@ -2106,7 +2107,7 @@ internal static PlcTag CreateResolvedPlcTag(VarInfo varInfo) var address = CreateItemAddress(varInfo); var isAggregateArray = varInfo.ArrayElementCount > 0; - var tag = PlcTags.TagFactory(varInfo.Name, address, varInfo.Softdatatype, isAggregateArray); + var tag = PlcTags.TagFactory(varInfo.Name, address, varInfo.Softdatatype, isAggregateArray, varInfo.MaxStringLength); tag?.SetTraceAddressMetadata(varInfo); if (!isAggregateArray || tag == null) { @@ -2120,7 +2121,9 @@ internal static PlcTag CreateResolvedPlcTag(VarInfo varInfo) varInfo.AccessSequence, accessId, RequiresArrayElementRelationSelector(varInfo.Softdatatype)), - varInfo.Softdatatype)) + varInfo.Softdatatype, + false, + varInfo.MaxStringLength)) .Where(elementTag => elementTag != null) .ToList(); tag.SetAggregateElements(elementTags); @@ -2321,7 +2324,9 @@ internal static bool TryCreateResolvedPrimitiveArrayElement( aggregateVariable.AccessSequence, accessId, RequiresArrayElementRelationSelector(aggregateVariable.Softdatatype)), - aggregateVariable.Softdatatype); + aggregateVariable.Softdatatype, + false, + aggregateVariable.MaxStringLength); return tag != null; } diff --git a/src/S7CommPlusDriver/S7CommPlusTagAccessorCatalog.cs b/src/S7CommPlusDriver/S7CommPlusTagAccessorCatalog.cs index 7658b5f..4a4d310 100644 --- a/src/S7CommPlusDriver/S7CommPlusTagAccessorCatalog.cs +++ b/src/S7CommPlusDriver/S7CommPlusTagAccessorCatalog.cs @@ -19,7 +19,7 @@ namespace S7CommPlusDriver /// public sealed class S7CommPlusTagAccessorCatalog { - private const int CurrentFormatVersion = 2; + private const int CurrentFormatVersion = 3; private const int MinimumSupportedFormatVersion = 1; private const int MaximumEntryCount = 2_000_000; private const int MaximumAggregateElementCount = 2_000_000; @@ -238,7 +238,7 @@ public static S7CommPlusTagAccessorCatalog ReadFrom(Stream source, string expect entries.Add( symbol, reader.ReadBoolean() - ? TagDescriptor.ReadFrom(reader, 0, version >= 2 ? symbol : null) + ? TagDescriptor.ReadFrom(reader, 0, version, version >= 2 ? symbol : null) : null); } return new S7CommPlusTagAccessorCatalog(structureHash, entries); @@ -329,16 +329,21 @@ private sealed class TagDescriptor /// The Siemens soft-datatype identifier. /// The immutable item-address descriptor. /// Scalar elements used to transfer an aggregate array. + /// + /// The PLC-declared maximum string length for String/WString tags, or zero for every other datatype. + /// private TagDescriptor( string name, uint datatype, AddressDescriptor address, - IReadOnlyList aggregateElements) + IReadOnlyList aggregateElements, + int maxStringLength) { Name = name; Datatype = datatype; _address = address; _aggregateElements = aggregateElements; + MaxStringLength = maxStringLength; } /// Gets the symbolic or generated element name. @@ -347,6 +352,9 @@ private TagDescriptor( /// Gets the Siemens soft-datatype identifier. private uint Datatype { get; } + /// Gets the PLC-declared maximum string length, or zero for every non-string datatype. + private int MaxStringLength { get; } + /// Creates a detached immutable descriptor from a resolved mutable driver tag. /// The resolved source tag. /// A recursively captured descriptor. @@ -357,14 +365,15 @@ internal static TagDescriptor FromTag(PlcTag tag) tag.Name, tag.Datatype, AddressDescriptor.FromAddress(tag.Address), - tag.AggregateElements.Select(FromTag).ToArray()); + tag.AggregateElements.Select(FromTag).ToArray(), + tag.GetMaxStringLength()); } /// Creates a fresh mutable driver tag and recursively recreates aggregate element tags. /// An accessor that shares no mutable state with previous materializations. internal PlcTag CreateTag(string nameOverride = null) { - var tag = PlcTags.TagFactory(nameOverride ?? Name, _address.CreateAddress(), Datatype, _aggregateElements.Count > 0); + var tag = PlcTags.TagFactory(nameOverride ?? Name, _address.CreateAddress(), Datatype, _aggregateElements.Count > 0, MaxStringLength); if (tag == null) { throw new InvalidDataException($"The cached datatype {Datatype} for symbol '{Name}' is not supported by this driver version."); @@ -391,13 +400,18 @@ internal void WriteTo(BinaryWriter writer, bool includeName = true) { element.WriteTo(writer); } + writer.Write(MaxStringLength); } /// Reads and validates one recursive descriptor from the catalog stream. /// The source reader. /// The current nesting depth used to reject maliciously recursive cache files. + /// + /// The catalog format version being read. Versions below 3 () predate persisted + /// string-length metadata; such caches fall back to 0, which recreates the tag's built-in default (254). + /// /// The validated descriptor. - internal static TagDescriptor ReadFrom(BinaryReader reader, int depth, string nameOverride = null) + internal static TagDescriptor ReadFrom(BinaryReader reader, int depth, int formatVersion, string nameOverride = null) { if (depth > MaximumDescriptorDepth) { @@ -414,9 +428,10 @@ internal static TagDescriptor ReadFrom(BinaryReader reader, int depth, string na var elements = new TagDescriptor[elementCount]; for (var index = 0; index < elementCount; index++) { - elements[index] = ReadFrom(reader, depth + 1); + elements[index] = ReadFrom(reader, depth + 1, formatVersion); } - return new TagDescriptor(name, datatype, address, elements); + var maxStringLength = formatVersion >= 3 ? reader.ReadInt32() : 0; + return new TagDescriptor(name, datatype, address, elements, maxStringLength); } } From a66defb56e859fe282c1595215db66887c1aea8e Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Mon, 14 Sep 2026 13:06:10 +0200 Subject: [PATCH 4/9] Reshape multidimensional aggregate array reads/writes into genuine multidimensional CLR arrays --- .../PlcTagMultiDimensionalArrayTests.cs | 268 ++++++++++++++++++ src/S7CommPlusDriver/ClientApi/PlcTag.cs | 140 ++++++++- .../S7CommPlusProtocolSession.cs | 27 +- 3 files changed, 421 insertions(+), 14 deletions(-) create mode 100644 src/S7CommPlusDriver.Tests/PlcTagMultiDimensionalArrayTests.cs diff --git a/src/S7CommPlusDriver.Tests/PlcTagMultiDimensionalArrayTests.cs b/src/S7CommPlusDriver.Tests/PlcTagMultiDimensionalArrayTests.cs new file mode 100644 index 0000000..16896a8 --- /dev/null +++ b/src/S7CommPlusDriver.Tests/PlcTagMultiDimensionalArrayTests.cs @@ -0,0 +1,268 @@ +using System; +using System.Linq; +using S7CommPlusDriver.ClientApi; +using Xunit; + +namespace S7CommPlusDriver.Tests +{ + /// + /// Verifies that a multidimensional primitive PLC array (e.g. Array[1..2, 1..3] of Int) is reshaped into a genuine + /// multidimensional CLR array on read, and correctly distributed back into its individual element tags on write, instead of + /// collapsing into (or requiring) a flat one-dimensional array. + /// + /// + /// Regression test for a design gap ported from the upstream fork's e91d8f2 ("Extended DataTypes") commit, which + /// introduced dedicated PlcTagXxxArrayMDim types there. This driver instead extends the existing + /// aggregate-element mechanism (, , + /// ) to reshape/distribute values without introducing new tag types. + /// + public sealed class PlcTagMultiDimensionalArrayTests + { + private static VarInfo CreateTwoByThreeIntArrayVarInfo() + { + return new VarInfo + { + Name = "DB.Matrix", + AccessSequence = "8A0E0001.F", + SymbolCrc = 0x11223344, + Softdatatype = Softdatatype.S7COMMP_SOFTDATATYPE_INT, + ArrayElementCount = 6, + ArrayDimensions = new[] + { + new S7CommPlusArrayDimension(1, 2), + new S7CommPlusArrayDimension(1, 3), + }, + }; + } + + [Fact] + public void CreateResolvedPlcTagReshapesMultiDimensionalReadIntoGenuineMultiDimensionalArray() + { + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByThreeIntArrayVarInfo()); + + Assert.Equal(6, tag.AggregateElements.Count); + for (var index = 0; index < tag.AggregateElements.Count; index++) + { + tag.AggregateElements[index].ProcessReadResult(new ValueInt((short)(index + 1)), 0); + } + tag.CompleteAggregateRead(0); + + var value = Assert.IsType(tag.GetValue()); + Assert.Equal(2, value.GetLength(0)); + Assert.Equal(3, value.GetLength(1)); + // Row-major: outermost (first declared) dimension varies slowest. + Assert.Equal(1, value[0, 0]); + Assert.Equal(2, value[0, 1]); + Assert.Equal(3, value[0, 2]); + Assert.Equal(4, value[1, 0]); + Assert.Equal(5, value[1, 1]); + Assert.Equal(6, value[1, 2]); + } + + [Fact] + public void CreateResolvedPlcTagDistributesMultiDimensionalWriteIntoElementTags() + { + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByThreeIntArrayVarInfo()); + + var writeValue = new short[2, 3] { { 10, 20, 30 }, { 40, 50, 60 } }; + tag.SetValue(writeValue); + + var elementValues = tag.AggregateElements + .Select(element => ((PlcTagInt)element).Value) + .ToArray(); + Assert.Equal(new short[] { 10, 20, 30, 40, 50, 60 }, elementValues); + } + + [Fact] + public void CreateResolvedPlcTagKeepsFlatArrayForOneDimensionalAggregate() + { + var varInfo = new VarInfo + { + Name = "DB.Values", + AccessSequence = "8A0E0001.F", + SymbolCrc = 0x55667788, + Softdatatype = Softdatatype.S7COMMP_SOFTDATATYPE_INT, + ArrayElementCount = 3, + ArrayDimensions = new[] { new S7CommPlusArrayDimension(0, 3) }, + }; + + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo); + for (var index = 0; index < tag.AggregateElements.Count; index++) + { + tag.AggregateElements[index].ProcessReadResult(new ValueInt((short)(index + 1)), 0); + } + tag.CompleteAggregateRead(0); + + var value = Assert.IsType(tag.GetValue()); + Assert.Equal(new short[] { 1, 2, 3 }, value); + } + + [Fact] + public void CreateResolvedPlcTagReshapesMultiDimensionalBoolReadIntoGenuineMultiDimensionalArray() + { + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_BOOL, "DB.Flags", 0x22334455)); + var expected = new[] { true, false, false, true }; + for (var index = 0; index < tag.AggregateElements.Count; index++) + { + tag.AggregateElements[index].ProcessReadResult(new ValueBool(expected[index]), 0); + } + tag.CompleteAggregateRead(0); + + var value = Assert.IsType(tag.GetValue()); + Assert.Equal(2, value.GetLength(0)); + Assert.Equal(2, value.GetLength(1)); + Assert.True(value[0, 0]); + Assert.False(value[0, 1]); + Assert.False(value[1, 0]); + Assert.True(value[1, 1]); + } + + [Fact] + public void CreateResolvedPlcTagDistributesMultiDimensionalBoolWriteIntoElementTags() + { + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_BOOL, "DB.Flags", 0x22334455)); + tag.SetValue(new[,] { { true, false }, { false, true } }); + + var elementValues = tag.AggregateElements + .Select(element => ((PlcTagBool)element).Value) + .ToArray(); + Assert.Equal(new[] { true, false, false, true }, elementValues); + } + + [Fact] + public void CreateResolvedPlcTagReshapesMultiDimensionalRealReadIntoGenuineMultiDimensionalArray() + { + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_REAL, "DB.Measurements", 0x33445566)); + var expected = new[] { 1.5f, 2.5f, 3.5f, 4.5f }; + for (var index = 0; index < tag.AggregateElements.Count; index++) + { + tag.AggregateElements[index].ProcessReadResult(new ValueReal(expected[index]), 0); + } + tag.CompleteAggregateRead(0); + + var value = Assert.IsType(tag.GetValue()); + Assert.Equal(2, value.GetLength(0)); + Assert.Equal(2, value.GetLength(1)); + Assert.Equal(1.5f, value[0, 0]); + Assert.Equal(2.5f, value[0, 1]); + Assert.Equal(3.5f, value[1, 0]); + Assert.Equal(4.5f, value[1, 1]); + } + + [Fact] + public void CreateResolvedPlcTagDistributesMultiDimensionalRealWriteIntoElementTags() + { + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_REAL, "DB.Measurements", 0x33445566)); + tag.SetValue(new[,] { { 1.5f, 2.5f }, { 3.5f, 4.5f } }); + + var elementValues = tag.AggregateElements + .Select(element => ((PlcTagReal)element).Value) + .ToArray(); + Assert.Equal(new[] { 1.5f, 2.5f, 3.5f, 4.5f }, elementValues); + } + + [Fact] + public void CreateResolvedPlcTagReshapesMultiDimensionalStringReadIntoGenuineMultiDimensionalArray() + { + var varInfo = CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_STRING, "DB.Texts", 0x44556677); + varInfo.MaxStringLength = 10; + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo); + var expected = new[] { "aa", "bb", "cc", "dd" }; + for (var index = 0; index < tag.AggregateElements.Count; index++) + { + tag.AggregateElements[index].ProcessReadResult(CreateStringReadValue(expected[index], 10), 0); + } + tag.CompleteAggregateRead(0); + + var value = Assert.IsType(tag.GetValue()); + Assert.Equal(2, value.GetLength(0)); + Assert.Equal(2, value.GetLength(1)); + Assert.Equal("aa", value[0, 0]); + Assert.Equal("bb", value[0, 1]); + Assert.Equal("cc", value[1, 0]); + Assert.Equal("dd", value[1, 1]); + } + + [Fact] + public void CreateResolvedPlcTagDistributesMultiDimensionalStringWriteIntoElementTags() + { + var varInfo = CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_STRING, "DB.Texts", 0x44556677); + varInfo.MaxStringLength = 10; + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo); + tag.SetValue(new[,] { { "aa", "bb" }, { "cc", "dd" } }); + + var elementValues = tag.AggregateElements + .Select(element => ((PlcTagString)element).Value) + .ToArray(); + Assert.Equal(new[] { "aa", "bb", "cc", "dd" }, elementValues); + } + + [Fact] + public void CreateResolvedPlcTagReshapesMultiDimensionalDtlStructReadIntoGenuineMultiDimensionalArray() + { + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(CreateTwoByTwoVarInfo(Softdatatype.S7COMMP_SOFTDATATYPE_DTL, "DB.Timestamps", 0x66778899)); + var expected = new[] + { + new DateTime(2020, 1, 1), + new DateTime(2020, 1, 2), + new DateTime(2020, 1, 3), + new DateTime(2020, 1, 4), + }; + for (var index = 0; index < tag.AggregateElements.Count; index++) + { + tag.AggregateElements[index].ProcessReadResult(CreateDtlReadValue(expected[index]), 0); + } + tag.CompleteAggregateRead(0); + + var value = Assert.IsType(tag.GetValue()); + Assert.Equal(2, value.GetLength(0)); + Assert.Equal(2, value.GetLength(1)); + Assert.Equal(expected[0], value[0, 0]); + Assert.Equal(expected[1], value[0, 1]); + Assert.Equal(expected[2], value[1, 0]); + Assert.Equal(expected[3], value[1, 1]); + } + + private static VarInfo CreateTwoByTwoVarInfo(uint softdatatype, string name, uint symbolCrc) + { + return new VarInfo + { + Name = name, + AccessSequence = "8A0E0001.F", + SymbolCrc = symbolCrc, + Softdatatype = softdatatype, + ArrayElementCount = 4, + ArrayDimensions = new[] + { + new S7CommPlusArrayDimension(0, 2), + new S7CommPlusArrayDimension(0, 2), + }, + }; + } + + private static ValueUSIntArray CreateStringReadValue(string text, byte maxLength) + { + var bytes = System.Text.Encoding.GetEncoding("ISO-8859-1").GetBytes(text); + var b = new byte[maxLength + 2]; + b[0] = maxLength; + b[1] = (byte)bytes.Length; + Array.Copy(bytes, 0, b, 2, bytes.Length); + return new ValueUSIntArray(b); + } + + private static ValueStruct CreateDtlReadValue(DateTime value) + { + var structValue = new ValueStruct(0x02000043); + var barr = new byte[12]; + barr[0] = (byte)(value.Year >> 8); + barr[1] = (byte)value.Year; + barr[2] = (byte)value.Month; + barr[3] = (byte)value.Day; + barr[5] = (byte)value.Hour; + barr[6] = (byte)value.Minute; + barr[7] = (byte)value.Second; + structValue.AddStructElement(0x02000043, new ValueByteArray(barr)); + return structValue; + } + } +} diff --git a/src/S7CommPlusDriver/ClientApi/PlcTag.cs b/src/S7CommPlusDriver/ClientApi/PlcTag.cs index 790d148..986e7ec 100644 --- a/src/S7CommPlusDriver/ClientApi/PlcTag.cs +++ b/src/S7CommPlusDriver/ClientApi/PlcTag.cs @@ -18,6 +18,7 @@ namespace S7CommPlusDriver.ClientApi public abstract class PlcTag { private IReadOnlyList m_AggregateElements = Array.Empty(); + private IReadOnlyList m_AggregateDimensions = Array.Empty(); public string Name; public ItemAddress Address; @@ -72,21 +73,36 @@ public virtual void ProcessWriteResult(UInt64 error) internal virtual int GetMaxStringLength() => 0; /// - /// Gets the tag's current typed value as a boxed via its public Value property. + /// Gets the tag's current typed value. For a multidimensional aggregate array (see and + /// ), the reshaped multidimensional array is returned because the concrete tag's Value + /// property can only expose a flat one-dimensional array. Every other tag returns its boxed public Value property. /// /// The boxed value, or when the concrete tag type has no public Value property. public virtual object GetValue() { + if (m_AggregateDimensions.Count > 1) + { + return AggregateValue; + } var valueProperty = GetType().GetProperty("Value", BindingFlags.Public | BindingFlags.Instance); return valueProperty?.GetValue(this); } /// - /// Sets the tag's current typed value from a boxed via its public Value property. + /// Sets the tag's current typed value. For a multidimensional aggregate array, is expected to be + /// a multidimensional matching the PLC-declared dimensions; it is reshaped and distributed directly + /// into the individual because the concrete tag's Value property can only hold a + /// flat one-dimensional array. Every other tag assigns to its public Value property. /// /// The value to assign, which must be assignable to the concrete tag's Value property type. public virtual void SetValue(object value) { + if (m_AggregateDimensions.Count > 1 && value is Array multiDimValue) + { + DistributeMultiDimensionalValue(multiDimValue); + AggregateValue = multiDimValue; + return; + } var valueProperty = GetType().GetProperty("Value", BindingFlags.Public | BindingFlags.Instance); valueProperty?.SetValue(this, value); } @@ -125,14 +141,21 @@ protected static int CheckErrorAndType(ulong error, object valueObj, Type checkT /// /// Configures the scalar element tags that collectively represent this aggregate PLC array. /// - /// Element tags in PLC declaration order. - internal void SetAggregateElements(IReadOnlyList elements) + /// Element tags in PLC declaration order (row-major, outermost dimension first). + /// + /// The PLC-declared dimension lengths in declaration order (outermost first), or /empty for a + /// one-dimensional aggregate array. A single-element list is treated the same as . + /// + internal void SetAggregateElements(IReadOnlyList elements, IReadOnlyList dimensions = null) { m_AggregateElements = elements ?? throw new ArgumentNullException(nameof(elements)); + m_AggregateDimensions = dimensions != null && dimensions.Count > 1 ? dimensions : Array.Empty(); } /// /// Publishes a completed set of scalar element reads as one aggregate value and mirrors it into a compatible typed array tag. + /// A multidimensional aggregate array ( has more than one entry) is reshaped into a + /// genuine multidimensional instead of the concrete tag's inherently flat Value property. /// /// The first element error, or zero when every element succeeded. internal virtual void CompleteAggregateRead(ulong itemError) @@ -153,29 +176,39 @@ internal virtual void CompleteAggregateRead(ulong itemError) return; } - var values = Array.CreateInstance(elementValueProperty.PropertyType, m_AggregateElements.Count); + var flatValues = Array.CreateInstance(elementValueProperty.PropertyType, m_AggregateElements.Count); for (var index = 0; index < m_AggregateElements.Count; index++) { - values.SetValue(elementValueProperty.GetValue(m_AggregateElements[index]), index); + flatValues.SetValue(elementValueProperty.GetValue(m_AggregateElements[index]), index); } - AggregateValue = values; - var aggregateValueProperty = GetType().GetProperty("Value", BindingFlags.Instance | BindingFlags.Public); - if (aggregateValueProperty?.PropertyType == values.GetType()) + if (m_AggregateDimensions.Count > 1) { - aggregateValueProperty.SetValue(this, values); + AggregateValue = ReshapeToMultiDimensional(elementValueProperty.PropertyType, flatValues, m_AggregateDimensions); + } + else + { + AggregateValue = flatValues; + var aggregateValueProperty = GetType().GetProperty("Value", BindingFlags.Instance | BindingFlags.Public); + if (aggregateValueProperty?.PropertyType == flatValues.GetType()) + { + aggregateValueProperty.SetValue(this, flatValues); + } } Quality = PlcTagQC.TAG_QUALITY_GOOD; } /// /// Copies a typed parent array value into its scalar element tags before an aggregate write is encoded. - /// Unsupported native array types intentionally keep their explicitly populated element values unchanged. + /// Unsupported native array types intentionally keep their explicitly populated element values unchanged. A + /// multidimensional aggregate array is intentionally skipped here because already distributed + /// its values directly into at assignment time (its concrete tag's Value property + /// cannot hold a multidimensional shape). /// /// A typed parent array does not contain exactly one value per aggregate element. internal virtual void PrepareAggregateWrite() { - if (m_AggregateElements.Count == 0) + if (m_AggregateElements.Count == 0 || m_AggregateDimensions.Count > 1) { return; } @@ -204,6 +237,89 @@ internal virtual void PrepareAggregateWrite() } } + /// + /// Distributes a caller-supplied multidimensional array directly into the scalar element tags' Value properties, + /// in the same row-major order used by and the PLC's own element access IDs. + /// + /// The multidimensional array to distribute. Its shape must match . + /// The array's rank or per-dimension lengths do not match the PLC-declared dimensions. + private void DistributeMultiDimensionalValue(Array multiDimValue) + { + if (multiDimValue.Rank != m_AggregateDimensions.Count) + { + throw new ArgumentException( + $"Aggregate tag '{Name}' requires an array of rank {m_AggregateDimensions.Count}, but received rank {multiDimValue.Rank}.", + nameof(multiDimValue)); + } + for (var dimension = 0; dimension < m_AggregateDimensions.Count; dimension++) + { + if (multiDimValue.GetLength(dimension) != m_AggregateDimensions[dimension]) + { + throw new ArgumentException( + $"Aggregate tag '{Name}' requires dimension {dimension} to have length {m_AggregateDimensions[dimension]}, but received {multiDimValue.GetLength(dimension)}.", + nameof(multiDimValue)); + } + } + + var flatIndex = 0; + foreach (var indices in EnumerateRowMajorIndices(m_AggregateDimensions)) + { + var elementValueProperty = m_AggregateElements[flatIndex].GetType().GetProperty("Value", BindingFlags.Instance | BindingFlags.Public); + if (elementValueProperty == null) + { + throw new InvalidOperationException($"Aggregate element '{m_AggregateElements[flatIndex].Name}' has no writable Value property."); + } + elementValueProperty.SetValue(m_AggregateElements[flatIndex], multiDimValue.GetValue(indices)); + flatIndex++; + } + } + + /// + /// Reshapes a flat array, ordered exactly like , into a genuine multidimensional array. + /// + /// The CLR element type shared by every scalar element tag's Value property. + /// The flat values in row-major order (outermost dimension first). + /// The PLC-declared dimension lengths in declaration order (outermost first). + /// A multidimensional array of with one entry per element of . + private static Array ReshapeToMultiDimensional(Type elementType, Array flatValues, IReadOnlyList dimensions) + { + var lengths = dimensions.Select(dimension => checked((int)dimension)).ToArray(); + var multiDimValues = Array.CreateInstance(elementType, lengths); + var flatIndex = 0; + foreach (var indices in EnumerateRowMajorIndices(dimensions)) + { + multiDimValues.SetValue(flatValues.GetValue(flatIndex), indices); + flatIndex++; + } + return multiDimValues; + } + + /// + /// Enumerates every index combination of a multidimensional shape in row-major order (outermost dimension varies + /// slowest), matching the order in which the PLC protocol enumerates aggregate array element access IDs. + /// + /// The dimension lengths in declaration order (outermost first). + private static IEnumerable EnumerateRowMajorIndices(IReadOnlyList dimensions) + { + var indices = new int[dimensions.Count]; + var totalCount = 1L; + foreach (var dimension in dimensions) + { + totalCount = checked(totalCount * dimension); + } + + for (var flatIndex = 0L; flatIndex < totalCount; flatIndex++) + { + var remainder = flatIndex; + for (var dimension = dimensions.Count - 1; dimension >= 0; dimension--) + { + indices[dimension] = checked((int)(remainder % dimensions[dimension])); + remainder /= dimensions[dimension]; + } + yield return indices; + } + } + protected static string ResultString(PlcTag tag, string value ) { return String.Format("{0:X02}: {1}", tag.Quality, value); diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs index e3891bf..c06a260 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs @@ -2086,10 +2086,30 @@ private static PlcTag CreateResolvedPlcTag(VarInfo varInfo, PVartypeListElement }) .Where(elementTag => elementTag != null) .ToList(); - tag.SetAggregateElements(elementTags); + tag.SetAggregateElements(elementTags, GetAggregateArrayDimensions(varType)); return tag; } + /// + /// Extracts the PLC-declared dimension lengths (outermost first) of an aggregate primitive array, for reshaping the + /// flat element-tag list into a genuine multidimensional array. Returns an empty list for one-dimensional arrays. + /// + /// The resolved member datatype and dimension metadata. + private static IReadOnlyList GetAggregateArrayDimensions(PVartypeListElement varType) + { + if (varType.OffsetInfoType is not IOffsetInfoType_MDim multiDimensional) + { + return Array.Empty(); + } + + // GetMdimArrayElementCount() stores dimensions inside-out; reverse to declaration order (outermost first) and + // drop trailing zero-length entries, matching GetAggregateArrayElementAccessIds's own filtering. + return multiDimensional.GetMdimArrayElementCount() + .TakeWhile(count => count > 0) + .Reverse() + .ToArray(); + } + /// /// Creates a typed tag directly from one aggregate browse result, avoiding a second walk through the PLC type catalog. /// @@ -2126,7 +2146,10 @@ internal static PlcTag CreateResolvedPlcTag(VarInfo varInfo) varInfo.MaxStringLength)) .Where(elementTag => elementTag != null) .ToList(); - tag.SetAggregateElements(elementTags); + var dimensions = varInfo.ArrayDimensions != null && varInfo.ArrayDimensions.Count > 1 + ? varInfo.ArrayDimensions.Select(dimension => dimension.ElementCount).ToArray() + : Array.Empty(); + tag.SetAggregateElements(elementTags, dimensions); return tag; } From 6e87805a9ec192be55ebc84afd71ac76b7219dc8 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Thu, 17 Sep 2026 10:39:23 +0200 Subject: [PATCH 5/9] perf: reduce O(n^2) payload-check serialization in read/write batching ExceedsSingleFramePayload previously reserialized the entire batch request from scratch on every single item added while building a chunk in ReadValues and CreateWriteRequestBatch (used by WriteValues). For large tag counts (e.g. 210k tags at 100 tags/request) this caused ~n^2 serializations per chunk and a large number of MemoryStream allocations. Introduces AppendItemsWithinPayloadLimit, which uses a galloping search followed by a binary search to find the exact same maximum item count as the previous linear scan (the serialized length grows monotonically with the number of appended items), reducing full-request serializations per chunk from O(n) to O(log n). Behavior is unchanged: same chunk contents, same first-item-always-included semantics, same error handling on communication failures (hard abort, no partial-result handling changed). Adds a theory-based regression test asserting the batch split point matches exactly across several sizes, including power-of-two boundaries where the galloping search transitions to its binary-search phase. --- .../S7CommPlusProtocolSessionReceiveTests.cs | 38 +++++ src/S7CommPlusDriver/S7CommPlusClient.cs | 5 + .../S7CommPlusProtocolSession.cs | 132 +++++++++++++++--- 3 files changed, 154 insertions(+), 21 deletions(-) diff --git a/src/S7CommPlusDriver.Tests/S7CommPlusProtocolSessionReceiveTests.cs b/src/S7CommPlusDriver.Tests/S7CommPlusProtocolSessionReceiveTests.cs index 39a4455..c643828 100644 --- a/src/S7CommPlusDriver.Tests/S7CommPlusProtocolSessionReceiveTests.cs +++ b/src/S7CommPlusDriver.Tests/S7CommPlusProtocolSessionReceiveTests.cs @@ -254,6 +254,44 @@ public void WriteBatchAlsoHonorsPlcItemLimit() Assert.InRange(batch.SerializedLength, 1, 987); } + [Theory] + [InlineData(1)] + [InlineData(2)] + [InlineData(3)] + [InlineData(7)] + [InlineData(8)] + [InlineData(9)] + [InlineData(16)] + [InlineData(17)] + [InlineData(63)] + [InlineData(64)] + [InlineData(65)] + [InlineData(500)] + public void WriteBatchSplitPointMatchesLinearScanForVariousSizes(int payloadItemLimitByCount) + { + // Uses uniformly sized items so the exact split point can be pinned down: probe the + // serialized length of exactly N items via the real predicate, then use that length as + // the payload limit and assert the batching under test stops at exactly N items. This + // exercises the galloping/binary-search based implementation across split points that + // land before, on, and after several power-of-two boundaries. + var connection = new S7CommPlusProtocolSession(); + const int totalItems = 1000; + var addresses = Enumerable.Range(1, totalItems) + .Select(i => new ItemAddress($"8A0E{i % 0xFFFF:X4}.1")) + .ToArray(); + var values = Enumerable.Range(1, totalItems) + .Select(i => (PValue)new ValueDInt(i)) + .ToArray(); + + var probe = connection.DebugCreateWriteRequestBatchForTests(addresses, values, payloadItemLimitByCount, int.MaxValue); + var maxPayloadSize = (int)probe.SerializedLength; + + var batch = connection.DebugCreateWriteRequestBatchForTests(addresses, values, totalItems, maxPayloadSize); + + Assert.Equal(payloadItemLimitByCount, batch.ItemCount); + Assert.InRange(batch.SerializedLength, 1, maxPayloadSize); + } + [Fact] public void LargeLegacyPayloadIsProtectedOnceBeforeTransportFragmentation() { diff --git a/src/S7CommPlusDriver/S7CommPlusClient.cs b/src/S7CommPlusDriver/S7CommPlusClient.cs index 488057a..ab01fd2 100644 --- a/src/S7CommPlusDriver/S7CommPlusClient.cs +++ b/src/S7CommPlusDriver/S7CommPlusClient.cs @@ -4,6 +4,7 @@ using S7CommPlusDriver.Internal; using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -1325,7 +1326,11 @@ public Task> ReadAsync(IEnumerabl return ExecuteReadOperationAsync("ReadTags", session => { var requestTags = ExpandAggregateTags(tagList); + _options.Logger.LogInformation("ReadTagValuesInBatches start..."); + var sw = Stopwatch.StartNew(); var (values, itemErrors) = ReadTagValuesInBatches(session, requestTags); + sw.Stop(); + _options.Logger.LogInformation($"ReadTagValuesInBatches finished: {sw.ElapsedMilliseconds}ms"); var items = new List(tagList.Count); var requestIndex = 0; foreach (var tag in tagList) diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs index c06a260..b1343de 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs @@ -679,6 +679,98 @@ private static bool ExceedsSingleFramePayload(IS7pRequest request, int maxPayloa return GetSerializedRequestLengthForBatching(request) > maxPayloadSize; } + /// + /// Appends as many candidate items as possible to a batch request without exceeding the negotiated + /// single-frame payload limit, mirroring the legacy "add one item, reserialize, roll back on overflow" + /// behavior but without the O(n^2) cost of fully reserializing the request after every single item. + /// + /// + /// The serialized request length grows monotonically with the number of appended items (each item + /// contributes at least one byte, and VLQ-encoded counters never shrink as items are added), so a + /// galloping search followed by a binary search finds the exact same maximum item count as the + /// original linear scan, using O(log n) full-request serializations per chunk instead of O(n). + /// The very first candidate item is always appended unconditionally (matching the legacy behavior of + /// never rejecting the first item of a chunk, even if it alone would exceed the payload limit). + /// + /// The maximum number of remaining candidate items available for this chunk. + /// The negotiated single-frame payload limit in bytes. + /// The request whose serialized size is checked via / side effects. + /// Appends the candidate item at the given zero-based offset (relative to the chunk start) to . + /// Removes the most recently appended candidate item from . + /// The number of candidate items that were appended to and fit within the payload limit. + private static int AppendItemsWithinPayloadLimit( + int maxCandidateCount, + int maxPayloadSize, + IS7pRequest request, + Action addItemAt, + Action removeLastItem) + { + if (maxCandidateCount <= 0) + { + return 0; + } + + // The first item of a chunk is always included unconditionally, matching the legacy behavior. + addItemAt(0); + var added = 1; + + var step = 1; + while (added < maxCandidateCount) + { + var next = Math.Min(step, maxCandidateCount - added); + for (var k = 0; k < next; k++) + { + addItemAt(added + k); + } + + if (ExceedsSingleFramePayload(request, maxPayloadSize)) + { + for (var k = 0; k < next; k++) + { + removeLastItem(); + } + + // Binary search for the exact number of additional items (within this galloping + // step) that still fit, exploiting the monotonic growth of the serialized length. + int lo = 0, hi = next; + while (lo < hi) + { + var mid = (lo + hi + 1) / 2; + for (var k = 0; k < mid; k++) + { + addItemAt(added + k); + } + var fits = !ExceedsSingleFramePayload(request, maxPayloadSize); + for (var k = 0; k < mid; k++) + { + removeLastItem(); + } + + if (fits) + { + lo = mid; + } + else + { + hi = mid - 1; + } + } + + for (var k = 0; k < lo; k++) + { + addItemAt(added + k); + } + added += lo; + break; + } + + added += next; + step *= 2; + } + + return added; + } + private static long GetSerializedRequestLengthForBatching(IS7pRequest request) { uint sessionId = request.SessionId; @@ -1436,17 +1528,13 @@ public int ReadValues(List addresslist, out List values, ou var getMultiVarReq = new GetMultiVariablesRequest(ProtocolVersion.V2); getMultiVarReq.AddressList.Clear(); - count_perChunk = 0; - while (count_perChunk < maxTagsPerRequest && (chunk_startIndex + count_perChunk) < addresslist.Count) - { - getMultiVarReq.AddressList.Add(addresslist[chunk_startIndex + count_perChunk]); - if (count_perChunk > 0 && ExceedsSingleFramePayload(getMultiVarReq, maxPayloadSize)) - { - getMultiVarReq.AddressList.RemoveAt(getMultiVarReq.AddressList.Count - 1); - break; - } - count_perChunk++; - } + var maxCandidates = Math.Min(maxTagsPerRequest, addresslist.Count - chunk_startIndex); + count_perChunk = AppendItemsWithinPayloadLimit( + maxCandidates, + maxPayloadSize, + getMultiVarReq, + offset => getMultiVarReq.AddressList.Add(addresslist[chunk_startIndex + offset]), + () => getMultiVarReq.AddressList.RemoveAt(getMultiVarReq.AddressList.Count - 1)); res = SendS7plusFunctionObjectAndWait(getMultiVarReq, m_ReadTimeout); if (res != 0) @@ -1557,19 +1645,21 @@ private static SetMultiVariablesRequest CreateWriteRequestBatch( out int itemCount) { var request = new SetMultiVariablesRequest(ProtocolVersion.V2); - itemCount = 0; - while (itemCount < maxItems && startIndex + itemCount < addresses.Count) - { - request.AddressListVar.Add(addresses[startIndex + itemCount]); - request.ValueList.Add(values[startIndex + itemCount]); - if (itemCount > 0 && ExceedsSingleFramePayload(request, maxPayloadSize)) + var maxCandidates = Math.Min(maxItems, addresses.Count - startIndex); + itemCount = AppendItemsWithinPayloadLimit( + maxCandidates, + maxPayloadSize, + request, + offset => + { + request.AddressListVar.Add(addresses[startIndex + offset]); + request.ValueList.Add(values[startIndex + offset]); + }, + () => { request.AddressListVar.RemoveAt(request.AddressListVar.Count - 1); request.ValueList.RemoveAt(request.ValueList.Count - 1); - break; - } - itemCount++; - } + }); return request; } From 56939b0f4d07c02d53605e5a50dbb274aa351b49 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Thu, 17 Sep 2026 11:40:03 +0200 Subject: [PATCH 6/9] perf: pool MemoryStream buffer used for payload-size probing during batching GetSerializedRequestLengthForBatching is called repeatedly per chunk by the galloping/binary-search batching introduced previously (potentially thousands of times for large tag counts). It previously allocated a new MemoryStream (and backing byte array) on every call just to measure the serialized request length. Reuses a [ThreadStatic] MemoryStream across calls, resetting its length instead of reallocating. Safe without locking because a single batching operation runs synchronously on one thread and this helper is never called reentrantly. Reduces GC pressure for large batched read/write operations without changing any observable behavior. --- .../S7CommPlusProtocolSession.cs | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs index b1343de..a03e74f 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs @@ -771,6 +771,15 @@ private static int AppendItemsWithinPayloadLimit( return added; } + // Reused across calls to GetSerializedRequestLengthForBatching to avoid allocating a new + // MemoryStream (and its backing buffer) for every payload-size probe. Batching a large tag + // count can trigger many thousands of these probes (galloping + binary search per chunk), + // so pooling the buffer meaningfully reduces GC pressure. [ThreadStatic] keeps this safe + // without locking, since a single batching operation runs synchronously on one thread and + // this helper is never called reentrantly. + [ThreadStatic] + private static MemoryStream t_PayloadLengthProbeStream; + private static long GetSerializedRequestLengthForBatching(IS7pRequest request) { uint sessionId = request.SessionId; @@ -784,7 +793,17 @@ private static long GetSerializedRequestLengthForBatching(IS7pRequest request) request.SequenceNumber = UInt16.MaxValue; request.IntegrityId = UInt32.MaxValue; - using var stream = new MemoryStream(); + var stream = t_PayloadLengthProbeStream; + if (stream == null) + { + stream = new MemoryStream(1024); + t_PayloadLengthProbeStream = stream; + } + else + { + stream.SetLength(0); + } + request.Serialize(stream); return stream.Length; } From da15bcc991c1b15fdd58262a5f620a3d4721a7cf Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Tue, 22 Sep 2026 14:30:39 +0200 Subject: [PATCH 7/9] perf: fast-path full-batch payload check before search fallback --- ...tch-read-write-performance-optimization.md | 124 ++++++++++++++++++ .../S7CommPlusProtocolSession.cs | 20 +++ 2 files changed, 144 insertions(+) create mode 100644 docs/batch-read-write-performance-optimization.md diff --git a/docs/batch-read-write-performance-optimization.md b/docs/batch-read-write-performance-optimization.md new file mode 100644 index 0000000..570e374 --- /dev/null +++ b/docs/batch-read-write-performance-optimization.md @@ -0,0 +1,124 @@ +# S7CommPlus Batch Read/Write Performance Optimization + +Notes on the analysis and optimizations made to the batched read/write path of the +S7CommPlus driver, targeted at scenarios with a very large number of tags. + +## Context / problem statement + +The optimization was driven by a real-world case: + +- **~210,000 tags** read in a single logical operation. +- `_tagsPerReadRequestMax = 100`, i.e. the PLC negotiates a maximum of **100 items per + read request**. +- Requirement: strictly preserve the existing **hard-abort-on-error** behavior. Partial + results are deliberately *not* surfaced to the caller (the protocol layer produces + them, but the client layer intentionally discards them via `ThrowIfError`). Only + performance may change, not observable behavior. + +## Where the time goes (baseline analysis) + +Key methods: + +- `S7CommPlusClient.ReadTagValuesInBatches` (client-side outer batching loop) +- `S7CommPlusProtocolSession.ReadValues` / `WriteValues` (protocol-layer request building) +- `GetSerializedRequestLengthForBatching` (payload-size probe) +- `ExceedsSingleFramePayload` (comparison against the negotiated single-frame payload limit) + +Findings: + +1. **2100 sequential requests.** With 100 items per request, 210,000 tags turn into + 2100 sequential request/response round-trips. This is dominated by the PLC item limit, + so it is mostly I/O-bound and hard to change without protocol-level pipelining (see + backlog item 6). + +2. **Two nested batching layers.** The client batches into `_tagsPerReadRequestMax` + chunks, and the protocol layer *again* splits by the negotiated TPDU payload size. + +3. **O(n^2) payload check (the main avoidable CPU cost).** For every candidate item that + was added, the *entire* request was fully re-serialized to measure its byte length. + Building one chunk of `n` items therefore cost `O(n)` full serializations, i.e. + `O(n^2)` byte-level work per chunk. Across 2100 chunks this dominated the CPU time. + +## Optimizations implemented + +### 1. Remove the O(n^2) reserialization (committed) + +- Branch `feature/perf-fix-batching-payload-check`, commit `f0bccad`. +- New helper `AppendItemsWithinPayloadLimit` in `S7CommPlusProtocolSession.cs`, used by + both the read path (`ReadValues`) and the write path (`CreateWriteRequestBatch`). +- Technique: **galloping search followed by binary search** to find the exact number of + items that fit within the payload limit, using **O(log n)** full-request serializations + per chunk instead of O(n). +- Correctness basis: the serialized request length **grows monotonically** with the item + count (each item contributes at least one byte; VLQ-encoded counters never shrink as + items are added). This makes the search find exactly the same maximum as the original + linear scan. +- Preserved semantics: the **first item of a chunk is always included unconditionally** + (matching the legacy "never reject the first item" behavior), even if that item alone + would exceed the limit. +- Regression test added: `WriteBatchSplitPointMatchesLinearScanForVariousSizes` + (12 cases across power-of-two boundaries) proves identical chunk boundaries vs. the old + linear scan. + +### 2. Pool the probe MemoryStream (committed) + +- Commit `8851fed`. +- `GetSerializedRequestLengthForBatching` previously allocated a new `MemoryStream` + (and backing byte array) on **every** probe call. With galloping/binary search this can + still be thousands of allocations for large tag counts. +- Now reuses a `[ThreadStatic] MemoryStream` across calls, resetting it via + `SetLength(0)` instead of reallocating. +- Thread-safety rationale: a single batching operation runs **synchronously on one + thread**, and the helper is never called reentrantly, so `[ThreadStatic]` is safe + without locking. The session is never used concurrently for read and write. + +### 3. Optimistic fast path in the batching check (implemented) + +- Adds an optimistic path at the top of `AppendItemsWithinPayloadLimit`: add **all** + candidate items of a chunk at once and check the payload limit **once**. If it fits, + return immediately. +- Rationale: the PLC-negotiated item limit (`TagsPerReadRequestMax` / + `TagsPerWriteRequestMax`) already usually fits the negotiated PDU payload, so the whole + chunk typically fits in a single frame. +- Effect: reduces full-request serializations per chunk from **O(log n)** (item 1) to + **1** in the common case. When the optimistic attempt overflows the limit, the code + rolls back and falls back to the galloping/binary-search path from item 1, so behavior + stays identical. +- Covered by the existing split-point regression tests (which exercise the fallback). + +### 4. Conditionally skip the payload check + +Implemented as part of item 3: because the payload check is only performed when it can +actually bind (optimistic attempt fails), the expensive search path is skipped entirely +in the common case. + +## Verification + +- Build clean (0 warnings / 0 errors) for **net48** and **net8.0**. +- Full test suite: **468/468 passing** after each step, including the batch-split + regression tests in `S7CommPlusProtocolSessionReceiveTests`. +- Debug hooks used to drive the split logic from tests: e.g. + `DebugCreateWriteRequestBatchForTests` in `S7CommPlusProtocolSession.cs`. + +## Remaining backlog + +5. **Typed values instead of `object`.** The read path returns `List`, which + boxes value types. Switching to typed values would cut allocations further but is an + **API break** and was deferred. + +6. **Pipelining / parallel sessions.** Issuing multiple requests concurrently could + overlap I/O across the 2100 round-trips. Likely **not feasible** given protocol + constraints: fragmented S7CommPlus response bodies do not carry the request sequence + number, so concurrent fragment-producing requests make fragment ownership ambiguous + (see `online-multiplexing-todo.md`). One foreground request stays in flight per + physical connection. + +## Behavior preserved (invariants) + +- Hard-abort-on-error semantics unchanged; partial results still discarded at the client + layer via `ThrowIfError`. +- Chunk boundaries byte-identical to the original linear scan. +- First item of a chunk always included unconditionally. +- Serialized-length monotonicity is the core assumption behind the search-based + optimizations; if item serialization ever stopped being monotonic, the galloping/binary + search would need to be revisited. diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs index a03e74f..ad1f007 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs @@ -710,6 +710,26 @@ private static int AppendItemsWithinPayloadLimit( return 0; } + // Fast path: the PLC's item-count limit (maxCandidateCount, derived from + // TagsPerReadRequestMax/TagsPerWriteRequestMax) is already negotiated to fit within the + // transport's PDU size in the vast majority of deployments, so the whole candidate set + // typically fits in a single frame. Optimistically add everything and check once; this + // costs a single full serialization instead of the O(log n) probes of the galloping/binary + // search below, which is only needed on the (uncommon) path where the optimistic attempt + // overflows the payload limit. + for (var i = 0; i < maxCandidateCount; i++) + { + addItemAt(i); + } + if (!ExceedsSingleFramePayload(request, maxPayloadSize)) + { + return maxCandidateCount; + } + for (var i = 0; i < maxCandidateCount; i++) + { + removeLastItem(); + } + // The first item of a chunk is always included unconditionally, matching the legacy behavior. addItemAt(0); var added = 1; From ae840a286dca20b6c5bd90ca3b9b164b79d8ef56 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Wed, 23 Sep 2026 11:55:36 +0200 Subject: [PATCH 8/9] Refactoring: remove logs and add toString() to the class VarInfo --- src/S7CommPlusDriver/ClientApi/Browser.cs | 5 +++++ src/S7CommPlusDriver/S7CommPlusClient.cs | 4 ---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/S7CommPlusDriver/ClientApi/Browser.cs b/src/S7CommPlusDriver/ClientApi/Browser.cs index d111bbd..a1ae191 100644 --- a/src/S7CommPlusDriver/ClientApi/Browser.cs +++ b/src/S7CommPlusDriver/ClientApi/Browser.cs @@ -955,6 +955,11 @@ public class VarInfo /// without the CRC of the containing array declaration because the PLC rejects that CRC/address combination. /// internal bool ContainsIndexedArray; + + public override string ToString() + { + return Name; + } } internal enum eNodeType diff --git a/src/S7CommPlusDriver/S7CommPlusClient.cs b/src/S7CommPlusDriver/S7CommPlusClient.cs index ab01fd2..2858c42 100644 --- a/src/S7CommPlusDriver/S7CommPlusClient.cs +++ b/src/S7CommPlusDriver/S7CommPlusClient.cs @@ -1326,11 +1326,7 @@ public Task> ReadAsync(IEnumerabl return ExecuteReadOperationAsync("ReadTags", session => { var requestTags = ExpandAggregateTags(tagList); - _options.Logger.LogInformation("ReadTagValuesInBatches start..."); - var sw = Stopwatch.StartNew(); var (values, itemErrors) = ReadTagValuesInBatches(session, requestTags); - sw.Stop(); - _options.Logger.LogInformation($"ReadTagValuesInBatches finished: {sw.ElapsedMilliseconds}ms"); var items = new List(tagList.Count); var requestIndex = 0; foreach (var tag in tagList) From f221eb9e9ae06a9f9a3dc6cba024e9d5c6ff476c Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Mon, 28 Sep 2026 08:36:24 +0200 Subject: [PATCH 9/9] Resolve struct/UDT members by symbol and expose declared array shape Restore the legacy driver behavior in browsePlcTagBySymbol: a relation-bearing member (struct/UDT instance, struct array, FB instance or DTL) resolves to a tag once the symbolic path is exhausted instead of returning null. The previous port restricted that branch to DTL, so GetTagBySymbolAsync threw for struct symbols and GetTagsBySymbolsAsync silently dropped them. Add the missing PlcTagStruct tag and the S7COMMP_SOFTDATATYPE_STRUCT case in TagFactory, which both had no counterpart in this repository, so the restored branch would still have produced null. Expose the PLC-declared array declaration through the new PlcTag.ArrayDimensions property (lower bound and element count per dimension, outermost first) and populate it from the low-level dimension metadata on the symbolic path and from VarInfo.ArrayDimensions on the browse path. Struct/UDT arrays report their shape this way instead of materializing one tag per element; single elements stay resolvable through their indexed symbol. Align RequiresArrayElementRelationSelector with HasRelation() so struct/UDT element addresses keep the literal .1 relation selector. Add PlcTagStructArrayTests covering the struct tag, the declared shape, the selective element expansion and the packed struct read/write behavior. --- .../PlcTagStructArrayTests.cs | 166 ++++++++++++++++++ src/S7CommPlusDriver/ClientApi/PlcTag.cs | 73 ++++++++ src/S7CommPlusDriver/ClientApi/PlcTags.cs | 4 + .../S7CommPlusProtocolSession.cs | 97 +++++++++- 4 files changed, 334 insertions(+), 6 deletions(-) create mode 100644 src/S7CommPlusDriver.Tests/PlcTagStructArrayTests.cs diff --git a/src/S7CommPlusDriver.Tests/PlcTagStructArrayTests.cs b/src/S7CommPlusDriver.Tests/PlcTagStructArrayTests.cs new file mode 100644 index 0000000..a385ce2 --- /dev/null +++ b/src/S7CommPlusDriver.Tests/PlcTagStructArrayTests.cs @@ -0,0 +1,166 @@ +using System.Linq; +using S7CommPlusDriver.ClientApi; +using Xunit; + +namespace S7CommPlusDriver.Tests +{ + /// + /// Verifies that a structural PLC member (UDT/STRUCT instance or struct array) resolves to a tag instead of + /// and that a struct array reports its PLC-declared shape through + /// without materializing one tag per element. + /// + /// + /// Regression test for a deviation ported into S7CommPlusProtocolSession.browsePlcTagBySymbol: the legacy + /// driver returned a tag for every relation-bearing member once the symbol path was exhausted, while the new driver + /// returned for everything except DTL and PlcTags.TagFactory had no + /// S7COMMP_SOFTDATATYPE_STRUCT case at all. Struct arrays are resolved through + /// because the legacy placeholder tag carried the array declaration as well. + /// + public sealed class PlcTagStructArrayTests + { + [Fact] + public void CreateResolvedPlcTagReturnsStructTagWithDeclaredArrayShape() + { + var varInfo = new VarInfo + { + Name = "fb_CaseVariables_DB.l_GML_Struct", + AccessSequence = "8A0E0001.F", + SymbolCrc = 0x10203040, + Softdatatype = Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT, + ArrayElementCount = 10000, + ArrayDimensions = new[] { new S7CommPlusArrayDimension(1, 10000) }, + }; + + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo); + + var structTag = Assert.IsType(tag); + var dimension = Assert.Single(structTag.ArrayDimensions); + Assert.Equal(1, dimension.LowerBound); + Assert.Equal(10000u, dimension.ElementCount); + // Struct elements are structures, not scalar wire values, so no element tag is materialized. + Assert.Empty(structTag.AggregateElements); + } + + [Fact] + public void CreateResolvedPlcTagReturnsStructTagWithAllDeclaredDimensions() + { + var varInfo = new VarInfo + { + Name = "DB.Matrix", + AccessSequence = "8A0E0001.F", + SymbolCrc = 0x20304050, + Softdatatype = Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT, + ArrayElementCount = 4, + ArrayDimensions = new[] + { + new S7CommPlusArrayDimension(1, 2), + new S7CommPlusArrayDimension(5, 2), + }, + }; + + var tag = Assert.IsType(S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo)); + + Assert.Collection( + tag.ArrayDimensions, + dimension => + { + Assert.Equal(1, dimension.LowerBound); + Assert.Equal(2u, dimension.ElementCount); + }, + dimension => + { + Assert.Equal(5, dimension.LowerBound); + Assert.Equal(2u, dimension.ElementCount); + }); + Assert.Empty(tag.AggregateElements); + } + + [Fact] + public void CreateResolvedPlcTagReturnsStructTagWithoutShapeForScalarStructMember() + { + var varInfo = new VarInfo + { + Name = "DB.Settings", + AccessSequence = "8A0E0001.F", + SymbolCrc = 0x30405060, + Softdatatype = Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT, + }; + + var tag = Assert.IsType(S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo)); + + Assert.Empty(tag.ArrayDimensions); + Assert.Empty(tag.AggregateElements); + } + + [Fact] + public void CreateResolvedPlcTagKeepsElementExpansionAndShapeForPrimitiveArray() + { + var varInfo = new VarInfo + { + Name = "DB.Measurements", + AccessSequence = "8A0E0001.F", + SymbolCrc = 0x40506070, + Softdatatype = Softdatatype.S7COMMP_SOFTDATATYPE_REAL, + ArrayElementCount = 6, + ArrayDimensions = new[] + { + new S7CommPlusArrayDimension(1, 2), + new S7CommPlusArrayDimension(1, 3), + }, + }; + + var tag = S7CommPlusProtocolSession.CreateResolvedPlcTag(varInfo); + + Assert.NotNull(tag); + Assert.Equal(6, tag.AggregateElements.Count); + Assert.Equal(new[] { 2u, 3u }, tag.ArrayDimensions.Select(dimension => dimension.ElementCount).ToArray()); + Assert.Equal(new[] { 1, 1 }, tag.ArrayDimensions.Select(dimension => dimension.LowerBound).ToArray()); + } + + [Fact] + public void PlcTagStructProcessReadResultPublishesPackedStructIdentifier() + { + var tag = new PlcTagStruct("DB.Settings", new ItemAddress("8A0E0001.F.5"), Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT); + + tag.ProcessReadResult(new ValueStruct(0x0200005A), 0); + + Assert.Equal(0x0200005Au, tag.Value); + Assert.Equal(PlcTagQC.TAG_QUALITY_GOOD, tag.Quality); + Assert.Equal(0UL, tag.LastReadError); + } + + [Fact] + public void PlcTagStructProcessReadResultFlagsItemErrors() + { + var tag = new PlcTagStruct("DB.Settings", new ItemAddress("8A0E0001.F.5"), Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT); + + tag.ProcessReadResult(new ValueStruct(0x0200005A), 1); + + Assert.Equal(PlcTagQC.TAG_QUALITY_BAD, tag.Quality); + Assert.Equal(1UL, tag.LastReadError); + } + + [Fact] + public void PlcTagStructProcessReadResultFlagsUnexpectedValueType() + { + var tag = new PlcTagStruct("DB.Settings", new ItemAddress("8A0E0001.F.5"), Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT); + + tag.ProcessReadResult(new ValueReal(1.5f), 0); + + Assert.Equal(PlcTagQC.TAG_QUALITY_BAD, tag.Quality); + } + + [Fact] + public void PlcTagStructGetWriteValueWritesPackedStructIdentifier() + { + var tag = new PlcTagStruct("DB.Settings", new ItemAddress("8A0E0001.F.5"), Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT) + { + Value = 0x0200005A, + }; + + var writeValue = Assert.IsType(tag.GetWriteValue()); + + Assert.Equal(0x0200005Au, writeValue.GetValue()); + } + } +} \ No newline at end of file diff --git a/src/S7CommPlusDriver/ClientApi/PlcTag.cs b/src/S7CommPlusDriver/ClientApi/PlcTag.cs index 986e7ec..8450be3 100644 --- a/src/S7CommPlusDriver/ClientApi/PlcTag.cs +++ b/src/S7CommPlusDriver/ClientApi/PlcTag.cs @@ -19,6 +19,7 @@ public abstract class PlcTag { private IReadOnlyList m_AggregateElements = Array.Empty(); private IReadOnlyList m_AggregateDimensions = Array.Empty(); + private IReadOnlyList m_ArrayDimensions = Array.Empty(); public string Name; public ItemAddress Address; @@ -48,6 +49,18 @@ public abstract class PlcTag /// public object AggregateValue { get; private set; } + /// + /// Gets the PLC-declared array declaration of the member this tag was resolved from, in declaration order + /// (outermost dimension first), or an empty list for scalar members. + /// + /// + /// Every entry carries the lower bound and element count of one declared dimension exactly as written in the PLC + /// program, so Array[1..10000] of MyUdt reports one entry with lower bound 1 and element count + /// 10000. Struct/UDT arrays report their declaration through this property instead of exposing thousands of + /// individual element tags; single elements are resolved on demand through their indexed symbol. + /// + public IReadOnlyList ArrayDimensions => m_ArrayDimensions; + public PlcTag(string name, ItemAddress address, uint softdatatype) { Name = name; @@ -152,6 +165,17 @@ internal void SetAggregateElements(IReadOnlyList elements, IReadOnlyList m_AggregateDimensions = dimensions != null && dimensions.Count > 1 ? dimensions : Array.Empty(); } + /// + /// Sets the PLC-declared array declaration this tag was resolved from, exposed through . + /// + /// + /// One entry per declared dimension in declaration order (outermost first), or for scalar members. + /// + internal void SetArrayDimensions(IReadOnlyList dimensions) + { + m_ArrayDimensions = dimensions ?? Array.Empty(); + } + /// /// Publishes a completed set of scalar element reads as one aggregate value and mirrors it into a compatible typed array tag. /// A multidimensional aggregate array ( has more than one entry) is reshaped into a @@ -1976,6 +2000,55 @@ public override string ToString() } } + /// + /// Structural PLC tag for relation-bearing members (UDT/STRUCT instances and their array elements) that are + /// resolved as a whole symbol without a remaining member path. + /// + /// + /// Mirrors the legacy driver's PlcTagStruct: the concrete value is the packed struct identifier reported by the + /// PLC, while the declared shape of a struct array is exposed through and the + /// individual members are read through their own member tags. Unlike this tag does not decode + /// packed system values, because a user-defined structure has no fixed wire layout. + /// + public class PlcTagStruct : PlcTag + { + private uint m_Value; + + /// Gets or sets the packed struct identifier transmitted by the PLC for this structural member. + public uint Value + { + get { return m_Value; } + set { m_Value = value; } + } + + public PlcTagStruct(string name, ItemAddress address, uint softdatatype) : base(name, address, softdatatype) { } + + public override void ProcessReadResult(object valueObj, ulong error) + { + LastReadError = error; + if (CheckErrorAndType(error, valueObj, typeof(ValueStruct)) == 0) + { + Value = ((ValueStruct)valueObj).GetValue(); + + Quality = PlcTagQC.TAG_QUALITY_GOOD; + } + else + { + Quality = PlcTagQC.TAG_QUALITY_BAD; + } + } + + internal override PValue GetWriteValue() + { + return new ValueStruct(Value); + } + + public override string ToString() + { + return ResultString(this, Value.ToString()); + } + } + #region Arrays public class PlcTagBoolArray : PlcTag diff --git a/src/S7CommPlusDriver/ClientApi/PlcTags.cs b/src/S7CommPlusDriver/ClientApi/PlcTags.cs index afd20a1..36fea8f 100644 --- a/src/S7CommPlusDriver/ClientApi/PlcTags.cs +++ b/src/S7CommPlusDriver/ClientApi/PlcTags.cs @@ -150,6 +150,10 @@ public static PlcTag TagFactory(string name, ItemAddress address, uint softdatat return new PlcTagUInt(name, address, softdatatype); case Softdatatype.S7COMMP_SOFTDATATYPE_BLOCKFC: return new PlcTagUInt(name, address, softdatatype); + // Struct/UDT instances (and their array roots) have no scalar wire value; the tag reports the packed + // struct identifier and, for arrays, the PLC-declared shape through PlcTag.ArrayDimensions. + case Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT: + return new PlcTagStruct(name, address, softdatatype); case Softdatatype.S7COMMP_SOFTDATATYPE_COUNTER: return new PlcTagUInt(name, address, softdatatype); diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs index ad1f007..468da78 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs @@ -2125,13 +2125,12 @@ private PlcTag browsePlcTagBySymbol(uint ti_relid, ref string symbol, VarInfo va } if (varType.OffsetInfoType.HasRelation()) { - if (symbol.Length <= 0 && varType.Softdatatype == Softdatatype.S7COMMP_SOFTDATATYPE_DTL) - { - return CreateResolvedPlcTag(varInfo, varType, isAggregateArray); - } if (symbol.Length <= 0) { - return null; + // Relation-bearing members (struct/UDT instances, their arrays, FB instances and DTL) resolve to a tag + // instead of being reported as "not found" once the symbol path is exhausted, matching the legacy + // driver's browsePlcTagBySymbol(). Struct arrays report their declared shape through PlcTag.ArrayDimensions. + return CreateResolvedPlcTag(varInfo, varType, isAggregateArray); } else { @@ -2204,6 +2203,15 @@ private static PlcTag CreateResolvedPlcTag(VarInfo varInfo, PVartypeListElement return tag; } + // The caller requested the complete array, so the tag always reports the PLC-declared shape. Struct/UDT arrays + // are deliberately not expanded into element tags: their elements are individually addressable structures rather + // than scalar wire values, and a large declaration would otherwise materialize thousands of synthetic addresses. + tag.SetArrayDimensions(GetArrayDeclaration(varType)); + if (!SupportsAggregateElementExpansion(varType.Softdatatype)) + { + return tag; + } + var elementTags = GetAggregateArrayElementAccessIds(varType) .Select(accessId => { @@ -2239,6 +2247,67 @@ private static IReadOnlyList GetAggregateArrayDimensions(PVartypeListEleme .ToArray(); } + /// + /// Builds the PLC-declared array declaration of one member from its low-level dimension metadata, in declaration + /// order (outermost dimension first), for . + /// + /// The resolved member metadata containing the dimension information. + /// One entry per declared dimension, or an empty list for scalar members. + private static IReadOnlyList GetArrayDeclaration(PVartypeListElement varType) + { + if (varType.OffsetInfoType is IOffsetInfoType_MDim multiDimensional) + { + // The protocol stores both the counts and the lower bounds inside-out, so reverse them into declaration order. + var counts = multiDimensional.GetMdimArrayElementCount().TakeWhile(count => count > 0).ToArray(); + var lowerBounds = multiDimensional.GetMdimArrayLowerBounds(); + var declaration = new S7CommPlusArrayDimension[counts.Length]; + for (var dimension = 0; dimension < counts.Length; dimension++) + { + var protocolIndex = counts.Length - dimension - 1; + declaration[dimension] = new S7CommPlusArrayDimension(lowerBounds[protocolIndex], counts[protocolIndex]); + } + return declaration; + } + + if (varType.OffsetInfoType is IOffsetInfoType_1Dim oneDimensional) + { + var elementCount = oneDimensional.GetArrayElementCount(); + if (elementCount == 0) + { + return Array.Empty(); + } + return new[] { new S7CommPlusArrayDimension(oneDimensional.GetArrayLowerBounds(), elementCount) }; + } + + return Array.Empty(); + } + + /// + /// Determines whether an aggregate array is expanded into individually addressable element tags. + /// + /// The Siemens soft-datatype of the array member. + /// + /// for members whose elements are scalar wire values; for structural + /// members, whose tag reports only the declared array shape (see ). + /// + private static bool SupportsAggregateElementExpansion(uint softdatatype) + { + return !IsStructLikeSoftdatatype(softdatatype) && softdatatype != Softdatatype.S7COMMP_SOFTDATATYPE_FOLDER; + } + + /// + /// Determines whether a soft-datatype describes a user-defined structure or a block instance, which is transferred as + /// a packed instead of a scalar value. + /// + /// The Siemens soft-datatype identifier. + private static bool IsStructLikeSoftdatatype(uint softdatatype) + { + return softdatatype == Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT + || softdatatype == Softdatatype.S7COMMP_SOFTDATATYPE_BLOCKFB + || softdatatype == Softdatatype.S7COMMP_SOFTDATATYPE_BLOCKFC + || softdatatype == Softdatatype.S7COMMP_SOFTDATATYPE_BLOCKUDT; + } + /// /// Creates a typed tag directly from one aggregate browse result, avoiding a second walk through the PLC type catalog. /// @@ -2263,6 +2332,14 @@ internal static PlcTag CreateResolvedPlcTag(VarInfo varInfo) return tag; } + // Browse results already carry the caller-facing array declaration, so it can be attached without any conversion. + // Struct/UDT arrays keep that declaration only and are not expanded into per-element tags, see the symbol overload. + tag.SetArrayDimensions(varInfo.ArrayDimensions); + if (!SupportsAggregateElementExpansion(varInfo.Softdatatype)) + { + return tag; + } + var elementTags = GetAggregateArrayElementAccessIds(varInfo) .Select(accessId => PlcTags.TagFactory( $"{varInfo.Name}[#{accessId}]", @@ -2395,9 +2472,17 @@ internal static ItemAddress CreateAggregateArrayElementAddress( return new ItemAddress($"{aggregateAccessSequence}.{accessId:X}{(requiresRelationSelector ? ".1" : string.Empty)}"); } + /// + /// Determines whether an aggregate array element address needs the protocol's literal relation selector (".1"). + /// Relation-bearing array members (structs/UDTs, block instances and DTL) place that selector between the element + /// access ID and the member access ID, exactly like calcAccessSeqFor1DimArray does for symbolic access. + /// + /// The Siemens soft-datatype of the array member. + /// when element addresses require the additional relation selector. private static bool RequiresArrayElementRelationSelector(uint softdatatype) { - return softdatatype == Softdatatype.S7COMMP_SOFTDATATYPE_DTL; + return IsStructLikeSoftdatatype(softdatatype) + || softdatatype == Softdatatype.S7COMMP_SOFTDATATYPE_DTL; } ///