Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Positive response suppression is not fully checked #247

Open
raul-klg opened this issue Oct 15, 2024 · 1 comment
Open

Positive response suppression is not fully checked #247

raul-klg opened this issue Oct 15, 2024 · 1 comment

Comments

@raul-klg
Copy link

Hi!

First of all thanks for this great piece of software.

I think I have found an incorrect behavior, but maybe I'm wrong.

Consider the following code snippet:

    with uds_client.suppress_positive_response(wait_nrc=True):
        response = uds_client.tester_present()
        assert response is None

If the ECU is correctly implemented there should not be positive response to the tester_present request. For example, this is the expected candump output:

(064.801721)  can0  0CDA1619   [3]  02 3E 80

For the above, the current udsoncan implementation is correct.

Now I am testing an incorrectly implemented ECU, and it incorrectly replies to the tester present request when positive response suppression is requested:

(104.301011)  can0  0CDA1619   [3]  02 3E 80
(000.006008)  can0  0CDA1916   [8]  02 7E 80 CC CC CC CC CC

I think udsoncan should check that when request is in suppress_positive_response context, there is no reply and should fail somehow. Currently, udsoncan tester_present call returns an empty response object but it internally does not check that the positive response is actually suppressed.

@pylessard
Copy link
Owner

pylessard commented Oct 15, 2024

Hi,
Indeed you are right, any unwanted messages is silently deleted by the client. It all happens here : https://github.com/pylessard/python-udsoncan/blob/master/udsoncan/client.py#L2157

A check for unwanted message is a little tricky to implement since the design of the client is synchronous. I could probably make a warning if the rxqueue is not empty when we empty it. But the error would show when waiting for the following request.

A more complex check would most likely require a listening thread that keeps an internal state about what we asked and what is expected. Such kind of design could also enable services like ResponseOnEvent. I don't necessarily plan on doing that as the synchronous single request/single response approach is more than enough more most use cases and my free time is dedicated to another project right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants