From 3c4a976307a2accad3f5a3c23b4a939b377dcc44 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Thu, 10 Sep 2026 12:53:53 +0200 Subject: [PATCH 01/21] 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 0c8b083..aa0b00d 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 4ce30590fe513347adb2c0de677882b5b91fad8e Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Fri, 11 Sep 2026 11:31:33 +0200 Subject: [PATCH 02/21] 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 dcde68357c39a0370aef4f9866bd3aee1127882d Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Mon, 14 Sep 2026 09:22:12 +0200 Subject: [PATCH 03/21] 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 aa0b00d..af74a25 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 2192f6d0e6bbab390c558c566d38e77a33ab3b0d Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Mon, 14 Sep 2026 13:06:10 +0200 Subject: [PATCH 04/21] 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 af74a25..b4edf8c 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 f0bccada88af92e2b2e04e19a64cf0fe2b0408bb Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Thu, 17 Sep 2026 10:39:23 +0200 Subject: [PATCH 05/21] 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 8851fedb70c755ddf3c1a1fe89ec4e9a2f00c502 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Thu, 17 Sep 2026 11:40:03 +0200 Subject: [PATCH 06/21] 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 5db3404bc4a8c8c58b5b90210f447ee4a523c457 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Tue, 22 Sep 2026 14:30:39 +0200 Subject: [PATCH 07/21] 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 710145e3dea948b7e276887a0eb7da2e98ed0e2b Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Wed, 23 Sep 2026 11:55:36 +0200 Subject: [PATCH 08/21] 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 5c6cc1eecad5098014aba0967c6e7d20d935d7db Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Mon, 28 Sep 2026 08:36:24 +0200 Subject: [PATCH 09/21] 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 b4edf8c..dd55671 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; } /// From cf1ef1735d1a58ed2804324185be8d58728f7cfd Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Wed, 30 Sep 2026 12:46:04 +0200 Subject: [PATCH 10/21] fix: keep negotiated read/write batch size across reconnects Reuse the SystemLimits negotiated during the connect handshake instead of resetting _tagsPerReadRequestMax/_tagsPerWriteRequestMax to the hard-coded default on every (re)connect via new GetCachedCommunicationResources. --- .../FakeS7CommPlusSession.cs | 9 +- .../S7CommPlusClientTests.cs | 135 ++++++++++++++++++ ...S7CommPlusCommunicationResourceSnapshot.cs | 14 ++ src/S7CommPlusDriver/IS7CommPlusSession.cs | 8 ++ src/S7CommPlusDriver/S7CommPlusClient.cs | 22 ++- .../S7CommPlusProtocolSession.Session.cs | 5 + .../S7CommPlusProtocolSession.cs | 10 ++ 7 files changed, 200 insertions(+), 3 deletions(-) diff --git a/src/S7CommPlusDriver.Tests/FakeS7CommPlusSession.cs b/src/S7CommPlusDriver.Tests/FakeS7CommPlusSession.cs index 695d96e..f54d8aa 100644 --- a/src/S7CommPlusDriver.Tests/FakeS7CommPlusSession.cs +++ b/src/S7CommPlusDriver.Tests/FakeS7CommPlusSession.cs @@ -76,6 +76,7 @@ internal sealed class FakeS7CommPlusSession : IS7CommPlusSession public Func<(int Error, S7CommPlusCpuCultureInfo CultureInfo)>? CpuCultureInfoHandler { get; set; } public Func, (int Error, S7CommPlusTextListCatalog TextLists)>? TextListsHandler { get; set; } public Func<(int Error, S7CommPlusCommunicationResourceSnapshot Resources)>? CommunicationResourcesHandler { get; set; } + public S7CommPlusCommunicationResourceSnapshot CachedCommunicationResources { get; set; } = new S7CommPlusCommunicationResourceSnapshot(); public Func, (int Error, List Values, List Errors)>? ReadHandler { get; set; } public Func, List, (int Error, List Errors)>? WriteHandler { get; set; } public Func, ushort, short, int>? CreateTagSubscriptionHandler { get; set; } @@ -254,11 +255,17 @@ public int GetTextLists(IEnumerable languageIds, out S7CommPlusTextListCata public int GetCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources) { - var result = CommunicationResourcesHandler?.Invoke() ?? (0, new S7CommPlusCommunicationResourceSnapshot()); + var result = CommunicationResourcesHandler?.Invoke() ?? (0, CachedCommunicationResources); resources = result.Resources; return result.Error; } + public int GetCachedCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources) + { + resources = CachedCommunicationResources; + return 0; + } + public int ReadValues(List addresslist, out List values, out List errors) { ReadCount++; diff --git a/src/S7CommPlusDriver.Tests/S7CommPlusClientTests.cs b/src/S7CommPlusDriver.Tests/S7CommPlusClientTests.cs index b10f502..e1567eb 100644 --- a/src/S7CommPlusDriver.Tests/S7CommPlusClientTests.cs +++ b/src/S7CommPlusDriver.Tests/S7CommPlusClientTests.cs @@ -3,6 +3,7 @@ using S7CommPlusDriver.Internal; using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.IO.Compression; using System.Linq; @@ -662,6 +663,140 @@ public async Task ReadReconnectsOnceAfterTransientDisconnect() Assert.True(result.Items[0].IsSuccess); } + [Fact] + public async Task ReconnectKeepsNegotiatedReadBatchSize() + { + const int negotiatedReadRequestMax = 25; + var observedBatchSizes = new List(); + var fake = new FakeS7CommPlusSession + { + CachedCommunicationResources = + new S7CommPlusCommunicationResourceSnapshot(negotiatedReadRequestMax, negotiatedReadRequestMax), + ReadHandler = addresses => + { + observedBatchSizes.Add(addresses.Count); + if (observedBatchSizes.Count == 1) + { + // Transient failure on the first attempt forces a reconnect and one retry. + return (S7Consts.errTCPDataReceive, new List(), new List()); + } + + return ( + 0, + addresses.Select(_ => (object?)new ValueInt(1)).ToList(), + new List(new ulong[addresses.Count])); + } + }; + var client = CreateClient(fake); + + // Must use the PlcTag overload: only that path batches by _tagsPerReadRequestMax, + // the ItemAddress overload sends all addresses in a single request. + var tags = Enumerable.Range(1, negotiatedReadRequestMax) + .Select(index => PlcTags.TagFactory( + $"DB.Value[{index}]", + new ItemAddress($"8A0E0001.{index:X}"), + Softdatatype.S7COMMP_SOFTDATATYPE_INT)) + .ToList(); + + var result = await client.ReadAsync(tags); + + Assert.True(result.Items.All(item => item.IsSuccess)); + // After the automatic reconnect the client must keep batching at the negotiated limit + // (one 25-item request per attempt), not fall back to the 20-item default which would + // produce a 20-item and a 5-item request. + Assert.Equal(new[] { negotiatedReadRequestMax, negotiatedReadRequestMax }, observedBatchSizes); + } + + [Fact] + public async Task ReconnectThenReadKeepsSteadyStateTiming() + { + // The negotiated limit is twice the hard-coded 20-item default; losing it on a reconnect + // doubles the number of read round trips and therefore the read duration. + const int negotiatedReadRequestMax = 40; + const int tagCount = 400; + const int readRequestLatencyMs = 25; + const int simulatedTimeoutMs = 250; + + var batches = new List(); + var failNextReadAttempt = false; + var fake = new FakeS7CommPlusSession + { + CachedCommunicationResources = + new S7CommPlusCommunicationResourceSnapshot(negotiatedReadRequestMax, negotiatedReadRequestMax), + ReadHandler = addresses => + { + batches.Add(addresses.Count); + + // The first attempt of the reconnect-carrying read simulates the transient timeout. + // That makes the driver reconnect and retry, and is intentionally slow. + if (failNextReadAttempt) + { + failNextReadAttempt = false; + Thread.Sleep(simulatedTimeoutMs); + return (S7Consts.errTCPDataReceive, new List(), new List()); + } + + Thread.Sleep(readRequestLatencyMs); + return ( + 0, + addresses.Select(_ => (object?)new ValueInt(1)).ToList(), + new List(new ulong[addresses.Count])); + } + }; + var client = CreateClient(fake, requestTimeoutMs: 5000); + + var tags = Enumerable.Range(1, tagCount) + .Select(index => PlcTags.TagFactory( + $"DB.Value[{index}]", + new ItemAddress($"8A0E0001.{index:X}"), + Softdatatype.S7COMMP_SOFTDATATYPE_INT)) + .ToList(); + + // Mirror the gateway: it explicitly negotiates the resources once after connecting. + // Without that step the baseline would already use the 20-item default and the reconnect + // regression would be invisible in the measured timings. + await client.GetCommunicationResourcesAsync(); + + // Baseline before the reconnect. + var baselineWatch = Stopwatch.StartNew(); + var baseline = await client.ReadAsync(tags); + baselineWatch.Stop(); + + // This call hits the simulated transient timeout, reconnects and retries. It is the + // "first call after the reconnect" and is expected to be slow, so it is only compared + // against steady state and never against the baseline. + failNextReadAttempt = true; + var reconnectWatch = Stopwatch.StartNew(); + var afterReconnect = await client.ReadAsync(tags); + reconnectWatch.Stop(); + + // Steady state after the reconnect. + var steadyWatch = Stopwatch.StartNew(); + var steady = await client.ReadAsync(tags); + steadyWatch.Stop(); + + Assert.True(baseline.Items.All(item => item.IsSuccess)); + Assert.True(afterReconnect.Items.All(item => item.IsSuccess)); + Assert.True(steady.Items.All(item => item.IsSuccess)); + + // The reconnect-carrying call must be clearly slower than steady state. + Assert.True( + reconnectWatch.ElapsedMilliseconds > steadyWatch.ElapsedMilliseconds, + $"Reconnect call ({reconnectWatch.ElapsedMilliseconds} ms) must be slower than steady state ({steadyWatch.ElapsedMilliseconds} ms)."); + + // Steady state must not roughly double compared to the baseline, i.e. the negotiated + // batch size must have survived the reconnect. + var steadyUpperBoundMs = baselineWatch.ElapsedMilliseconds * 1.5 + 40; + Assert.True( + steadyWatch.ElapsedMilliseconds <= steadyUpperBoundMs, + $"Steady-state read after reconnect took {steadyWatch.ElapsedMilliseconds} ms, baseline {baselineWatch.ElapsedMilliseconds} ms (limit {steadyUpperBoundMs:F0} ms). " + + $"Batches: [{string.Join(", ", batches)}]."); + + // Deterministic backing check independent of timing jitter: every request uses the + // negotiated limit instead of the 20-item default. + Assert.All(batches, batch => Assert.Equal(negotiatedReadRequestMax, batch)); + } + [Fact] public async Task ProtocolErrorDoesNotReconnect() { diff --git a/src/S7CommPlusDriver/Core/S7CommPlusCommunicationResourceSnapshot.cs b/src/S7CommPlusDriver/Core/S7CommPlusCommunicationResourceSnapshot.cs index cc33982..d3cb329 100644 --- a/src/S7CommPlusDriver/Core/S7CommPlusCommunicationResourceSnapshot.cs +++ b/src/S7CommPlusDriver/Core/S7CommPlusCommunicationResourceSnapshot.cs @@ -30,6 +30,20 @@ internal sealed class S7CommPlusCommunicationResourceSnapshot public int SubscriptionMemoryMax { get; private set; } public int SubscriptionMemoryFree { get; private set; } + public S7CommPlusCommunicationResourceSnapshot() + { + } + + /// + /// Creates a snapshot with explicit item limits, used by tests and internal callers that already + /// know the negotiated values. + /// + internal S7CommPlusCommunicationResourceSnapshot(int tagsPerReadRequestMax, int tagsPerWriteRequestMax) + { + TagsPerReadRequestMax = tagsPerReadRequestMax; + TagsPerWriteRequestMax = tagsPerWriteRequestMax; + } + public int ReadMax(S7CommPlusProtocolSession conn) { // Read SystemLimits diff --git a/src/S7CommPlusDriver/IS7CommPlusSession.cs b/src/S7CommPlusDriver/IS7CommPlusSession.cs index 9e8bddc..1366c4a 100644 --- a/src/S7CommPlusDriver/IS7CommPlusSession.cs +++ b/src/S7CommPlusDriver/IS7CommPlusSession.cs @@ -37,6 +37,14 @@ internal interface IS7CommPlusSession int GetCpuCultureInfo(out S7CommPlusCpuCultureInfo cultureInfo); int GetTextLists(IEnumerable languageIds, out S7CommPlusTextListCatalog textLists); int GetCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources); + + /// + /// Returns the communication resource limits that were negotiated during the connect handshake + /// without issuing additional PLC requests. Unlike , this + /// reflects the SystemLimits already read while establishing the session and is therefore safe to + /// call on every (re)connect. + /// + int GetCachedCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources); int GetActiveAlarms(out List alarmList, int languageId, Func textListResolver); int ReadValues(List addresses, out List values, out List errors); int WriteValues(List addresses, List values, out List errors); diff --git a/src/S7CommPlusDriver/S7CommPlusClient.cs b/src/S7CommPlusDriver/S7CommPlusClient.cs index 2858c42..1e8d2d1 100644 --- a/src/S7CommPlusDriver/S7CommPlusClient.cs +++ b/src/S7CommPlusDriver/S7CommPlusClient.cs @@ -1745,11 +1745,29 @@ private async Task ConnectCoreAsync(S7CommPlusConnectionState connectingState, C } SetState(S7CommPlusConnectionState.Connected); - _tagsPerReadRequestMax = DefaultTagsPerRequest; - _tagsPerWriteRequestMax = DefaultTagsPerRequest; + ApplyNegotiatedCommunicationResourceLimits(); _options.Logger.LogInformation("Connected to PLC {Endpoint}.", Endpoint); } + /// + /// Applies the communication resource limits that the protocol session negotiated during the connect + /// handshake instead of resetting them to the hard-coded defaults. The session reads the PLC + /// SystemLimits as part of connecting, so this keeps read/write batching at the real PLC limit even + /// across automatic reconnects, where the application does not get a chance to re-query the resources. + /// + private void ApplyNegotiatedCommunicationResourceLimits() + { + if (_session == null || _session.GetCachedCommunicationResources(out var resources) != 0) + { + _tagsPerReadRequestMax = DefaultTagsPerRequest; + _tagsPerWriteRequestMax = DefaultTagsPerRequest; + return; + } + + _tagsPerReadRequestMax = Math.Max(1, resources.TagsPerReadRequestMax); + _tagsPerWriteRequestMax = Math.Max(1, resources.TagsPerWriteRequestMax); + } + private async Task ReconnectCoreAsync(CancellationToken cancellationToken) { SetState(S7CommPlusConnectionState.Reconnecting); diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.Session.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.Session.cs index a2d3143..d762bd6 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.Session.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.Session.cs @@ -160,6 +160,11 @@ int IS7CommPlusSession.GetCommunicationResources(out S7CommPlusCommunicationReso return GetCommunicationResources(out resources); } + int IS7CommPlusSession.GetCachedCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources) + { + return GetCachedCommunicationResources(out resources); + } + int IS7CommPlusSession.GetActiveAlarms(out List alarmList, int languageId, Func textListResolver) { return AlarmBrowser.GetActiveAlarms(out alarmList, languageId, textListResolver); diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs index 468da78..7c43ea4 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs @@ -1371,6 +1371,16 @@ public int GetCommunicationResources(out S7CommPlusCommunicationResourceSnapshot return resources.ReadFree(this); } + /// + /// Returns the communication resource limits negotiated during connection setup (SystemLimits are + /// read as part of the connect handshake) without issuing additional PLC requests. + /// + public int GetCachedCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources) + { + resources = m_CommunicationResources ?? new S7CommPlusCommunicationResourceSnapshot(); + return 0; + } + public void Disconnect() { TryDisconnect(m_ReadTimeout); From 45a5e18dd1d9ed2124ddf596ea4ae175713f4751 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Thu, 10 Sep 2026 12:53:53 +0200 Subject: [PATCH 11/21] 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 819224a83c4a583f81dd5d6af69b30cebc30ce4c Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Fri, 11 Sep 2026 11:31:33 +0200 Subject: [PATCH 12/21] 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 2cd98d064d57c283d0c3ad407f1dc3ae8d908683 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Mon, 14 Sep 2026 09:22:12 +0200 Subject: [PATCH 13/21] 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 7493ae638be78cfddedf1449310513123232fcc9 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Mon, 14 Sep 2026 13:06:10 +0200 Subject: [PATCH 14/21] 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 0ee4b9bd599f4effa70b0f29523aecb316542561 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Thu, 17 Sep 2026 10:39:23 +0200 Subject: [PATCH 15/21] 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 b33e38846e28f41261a9301d7b91d91f5b31cbed Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Thu, 17 Sep 2026 11:40:03 +0200 Subject: [PATCH 16/21] 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 092a9ff5c2e203f3eb54f8204dc33e485cea6674 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Tue, 22 Sep 2026 14:30:39 +0200 Subject: [PATCH 17/21] 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 df3adcc65f3bced3ff621c256ba9603fbddb96b7 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Wed, 23 Sep 2026 11:55:36 +0200 Subject: [PATCH 18/21] 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 dad92d7b46da96b51808771169f903236c45379e Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Mon, 28 Sep 2026 08:36:24 +0200 Subject: [PATCH 19/21] 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; } /// From 1f7cd00bacfb5ba0b869354455312b29cded1478 Mon Sep 17 00:00:00 2001 From: Gerald Pohn Date: Wed, 30 Sep 2026 12:46:04 +0200 Subject: [PATCH 20/21] fix: keep negotiated read/write batch size across reconnects Reuse the SystemLimits negotiated during the connect handshake instead of resetting _tagsPerReadRequestMax/_tagsPerWriteRequestMax to the hard-coded default on every (re)connect via new GetCachedCommunicationResources. --- .../FakeS7CommPlusSession.cs | 9 +- .../S7CommPlusClientTests.cs | 135 ++++++++++++++++++ ...S7CommPlusCommunicationResourceSnapshot.cs | 14 ++ src/S7CommPlusDriver/IS7CommPlusSession.cs | 8 ++ src/S7CommPlusDriver/S7CommPlusClient.cs | 22 ++- .../S7CommPlusProtocolSession.Session.cs | 5 + .../S7CommPlusProtocolSession.cs | 10 ++ 7 files changed, 200 insertions(+), 3 deletions(-) diff --git a/src/S7CommPlusDriver.Tests/FakeS7CommPlusSession.cs b/src/S7CommPlusDriver.Tests/FakeS7CommPlusSession.cs index 695d96e..f54d8aa 100644 --- a/src/S7CommPlusDriver.Tests/FakeS7CommPlusSession.cs +++ b/src/S7CommPlusDriver.Tests/FakeS7CommPlusSession.cs @@ -76,6 +76,7 @@ internal sealed class FakeS7CommPlusSession : IS7CommPlusSession public Func<(int Error, S7CommPlusCpuCultureInfo CultureInfo)>? CpuCultureInfoHandler { get; set; } public Func, (int Error, S7CommPlusTextListCatalog TextLists)>? TextListsHandler { get; set; } public Func<(int Error, S7CommPlusCommunicationResourceSnapshot Resources)>? CommunicationResourcesHandler { get; set; } + public S7CommPlusCommunicationResourceSnapshot CachedCommunicationResources { get; set; } = new S7CommPlusCommunicationResourceSnapshot(); public Func, (int Error, List Values, List Errors)>? ReadHandler { get; set; } public Func, List, (int Error, List Errors)>? WriteHandler { get; set; } public Func, ushort, short, int>? CreateTagSubscriptionHandler { get; set; } @@ -254,11 +255,17 @@ public int GetTextLists(IEnumerable languageIds, out S7CommPlusTextListCata public int GetCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources) { - var result = CommunicationResourcesHandler?.Invoke() ?? (0, new S7CommPlusCommunicationResourceSnapshot()); + var result = CommunicationResourcesHandler?.Invoke() ?? (0, CachedCommunicationResources); resources = result.Resources; return result.Error; } + public int GetCachedCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources) + { + resources = CachedCommunicationResources; + return 0; + } + public int ReadValues(List addresslist, out List values, out List errors) { ReadCount++; diff --git a/src/S7CommPlusDriver.Tests/S7CommPlusClientTests.cs b/src/S7CommPlusDriver.Tests/S7CommPlusClientTests.cs index da43374..ea27edf 100644 --- a/src/S7CommPlusDriver.Tests/S7CommPlusClientTests.cs +++ b/src/S7CommPlusDriver.Tests/S7CommPlusClientTests.cs @@ -3,6 +3,7 @@ using S7CommPlusDriver.Internal; using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.IO.Compression; using System.Linq; @@ -712,6 +713,140 @@ public async Task ReadReconnectsOnceAfterTransientDisconnect() Assert.True(result.Items[0].IsSuccess); } + [Fact] + public async Task ReconnectKeepsNegotiatedReadBatchSize() + { + const int negotiatedReadRequestMax = 25; + var observedBatchSizes = new List(); + var fake = new FakeS7CommPlusSession + { + CachedCommunicationResources = + new S7CommPlusCommunicationResourceSnapshot(negotiatedReadRequestMax, negotiatedReadRequestMax), + ReadHandler = addresses => + { + observedBatchSizes.Add(addresses.Count); + if (observedBatchSizes.Count == 1) + { + // Transient failure on the first attempt forces a reconnect and one retry. + return (S7Consts.errTCPDataReceive, new List(), new List()); + } + + return ( + 0, + addresses.Select(_ => (object?)new ValueInt(1)).ToList(), + new List(new ulong[addresses.Count])); + } + }; + var client = CreateClient(fake); + + // Must use the PlcTag overload: only that path batches by _tagsPerReadRequestMax, + // the ItemAddress overload sends all addresses in a single request. + var tags = Enumerable.Range(1, negotiatedReadRequestMax) + .Select(index => PlcTags.TagFactory( + $"DB.Value[{index}]", + new ItemAddress($"8A0E0001.{index:X}"), + Softdatatype.S7COMMP_SOFTDATATYPE_INT)) + .ToList(); + + var result = await client.ReadAsync(tags); + + Assert.True(result.Items.All(item => item.IsSuccess)); + // After the automatic reconnect the client must keep batching at the negotiated limit + // (one 25-item request per attempt), not fall back to the 20-item default which would + // produce a 20-item and a 5-item request. + Assert.Equal(new[] { negotiatedReadRequestMax, negotiatedReadRequestMax }, observedBatchSizes); + } + + [Fact] + public async Task ReconnectThenReadKeepsSteadyStateTiming() + { + // The negotiated limit is twice the hard-coded 20-item default; losing it on a reconnect + // doubles the number of read round trips and therefore the read duration. + const int negotiatedReadRequestMax = 40; + const int tagCount = 400; + const int readRequestLatencyMs = 25; + const int simulatedTimeoutMs = 250; + + var batches = new List(); + var failNextReadAttempt = false; + var fake = new FakeS7CommPlusSession + { + CachedCommunicationResources = + new S7CommPlusCommunicationResourceSnapshot(negotiatedReadRequestMax, negotiatedReadRequestMax), + ReadHandler = addresses => + { + batches.Add(addresses.Count); + + // The first attempt of the reconnect-carrying read simulates the transient timeout. + // That makes the driver reconnect and retry, and is intentionally slow. + if (failNextReadAttempt) + { + failNextReadAttempt = false; + Thread.Sleep(simulatedTimeoutMs); + return (S7Consts.errTCPDataReceive, new List(), new List()); + } + + Thread.Sleep(readRequestLatencyMs); + return ( + 0, + addresses.Select(_ => (object?)new ValueInt(1)).ToList(), + new List(new ulong[addresses.Count])); + } + }; + var client = CreateClient(fake, requestTimeoutMs: 5000); + + var tags = Enumerable.Range(1, tagCount) + .Select(index => PlcTags.TagFactory( + $"DB.Value[{index}]", + new ItemAddress($"8A0E0001.{index:X}"), + Softdatatype.S7COMMP_SOFTDATATYPE_INT)) + .ToList(); + + // Mirror the gateway: it explicitly negotiates the resources once after connecting. + // Without that step the baseline would already use the 20-item default and the reconnect + // regression would be invisible in the measured timings. + await client.GetCommunicationResourcesAsync(); + + // Baseline before the reconnect. + var baselineWatch = Stopwatch.StartNew(); + var baseline = await client.ReadAsync(tags); + baselineWatch.Stop(); + + // This call hits the simulated transient timeout, reconnects and retries. It is the + // "first call after the reconnect" and is expected to be slow, so it is only compared + // against steady state and never against the baseline. + failNextReadAttempt = true; + var reconnectWatch = Stopwatch.StartNew(); + var afterReconnect = await client.ReadAsync(tags); + reconnectWatch.Stop(); + + // Steady state after the reconnect. + var steadyWatch = Stopwatch.StartNew(); + var steady = await client.ReadAsync(tags); + steadyWatch.Stop(); + + Assert.True(baseline.Items.All(item => item.IsSuccess)); + Assert.True(afterReconnect.Items.All(item => item.IsSuccess)); + Assert.True(steady.Items.All(item => item.IsSuccess)); + + // The reconnect-carrying call must be clearly slower than steady state. + Assert.True( + reconnectWatch.ElapsedMilliseconds > steadyWatch.ElapsedMilliseconds, + $"Reconnect call ({reconnectWatch.ElapsedMilliseconds} ms) must be slower than steady state ({steadyWatch.ElapsedMilliseconds} ms)."); + + // Steady state must not roughly double compared to the baseline, i.e. the negotiated + // batch size must have survived the reconnect. + var steadyUpperBoundMs = baselineWatch.ElapsedMilliseconds * 1.5 + 40; + Assert.True( + steadyWatch.ElapsedMilliseconds <= steadyUpperBoundMs, + $"Steady-state read after reconnect took {steadyWatch.ElapsedMilliseconds} ms, baseline {baselineWatch.ElapsedMilliseconds} ms (limit {steadyUpperBoundMs:F0} ms). " + + $"Batches: [{string.Join(", ", batches)}]."); + + // Deterministic backing check independent of timing jitter: every request uses the + // negotiated limit instead of the 20-item default. + Assert.All(batches, batch => Assert.Equal(negotiatedReadRequestMax, batch)); + } + [Fact] public async Task ProtocolErrorDoesNotReconnect() { diff --git a/src/S7CommPlusDriver/Core/S7CommPlusCommunicationResourceSnapshot.cs b/src/S7CommPlusDriver/Core/S7CommPlusCommunicationResourceSnapshot.cs index cc33982..d3cb329 100644 --- a/src/S7CommPlusDriver/Core/S7CommPlusCommunicationResourceSnapshot.cs +++ b/src/S7CommPlusDriver/Core/S7CommPlusCommunicationResourceSnapshot.cs @@ -30,6 +30,20 @@ internal sealed class S7CommPlusCommunicationResourceSnapshot public int SubscriptionMemoryMax { get; private set; } public int SubscriptionMemoryFree { get; private set; } + public S7CommPlusCommunicationResourceSnapshot() + { + } + + /// + /// Creates a snapshot with explicit item limits, used by tests and internal callers that already + /// know the negotiated values. + /// + internal S7CommPlusCommunicationResourceSnapshot(int tagsPerReadRequestMax, int tagsPerWriteRequestMax) + { + TagsPerReadRequestMax = tagsPerReadRequestMax; + TagsPerWriteRequestMax = tagsPerWriteRequestMax; + } + public int ReadMax(S7CommPlusProtocolSession conn) { // Read SystemLimits diff --git a/src/S7CommPlusDriver/IS7CommPlusSession.cs b/src/S7CommPlusDriver/IS7CommPlusSession.cs index 9e8bddc..1366c4a 100644 --- a/src/S7CommPlusDriver/IS7CommPlusSession.cs +++ b/src/S7CommPlusDriver/IS7CommPlusSession.cs @@ -37,6 +37,14 @@ internal interface IS7CommPlusSession int GetCpuCultureInfo(out S7CommPlusCpuCultureInfo cultureInfo); int GetTextLists(IEnumerable languageIds, out S7CommPlusTextListCatalog textLists); int GetCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources); + + /// + /// Returns the communication resource limits that were negotiated during the connect handshake + /// without issuing additional PLC requests. Unlike , this + /// reflects the SystemLimits already read while establishing the session and is therefore safe to + /// call on every (re)connect. + /// + int GetCachedCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources); int GetActiveAlarms(out List alarmList, int languageId, Func textListResolver); int ReadValues(List addresses, out List values, out List errors); int WriteValues(List addresses, List values, out List errors); diff --git a/src/S7CommPlusDriver/S7CommPlusClient.cs b/src/S7CommPlusDriver/S7CommPlusClient.cs index 2858c42..1e8d2d1 100644 --- a/src/S7CommPlusDriver/S7CommPlusClient.cs +++ b/src/S7CommPlusDriver/S7CommPlusClient.cs @@ -1745,11 +1745,29 @@ private async Task ConnectCoreAsync(S7CommPlusConnectionState connectingState, C } SetState(S7CommPlusConnectionState.Connected); - _tagsPerReadRequestMax = DefaultTagsPerRequest; - _tagsPerWriteRequestMax = DefaultTagsPerRequest; + ApplyNegotiatedCommunicationResourceLimits(); _options.Logger.LogInformation("Connected to PLC {Endpoint}.", Endpoint); } + /// + /// Applies the communication resource limits that the protocol session negotiated during the connect + /// handshake instead of resetting them to the hard-coded defaults. The session reads the PLC + /// SystemLimits as part of connecting, so this keeps read/write batching at the real PLC limit even + /// across automatic reconnects, where the application does not get a chance to re-query the resources. + /// + private void ApplyNegotiatedCommunicationResourceLimits() + { + if (_session == null || _session.GetCachedCommunicationResources(out var resources) != 0) + { + _tagsPerReadRequestMax = DefaultTagsPerRequest; + _tagsPerWriteRequestMax = DefaultTagsPerRequest; + return; + } + + _tagsPerReadRequestMax = Math.Max(1, resources.TagsPerReadRequestMax); + _tagsPerWriteRequestMax = Math.Max(1, resources.TagsPerWriteRequestMax); + } + private async Task ReconnectCoreAsync(CancellationToken cancellationToken) { SetState(S7CommPlusConnectionState.Reconnecting); diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.Session.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.Session.cs index a2d3143..d762bd6 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.Session.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.Session.cs @@ -160,6 +160,11 @@ int IS7CommPlusSession.GetCommunicationResources(out S7CommPlusCommunicationReso return GetCommunicationResources(out resources); } + int IS7CommPlusSession.GetCachedCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources) + { + return GetCachedCommunicationResources(out resources); + } + int IS7CommPlusSession.GetActiveAlarms(out List alarmList, int languageId, Func textListResolver) { return AlarmBrowser.GetActiveAlarms(out alarmList, languageId, textListResolver); diff --git a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs index 468da78..7c43ea4 100644 --- a/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs +++ b/src/S7CommPlusDriver/S7CommPlusProtocolSession.cs @@ -1371,6 +1371,16 @@ public int GetCommunicationResources(out S7CommPlusCommunicationResourceSnapshot return resources.ReadFree(this); } + /// + /// Returns the communication resource limits negotiated during connection setup (SystemLimits are + /// read as part of the connect handshake) without issuing additional PLC requests. + /// + public int GetCachedCommunicationResources(out S7CommPlusCommunicationResourceSnapshot resources) + { + resources = m_CommunicationResources ?? new S7CommPlusCommunicationResourceSnapshot(); + return 0; + } + public void Disconnect() { TryDisconnect(m_ReadTimeout); From 0ad021fccef2d70faeaab5276449f43546965b96 Mon Sep 17 00:00:00 2001 From: Stefan Starlinger Date: Mon, 5 Oct 2026 09:09:25 +0200 Subject: [PATCH 21/21] sign all projects with strong name --- src/DriverTest/DriverTest.csproj | 2 ++ .../S7CommPlusDriver.Tests.csproj | 2 ++ src/S7CommPlusDriver.snk | Bin 0 -> 596 bytes src/S7CommPlusDriver/Properties/AssemblyInfo.cs | 6 +++--- src/S7CommPlusDriver/S7CommPlusDriver.csproj | 14 +++++++------- .../S7CommPlusGUIBrowser.csproj | 4 +++- 6 files changed, 17 insertions(+), 11 deletions(-) create mode 100644 src/S7CommPlusDriver.snk diff --git a/src/DriverTest/DriverTest.csproj b/src/DriverTest/DriverTest.csproj index a337fa3..dbd0fc8 100644 --- a/src/DriverTest/DriverTest.csproj +++ b/src/DriverTest/DriverTest.csproj @@ -22,5 +22,7 @@ 0.1.0.0 0.1.0.0 x86 + True + ..\S7CommPlusDriver.snk \ No newline at end of file diff --git a/src/S7CommPlusDriver.Tests/S7CommPlusDriver.Tests.csproj b/src/S7CommPlusDriver.Tests/S7CommPlusDriver.Tests.csproj index 52fac51..1c6baa3 100644 --- a/src/S7CommPlusDriver.Tests/S7CommPlusDriver.Tests.csproj +++ b/src/S7CommPlusDriver.Tests/S7CommPlusDriver.Tests.csproj @@ -6,6 +6,8 @@ enable latest $(NoWarn);xUnit1051 + True + ..\S7CommPlusDriver.snk diff --git a/src/S7CommPlusDriver.snk b/src/S7CommPlusDriver.snk new file mode 100644 index 0000000000000000000000000000000000000000..abd0895a6ec5fc459ebbc76782ed10253622e61a GIT binary patch literal 596 zcmV-a0;~N80ssI2Bme+XQ$aES1ONa500971_yDtUK>X*wP{6w@0{9=KO7`WPOKZPR zv%pxha5Sca=BVNC%?#Cf<~F6;8;iA~6T;*Djl-^~EJ*kvc76cqyJ+pk=0WeuaA&{HolWp%c+@KVN3Fj)y6H%-4c zoeABxvSm?mV=hVMhkO~F1d@B84mRJ}xt9bsVA;@z*1wgWWbb21I89BU+0%1RsaQ&i zHq!6E$hrd;$SPAUZyi zBOFMeB|4ku}*l5M}odEO#l?K zt2ZsiU@^Bz0b%WcKALgr2zEI^isD+8X47OCj0B2#icc~`#vFV8++@wPS+?TVMnPi! iexqf%j;BRl>wKs%4WS^r(|wX*V7{GUZpE0Wfg!nA{~xFT literal 0 HcmV?d00001 diff --git a/src/S7CommPlusDriver/Properties/AssemblyInfo.cs b/src/S7CommPlusDriver/Properties/AssemblyInfo.cs index 9c30261..bbc8890 100644 --- a/src/S7CommPlusDriver/Properties/AssemblyInfo.cs +++ b/src/S7CommPlusDriver/Properties/AssemblyInfo.cs @@ -1,5 +1,5 @@ using System.Runtime.CompilerServices; -[assembly: InternalsVisibleTo("S7CommPlusDriver.Tests")] -[assembly: InternalsVisibleTo("DriverTest")] -[assembly: InternalsVisibleTo("S7CommPlusGUIBrowser")] +[assembly: InternalsVisibleTo("S7CommPlusDriver.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010055f800b37140fce7bf50c0bb2b02f81fa44af6e59c4b6bbf4fb3c058b37034a683e6a8e1efcd0cd579e636a5db262313ab556fd51a2b7cec8502ec1f7e96e807137a7e416a5311b9aea6b24eeba491b65b894dc87de28d65aedfcd00bad86527a1141502e7dbaf5c6d650da39ed0532f316575b6b4f052c64e3059091e374dbf")] +[assembly: InternalsVisibleTo("DriverTest, PublicKey=002400000480000094000000060200000024000052534131000400000100010055f800b37140fce7bf50c0bb2b02f81fa44af6e59c4b6bbf4fb3c058b37034a683e6a8e1efcd0cd579e636a5db262313ab556fd51a2b7cec8502ec1f7e96e807137a7e416a5311b9aea6b24eeba491b65b894dc87de28d65aedfcd00bad86527a1141502e7dbaf5c6d650da39ed0532f316575b6b4f052c64e3059091e374dbf")] +[assembly: InternalsVisibleTo("S7CommPlusGUIBrowser, PublicKey=002400000480000094000000060200000024000052534131000400000100010055f800b37140fce7bf50c0bb2b02f81fa44af6e59c4b6bbf4fb3c058b37034a683e6a8e1efcd0cd579e636a5db262313ab556fd51a2b7cec8502ec1f7e96e807137a7e416a5311b9aea6b24eeba491b65b894dc87de28d65aedfcd00bad86527a1141502e7dbaf5c6d650da39ed0532f316575b6b4f052c64e3059091e374dbf")] diff --git a/src/S7CommPlusDriver/S7CommPlusDriver.csproj b/src/S7CommPlusDriver/S7CommPlusDriver.csproj index 9326e25..1c85a6e 100644 --- a/src/S7CommPlusDriver/S7CommPlusDriver.csproj +++ b/src/S7CommPlusDriver/S7CommPlusDriver.csproj @@ -44,13 +44,11 @@ - - - 1.1.4 - - - 1.1.4 - + + + + + S7CommPlusDriver @@ -65,5 +63,7 @@ https://github.com/dotnetprojects/S7CommPlusDriver LGPL-3.0-or-later README.md + True + ..\S7CommPlusDriver.snk diff --git a/src/S7CommPlusGUIBrowser/S7CommPlusGUIBrowser.csproj b/src/S7CommPlusGUIBrowser/S7CommPlusGUIBrowser.csproj index bf88170..6febeac 100644 --- a/src/S7CommPlusGUIBrowser/S7CommPlusGUIBrowser.csproj +++ b/src/S7CommPlusGUIBrowser/S7CommPlusGUIBrowser.csproj @@ -6,7 +6,9 @@ true true true - false + false + True + ..\S7CommPlusDriver.snk 1701;1702;CA1416