P2* timeout not used after RequestCorrectlyReceived_ResponsePending about python-udsoncan HOT 18 CLOSED

I have met this issue recently and I will try yo fix it as you suggest. I will create a pull request if it fixes the issue.

from python-udsoncan.

Hello @martinjthompson and @jmoren13
The overall timeout is meant to be a last resort to make sure that your client does not hang indefinetely if the server wants it.
The proposed fix will remove that feature. I think that if you would need a mechanism to disable the global timeout on demand instead of always overiding it.

What do you think?

from python-udsoncan.

This timeout is good when the server doesn't return a response, but in this case it is actively sending NRC 0x78, so I don't think it should trigger the timeout.
In my case, the server sends NRC 0x78 for a few mins while processing some info and I need to set an overall timeout of minutes. But in case the connection is broken and I stop receiving 0x78 or other response, I don't want to wait for such a long timeout.

from python-udsoncan.

If connection stops in the situation you described, then p2* timeout will trigger. Isn 't that enough?

from python-udsoncan.

The problem is that, if NRC 0x78 is received, the client should wait for p2* timeout until it receives a positive response or another NRC 0x78.
This doesn't happen, the overall_timeout_time is not reset and it stops the connection without waiting for p2* seconds.
I think @martinjthompson explains it better in his comment in the top.

from python-udsoncan.

Yes, the problem is as @jmoren13 describes. If the server dies, the timeout works correctly. But, if the server returns RCR_RP, the timeout is not correctly used. It is updated, but then is not actually applied by the code as it currently is.

The fix proposed by @jmoren13 is exactly the one I have already successfully used on one ECU, and have been meaning to submit for weeks, sorry.

from python-udsoncan.

I understand, but imagine there's no overall_timeout_time at all. Only P2 and P2*, then you would get the behavior you are looking for. Am I wrong?

from python-udsoncan.

Even without overall_timeout_time, using only P2 and P2*, the Client returns timeout without waiting for P2* seconds. It keeps the old value in overall_timeout_time and triggers the timeout in the try block (client.py: line 1639).
single_request_timeout is correctly set to P2* but overall_timeout_time still has the value set from the previous request.

I have just tested:

my_config ={
    'p2_timeout'                            : 25,
    'p2_star_timeout'                       : 30,
}

After receiving the NRC 0x78, I get the following exception:
Did not receive response in time. Global request timeout time has expired (timeout=4.911 sec)

Which, 4.91 seconds is not even any of the timeouts given. It is just that keeps the value from when it sent the request that received NRC 0x78.

from python-udsoncan.

Ha! You set 2 timeouts out of 3. There's a third timeout called "request_timeout" that has the default value of 5 sec. That's why it says "Global request timeout"

As I said, I designed this timeout to superseed everything so that the software can't be locked in a wait state forever. Try putting a large value in that config and tell me if you get the wanted behavior please.

from python-udsoncan.

from python-udsoncan.

Yes, you can look at all timing test cases. There isn't that much.
I will double check how things works to make sure I haven't let a case slip.

Just in case, here's how I designed it initially (and there's an historical reason behind it). Once we agree upon what is happening, then we can discuss about what should happen :)

from python-udsoncan.

Using this config is the same. It uses 25 seconds as overall_timeout_time, Ignores my 'request_timeout' because is bigger.

So, after receiving NRC 0x78, it is not waiting for a whole p2* before triggering the timeout it just waits for p2_timeout because it is the value it had in the previous loop iteration.

my_config ={
    'request_timeout'                       : 100,
    'p2_timeout'                            : 25,
    'p2_star_timeout'                       : 30,
}

from python-udsoncan.

I have modified my changes, I update the timeout value everytime the client receives the NRC 0x78 instead of just the first time.

I believe the desired behaviour is that one. The client should not stop the communication when the server is telling 'please wait'.

                if response.code == Response.Code.RequestCorrectlyReceived_ResponsePending:
                    done_receiving = False
                    if not using_p2_star:
                        # Received a 0x78 NRC: timeout is now set to P2*
                        p2_star = self.config['p2_star_timeout'] if self.session_timing['p2_star_server_max'] is None else self.session_timing['p2_star_server_max']
                        single_request_timeout = p2_star
                        using_p2_star = True
--                      overall_timeout_time = time.time() + single_request_timeout
                        self.logger.debug("Server requested to wait with response code %s (0x%02x), single request timeout is now set to P2* (%.3f seconds)" % (response.code_name, response.code, single_request_timeout))
++                  overall_timeout_time = time.time() + single_request_timeout
                else:
                    raise NegativeResponseException(response)

from python-udsoncan.

I will review soon. I think an exit condition should always be possible if required. I'd prefer have a parameters to choose wetger the client can block indefinetely or not

from python-udsoncan.

from python-udsoncan.

Indeed, but having an external device being able to set the state of the software seems like a vulnerability to me. I initially designed this library to debug the implementation of a server. I'd like that the possibility to force an exit still exist.

In any case, I don't mind having a parameter to behave as you describe and set it as default behaviour. At this point, it'S pretty much what you ask for.

from python-udsoncan.

from python-udsoncan.

Included in v1.14

from python-udsoncan.