DanielMartensson / Open-SAE-J1939

SAE J1939 protocol free to use for embedded systems or PC with CAN-bus
MIT License
455 stars 164 forks source link

DM1/DM2 - Active DTC Count and Multiple Active Message Processing #14

Closed tptGit closed 2 years ago

tptGit commented 2 years ago

Forgive my potentially rough code, i'm not a proficient C programmer. But i'm quite good with CANbus.

I believe the number of active DTC count when more than 1 is not accurate, nor is the condition for clearing the number of active messages.

In any FECA DMx message, the first two bytes are the lamp codes, followed by 4 byes for each DTC When receiving FECA normally it assumes there is 1 code, which is fine, but when receiving multiple codes the number of active codes should case PGN_DM1: SAE_J1939_Read_Response_Request_DM1(j1939, SA, complete_data, (total_message_size-2)/4); /* Sequence number is the last index */ break;

When all codes are clear, the DM1 protocol requires the ECU to send an additional FECA message with all zeros for the SPN, FMI and OC. So the best way to identify no active messages would be like this: /* Check if we have no fault cause */ if (errors_dm1_active == 1 && j1939->from_other_ecu_dm.dm1.SPN == 0) j1939->from_other_ecu_dm.errors_dm1_active = 0; else j1939->from_other_ecu_dm.errors_dm1_active = errors_dm1_active;

FMI 31 (FMI_NOT_AVAILABLE) is reserved for a generic but undefined "condition exists" This may be just a confusingly named enumeration. Might be better to be something like FMI_UNDEFINED_CONDITION or something to avoid confusion.

To decode all the DTC messages, the code might look a little like this:

for (uint8_t i = 0; i < errors_dm1_active; i++){ j1939->from_other_ecu_dm.dm1.SPN = ((data[(i*4)+4] & 0b11100000) << 11) | (data[(i*4)+3] << 8) | data[(i*4)+2]; j1939->from_other_ecu_dm.dm1.FMI = data[(i*4)+4] & 0b00011111; j1939->from_other_ecu_dm.dm1.SPN_conversion_method = data[(i*4)+5] >> 7; j1939->from_other_ecu_dm.dm1.occurrence_count = data[(i*4)+5] & 0b01111111; j1939->from_other_ecu_dm.dm1.from_ecu_address = SA; }

Obviously the J1939 structure only has space to assign a value to 1 SPN, but maybe that could be updated to be an array of SPNs? I think the code might also need to handle if the number of active SPN's is less than the previous number, it would need to clear the values from the array that might remain.

Thank you for considering.

DanielMartensson commented 2 years ago

Thank you for asking.

Can you fork this project and then do a pull request of the change you want? It would be very good =)

The reason why it looks like this is because Open SAE J1939 taking account to memory. But if you have found another way around and still keeping the low memory aspects. Then feel free to do a pull request.

tptGit commented 2 years ago

I'll give it a go Daniel.

DanielMartensson commented 2 years ago

I'll give it a go Daniel.

Thank you for contributing to Open SAE J1939. Just ask me if you need some help with the J1939 struct. Don't forget to update the DM1 example and add a DM2 example as well.