Skip to content

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

Closed
brendandburns wants to merge 2 commits into
fenugrec:masterfrom
brendandburns:dtc
Closed

Refactor dtcs into a ecu->dtc_list table of tables, move them out into separate files#98
brendandburns wants to merge 2 commits into
fenugrec:masterfrom
brendandburns:dtc

Conversation

@brendandburns

Copy link
Copy Markdown
Contributor

@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

fenugrec commented Apr 11, 2024

Copy link
Copy Markdown
Owner

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

  • printable strings should be const char *
  • ptr to DTC table should be const struct XYZ *
  • table, and table of tables, should also be const (I didn't notice if they already are, ignore this comment if so )

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

brendandburns commented Apr 12, 2024

Copy link
Copy Markdown
Contributor Author

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

aaeegg commented Apr 14, 2024

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Owner

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)

Move them out into separate files.
@brendandburns

Copy link
Copy Markdown
Contributor Author

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

fenugrec commented Apr 22, 2024

Copy link
Copy Markdown
Owner

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

Copy link
Copy Markdown
Owner

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 .

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 this pull request may close these issues.

4 participants