refactor: Unified the language for defining and addressing array elements. - #2719
Conversation
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.
This PR unifies PLC4X array element selection syntax across Java drivers by extracting a shared array-notation grammar into SPI code, updating driver parsers/serializers to use it, and aligning docs/tests accordingly.
Changes:
- Introduces shared array selection parsing/rendering (
ArrayNotationParser) plus protocol capability constraints (AddressConstraints) and extendsArrayInfoto carry base/range semantics. - Migrates multiple Java drivers (S7, Modbus, SLMP, ADS, UMAS, EtherNet/IP, Profinet, Simulated, Firmata, OPC-UA) to the unified “
[selection]before:TYPE” notation and updates test suites/resources. - Adds user docs for the shared notation and documents incompatibilities in
RELEASE_NOTES.
Reviewed changes
Copilot reviewed 113 out of 113 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| website/asciidoc/modules/users/pages/protocols/umas.adoc | Notes UMAS now uses shared array selection notation and links to new doc page. |
| website/asciidoc/modules/users/pages/protocols/slmp.adoc | Notes SLMP now uses shared array selection notation and links to new doc page. |
| website/asciidoc/modules/users/pages/protocols/simulated.adoc | Notes Simulated driver now uses shared array selection notation and links to new doc page. |
| website/asciidoc/modules/users/pages/protocols/s7.adoc | Updates S7 address grammar to place selection before type and explains shared notation. |
| website/asciidoc/modules/users/pages/protocols/profinet.adoc | Notes Profinet now uses shared array selection notation and links to new doc page. |
| website/asciidoc/modules/users/pages/protocols/opcua.adoc | Notes OPC-UA uses shared array selection notation and links to new doc page. |
| website/asciidoc/modules/users/pages/protocols/modbus.adoc | Updates Modbus address grammar/examples to shared selection-before-type notation. |
| website/asciidoc/modules/users/pages/protocols/firmata.adoc | Warns about Firmata’s silent meaning change and shows rewrite guidance. |
| website/asciidoc/modules/users/pages/protocols/eip.adoc | Updates EIP docs to range selection and documents CIP index limit. |
| website/asciidoc/modules/users/pages/protocols/ads.adoc | Notes ADS now uses shared array selection notation and links to new doc page. |
| website/asciidoc/modules/users/pages/array-notation.adoc | Adds the new “Addressing arrays” documentation page defining shared grammar and behavior. |
| website/asciidoc/modules/users/nav.adoc | Adds “Addressing arrays” page to the Users navigation. |
| protocols/modbus/src/test/resources/protocols/modbus/tcp/DriverTestsuiteOptimized.xml | Updates Modbus test-suite addresses to new selection syntax. |
| protocols/modbus/src/test/resources/protocols/modbus/tcp/DriverTestsuite.xml | Updates Modbus test-suite addresses to new selection syntax. |
| protocols/eip/src/test/resources/protocols/eip/DriverTestsuite.xml | Updates EIP test-suite addresses to new selection syntax. |
| plc4j/spi/drivers/src/test/java/org/apache/plc4x/java/spi/drivers/model/MultiDimensionConstraintTest.java | Adds SPI-level tests for multi-dimension constraints and error messaging. |
| plc4j/spi/drivers/src/test/java/org/apache/plc4x/java/spi/drivers/model/ArrayNotationRoundTripTest.java | Adds SPI-level round-trip tests for parse/render canonicalization behavior. |
| plc4j/spi/drivers/src/main/java/org/apache/plc4x/java/spi/drivers/model/DefaultArrayInfo.java | Extends array metadata to include base and whether the dimension was written as a range. |
| plc4j/spi/drivers/src/main/java/org/apache/plc4x/java/spi/drivers/model/ArrayNotationParser.java | Introduces shared parser/renderer and migration helpers for legacy address forms. |
| plc4j/spi/drivers/src/main/java/org/apache/plc4x/java/spi/drivers/model/AddressConstraints.java | Adds protocol capability constraints (max index/dimensions, range-position rule). |
| plc4j/drivers/umas/src/test/java/org/apache/plc4x/java/umas/tag/SymbolicUmasTagSelectionTest.java | Adds UMAS tests for selection parsing/reporting and range placement rules. |
| plc4j/drivers/umas/src/main/java/org/apache/plc4x/java/umas/tag/SymbolicUmasTag.java | Parses trailing selection with shared parser and exposes selection/base intent. |
| plc4j/drivers/umas/src/main/java/org/apache/plc4x/java/umas/UmasConnection.java | Refuses element-selection operations as UNSUPPORTED and strips selection for symbol lookup. |
| plc4j/drivers/slmp/src/test/resources/slmp/slmp-driver-testsuite.xml | Updates SLMP test-suite addresses to new selection syntax. |
| plc4j/drivers/slmp/src/test/java/org/apache/plc4x/java/slmp/tag/SlmpTagTest.java | Updates SLMP tag parsing tests for selection-before-type syntax and new edge cases. |
| plc4j/drivers/slmp/src/test/java/org/apache/plc4x/java/slmp/tag/SlmpLegacyAddressTest.java | Adds tests ensuring legacy SLMP forms fail with upgrade guidance. |
| plc4j/drivers/slmp/src/test/java/org/apache/plc4x/java/slmp/tag/SlmpArrayParityTest.java | Adds parity tests ensuring selection semantics match the shared contract. |
| plc4j/drivers/slmp/src/test/java/org/apache/plc4x/java/slmp/SlmpRequestBuildTest.java | Updates request-building test to new SLMP selection syntax. |
| plc4j/drivers/slmp/src/test/java/org/apache/plc4x/java/slmp/SlmpErrorMappingTest.java | Updates error-mapping tests to new SLMP selection syntax. |
| plc4j/drivers/slmp/src/main/java/org/apache/plc4x/java/slmp/tag/SlmpTag.java | Migrates SLMP tag parsing and address rendering to shared array-notation components. |
| plc4j/drivers/simulated/src/test/java/org/apache/plc4x/java/simulated/tag/SimulatedTagTest.java | Updates Simulated tag tests to new selection syntax. |
| plc4j/drivers/simulated/src/test/java/org/apache/plc4x/java/simulated/tag/SimulatedTagBoundTest.java | Updates bounds tests for new selection syntax (inclusive ranges). |
| plc4j/drivers/simulated/src/test/java/org/apache/plc4x/java/simulated/tag/SimulatedLegacyAddressTest.java | Adds tests ensuring legacy simulated forms fail with upgrade guidance. |
| plc4j/drivers/simulated/src/test/java/org/apache/plc4x/java/simulated/tag/SimulatedArrayParityTest.java | Adds parity tests and enforces “must start at element 0” constraint. |
| plc4j/drivers/simulated/src/test/java/org/apache/plc4x/java/simulated/connection/SimulatedDeviceTest.java | Updates simulated device tests to new selection syntax. |
| plc4j/drivers/simulated/src/test/java/org/apache/plc4x/java/simulated/ManualSimulatedDriverTest.java | Updates manual simulated driver example to new selection syntax. |
| plc4j/drivers/simulated/src/main/java/org/apache/plc4x/java/simulated/tag/SimulatedTag.java | Migrates simulated tag parsing/rendering to shared array-notation components. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7TagTest.java | Updates S7 tag tests to new selection syntax. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7TagSerializeTest.java | Updates S7 serialize tests to new selection syntax. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7TagElementCountBoundTest.java | Updates S7 element-count bounds tests to new selection syntax. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7StringTagTest.java | Updates S7 string-tag tests to new selection syntax. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7LegacyAddressTest.java | Adds tests ensuring legacy S7 forms fail with upgrade guidance. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7DeclaredBaseTest.java | Adds tests for declared lower bounds (;base) behavior in S7. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/tag/S7ArrayParityTest.java | Adds parity tests ensuring S7 selection semantics match shared contract. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/optimizer/S7OptimizerTest.java | Updates optimizer tests to new S7 selection syntax. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/optimizer/S7BlockReadOptimizerTest.java | Updates block-read optimizer tests to new S7 selection syntax. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/manual/ManualWallS7300DriverTest.java | Updates manual S7 examples to new selection syntax. |
| plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/manual/ManualFactoryS71200DriverTest.java | Updates manual S7 examples to new selection syntax. |
| plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7Tag.java | Migrates S7 parsing to selection-before-type and resolves selection into byte offsets. |
| plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringVarLengthTag.java | Migrates var-length string tags to selection-before-type parsing. |
| plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringFixedLengthTag.java | Migrates fixed-length string tags to selection-before-type parsing. |
| plc4j/drivers/profinet/src/test/java/org/apache/plc4x/java/profinet/tag/ProfinetTagTest.java | Updates Profinet tests to new selection syntax and legacy rejection. |
| plc4j/drivers/profinet/src/test/java/org/apache/plc4x/java/profinet/tag/ProfinetLegacyAddressTest.java | Adds tests ensuring legacy Profinet forms fail with upgrade guidance. |
| plc4j/drivers/profinet/src/main/java/org/apache/plc4x/java/profinet/tag/ProfinetTag.java | Migrates Profinet parsing/rendering to shared notation and selection rules. |
| plc4j/drivers/profinet-ng/src/test/java/org/apache/plc4x/java/profinet/tag/ProfinetTagTest.java | Updates Profinet-NG tests to new selection syntax. |
| plc4j/drivers/profinet-ng/src/test/java/org/apache/plc4x/java/profinet/tag/ProfinetNgLegacyAddressTest.java | Adds tests ensuring legacy Profinet-NG forms fail (and missing-index is reported). |
| plc4j/drivers/profinet-ng/src/test/java/org/apache/plc4x/java/profinet/ManualProfinetIoTestZylkSimocode.java | Updates manual Profinet-NG example to new selection syntax. |
| plc4j/drivers/profinet-ng/src/test/java/org/apache/plc4x/java/profinet/ManualProfinetIoTestSimocodePN.java | Updates manual Profinet-NG example to new selection syntax. |
| plc4j/drivers/profinet-ng/src/main/java/org/apache/plc4x/java/profinet/tag/ProfinetTag.java | Migrates Profinet-NG parsing/rendering to shared notation and selection rules. |
| plc4j/drivers/opcua/src/test/java/org/apache/plc4x/java/opcua/tag/OpcuaIndexRangeTest.java | Adds tests ensuring shared selection maps to OPC-UA IndexRange correctly. |
| plc4j/drivers/opcua/src/test/java/org/apache/plc4x/java/opcua/manual/ManualOpcUaS71500NewFWDriverTest.java | Updates manual OPC-UA connection example to include security query params. |
| plc4j/drivers/opcua/src/test/java/org/apache/plc4x/java/opcua/manual/ManualOpcUaS71500NewFWBrowse.java | Updates manual OPC-UA browse example to include security query params. |
| plc4j/drivers/opcua/src/main/java/org/apache/plc4x/java/opcua/tag/OpcuaTag.java | Reuses shared array notation parser for index-range parsing/rendering and arrayInfo reporting. |
| plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/ModbusTagTest.java | Updates Modbus tag tests to new selection syntax and inclusive range templates. |
| plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/ModbusStringTest.java | Updates string handling tests to new selection syntax. |
| plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/ModbusLegacyAddressTest.java | Adds tests ensuring legacy Modbus forms fail with upgrade guidance. |
| plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/ModbusEncodeTest.java | Updates Modbus encode tests to new selection syntax. |
| plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/ModbusBitStringArrayTest.java | Updates bitstring array tests to new selection syntax. |
| plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/ModbusArrayParityTest.java | Adds parity tests ensuring Modbus selection semantics match shared contract. |
| plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/tag/ModbusTagInputRegister.java | Applies shared selection parsing to input-register tag offsets/quantities. |
| plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/tag/ModbusTagHoldingRegister.java | Applies shared selection parsing to holding-register tag offsets/quantities. |
| plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/tag/ModbusTagExtendedRegister.java | Applies shared selection parsing to extended-register tag offsets/quantities. |
| plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/tag/ModbusTagDiscreteInput.java | Applies shared selection parsing to discrete-input tag offsets/quantities. |
| plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/tag/ModbusTagCoil.java | Applies shared selection parsing to coil tag offsets/quantities. |
| plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/tag/ModbusTag.java | Migrates Modbus shared parsing (selection before type) and address rendering. |
| plc4j/drivers/knxnetip/src/test/java/org/apache/plc4x/java/knxnetip/maual/ManualKnxNetIpWrite.java | Changes KNX manual test connection parameters to placeholder values. |
| plc4j/drivers/knxnetip/src/test/java/org/apache/plc4x/java/knxnetip/maual/ManualKnxNetIpSubscription.java | Changes KNX manual test connection parameters to placeholder values. |
| plc4j/drivers/knxnetip/src/test/java/org/apache/plc4x/java/knxnetip/maual/ManualKnxNetIpRead.java | Changes KNX manual test connection parameters to placeholder values. |
| plc4j/drivers/knxnetip/src/test/java/org/apache/plc4x/java/knxnetip/maual/ManualKnxNetIpBrowse.java | Changes KNX manual test connection parameters to placeholder values. |
| plc4j/drivers/firmata/src/test/java/org/apache/plc4x/java/firmata/tag/FirmataTagTest.java | Updates Firmata tests to new meaning of brackets (ranges vs counts). |
| plc4j/drivers/firmata/src/test/java/org/apache/plc4x/java/firmata/tag/FirmataTagPinSpanTest.java | Updates Firmata pin span tests to use inclusive ranges. |
| plc4j/drivers/firmata/src/main/java/org/apache/plc4x/java/firmata/tag/FirmataTagDigital.java | Renders selection using shared renderer and fixes inclusive bounds in arrayInfo. |
| plc4j/drivers/firmata/src/main/java/org/apache/plc4x/java/firmata/tag/FirmataTagAnalog.java | Renders selection using shared renderer and fixes inclusive bounds in arrayInfo. |
| plc4j/drivers/firmata/src/main/java/org/apache/plc4x/java/firmata/tag/FirmataTag.java | Migrates Firmata address parsing to shared selection notation. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagTest.java | Updates EIP tests for selection-based element count instead of count suffix. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagPathTest.java | Updates path handling tests for selection semantics and bracket validation. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagHandlerTest.java | Updates handler tests for new EIP syntax. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagCoverageTest.java | Updates coverage tests for new EIP syntax and round-tripping address string. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipLegacyAddressTest.java | Adds tests ensuring legacy EIP count suffix is rejected. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipArrayParityTest.java | Adds parity tests ensuring EIP selection semantics match shared contract. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipUnsignedIntegerTypeTest.java | Updates EIP decode tests to new selection syntax. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipDockerIT.java | Updates EIP integration test to new selection syntax. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipBitStringTypeTest.java | Updates EIP bitstring decode tests to new selection syntax. |
| plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipArrayReadTest.java | Updates EIP array read tests to new selection syntax. |
| plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/tag/EipTag.java | Migrates EIP parsing/rendering to shared selection parser and enforces CIP constraints. |
| plc4j/drivers/ads/src/test/java/org/apache/plc4x/java/ads/tag/SymbolicAdsTagRangeTest.java | Adds ADS symbolic tests for allowable range placement. |
| plc4j/drivers/ads/src/test/java/org/apache/plc4x/java/ads/tag/DirectAdsTagTest.java | Updates ADS direct tag tests to new selection syntax. |
| plc4j/drivers/ads/src/test/java/org/apache/plc4x/java/ads/tag/DirectAdsTagBoundTest.java | Updates ADS bounds tests for new selection syntax and empty-selection rejection. |
| plc4j/drivers/ads/src/test/java/org/apache/plc4x/java/ads/tag/DirectAdsStringTagTest.java | Updates ADS string tag tests to new selection syntax. |
| plc4j/drivers/ads/src/test/java/org/apache/plc4x/java/ads/tag/AdsLegacyAddressTest.java | Adds tests ensuring legacy ADS forms fail with upgrade guidance. |
| plc4j/drivers/ads/src/test/java/org/apache/plc4x/java/ads/tag/AdsArrayParityTest.java | Adds parity tests ensuring ADS selection semantics match shared contract. |
| plc4j/drivers/ads/src/test/java/org/apache/plc4x/java/ads/resolution/TagResolverTest.java | Expands ADS resolver tests for whole-array reads, base verification, and member-access rules. |
| plc4j/drivers/ads/src/test/java/org/apache/plc4x/java/ads/manual/Scanner.java | Updates ADS manual scanner example to new selection syntax. |
| plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/tag/SymbolicAdsTag.java | Adds shared selection parsing for ADS symbolic tags and selection/base accessors. |
| plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/tag/DirectAdsTag.java | Migrates ADS direct tags to selection-before-type syntax and shared error messaging. |
| plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/tag/DirectAdsStringTag.java | Migrates ADS direct string tags to selection-before-type syntax. |
| plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/resolution/TagResolver.java | Applies selection semantics in ADS resolution, verifies declared base, rejects ambiguous member reads. |
| plc4j/api/src/main/java/org/apache/plc4x/java/api/model/ArrayInfo.java | Extends ArrayInfo interface docs and adds default methods for base and range semantics. |
| plc4go/assets/testing/protocols/modbus/tcp/DriverTestsuiteOptimized.xml | Updates plc4go Modbus test-suite addresses to new selection syntax. |
| plc4go/assets/testing/protocols/modbus/tcp/DriverTestsuite.xml | Updates plc4go Modbus test-suite addresses to new selection syntax. |
| plc4go/assets/testing/protocols/eip/DriverTestsuite.xml | Updates plc4go EIP test-suite addresses to new selection syntax. |
| RELEASE_NOTES | Documents the incompatible address syntax changes and migration guidance. |
Suppressed comments (3)
plc4j/spi/drivers/src/main/java/org/apache/plc4x/java/spi/drivers/model/ArrayNotationParser.java:1
EXPRESSION_REGEXappears to be missing the literal closing bracket\\]for each bracket group (it currently matches\\[+ DIMENSION ... but not the terminating]). As written,EXPRESSION_PATTERNwould fail to match valid expressions like[0..3]because the input contains]at the end. Fix by including\\]in the expression (and any related patterns/constants that rely on it) soexpressionPart()/addressPart()andparse()can reliably detect and validate bracket runs.
plc4j/drivers/knxnetip/src/test/java/org/apache/plc4x/java/knxnetip/maual/ManualKnxNetIpWrite.java:1- These KNXnet/IP manual test changes (replacing connection parameters with
huiiiii/lalala) are unrelated to the PR’s stated purpose (array-notation unification) and also reduce the usefulness/clarity of the manual example. Consider reverting these changes or moving them to a dedicated PR, and use neutral placeholders likeknxproj-file-path=/path/to/project.knxprojandknxproj-password=***if scrubbing secrets is needed. (The same issue appears in the other KNX manual files changed here.)
plc4j/drivers/opcua/src/test/java/org/apache/plc4x/java/opcua/manual/ManualOpcUaS71500NewFWDriverTest.java:1 - This manual example hard-codes
message-security=NONEandinsecure-certificate-verification=true. Even in test/manual code, this can normalize insecure defaults and be copy-pasted into production usage. Prefer leaving secure defaults in examples and, if this is required for a specific environment, add an inline comment explaining why and/or provide a separate insecure example block clearly labeled as such (same applies to the corresponding Browse manual file).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 180 out of 181 changed files in this pull request and generated 33 comments.
Files not reviewed (1)
- plc4go/internal/eip/mocks_test.go: Generated file
Suppressed comments (2)
plc4go/internal/ads/model/Tag.go:114
- Direct ranges are exposed here, but ADS execution was not updated consistently. Reads decode with
DataType.GetArrayInfo()(and single reads request one element), so a primitive range returns one scalar; writes pass this range with a primitive datatype intoserializePlcValue, which expects an ADSARRAY ... OF ...datatype and fails lookup. Update direct read/write paths to size and encode/decode primitive selections using this resolved shape.
func (m DirectPlcTag) GetArrayInfo() []apiModel.ArrayInfo {
return shapeOf(m.ArrayInfo)
}
plc4go/internal/ads/TagHandler.go:167
- This numeric direct branch also stores the selection without applying it to ADS's byte-based
indexOffset.0x4020/0[3]:DINTtherefore still accesses offset 0 rather than byte offset 12. Resolve(lower-base) * elementByteSizeduring datatype resolution and validate uint32 overflow.
// The selection goes through the shared parser, so plc4go accepts exactly what plc4j
// accepts. A direct address names a memory location, so it carries one dimension.
arrayInfo, err := spiModel.ParseArrayExpression(match["array"], query, spiModel.SingleDimension)
if err != nil {
return nil, err
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 190 out of 191 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- plc4go/internal/eip/mocks_test.go: Generated file
Suppressed comments (4)
plc4j/drivers/knxnetip/src/test/java/org/apache/plc4x/java/knxnetip/maual/ManualKnxNetIpWrite.java:1
- The manual KNXnet/IP examples replace real values with informal placeholders (e.g., 'huiiiii', 'lalala'), which reduces the usefulness of these examples and looks unprofessional. Consider switching to conventional placeholders (e.g., '/path/to/project.knxproj' and '' or a system property/env-var lookup) and apply the same change in the other ManualKnxNetIp* files updated in this PR.
plc4j/drivers/firmata/src/main/java/org/apache/plc4x/java/firmata/tag/FirmataTagAnalog.java:1 - The method closing brace is on the same line as the return statement, which is inconsistent with the surrounding formatting and makes diffs/error locations harder to read. Reformat this to put the closing
}on its own line.
plc4go/internal/knxnetip/Tag.go:1 - With the unified array notation, a one-element range (e.g.,
[0..0]) should still be reported as an array (list-of-one) because callers useGetArrayInfo()/IsRange()to distinguish scalar vs list shape. This helper drops array info for anynumElements == 1, losing that distinction. To preserve correct shape, store an explicit 'rangeWritten' flag on these tags (similar to other drivers in this PR) and use that to decide whetherGetArrayInfo()is empty, rather than inferring from element count alone.
plc4j/drivers/umas/src/main/java/org/apache/plc4x/java/umas/UmasConnection.java:1 - Using
toLowerCase()without an explicit locale can cause incorrect case-folding in certain locales (notably Turkish), which could break symbol table lookups at runtime. UsetoLowerCase(Locale.ROOT)(and similarly in the write path) for locale-stable normalization.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 197 out of 197 changed files in this pull request and generated 7 comments.
Suppressed comments (19)
Previously missed (15) — in code that hasn't changed since the last review.
plc4go/internal/eip/TagHandler.go:40
- The leading
%remains mandatory here, while PLC4J and the EIP documentation accept bothmyTagand%myTag. This prevents the same unified EIP address from being reused across the two bindings. Make the prefix optional;GetAddressStringcan still emit%canonically.
addressPattern: regexp.MustCompile(`^%(?P<tag>[%a-zA-Z_.0-9]+)` + spiModel.ArrayGroupPattern + `(?::(?P<dataType>[A-Z]+))?$`),
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/tag/ModbusTagHoldingRegister.java:96
quantityis an element count, but both bounds below are Modbus register-count limits. Wide values therefore pass invalid requests—for example, 63 DINTs pass the<=125check but require 126 registers, and an address near the end of the space is checked two registers short per DINT. Validatequantity * registersPerElement(...)instead.
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/tag/ModbusTagInputRegister.java:96quantitycounts values, while the following range and 125-register limits apply to registers. Thus wide selections such as 63 DINTs are accepted even though they generate a 126-register request, and end-of-space checks undercount their span. Validate the transfer's register count instead.
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/tag/ModbusTagExtendedRegister.java:98- The validation below treats this element count as a register count. Multi-register types can therefore exceed both the 125-register request limit and the end of the address space while parsing successfully. Compute and validate the actual register span.
plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringVarLengthTag.java:129 - The short-form path also discards whether the source used a range. A one-element range such as
%DB1:0[4..4]:STRINGis therefore exposed and rendered as a scalar. Preserveselection[2]in the constructed tag.
plc4go/internal/knxnetip/Tag.go:368 - This helper derives array shape only from the count, discarding whether the parser saw a range. Consequently a KNX device address ending in
[4..4]becomes a scalar and renders without brackets, contrary to the shared[n..n]list-of-one contract. CarryIsRange()through the tag constructors and use it here.
plc4go/internal/knxnetip/Browser.go:322 - When the device reports zero association entries, subtracting one from this
uint16produces 65535, so the generated[0..65535]selection fails parsing instead of treating the table as empty. Handle zero before constructing either request, as the earlier group-address-table path does.
plc4go/internal/knxnetip/Tag.go:354 %Xcan emit an odd number of hex digits (for example address 10 becomesA), butParseTaguseshex.DecodeString, which rejects odd-length input. Such tags no longer round-trip. Since the stored address is 16-bit, render four hex digits.
plc4j/spi/drivers/src/main/java/org/apache/plc4x/java/spi/drivers/model/ArrayNotationParser.java:122- A syntactically legacy address with an oversized count reaches
Integer.parseInthere and throwsNumberFormatExceptionwhile constructing the intendedPlcInvalidTagException. Invalid addresses such asD100:INT[99999999999]should remain controlled tag-parse failures; parse safely and omit the migration rewrite when the count cannot be represented.
plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringVarLengthTag.java:109 - The parser records whether this is a range in
selection[2], but this path only carries the count forward. As a result%DB1.DB0[4..4]:STRINGis exposed and rendered as a scalar. Preserve the explicit-range flag through the constructed subtype, as the base S7 parser does.
website/asciidoc/modules/users/pages/array-notation.adoc:110 - This is not a universal restriction: the preceding multidimensional example contains a non-trailing range, and OPC-UA supports multidimensional
IndexRangeselections. Qualify this as a restriction of symbolic-path drivers/protocols that can encode only one contiguous count, rather than stating that every driver rejects it.
website/asciidoc/modules/users/pages/protocols/firmata.adoc:73 - The format now names
selection, but the following sentence still saysarray-sizeis a simple integer. That is no longer true because selections include indices, inclusive ranges, and declared bases. Update the adjacent description to match the new grammar.
This issue also appears on line 95 of the same file.
plc4go/spi/model/ArrayNotationParser.go:270
- On parse overflow this returns
[0], so an invalid legacy request for a huge array is misleadingly rewritten as a single-element request. Make the conversion fallible and letCurrentFormOfdecline the rewrite when the count cannot be represented.
plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringFixedLengthTag.java:146 - Although
selectionOfcaptures whether the address used a range, this path only forwards its count. Therefore%DB1.DB0[4..4]:STRING(40)becomes a scalar and round-trips without the brackets. Carryselection[2]through a shape-aware constructor.
plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7StringFixedLengthTag.java:165 - The short fixed-string form has the same shape loss: a one-element range is reduced to
numElements == 1, so the superclass infers scalar. Preserve the parsed explicit-range flag when constructing the tag.
website/asciidoc/modules/users/pages/protocols/firmata.adoc:95
- As with the digital format, this now uses
selectionwhile the next sentence still documents an integerarray-size. Update that prose so users do not interpret[n]as the old count syntax.
plc4go/internal/modbus/Tag.go:283 - This rounds each sub-register element up independently, which disagrees with the packed codec. Two SINT values occupy one register, but offset 2 is multiplied by one register and skips two; odd-byte starts cannot be represented by a register address alone. Resolve the total byte offset and reject or retain non-word-aligned starts.
plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7Tag.java:287 - This short-form path also scales an unconstrained index in
int, allowing wraparound to a different S7 byte offset. Use overflow-safe arithmetic and verify the shifted address plus selected span stays within the addressable area.
plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/tag/S7Tag.java:326 - Large valid index text can overflow this
intoffset calculation and silently target another address. Calculate inlongand reject any shifted span beyondMAX_BYTE_OFFSETbefore constructing the tag.
PLC4X tag addresses express "which elements of this address do I want" in at least four
mutually incompatible ways today, and the same bracket expression means different things
depending on which driver reads it:
[4]meansns=2;i=MyInt[3..8];DINTmyArray[4]:DINT:8(index and count, separately)%DB42:28.0:BYTE[4]40001:INT[4]D100:INT[4]0x4020/0:DINT[4]MAIN.g_arr[1](no type - the symbol table has it)MyVar[1](no type - device metadata has it)3[4](no type in the address at all)…:INT[4]The OPC-UA driver already implements the target notation in full — single index, inclusive
range, an optional declared lower bound, and multiple dimensions — in
OpcuaTag(
INDEX_RANGE_PATTERN,SINGLE_BRACKET_PATTERN,toOpcuaIndexRange). This feature adoptsthat grammar everywhere rather than inventing one, and moves it into shared code so the
implementations cannot drift apart again.
The notation was agreed on the developer mailing list by lazy consensus. Two refinements were
made after the original post and need to be carried back to the thread: the brackets move
before the
:TYPEsuffix (the posted S7 example had them after), and the change affectsten tag classes rather than the two named in the thread.