silent_interval / idle_time() is broken

[astrand]

On #2510, the handling of silent_interval was silently (sic) dropped. Currently, the only place where self.silent_interval is checked is the idle_time() method, which is in turn unused. The description is also incorrect, claiming that this is handled automatically.

Normally, with slow hosts and devices which requires only a standard interval of 3.5 characters, this might not be a problem, but some devices (for example some energy meters) require a silent interval of 40 ms.

[janiversen]

Pymodbus follow the standard modbus protocol, which defines an exact waiting time. All the energy devices we are aware of follow the modbus standard.

I think idle_time have been dropped for quite a long time.

However pull requests are welcome if you want the code to work differently.

[janiversen]

silent_interval was not part of the API, which is the reason it is not mentioned,

Internal code are changed without notice, otherwise the release doc would be identical to the commits.

[astrand]

Standard silent interval is 3.5 chars as you say, which corresponds to ~4 ms at 9600 bps. But as far as I can tell, there is no "sleep" in the code tha asserts this? Likely, it works anyway due to Python latency.

https://www.gavazziautomation.com/en-us/product/EM340DINAV23XS1PFA is example of an energy meter which requires 40 ms silent interval. From https://www.gavazziautomation.com/fileadmin/images/PIM/OTHERSTUFF/COMPRO/EM330_EM340_ET330_ET340_CP.pdf :
T delay: Minimum time before a new query 40 ms

If idle_time() and silent_interval is no longer used and not part of the public API, why is it still present in the code base? This is just confusing.

[janiversen]

who needs a sleep in async code ?

Furthermore a sleep would be wrong, because how do pymodbus know when the app sends the next request.

When you look in the internal code you will find a lot of confusing lines, most of them have historic reasons.

But as I wrote pull requests are welcome.

[janiversen]

And just to be clear, the 40ms is very easy to handle in the app...put a sleep between the requests, there are no real advantage of doing it at a lower layer.

[astrand]

who needs a sleep in async code ?

Furthermore a sleep would be wrong, because how do pymodbus know when the app sends the next request.

When you look in the internal code you will find a lot of confusing lines, most of them have historic reasons.

But as I wrote pull requests are welcome.

It is easy to calculate the sleep. You need a timestamp of the last character. pymodbus already stores this in self.last_frame_end. Then, when it is time to send a new request, you can calculate the amount of sleep required, if any. The pymodbus function idle_time() basically does this, but you need to substract time.time() and check for negative result.

Such sleep is never be "wrong".

You can of course do anything and everything in the application, but the purpose of a library such as pymodbus is to make it easier for the application programmer.

Btw, idle_time() is not internal, it is still mentioned on https://pymodbus.readthedocs.io/en/latest/source/client.html , so it is incorrect to say that it has been dropped.

We are currently not using the "async" API, but the sync one.

Since pymodbus is Open Source, I don't expect fixes for free, and I could consider making a PR. However, to be honest, when reading between the lines (also on other issues), it seems like you are focusing more on defending the current code base rather than acknowledging that there are issues. Let's keep calm and make this a good open source project.

Something needs to be done, either fix the implementation or remove the historical and confusing code. Please keep this issue open until we have either solution.

[janiversen]

If you look at other issues, you should have seen that nearly all PRs are accepted....and issues with real bugs are normally solved within a couple of days.

A number of issues is about non standard modbus and that is something very hard to maintain so in these cases the current codebase is defended.

But you are correct, issues where the autor just complains or even describe the solution but expect others to do the implementacion are not receiving a positive aptitude for obvious reasons.

Feel free to make a PR.

[janiversen]

The sleep used earlier was seen by some users as a speed limitación which would be better handled in the app. So please do not say a "sleep" cannot be wrong.

the timestamp of the last charecter was proven to be wrong, because it tells when the character leaves the user space and entres the kernel buffer, it tells nothing about when the charecter is transmitted on the rs485.

[astrand]

The sleep used earlier was seen by some users as a speed limitación which would be better handled in the app. So please do not say a "sleep" cannot be wrong.

the timestamp of the last charecter was proven to be wrong, because it tells when the character leaves the user space and entres the kernel buffer, it tells nothing about when the charecter is transmitted on the rs485.

The sleep should of course only be done if necessary. Right now, you are relying on the latency of Python and the platform; you do not have any guarantee that you fulfill the standard Modbus 3.5 character silence. As time goes by and computers gets faster, you will perhaps get problems sooner or later.

You seems to be confused about the code: self.last_frame_end was the timestamp of the last character received; not transmitted. This timestamp is for sure later than the time when the character was transmitted on the bus.

I think you should be more careful with making changes to this library. You can of course do all sorts of changes with proper motivation, but the commit 36bd92d you did was not properly describing that the silent_interval sleep was removed, neither why.

It seems like most commits by you are only described by short "one-liners", like "fix X" or "improve Y". This makes it difficult to understand why the change was made.

[janiversen]

pymodbus is not relying on the latency of pymodbus, I think you miss that modbus is a master-slave protocol so whenever the client sends a request it waits for a response which is far longer than 3.5char, and thus there are no need for additional wait !

The 3.5 char is important to a server, because it signals End Of Frame, which is needed because somemodbus requests have a dynamic size without a length field. There are no responses which cannot be identified as either fix length or do have a length field.

I also think you have a wrong impression, I have done most of the work on the version 3 line, which includes a total rewrite of the both the client and the server, so I do not think I need to be more careful about making changes to the library :-)

The commit texts are for internal use, API_changes and the release text are what is made for external use....I guess if you dive deeper into the code as some do, you will understand the commit messages and their motivation better.

It also seem you read the modbus standard wrongly, the 3.5 char delay is defined as the interval demanded between SENDING frames, not after receiving a frame.

[janiversen]

Just a last remark, the use of "one-liners" is a project choice. When merging all commits are squashed and only the title text remain. If you do not like that, I suggest you become active in the project and thus gets to influence the work we do.

[astrand]

The 3.5 character delay must be asserted between the end of the reply and start of new request. Without a sleep, it is possible that the master/client sends a new request too soon. Take a look in the linked PDF above and you will see.

[janiversen]

No that is not true !

looking at page 6, the 3.5 char delay (Tnull) only applies between request frames.

Delay after receiving a response (Tdelay) is not part of the modbus standard. Tdelay is something special for these devices and actually something that is very rare in modern devices.

Can I politely suggest that you inform yourself on the modbus standard, as well as who is who in this project !

Your remark "I think you should be more careful with making changes to this library" seem quite offensive, considering we talk about code that I have written/rewritten.