fix(plc4j/ads): cap sum-command item count at 500 - #2714
fix(plc4j/ads): cap sum-command item count at 500#2714LivingLikeKrillin wants to merge 1 commit into
Conversation
multiRead/multiWrite packed every tag into a single ADS sum command (ADSIGRP_MULTIPLE_READ/_WRITE) with no bound on item count or expected response size; oversized sum frames are rejected by TwinCAT router and device buffers. Chunk the item list into consecutive groups of at most 500 (Beckhoff's documented sum-command guidance), send the groups sequentially, and merge the per-group results. Requests with <= 500 tags produce wire-identical traffic to before. The cap bounds the item count only. A group of 500 large tags can still exceed the frame the device will answer, because the expected response size (4 + sizeInBytes per item) is not itself budgeted; that is noted on the constant and left for a follow-up. The group chain composes with thenComposeAsync: the previous group's stage completes on the receive thread, so composing synchronously would build and write the next sum command from that callback, and would nest one stage per group whenever a stage completes inline. The throttle no longer blocks the caller (see 22ca9ed), so the hop is about where the work runs, not about a parked permit -- it does mean that even a single-group request now dispatches through the common pool, where it previously ran on the calling thread. Per-group failure isolation is documented on the constant: a group-level ReturnCode != OK fails only that group's tags. A group that fails outright still fails the whole request, so a multi-group write that fails midway leaves the earlier groups applied on the device. Signed-off-by: Jooyoung Jung <livinglikekrillin@gmail.com>
|
"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 MSP 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.
| 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 |
What
multiRead/multiWritepacked every requested tag into a single ADS sum command(
ADSIGRP_MULTIPLE_READ/ADSIGRP_MULTIPLE_WRITE) with no bound on the item count. This splits theitem list into consecutive groups of at most 500, sends them sequentially, and merges the results
back into one response in the request's tag order.
MAX_SUM_COMMAND_ITEMS = 500follows Beckhoff's sum-command guidance. I could not pin that to asingle citable page, so treat it as a conservative default — happy to make it an
AdsConfigurationparameter if you would rather it be tunable.
What this does not fix
The cap bounds the item count. It does not bound the frame size. The expected response size of a sum read is
4 + sizeInBytesper item and is not budgeted, so 500 large tags can still exceed what the devicewill answer. The EIP driver has the same defect class and is fixed by byte budget in a separate PR;
doing the same here means computing an ADS payload budget and is a follow-up, not this change.
Behaviour changes
ceil(n/500)sequential round trips, each with its own requesttimeout, so a large request's total deadline scales with the number of groups.
that means the groups already sent are applied on the device while the API future fails. Previously
such a request produced one oversized frame that the device rejected outright.
ReturnCode != OKfails only that group's tags. Fora single group that is the previous all-or-nothing behaviour.
thenComposeAsync, so even a single-group request nowdispatches through the common pool instead of running on the calling thread. The reason for the hop
is that the previous group's stage completes on the receive thread and the next send should not be
built and written from that callback. (The throttle itself stopped blocking the caller in
22ca9ed054; the hop is about where the work runs.)Testing
AdsSumChunkTest— 9 cases overchunk(): empty, single, under/at/one-over the cap, full groupsplus remainder, order preservation, a smaller cap, and rejection of a non-positive cap.
mvn -pl plc4j/drivers/ads test→ 167 tests, 0 failures;verifygreen. (AdsDriverITis@Disabledupstream and stays that way.)the merge keeps request order, or that per-group isolation holds — ADS has no driver testsuite and
I have no TwinCAT device. If a
Manual*test against a TC3 runtime would help, I will add one.