Some thoughts around the codebase

[ptsd]

Hi,

Just some thoughts from someone who codes as a day job. No offence intended, on a whole the codebase is pretty well laid out and easy to follow. However, i'm playing with adding my custom STM32 based USB CAN adapter to the lib and have the following thoughts:

Solution Structure

• There is some serious abstraction leaking going on between CANFLasher and CANLib, would probably make sense to harden the API and actually split these into separate repositories.

ICANDevice

• The abstract class isn't super clear, I'd expect an implementation to work as long as it successfully implements all abstract methods, but there are some overrides required you can only work out by either reviewing other implementations or trying to run and fixing all the errors.

General Style:

• Magic numbers are everywhere! This is to be expected in a low(ish) level hardware abstraction, but is seriously hard to follow when you aren't knee deep in this stuff. They are commented in a lot of places, but would be nice to move a lot of these into a dedicated area.
• Commented out code is everywhere. This is the purpose of source control, there should be no commented out code. If you need to explain why something has changed, a good commit message, plus a source comment if absolutely necessary is sufficient. Most modern git implementations do an outstanding job of showing the history of a particular file or section of code.
• General formatting is a mish mash, would be good to get some kind of standard and then get a linter in.

Would you be amenable to some PRs around these areas if I have some free time in the next couple of months?

Again i'm not poking holes here, just taking the approach I would with any new (to me) codebase.

Regards,

Pat

[mattiasclaesson]

Hi,

I'm always open to PRs. Please check with me before you do alot of work.

I really would like more unittests, we done some, for the vindecoder in the suite but that is it really.
Did some tries with simple regression tests, but the integrated ui teststuff in Visual Studio seem to be hard to use. Maybe a 3rd party tool would be better.

Yes the API is sort of the same as Dilemma the original author came up with a long time ago. I did have a idea to combine support for the different flavors of trionic in one program to avoid the massive code duplicating that existed, and trioniccanflasher was born. It has sort of grown out of the trionic space now so maybe a namechange will come. The original author did not use source control.

If you think the flasher is interesting, check the tuning suits. But I find it relaxing to make it better with no rush. Have enough of that at work.

One thing is that I really wanted to use the same CAN lib for both the suits and the flasher. Apart from t5suite and t5canflasher all CAN code now share the same lib.

The suites use a commonsuite lib to take care of shared code, Now the suite and flasher will more and more overlap in functionality. Features like checksum validation/correction is usable in the flasher aswell as the suite. When ever I look around the code the t7 and t8 suites becomes more similar.

I really did think I would be splitting up components one per repo, but it sort of grew together instead.
Doing refactorings is really painfull now with the suite code in one repo and the CAN lib in another one.
I'm considering moving the flasher stuff into the tuningsuite repo to make it easier to work with both at the same time.

I just did a test to go over to wix installer in the flasher, maybe you have used that one?

Please reach out on messenger, skype, hangout and we can discuss more.

Best Regards,
Mattias Claesson