DanielMartensson/Open-SAE-J1939

Why valor Open_SAE_J1939_Listen_For_Messages() in multiple places of the code

gustavowd opened this issue · 7 comments

gustavowd commented

Hi Daniel. Thank you very much for your open source code for the j1939 protocol. It will be very useful for many people. However, I have a question regarding your API. Why do you call the Open_SAE_J1939_Listen_For_Messages() function in the loop and multiple times in the example requests if the function is in the stm32 interrupt callback? Will the function not be called whenever a frame comes through the CAN network? I'm starting to develop j1939 devices with your API and I would like to understand why call the function multiple times out of the interrupt. I intend to use an rtos and synchronize the requests by queues and semaphores.

Thanks in advance.
Gustavo

gustavowd commented

Hi Daniel. Let's see if i understood your code and J1939 protocol.

For instance, in this example (Software Identification):
/* Create our J1939 structure with two ECU */
J1939 j1939_1 = {0};
J1939 j1939_2 = {0};

/* Important to sent all non-address to 0xFF - Else we cannot use ECU address 0x0 */
for(uint8_t i = 0; i < 255; i++){
	j1939_1.other_ECU_address[i] = 0xFF;
	j1939_2.other_ECU_address[i] = 0xFF;
}

/* Set the ECU address */
j1939_1.information_this_ECU.this_ECU_address = 0xA2;											/* From 0 to 253 because 254 = error address and 255 = broadcast address */
j1939_2.information_this_ECU.this_ECU_address = 0x90;

/* Set the Software Identification */
j1939_1.information_this_ECU.this_identifications.software_identification.number_of_fields = 15;
char text[15] = "SAE J1939!!!";
for(uint8_t i = 0; i < 15; i++)
	j1939_1.information_this_ECU.this_identifications.software_identification.identifications[i] = (uint8_t) text[i];

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

/* Response request from ECU 1 perspective - Don't worry, in real CAN applications you don't need this mess. */
Open_SAE_J1939_Listen_For_Messages(&j1939_1);
Open_SAE_J1939_Listen_For_Messages(&j1939_2);
Open_SAE_J1939_Listen_For_Messages(&j1939_1);

/* Read response request from ECU 1 to ECU 2 */
for(uint8_t i = 0; i < 5; i++)
	Open_SAE_J1939_Listen_For_Messages(&j1939_2);

/* 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);

As I understand it, you simulate two devices in this code. This mean that such code do this:

**/* Create J1939 structures for two ECU */**
J1939 j1939_1 = {0};
J1939 j1939_2 = {0};

**/* Set the ECU address of the two devices: device 1 with address 0xA2 and device 2 with address 0x90 */**
j1939_1.information_this_ECU.this_ECU_address = 0xA2;
j1939_2.information_this_ECU.this_ECU_address = 0x90;

/* Set the Software Identification */
    **/* I do not understood why 15 identification fields if the text has 13 characters */**
j1939_1.information_this_ECU.this_identifications.software_identification.number_of_fields = 15;
char text[15] = "SAE J1939!!!";
for(uint8_t i = 0; i < 15; i++)
	j1939_1.information_this_ECU.this_identifications.software_identification.identifications[i] = (uint8_t) text[i];

/* Request Software Identification from ECU 2 to ECU 1 */
    **/* This code send a RTS packet to device 1 */**
SAE_J1939_Send_Request(&j1939_2, 0xA2, PGN_SOFTWARE_IDENTIFICATION);

**/* this code receives the RTS message and responde it with a CTS message */**
Open_SAE_J1939_Listen_For_Messages(&j1939_1);
    **/ * this code receives the CTS message and send the data transfer packet with the software identification request */**
Open_SAE_J1939_Listen_For_Messages(&j1939_2);
     **/* this code receives the data transfer packet with the software identification request and sends the end of message acknowledgement */**
Open_SAE_J1939_Listen_For_Messages(&j1939_1);

    **/* as soon i understand, ECU 1 will responde the software identification requested by ECU 2 with broadcast messages */**
    **/* the first received message will be the end of message acknowledgement */**
    **/* and then the 15 fields text software identification */**
**/* but why 5 packets, if each packet can send 7 bytes of data? */**
    **/* i think that 4 listen messages calls would be sufficient (1 end of message acknowledgement and 3 data packets (15 characters)) */**
for(uint8_t i = 0; i < 5; i++)
	Open_SAE_J1939_Listen_For_Messages(&j1939_2);

I am right? If not, can you clarify the code better?

Thanks in advance,
best regards,
Gustavo

gustavowd commented

Hi Daniel. Second possible interpretation.

/ Create J1939 structures for two ECU /
J1939 j1939_1 = {0};
J1939 j1939_2 = {0};

/ Set the ECU address of the two devices: device 1 with address 0xA2 and device 2 with address 0x90 /
j1939_1.information_this_ECU.this_ECU_address = 0xA2;
j1939_2.information_this_ECU.this_ECU_address = 0x90;

/* Set the Software Identification */
/ I do not understood why 15 identification fields if the text has 13 characters /
j1939_1.information_this_ECU.this_identifications.software_identification.number_of_fields = 15;
char text[15] = "SAE J1939!!!";
for(uint8_t i = 0; i < 15; i++)
j1939_1.information_this_ECU.this_identifications.software_identification.identifications[i] = (uint8_t) text[i];

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

/ this code receives the Request Software Identification message and responde it with a RTS message /
Open_SAE_J1939_Listen_For_Messages(&j1939_1);
*/ * this code receives the RTS message and send a CTS packet /
Open_SAE_J1939_Listen_For_Messages(&j1939_2);
/ this code receives the CTS packet and sends first data transfer packet /
Open_SAE_J1939_Listen_For_Messages(&j1939_1);

**/* here will be receive the 15 fields text software identification */**

/ but why 5 packets, if each packet can send 8 bytes of data? /
/ i think that 3 listen messages calls would be sufficient (2 data packets (15 characters) and 1 end of message acknowledgement and ) /
for(uint8_t i = 0; i < 5; i++)
Open_SAE_J1939_Listen_For_Messages(&j1939_2);

I am right? If not, can you clarify the code better?

mr337 commented

I had a few thoughts on this but after reviewing more I don't have those thoughts anymore. Some of this could be just errors in part of a copy and paste. 🤷 We will have to see when the author replies back!

DanielMartensson commented

Hi Daniel. Thank you very much for your open source code for the j1939 protocol. It will be very useful for many people. However, I have a question regarding your API. Why do you call the Open_SAE_J1939_Listen_For_Messages() function in the loop and multiple times in the example requests if the function is in the stm32 interrupt callback? Will the function not be called whenever a frame comes through the CAN network? I'm starting to develop j1939 devices with your API and I would like to understand why call the function multiple times out of the interrupt. I intend to use an rtos and synchronize the requests by queues and semaphores.

Thanks in advance. Gustavo

Hello!

Why do you call the Open_SAE_J1939_Listen_For_Messages() function in the loop and multiple times in the example requests if the function is in the stm32 interrupt callback?

Not all users are using STM32. Some users are using this over USB or at Arduino.

DanielMartensson commented

Yes. I simulate two devices here. The source code works for all hardware. Right now I'm using this to communicate with QT application for Windows systems. QT are using C++ and this library does not take account on hardware or if it's C++ language.

gustavowd commented

Thank you very much Daniel. Could you comment if any of my interpretations of the process of requesting a software identification are correct? And try to clarify my doubts in the code comment?

DanielMartensson commented

Thank you very much Daniel. Could you comment if any of my interpretations of the process of requesting a software identification are correct? And try to clarify my doubts in the code comment?

If you want to have software identification onto an embedded system. Use this code then.

/*
 * Main.c
 *
 *  Created on: 16 juli 2021
 *      Author: Daniel Mårtensson
 */

#include <stdlib.h>
#include <stdio.h>

/* Include Open SAE J1939 */
#include "Open_SAE_J1939/Open_SAE_J1939.h"

int main() {

	/* Create our J1939 structure */
	J1939 j1939 = {0};

	/* Load your ECU information */
	Open_SAE_J1939_Startup_ECU(&j1939);

	/* Set the ECU address */
	j1939.information_this_ECU.this_ECU_address = 0x90;											/* From 0 to 

	/* Set the Software Identification if 0xA2 will ask 0x90 for its software identification */
	j1939.information_this_ECU.this_identifications.software_identification.number_of_fields = 15;
	char text[15] = "SAE J1939!!!";
	for(uint8_t i = 0; i < 15; i++)
		j1939.information_this_ECU.this_identifications.software_identification.identifications[i] = (uint8_t) text[i];

	/* Request Software Identification from an ECU that has an ID 0xA2 */
	SAE_J1939_Send_Request(&j1939, 0xA2, PGN_SOFTWARE_IDENTIFICATION);

	while(1){
           /* Read incomming message from 0xA2 */
	   Open_SAE_J1939_Listen_For_Messages(&j1939);
	}

	return 0;
}