search

grid-x / modbus

BSD 3-Clause "New" or "Revised" License
82 stars 28 forks source link

Fix errTransactionIDMismatch with no ProtocolRecoveryTimeout set #66

Closed hnicolaysen closed 1 year ago

hnicolaysen commented 1 year ago

This PR fixes a problem where errTransactionIDMismatches are not prevented when no ProtocolRecoveryTimeout is set, even though they are unrelated.

hnicolaysen commented 1 year ago

Do we also need to do the same separation in the default case?

I don't think so as nothing special similar to the mismatch case is happening here. In default, we just retry when protocol ist allowed to recover (ProtocolRecoveryTimeout > 0) and the deadline is not reached. This looks good to me.

hnicolaysen commented 1 year ago

However, as second discussion point but unrelated to this issue: ProtocolRecoveryTimeout is a time.Duration but never used as such. It's only usage is checking if it's larger than 0. Is this indended? 🤔

andig commented 1 year ago

I wrote about ProtocolRecoveryTimeout before in https://github.com/grid-x/modbus/pull/60#issue-1437432647. Seems almost as if it could be removed, in any case it's not doing what people think it does...

andig commented 1 year ago

Could you also cross-check https://github.com/grid-x/modbus/pull/60 which includes minor optimisation? Would get stale otherwise.

hnicolaysen commented 1 year ago

I approved and merged #60 and rebased this one onto it. I think this is a solid solution now. Note that ProtocolRecoveryTimeout is still odd but should not be part of these two PRs

hnicolaysen commented 1 year ago

@tretmar Before merging, let's discuss this.

hnicolaysen commented 1 year ago

@tretmar Before merging, let's discuss this.

I created new PR for this: https://github.com/grid-x/modbus/pull/72