SPI 3 Refactoring

[chrisdutz]

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.

[sruehl]

really like the new syntax... :)

[splatch]

@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 (Reader/Writer)Buffer, thus this PR does not touch netty part yet. I believe we still need other half of the changes.

[chrisdutz]

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.

[chrisdutz]

[splatch]

Yeah, github UI couldn't get more than 3000 files, transports fall apart from diff. Will have a look on them!

[sruehl]

Changes look good, awesome work, keep them coming :)

[chrisdutz]

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.

[Copilot]

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.

[Copilot]

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.

Suggested change

	<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>

[Copilot]

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().

[Copilot]

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.

Suggested change

	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));

[Copilot]

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.

Suggested change

	.addSuperinterface(ClassName.get(targetPackage, parentTypeName + "." + parentTypeName + "Builder"))
	.addSuperinterface(ClassName.get(targetPackage, parentTypeName).nestedClass(parentTypeName + "Builder"))

[Copilot]

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.

[Copilot]

Corrected spelling of 'holing' to 'holding'.

Suggested change

	// Generate some java constants for holing their values
	// Generate some java constants for holding their values

[Copilot]

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.

Suggested change

	LOGGER.error("Error generating enum {}", dataIoTypeDefinition.getName(), e);
	LOGGER.error("Error generating DataIo type {}", dataIoTypeDefinition.getName(), e);

[Copilot]

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.

Suggested change

	LOGGER.error("Error generating enum {}", complexTypeDefinition.getName(), e);
	LOGGER.error("Error generating complex type {}", complexTypeDefinition.getName(), e);

[Copilot]

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.

Suggested change

	LOGGER.error("Error generating enum {}", constantsTypeDefinition.getName(), e);
	LOGGER.error("Error generating constants type {}", constantsTypeDefinition.getName(), e);

[sruehl]

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 functions. I would propose to name it operations to not confuse it with functions used in programming.

[chrisdutz]

I just took the liberty to run the manual test benchmark in both the old and the new driver:

Old driver:

=== Test Summary ===
Single Item:
  Average Read Time:  13,80 ms (15 requests, 207 ms total)
  Average Write Time: 12,00 ms (15 requests, 180 ms total)
  Average Time:       12,90 ms
Multi Item (each 15 items):
  Average Read Time:  11,81 ms (100 requests, 1181 ms total)
  Average Write Time: 175,98 ms (100 requests, 17598 ms total)
  Average Time:       93,90 ms
======================
Test executed in 21570ms

New driver:

=== Test Summary ===
Protocol:  Modbus TCP (modbus-tcp)
Transport: TCP (tcp)
Single Item:
  Average Read Time:  14,60 ms (15 requests, 219 ms total)
  Average Write Time: 11,53 ms (15 requests, 173 ms total)
  Average Time:       13,07 ms
#Multi Item (each 15 items):
  Average Read Time:  16,47 ms (100 requests, 1647 ms total)
  Average Write Time: 174,59 ms (100 requests, 17459 ms total)
  Average Time:       95,53 ms
======================
Test executed in 19863ms

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.