If RTU response is empty, don't validate CRC but fail earlier.

[petonic]

Hi -- I fired this up really quick to see if I could use it in a home-rolled program, and correct me if I'm wrong, but there doesn't seem to be any error checking (specifically around validate_crc()) on the responses, right?

I'm firing a write_multiple_registers command with no RS485 devices on the serial bus. Output is fine, I guess, but my simple driver program just craps out because of an exception in validate_crc().

Can you confirm? Also, good luck on further developing this. I like the approach!

[OrangeTux]

If I understand you correctly the validate_crc of a response failes? Could you post a stacktrace/error?

[petonic]

Hi! Thanks for responding.

I can dig up the exception, but it has to do with the fact that write_multiple_registers() is expecting a response and when it times out, it still calls validate_crc() with an empty buffer, which then gives the exception. Should I trap for that and then handle exceptions that way?

Do you still need a stack trace or is that expected behavior?

[OrangeTux]

I understand the case and I agree that the raising the exception in validate_crc is a bit weird. Earlier in the process an error should be raised.

def send_message(adu, serial_port):
    """ Send Modbus message over serial port and parse response. """
    serial_port.write(adu)
    response = serial_port.read(serial_port.in_waiting)

    if len(response) == 0:
        raise TimeoutError

    return parse_response_adu(response, adu)

[petonic]

Perfect. I'll go ahead and try doing it again with that modification.
Definitely a better place, I agree.

On 30 Jan 2017, at 9:02, Auke Willem Oosterhoff wrote:

I understand the case and I agree that the raising the exception in validate_crc is a bit weird. Earlier in the process an error should be raised.

def send_message(adu, serial_port):
    """ Send Modbus message over serial port and parse response. """
    serial_port.write(adu)
    response = serial_port.read(serial_port.in_waiting)

    if len(response) == 0:
        raise TimeoutError

    return parse_response_adu(response, adu)

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/AdvancedClimateSystems/uModbus/issues/50#issuecomment-276120703

[OrangeTux]

If you want to fix it I'm happy to receive a pull request.

[petonic]

I'm actually making a couple of fixes on it and then I'll submit what I have to you in the next couple of days. My use-case is an RTU server (and client).

I really like your flask-like paradigm, and it looks pretty good. Wild guess: the RTU side probably didn't evolve as much as perhaps the TCP side, am I right?

For instance, in rtu.py, serve_once(self) can get back 0 bytes if it's just spinning, and if it does, then it'll try to handle that request, and the CRC routine will blow up again. Fixing the exception handling in the CRC routine makes sense in any case, but for this, I don't think that serve_once should even return unless it gets an ADU back.

Well, maybe it should, and then init.py/serve_forever() should be the one ignoring null input. We can have it so that serve_once() just returns if the ADU is empty. Of course, we'll have to make sure that serve_once() doesn't try to process the ADU if it's null.

Does this approach make sense to you and would be in keeping with your approach?

On 30 Jan 2017, at 10:42, Auke Willem Oosterhoff wrote:

If you want to fix it I'm happy to receive a pull request.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/AdvancedClimateSystems/uModbus/issues/50#issuecomment-276151472

[OrangeTux]

Good finding. I don't think that validate_crc is a proper place to do the length check. What would be good error tho raise in this place and how should both rtu client and server handle it?

I think rtu.RTUServer.serve_once should check if length of request_adu. If length is 0 the method must not return the try to process the request and return a response, like you said, but it should raise an error. I think a TimeoutError fits best.

[clevert-pretto]

Hi there, Is this issue is still unsolved?

[OrangeTux]

No, it isn't yet. But the solution already thought trough in the above comments. So it's very easy to implement. I'll try to fix in somewhere this week.

[OrangeTux]

This has been fixed in the 1.0.0