pymodbus-dev / pymodbus

A full modbus protocol written in python
Other
2.16k stars 891 forks source link

Add timeout handler callback to ModbusRequest #2020

Closed jameshilliard closed 2 months ago

jameshilliard commented 4 months ago

Timeout handler callback which allows for custom recovery logic when a ModbusRequest times out.

jameshilliard commented 4 months ago

I think I wrote that callbacks are information only.

Well since callback_data can't be used to inject a response on timeout when using a queue...this provides the ability to do so.

janiversen commented 4 months ago

Well since callback_data can't be used to inject a response on timeout when using a queue...this provides the ability to do so.

It would be very illogical to wait for the timeout, when you know the device is not going to respond !

callback_data was to be used when the app makes the request.

And since we not know how the queue concept will be implemented, how do you know callback_data() cannot be used, e.g. with a flush() it would be highly possible.

jameshilliard commented 4 months ago

It would be very illogical to wait for the timeout, when you know the device is not going to respond !

Well you mentioned that "rs485 prescribes a delay to turn the electric bus and allow a client to respond." so the timeout would effectively be our minimum delay in this particular case.

callback_data was to be used when the app makes the request.

But with a FIFO queue that would not work because the application request and the actual request being sent over the wire would be effectively decoupled.

And since we not know how the queue concept will be implemented, how do you know callback_data() cannot be used, e.g. with a flush() it would be highly possible.

Queue would at a minimum be a FIFO queue so I don't see a clear way to associate a callback_data() injection to the current queue item being processed.

janiversen commented 4 months ago

Well you mentioned that "rs485 prescribes a delay to turn the electric bus and allow a client to respond." so the timeout would effectively be our minimum delay in this particular case.

I do not think that has anything to do with a timeout callback.

But with a FIFO queue that would not work because the application request and the actual request being sent over the wire would be effectively decoupled.

We do not have a FIFO queue, serial actually waits for a response (as the protocol demands) before sending a new request.

Queue would at a minimum be a FIFO queue so I don't see a clear way to associate a callback_data() injection to the current queue item being processed.

if you do not see it, then so be it, I do not share that opinion, I have made several queue implementation that provide means to act synchronized when needed, even without adding a lot of code.

jameshilliard commented 4 months ago

I do not think that has anything to do with a timeout callback.

The timeout callback would be where we would inject our fake response as opposed to callback_data.

We do not have a FIFO queue, serial actually waits for a response (as the protocol demands) before sending a new request.

I'm referring to a FIFO queue that would be somewhat similar to #1982.

janiversen commented 4 months ago

The timeout callback would be where we would inject our fake response as opposed to callback_data.

That is too late, it needs to be before the timeout to have effect.

I'm referring to a FIFO queue that would be somewhat similar to #1982.

I know, but we do not have it, so no need to make assumptions about how a future implementation will work. A sync/flush concept is independent of the queue.

jameshilliard commented 4 months ago

That is too late, it needs to be before the timeout to have effect.

Depends on what sort of effect you are looking for I suppose. If you merely want to alter the timeout response it would work.

github-actions[bot] commented 2 months ago

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions[bot] commented 2 months ago

This PR was closed because it has been stalled for 10 days with no activity.