Unnecessary add_writer call in SerialTransport?

[yawor]

Hi,

I've been analyzing the SerialTransport code in regards to #2205 and I've stumbled upon these four lines:
https://github.com/pymodbus-dev/pymodbus/blob/30e4aa759024ee3faec1bc54ff2e689be9c86448/pymodbus/transport/serialtransport.py#L138C1-L141C22

Maybe there's some reason behind calling add_reader here again, but I can't find any. The Python's documentation of add_reader is lacking, so I've checked the Python's source code and calling this method for the same file descriptor multiple times is not dangerous, it just replaces old writer with a new one. The writer is not removed unless remove_writer is called. This is not a bug, but possibly wastes CPU cycles and delays the return to the loop.

[janiversen]

As far as I remember we only call add_writer when there is something to write, which is according to the manual.

This is the normal procedure, since it eases sending a new packet to be written. As far as I know the add_writer "dies" when the data at hand have been sent (otherwise it would be constantly polling).

However if you have a more efficient solution, then a pull request is welcome.

[yawor]

I'd need to do some testing, but I don't think that the writer removes itself after calling the callback. I've compared SerialTransport in pymodbus to the one in pyserial-asyncio (it's from the authors of pyserial). The overall approach is quite similar, but their implementation doesn't call add_writer from the writer's callback at all. They only call add_writer in the write method to start sending data from internal buffer. When all the data in the buffer gets sent then the callback calls the remove_writer method.

[janiversen]

As I wrote pull requests preferable with extra tests are welcome.

And just for information, the original pyserial_asyncio had several bugs, but is not maintained, you can find pull requests from me, that is the only reason we decided to move a copy to our repo. We have later made some significant changes, that makes it better adapted to modbus.