You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by "splatch (via GitHub)" <gi...@apache.org> on 2023/02/23 15:49:33 UTC
[PR] feat(plc4j) Better handling of timeouts in plc4j (#821). (plc4x)
splatch opened a new pull request, #822:
URL: https://github.com/apache/plc4x/pull/822
The tricky part I was not able to solve yet is how to wire custom timeout manager into the driver. The configuration instances we have are not well defined and passing them around is not clear to me.
--
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
Re: [PR] feat(plc4j) Better handling of timeouts in plc4j (#821). (plc4x)
Posted by Cesar Garcia <ce...@ceos.com.ve>.
Hi,
This is quite an interesting problem, as they point out, the state machine
of the driver (plc4x) and the state of the Netty pipe must be handled,
For the S7 driver, I have the S7HA version (high availability) in which I
solve part of that problem and it may help you. You can see it in [1].
I hope to be able to place this version this week in the plc4x repo since
it will be the one I will use for my tests, but I see the solution that you
propose as interesting and on track.
My grain of sand,
Kind regards,
1. https://github.com/glcj/plc4x/tree/develop/plc4j/drivers/s7
El lun, 27 feb 2023 a las 11:11, splatch (via GitHub) (<gi...@apache.org>)
escribió:
>
> splatch commented on code in PR #822:
> URL: https://github.com/apache/plc4x/pull/822#discussion_r1118881055
>
>
> ##########
> plc4j/spi/src/main/java/org/apache/plc4x/java/spi/Plc4xNettyWrapper.java:
> ##########
> @@ -200,85 +198,60 @@ public void userEventTriggered(ChannelHandlerContext
> ctx, Object evt) throws Exc
> // by sending a connection request to the plc.
> logger.debug("User Event triggered {}", evt);
> if (evt instanceof ConnectEvent) {
> - this.protocolBase.onConnect(new
> DefaultConversationContext<>(ctx, authentication, passive));
> + this.protocolBase.onConnect(new
> DefaultConversationContext<>(this::registerHandler, ctx, authentication,
> passive));
> } else if (evt instanceof DisconnectEvent) {
> - this.protocolBase.onDisconnect(new
> DefaultConversationContext<>(ctx, authentication, passive));
> + this.protocolBase.onDisconnect(new
> DefaultConversationContext<>(this::registerHandler, ctx, authentication,
> passive));
> } else if (evt instanceof DiscoverEvent) {
> - this.protocolBase.onDiscover(new
> DefaultConversationContext<>(ctx, authentication, passive));
> + this.protocolBase.onDiscover(new
> DefaultConversationContext<>(this::registerHandler, ctx, authentication,
> passive));
> } else if (evt instanceof CloseConnectionEvent) {
> - this.protocolBase.close(new DefaultConversationContext<>(ctx,
> authentication, passive));
> + this.protocolBase.close(new
> DefaultConversationContext<>(this::registerHandler, ctx, authentication,
> passive));
>
> Review Comment:
> You're right, however I am not entirely sure of netty vs plc4x
> lifecycle. Namely can we reestablish connection once we reached that
> point? At high level plc4x drivers support `connect` call, we would need to
> assure that it always configure fresh pipeline and uses new timeout
> manager forcing netty to fire bootstrap procedure again.
>
>
>
> --
> 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
>
>
--
*CEOS Automatización, C.A.*
*GALPON SERVICIO INDUSTRIALES Y NAVALES FA, C.A.,*
*PISO 1, OFICINA 2, AV. RAUL LEONI, SECTOR GUAMACHITO,*
*FRENTE A LA ASOCIACION DE GANADEROS,BARCELONA,EDO. ANZOATEGUI*
*Ing. César García*
*Cel: +58 414-760.98.95*
*Hotline Técnica SIEMENS: 0800 1005080*
*Email: support.aan.automation@siemens.com
<su...@siemens.com>*
Re: [PR] feat(plc4j) Better handling of timeouts in plc4j (#821). (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #822:
URL: https://github.com/apache/plc4x/pull/822#discussion_r1118926053
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/Plc4xNettyWrapper.java:
##########
@@ -200,85 +198,60 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
// by sending a connection request to the plc.
logger.debug("User Event triggered {}", evt);
if (evt instanceof ConnectEvent) {
- this.protocolBase.onConnect(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.onConnect(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
} else if (evt instanceof DisconnectEvent) {
- this.protocolBase.onDisconnect(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.onDisconnect(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
} else if (evt instanceof DiscoverEvent) {
- this.protocolBase.onDiscover(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.onDiscover(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
} else if (evt instanceof CloseConnectionEvent) {
- this.protocolBase.close(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.close(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
Review Comment:
Fresh pipeline finnaly always invoke `PlcConnectionManager.getConnection` it will create fesh pipeline. If you don't stop the TimeManager it will caouse memory leak
--
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
Re: [PR] feat(plc4j) Better handling of timeouts in plc4j (#821). (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on PR #822:
URL: https://github.com/apache/plc4x/pull/822#issuecomment-1446566389
> Thanks for initial review, were you able to test this PR against your environment?
What's you changed?
--
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
Re: [PR] feat(plc4j) Better handling of timeouts in plc4j (#821). (plc4x)
Posted by "splatch (via GitHub)" <gi...@apache.org>.
splatch merged PR #822:
URL: https://github.com/apache/plc4x/pull/822
--
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
Re: [PR] feat(plc4j) Better handling of timeouts in plc4j (#821). (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #822:
URL: https://github.com/apache/plc4x/pull/822#discussion_r1118009413
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/Plc4xNettyWrapper.java:
##########
@@ -200,85 +198,60 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
// by sending a connection request to the plc.
logger.debug("User Event triggered {}", evt);
if (evt instanceof ConnectEvent) {
- this.protocolBase.onConnect(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.onConnect(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
} else if (evt instanceof DisconnectEvent) {
- this.protocolBase.onDisconnect(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.onDisconnect(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
} else if (evt instanceof DiscoverEvent) {
- this.protocolBase.onDiscover(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.onDiscover(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
} else if (evt instanceof CloseConnectionEvent) {
- this.protocolBase.close(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.close(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
Review Comment:
Here we should stop TimeManager
--
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
Re: [PR] feat(plc4j) Better handling of timeouts in plc4j (#821). (plc4x)
Posted by "splatch (via GitHub)" <gi...@apache.org>.
splatch commented on PR #822:
URL: https://github.com/apache/plc4x/pull/822#issuecomment-1575226211
This PR was open for quite long time. I'm going to address @spnettec review comments in separate change set.
--
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
Re: [PR] feat(plc4j) Better handling of timeouts in plc4j (#821). (plc4x)
Posted by "splatch (via GitHub)" <gi...@apache.org>.
splatch commented on code in PR #822:
URL: https://github.com/apache/plc4x/pull/822#discussion_r1118881055
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/Plc4xNettyWrapper.java:
##########
@@ -200,85 +198,60 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
// by sending a connection request to the plc.
logger.debug("User Event triggered {}", evt);
if (evt instanceof ConnectEvent) {
- this.protocolBase.onConnect(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.onConnect(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
} else if (evt instanceof DisconnectEvent) {
- this.protocolBase.onDisconnect(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.onDisconnect(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
} else if (evt instanceof DiscoverEvent) {
- this.protocolBase.onDiscover(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.onDiscover(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
} else if (evt instanceof CloseConnectionEvent) {
- this.protocolBase.close(new DefaultConversationContext<>(ctx, authentication, passive));
+ this.protocolBase.close(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));
Review Comment:
You're right, however I am not entirely sure of netty vs plc4x lifecycle. Namely can we reestablish connection once we reached that point? At high level plc4x drivers support `connect` call, we would need to assure that it always configure fresh pipeline and uses new timeout manager forcing netty to fire bootstrap procedure again.
--
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
Re: [PR] feat(plc4j) Better handling of timeouts in plc4j (#821). (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on PR #822:
URL: https://github.com/apache/plc4x/pull/822#issuecomment-1445251965
> The tricky part I was not able to solve yet is how to wire custom timeout manager into the driver. The configuration instances we have are not well defined and passing them around is not clear to me.
I think we can config in the driver
`@ConfigurationParameter("timeout-request")
@IntDefaultValue(4000)
protected int timeoutRequest;`
And send request like this
```
transaction.submit(() -> context.sendRequest(tpktPacket)
.onTimeout(new TransactionErrorCallback<>(future, transaction))
.onError(new TransactionErrorCallback<>(future, transaction))
.expectResponse(TPKTPacket.class, Duration.ofMillis(configuration.getTimeoutRequest()))
.check(p -> p.getPayload() instanceof COTPPacketData)
.unwrap(p -> ((COTPPacketData) p.getPayload()))
.unwrap(COTPPacket::getPayload)
.check(p -> p.getTpduReference() == tpduId)
```
--
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