Repository navigation
Feature/optimize 07 tags per read request max - #17
Merged
jogibear9988 merged 10 commits intoOct 7, 2026
Merged
jogibear9988 merged 10 commits into
jogibear9988 merged 10 commits into
Conversation
…r lists (GML-1187)
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.
…ING/WSTRING writes to fail
…ltidimensional CLR arrays
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.
…atching 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.
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.
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.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Array handling, binary compatibility, and TLS synchronization have unresolved correctness issues.
Review effort: Balanced
Findings: 3
Open (9)
What changed in this PR
This PR improves S7CommPlus read/write batching and expands tag metadata and array support.
Changes:
- Optimizes payload-size checks and preserves negotiated batching limits after reconnects.
- Adds string-length metadata, multidimensional values, and structural tags.
- Adds TLS queue locks, regression tests, and performance documentation.
| File | Description |
|---|---|
| src/S7CommPlusDriver/S7CommPlusTagAccessorCatalog.cs | Persists string lengths in catalog format v3. |
| src/S7CommPlusDriver/S7CommPlusProtocolSession.Session.cs | Exposes cached communication resources. |
| src/S7CommPlusDriver/S7CommPlusProtocolSession.cs | Optimizes batching and extends tag resolution. |
| src/S7CommPlusDriver/S7CommPlusClient.cs | Applies negotiated limits after connection. |
| src/S7CommPlusDriver/OpenSSL/OpenSSLConnector.cs | Locks pending TLS queues. |
| src/S7CommPlusDriver/IS7CommPlusSession.cs | Adds cached-resource access. |
| src/S7CommPlusDriver/Core/S7CommPlusCommunicationResourceSnapshot.cs | Supports explicit item limits. |
| src/S7CommPlusDriver/Core/POffsetInfoType.cs | Extracts string-length metadata. |
| src/S7CommPlusDriver/ClientApi/PlcTags.cs | Extends factory metadata and structural support. |
| src/S7CommPlusDriver/ClientApi/PlcTag.cs | Adds array shapes and multidimensional value handling. |
| src/S7CommPlusDriver/ClientApi/Browser.cs | Carries string lengths through browsing. |
| src/S7CommPlusDriver.Tests/S7CommPlusProtocolSessionReceiveTests.cs | Tests batching split boundaries. |
| src/S7CommPlusDriver.Tests/S7CommPlusClientTests.cs | Tests reconnect batching and timing. |
| src/S7CommPlusDriver.Tests/PlcTagStructArrayTests.cs | Tests structural tags and array declarations. |
| src/S7CommPlusDriver.Tests/PlcTagStringMaxLengthTests.cs | Tests declared string lengths. |
| src/S7CommPlusDriver.Tests/PlcTagMultiDimensionalArrayTests.cs | Tests multidimensional value conversion. |
| src/S7CommPlusDriver.Tests/FakeS7CommPlusSession.cs | Supplies cached resource limits. |
| docs/batch-read-write-performance-optimization.md | Documents batching optimizations and constraints. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+115
to
+116
| DistributeMultiDimensionalValue(multiDimValue); | ||
| AggregateValue = multiDimValue; |
| /// </param> | ||
| /// <returns>The matching concrete tag implementation, or <see langword="null"/> for an unsupported datatype.</returns> | ||
| 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) |
Comment on lines
+36
to
+37
| private readonly object m_readListLock = new object(); | ||
| private readonly object m_writeListLock = new object(); |
Comment on lines
+100
to
+101
| var valueProperty = GetType().GetProperty("Value", BindingFlags.Public | BindingFlags.Instance); | ||
| return valueProperty?.GetValue(this); |
Comment on lines
+119
to
+120
| var valueProperty = GetType().GetProperty("Value", BindingFlags.Public | BindingFlags.Instance); | ||
| valueProperty?.SetValue(this, value); |
| internal virtual void PrepareAggregateWrite() | ||
| { | ||
| if (m_AggregateElements.Count == 0) | ||
| if (m_AggregateElements.Count == 0 || m_AggregateDimensions.Count > 1) |
Comment on lines
+155
to
+156
| case Softdatatype.S7COMMP_SOFTDATATYPE_STRUCT: | ||
| return new PlcTagStruct(name, address, softdatatype); |
| /// Gets the maximum string length for a String/WString offset info, or 0 when not applicable. | ||
| /// </summary> | ||
| /// <returns>The maximum length in characters, or 0 for non-string offset info types.</returns> | ||
| public virtual int MaxStringLength() { return 0; } |
Comment on lines
+368
to
+369
| tag.AggregateElements.Select(FromTag).ToArray(), | ||
| tag.GetMaxStringLength()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contains the sources from branch feature/optimize-06