Add extra DTCs for Volvo 850 & S/C/V70 as well as parser for generating them by brendandburns · Pull Request #99 · fenugrec/freediag

brendandburns · 2024-04-18T03:06:16Z

This is a replacement for #95

Consider it a rough draft to see if it is in the right ballpark. It depends on the refactors in #98

Open questions:

Do we want to compile both lists in so that users can select at runtime?
Previously there was a desire to split into descriptions and tips, any ideas about how to do this programatically?
Should we include the original source files into the repo? I'm not sure about the licensing/OSS implications of that.

Anyway, please take a quick look to see if it is reasonable before we move onto more detailed review.

cc @aaeegg

aaeegg · 2024-04-24T06:36:53Z

I think we ought to merge the lists into a single "best available" list.

The tips are mostly enclosed in parentheses or brackets, or after a newline character.

The Xiaotec import didn't come out quite right - decoded suffix is duplicated in the description string, SRS and ROPS codes ended up in the CI table, duplicate SRS codes for 1 and 2 byte DTCs...

for ecu/dtc.h

brendandburns · 2024-04-27T20:44:51Z

@aaeegg @fenugrec refactored this to match the changes from that were recently merged.

I think that this is mostly ready to go, modulo the discussion about splitting some of the longer descriptions into descriptions and tips.

Please take a look.

fenugrec

Have you given thought to combining the arrays ecu_dtc_map[] and ecu_list[] ?
e.g.

    { 0x41, "IMMO", "IMM", dtc_list_41 },
    { 0x42, "CCU ( Cab Control Unit ) aka Convertible Top / Roof Module or Later modules known as CAB (Cab Control System)", "unknown", dtc_list_empty },

One drawback I see is that the ECUs with no known DTC lists need to be handled slightly differently to point to a dummy dtc_list_empty array.

fenugrec · 2024-04-28T00:03:25Z

scantool/scantool_850/ecu.c

 }


+#ifdef DEFAULT_850_ECU


Do we need to keep these at all ?

fenugrec · 2024-04-28T00:04:26Z

scantool/scantool_850/config.h

+// #define __SCANTOOL_850_DTC_XIAOTEC__
+
+// Sourced from Richard Jones (https://jonesrh.info/volvo850/index.html)
+// #define __SCANTOOL_850_DTC_FROBBED__


why frobbed ?

fenugrec · 2024-04-28T00:06:23Z

scantool/scantool_850/parser.py

+        self.prefix = prefix
+
+
+# this loads dtcs from the xiaotec file export


is this function just applicable to the xiaotec import ?
Then naming should be made more uniform in comments, function names, and filenames; currently I see inconsistent use of "Aleksi", "richard", "xiaotec", and "frobbed"

fenugrec · 2024-04-28T00:07:10Z

scantool/scantool_850/parser.py

+    for d in dtcs:
+        key = d.prefix + d.suffix
+        ecu_key = d.ecu
+        if ecu_key == '2E':


wha'ts special about 2E / 2F here ?

2E is the left power seat and 2F is the right power seat. The DTCs are the same, only the address is different. (Except on model year '96, where they put them both on 2F, and the tester can't talk to them unless you unhook one or the other. Oops.)

fenugrec · 2024-04-28T00:11:11Z

scantool/scantool_850/parser.py

+                .replace('{{ecu_address}}', ecu.address)
+                .replace('{{ecu_description}}', ecu.description)
+                .replace('{{ecu_prefix}}', ecu.prefix))
+#            ecu_list.append('    { .addr = 0x' + ecu.address + ', .desc = "' + ecu.description + '", .dtc_prefix = "' + ecu.prefix + '" },')


dead code ? should be removed

fenugrec · 2024-04-28T00:12:46Z

scantool/scantool_850/parser.py

+        suffix_template = suffix_template.replace('{{ecu_list}}', '\n'.join(dtc_ecu_list))
+        print(suffix_template)
+
+# Example call: python3 parser.py richard sources/export_2024-04-06_frobbed.txt templates/ frobbed > frobbed.c


These are nice !! Better put them near the top, not hidden way at the end

fenugrec · 2024-04-28T00:15:09Z

scantool/scantool_850/templates/ecu_item.mustache

@@ -0,0 +1 @@
+    { .addr = 0x{{ecu_address}}, .desc = "{{ecu_description}}", .dtc_prefix = "{{ecu_prefix}}" },


As I mentioned earlier, unless there's a really compelling reason to use designated initializers, I think {0x{{ecu_address}}, "{{ecu_description}}", "{{ecu_prefix}}" }, will be unambiguously clear ?

aaeegg · 2024-05-14T08:16:57Z

@aaeegg @fenugrec refactored this to match the changes from that were recently merged.

I think that this is mostly ready to go, modulo the discussion about splitting some of the longer descriptions into descriptions and tips.

Please take a look.

@brendandburns @fenugrec

{0x05, 313, "ABS-313: Left Rear Wheel Sensor, open/short? [or bad ABS module solders or ignition switch]", NULL},

All the entries in xiaotec.c are like this - the suffix is in the description string when it shouldn't be. The dtc command prints the DTC designator, so you would see it twice - it would print "ABS-313 (05) ABS-313: Left Rear Wheel Sensor...". It appears that the code was not tested or proofread before submitting this PR.

    {0x73, 324, "SRS-324: Right Rear Seat Belt Tensioner signal too high", NULL},
    {0x0001, 112, "SRS-112: Crash Sensor Module internal fault", NULL},

One-byte and two-byte SRS DTCs mixed in the same table, mentioned in #99 (comment) before you "refactored", not fixed.

    {0xFD, 414, "CI-414: Road Speed Signal missing or faulty", NULL},
    {0x01, 112, "SRS-112: Crash Sensor Module internal fault", NULL},

SRS and ROPS DTCs wrongly placed in the CI table! Mentioned in #99 (comment)! Not fixed!

    {0x11, 321, "Left Front Wheel Sensor, irregular > 25 mph (ie, interference or excess oscillation > 40 km/h) [or bad ABS module solders or ignition switch].", NULL},
    {0x13, 211, "Left Front Wheel Sensor, wrong wheel speed (ie, signal absent yet circuit intact, or signal absent when moving off) [or bad ABS module solders or ignition switch].", NULL},

DTC 0x12 for ABS missing from frobbed.c. I had mentioned in #95 (comment) that 888, as in Richard's list, is not the real suffix for this DTC; the suffix is (was) unknown. But although the suffix is unknown, the description is known. Removing the DTC is throwing out the baby with the bathwater.

const struct ecu_dtc_table_map_entry ecu_dtc_map[] = {
    {0x01, dtc_list_01},
    {0x11, dtc_list_11},
    {0x29, dtc_list_29},
    {0x41, dtc_list_41},
    {0x51, dtc_list_51},
    {0x58, dtc_list_58},
    {0, NULL},
};

What happened to 0x7A? All the Motronic M4.4 DTCs are missing from frobbed.c! If you try to scan the EFI of your '98 V70, no DTCs for you!

    {0xFE, 337, "EFI-337: Torque Limiting Signal from DSA faulty", NULL},
    {0x65, 112, "EFI-112: MFI Control Module (CM) fault", NULL},

And in xiaotec.c, the DTCs for EMS2000 and M4.4 have been thrown into one table. That's not going to work.

{0x3F, 551, "EFI-551: Misfire, cylinder 1", NULL},

I mentioned in #95 (comment) that EFI-551...555 and 545 are misfires with catalyst damage, description has not been updated to distinguish them.

{0xA4, 335, "EFI-335: Request for MIL lighting from TCM [P1618] [or possibly Comm Failure between Motronic 4.4 ECM and AW 50-42 TCM]", NULL},

As I mentioned in #95 (comment), before you even opened this revised PR, EFI-335 is not MIL request from TCM.

{0x37, 414, "Driver's (or passenger's) side interior temperature sensor inlet fan seized (or open/short circuit or faulty signal) [carefully clean out any lint from temp sensor fan(s)]; else (inlet)Fan motor passenger comp (interior) temp sensor, Faulty Signal [according to xiaotec \"850 OBD-II\" V1.3.4 app].", NULL},

As I mentioned in #95 (comment), there is no such thing as a driver's or passenger's side interior temperature sensor on any vehicle whose ECC we can talk to. This is the code you observed on your car, and you can verify that the description is wrong because there is only one interior temperature sensor.

I don't think the code in this PR is "mostly ready to go".

I submitted a merged, hand-edited table as a new PR #100 with @brendandburns as co-author for the commit.

fenugrec · 2024-09-09T18:36:52Z

I merged #100 in the meantime, since this current PR needs work for acceptance. I'm leaving this open since a 'proper' script may be a useful future upgrade.

brendandburns mentioned this pull request Apr 18, 2024

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

Closed

fenugrec mentioned this pull request Apr 22, 2024

Refactor dtcs into a ecu->dtc_list table of tables, move them out into separate files #98

Closed

brendanburns and others added 4 commits April 25, 2024 18:43

Add code generation from files

183ba11

Add code generation from files

425147a

Add two new sets of DTCs and a parser to generate them

3cefed9

Rebase and refactor to match the new structure

78a73ec

for ecu/dtc.h

brendandburns force-pushed the stage2 branch from 8239d65 to 78a73ec Compare April 27, 2024 20:43

fenugrec reviewed Apr 28, 2024

View reviewed changes

aaeegg mentioned this pull request Aug 1, 2024

Interpretation for many more DTCs on Volvo vehicles #100

Closed

		self.prefix = prefix


		# this loads dtcs from the xiaotec file export

		@@ -0,0 +1 @@
		{ .addr = 0x{{ecu_address}}, .desc = "{{ecu_description}}", .dtc_prefix = "{{ecu_prefix}}" }, No newline at end of file

		}


		#ifdef DEFAULT_850_ECU

Conversation

brendandburns commented Apr 18, 2024

Uh oh!

aaeegg commented Apr 24, 2024

Uh oh!

brendandburns commented Apr 27, 2024

Uh oh!

fenugrec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaeegg commented May 14, 2024

Uh oh!

fenugrec commented Sep 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants