sruehl commented on code in PR #1179:
URL: https://github.com/apache/plc4x/pull/1179#discussion_r1375982833
##########
plc4j/drivers/ads/pom.xml:
##########
@@ -140,11 +140,6 @@
<artifactId>plc4j-transport-tcp</artifactId>
<version>0.12.0-SNAPSHOT</version>
</dependency>
- <dependency>
- <groupId>org.apache.plc4x</groupId>
- <artifactId>plc4j-transport-serial</artifactId>
Review Comment:
how is this related to this PR? or is that just a simple cleanup?
##########
plc4j/drivers/ab-eth/src/main/java/org/apache/plc4x/java/abeth/configuration/AbEthConfiguration.java:
##########
@@ -18,12 +18,10 @@
*/
package org.apache.plc4x.java.abeth.configuration;
-import org.apache.plc4x.java.abeth.AbEthDriver;
import org.apache.plc4x.java.spi.configuration.Configuration;
import
org.apache.plc4x.java.spi.configuration.annotations.ConfigurationParameter;
-import org.apache.plc4x.java.transport.tcp.TcpTransportConfiguration;
-public class AbEthConfiguration implements Configuration,
TcpTransportConfiguration {
Review Comment:
instead of separating it you could have just added extends
`DefaultTcpTransportConfiguration` but I don't see a issue in having a separate
class `AbEthTcpTransportConfiguration`
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/connection/CustomProtocolStackConfigurer.java:
##########
@@ -101,7 +102,7 @@ public Plc4xProtocolBase<BASE_PACKET_CLASS>
configurePipeline(Configuration conf
if (driverContext != null) {
protocol.setDriverContext(driverContext);
}
- Plc4xNettyWrapper<BASE_PACKET_CLASS> context = new
Plc4xNettyWrapper<>(configuration.getTimeoutManager(), pipeline, passive,
protocol, authentication, basePacketClass);
+ Plc4xNettyWrapper<BASE_PACKET_CLASS> context = new
Plc4xNettyWrapper<>(new NettyHashTimerTimeoutManager(), pipeline, passive,
protocol, authentication, basePacketClass);
Review Comment:
what is this change about?
##########
plc4j/spi/src/test/java/org/apache/plc4x/java/spi/configuration/ConfigurationFactoryTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.plc4x.java.spi.configuration;
+
+import org.apache.plc4x.java.spi.configuration.config.*;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+public class ConfigurationFactoryTest {
Review Comment:
nice, tests... :)
##########
plc4j/transports/pcap-shared/src/main/java/org/apache/plc4x/java/transport/pcap/PcapTransportConfiguration.java:
##########
@@ -19,22 +19,18 @@
package org.apache.plc4x.java.transport.pcap;
import org.apache.plc4x.java.spi.transport.TransportConfiguration;
-import org.apache.plc4x.java.utils.pcap.netty.config.PcapChannelConfig;
import org.apache.plc4x.java.utils.pcap.netty.handlers.PacketHandler;
public interface PcapTransportConfiguration extends TransportConfiguration {
+ int NO_DEFAULT_PORT = -1;
- default boolean getSupportVlans() {
- return false;
- }
+ boolean getSupportVlans();
default int getDefaultPort() {
- return PcapChannelConfig.ALL_PORTS;
+ return NO_DEFAULT_PORT;
Review Comment:
why is this logic change here?
##########
plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/configuration/AdsConfiguration.java:
##########
@@ -19,22 +19,17 @@
package org.apache.plc4x.java.ads.configuration;
import org.apache.commons.lang3.ArrayUtils;
-import org.apache.plc4x.java.ads.readwrite.AdsConstants;
import org.apache.plc4x.java.ads.readwrite.AmsNetId;
import org.apache.plc4x.java.spi.configuration.Configuration;
import org.apache.plc4x.java.spi.configuration.ConfigurationParameterConverter;
-import
org.apache.plc4x.java.spi.configuration.annotations.ConfigurationParameter;
-import org.apache.plc4x.java.spi.configuration.annotations.ParameterConverter;
-import org.apache.plc4x.java.spi.configuration.annotations.Required;
+import org.apache.plc4x.java.spi.configuration.annotations.*;
import
org.apache.plc4x.java.spi.configuration.annotations.defaults.BooleanDefaultValue;
import
org.apache.plc4x.java.spi.configuration.annotations.defaults.IntDefaultValue;
-import org.apache.plc4x.java.transport.serial.SerialTransportConfiguration;
-import org.apache.plc4x.java.transport.tcp.TcpTransportConfiguration;
import java.util.regex.Pattern;
import java.util.stream.Stream;
-public class AdsConfiguration implements Configuration,
TcpTransportConfiguration, SerialTransportConfiguration {
+public class AdsConfiguration implements Configuration {
Review Comment:
ok guess it is because here you need to extend 2 types... In Golang this
could be also done with delegates but in Java world it seem easier to just
throw it in 2 classes 👍🏼
##########
plc4j/drivers/c-bus/src/main/java/org/apache/plc4x/java/cbus/configuration/CBusTcpTransportConfiguration.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.plc4x.java.cbus.configuration;
+
+import org.apache.plc4x.java.transport.tcp.DefaultTcpTransportConfiguration;
+
+public class CBusTcpTransportConfiguration extends
DefaultTcpTransportConfiguration {
+
+ @Override
+ public int getDefaultPort() {
+ return 123;
Review Comment:
One thing noticing here. Guess this is a placeholder but there is a constant
in the mspec for that
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/connection/SingleProtocolStackConfigurer.java:
##########
@@ -111,7 +112,7 @@ public Plc4xProtocolBase<BASE_PACKET_CLASS>
configurePipeline(Configuration conf
if (driverContextClass != null) {
protocol.setDriverContext(configure(configuration,
createInstance(driverContextClass)));
}
- Plc4xNettyWrapper<BASE_PACKET_CLASS> context = new
Plc4xNettyWrapper<>(configuration.getTimeoutManager(), pipeline, passive,
protocol,
+ Plc4xNettyWrapper<BASE_PACKET_CLASS> context = new
Plc4xNettyWrapper<>(new NettyHashTimerTimeoutManager(), pipeline, passive,
protocol,
Review Comment:
same as above: why is this change?
##########
plc4j/drivers/firmata/pom.xml:
##########
@@ -122,12 +122,6 @@
<version>0.12.0-SNAPSHOT</version>
</dependency>
- <dependency>
- <groupId>org.apache.plc4x</groupId>
- <artifactId>plc4j-transport-serial</artifactId>
Review Comment:
same here, why is the dependency disappearing
##########
plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/configuration/AdsConfiguration.java:
##########
@@ -19,22 +19,17 @@
package org.apache.plc4x.java.ads.configuration;
import org.apache.commons.lang3.ArrayUtils;
-import org.apache.plc4x.java.ads.readwrite.AdsConstants;
import org.apache.plc4x.java.ads.readwrite.AmsNetId;
import org.apache.plc4x.java.spi.configuration.Configuration;
import org.apache.plc4x.java.spi.configuration.ConfigurationParameterConverter;
-import
org.apache.plc4x.java.spi.configuration.annotations.ConfigurationParameter;
-import org.apache.plc4x.java.spi.configuration.annotations.ParameterConverter;
-import org.apache.plc4x.java.spi.configuration.annotations.Required;
+import org.apache.plc4x.java.spi.configuration.annotations.*;
import
org.apache.plc4x.java.spi.configuration.annotations.defaults.BooleanDefaultValue;
import
org.apache.plc4x.java.spi.configuration.annotations.defaults.IntDefaultValue;
-import org.apache.plc4x.java.transport.serial.SerialTransportConfiguration;
-import org.apache.plc4x.java.transport.tcp.TcpTransportConfiguration;
import java.util.regex.Pattern;
import java.util.stream.Stream;
-public class AdsConfiguration implements Configuration,
TcpTransportConfiguration, SerialTransportConfiguration {
+public class AdsConfiguration implements Configuration {
Review Comment:
ok guess it is because here you need to extend 2 types... In Golang this
could be also done with delegates but in Java world it seem easier to just
throw it in 2 classes 👍🏼
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/transport/Transport.java:
##########
@@ -28,4 +28,8 @@ public interface Transport {
ChannelFactory createChannelFactory(String transportConfig);
+ default Class<? extends TransportConfiguration> getTransportConfigType() {
+ return null;
Review Comment:
we could also consider the use of optional, depending on how it is use (not
a issue with null)
##########
plc4j/drivers/ab-eth/src/main/java/org/apache/plc4x/java/abeth/configuration/AbEthConfiguration.java:
##########
@@ -18,12 +18,10 @@
*/
package org.apache.plc4x.java.abeth.configuration;
-import org.apache.plc4x.java.abeth.AbEthDriver;
import org.apache.plc4x.java.spi.configuration.Configuration;
import
org.apache.plc4x.java.spi.configuration.annotations.ConfigurationParameter;
-import org.apache.plc4x.java.transport.tcp.TcpTransportConfiguration;
-public class AbEthConfiguration implements Configuration,
TcpTransportConfiguration {
Review Comment:
Guess it is for consistency... see comment below on the other class
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/connection/GeneratedDriverBase.java:
##########
@@ -150,8 +153,24 @@ public PlcConnection getConnection(String
connectionString, PlcAuthentication au
throw new PlcConnectionException("Unsupported transport " +
transportCode);
}
- // Inject the configuration into the transport.
- configure(configuration, transport);
+ // Find out the type of the transport configuration.
+ Class<? extends TransportConfiguration> transportConfigurationType =
transport.getTransportConfigType();
+ if(this instanceof TransportConfigurationTypeProvider) {
+ TransportConfigurationTypeProvider
transportConfigurationTypeProvider =
+ (TransportConfigurationTypeProvider) this;
+ Class<? extends TransportConfiguration>
driverTransportConfigurationType =
+
transportConfigurationTypeProvider.getTransportConfigurationType(transportCode);
+ if(driverTransportConfigurationType != null) {
Review Comment:
maybe some trace/debug level logging might be helpful here at some point
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/transport/TransportConfigurationTypeProvider.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.plc4x.java.spi.transport;
+
+public interface TransportConfigurationTypeProvider {
+
+ Class<? extends TransportConfiguration>
getTransportConfigurationType(String transportCode);
Review Comment:
see optional comment above
##########
plc4j/drivers/c-bus/src/main/java/org/apache/plc4x/java/cbus/configuration/CBusTcpTransportConfiguration.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.plc4x.java.cbus.configuration;
+
+import org.apache.plc4x.java.transport.tcp.DefaultTcpTransportConfiguration;
+
+public class CBusTcpTransportConfiguration extends
DefaultTcpTransportConfiguration {
+
+ @Override
+ public int getDefaultPort() {
+ return 123;
Review Comment:
or should be....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
