rnd-ash / ecu_diagnostics

A Rust crate for ECU diagnostic protocols (UDS / KWP)
GNU General Public License v3.0
170 stars 28 forks source link

Unified diagnostic server #20

Closed rnd-ash closed 1 year ago

rnd-ash commented 1 year ago

This is a draft PR until the full transition is done. But adds a load of changes.

rnd-ash commented 1 year ago

hey @nyurik , would you be able to help review this? At least, have a look at the changes to the UDS side.

nyurik commented 1 year ago

@rnd-ash sure, will do shortly. I think you based it of of an older version? Seems like there are quiet a few merge conflicts. I will look at the UDS stuff of course

rnd-ash commented 1 year ago

Your right. I'll rebase it on the latest from main

rnd-ash commented 1 year ago

@nyurik I've done some more refactoring and rebased of the latest main. I think that https://github.com/nyurik/auto_uds/issues/3 should be addressed at some point on your end, but the refactoring I've done now lets us define our own diagnostic modes on the dynamic_diag server, useful for when trying to specify OEM dependent modes. I've also added a Tx done hook that you can register with. Check the KWP test example to see how it works.

nyurik commented 1 year ago

Naming question: should the ByteWrapper use Standard(T) vs NonStandard(u8) enum vals, or should they be named Standard vs Extended ?

Update: I went ahead and renamed it. v0.5 uses Extended(u8) instead of NonStandard(u8) (everything else is the same)

rnd-ash commented 1 year ago

@nyurik so I have done a few more changes, re-introduced the harsh linting, generated documentation for everything. I think we can transition this now to a full PR ready to merge - Obviously this would be a massive breaking change. So will require quite the version bump.

nyurik commented 1 year ago

I was looking through the KWP2000 and realized that maybe we should move all those constants to auto_uds, perhaps renaming it to auto_diagnostics instead? I'm not an expert in the field, so don't know how to best organize it, but it seems it would be easier to maintain them all in one crate. Also, to optimize compilation, we can put them all behind feature flags, e.g. one would specify with_uds and with_kwp2k if needed (both can be enabled by default). Plus possibly OBD2?

P.S. all these constants will need to be under MIT+Apache2 license -- noone will ever use these libs in their vehicles or other software if they are anything more restrictive. The project you are building on the other hand can have a more app-like license (GPL, etc)

nyurik commented 1 year ago

first pass at implementing constants - https://github.com/nyurik/auto_uds/pull/4/files -- maybe its worth publishing those constants first, and using them in your PR? Or could be done as a separate PR

rnd-ash commented 1 year ago

@nyurik final feature has been added! Check this example I am running on my ECU. After a power on reset and sending a new request, if the ECU responds with wrong session, the diag server attempts to automatically recover the last session mode the user had requested.

image

rnd-ash commented 1 year ago

Reset detection with 5608e9fa33cd2f47151f3c4e9f6cab45fb9b0fbe

image

rnd-ash commented 1 year ago

hey @nyurik did a few more tweaks. The DynamicDiagServer can now be used without mutability. Shall we call this ready to merge? I am testing it in my TCU's config app, without issue and also with much lower CPU overhead

nyurik commented 1 year ago

@rnd-ash looks good! I have left a few comments throughout the code - do you want to apply those? Also, do you have any thoughts about https://github.com/rnd-ash/ecu_diagnostics/pull/21#issuecomment-1509948390 ? Thx!

rnd-ash commented 1 year ago

Ok, I think we are ready to merge!

rnd-ash commented 1 year ago

@nyurik are we ready to merge?

nyurik commented 1 year ago

sure :) Will fix whatever's left incrementally