SPI 3 Refactoring#2382
Conversation
| default: | ||
| return boxSideBySide(boxes.get(0), mergeHorizontal(new ArrayList<>(boxes).subList(1, boxes.size()))); | ||
| } | ||
| return switch (boxes.size()) { |
There was a problem hiding this comment.
really like the new syntax... :)
There was a problem hiding this comment.
Pull request overview
This pull request refactors the SPI (Service Provider Interface) layer by replacing Netty-based core with lightweight sub-modules that eliminate Netty dependencies and minimize third-party dependencies. This architectural change enables independent use of the code-generation framework and facilitates porting PLC4X to other languages.
Changes:
- Migrated data reader/writer classes from
org.apache.plc4x.java.spi.codegen.iotoorg.apache.plc4x.java.spi.fields.data.reader/writerpackages - Replaced exception types from
SerializationException/ParseExceptiontoBufferException - Updated method signatures to use
WithOptioninstead ofWithWriterArgs/WithReaderArgs - Created new buffer implementation modules (XML, byte-based) with comprehensive encoding support
Reviewed changes
Copilot reviewed 163 out of 503 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| DataWriter*.java | Refactored writer classes with updated package paths, simplified API removing logical names, and new exception types |
| DataReader*.java | Refactored reader classes with updated package paths, position tracking changes, and new exception types |
| WriteBufferXmlBased.java | New XML-based buffer implementation with StAX event writing |
| ReadBufferXmlBased.java | New XML-based buffer implementation with StAX event reading |
| Encoding*.java tests | Comprehensive test suites for various encoding implementations (UTF-8, UTF-16, ASCII, BCD, etc.) |
| pom.xml | New Maven module definitions for fields and buffer implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chrisdutz
force-pushed
the
refactor/spi3
branch
from
233a550 to
a2cbfeb
Compare
|
@chrisdutz I see a change in generated code, but I can't see any changes around transports SPI. If I understand correctly main point of this pull request is to update plc4j-spi to not rely on netty to initialize client. Do you have a separate branch for that work? Frame representations we generated before and with above changes interact only with |
|
Hmmm ... that's odd ... I think this PR has become so big GitHub can't show it anymore ... if you have a look at https://github.com/apache/plc4x/tree/refactor/spi3/plc4j/transports ... it's there. |
|
|
Yeah, github UI couldn't get more than 3000 files, transports fall apart from diff. Will have a look on them! |
|
Changes look good, awesome work, keep them coming :) |
|
I've just pushed a first port of the Modbus driver to the new SPI3 ... please have a look and leave some feedback here. I'm now going to pause for a few days so you all have time to review and reply. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 4911 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <groupId>org.apache.plc4x</groupId> | ||
| <artifactId>plc4x-code-generation-language-java</artifactId> | ||
| <version>0.14.0-SNAPSHOT</version> | ||
| <artifactId>plc4x-code-generation-language-java-jp</artifactId> <version>0.14.0-SNAPSHOT</version> |
There was a problem hiding this comment.
This line is not valid XML because <artifactId> and <version> are on the same line as two separate elements, which will break Maven parsing. Split this into separate properly-indented <artifactId> and <version> lines.
| <artifactId>plc4x-code-generation-language-java-jp</artifactId> <version>0.14.0-SNAPSHOT</version> | |
| <artifactId>plc4x-code-generation-language-java-jp</artifactId> | |
| <version>0.14.0-SNAPSHOT</version> |
| case "List": { | ||
| if (typeReference.isArrayTypeReference() && typeReference.asArrayTypeReference().orElseThrow().getElementTypeReference().isByteBased()) { | ||
| return CodeBlock.of("_value.getRaw()"); | ||
| } else if (typeReference.isArrayTypeReference()) { | ||
| ArrayTypeReference arrayTypeReference = typeReference.asArrayTypeReference().orElseThrow(); | ||
| TypeReference elementTypeReference = arrayTypeReference.getElementTypeReference(); | ||
| TypeName languageTypeNameForTypeReference = getLanguageTypeNameForTypeReference(elementTypeReference, true); | ||
| String typeName = StaticHelper.CAPITALIZE(languageTypeNameForTypeReference.toString()); | ||
| if (languageTypeNameForTypeReference.equals(TypeName.get(BigInteger.class))) { | ||
| typeName = "BigInteger"; | ||
| } else if (languageTypeNameForTypeReference.equals(TypeName.get(String.class))) { | ||
| typeName = "String"; | ||
| } | ||
| //_value.getList().stream().map(PlcValue::getBoolean).collect(Collectors.toList()) | ||
| return CodeBlock.of("_value.getList().stream().map($T::get$L).collect($T.toList())", PlcValue.class, typeName, Collectors.class); | ||
| } | ||
| } | ||
| case "Struct": { | ||
| // _value.getStruct().get("control").getBoolean() | ||
| CodeBlock typeName = CodeBlock.of("Struct"); | ||
| if ((typeReference != null) && typeReference.isArrayTypeReference() && typeReference.asArrayTypeReference().orElseThrow().getElementTypeReference().isByteBased()) { | ||
| typeName = CodeBlock.of("Raw"); | ||
| } else if ((typeReference != null) && typeReference.isSimpleTypeReference()) { | ||
| TypeName languageTypeNameForTypeReference = getLanguageTypeNameForTypeReference(typeReference.asSimpleTypeReference().orElseThrow(), true); | ||
| typeName = CodeBlock.of("$L", StaticHelper.CAPITALIZE(languageTypeNameForTypeReference.toString())); | ||
| } | ||
| return CodeBlock.of("_value.getStruct().get($S).get$L()", fieldName, typeName); | ||
| } |
There was a problem hiding this comment.
In getGetValueBlockForCase, the case \"List\" block can fall through into case \"Struct\" when typeReference is not an array type (no return/break at the end of the List case). This will generate incorrect accessors for list values. Additionally, building typeName via languageTypeNameForTypeReference.toString() can produce fully-qualified names (e.g., java.lang.Integer) which would generate invalid getter calls like PlcValue::getJava.lang.Integer. Add an explicit return/fallback for non-array lists, and derive the getter suffix from a controlled mapping (e.g., based on PlcValueType or ClassName.simpleName() with special-cases for primitives/wrappers) instead of TypeName.toString().
| CodeBlock getValueBlock = CodeBlock.of("$L", field.asNamedField().orElse(() -> "unnamed").getName()); | ||
| switch (field.getTypeName()) { | ||
| case "array": { | ||
| generateArrayField(complexTypeDefinition, field, fieldIndex, allParserArguments.orElse(Collections.emptyList()), getValueBlock, getValueBlock, codeBlocksParse, codeBlocksSerialize, codeBlocksGetLengthInBits); | ||
| if (field.asArrayField().orElseThrow().getType().asArrayTypeReference().orElseThrow().getElementTypeReference().isByteBased()) { | ||
| constructorArgs.put(field.asNamedField().orElse(() -> "unnamed").getName(), ArrayTypeName.of(TypeName.BYTE)); | ||
| } else { | ||
| constructorArgs.put(field.asNamedField().orElse(() -> "unnamed").getName(), getFieldTypeClassName(field)); |
There was a problem hiding this comment.
Optional.orElse(...) expects a NamedField value, but this passes a lambda () -> \"unnamed\", which won’t compile (and then calls .getName() on the lambda). Use field.asNamedField().map(NamedField::getName).orElse(\"unnamed\") (or similar) to safely obtain the name string.
| CodeBlock getValueBlock = CodeBlock.of("$L", field.asNamedField().orElse(() -> "unnamed").getName()); | |
| switch (field.getTypeName()) { | |
| case "array": { | |
| generateArrayField(complexTypeDefinition, field, fieldIndex, allParserArguments.orElse(Collections.emptyList()), getValueBlock, getValueBlock, codeBlocksParse, codeBlocksSerialize, codeBlocksGetLengthInBits); | |
| if (field.asArrayField().orElseThrow().getType().asArrayTypeReference().orElseThrow().getElementTypeReference().isByteBased()) { | |
| constructorArgs.put(field.asNamedField().orElse(() -> "unnamed").getName(), ArrayTypeName.of(TypeName.BYTE)); | |
| } else { | |
| constructorArgs.put(field.asNamedField().orElse(() -> "unnamed").getName(), getFieldTypeClassName(field)); | |
| String fieldName = field.asNamedField().map(NamedField::getName).orElse("unnamed"); | |
| CodeBlock getValueBlock = CodeBlock.of("$L", fieldName); | |
| switch (field.getTypeName()) { | |
| case "array": { | |
| generateArrayField(complexTypeDefinition, field, fieldIndex, allParserArguments.orElse(Collections.emptyList()), getValueBlock, getValueBlock, codeBlocksParse, codeBlocksSerialize, codeBlocksGetLengthInBits); | |
| if (field.asArrayField().orElseThrow().getType().asArrayTypeReference().orElseThrow().getElementTypeReference().isByteBased()) { | |
| constructorArgs.put(fieldName, ArrayTypeName.of(TypeName.BYTE)); | |
| } else { | |
| constructorArgs.put(fieldName, getFieldTypeClassName(field)); |
| String parentTypeName = complexTypeDefinition.getParentType().orElseThrow().getName(); | ||
| // Add the builder implementation | ||
| TypeSpec.Builder builderImplementationBuilder = TypeSpec.classBuilder(parentTypeName + "BuilderImpl") | ||
| .addSuperinterface(ClassName.get(targetPackage, parentTypeName + "." + parentTypeName + "Builder")) |
There was a problem hiding this comment.
This attempts to create a ClassName using a dotted name in the 'simple name' position (\"Parent.ParentBuilder\"). JavaPoet ClassName.get(package, className) is for top-level types and generally does not accept dots in the class name; it can produce invalid output or throw at runtime. Use ClassName.get(targetPackage, parentTypeName).nestedClass(parentTypeName + \"Builder\") (or ClassName.get(targetPackage, parentTypeName, parentTypeName + \"Builder\")) to reference the nested builder interface correctly.
| .addSuperinterface(ClassName.get(targetPackage, parentTypeName + "." + parentTypeName + "Builder")) | |
| .addSuperinterface(ClassName.get(targetPackage, parentTypeName).nestedClass(parentTypeName + "Builder")) |
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| # | ||
| org.apache.plc4x.codegeneration.language.java.JavaLanguageOutput |
There was a problem hiding this comment.
The java-jp module registers org.apache.plc4x.codegeneration.language.java.JavaLanguageOutput, which strongly suggests it shares the same fully-qualified class name as the existing code-generation/language/java implementation. If both artifacts end up on the classpath, this becomes a classpath collision (one class shadows the other) and can make ServiceLoader behavior non-deterministic. Consider renaming the package and/or the implementation class for the JavaPoet variant (e.g., ...language.java.jp.JavaPoetLanguageOutput) and updating this service entry accordingly to ensure both variants can coexist safely during the migration.
| .addModifiers(Modifier.PUBLIC) | ||
| .addJavadoc("Code generated by code-generation. DO NOT EDIT.\n"); | ||
|
|
||
| // Generate some java constants for holing their values |
There was a problem hiding this comment.
Corrected spelling of 'holing' to 'holding'.
| // Generate some java constants for holing their values | |
| // Generate some java constants for holding their values |
| // Write to the configured output root directory; JavaPoet will create package folders. | ||
| javaFile.writeTo(outputRootDir); | ||
| } catch (IOException e) { | ||
| LOGGER.error("Error generating enum {}", dataIoTypeDefinition.getName(), e); |
There was a problem hiding this comment.
This log message says "Error generating enum" but this generator is producing a DataIo type. Update the message to reflect the actual generated artifact (e.g., "data-io"/"DataIo type") to make failures diagnosable.
| LOGGER.error("Error generating enum {}", dataIoTypeDefinition.getName(), e); | |
| LOGGER.error("Error generating DataIo type {}", dataIoTypeDefinition.getName(), e); |
| // Write to the configured output root directory; JavaPoet will create package folders. | ||
| javaFile.writeTo(outputRootDir); | ||
| } catch (IOException e) { | ||
| LOGGER.error("Error generating enum {}", complexTypeDefinition.getName(), e); |
There was a problem hiding this comment.
This log message says "Error generating enum" but this generator is producing a complex type. Adjust the message to "Error generating complex type" (or similar) for accurate diagnostics.
| LOGGER.error("Error generating enum {}", complexTypeDefinition.getName(), e); | |
| LOGGER.error("Error generating complex type {}", complexTypeDefinition.getName(), e); |
| // Write to the configured output root directory; JavaPoet will create package folders. | ||
| javaFile.writeTo(outputRootDir); | ||
| } catch (IOException e) { | ||
| LOGGER.error("Error generating enum {}", constantsTypeDefinition.getName(), e); |
There was a problem hiding this comment.
This log message says "Error generating enum" but this generator is producing a constants type. Update the message to match what’s being generated to avoid confusion during troubleshooting.
| LOGGER.error("Error generating enum {}", constantsTypeDefinition.getName(), e); | |
| LOGGER.error("Error generating constants type {}", constantsTypeDefinition.getName(), e); |
|
wow awesome work @chrisdutz. Going through the changes, CoPilot also found some interessting things, but one thing that jumped to my eye was the use of the package name |
|
I just took the liberty to run the manual test benchmark in both the old and the new driver: Old driver: New driver: The new connection handling is quite a bit faster than the old Netty version. However, I see the new driver is in average 1ms/request slower ... will look into what I could do in order to improve that. |
|
Ok ... after the last commit, the new driver performs even a bit better than the old one: |
sruehl
force-pushed
the
refactor/spi3
branch
2 times, most recently
from
0df2299 to
959b2aa
Compare
|
Hi @chrisdutz , I am doing some test on s7 to read some variables and it is not working. It does not even connect to the Plc. The same test is working using the code of develop branch. To run the test i needed to change at s7Driver file like shown below because the class does not exist in the repo. Is it exist in your development environment? org.apache.plc4x.java.s7.readwrite.S7Driver -> org.apache.plc4x.java.s7.S7Driver Thanks, |
Did you check out the branch and build the entire project? I did move the locations of the driver classes.. But if you bike the full project it should work. Chris |
Yes, the entire project builds successfully. However, I am also testing other drivers and ran into an issue. The ServiceLoader is throwing an exception because the class path listed in s7/META-INF/services is incorrect. To fix it locally, I had to modify the mapping: From: org.apache.plc4x.java.s7.readwrite.S7Driver (does not exist) It seems either the META-INF/services file is pointing to the wrong package, or the readwrite.S7Driver class was accidentally left out of the repository. Could you please check which one is correct? Once iI know I can continue my tests for s7 PLC. Thanks, |
|
Ok ... so I've finished porting all drivers and merged in all recent changes from develop ... I'll be merging this PR back to develop in a few days. |
o Implement new Read-/WriteBuffers that work without third party dependencies
o Implement an updated code-generation framework
o Implement a new system for pluggable transports
o Implement the code for the concept of layered protocol drivers
- Update existing drivers to the new SPI
|
TY for the PR, changes have been merged into main. Great work |
Hi @chrisdutz , Did you have a chance to check my comment above? Maybe I am missing something. Thank you, |
|
Hi @andvasp ... I just had another look and you were right ... the services file was pointing to the wrong class ... but the thing that really puzzles me, is that I was running the tests in the tests manual package and they use the service-lookup and were working fine. Thanks for bringing this back up ... I'll fix it in develop. Chris |
|
Hi @chrisdutz, I have completed the initial tests for this refactoring using the HelloPlc4xRead example from plc4x-extras. I compared the develop branch after the merge (commit e095044) against the version before the merge (commit a495bbf). Here are the results: OPC UA: Working on both versions. I will investigating further. Notes & Issues: ╔═S7Message═════════════════════════════════════════════════════════════════════════════════════════════════════════╗ Best regards, |
Could you please explain a bit more what exactly is not working and how it's failing? With only this bit of information I unfortunately can't help much ... and could you please open Issues for each driver that's failing? This makes tracking them down a lot simpler. And regarding the output ... in the old SPI we pulled in all of the stuff in one big jar. That contained the normal output, the json output, the xml output and the box-based output (which you are referring to) ... the SPI3 I made a lot more modular. So I opted out for having a hard dependency on the box-based layout for this sort of thing and added the concept of the audit-log. The driver calls audit log output calls which default to a no-op thing unless you add a dependency to the audit-log-impl in your application and configure the audit log via your connection string. Then it's a lot more verbose. I should probably add a documentation on the audit-log on our website. |
Hi @chrisdutz! This was just a brief report to keep you in the loop. No need to worry about helping me just yet. I will investigate further and open a issue with more details if it turns out to be a real bug and not just my test. Thanks! |
In this refactoring, we are replacing the existing Netty-based core into a set of lightweight sub-modules without requiring Netty at all and with as little third party dependencies as possible.
This will allow using our code-generation framework independently from building PLC4J drivers and allow porting PLC4X to other languages as we no longer rely on external libraries for which counterparts need to be integrated for new languages.