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:
[email protected]
