You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by GitBox <gi...@apache.org> on 2021/04/19 23:50:54 UTC
[GitHub] [plc4x] splatch opened a new pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
splatch opened a new pull request #241:
URL: https://github.com/apache/plc4x/pull/241
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
splatch commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616461714
##########
File path: plc4j/spi/src/main/java/org/apache/plc4x/java/spi/EventListenerMessageCodec.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.handler.codec.MessageToMessageCodec;
+import org.apache.plc4x.java.api.listener.EventListener;
+import org.apache.plc4x.java.api.listener.MessageExchangeListener;
+import org.apache.plc4x.java.spi.generation.Message;
+
+import java.util.List;
+
+/**
+ * Codec which propagate received or sent messages to connection event listeners.
+ */
+public class EventListenerMessageCodec extends MessageToMessageCodec<Message, Message> {
+
+ private final List<EventListener> listeners;
+
+ public EventListenerMessageCodec(List<EventListener> listeners) {
+ this.listeners = listeners;
+ }
+
+ @Override
+ protected void encode(ChannelHandlerContext ctx, Message msg, List<Object> out) throws Exception {
+ if (msg instanceof Message) {
Review comment:
It really depends on order in which codecs are added - this was an extra fence I made while looking right place to add codec. So yeah, this `if` statement can be safely removed now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616478823
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/EventPlcConnection.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.api;
+
+import org.apache.plc4x.java.api.listener.EventListener;
+
+/**
+ * An extended kind of connection which can broadcast events.
+ *
+ * Particular type of event might vary, it might be a connection state or message exchange.
+ */
+public interface EventPlcConnection extends PlcConnection {
Review comment:
I mean @splatch has a valid point regarding the pool, so maybe we can merge these to together later when we have full support. Otherwise this feature would be blocked till we have support in pool for example. From an interface perspective this should be fine as we don't expose the api yet when it's not guaranteed to work the intended way. Once this is validated we can just pull the methods up into the PlcConnection. Implementation wise merging later should have almost now effect when done later then.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616418042
##########
File path: plc4j/drivers/canopen/src/main/java/org/apache/plc4x/java/canopen/CANOpenPlcDriver.java
##########
@@ -111,8 +112,8 @@ protected BaseOptimizer getOptimizer() {
}
@Override
- protected ProtocolStackConfigurer<CANOpenFrame> getStackConfigurer() {
- return SingleProtocolStackConfigurer.builder(CANOpenFrame.class, CANOpenSocketCANFrameIO.class)
+ protected ProtocolStackConfigurer<CANOpenFrame> getStackConfigurer(Configuration configuration, Transport transport) {
+ return SingleProtocolStackConfigurer.builder(CANOpenFrame.class, CANOpenSocketCANFrameIO.class, configuration)
.withProtocol(CANOpenProtocolLogic.class)
Review comment:
@splatch Wouldn't it have been easier to add 2 new methods here instead of refactoring the whole spi?
like:
```suggestion
.withProtocol(CANOpenProtocolLogic.class)
.withConfiguration(configuration)
.withTransport(transport)
```
what is confusing to me is that you changed the SPI for CanOpen but you don't make use of it here
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616449259
##########
File path: plc4j/spi/src/main/java/org/apache/plc4x/java/spi/connection/SingleProtocolStackConfigurer.java
##########
@@ -42,69 +47,57 @@
public class SingleProtocolStackConfigurer<BASE_PACKET_CLASS extends Message> implements ProtocolStackConfigurer<BASE_PACKET_CLASS> {
Review comment:
The whole idea of the `SingleProtocolStackConfigurer ` is to work class based.
So maybe we rename this configurer into `SingleDIProtocolStackConfigurer` and we create a second `SingleSupplierProtocolStackConfigurer`.
Like now you force every stack to use the new api instead of providing an alternate for for drivers where its neccessary.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616472152
##########
File path: plc4j/spi/src/main/java/org/apache/plc4x/java/spi/EventListenerMessageCodec.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.handler.codec.MessageToMessageCodec;
+import org.apache.plc4x.java.api.listener.EventListener;
+import org.apache.plc4x.java.api.listener.MessageExchangeListener;
+import org.apache.plc4x.java.spi.generation.Message;
+
+import java.util.List;
+
+/**
+ * Codec which propagate received or sent messages to connection event listeners.
+ */
+public class EventListenerMessageCodec extends MessageToMessageCodec<Message, Message> {
+
+ private final List<EventListener> listeners;
+
+ public EventListenerMessageCodec(List<EventListener> listeners) {
+ this.listeners = listeners;
+ }
+
+ @Override
+ protected void encode(ChannelHandlerContext ctx, Message msg, List<Object> out) throws Exception {
+ if (msg instanceof Message) {
Review comment:
No ... I think it doesn't ... right now the only option for which this isn't always true is if you pass in "msg = null".
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616419965
##########
File path: plc4j/spi/src/main/java/org/apache/plc4x/java/spi/connection/SingleProtocolStackConfigurer.java
##########
@@ -142,31 +141,86 @@ public SingleProtocolStackBuilder(Class<BASE_PACKET_CLASS> basePacketClass, Clas
}
public SingleProtocolStackBuilder<BASE_PACKET_CLASS> withProtocol(Class<? extends Plc4xProtocolBase<BASE_PACKET_CLASS>> protocol) {
- this.protocol = protocol;
+ return withProtocol(configuredType(protocol));
Review comment:
I have to admit, that I would prefer if we refactored the existing drivers to use the new version instead of doing this.
##########
File path: plc4j/spi/src/main/java/org/apache/plc4x/java/spi/EventListenerMessageCodec.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.handler.codec.MessageToMessageCodec;
+import org.apache.plc4x.java.api.listener.EventListener;
+import org.apache.plc4x.java.api.listener.MessageExchangeListener;
+import org.apache.plc4x.java.spi.generation.Message;
+
+import java.util.List;
+
+/**
+ * Codec which propagate received or sent messages to connection event listeners.
+ */
+public class EventListenerMessageCodec extends MessageToMessageCodec<Message, Message> {
+
+ private final List<EventListener> listeners;
+
+ public EventListenerMessageCodec(List<EventListener> listeners) {
+ this.listeners = listeners;
+ }
+
+ @Override
+ protected void encode(ChannelHandlerContext ctx, Message msg, List<Object> out) throws Exception {
+ if (msg instanceof Message) {
Review comment:
Isn't this trivially true ... I mean ... if msg is of type Message, it will probably also match this?
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/EventPlcConnection.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.api;
+
+import org.apache.plc4x.java.api.listener.EventListener;
+
+/**
+ * An extended kind of connection which can broadcast events.
+ *
+ * Particular type of event might vary, it might be a connection state or message exchange.
+ */
+public interface EventPlcConnection extends PlcConnection {
Review comment:
And in addition, we could use the opportunity to add more events ... like "Connected" event "ConnectionFailed", "Identified" (fired as soon as a connection has identified the remote PLC type (Might be interesting for the users), "MetadataChanged", ...
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/EventPlcConnection.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.api;
+
+import org.apache.plc4x.java.api.listener.EventListener;
+
+/**
+ * An extended kind of connection which can broadcast events.
+ *
+ * Particular type of event might vary, it might be a connection state or message exchange.
+ */
+public interface EventPlcConnection extends PlcConnection {
Review comment:
I think as this is in the API module, there shouldn't be an event-emitting set of connections and non emitting ones. I would rather extend the PlcConnection with these two methods and have them implemented in all drivers.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r679308642
##########
File path: protocols/socketcan/pom.xml
##########
@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+ -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.plc4x</groupId>
+ <artifactId>plc4x-protocols</artifactId>
+ <version>0.9.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>plc4x-protocols-socketcan</artifactId>
+
+ <name>Protocols: CANopen</name>
+ <description>Protocol of binary representation of SocketCAN frames.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.plc4x</groupId>
+ <artifactId>plc4x-code-generation-protocol-base-mspec</artifactId>
+ <version>0.9.0-SNAPSHOT</version>
+ </dependency>
+ </dependencies>
+
+</project>
Review comment:
EOL missing
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch merged pull request #241: PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
splatch merged pull request #241:
URL: https://github.com/apache/plc4x/pull/241
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
splatch commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616467389
##########
File path: plc4j/spi/src/main/java/org/apache/plc4x/java/spi/connection/SingleProtocolStackConfigurer.java
##########
@@ -42,69 +47,57 @@
public class SingleProtocolStackConfigurer<BASE_PACKET_CLASS extends Message> implements ProtocolStackConfigurer<BASE_PACKET_CLASS> {
Review comment:
I have removed configuration handling from configurer cause now it is available to builder. Given above we now can have multiple builders which construct stack in different ways. One note, leaving `SingleProtocolStackConfigurer` with its initial class based approach will require us to introduce second type which will differ only by single lines. These lines will be calls to suppliers instead of reflection.
Are you suggesting to revert `SingleSupplierProtocolStackConfigurer` and duplicate its functionality to other place?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616474494
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/EventPlcConnection.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.api;
+
+import org.apache.plc4x.java.api.listener.EventListener;
+
+/**
+ * An extended kind of connection which can broadcast events.
+ *
+ * Particular type of event might vary, it might be a connection state or message exchange.
+ */
+public interface EventPlcConnection extends PlcConnection {
Review comment:
Well if you change the core of things it is natural that they need changes in a lot of places ;-) ... the later you introduce the change, the more you need to change. But I would not be in favor of a change that makes our uses have to do instanceof stuff in order to use the API.
You wouldn't have to do all on your own. We could probably all do our part. I do think that some sort of "ups I think I just lost the connection" mechanism is indeed very important.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r679308115
##########
File path: protocols/canopen/src/main/resources/protocols/can/canopen.mspec
##########
@@ -352,3 +352,12 @@
[simple IndexAddress 'address']
[array int 8 'data' count '4']
]
+
+// A compact, byte aligned structure for test and embedding purposes
+[type 'CANOpenFrame'
+ [simple uint 8 'nodeId']
+ [simple CANOpenService 'service']
+ [const uint 4 'alignment' '0x00']
+ [simple CANOpenPayload 'payload' ['service']]
+ [padding uint 8 'alignment' '0x00' '8 - (payload.lengthInBytes)']
+]
Review comment:
EOL missing
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
splatch commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616485176
##########
File path: plc4j/drivers/canopen/src/main/java/org/apache/plc4x/java/canopen/CANOpenPlcDriver.java
##########
@@ -111,8 +112,8 @@ protected BaseOptimizer getOptimizer() {
}
@Override
- protected ProtocolStackConfigurer<CANOpenFrame> getStackConfigurer() {
- return SingleProtocolStackConfigurer.builder(CANOpenFrame.class, CANOpenSocketCANFrameIO.class)
+ protected ProtocolStackConfigurer<CANOpenFrame> getStackConfigurer(Configuration configuration, Transport transport) {
+ return SingleProtocolStackConfigurer.builder(CANOpenFrame.class, CANOpenSocketCANFrameIO.class, configuration)
.withProtocol(CANOpenProtocolLogic.class)
Review comment:
> what is confusing to me is that you changed the SPI for CanOpen but you don't make use of it here
Changes for canopen will follow. There are changes necessary to transport layer which must be made in order to do anything more. Shape of this part for canopen stack configuration could then look more like this:
```java
@Override
protected ProtocolStackConfigurer<CANOpenFrame> getStackConfigurer(Configuration configuration, Transport transport) {
if (!(transport instanceof CANBusTransport)) {
throw new IllegalArgumentException("CANopen requires CANBusTransport.");
}
CANBusTransport canTransport = (CANBusTransport) transport;
return SingleProtocolStackConfigurer.builder(CANOpenFrame.class, canTransport.getWireType(), configuration)
// Frame builder allows to construct CAN frame valid in context of given transport in a way
// which is managed from start to end by transport itself
.withProtocol(() -> new CANOpenProtocolLogic(canTransport.getFrameBuilder()))
.withDriverContext(CANOpenDriverContext.class)
.withPacketSizeEstimator(canTransport.getPacketSizeEstimator())
.littleEndian()
.build();
}
```
```java
interface CANTransport extends Transport {
Class<? extends Message> getWireType();
Class<? extends ToIntFunction<ByteBuf>> getPacketSizeEstimator();
CANBusFrameBuilder getFrameBuilder();
}
```
I am not 100% sure if this will be sufficient ATM as I will find it only when I will start doing it.
Keep in mind that most of above changes is intended to decouple configurer from configuration and give a bit more control to driver itself on how the stack is made. At the moment driver can not do anything conditionally depending on transport leading to situations when we can not effectively support cases which can rely on serial and tcp transport with same payload (Ams/Modbus/CAN).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
splatch commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616462636
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/EventPlcConnection.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.api;
+
+import org.apache.plc4x.java.api.listener.EventListener;
+
+/**
+ * An extended kind of connection which can broadcast events.
+ *
+ * Particular type of event might vary, it might be a connection state or message exchange.
+ */
+public interface EventPlcConnection extends PlcConnection {
Review comment:
> I think as this is in the API module, there shouldn't be an event-emitting set of connections and non emitting ones
I wasn't sure of that because of amount of changes it will force in connection pool module. Logic in pooled connections is a bit different and I am not entirely sure if I will be able to deliver the same functionality.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616484062
##########
File path: plc4j/spi/src/main/java/org/apache/plc4x/java/spi/connection/SingleProtocolStackConfigurer.java
##########
@@ -42,69 +47,57 @@
public class SingleProtocolStackConfigurer<BASE_PACKET_CLASS extends Message> implements ProtocolStackConfigurer<BASE_PACKET_CLASS> {
Review comment:
oh and btw.: I would recommend to let the supplier accept the configuration as an argument. This way you could always decided for yourself if you would work with the configuration or just ignore it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r679306045
##########
File path: plc4j/drivers/can/src/test/java/org/apache/plc4x/java/can/generic/field/GenericCANFieldHandlerTest.java
##########
@@ -0,0 +1,18 @@
+package org.apache.plc4x.java.can.generic.field;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+class GenericCANFieldHandlerTest {
+
+ @Test
+ void checkFieldSyntax() {
+ GenericCANField field = new GenericCANFieldHandler().createField("200:BYTE[8]");
+
+ assertEquals(200, field.getNodeId());
+ assertEquals("BYTE", field.getPlcDataType());
+ assertEquals(8, field.getArraySize());
+ }
+
+}
Review comment:
EOL is missing here
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r679309020
##########
File path: protocols/socketcan/src/main/resources/META-INF/services/org.apache.plc4x.plugins.codegenerator.protocol.Protocol
##########
@@ -0,0 +1,19 @@
+#
+# 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.
+#
+org.apache.plc4x.protocol.socketcan.SocketCANProtocol
Review comment:
EOL missing
##########
File path: protocols/socketcan/src/main/resources/protocols/can/socketcan.mspec
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.
+ */
+
+[type 'SocketCANFrame'
+ [simple int 32 'rawId']
+ [virtual int 32 'identifier'
+ 'STATIC_CALL("org.apache.plc4x.java.transport.socketcan.helper.HeaderParser.readIdentifier", rawId)'
+ ]
+ [virtual bit 'extended'
+ 'STATIC_CALL("org.apache.plc4x.java.transport.socketcan.helper.HeaderParser.isExtended", rawId)'
+ ]
+ [virtual bit 'remote'
+ 'STATIC_CALL("org.apache.plc4x.java.transport.socketcan.helper.HeaderParser.isRemote", rawId)'
+ ]
+ [virtual bit 'error'
+ 'STATIC_CALL("org.apache.plc4x.java.transport.socketcan.helper.HeaderParser.isError", rawId)'
+ ]
+ [implicit uint 8 'size' 'COUNT(data)']
+ [reserved uint 8 '0x0'] //flags
+ [reserved uint 8 '0x0'] // padding 1
+ [reserved uint 8 '0x0'] // padding 2
+ [array byte 'data' count 'size']
+ [padding uint 8 'alignment' '0x00' '8 - (COUNT(data))']
+]
Review comment:
EOL missing
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
splatch commented on pull request #241:
URL: https://github.com/apache/plc4x/pull/241#issuecomment-883786340
A new version of PR is ready for review. It contains CAN stack updates, event handling updates but most importantly it brings a complete separation between CAN drivers and CAN transports.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
splatch commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616468290
##########
File path: plc4j/spi/src/main/java/org/apache/plc4x/java/spi/connection/SingleProtocolStackConfigurer.java
##########
@@ -142,31 +141,86 @@ public SingleProtocolStackBuilder(Class<BASE_PACKET_CLASS> basePacketClass, Clas
}
public SingleProtocolStackBuilder<BASE_PACKET_CLASS> withProtocol(Class<? extends Plc4xProtocolBase<BASE_PACKET_CLASS>> protocol) {
- this.protocol = protocol;
+ return withProtocol(configuredType(protocol));
Review comment:
I think we can safely follow @sruehl suggestion to have two versions of stack configurers (or at least builders) - one which supports injections and other which does not.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616445398
##########
File path: plc4j/drivers/canopen/src/main/java/org/apache/plc4x/java/canopen/CANOpenPlcDriver.java
##########
@@ -111,8 +112,8 @@ protected BaseOptimizer getOptimizer() {
}
@Override
- protected ProtocolStackConfigurer<CANOpenFrame> getStackConfigurer() {
- return SingleProtocolStackConfigurer.builder(CANOpenFrame.class, CANOpenSocketCANFrameIO.class)
+ protected ProtocolStackConfigurer<CANOpenFrame> getStackConfigurer(Configuration configuration, Transport transport) {
+ return SingleProtocolStackConfigurer.builder(CANOpenFrame.class, CANOpenSocketCANFrameIO.class, configuration)
.withProtocol(CANOpenProtocolLogic.class)
Review comment:
this would then be the code in the SingleProtocolStackConfigurer:
```
if (protocolClass != null)
protocol = configure(configuration, createInstance(protocolClass));
else
protocol = protocolSupplier(configuration);
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
splatch commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616462636
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/EventPlcConnection.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.api;
+
+import org.apache.plc4x.java.api.listener.EventListener;
+
+/**
+ * An extended kind of connection which can broadcast events.
+ *
+ * Particular type of event might vary, it might be a connection state or message exchange.
+ */
+public interface EventPlcConnection extends PlcConnection {
Review comment:
> I think as this is in the API module, there shouldn't be an event-emitting set of connections and non emitting ones
I wasn't sure of that because of amount of changes it will force in connection pool module. Logic in pooled connections is a bit different and I am not entirely sure if I will be able to deliver that.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on pull request #241: PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
splatch commented on pull request #241:
URL: https://github.com/apache/plc4x/pull/241#issuecomment-921018939
I merged this PR so I can continue on develop these changes should land in 0.9 release.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
sruehl commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616472268
##########
File path: plc4j/spi/src/main/java/org/apache/plc4x/java/spi/connection/SingleProtocolStackConfigurer.java
##########
@@ -42,69 +47,57 @@
public class SingleProtocolStackConfigurer<BASE_PACKET_CLASS extends Message> implements ProtocolStackConfigurer<BASE_PACKET_CLASS> {
Review comment:
Basically yes: This also would clean up the PR diff as we could then clearly see at which point we need the second configurer. The missing documentation on the configurer is on me (we where pretty time limited back then) :)
I will document it ASAP.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #241: [WIP] PLC4X-294 / PLC4X-296 Reorganization of stack configurer / events support in public api
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #241:
URL: https://github.com/apache/plc4x/pull/241#discussion_r616474494
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/EventPlcConnection.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.api;
+
+import org.apache.plc4x.java.api.listener.EventListener;
+
+/**
+ * An extended kind of connection which can broadcast events.
+ *
+ * Particular type of event might vary, it might be a connection state or message exchange.
+ */
+public interface EventPlcConnection extends PlcConnection {
Review comment:
Well if you change the core of things it is natural that they need changes in a lot of places ;-) ... the later you introduce the change, the more you need to change. But I would not be in favor of a change that makes our users have to do instanceof stuff in order to use the API.
You wouldn't have to do all on your own. We could probably all do our part. I do think that some sort of "ups I think I just lost the connection" mechanism is indeed very important.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org