Repository navigation
Feature/optimize 06 struct by symbol browse fix - #16
Merged
jogibear9988 merged 9 commits intoOct 7, 2026
Merged
jogibear9988 merged 9 commits into
jogibear9988 merged 9 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.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Array metadata loss, multidimensional read/write errors, and incomplete TLS synchronization remain unresolved.
Review effort: Balanced
Findings: 2
Open (7)
What changed in this PR
Improves structural symbol browsing, array handling, and batch performance in the S7CommPlus driver.
Changes:
- Adds structural tags, multidimensional values, and declared string-length propagation.
- Optimizes payload-size checks and adds TLS queue locking.
- Adds regression tests and performance documentation.
| File | Description |
|---|---|
| src/S7CommPlusDriver/S7CommPlusTagAccessorCatalog.cs | Persists string lengths in catalog format v3. |
| src/S7CommPlusDriver/S7CommPlusProtocolSession.cs | Optimizes batching and expands tag resolution. |
| src/S7CommPlusDriver/S7CommPlusClient.cs | Adds a diagnostics import. |
| src/S7CommPlusDriver/OpenSSL/OpenSSLConnector.cs | Locks pending-data queues. |
| src/S7CommPlusDriver/Core/POffsetInfoType.cs | Exposes string-length metadata. |
| src/S7CommPlusDriver/ClientApi/PlcTags.cs | Creates structural and length-aware string tags. |
| src/S7CommPlusDriver/ClientApi/PlcTag.cs | Adds array shapes, multidimensional values, and structural tags. |
| src/S7CommPlusDriver/ClientApi/Browser.cs | Propagates string lengths into browse results. |
| src/S7CommPlusDriver.Tests/S7CommPlusProtocolSessionReceiveTests.cs | Tests batch split boundaries. |
| src/S7CommPlusDriver.Tests/PlcTagStructArrayTests.cs | Tests structural tags and array declarations. |
| src/S7CommPlusDriver.Tests/PlcTagStringMaxLengthTests.cs | Tests string-length propagation. |
| src/S7CommPlusDriver.Tests/PlcTagMultiDimensionalArrayTests.cs | Tests multidimensional reads and assignments. |
| 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; |
Comment on lines
+36
to
+37
| private readonly object m_readListLock = new object(); | ||
| private readonly object m_writeListLock = new object(); |
Comment on lines
+209
to
+211
| if (m_AggregateDimensions.Count > 1) | ||
| { | ||
| aggregateValueProperty.SetValue(this, values); | ||
| AggregateValue = ReshapeToMultiDimensional(elementValueProperty.PropertyType, flatValues, m_AggregateDimensions); |
Comment on lines
+235
to
237
| if (m_AggregateElements.Count == 0 || m_AggregateDimensions.Count > 1) | ||
| { | ||
| return; |
Comment on lines
+289
to
+296
| 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)); |
| /// 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-05