[WIP] Attempt to rebase fork from jonesh

[brendandburns]

This attempts to rebase the jonesh fork of this code onto the current codebase.

It was actually mostly straightforward, the biggest changes/conflicts were in the handling of live data. I went with the jonesh approach, but ported in a couple of data types that weren't present in that code.

This is a work in progress, but feedback is welcome. If it is easier to review in increments, I can send more modular PRs also.

cc @aaeegg

See: #67

[aaeegg]

Thank you for the PR.

If you look at the code currently in freediag, you will see that I have tried hard to make the data interpretation correct. This is why I haven't simply copy-and-pasted the code from Richard H. Jones's version, even though his version is more comprehensive. However, it has been 6 years and freediag is still lacking the full DTC list and live data decoding. It would be good to integrate some of Richard's changes.

Do you honestly believe that the following belongs in scantool_850.c, and not in a commit log?

 * 2017-12-06  jonesrh	Change so same data can be viewed from different ECUs' perspective.
 *			- Requires knowing the presently connected ECU.
 *			- Groups data according to ECU #, so they are easier to find.
 *			- Similar data from different ECUs can be located by searching for part of the printf output,
 *			  eg, to locate all perspectives of the battery voltage, search for "Battery",
 *			  to locate all temperatures, search for "Temp", etc.
 *			Change so non-NS_MEMORY addr is *not* shifted up by 8 bits by the caller,
 *			  since there seems to be no need for that.
 *
 * 2017-12-07  jonesrh	Remove the UNUSED attribute on len parameter, since
 *			  non-NS_MEMORY blocks may use the length to infer info.
 *			Ahhh!  Now I see why the non-NS_MEMORY were coded with that 8-bit shift --
 *			  the 10 bytes of an 850 COMBI E507 (live data 07) message would have been passed to
 *			  interpret_value() as 10 addresses of: 0700, 0701, 0702, ..., 0708, 0709.
 *			  I get it.  That's actually a very valid approach.  
 *			  But that's *NOT* how I've transcoded the KWPD3B0 interpreter into interpret_value(),
 *			  and I just don't want to (nor have the time to) alter the days of work already spent 
 *			  making the guts of the KWPD3B0 interpreter work in this freediag 850 / ELM327 / D2 interpreter.
 *			  So I've arbitrarily decided to let stand this architectural change to interpret_block(),
 *			  since the old (pre-2017-12-06) implementation causes bogus interpretations
 *			  when interpret_value() is called for a 2-byte data item.
 *			Late 2017-12-07:
 *			  For conditionalization simplicity, keep the old interpret_value() as is,
 *			  and rename the jonesrh version of interpret_value() as interpret_value_or_msg().
 *			  This also makes comparison between the fenugrec 2017-11-06 and jonesrh 2017-12-07
 *			  versions easier to see.

Likewise, does the following seem to be of the same quality and style as the surrounding code? The variable i, used as an index counter elsewhere in the function, is reused here to hold a destination address. Then there are a bunch of unexplained special cases for specific destination addresses. Then the retry loop uses a variables with impenetrable names, one of which has had its value set nearly 150 lines previously -- but moreover, the retry loop is a kludge to handle a condition that was already fixed in eb02732. The changes to cmd_850_id_d2 do include an actual improvement, to handle revision numbers that contain nulls, but that's buried among the garbage.

	i = global_l2_conn->diag_l2_destaddr;
	switch (i) {
	case 0x11:
	case 0x29:
	case 0x2D:
	case 0x51:
	case 0x58:
		for (iddatetimetry=1; iddatetimetry <= maxiddatetimetry; iddatetimetry++) {

These are just two examples, chosen at random, where your approach seems to have been "throw in this pile of code and try to get it to compile, without understanding or even reading any of it".

I disapprove of this and #95 being accepted in their current form.

[brendandburns]

@aaeegg thanks for the feedback.

This is definitely a work in progress and not intended to merge as is. It was basically an attempt to incorporate all of the existing diffs from the 2017 fork into the existing master. The code (and the comments) were included verbatim from the fork without any attempt to improve the style of functionality. This was done to make it easy to understand relative to the forked codebase.

If the preference is to take inspiration from the fork but to write fresh code, that's fine too.

For #95 what is the specific objection? I don't see any obvious style issues with that list of DTCs but if you have specific edits that you'd prefer, I'm happy to address those.

As a way to move forward, since it seems that merging the code from the existing fork into this codebase isn't viable, is there a highest value area to rewrite? Perhaps the significantly expanded live data handling code? I'd be happy to adapt that to match the existing style.

[aaeegg]

@brendandburns I'm sorry for my harsh words. As you can see, Richard did a tremendous amount of work researching and documenting the Volvo diagnostics. Without him, D2 protocol support in freediag would not have been possible. There is good stuff in his fork, but I feel that a lot of it either doesn't belong in freediag, or should be rewritten before integrating it. My approach so far has been to add specific functionality as I need it.

Since the DTC list solved an actual pain point you were experiencing, why don't we start with that? I will comment about that on #95.

[brendandburns]

Sounds good to me. I would like to also import the livedata visualization stuff which is also significantly more extensive than what is currently in freediag. I will send that as a stand-alone PR (with cleanups) after we get the DTC one merged.

[brendandburns]

I'm going to close this PR for now since I don't think it is moving forward. I'll submit a refactored live data PR sometime later.