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

Two operations in rapid succession can confuse diag_l0_elm #87

Open
aaeegg opened this issue Oct 9, 2023 · 3 comments
Open

Two operations in rapid succession can confuse diag_l0_elm #87

aaeegg opened this issue Oct 9, 2023 · 3 comments

Comments

@aaeegg
Copy link
Contributor

aaeegg commented Oct 9, 2023

If scantool initiates a command or request right after a previous request completed, the interface might still be listening for more responses from the vehicle when we send it the second operation. The interface will then send a STOPPED error, and the second operation will fail.

Repro cases: cleardtc followed by disconnect command on AW50-42 TCM (the ATPC command interrupts the stopDiagnosticSession transaction), or two cleardtcs in a row on AW50-42 TCM (the clearDiagnosticInformation request interrupts the readDiagnosticTroubleCodes transaction). cleardtc on AW50-42 sets the stage for this to happen on future operations because the >50ms response time causes the interface's adaptive timing algorithm to increase the response timeout on future requests.

See discussion in #84.

@fenugrec
Copy link
Owner

Right. The points I'm wondering about are :

  • is adaptive timing useful at all in our use case ?
  • can we disable it once at device init ?

@aaeegg
Copy link
Contributor Author

aaeegg commented Oct 13, 2023

is adaptive timing useful at all in our use case ?

For D2, adaptive timing is probably actively harmful. The interface could set the timeout too low and miss responses.

For J1979, in theory, adaptive timing could speed up live data retrieval. But I'm not sure whether we currently use the ELM's > prompt as a hint that there won't be any more responses, or if we'll just keep listening to the ELM even though it already gave up listening to the car.

can we disable it once at device init ?

We can, but if that's the only change we make, it'll make things worse. D2 protocol seems to have P2 timeout somewhere between 50ms and 100ms. Suppose we set the ELM to a fixed timeout of 100ms. Now, if we do two operations in rapid succession, we're always going to hit the failure case that we previously hit only when the adaptive timing had set a long timeout. It's always going to be listening for a second response for 100ms after the first response, when freediag has already moved on to the next request.

If we do as I suggested in #84 (comment), we could set the ELM timeout to its maximum value of 1.02 seconds, leaving the interface in receive state after all operations, and interrupting the trailing receive when we do our next transmit or command.

By the way, after initially connecting, "850 connect" currently sends a few pings to the ECU with delays between them, as a workaround for what seemed to be a problem with adaptive timing. I had believed that the timeout was starting out too short and causing us to miss responses, but it's possible that the timeout was starting out too long and triggering the problem described here.

@fenugrec
Copy link
Owner

fenugrec commented Oct 18, 2023

This all reinforces my view that the ELM protocol is just not well adapted to freediag's architecture (which itself isn't great either, as we know...)

I am no longer in a position to test ELM code and well past the point of caring about it, but if you manage to come up with something workable that doesn't need to refactor the entire L1/L2 layer, I'm willing to review and merge (I will however require confirmation that it doesn't break plain J1979 over 9141/14230).

In fact, if you want to run the entire L0_elm layer in its own thread... that would be intriguing

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

Successfully merging a pull request may close this issue.

2 participants