Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mode 0x03: Request trouble codes. #9

Closed
produmann opened this issue Jan 21, 2018 · 38 comments · Fixed by #10
Closed

Mode 0x03: Request trouble codes. #9

produmann opened this issue Jan 21, 2018 · 38 comments · Fixed by #10

Comments

@produmann
Copy link

produmann commented Jan 21, 2018

hello, your code does not have a bug-viewing mode, that's how I implemented it
cutting from my program code:

void error03(void) {     //    Error codes view основные
 flag5=1;
   kod1="0";
   kod2="0";
   kod3="0";
         
 if (init_success){
 res = obd.getPID(0, 0x03, 6);
    for (int i=0; i<6; i++){
                rxData[i]=String(obd.readUint8(i), HEX);
               }
  if (rxData[0].toInt()<10) {   //// КОД 1
       kod1="0"+rxData[0];
      } else {kod1=rxData[0];}
       if (rxData[1].toInt()<10) {
       kod1=kod1+"0"+rxData[1];
      } else {kod1=kod1+rxData[1];}

      if (rxData[2].toInt()<10) {   //// КОД 2
       kod2="0"+rxData[2];
      } else {kod2=rxData[2];}
       if (rxData[3].toInt()<10) {
       kod2=kod2+"0"+rxData[3];
      } else {kod2=kod2+rxData[3];}

      if (rxData[4].toInt()<10) {   //// КОД 3
       kod3="0"+rxData[4];
      } else {kod3=rxData[4];}
       if (rxData[5].toInt()<10) {
       kod3=kod3+"0"+rxData[5];
      } else {kod3=kod3+rxData[5];}
               
    }
}

I had to tweak the library OBD9141.cpp a bit for this:

uint8_t OBD9141::readUint8(uint8_t index){
    return this->buffer[4 + index];

buffer[4 + index] instead buffer[5 + index]

Only I can not determine the checksum for the response
when there are no engine errors, the checksum = 5
when there are errors, the checksum from 1 to 15 is not suitable, maybe you need another method?
Can you suggest your method of displaying errors?
My method works well, but without a checksum ...
here are my debug logs:

Result 0x0C (RPM): 6
Trying to get x ret_len bytes: 8
72 107 1 65 12 11 192 204 
Result: 752
Result 0x01: 6
Trying to get x ret_len bytes: 10
72 107 1 65 1 129 7 97 64 31 
Result 0x03 : 6
Trying to get x ret_len bytes: 26
72 107 1 67 1 19 0 0 0 0 11 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 


hex

Result 0x0C (RPM): 6
Trying to get x ret_len bytes: 8

48 6B 1 41 C B 94 A0 
Result: 741
Result 0x01: 6
Trying to get x ret_len bytes: 10

48 6B 1 41 1 81 7 61 40 1F 
Result 0x03 : 6
Trying to get x ret_len bytes: 10

48 6B 1 43 1 13 0 0 0 0  //response contains one error p0113
1 13 0 0 0 0 





Result 0x0C (RPM): 6
Trying to get x ret_len bytes: 8

48 6B 1 41 C B D4 E0 
Result: 757
Result 0x01: 6
Trying to get x ret_len bytes: 10

48 6B 1 41 1 81 7 61 40 1F 
Result 0x03 : 6
Trying to get x ret_len bytes: 10

48 6B 1 43 1 13 0 0 0 0 
1 13 0 0 0 0 
Result del: 6
Trying to get x ret_len bytes: 10

C7 0 0 0 0 0 0 0 0 0 // response to 0x04 mode when the engine is running



Result 0x0C (RPM): 6
Trying to get x ret_len bytes: 8

48 6B 1 41 C 0 0 1 
Result: 0
Result 0x01: 6
Trying to get x ret_len bytes: 10

48 6B 1 41 1 81 7 61 40 1F 
Result 0x03 : 6
Trying to get x ret_len bytes: 10

48 6B 1 43 1 13 0 0 0 0 
1 13 0 0 0 0 Result del: 6
Trying to get x ret_len bytes: 10

C7 48 6B 1 44 F8 0 0 0 0  //response to the 0x04 mode with the engine off, when the fault reset is complete



Result 0x0C (RPM): 6
Trying to get x ret_len bytes: 8

48 6B 1 41 C 0 0 1 
Result: 0
Result 0x01: 6
Trying to get x ret_len bytes: 10

48 6B 1 41 1 0 7 61 61 BF 
Result 0x03 : 6
Trying to get x ret_len bytes: 10

48 6B 1 43 0 0 0 0 0 0  //response mode 0x03 when there are no errors
0 0 0 0 0 0 

@produmann
Copy link
Author

I would like to see in your library modes 0x03, 0x07, 0x04

@produmann
Copy link
Author

you can probably use the 0x01 mode, 0x01 PID, which tells us how many errors we'll have when we query in 0x03 mode

@iwanders
Copy link
Owner

Hi @produmann , thanks for the extensive post.

I'll try to take a look at your code for the diagnostic codes (mode 03) over the weekend. It could be that the offset you were required to make was to account for the list encapsulation described on wikipedia, but I'm a bit skeptical that that is also used for 9141-2.

In regards to mode 0x04, the MIL reset, could you read #8 and check if that branch / method works for you? I never heard back from the original reporter and would really like to know if that works, because then mode 0x04 is already complete.

@iwanders
Copy link
Owner

iwanders commented Jan 27, 2018

Hey,

Ok, took a look at your debug logs, very helpful! Thanks so much for posting the raw data it makes it so much easier to help out 👍

Regarding the following:

I had to tweak the library OBD9141.cpp a bit for this:

uint8_t OBD9141::readUint8(uint8_t index){
   return this->buffer[4 + index];

buffer[4 + index] instead buffer[5 + index]

You are indeed correct! The readUint8 method assumes we are reading a value that was returned by mode 01! At the end of the pid method there is a check that checks the value at index 4 and verifies if this is the PID that was originally requested. the readUint8 and other methods assume you are trying to decode a value returned by the PID mode 1 requests. For the diagnostic trouble codes we will have to read from the internal buffer, bypassing the readUint8 method... Maybe we should have a uint8_t raw(uint8_t index) method to retrieve raw values from the buffer? What do you think?

Regarding the checksums... I think you're on the right track. We should use mode 0x01 PID 0x01 to obtain how many errors there are, then we can get the appropriate number of bytes from mode 0x01 and the checksum should work... I say should, because lets look at the following snippet from your logs:

Result 0x03 : 6
Trying to get x ret_len bytes: 26
72 107 1 67 1 19 0 0 0 0 11 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0

If we run the checksum method on the first 10 bytes of that, we actually obtain the 0x11 which is at the 11th byte. You should be able to test this with:

uint8_t result[] = {72, 107, 1, 67, 1, 19, 0, 0, 0, 0, 11, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
Serial.print("Checksum: ");
Serial.println(OBD9141::checksum(result, 10)); // this will print 11..

One thing I'm not sure of is how many bytes are actually returned, it appears the response header is 4 bytes again, then follow the 1, 19, 0,0, 0,0 bytes, every two bytes can be a trouble code. This corresponds to the result from your mode 0x01 pid 0x01 request where I decoded (manually) that there were three error codes. But I don't really understand the n*6 that's written on the wiki page for mode 03. So I'm not really sure how we can calculate the length of mode 0x03 requests.

Can you suggest your method of displaying errors?

Certainly, I don't have access to an Arduino or OBD9141 hardware at the moment, so can't really test anything. But I would do something along the lines of (based on explanation on wikipedia):

// This code is incorrect, scroll down further!
/**
 * Decodes the two bytes at input_bytes into the diagnostic troublecode, written
 * in printable format to output_string.
 */
void decodeDTC(void* input_bytes, char* output_string) {
  const uint8_t A = reinterpret_cast<uint8_t*>(input_bytes)[0];
  const uint8_t B = reinterpret_cast<uint8_t*>(input_bytes)[1];
  const static char type_lookup[4] = {'P', 'C', 'B', 'U'};
  const static char digit_lookup[16] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};

  output_string[0] = type_lookup[A >> 6];
  output_string[1] = digit_lookup[(A >> 4) & 0b11];
  output_string[2] = digit_lookup[B >> 4];
  output_string[3] = digit_lookup[B & 0b1111];
}

Use like:

  uint8_t errorcodes[] = {0x48, 0x6B, 0x1, 0x43, 0x1, 0x13, 0x0, 0x0, 0x0, 0x0};
  char buffer[5] = {0}; // just a buffer to hold the formatted string, zero terminated for testing on pc
  decodeDTC(&(errorcodes[4]), buffer); // interpretets the 0x1, 0x13 bytes as an error code
  // buffer now holds a null terminated string that can be printed...
  // std::cout << " dtc: " << buffer << std::endl;   // dtc: P013
  Serial.print("dtc: ");
  Serial.write(buffer, 4); // write the first four bytes from buffer to the serial port

If you place this method inside the class it can obviously directly access the internal buffer and the input_bytes argument can be replaced by the DTC index or something. So I decoded P013 from your data as trouble code.

Moving forward, if we really want support for mode 0x03, we need a way to figure out how many trouble codes there are, using the 0x01 mode and 0x01 pid. Then we can request the appropriate number of bytes from mode 0x03, after which we can iterate over the troublecodes that are now stored in the buffer and print those using something like the function I proposed. That's the next steps if you want to work towards a complete solution for this.

@produmann
Copy link
Author

produmann commented Jan 28, 2018

Ok, I understand, I will check your code in my home, but according to your method, the error code is not defined correctly, it should not be P013, it should be P0113 - Intake Air Temperature Circuit High Input

@produmann
Copy link
Author

I read Wikipedia, but I still did not understand the principle of determining the first letter in the error code, namely 'P', 'C', 'B', 'U'

@produmann
Copy link
Author

about the checksum, I selected it in the range 1-15 and neither gave the correct result, maybe it is more, although it's vryatli, it can not be checked at that time, but simply erase all the data from the serial buffer to make room for the request PID, because when too small ret_len is specified, the old data interferes with the next request

@produmann
Copy link
Author

maybe I do not understand something :)

@iwanders
Copy link
Owner

I will check your code in my home, but according to your method, the error code is not defined correctly, it should not be P013, it should be P0113 - Intake Air Temperature Circuit High Input

Hah, you are completely right, I didn't read the page carefully enough and forgot the third character from the first byte. Updated code below.

but I still did not understand the principle of determining the first letter in the error code, namely 'P', 'C', 'B', 'U'

This is given by the two most significant bits of the first byte. If these two bytes are 0b00 it is a Powertrain error, if 0b01 it is a Chassis error, if 0b10 Body and 0b11 is a U which stands for Network (not N :(). Since there are four options I used a lookup table to retrieve the appriopriate letter based on the index. We can obtain the top two bits of the byte by bitshifting to the right 6 times. Then we use that value, which is now either 0, 1, 2 or 3 as an index for the type_lookup table.

about the checksum, I selected it in the range 1-15 and neither gave the correct result, maybe it is more, although it's vryatli, it can not be checked at that time, but simply erase all the data from the serial buffer to make room for the request PID, because when too small ret_len is specified, the old data interferes with the next request

Hmm, interesting. The request does indeed only read up to the ret_len number of bytes. This is because if we know the number of bytes that's expected we can use the readBytes call on the serial port which only blocks until that number is received. If we were to retrieve mode 0x01 and pid 0x01, we could do A & 0x7F to obtain the number of error codes... Retrieving the appropriate number from the mode 0x03 should result in a valid checksum. Unless the mention of ISO 15765-2 on wikipedia is applied in your case, but from the data that doesn't seem to be the case.

Fixed code for parsing the error code:

/**
 * Decodes the two bytes at input_bytes into the diagnostic troublecode, written
 * in printable format to output_string.
 */
void decodeDTC(void* input_bytes, char* output_string) {
  const uint8_t A = reinterpret_cast<uint8_t*>(input_bytes)[0];
  const uint8_t B = reinterpret_cast<uint8_t*>(input_bytes)[1];
  const static char type_lookup[4] = {'P', 'C', 'B', 'U'};
  const static char digit_lookup[16] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};

  // A7-A6 is first dtc character, error type:
  output_string[0] = type_lookup[A >> 6];
  // A5-A4 is second dtc character
  output_string[1] = digit_lookup[(A >> 4) & 0b11];
  // A3-A0 is third dtc character.
  output_string[2] = digit_lookup[A & 0b1111];
  // B7-B4 is fourth dtc character
  output_string[3] = digit_lookup[B >> 4];
  // B3-B0 is fifth dtc character
  output_string[4] = digit_lookup[B & 0b1111];
}
// use like:
  uint8_t errorcodes[] = {0x48, 0x6B, 0x1, 0x43, 0x1, 0x13, 0x0, 0x0, 0x0, 0x0};
  char buffer[6] = {0}; // just a buffer to hold the formatted string, zero terminated for testing on pc
  decodeDTC(&(errorcodes[4]), buffer); // interprets the 0x1, 0x13 bytes as an error code
  // buffer now holds a null terminated string that can be printed...
  // std::cout << " dtc: " << buffer << std::endl;   // dtc: P0113
  Serial.print("dtc: ");
  Serial.write(buffer, 4); // write the first four bytes from buffer to the serial port

@iwanders
Copy link
Owner

Oh, and regarding clearing the troublecodes. As part of the #8 issue I mentioned the method to clear the trouble codes is available obd.clearTroubleCodes(), it would be awesome if you could check if that works as it should.

@produmann
Copy link
Author

thanks, I will definitely check this code and clear errors, as there will be free time, I'm a very busy person :) You are the first to learn about this.

Unless the mention of ISO 15765-2

I'm using iso9141-2

If we were to retrieve mode 0x01 and pid 0x01, we could do A & 0x7F to obtain the number of error codes... Retrieving the appropriate number from the mode 0x03 should result in a valid checksum.

this is suitable only for 0x03 mode, for 0x07 mode this possible method will not work, it's easier to write a function to erase all data from the serial buffer for correct work with subsequent requests, how to implement it correctly?

@produmann
Copy link
Author

Serial.write(buffer, 4); // write the first four bytes from buffer to the serial port

correction, Serial.write(buffer, 5); that's right

@produmann
Copy link
Author

produmann commented Jan 28, 2018

  uint8_t errorcodes[] = {0x48, 0x6B, 0x1, 0x43, 0x1, 0x13, 0x1, 0x14, 0x11, 0x15}; // temporary data for debugging
  char buffer[6] = {0}; // just a buffer to hold the formatted string, zero terminated for testing on pc

  decodeDTC(&(errorcodes[4]), buffer); // interprets the 0x1, 0x13 bytes as an error code
  // buffer now holds a null terminated string that can be printed...
  // std::cout << " dtc: " << buffer << std::endl;   // dtc: P0113
  Serial.print("dtc: ");
  Serial.write(buffer, 5); // write the first four bytes from buffer to the serial port
  Serial.println(" ");
  decodeDTC(&(errorcodes[6]), buffer); // interprets the 0x1, 0x14 bytes as an error code
  // buffer now holds a null terminated string that can be printed...
  // std::cout << " dtc: " << buffer << std::endl;   // dtc: P0114
  Serial.print("dtc: ");
  Serial.write(buffer, 5); // write the first four bytes from buffer to the serial port
  Serial.println(" ");
  decodeDTC(&(errorcodes[8]), buffer); // interprets the 0x11, 0x15 bytes as an error code
  // buffer now holds a null terminated string that can be printed...
  // std::cout << " dtc: " << buffer << std::endl;   // dtc: P1115
  Serial.print("dtc: ");
  Serial.write(buffer, 5); // write the first four bytes from buffer to the serial port
  Serial.println(" ");

void decodeDTC(void* input_bytes, char* output_string) {
  const uint8_t A = reinterpret_cast<uint8_t*>(input_bytes)[0];
  const uint8_t B = reinterpret_cast<uint8_t*>(input_bytes)[1];
  const static char type_lookup[4] = {'P', 'C', 'B', 'U'};
  const static char digit_lookup[16] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};

  // A7-A6 is first dtc character, error type:
  output_string[0] = type_lookup[A >> 6];
  // A5-A4 is second dtc character
  output_string[1] = digit_lookup[(A >> 4) & 0b11];
  // A3-A0 is third dtc character.
  output_string[2] = digit_lookup[A & 0b1111];
  // B7-B4 is fourth dtc character
  output_string[3] = digit_lookup[B >> 4];
  // B3-B0 is fifth dtc character
  output_string[4] = digit_lookup[B & 0b1111];
}

excellent, this method correctly identifies 3 error codes
Please tell me how to correctly export the response from the serial port to "char buffer"?

@iwanders
Copy link
Owner

this is suitable only for 0x03 mode, for 0x07 mode this possible method will not work, it's easier to write a function to erase all data from the serial buffer for correct work with subsequent requests,

Hmm, you are right. I didn't know 0x07, there appears to be no way to figure out how many return bytes will be replied to that response. We actually set all the bytes from the internal buffer to zero before we do a request. So the previous results should never interfere with the newly received bytes.

I guess we finally need a method that can deal with a response of a variable number of bytes, as I proposed two years ago under issue #1... Easiest is probably to use loop on readBytes(, 1) until it takes longer than the intersymbol duration and times out, at which point we know that the transmission is complete.

@produmann
Copy link
Author

produmann commented Jan 29, 2018

at the moment I'm doing just like this: res = obd.getPID (0, 0x03, 6);
put the maximum possible response length = 6 and everything works, but of course the checksum does not match, you need to make readBytes until you see the last byte in the response, you offered it

@produmann
Copy link
Author

tell me please on an example how to return to me the buffer of the answer in an external variable "buffer" that I could process the answer here:
decodeDTC(&(errorcodes[4]), buffer);
can be used?:
bool request ( void * request, uint8_t request_len, uint8_t ret_len);
but how to do it right by example? I will be very grateful

@iwanders
Copy link
Owner

iwanders commented Feb 2, 2018

Okay... I wrote some code without being able to test any of it. So I need your help in testing it!

In 3833cd2 I've implemented the following:

  • Method that supports variable length responses
  • Method that allows raw access to the internal buffer.
  • Method that can retrieve the trouble codes
  • Method that obtain the address of the trouble code in the buffer.
  • Method that can interpret the two-byte trouble code and convert it to human readable string.
  • Example that hopefully prints the trouble codes.

Please take a look at the mentioned commit and checkout branch dtc_support, and test the example from there. You may want to drop some extra prints in there to see what works and what doesn't.

Please get back to me with your findings and be thorough in logging, I really want to get these changes tested and merged into the master branch, but I can't do it without your help in testing them.

@produmann
Copy link
Author

produmann commented Feb 2, 2018

ok, I'll check everything and write you the results and, if possible, with the logs

@produmann
Copy link
Author

hello, it's time to check this code, all in order:

  1. res = obd.readTroubleCodes (); does not work at all, there is no response from the ECU
  2. for the query, I used my old method:
    res = obd.getPID (0, 0x03, 6); it works and comes a response from the ECU, along with this I manually put the index on 1 and got here
    OBD9141 :: decodeDTC (obd.getTroubleCode (index), dtc_buf);
    correct error code: P0113

Next is my modified work code:

 while(1){
            res = obd.getPID(0, 0x03, 6);
            res=1;
            Serial.print("res ");
             Serial.println(res);
            
                Serial.print("Read ");
                Serial.print(res);
                Serial.print(" codes:");
                for (uint8_t index=0; index < res; index++)
                {
                  // convert the DTC bytes from the buffer into readable string
                  OBD9141::decodeDTC(obd.getTroubleCode(index), dtc_buf);
                  Serial.write(dtc_buf, 5);
                  Serial.println();
                }
            
            Serial.println();

            delay(200);
        }

here is the log of my code:

W: E: Looping
Before magic 5 baud.
Before setting port.
After setting port.
First read is: 85
read v1: 8
read v2: 8
v1: 8
v2: 8
w: 247
init_success:1
w: 104
w: 106
w: 241
w: 3
w: 0
w: 198
Trying to get x bytes: 12
72 107 1 67 1 19 0 0 0 0 11 0 
res 1
Read 1 codes:A 1
B 19
P0113

the log of your code
res = obd.readTroubleCodes ();
is completely empty

@iwanders
Copy link
Owner

Hey, are you sure you ran it with the debug flag enabled when you tested the new readTroubleCodes() method?

Because I would really expect atleast the request to be printed. I don't really see how it could be completely empty. If you have the chance, could you check if you tested with the debug flag?

I'm likely to build the hardware that allows me to test it myself, I ordered some parts already, but it'll probably be a few more months before I get around to it.

@produmann
Copy link
Author

Hi, I used your code
https://github.com/iwanders/OBD9141/blob/dtc_support/examples/readDTC/readDTC.ino
there was set parameter
#define OBD9141_DEBUG 1
I do not know why the log is empty .....

@iwanders
Copy link
Owner

Ah yes... That may not work with the official Arduino IDE; if the libraries are build separately from the sketches that doesn't work.
To rule that out, can you try to uncomment the flag here and try again? You should atleast get some output then.

@produmann
Copy link
Author

Hi, OBD9141_DEBUG does not work, I had to edit the library OBD9141.cpp, for example:

//#ifdef OBD9141_DEBUG
    for (uint8_t i=0; i < (answer_length); i++){
        Serial.print(buffer[i]);Serial.print(" ");
    };Serial.println();
//#endif

error code:

Arduino: 1.8.5 (Windows 8.1), Плата:"Arduino/Genuino Mega or Mega 2560, ATmega2560 (Mega 2560)"

In file included from C:\Users\pro\Documents\Arduino\libraries\OBD9141-dtc_support\src\OBD9141.cpp:5:0:

C:\Users\pro\Documents\Arduino\libraries\OBD9141-dtc_support\src\OBD9141.cpp: In member function 'uint8_t OBD9141::request(void*, uint8_t)':

C:\Users\pro\Documents\Arduino\libraries\OBD9141-dtc_support\src\OBD9141.cpp:153:22: error: 'his' was not declared in this scope

         OBD9141print(his->buffer[i]);OBD9141print(" ");

                      ^

C:\Users\pro\Documents\Arduino\libraries\OBD9141-dtc_support\src\OBD9141.h:81:42: note: in definition of macro 'OBD9141print'

     #define OBD9141print(a) Serial.print(a);

                                          ^

exit status 1
Compilation error for Arduino / Genuino Mega or Mega 2560 card.

This report will have more information from
enabled option File -> Settings ->
"Show detailed output at compile time"

here is the log of your code, the query does not work:

Looping
Before magic 5 baud.
Before setting port.
After setting port.
First read is: 85
read v1: 8
read v2: 8
v1: 8
v2: 8
w: 247
read 0xCC?: CC
init_success:1
W: 104 106 241 3 198 
E: 104 106 241 3 228 
A: 

nothing happens next......

Here's a working log, with a query via obd.getPID (0, 0x03, 6); :

Looping
Before magic 5 baud.
Before setting port.
After setting port.
First read is: 85
read v1: 8
read v2: 8
v1: 8
v2: 8
w: 247
read 0xCC?: CC
init_success:1
w: 104
w: 106
w: 241
w: 3
w: 0
w: 198
Trying to get x bytes: 12
72 107 1 67 1 19 0 0 0 0 11 0 
res 1
Read 1 codes:P0113

@iwanders
Copy link
Owner

Hey,

Thanks for testing again! I've fixed the typo and set OBD9141_DEBUG in the header file to true now, it should work.

W: 104 106 241 3 198
E: 104 106 241 3 228

That last byte should be the same, the last byte is the checksum byte. This shows that the value sent on the bus and then read as echo are different... This is a great find! I had a mistake and forgot to sent the checksum byte. I've fixed this!

A: 

nothing happens next......

Interesting... I'm surprised it pauses after that, I've added printing the length of the response from the ECU. To provide some more information, even with our malformed request it shouldn't block on that line, there could've been an out of bounds read on the array, I've fixed that as well.

Please see the changes. We definitely fixed a bug in this iteration, thanks so much for testing. If you have time, can you please test again with latest version from the dtc_support branch?

@produmann
Copy link
Author

of course, the results will tell

@produmann
Copy link
Author

hi, the query is working, but res is not assigned a value, always res = 0
here is the log:

Looping
Before magic 5 baud.
Before setting port.
After setting port.
First read is: 85
read v1: 8
read v2: 8
v1: 8
v2: 8
init_success:1
W: 104 106 241 3 198 
E: 104 106 241 3 198 
A (11): 72 107 1 67 1 19 0 0 0 0 11 

C: 0
obd.readTroubleCodes(); res =0 

I assign the value res = 1 manually and everything works:

Looping
Before magic 5 baud.
Before setting port.
After setting port.
First read is: 85
read v1: 8
read v2: 8
v1: 8
v2: 8
init_success:1
W: 104 106 241 3 198 
E: 104 106 241 3 198 
A (11): 72 107 1 67 1 19 0 0 0 0 11 

C: 0
res 1
Read 1 codes:P0113

@iwanders
Copy link
Owner

Hey,

I'm super grateful that you are taking the time to test this and help bring this feature to the library! 👍 We're almost there!

I've fixed the bug that caused the checksum to be calculated incorrectly in 1283b9c, I made an off-by-one error on reading the checksum byte. I've fixed it, please try the latest version on the branch.

If it works, and you have any way to get the ECU to return more than one trouble code, could you try that as well? Then we know if the number of trouble codes is correctly calculated by the display code.

@produmann
Copy link
Author

hello, I checked it works, but res does not determine it correctly:

Looping
Before magic 5 baud.
Before setting port.
After setting port.
First read is: 85
read v1: 8
read v2: 8
v1: 8
v2: 8
init_success:1
W: 104 106 241 3 198 
E: 104 106 241 3 198 
A (11): 72 107 1 67 1 24 1 19 0 0 36

C: 1
Read 3 codes:
P0118
P0113
P0000

please indicate me in the participating in the development

@iwanders
Copy link
Owner

but res does not determine it correctly:

Hmm... the return of the function does really need to work, otherwise others can't use this new functionality. In the log you show, did you set res to 3 manually to ensure it prints? I looked through the code, but don't see where it can fail yet, I had one floating point division that should've been a integer division, I fixed that and added a bunch of new print statements that should help debug this, could you try the latest version? (49b1427)

please indicate me in the participating in the development

Yes, definitely! I'll write a section in the main readme on the trouble codes, how they work and are supposed to be used. I'll add a word of thanks in that paragraph.

Before we do that though, I want the example to work without modification.

@produmann
Copy link
Author

In the log you show, did you set res to 3 manually to ensure it prints?

no, I did not touch anything, ran the code as it is, res itself assigned a value of 3

@produmann
Copy link
Author

all the same res = 3:

Looping
Before magic 5 baud.
Before setting port.
After setting port.
First read is: 85
read v1: 8
read v2: 8
v1: 8
v2: 8
init_success:1
W: 104 106 241 3 198 
E: 104 106 241 3 198 
A (11): 72 107 1 67 1 19 1 24 0 0 36 
C: 1
S: 1
R: 10
T: 3
Read 3 codes:P0113
P0118
P0000

I would write such an algorithm of actions: read the response buffer to the first zero byte and calculate how many bytes were read

Or knowing that 72 107 1 67 1 19 1 24 0 0 = only 10 bytes response; 5,6,7,8,9,10 bytes carry information about the code.
We have that:
5 and 6 bytes are 1 and 19
7 and 8 bytes are 1 and 24
9 and 10 bytes have no values and are zero
Thus, we can cut off the null bytes 9 and 10 and only have 5,6 and 7,8 bytes, which makes us understand that we only have 2 diagnostic error codes.
You can correct me if you have a different opinion.

@produmann
Copy link
Author

You can also manually insert the response buffer 72 107 1 67 1 19 1 24 0 0 36 in the program code to simulate a response from the ECU for tests without connecting to the car

@iwanders
Copy link
Owner

iwanders commented Mar 1, 2018

Hey,

no, I did not touch anything, ran the code as it is, resitself assigned a value of 3

Oh that is great! That means everything is working now! 🎉

I would write such an algorithm of actions: read the response buffer to the first zero byte and calculate how many bytes were read [...] You can correct me if you have a different opinion.

Yeah, I get what you mean. However, the ECU has returned a trouble code that is P0000, I want to show that to the user. I don't know what other cars do, maybe the P0000 code is the 'magic' value for no further trouble codes. To accomodate this, I've changed the internal data type for the trouble codes in 40a3d77 and updated the example in cd3b012. This allows the user to easily decide whether or not to show the P0000 code or to discard it.

I've also updated the readme to reflect your help in this and added a section on the trouble codes. Besides this I've updated the comments here and there, please see the latest changes.

Can you give it a final test and revise the changes? Then I'll merge this into master!

@iwanders iwanders changed the title mode 0x03 Mode 0x03: Request trouble codes. Mar 1, 2018
@produmann
Copy link
Author

okay, I'll check it out

@produmann
Copy link
Author

produmann commented Mar 1, 2018

Everything works great

Looping
init_success:1
Read 3 codes:
P0113
P0118
 - 

The only thing I would correct that the symbol "-" instead of P0000 is better not to print at all, the user does not need to know this, he will be interested in specific trouble codes and Read 3 codes - here to write the correct number of trouble codes, in this case 2pcs

@produmann
Copy link
Author

produmann commented Mar 1, 2018

// res will hold the number of trouble codes that were received.
           // if no diagnostic trouble codes were retrieved it will be zero.

this is not a valid definition of res, since res takes a value of 3 even if the number of errors is zero, the ECU will simply return these bytes 72 107 1 67 0 0 0 0 0 0 and the checksum will res = 3 :

Looping
init_success:1
Read 3 codes:
 - 
 - 
 - 

@iwanders
Copy link
Owner

iwanders commented Mar 2, 2018

I see your point, but disagree. Thing is, if the ECU returns:

72 107 1 67 0 0 0 0 0 0 247

The ECU really returned 6 0x00 bytes, this means the ECU returned three trouble codes, three times P0000. The ECU returned 3 trouble codes and the library's response should represent that.

This is the reason I switched the internal data representation to uint16_t, this allows the user of the library to easily filter out the P0000 codes.

Additionally, by keeping this P0000 code around, the user knows that we have read all the trouble codes the ECU had available. Without this indication the user wouldn't know whether he has retrieved all the trouble codes correctly. Actually, thinking about this I think this is very important to clearly indicate. I've updated the example to be more clear on this (2f8b816), the user of the library can always easily modify the example to stop this print from happening.

@produmann
Copy link
Author

Hi, I added the 0x07 method. Show pending Diagnostic Trouble Codes
OBD9141::readPendingTroubleCodes()
src.zip

can be added to the main library and I think issues will be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants