stanleyhuangyc / Freematics

Official source code repository for Freematics
https://freematics.com
430 stars 349 forks source link

'isValidPID' causing problems accessing available/valid PIDs on vehicle #135

Open wally2511 opened 3 years ago

wally2511 commented 3 years ago

Having issues accessing several PIDs. When I disabled the 'isValidPID' line of code the data flows fine.

//if (!obd.isValidPID(pid)) continue;

maxdd commented 3 years ago

Have you checked the content of pidmap and the behaviour of the function? pidmap is created at runtime and if in the init phase the pid request is not answered then it is considered has a not valid/available pid.

wally2511 commented 3 years ago

@maxdd What's the best method of debugging the contents of pidmap? I saw it was part of the obd.init function but couldn't work out how to print/dump the contents of it.

It's not as simple as Serial.println(pidmap); from the main sketch is it?

maxdd commented 3 years ago

i would start here

https://github.com/stanleyhuangyc/Freematics/blob/893f9d94705dae0d4e57ff21b0bd6fbd25e8cd1e/libraries/FreematicsPlus/FreematicsOBD.cpp#L331

wally2511 commented 3 years ago

I used the uart_forwarder sketch to run the "PIDMAP" command. This was the result

01 00
41 00 98 3B 80 13 
41 00 88 18 00 13 
wally2511 commented 3 years ago

Ok. So an update.

I found a bit of software to decode payloads from the 0100, 0200 and 0400 AT commands (https://glmsoftware.com/Downloads1.aspx). All the PIDs in my sketch are supported by the vehicle but still doesn't work.

I am still trying to nut out how the PIDMAP is created in the init function. I am wondering if the two-line response to 01 00 is causing issues?

The first line is the response from the engine ECU and the second is from the transmission.

Is PIDMAP only getting populated with the supported PIDs of the automatic transmission? ie 0x01, 0x05, 0x0c, 0x0d, 0x1c, 0x1f

maxdd commented 3 years ago

I don't understand if the isValidPID is returning true or false for you. If it returns true then i would say they are supported. Also have u checked the behaviour at this line?

https://github.com/stanleyhuangyc/Freematics/blob/893f9d94705dae0d4e57ff21b0bd6fbd25e8cd1e/libraries/FreematicsPlus/FreematicsOBD.cpp#L389

What happens here? Also the sketch about uart forwarding is just forwarding commands between the uart and not checking the "internal" behaviour. It doesn't even use FreematicsOBD.cpp!!

wally2511 commented 3 years ago

The isValidPID is return true for some PIDs and false for other PIDs that I know are valide (ie. they return results when I disable isValidPID. It only returns true for PIDs that the automatic transmission ECU responds (the second line) to the 01 00 command with. It appears that the PIDMAP doesn't contain the valid PIDs reported by the engine ECU (the first line).

01 00 41 00 98 3B 80 13 41 00 88 18 00 13

maxdd commented 3 years ago

That was already discovered by you, now we need to understand why. The pidmap is filled only with the pids answered from pid 0, pid 20, pid 40 ecc.. which are the pids used to retrieve the supported ones. In which case does it responds as timeout or with errors? Also what pid are you interested in? Seems like 0x4, 0xB, 0xF, 0x10, 0x11

You can check here

https://github.com/stanleyhuangyc/Freematics/blob/893f9d94705dae0d4e57ff21b0bd6fbd25e8cd1e/libraries/FreematicsPlus/FreematicsOBD.cpp#L419

for the error you are receiving

wally2511 commented 3 years ago

I'm after the folloiwng PIDs {PID_SPEED, 1}, {PID_RPM, 1}, {PID_RELATIVE_THROTTLE_POS, 1}, {PID_ENGINE_LOAD, 1}, {PID_INTAKE_MAP, 1}, {PID_MAF_FLOW, 1}, {PID_COMMANDED_EGR, 2}, {PID_EGR_ERROR, 2}, {PID_COOLANT_TEMP, 3}, {PID_INTAKE_TEMP, 3}, {PID_BAROMETRIC, 3},

In the main sketch it responds with a false to the isValidPID from some pids and continues on with the processOBD for loop.

I can't quite get my head around how it works but believe the problem exists in the following snippet of code. It isn't reading the buffering the engine ECU response 41 00 98 3B 80 13

// load pid map
    memset(pidmap, 0xff, sizeof(pidmap));
    for (byte i = 0; i < 8; i++) {
        byte pid = i * 0x20;
        sprintf(buffer, "%02X%02X\r", dataMode, pid);
        link->send(buffer);
        if (!link->receive(buffer, sizeof(buffer), OBD_TIMEOUT_LONG) || checkErrorMessage(buffer)) {
            break;
        }
        for (char *p = buffer; (p = strstr(p, "41 ")); ) {
            p += 3;
            if (hex2uint8(p) == pid) {
                p += 2;
                for (byte n = 0; n < 4 && *(p + n * 3) == ' '; n++) {
                    pidmap[i * 4 + n] = hex2uint8(p + n * 3 + 1);
                }
            }
        }
    }
wally2511 commented 3 years ago

I am not sure how to use the checkErrorMessage function? What parameter do I pass to it?

maxdd commented 3 years ago

If you print the "buffer" content you should see the answer to each supported pid request e.g. (41 00 98 3B 80 13). Also we don't have access to STM32 code which is in charge of OBD/CAN (the developer didnt release/public it) It might be that the ECU is actually answering that those pids are not available. If you want a fast/hacky solution you could create your own function that adds known pids to the pidmap variable. checkErrorMessage is not a function you should call but it is already called in the official firmware so you should modify the function to print the value

wally2511 commented 3 years ago

The ECU is answering that the PIDs are available it will return the live data when I comment out isValidPID. For now, I'm just I'm just leaving out the isValidPID line int he telemetry sketch.

I will play with the checkErrorMessage function when I can get the device to quite boot looping from the esp_himem: Cannot allocate memory for meta info. Not initializing!

It might be that the ECU is actually answering that those pids are not available.

maxdd commented 3 years ago

I think you are misunderstanding few things. Ill strongly suggest you to have a look at https://en.wikipedia.org/wiki/OBD-II_PIDs#Service_01_PID_00 One thing is the pid to retrieve the "supported pid" (done in the OBD init and contained in pidmap) One thing is the actual pid containing the information such as vehicle speed, engine speed ecc "01 00" is the request for "supported pid" and the ECU might respond with a value (unfortunately in this case) which is not the truth. If the value contains for example the 4th bit high then it means it supports pid "04" "01 04" is for example the calculated engine load and if it is supported (4th bit of the above answer) then it is answered with a value. What it is curious is that you mentioned you are using the Automatic Transmission ECU, could it be that the ECU itself responds with unsupported pids because it is forwarding it to the ECM? Are you testing on a car or on a bench?

wally2511 commented 3 years ago

I think I'm tracking with your train of thought.

I understand how the 01 00 query and payload works. I used an app to decode the payload and saw all the supported PIDs I understand that 0C will return a payload that displays the current RPM

I am testing in the car. Below is my results of running "01 00" query on two of my cars (I turned on headers so you can see the addresses of the ECUs that are replying)

Mitsubishi Pajero - 01 00 7E8 06 41 00 98 3B 80 13 7E9 06 41 00 88 18 00 13

Hyundai i30 - 01 00 7E8 06 41 00 98 3B 00 11

You can see that the Pajero returns two payloads and the Hyundai only one. The first payload from the pajero is the and ECU contains all the supported PIDs. When I bypass isValidPID I can get the freematics device to return the PID values (rpm, intake temp, speed ...etc). So the Pids are supported by the ECU and they return the correct value when queried from the telemetry sketch.

What seems to be the problem is the isValidPID function and the values in the pidmap array?? The isValidPID function on the Pajero returns true on all the values in the second payload which is from the Transmission.

On Fri, 23 Apr 2021 at 19:06, maxdd @.***> wrote:

I think you are misunderstanding few things. Ill strongly suggest you to have a look at https://en.wikipedia.org/wiki/OBD-II_PIDs#Service_01_PID_00 One thing is the pid to retrieve the "supported pid" (done in the OBD init and contained in pidmap) One thing is the actual pid containing the information such as vehicle speed, engine speed ecc "01 00" is the request for "supported pid" and the ECU might respond with a value (unfortunately in this case) which is not the truth. If the value contains for example the 4th bit high then it means it supports pid "04" "01 04" is for example the calculated engine load and if it is supported (4th bit of the above answer) then it is answered with a value. What it is curious is that you mentioned to using the Automatic Transmission ECU, could it be that the ECU itself responds with unsupported pids because it is forwarding it to the ECM? Are you testing on a car or on a bench?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stanleyhuangyc/Freematics/issues/135#issuecomment-825516815, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHEOZ7DCIRANXCNAUYPDRGLTKE2BHANCNFSM43JRREMQ .

maxdd commented 3 years ago

Oh now i understand why. Hyundai has manual transmission right? As far as i know TCM (Automatic Trasmission) is OBD relevant. In fact 0x7E0 request and 0x7E8 response is the ECM while 0x7E1 request and 0x7E9 response is the TCM. Most likely the current implementation doesnt support 2 OBD relevant ECUs and the pidmap is most likely filled in with the last payload which is the TCM. When you do your request you are most likely sending them to the ECM. It was bad to remove the headers :D

wally2511 commented 3 years ago

Perfect, that's exactly what I believe the problem is as well.

Yes, the Hyundai had a manual transmission!

On Fri, 23 Apr 2021, 8:38 pm maxdd, @.***> wrote:

Oh now i understand why. Hyundai has manual transmission right? As far as i know TCM (Automatic Trasmission) is OBD relevant. In fact 0x7E0 request and 0x7E8 response is the ECM while 0x7E1 request and 0x7E9 response is the TCM. Most likely the current implementation doesnt support 2 OBD relevant ECUs and the pidmap is most likely filled in with the last payload.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stanleyhuangyc/Freematics/issues/135#issuecomment-825568727, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHEOZ7BXUSFCQZQCC475KFTTKFE3RANCNFSM43JRREMQ .

maxdd commented 3 years ago

Unfortunately the STM32 (referred as "link") has no public code and most likely it is forwarding your request to the ECM only (hardcoded). Do you have a way to specify 7E0 and 7E1? The only thing you might be able to do is to change a little bit the init function to see if you can catch only the first row of the payloads and use that as input. Still it sucks that we can't dialogue freely with other OBD related ECU (i might be wrong) Maybe you can re-implement your obd class based on

https://github.com/stanleyhuangyc/Freematics/blob/a42b6ab98a296ae81ab4c25a8a5bf4a4b3d3a413/firmware_v5/can_sniffer/can_sniffer.ino#L38

The repo developer doesn't seem so proactive/responsive overall

wally2511 commented 3 years ago

@maxdd The filter is a good idea!!! I might have a fiddle and see if can get it to work.

I am disappointed at the responsiveness of the developer, I've tried contacting through different channels and have heard nothing. If I have no joy getting the device working soon I'll be looking for a refund.

maxdd commented 3 years ago

It's a shame he spent so much effort and now just dropped. I had the same feeling given he never replied to me. Maybe one day he will be back.