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

PGN: 0x000002 Delete Address #24

Closed galbers closed 1 year ago

galbers commented 1 year ago

Hi! I've been reviewing the code deciphering how you do the decoding of incoming J1939 message ID fields and which functions to call. I am still new to J1939 but I believe some of your interpretation are a bit off.

A PGN is the collection of 18 bits which include one reserved bit, one data-page (DP) bit, the PDU Format byte (PF) and the PDU Specific Byte (PS). Bits 8 through 25 of the 29bit extended CAN ID.

The "id0" field is merely the "priority" of the message. See here for more information about the priority field

You're "id1" field used in Listen_For_Messages.c is actually the PDU Format Field (PF). The PF will tell you how to interpret the rest of the message including the destination address (DA) and the actual 8 byte data payload. This is how you've correctly written the SAE_J1939_Read_Request() function

Now I believe the issue lies here in the usage of id1 == 0x2 as seen below for the delete address function

}else if (id0 == 0x0 && id1 == 0x2 && (DA == j1939->information_this_ECU.this_ECU_address || DA == 0xFF)){
            SAE_J1939_Read_Address_Delete(j1939, data);                                                         /* Not a SAE J1939 standard */
}

The crux of the issue is that PDU Format 0x02 is already assigned to PGN "Electronic Brake System #1/1" EBS11. It is defined in ISO 11992-2 for more PG details. So probably not a great idea to use this for the delete function.

Instead I would recommend using PDU Format (PF) 0xFF which is for PGN Proprietary Bwhich is a broadcast message that you can specify your own behavior based on the data payload (similar to the read request from above).

Anywho, I hope I explained myself well here. Thanks for all the great work with this library!

DanielMartensson commented 1 year ago

Thank you so much! If you want to correct this, please send me a pull request.

The reason why I choose 0x2 as "Delete"-PNG is because I found it free at https://www.isobus.net/isobus/PGNAndSPN

https://github.com/DanielMartensson/Open-SAE-J1939/blob/main/Src/SAE_J1939/SAE_J1939-81_Network_Management_Layer/Address_Delete.c

https://github.com/DanielMartensson/Open-SAE-J1939/blob/main/Src/SAE_J1939/SAE_J1939_Enums/Enum_PGN.h