Added receive timeout to wait for incoming data for at least specified time

[denis-ftc-denisov]

Hello!

I've added timeout feature. It could help with the case when one modbus message comes via several tcp packets. In that case we start to parse already received data, but the message is not finished yet, so modbus transaction fails. With timeout ModbusTCPTransport and ModbusTCPRTUTransport will wait for incoming data for some time.

This case appeared when i've tried to connect serial port device to OpenHAB via modbus protocol. I was unable to get OpenHAB talking to serial port directly, so i've used ser2net program to export serial port as TCP port. But after some logging it was clear that OpenHAB does not wait for full response message to appear (it seems ser2net splits message into several parts).

The timeout could be specified via constructor, or by system property. Latter option simplifies integration with current OpenHAB install (just specify it in EXTRA_JAVA_OPTS).

Also, when no timeout is specified, the code will work just the same as before.

[ssalonen]

Hi

There should be timeouts already there

See TCPMasterConnection:setTimeout. Similarly for SerialConnection. And these are certainly respected, the error is propagated to the thing status, see e.g. https://community.openhab.org/t/read-time-out-error/49113/18

Having said this, I have heard reports when tcp packets are fragmented, so having solution there would be great.

What kind of error you are getting when it fails?

Edit:

had a closer look at the original implementation and noticed quite a significant bug : DataInputStream.read is used instead of DataInputStrea.readFully. What an oversight!

There is no guarantees naturally that read manages to read all the bytes that were requested.

Interesting to see if serial transport suffers from similar issue.

Comparing to newer jamod fork, steveohara/j2mod we can see that readFully is used as well.

Would it not better let java socket handle the timeout implementation and use the readFully method?

[denis-ftc-denisov]

Yep, there is a receive timeout for SerialConnection and here i was trying to implement the same for TCPSlaveConnection.

Seems that m_Timeout in TCPSlaveConnection should do the job, but does not really. Well, i've just noticed that m_Socket.setSoTimeout is called only inside setTimeout, which is not called anywhere. It seems, better to add same call to constructor.

As for errors, i've received CRC Error in received frame: 3 bytes: 01 03 14 and same errors with different amounts of bytes received so far (but ser2net log contains full response message).

[ssalonen]

setTimeout is called in TCPMasterConnection:connect method, is it not?

Don't you mean TCPMasterConnection? (Connection to modbus tcp server)

TcpSlaveConnection is not used at all in the binding as it is meant for building a modbus tcp server.

All right, I can imagine CRC errors when you take into consideration what I wrote above regarding read vs readFully, would you agree?

[ssalonen]

Btw, it seems also the SerialTransport is using read method, not readFully. But with serial we have the luxury of set Receive Threshold which probably helps (one of the bug fixes in this openhab fork)..

[denis-ftc-denisov]

According to https://docs.oracle.com/javase/7/docs/api/java/net/Socket.html#setSoTimeout(int) if we let socket handle the timeout, we should stick with read method. Or maybe i'm missing something, don't have much experience with Java.

Well, my bad, did not realized i need to look at TCPMasterConnection. In that case i don't really understand what happens - Modbus.DEFAULT_TIMEOUT is 3000, that's really enough for message to arrive (now i'm setting timeout of 150ms via property and messages are read fully). But why in this case read truncates message (according to docs, it should block for at least 3000ms).

[ssalonen]

I think readFully is a convenience over read.

It fails because read(byte[]) is not promising to filo the whole buffer. It just reads some number of bytes but not always the whole buffer. Now the problem is that the jamod code actually does not check how many bytes were read (return value of the read method).

Readfully repeatedly calls read as long as needed to fill the buffer. Exception is raised on timeout

[denis-ftc-denisov]

I see now. Will try to check with readFully tomorrow and inform about results.

[denis-ftc-denisov]

I've tried with readFully and it works like a charm. Created another PR with that changes. This one could be closed as irrelevant. Thanks for your help @ssalonen