fenugrec / freediag

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

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

Closed brendandburns closed 7 months ago

brendandburns commented 7 months ago

@aaeegg @fenugrec

This is the first step as discussed in #95

I have refactored the existing codes/codebase to make the table of tables and prepare for different lists of dtcs.

There should be no actual changes in functionality.

Please let me know if there's any feedback or changes to the approach desired.

fenugrec commented 7 months ago

Just skimming the code in less than 5 minutes (will need to take a closer look later), I think you're on the right track.

Some notes; many struct members should be marked const, such as

also, make sure you've run our uncrustify.cfg formatter prior to committing. I didn't check if your 'formatting' commit applies to existing code, or stuff you just added.

Commit message also need a bit of work, see how previous messages are formatted.

Also, I think the table of tables should be "unpacked", currently you have everything in the one main array, but that will quickly get out of hand when adding more. So e.g. I would prefer

static const struct dtc_table_entry ecu_whatever_dtclist[] = {
  {0x54, 445, "Pulsed secondary air injection system pump signal", NULL},
  {0,0,NULL,NULL}
};

static const struct ecu_dtc_table_map_entry? ecu_dtc_map[] = {
  {0x10, ecu_whatever_dtclist},
  { .....},
  {0, NULL}
};

(I wouldn't bother with designated initializers for these tables unless there's a valid case where some members should not be initialized, or if there are so many members that readability suffers)

brendandburns commented 7 months ago

Just so I'm clear, the current structs for ECU: https://github.com/fenugrec/freediag/blob/master/scantool/scantool_850.c#L57

And DTC: https://github.com/fenugrec/freediag/blob/master/scantool/scantool_850.c#L95

Have printable strings which are char* not const char*

I don't mind making them const in this PR (or a separate PR) but I wanted to make sure that was the desired intent.

Let me know what you prefer.

I will refactor the table of tables to be "unpacked" as you suggest.

aaeegg commented 7 months ago

The strings aren't going to be modified during execution, so they should have the const qualifier so the compiler knows to put them in the rodata section and apply appropriate optimizations. I snuck the omission past fenugrec but you weren't so lucky.

As long as you're making tweaks - freediag uses C-style comments in some places and C++-style comments elsewhere, but the new comment in dtc_raw_by_printable should be C-style to match the surrounding code.

fenugrec commented 7 months ago

I don't mind making them const in this PR

Thanks, I just took care of it (as well as some other unrelated, low-hanging fruit in scantool_850.c flagged by cppcheck)

brendandburns commented 7 months ago

Ok, rebased and I think I addressed all of your comments.

I also ran uncrustify on scantool_850.c which resulted in some unrelated changes. Let me know if you'd prefer those in a separate PR.

Please take another look when you get a chance.

fenugrec commented 7 months ago

Merged (reworded commit message) as of #8aabe66c834230e7152f9c066cd20428b289e165

Slightly reorganized code and functions, I wasn't a fan of having a struct definition in basic.h.

I also split DTC and ECU lists in separate files, thinking they may evolve independantly, but I don't mind re-merging them if you guys want to eliminate the ecu_dtc_map array entirely; its sole interesting member const struct dtc_table_entry *dtc_table; could just be moved to struct ecu_info ?

fenugrec commented 7 months ago

Oops, I should've looked at your PR #99 first. Some of the changes I made make less sense. Let me know if anything needs be reverted / changed to not completely break your code generators .