fenugrec / freediag

OBD2 scantool
GNU General Public License v3.0
338 stars 75 forks source link

Add lots of DTC codes for Volvo 850/SVC70 #95

Closed brendandburns closed 7 months ago

brendandburns commented 8 months ago

See #67

Sourced from http://jonesrh.info/volvo850/freediag_v1_08_20171212.zip

More to come eventually.

fenugrec commented 8 months ago

Oh wow that is quite a list, thanks for extracting that.

Is there a reason for the ordering of the sections ? e.g. one block of codes starting with {0x01 is after 0x58, etc.

brendandburns commented 8 months ago

This is cut/paste straight from that zip file. I preserved the existing order. If you'd prefer a different order, I can do that.

brendandburns commented 8 months ago

fwiw, this change fixed it so that I got a human readable description for my ECC error code (0x37)

aaeegg commented 8 months ago

The current table of just two DTCs is almost just a placeholder. It would be nice to use this longer table, but it could use some cleanup.

@fenugrec, any thoughts on moving the DTC table out of scantool_850.c into its own file?

Here is some background on the big DTC table:

This is an accurate representation of how the KWPD3B0 interpreter at http://jonesrh.info/volvo850/kwpd3b0_interpreter.html translates the 2 hex digit raw DTC code sent from the ECUs into the 3-digit Volvo DTC code that the Volvo world discusses (as of the date of this document).

However, THERE IS ABSOLUTELY NO GUARANTEE THAT THE 2-DIGIT RAW TO 3-DIGIT VOLVO DTC INTERPRETATIONS SHOWN BELOW ACCURATELY REFLECT THE CORRECT MEANING OF THE RAW DTC CODE!!! This is simply our best attempt at understanding them with the limited resources available. So, DIYer, feel free to use them as a guide, but do not consider them as an authoritative guide. We have no idea how well they correlate with the DTCs declared by an official Volvo VST tool (or a VST-lookalike tool). Determining their accuracy is left as the proverbial "exercise for the reader".

fenugrec commented 8 months ago

It would be nice to use this longer table, but it could use some cleanup.

Agreed. Not familiar with D2 but it looks like it could almost be a table-of-tables (one table per "ECU" type (first byte), and one table that holds all these known types) ? Whatever makes further maintenance and/or restructuring easier.

@fenugrec, any thoughts on moving the DTC table out of scantool_850.c into its own file?

Totally in favour.

fenugrec commented 8 months ago

this can be a separate discussion, but you guys mention this DTC list was somehow generated from another source. Is that process automated, with an 'upstream' we should link/refer to ? We don't need to absorb all of that into freediag of course, but maybe worth looking into tweaking that generation step to produce something directly usable in freediag (i.e. already trimmed of the items @aaeegg marked as 'doesn't belong in here', the less well-defined entries, etc )

aaeegg commented 8 months ago

it looks like it could almost be a table-of-tables (one table per "ECU" type (first byte), and one table that holds all these known types) ?

Yes, there's structure in the data that's not represented by the current format. Splitting into a table-of-tables would also let us remove the duplication between the Power Seat Left and Power Seat Right DTCs. The advantage of leaving it in its current format is that no one has to change the code...

aaeegg commented 8 months ago

this can be a separate discussion, but you guys mention this DTC list was somehow generated from another source. Is that process automated, with an 'upstream' we should link/refer to ? We don't need to absorb all of that into freediag of course, but maybe worth looking into tweaking that generation step to produce something directly usable in freediag (i.e. already trimmed of the items @aaeegg marked as 'doesn't belong in here', the less well-defined entries, etc )

Richard has a website where you can paste the raw ELM327 output from a terminal program, and it will interpret the DTCs. This list is the DTC list he maintained for that website. He has updated his site since this list was exported in 2017 - so the list in this PR is outdated. I've asked his permission to scrape a new list.

The previous export was manual, but it should be possible to automate it.

brendandburns commented 8 months ago

@aaeegg I'm happy to see about writing some code to automate this if you want. I'll also attempt the cleanups that you describe above, specifically the table of tables.

aaeegg commented 8 months ago

Richard has given permission to pull an updated DTC list from his interpreter, but he suggested another developer who might be willing to share a better list. Waiting for a response from that person.

fenugrec commented 8 months ago

splitting into a table-of-tables would also let us remove the duplication between the Power Seat Left and Power Seat Right DTCs. The advantage of leaving it in its current format is that no one has to change the code...

Ok. What do you guys think about this sequence :

  1. split out current handful of DTCs in separate file, with some kind of structure (table of tables or whatever is appropriate) in preparation for next step
  2. add either the 2017 list of DTCs above (including fixes from @aaeegg 's review), or a newer scrape of Richard's list, or that potential other dev you mentioned
  3. optional future step for a separate PR, anything needed to automate or bring the DTC list in a state that makes it easier to maintain and/or regenerate
aaeegg commented 8 months ago

Splitting the DTC table into a separate file is definitely the first step.

A table of tables is probably better than the current format. That said, I don't see an immediate pressing need to change the format. If @brendandburns wants to do the work, I have no objection.

Automation would depend on which DTC list we end up using. If we don't hear back on the better list soon (maybe by Monday?), an updated version of Richard's list should be the best way to move forward.

aaeegg commented 8 months ago

Aleksi Venäläinen, author of the 850 OBD-II Android app, has agreed to provide us with his DTC list.

brendandburns commented 8 months ago

@aaeegg once you get it, if you want to attach the DTC list to this PR, I can refactor this PR to contain that list.

In the meantime I bought Aleksi's app, so maybe that's Karma :)

aaeegg commented 8 months ago

@brendandburns @fenugrec

DTC_List_850OBDII_D2.txt - Aleksi's current DTC list export_2024-04-06_frobbed.txt - Richard's current DTC list

Aleksi's list has more DTCs, but Richard's list seems to have a few that aren't in Aleksi's, so I attached both.

Aleksi's list retains some of the problems of Richard's, such as the typo of EFI-445 for cylinder 6 misfire on 960 (confirmed in VADIS as EFI-456; raw value remains unconfirmed), and the lack of detail on minor vs major misfires.

It turns out that Motronic M4.4, Siemens EMS2000 and Denso ECUs all respond to address 7A, but have different DTCs. So, to handle all these ECUs, we will need to split the table, like fenugrec suggested.

Richard has tried to include as much helpful information as possible in the text, but my opinion is the DTC table should just have the DTC description, and either remove the parentheticals entirely, or put them in a separate "tips" table. Any thoughts on this?

As an aside, it would be nice to also add decoding for J1979 DTCs.

fenugrec commented 8 months ago

either remove the parentheticals entirely, or put them in a separate "tips" table

agreed. Either would be fine. Could also be formatted as an extra field in the eventual DTC struct ?, i.e.

struct {
  u8 dtc[?];
 ...
  const char *short_description;
  const char *tips;

If the source list is in a fairly uniform format, a fancy sed command may even be able to automatically generate the table entries. Whatever the automation method, I think it should be documented / included in one of the table files.

brendandburns commented 7 months ago

Ok, I will take a stab at adding the extra tips field.

Thanks for all the info!

brendandburns commented 7 months ago

Closing in favor of #99