MainForm.ParameterGrid.cs line 80 assumes that ID will be unique

[pmcquay]

At least, I'm reasonably sure thats where this problem comes from. When I load a profile that contains the wideband AFR via ac pressure parameter, it turns into the plain AC pressure parameter.

I suggest using the name as well, or for a more comprehensive solution, getting rid of the ID attribute and using the location convention that is in the RAM parameters XML. Certain PIDs are only available from certain PCMs, so it makes sense to have that be the way they are looked up as well.

I would be open to doing the work to submit a PR, I just want to talk about the best way to fix that before getting started.

[pmcquay]

just had a thought. This issue could be fixed by moving the wideband parameters into the math parameters file, and then this wouldnt be a problem. I guess if I think about it like "standard" parameters are expected to match obd2 pids, then they arent likely to have ids that conflict. Maybe if different manufacturers/pcms use the same id for different things?

[pmcquay]

hmm. I think that solution wouldn't work, because there are quite a few other PIDs in the standard file with multiple definitions, and though I can see where there is code for different parameters being supported by different operating systems, the check just returns true, so it looks like all parameters are loaded into the grid and then referenced by ID. When I select one that isn't supported by my truck, the whole logging interface changes to an error, so I assume there is something going on to ask the PCM if it supports pid X, but even then, its not really enough, because if pid X is supported, and there are multiple definitions of it, then there's a problem.

I cant think of a way to deal with that that doesn't involve making a separate list of pids for every OS, which is definitely a task.

For now, I think the best solution is something like:

1. cut the parameterIdsToRows prop out, because its just used to reference the row in the gridview, and the rows themselves contain the parameter.
2. Implement IEqualityComparer on the parameter base class, and its derivatives (and compare more than ID)
3. use that to compare the column.Parameter prop on line 80 to parameterGrid.Rows, something like this: DataGridViewRow row = parameterGrid.Rows.firstordefault(r => r.cells[1] == column.parameter), and then if(row != null).

I'm going to experiment around in that direction and see what I can come up with. Ideally the whole grid would use databinding, and then it would eliminate most of the back and forth code, as well as the setup code near line 33, that is a much more ambitious task though.

[LegacyNsfw]

First of all, let me acknowledge that you found a real problem here.

Some context on why the parameter files are the way they are, just to make sure we're all on the same page... and keep in mind that I didn't foresee the EGR-as-wideband thing in the beginning:

With the 'standard' parameters, no matter what OS the PCM is running, if you ask it for parameter X, you always get a value with the same meaning (well, as long as that OS supports that parameter). For example, 0x03 is always the fueling mode, and 0x0B is always the MAP sensor, and so on. Many of these parameters are mandated by the OBD2 standard, so all PCMs support them. Others are not as universal - I don't know if that means they were added in a later version of the OBD spec, or if they're outside the OBD spec but GM wanted consistency across their vehicles without putting a lot of OS-specific code into their own diagnostic tools.

Internally, there's a big array of structures in the firmware, kinda like: struct PidTableEntry { int pid; // ID of the parameter int (*function)(); // pointer to a function that gets the parameter value, and converts is to the mandatory units }

When the logger asks for a parameter with an ID in the 0x0-0x2000 range, the PCM looks for the matching element in that array, calls the function to get the value, and returns the value. (If I remember right, it's a sparse array with a few ranges of supported parameters, and finding the matching element is mostly a matter of finding which range it's in, and then indexing into it.) The units are whatever the spec calls for, usually metric with a simple function to map the expected range of values onto 0-255 or 0-65536.

The 'RAM' parameters are completely nonstandard. When the logger asks for a parameter with an ID greater than 0xFF0000, the PCM just reads the value that's in RAM at that address, and returns whatever it finds. Which values are at which addresses depends entirely on what the compiler felt like doing when the OS was compiled. Every OS is different, so I wanted to put all of the parameters for each OS into a single file for that specific OS, partly to avoid confusion and partly to make version control simpler (especially diffing).

The 'Math' parameters were create so that we could have the logger request two or more parameters, do some math on them, and then show the result. Injector duty cycle depends on RPM and pulse width, for example. You could create a MPG parameter with this approach, or volumetric efficiency, or whatever.

So, that said...

Creating parameters with duplicate IDs in the Parameters.Standard.xml file broke some assumptions that I hadn't even realized I was making when I was writing the code, which is why the parameter names change when you save and reopen the files.

I like the idea of using Math parameters to resolve this uniqueness problem. I hadn't envisioned math parameters that only use a single underlying parameter, but I'm pretty sure it will work. They could use names like "Wideband AFR via EGR," IDs like MathWidebandEgr, and so on.

Which IDs did you see that are duplicated in the Parameters.Standard.xml file? I noticed a couple of wideband AFR ones, but I don't think there are too many of them. Moving them into the Math file seems like a pretty straightforward solution.

Assuming it works, of course. I haven't tried it.

[patrickmcquay]

That description jives with what I understand about it, not having the background on how the OS actually works under the hood. I basically understood that there were "universal" parameters, and then "OS specific parameters", but knowing how they work is good.

I just skimmed the file, and it looks like 1104, 1105, 1106, and 1107 have duplicates that don't appear to be related. I haven't tried it either, but when I looked at the code for doing math, I don't recall anything that would prevent it from working on one PID.

[patrickmcquay]

According to excel, these are the ids of all of the PIDs in parameters.standard that have duplicates:

1116 1102 1100 1104 1105 1103 12EE 1106 1144 114B 115C 125B 132A FC05

[patrickmcquay]

I believe I've identified another issue in the math system here that the above PR doesn't fix. (essentially the same issue, but there is no way to use a different identifier in the math system) I need to dig more into it before I am certain, but I think its not really possible for it to differentiate between parameters that have the same ID. I think that it would cause weird issues if one were to be recording two parameters with the same ID, or if you have a math param selected, and change one of the other logged params to a different one with the same ID.

I would still personally prefer to have that PR merged since it doesn't affect the math system, and contains several other QOL fixes. As well, I dont think that this would affect the math as long as you start out with the right parameters selected. The math system does auto-include parameters though, so I think it might automatically pick the wrong one.

In the meantime, unless anyone objects, I'd like to work on the more fundamental change to split the "key" of a parameter away from the pid of a parameter. That would allow quite a few improvements, including fixing the math issue. There are a few ways that that work could go. If this were a personal project I would scrap using XML entirely in favour of JSON, because it is trivially easy to serialize and deserialize. I think for the lists of parameters, a dictionary of objects with a string key (that isn't based on the PID, in the case of PID parameters) would be the most flexible. It would also eliminate something like 200 lines of code from ParameterDatabase.cs that deal with parsing xml and making sure everything goes where it should.

All that said, I know its annoying to have some random come into the project and be like "well hey if you do it this completely different way it would be better!" and I know my personal preferences are not the preferences of @LegacyNsfw or any of the other devs here. I want to work with all of you instead of just trampling things you've done, so I'd like at least a little direction on what the sort of "scope" of changes I should be targeting is:

1. Should I try to maintain backwards compatibility with old lists of params?
2. How many people are using logger? (I've used it to great effect so far)
3. Is anyone here particularly attached to this style of code? (I'm a .net web app dev by trade, so things like in logger where its building a list of parameter groups with 2 byte and 1 byte parameters and splitting them up into different dpids in a big for loop is very old school looking to me. I would have filtered the list to PcmColumns and then ordered the list by ByteCount descending, and then done one for loop over that. Heck there might be a way to split the list into buckets with 6 bytes each with linq as well.)

Wow I've written a book. Sorry about that, I get wordy when I get excited about working on something.