Logger improvements

They go by many names, P01, P59, VPW, '0411 etc. Also covering E38 and newer here.
pmcquay
Posts: 51
Joined: Fri Jan 31, 2020 4:38 am

Re: Logger improvements

Post by 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
Posts: 27
Joined: Thu Apr 13, 2023 6:52 am

Re: Logger improvements

Post by rjdrew1986 »

Just curious, what are the symptoms when Crank Relearn is needed?
gmtech825
Posts: 197
Joined: Fri Feb 24, 2017 11:27 am

Re: Logger improvements

Post by 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).
User avatar
NSFW
Posts: 691
Joined: Fri Feb 02, 2018 3:13 pm

Re: Logger improvements

Post by 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.
Please don't PM me with technical questions - start a thread instead, and send me a link to it. That way I can answer in public, and help other people who have the same question. Thanks!
User avatar
NSFW
Posts: 691
Joined: Fri Feb 02, 2018 3:13 pm

Re: Logger improvements

Post by NSFW »

Boostedforlife wrote: Mon May 06, 2024 2:52 am 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.
Please don't PM me with technical questions - start a thread instead, and send me a link to it. That way I can answer in public, and help other people who have the same question. Thanks!
User avatar
NSFW
Posts: 691
Joined: Fri Feb 02, 2018 3:13 pm

Re: Logger improvements

Post by NSFW »

pmcquay wrote: Wed May 08, 2024 3:45 am 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.
Please don't PM me with technical questions - start a thread instead, and send me a link to it. That way I can answer in public, and help other people who have the same question. Thanks!
pmcquay
Posts: 51
Joined: Fri Jan 31, 2020 4:38 am

Post by 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.
User avatar
NSFW
Posts: 691
Joined: Fri Feb 02, 2018 3:13 pm

Re: Logger improvements

Post by 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 }".
Please don't PM me with technical questions - start a thread instead, and send me a link to it. That way I can answer in public, and help other people who have the same question. Thanks!
pmcquay
Posts: 51
Joined: Fri Jan 31, 2020 4:38 am

Re: Logger improvements

Post by 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 :D. 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.
User avatar
antus
Site Admin
Posts: 8299
Joined: Sat Feb 28, 2009 8:34 pm
cars: TX Gemini 2L Twincam
TX Gemini SR20 18psi
Datsun 1200 Ute
Subaru Blitzen '06 EZ30 4th gen, 3.0R Spec B
Contact:

Re: Logger improvements

Post by 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.
Have you read the FAQ? For lots of information and links to significant threads see here: http://pcmhacking.net/forums/viewtopic.php?f=7&t=1396
Post Reply