SerialCommunicationHub always waits for timeout before returning the result

[golovasteek]

I was investigating why reading the values from the meter always take a while. And it looks like the tiny_modbus_rtu is implemented in the way, that we always return when the response timeout is reached and not earlier.

This maybe not a problem with quick devices, but some devices like Bauer Meter, may have long response time sometimes. So we need to keep the timeout large.

Expected behaviour:

• serial communication hub return the response, as fast as it is received.

[corneliusclaussen]

The original implementation returned immediately when the expected number of bytes were received, it only waited for the timeout if bytes were missing. Not sure if that broke with a recent commit or so

[barsnick]

I believe this check:

everest-core/modules/SerialCommHub/tiny_modbus_rtu.cpp

Lines 301 to 305 in c144d56

	// do we have space in the rx buffer left?
	if (bytes_read_total >= rxbuf_len) {
	// no buffer space left, but more to read.
	break;
	}

should be done after this block, not before:

everest-core/modules/SerialCommHub/tiny_modbus_rtu.cpp

Lines 307 to 310 in c144d56

	int bytes_read = read(fd, rxbuf + bytes_read_total, rxbuf_len - bytes_read_total);
	if (bytes_read > 0) {
	bytes_read_total += bytes_read;
	}

Because, in the current code, if you have reached the buffer space limit, you wait one more select timeout to break out of the loop, instead of breaking out immediately (since no more data is being read anyway).

This is just a visual review, untested.

[golovasteek]

As far as I see we always request the response of the maximum possible size

everest-core/modules/SerialCommHub/tiny_modbus_rtu.cpp

Lines 443 to 444 in c144d56

	uint8_t rxbuf[MODBUS_MAX_REPLY_SIZE];
	int bytes_read_total = read_reply(rxbuf, sizeof(rxbuf));

So we always will wait for time out if we want shorter response.

[corneliusclaussen]

As far as I see we always request the response of the maximum possible size

everest-core/modules/SerialCommHub/tiny_modbus_rtu.cpp

Lines 443 to 444 in c144d56

	uint8_t rxbuf[MODBUS_MAX_REPLY_SIZE];
	int bytes_read_total = read_reply(rxbuf, sizeof(rxbuf));

So we always will wait for time out if we want shorter response.

The read_reply function will not always fill the buffer, it is just the maximum size

[corneliusclaussen]

Because, in the current code, if you have reached the buffer space limit, you wait one more select timeout to break out of the loop, instead of breaking out immediately (since no more data is being read anyway).

This is just a visual review, untested.

Correct, this should be fixed. It should however only occur on transfers with 261 bytes, everything smaller will always have the "within_message_timeval" message timeout

[corneliusclaussen]

The default is configurable with a default of 100ms. In many implementations this can probably be reduced quite a bit, but I do not have enough hardware to find a good value. It is the inter character timeout once the peer started to send the reply message. It should be really quick.
If we want to make it faster we would need to calculate before hand how many bytes the reply should be, then we can stop immediatly when those are received without waiting until nothing more is received. We would however need to deal with modbus devices that just send more bytes then expected (this happens).

So for now I'd suggest to just reduce the 100ms timeout in your config if that is too long. You should be able to keep the initial timeout on a larger value. My guess is your power meter takes a long time to respond, but once it has sent the first bytes it will be quick sending the remaining bytes right?

In this case, it should not always wait for the first timeout:

The select should return whenever there is at least one byte available for reading, then the reading should not block and return some bytes (let's assume it was the complete reply).
The loop runs again, waiting for select (but now with the shorter timeout). If that times out, we assume there are no more bytes coming and return.

So there is always the "within_message_timeout_ms" timeout at the end, but it is not very easy to get rid of this one, as the read_reply does not know how many bytes to read. But since this is short, I do not think it is an issue.

@golovasteek Did you find another solution in the mean time? Would that resolve your issue?