Callbacks for user identify new application layer messages received

[gustavowd]

Hi Daniel.

Don't you think it would be interesting to have user-initiated callbacks to be called when certain packets were received by the application layer? For example, here: void SAE_J1939_Read_Transport_Protocol_Data_Transfer(J1939 j1939, uint8_t SA, uint8_t data[]) { / Save the sequence data / j1939->from_other_ecu_tp_dt.sequence_number = data[0]; j1939->from_other_ecu_tp_dt.from_ecu_address = SA; uint8_t index = data[0] - 1; for (uint8_t i = 1; i < 8; i++) j1939->from_other_ecu_tp_dt.data[index7 + i-1] = data[i]; / For every package, we send 7 bytes of data where the first byte data[0] is the sequence number / / Check if we have completed our message - Return = Not completed / if (j1939->from_other_ecu_tp_cm.number_of_packages != j1939->from_other_ecu_tp_dt.sequence_number || j1939->from_other_ecu_tp_cm.number_of_packages == 0) return; / Our message are complete - Build it and call it complete_data[total_message_size] / uint32_t PGN = j1939->from_other_ecu_tp_cm.PGN_of_the_packeted_message; uint16_t total_message_size = j1939->from_other_ecu_tp_cm.total_message_size; uint8_t complete_data[total_message_size]; uint16_t inserted_bytes = 0; for (uint8_t i = 0; i < j1939->from_other_ecu_tp_dt.sequence_number; i++) for (uint8_t j = 0; j < 7; j++) if (inserted_bytes < total_message_size) complete_data[inserted_bytes++] = j1939->from_other_ecu_tp_dt.data[i7 + j]; / Send an end of message ACK back / if(j1939->from_other_ecu_tp_cm.control_byte == CONTROL_BYTE_TP_CM_RTS) SAE_J1939_Send_Acknowledgement(j1939, SA, CONTROL_BYTE_TP_CM_EndOfMsgACK, GROUP_FUNCTION_VALUE_NORMAL, PGN); / Check what type of function that message want this ECU to do / switch (PGN) { case PGN_COMMANDED_ADDRESS: SAE_J1939_Read_Commanded_Address(j1939, complete_data); / Insert new name and new address to this ECU / break; case PGN_DM1: SAE_J1939_Read_Response_Request_DM1(j1939, SA, complete_data, complete_data[8]); / Sequence number is the last index / break; case PGN_DM2: SAE_J1939_Read_Response_Request_DM2(j1939, SA, complete_data, complete_data[8]); / Sequence number is the last index / break; case PGN_DM16: SAE_J1939_Read_Binary_Data_Transfer_DM16(j1939, SA, complete_data); break; case PGN_SOFTWARE_IDENTIFICATION: SAE_J1939_Read_Response_Request_Software_Identification(j1939, SA, complete_data); break; case PGN_ECU_IDENTIFICATION: SAE_J1939_Read_Response_Request_ECU_Identification(j1939, SA, complete_data); break; case PGN_COMPONENT_IDENTIFICATION: SAE_J1939_Read_Response_Request_Component_Identification(j1939, SA, complete_data); break; / Add more here / } / Delete TP DT and TP CM */ memset(&j1939->from_other_ecu_tp_dt, 0, sizeof(j1939->from_other_ecu_tp_dt)); memset(&j1939->from_other_ecu_tp_cm, 0, sizeof(j1939->from_other_ecu_tp_cm));}

It would be nice to have a callback to inform that new Software identification was received, after calling the SAE_J1939_Read_Response_Request_Software_Identification function.

What do you think about it?

[mr337]

@gustavowd Do you mind to format that snippet? It is difficult to read in the current form.

[gustavowd]

Sorry. I have wrote from my phone.

When a user call a function like this:

/ Request Software Identification from ECU 2 to ECU 1 / SAE_J1939_Send_Request(&j1939_2, 0xA2, PGN_SOFTWARE_IDENTIFICATION);

It's not possible to know when the response was received, since all the remaining messages are sent/received in interrupts by the function "Open_SAE_J1939_Listen_For_Messages()".

So, if one whats to, per example, print the received software identification, it should wait for a specific time and hope to have received the response for such request.

This occurs because functions like SAE_J1939_Read_Transport_Protocol_Data_Transfer(J1939 *j1939, uint8_t SA, uint8_t data[]) are called inside the "Open_SAE_J1939_Listen_For_Messages()" function.

But, if there is a callback for the requested software identification PGN, it is possible to add a printf function (or any user code) in the callback:

void PGN_SOFTWARE_IDENTIFICATION_callback(void){ / Display what ECU 2 got / printf("Number of fields = %i\nIdentifications = %s\nFrom ECU address = 0x%X", j1939_2.from_other_ecu_identifications.software_identification.number_of_fields, j1939_2.from_other_ecu_identifications.software_identification.identifications, j1939_2.from_other_ecu_identifications.software_identification.from_ecu_address); }

The basic ideia is make it possible to an user specify a callback function for some specific PGNs. For example: typedef void (*callback_pointer_t)(void);

void Set_PGN_SOFTWARE_IDENTIFICATION_callback(J1939 *j1939, callback_pointer_t callback){ j1939->PGN_SOFTWARE_IDENTIFICATION_callback = callback; }

Then, in the user code it is defined the user callback function:

void PGN_SOFTWARE_IDENTIFICATION_user(void){ / Display what ECU 2 got / printf("Number of fields = %i\nIdentifications = %s\nFrom ECU address = 0x%X", j1939_2.from_other_ecu_identifications.software_identification.number_of_fields, j1939_2.from_other_ecu_identifications.software_identification.identifications, j1939_2.from_other_ecu_identifications.software_identification.from_ecu_address); }

and then call: Set_PGN_SOFTWARE_IDENTIFICATION_callback(&j1939, PGN_SOFTWARE_IDENTIFICATION_user);

/ Request Software Identification from ECU 2 to ECU 1 / SAE_J1939_Send_Request(&j1939_2, 0xA2, PGN_SOFTWARE_IDENTIFICATION);

In the SAE_J1939_Read_Transport_Protocol_Data_Transfer() function, the callback is executed if it is different from NULL:

void SAE_J1939_Read_Transport_Protocol_Data_Transfer(J1939 j1939, uint8_t SA, uint8_t data[]) { ..... / Check what type of function that message want this ECU to do */ switch (PGN) { .... case PGN_SOFTWARE_IDENTIFICATION: SAE_J1939_Read_Response_Request_Software_Identification(j1939, SA, complete_data); if (j1939->PGN_SOFTWARE_IDENTIFICATION_callback != NULL){ j1939->PGN_SOFTWARE_IDENTIFICATION_callback(); } break;

           ....

}

I intend to use this protocol API with a RTOS. So, with callbacks it is possible to synchronize user tasks to activate by semaphores when specific callbacks are called.

This is the basic ideia. What do you think about it?

Best regards, Gustavo

[mr337]

Here are some of my thoughts. I do want to add that I am following this repo since I am looking to adopt this library as well for a project of mine. I have not implemented it yet so my understanding of the codebase is still new.

I have used another library for J1939 that focuses around a single active loop. At first I didn't care much for it but after a while got used to it. I am also looking at the issue https://github.com/DanielMartensson/Open-SAE-J1939/issues/5 and have a few thoughts for it as well :)

I believe the main idea is you'll need to run a loop to parse messages as they come in. Here is an example https://github.com/DanielMartensson/Open-SAE-J1939/blob/main/Src/Examples/Open%20SAE%20J1939/Startup.txt#L28

When a new message comes in you'll need to figure out how to handle it at that point.

Back to your question of a custom handler, it really depends on what you are trying to accomplish. In the current loop form it should be a simple if or a select statement.

Now if you want the library to do it there will be some extra work to add that functionality, and the other problem is you'll be constrained to how the library implements that. Also there isn't a performance gain or penalty since either in your code or the library will be doing a function callback lookup to figure where to send the message for processing.

I don't have too much RTOS programming experience, but do you mind to expand on this comment?

with callbacks it is possible to synchronize user tasks

Is this possible since you have an external dependency of the CANBUS network. One can't guarantee an ECU will respond in a timely fashion, maybe there is other device chatter on the network, or the ECU is slow to respond etc. Or do you plan to have another task queue messages up and every 0.25s then process all the callbacks in a single context switch?

[DanielMartensson]

Hi Daniel.

Don't you think it would be interesting to have user-initiated callbacks to be called when certain packets were received by the application layer? For example, here: void SAE_J1939_Read_Transport_Protocol_Data_Transfer(J1939 _j1939, uint8_t SA, uint8t data[]) { / Save the sequence data _/ j1939->from_other_ecu_tp_dt.sequence_number = data[0]; j1939->from_other_ecu_tp_dt.from_ecu_address = SA; uint8_t index = data[0] - 1; for (uint8_t i = 1; i < 8; i++) j1939->from_other_ecu_tp_dt.data[index7 + i-1] = data[i]; /* For every package, we send 7 bytes of data where the first byte data[0] is the sequence number / / Check if we have completed our message - Return = Not completed / if (j1939->from_other_ecu_tp_cm.number_of_packages != j1939->from_other_ecu_tp_dt.sequence_number || j1939->from_other_ecu_tp_cm.number_ofpackages == 0) return; / Our message are complete - Build it and call it complete_data[total_messagesize] / uint32_t PGN = j1939->from_other_ecu_tp_cm.PGN_of_the_packeted_message; uint16_t total_message_size = j1939->from_other_ecu_tp_cm.total_message_size; uint8_t complete_data[total_message_size]; uint16_t inserted_bytes = 0; for (uint8_t i = 0; i < j1939->from_other_ecu_tp_dt.sequence_number; i++) for (uint8_t j = 0; j < 7; j++) if (inserted_bytes < total_message_size) complete_data[inserted_bytes++] = j1939->from_other_ecu_tp_dt.data[i7 + j]; /* Send an end of message ACK back / if(j1939->from_other_ecu_tp_cm.control_byte == CONTROL_BYTE_TP_CM_RTS) SAE_J1939_Send_Acknowledgement(j1939, SA, CONTROL_BYTE_TP_CM_EndOfMsgACK, GROUP_FUNCTION_VALUENORMAL, PGN); / Check what type of function that message want this ECU to do _/ switch (PGN) { case PGN_COMMANDED_ADDRESS: SAE_J1939_Read_Commanded_Address(j1939, completedata); / Insert new name and new address to this ECU _/ break; case PGN_DM1: SAE_J1939_Read_Response_Request_DM1(j1939, SA, complete_data, completedata[8]); / Sequence number is the last index _/ break; case PGN_DM2: SAE_J1939_Read_Response_Request_DM2(j1939, SA, complete_data, completedata[8]); / Sequence number is the last index _/ break; case PGN_DM16: SAE_J1939_Read_Binary_Data_Transfer_DM16(j1939, SA, complete_data); break; case PGN_SOFTWARE_IDENTIFICATION: SAE_J1939_Read_Response_Request_Software_Identification(j1939, SA, complete_data); break; case PGN_ECU_IDENTIFICATION: SAE_J1939_Read_Response_Request_ECU_Identification(j1939, SA, complete_data); break; case PGN_COMPONENT_IDENTIFICATION: SAE_J1939_Read_Response_Request_Component_Identification(j1939, SA, completedata); break; / Add more here / } / Delete TP DT and TP CM */ memset(&j1939->from_other_ecu_tp_dt, 0, sizeof(j1939->from_other_ecu_tp_dt)); memset(&j1939->from_other_ecu_tp_cm, 0, sizeof(j1939->from_other_ecu_tp_cm));}

It would be nice to have a callback to inform that new Software identification was received, after calling the SAE_J1939_Read_Response_Request_Software_Identification function.

What do you think about it?

It would be possible. Sure. But does it works with all systems? As I know, all systems does not have interupts.

[gustavowd]

Hi Daniel.

I invited you to a project of mine, in which I develop a possible concurrent/rtos approach to your code. It is protect with mutexes, and synchronized by queues. The code is private because i will test in a company product, but if you like the approach, i can bring it to your project through a pull request.

Best regards, Gustavo

[gustavowd]

Daniel, by the way, i saw you referring to the misra c in another issue. If you refer to misra c you should always use braces in conditional and loops statements. I noticed that you do not used it in the contiditonal statements of function Open_SAE_J1939_Listen_For_Messages() , for example.

[DanielMartensson]

Hi Daniel.

I invited you to a project of mine, in which I develop a possible concurrent/rtos approach to your code. It is protect with mutexes, and synchronized by queues. The code is private because i will test in a company product, but if you like the approach, i can bring it to your project through a pull request.

Best regards, Gustavo

Will it still have 100% portability for all systems? Because mutexes and queues sounds like you are using RTOS.

Open SAE J1939 is meant to be implemented with RTOS just by copy and paste the C-code directly into a project, without modify the project.

I have been using a lot of C-libraries and projects and every C-library/project I have been using, it has been always difficult to "install" the software because the C-code is written in a specific way that only fits a small purpose and it takes account on a specific hardware.

My C code is written in C99 standard and does not have threads. Me recommendation for you is to use the Open_SAE_J1939_Listen_For_Message function inside a thread. And...that's it. =)

[DanielMartensson]

Daniel, by the way, i saw you referring to the misra c in another issue. If you refer to misra c you should always use braces in conditional and loops statements. I noticed that you do not used it in the contiditonal statements of function Open_SAE_J1939_Listen_For_Messages() , for example.

Thanks for notice that. I have been working a lot for code this project as very close to MIRSA C standard. Lot of people have been using this library and they are using the MIRSA C standard as well. They have not told me that I have missed a bracket. Even if a modern C-compilers will handle that if-statements, for-loops etc for one statement without brackets.

But I'm working on it. I have plans to update this repository with a better QT-USB example.