fenugrec / freediag

OBD2 scantool
GNU General Public License v3.0
338 stars 75 forks source link

Fix readiness monitor display and add summary count #89

Closed aaeegg closed 1 year ago

aaeegg commented 1 year ago

This fixes a few bugs in the "test readiness" command and adds a summary count to its output.

The "test readiness" command was displaying the readiness monitor status backwards: complete monitors were displayed as not complete, and vice versa. This was due to cmd_test_readiness interpreting a set bit in the Mode 1 PID $01 response to mean that the monitor is complete, but actually, a set bit means that the monitor is incomplete. (I mentioned this on IRC a few years ago, and I think fenugrec and I each assumed the other was going to fix it, so here we are...)

My freshly reset bench ECU responds to Mode 1 PID $01 with 41 01 00 07 ED ED. That decodes as follows, per SAE J1979 APR2002 Tables 17 and B2:

41 - SID - Request current powertrain diagnostic data response 01 - PID - Monitor status since DTCs cleared 00 - DATA_A - MIL off, 0 DTCs stored 07 - DATA_B - MIS, FUEL and CCM complete; MIS, FUEL and CCM supported ED - DATA_C - EGR, HTR, O2S, AIR, EVAP, CAT supported; ACRF, HCAT not supported ED - DATA_D - EGR, HTR, O2S, AIR, EVAP, CAT not complete; ACRF, HCAT N/A

freediag was reporting the readiness status wrong:

ECU 0: Misfire Monitoring: NOT Complete ECU 1: Fuel System Monitoring: NOT Complete ECU 2: Comprehensive Component Monitoring: NOT Complete ECU 4: Catalyst Monitoring: Complete ECU 5: Heated Catalyst Monitoring: Not Supported ECU 6: Evaporative System Monitoring: Complete ECU 7: Secondary Air System Monitoring: Complete ECU 8: A/C System Refrigerant Monitoring: Not Supported ECU 9: Oxygen Sensor Monitoring: Complete ECU 10: Oxygen Sensor Heater Monitor: Complete ECU 11: EGR System Monitoring: Complete

With the change, it gets it right:

ECU 0: Misfire Monitoring: Complete ECU 1: Fuel System Monitoring: Complete ECU 2: Comprehensive Component Monitoring: Complete ECU 4: Catalyst Monitoring: NOT Complete ECU 5: Heated Catalyst Monitoring: Not Supported ECU 6: Evaporative System Monitoring: NOT Complete ECU 7: Secondary Air System Monitoring: NOT Complete ECU 8: A/C System Refrigerant Monitoring: Not Supported ECU 9: Oxygen Sensor Monitoring: NOT Complete ECU 10: Oxygen Sensor Heater Monitor: NOT Complete ECU 11: EGR System Monitoring: NOT Complete

But note the weird ECU 0, 1, 2, 4 prefix. What's going on there is that, at least in theory, multiple ECUs can respond to the same Mode 1 PID $01 request, and freediag tries to iterate over all the replies it got and print all of them. But the inner loop, which iterates over the bits for each monitor, uses the same index variable as the outer loop, which iterates over the responses! So it prints the monitor index as the ECU number, and also, the outer loop quits after the first iteration so if there actually were multiple responses, it only prints the first. Adding a second index variable fixes this:

ECU 1: Misfire Monitoring: Complete ECU 1: Fuel System Monitoring: Complete ECU 1: Comprehensive Component Monitoring: Complete ECU 1: Catalyst Monitoring: NOT Complete ECU 1: Heated Catalyst Monitoring: Not Supported ECU 1: Evaporative System Monitoring: NOT Complete ECU 1: Secondary Air System Monitoring: NOT Complete ECU 1: A/C System Refrigerant Monitoring: Not Supported ECU 1: Oxygen Sensor Monitoring: NOT Complete ECU 1: Oxygen Sensor Heater Monitor: NOT Complete ECU 1: EGR System Monitoring: NOT Complete

I also added a summary display at the end showing how many continuous and non-continuous monitors were incomplete and the total number of incomplete monitors:

Number of incomplete continuous monitors: None Number of incomplete non-continuous monitors: 6 Total number of incomplete monitors: 6

This allows for rapid assessment of whether a car is ready for emissions testing. The exact number of incomplete monitors allowed varies by jurisdiction and model year.

Note that in do_j1979_getdtcs, the initial "scan" command also looks at the Mode 1 PID $01 response and prints a message if there are incomplete monitors. But the existing code there is already correct.

fenugrec commented 1 year ago

wow good find re the wrong inner/outer index var ! That is a very dusty part of the code...

I'm surprised we don't already have a test for this, was expecting Ctest to start failing after this patch. Would you be able to add coverage for this ?

aaeegg commented 1 year ago

I've added a test.

fenugrec commented 1 year ago

rebased+merged @ 933a3c57483cb0fb9c12022ea9b83cd153b4bb18 . Thanks !