Logger improvements

[pmcquay]

If you want someone to help, I'd be more than willing, and yeah not having the logging mess with the flashing would be priority there I'd imagine. I'd just need to know where to start so as to not step on anyone's toes, or duplicate work. I'm willing to do simple code cleanup and organization tasks as well, if things need moved around for that sort of work.

[rjdrew1986]

Just curious, what are the symptoms when Crank Relearn is needed?

[gmtech825]

If the value has never been learned than the ecm sets a dtc. otherwise some service procedures require you to relearn it for misfire detection (engine replacement).

[NSFW]

Yep. I want it for the misfire detection. What I thought was a rough cam turned out to be a flaky ignition harness, which I confirmed by using a temperature gun on the exhaust headers. I thought I had it fixed, but now I'm not sure.

[NSFW]

NSFW do you have a text version of the disassembly? so I can look at it and learn how the computer work might help me put 2 and 2 together

I do, sorry for the delay.

https://raw.githubusercontent.com/Legac ... itized.asm

At 14mb, it's quite large. I don't know what software will be able to handle it gracefully, but try Visual Studio Code if you don't have something already.

[NSFW]

If you want someone to help, I'd be more than willing, and yeah not having the logging mess with the flashing would be priority there I'd imagine. I'd just need to know where to start so as to not step on anyone's toes, or duplicate work. I'm willing to do simple code cleanup and organization tasks as well, if things need moved around for that sort of work.

There is a code-cleanup thing that's been on my mind for a looong time, but it's not exactly simple.

When I started writing the code, I had this idea that it might be handy to have most functions return an object containing two values: a status code and the value that the function was really supposed to provide. It's similar to how web servers return an HTTP status code and a page of HTML (or a JSON data structure, or XML, or whatever). So there are a ton of functions that return a Response<bool> or a Response<string> or something like that. This turned out to be a bad idea.

It's a lot easier to just return the bool or string, and throw an exception if something goes wrong. That way the exception only needs to be caught by the code that really wants to handle it. Whereas with the return-an-error-code approach, we have to check response.Status at every layer in the call stack, and explicitly return a response object with the same status code (and often a different default value). It just adds a bunch of extra code that provides no real value, and it creates a place for bugs to creep in, and makes it harder to find bugs.

I just pushed up a new branch named 'exceptions' that contains a new ObdException class.

This:

Code: Select all

return Response.Create(ResponseStatus.Success, foo) 

Should become this:

Code: Select all

return foo

This:

Code: Select all

return Response.Create(ResponseStatus.some-error, foo) 

Should become this:

Code: Select all

throw new ObdException(ObdExceptionReason.some-error, "a few words about what went wrong")

Things like this - checking for an error just to return a slightly different Response object - should probably just be removed, because exceptions will bubble up automatically. There might be some value in catching, logging a message, and re-throwing, but I suspect that in most cases that won't really be helpful.

Code: Select all

                    if (response.Status != ResponseStatus.Success)
                    {
                        logger.AddUserMessage("Failed to load loader from file.");
                        return new Response<Stream>(response.Status, null);
                    }

Something like this - checking for an error and returning something simple:

Code: Select all

                        response = await vehicle.LoadKernelFromFile(this.pcmInfo.LoaderFileName);
                        if (response.Status != ResponseStatus.Success)
                        {
                            logger.AddUserMessage("Failed to load loader from file.");
                            return false;
                        }

...should turn into this:

Code: Select all

                        try
                        {
                            file = await vehicle.LoadKernelFromFile(this.pcmInfo.LoaderFileName);
                        }
                        catch (Exception exception)
                        {
                            logger.AddUserMessage("Failed to load loader from file.");
                            logger.AddUserMessage(exception.Message);
                            return false;
                        }

I suspect that those patterns will cover most of the changes that need to be made, but I also suspect there will be places where it won't be obvious what should be done. But we can figure it out.

[pmcquay]

I can certainly take a look at untangling that sort of thing. Honestly refactoring and simplifying stuff to be more readable is one of the things I enjoy most in programming. I did a little of it I think when I fixed the duplicate IDs issue. I wasn't sure if that's the way you wanted to go though. There is a lot of "TryX" type code that's in there as well that could be improved similarly. Stuff that just returns a boolean and has an out parameter, when it could just return what it needs to return (and when it needs to return it, without having to go through the whole function), and then throw if something exceptional happens.

[NSFW]

I'm a fan of the TryX pattern, because I think "int foo; if (!TryX(out foo)) { handle error }" is a little more readable than "int foo; try { foo = X() } catch (Exception ex) { handle error }".

[pmcquay]

Sure, It's a more "C" way of doing it, and it has its uses. The problem comes when you want relevant information with your errors, or you have many things that you need to check, or you're working in a language that expects you to handle exceptions so you need to check those too . Also, the second pattern doesn't always need to be a try catch. Most of the time, exception handling code is and should be done in a central place, making your second example just "int foo = X();". IMO, With the addition of a global logging framework that doesn't need to be passed around everywhere for informational logging, it would really clean up the library code.

It is possible to do global exception handling in winforms, see: https://stackoverflow.com/questions/814 ... n-handling, but unless the rest of the project expects it, I think its a bit ambitious. I would generally favour handling exceptions in the winforms events.

I'm primarily a .net dev, but I do really wish sometimes that .net had the equivalent of the Java "throws" method signature keyword. It makes dealing with exceptions way more obvious. I'm a huge fan of patterns that do not allow me to shoot myself in the foot by forgetting something.

[antus]

I would like to merge antus/p08 branch in to develop soon. If anyone is thinking about refactoring, please give me a heads up first and I'll merge. Just not certain the P12 code is healthy at the moment, as there was an error on one of Darkmans's PCMS. Not sure if it was something specific to the code or just a 1 off from some other external factor. It works on my P12b OK. There are heavy changes and several new PCM targets in that branch and we're going to get in to merge hell if we refactor from develop now.

It's also be good to start thinking about proper can bus support. E38 or some other can target might come next. No promises.