Repository navigation
fix(plc4j/ads): cap sum-command item count, configurably - #2714
LivingLikeKrillin wants to merge 1 commit into
Conversation
|
"I could not pin that to a |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Caps ADS sum-command (multiRead/multiWrite) sub-command count to avoid oversized frames by chunking requests into groups of up to 500 items, sending groups sequentially, and merging results back into the original tag order.
Changes:
- Add
MAX_SUM_COMMAND_ITEMS = 500and achunk()helper to partition sum-command items. - Update
multiRead/multiWriteto execute chunked sum-commands sequentially and merge per-group results. - Add unit tests covering chunk partitioning behavior (
AdsSumChunkTest).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/AdsTcpConnection.java | Introduces sum-command item cap, chunking helper, and chunked multiRead/multiWrite execution/merge logic. |
| plc4j/drivers/ads/src/test/java/org/apache/plc4x/java/ads/AdsSumChunkTest.java | Adds unit tests validating AdsTcpConnection.chunk() partitioning and order preservation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| CompletableFuture<Map<String, PlcResponseItem<PlcValue>>> merged = | ||
| CompletableFuture.completedFuture(new LinkedHashMap<>()); | ||
| for (List<String> groupNames : chunk(orderedNames, MAX_SUM_COMMAND_ITEMS)) { | ||
| merged = merged.thenComposeAsync(accumulated -> | ||
| multiReadChunk(groupNames, resolved).thenApply(groupValues -> { | ||
| accumulated.putAll(groupValues); | ||
| return accumulated; | ||
| })); | ||
| } |
There was a problem hiding this comment.
There is no executor in the SPI to pass, and no call site in plc4j passes one to an *Async method today. Modbus, SLMP and both no-message-router paths in EipTcpConnection chain the same way on develop.
The hop itself is deliberate: the previous group's stage completes on the receive thread, and the next group should not be built and written from that callback.
Edited 2026-10-19: Corrected "both MSP paths" to "both no-message-router paths".
| for (List<String> groupNames : chunk(orderedNames, MAX_SUM_COMMAND_ITEMS)) { | ||
| merged = merged.thenComposeAsync(accumulated -> | ||
| multiReadChunk(groupNames, resolved).thenApply(groupValues -> { | ||
| accumulated.putAll(groupValues); | ||
| return accumulated; | ||
| })); | ||
| } |
|
It's in Beckhoff's documentation. I didn't find the page before opening this, which is on me.
https://infosys.beckhoff.com/content/1033/tc3_adsdll2/124835083.html - "ADS-sum command: Read or Write a list of variables with one single ADS-command", the page for I drafted the description with LLM help and reviewed it myself, but I didn't catch that it claimed Beckhoff guidance without a source. Sorry about that. I should have checked before opening the PR. |
|
rebase on develop; the driver testsuite now runs, so the unanswered Copilot test-coverage comment is directly addressable - and since you agreed the config idea is good, ask for MAX_SUM_COMMAND_ITEMS as an AdsConfiguration parameter, then an e2e testcase with cap=2 and 3 tags covers the split, order-preserving merge, and per-group isolation without needing 501 tags in XML. Note the four still-commented-out testcases (direct/multi-element/symbolic reads) are separate revival candidates I didn't touch. |
|
FYI: 4de8bf1 pushed a update to the testsuite so be sure to base on that |
The ADS driver's multiRead/multiWrite packed every tag they sent into a single ADS sum command (ADSIGRP_MULTIPLE_READ / ADSIGRP_MULTIPLE_WRITE) with no bound on the number of sub-commands. The item list is now split into consecutive groups of at most max-sum-command-items; the groups are sent sequentially and their results merged into one response. A request within the cap produces the same sum command as before: same items, same order, same payload. max-sum-command-items is an AdsConfiguration parameter (default 500); onConnect rejects a value below 1. Beckhoff's manual [1] says "Number of Sub-ADS calls: Highly recommended to max. 500!". The groups are chained with thenComposeAsync, so the next group is not built from the receive-thread callback that completed the previous one. As a consequence even a request that takes the sum path with a single group now builds and sends that sum command through the common pool instead of on the calling thread; requests left with one tag after resolution keep their separate non-sum path. A group-level ReturnCode != OK fails only that group's tags. A group that fails outright (send failure or timeout) fails the whole request and the remaining groups are not sent; in a multi-group write the earlier groups' writes may already have taken effect and are not rolled back. The same manual section also requires bounding the byte length of the requested data. This change does not do that. Also not covered: the separate, uncapped sum command used for symbolic handle resolution (ADSIGRP_SYM_HNDBYNAME sub-items). Both are left for a follow-up. The shared ADS driver testsuite gains three cases with the cap set to two and three tags: a split read, a split write, and one where the second group fails and only that group's tag reports the error. plc4go's multi-tag paths send one sum command per request and have no such parameter, so its runner skips these three cases. Unit tests cover the grouping helper chunk(), the two merge helpers, and the parameter default. [1] https://download.beckhoff.com/download/document/automation/twincat3/TwinCAT_ADS-DLL_C_EN.pdf TwinCAT 3 ADS-DLL C++ manual v1.1.3, section "ADS-sum command: Read or Write a list of variables with one single ADS-command" Signed-off-by: Jooyoung Jung <livinglikekrillin@gmail.com>
377f2cf to
00c7581
Compare
|
@sruehl Rebased onto current develop (
Three testsuite cases at cap 2 with three tags: a split read, a split write, and one where the device answers the second sum command with The split read pins the group order on the wire (offsets 1 and 2 in the first sum command, 3 in the second) and checks the merged values per tag. With BOOLs it cannot catch hurz1 and hurz3 swapping values; The multi-tag paths in Java: 217 unit, 9 IT. Go: |
LivingLikeKrillin
changed the title
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
Math.ceilDiv(int, int)is not available on many commonly used Java baselines (it was introduced… · NewthenComposeAsync(...)without an explicit executor usesForkJoinPool.commonPool(). That makes… · New
2 resolved since last review
Review effort: Lite
| if (maxPerChunk <= 0) { | ||
| throw new IllegalArgumentException("maxPerChunk must be positive, got " + maxPerChunk); | ||
| } | ||
| List<List<T>> chunks = new ArrayList<>(Math.ceilDiv(items.size(), maxPerChunk)); |
There was a problem hiding this comment.
The build compiles with <release>${java.version}</release> and java.version is 21; Math.ceilDiv(int, int) has been in the JDK since Java 18. The previous revision of this PR used the suggested (items.size() + maxPerChunk - 1) / maxPerChunk, but it was replaced: maxPerChunk is the max-sum-command-items setting, the connection only rejects values below 1, and with Integer.MAX_VALUE and two items the sum overflows, the old expression gives -1, and new ArrayList<>(-1) throws IllegalArgumentException. AdsSumChunkTest.maximumCapYieldsOneChunk covers that case.
| CompletableFuture<Map<String, PlcResponseItem<PlcValue>>> merged = | ||
| CompletableFuture.completedFuture(new LinkedHashMap<>()); | ||
| for (List<String> groupNames : chunk(orderedNames, getConfiguration().getMaxSumCommandItems())) { | ||
| merged = merged.thenComposeAsync(accumulated -> | ||
| multiReadChunk(groupNames, resolved).thenApply(groupValues -> { | ||
| accumulated.putAll(groupValues); | ||
| return accumulated; | ||
| })); | ||
| } |
There was a problem hiding this comment.
As noted in the earlier thread (#2714 (comment)), the SPI has no executor to pass and the hop off the receive thread is deliberate.
What
multiReadandmultiWritein the ADS driver previously packed every tag they sent (those that resolved and serialized) into a single ADS sum command (ADSIGRP_MULTIPLE_READ/ADSIGRP_MULTIPLE_WRITE) with no bound on the number of sub-commands.Now, items are split into consecutive groups of at most
max-sum-command-items. The groups are sent sequentially, and per-group results are merged into one response. Requests within the cap produce the same sum command as before: same items, same order, same payload.max-sum-command-itemsis added as anAdsConfigurationparameter (default 500).onConnectrejects values below 1 with aPlcConnectionException; otherwise an unusable value would only surface later as an unchecked exception out ofread().Why 500
Beckhoff's TwinCAT 3 ADS-DLL C++ manual (v1.1.3, section "ADS-sum command: Read or Write a list of variables with one single ADS-command"):
Behaviour changes
ntags to send, requests above the cap becomeceil(n/cap)sequential round trips; each gets a fresh request timeout and there is no request-wide deadline.ReturnCode != OKfails only that group's tags; other groups return their real results.INTERNAL_ERROR.thenComposeAsyncso the next group is not built and written from the receive-thread callback. Consequently, even a request that takes the sum path with a single group now builds and sends that sum command through the common pool instead of on the calling thread. Requests left with one tag after resolution are unaffected; they still take the separate non-sum path.Testing
AdsDriverIT(9 cases) gains three cases (cap=2, 3 directBOOLaddresses):ADSIGRP_MULTIPLE_READrequests on the wire (two sub-commands, then one) and merged values.ADSIGRP_MULTIPLE_WRITErequests, per-group payload bytes, and merged response codes.ADSERR_DEVICE_SYMBOLNOTFOUND; first group tags keep values, failing group tag carriesINVALID_ADDRESS.ADSERR_DEVICE_SYMBOLNOTFOUND→INVALID_ADDRESSmapping fails the isolation case.AdsSumChunkTest(10 cases forchunk(), including a cap ofInteger.MAX_VALUE),AdsSumMergeTest(5 cases over the two merge helpers; each tag keeps its own value or code, checked by keyed lookup), andAdsConfigurationTest(default 500, can be lowered).plc4j/drivers/ads: 217 unit tests and 9 IT cases, 0 failures.Reader.goandWriter.gosend one sum command for the whole request and have no equivalent parameter, so the three new cases are skipped viaWithSkippedTestCases.go test -run TestAdsDriverpasses.Not in this PR
resolveMultipleSymbolicAddresses) issues its own uncapped sum (ADSIGRP_MULTIPLE_READ_WRITEoverADSIGRP_SYM_HNDBYNAMEsub-items). Reads and writes do not go through it and resolve from locally cached tables (separate change).