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

TP.CM_RTS and TP.CM_CTS #42

Closed KomarovAlexandr closed 7 months ago

KomarovAlexandr commented 7 months ago

Hi! As a result of reading the code and applying it in practice, I found out that the response (TP.CM_CTS) and the request (TP.CM_RTS) to connect the transport protocol are the same. The remaining scripts and communication packets via the transport protocol did not have to be checked photo_2024-04-15_11-58-57

DanielMartensson commented 7 months ago

Thank you. But notice that that is only applied already. I, by myself, have implemented this so TP.CM_RTS and TP.CM_CTS does only change the control byte.

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Connection_Management.c#L21

If you are sure about your PR, then, make sure that the other examples are working as well.

Thank you.

KomarovAlexandr commented 7 months ago

Why does only the control byte change? I don't understand. The rest of the data package is completely different.

While watching the CAN-bus dump in debug, I was looking at the control byte and didn't understand why my system wasn't working. And I thought that everything was in order, since the control byte changes. Then I noticed in the documentation that the package was also different.

Thank you.

DanielMartensson commented 7 months ago

Why does only the control byte change? I don't understand. The rest of the data package is completely different.

While watching the CAN-bus dump in debug, I was looking at the control byte and didn't understand why my system wasn't working. And I thought that everything was in order, since the control byte changes. Then I noticed in the documentation that the package was also different.

Thank you.

@KomarovAlexandr

Thank you for noticing it. I will give you a full explanation.

Because when you are sending data larger than 8 bytes, you need to send it either as BAM or CTS/RTS.

BAM is used when you want to broadcast to all ECUs. CTS/RTS is used when you want to transmit to a specific ECU.

So, when two ECUs is going to communicate with data larger than 8 bytes, e.g 9 bytes. Let's have a pratical example.

Assume that ECU1 want to change the address on ECU2. From 0x80 to 0x81. First ECU1 send a RTS request with

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-81_Network_Management_Layer/Commanded_Address.c#L19-L38

We then transmitting the RTS request from ECU1 to ECU2

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Connection_Management.c#L38-L50

Once ECU2 is receiving the RTS request

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/Open_SAE_J1939/Listen_For_Messages.c#L53-L55

Then ECU2 read the message, but it sends back a CTS response, when the same amount of data. The RTS changes to CTS.

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Connection_Management.c#L10-L26

So when ECU1 is receiving the CTS response

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/Open_SAE_J1939/Listen_For_Messages.c#L53-L55

Then ECU1 read the CTS response

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Connection_Management.c#L28-L31

And then ECU1 do the Data Transfer to ECU2

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Data_Transfer.c#L83-L108

And ECU2 is reading the response

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/Open_SAE_J1939/Listen_For_Messages.c#L56-L58

And ECU2 storing the data as long as it has not fully receiving the complete message

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Data_Transfer.c#L15-L81

Notice that ECU2 is sending back and confirm/acknowledgement once the message is totaly transmitted

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Data_Transfer.c#L45-L48

So your SAE J1939 protocol, the image you are posting, is different from the protocols I have been using. Your image say that ECU2 is going to send a confirmation/acknowledgement to the transmitter for every package that has been send, e.g

  1. RTS request to receiver
  2. The receiver receives the RTS request and send a CTS to the transmitter
  3. The transmitter is reading the CTS response and send the first package
  4. The receiver receives the first package and send an confirmation/acknowledgement to the transmitter that the next package can be transmitted.
  5. The transmitter sends the next package and waiting for a confirmation/acknowledgement from the receiver.

But Open SAE J1939 works like other manufactures that are using SAE J1939 protocol. I have not made this image.

Source: https://copperhilltech.com/blog/sae-j1939-programming-with-arduino-multipacket-peertopeer-rtscts-session/ image

If you want to implement the CTS response after each TP.DT package, you need to send the CTS response here. https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Data_Transfer.c#L28-L30

And the transmitter should read the CTS response through a callback pointer function, instead of the callback pointer function CAN_Delay(100);. If the tranmitter is not given any CTS response within a certain time, then the for-loop must break. https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Data_Transfer.c#L100-L103

Create a CTS function that very much look as this RTS function. Or add an enum argument to the function CTS_RTS etc... https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Connection_Management.c#L38-L50

I would be good if you could send a PR to this repo for that.

I hope this information helps. Thank you

KomarovAlexandr commented 7 months ago

Thank you very much for the detailed explanation! I completely agree with everything you wrote. We discussed this with the team again. I’ll also try to describe in detail the problem I’m talking about.

According to the SAE J1939 standard, RTS and CTS packages differ from each other by five bytes (the screenshot was presented in the first message):

  1. In RTS the control byte is 16, in CTS the control byte is 17
  2. In RTS, the second and third bytes store the total message size. In CTS, the second byte contains the number of packets that will be transmitted. The third byte in the CTS contains the number of the next packet sent
  3. In RTS, the fourth byte is equal to the number of packets. In CTS, the fourth byte is 0xFF.
  4. In RTS, the fifth byte is equal to the maximum number of packets. In CTS, the fifth byte is also 0xFF. Bytes six through eight in RTS and CTS are the same and contain the PGN.

That is, in the CST and RTS packets only the last three bytes are the same, otherwise they are completely different.

Next, let's move on to the transmission chain that you wrote about above. Let's consider the same example.

Assume that ECU1 want to change the address on ECU2. From 0x80 to 0x81. First ECU1 send a RTS request with

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-81_Network_Management_Layer/Commanded_Address.c#L18-L35

Note that this code fragment ends with a call SAE_J1939_Send_Transport_Protocol_Connection_Management(), which will generate an RTS packet in the buffer and transmit it to the CAN_Send_Message() function.

Next, ECU2 receives the RTS packet, processes it and generates a CTS response. Packet formation is also completed by calling SAE_J1939_Send_Transport_Protocol_Connection_Management()

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Connection_Management.c#L14-L26

So, in both cases, ECU1 and ECU2 will call the SAE_J1939_Send_Transport_Protocol_Connection_Management() function. But the SAE_J1939_Send_Transport_Protocol_Connection_Management() function forms the same byte order and does not take into account the fact that the packets are different. The only difference between RTS and CTS that will be reflected in the final packet is the control byte.

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Connection_Management.c#L38-L50

DanielMartensson commented 7 months ago

So, in both cases, ECU1 and ECU2 will call the SAE_J1939_Send_Transport_Protocol_Connection_Management() function. But the SAE_J1939_Send_Transport_Protocol_Connection_Management() function forms the same byte order and does not take into account the fact that the packets are different. The only difference between RTS and CTS that will be reflected in the final packet is the control byte.

https://github.com/DanielMartensson/Open-SAE-J1939/blob/3c80bbdd0458b10e5fc77ca87a4c8b8a05d0296e/Src/SAE_J1939/SAE_J1939-21_Transport_Layer/Transport_Protocol_Connection_Management.c#L38-L50

@KomarovAlexandr

Yes. So it must be a Transport_Protocol_Connection_Management_RTS.c and Transport_Protocol_Connection_Management_CTS.c function. Or something like that e.g the suggestion you wrote. It seems to work well.

Notice that during the DP.DT, the CTS need to be transmitted every time.

Are you going to send a PR about this? I can help you with suggestions, code and recommendations.

KomarovAlexandr commented 7 months ago

@DanielMartensson I can create two functions: SAE_J1939_Send_Transport_Protocol_Request_To_Send(J1939 *j1939, uint8_t DA) SAE_J1939_Send_Transport_Protocol_Clear_To_Send(J1939 *j1939, uint8_t DA) or SAE_J1939_Send_Transport_Protocol_RTS(J1939 *j1939, uint8_t DA) SAE_J1939_Send_Transport_Protocol_CTS(J1939 *j1939, uint8_t DA)

It seems unnecessary to me to separate these two short functions into separate files. Moreover, these functions differ in only three lines.

DanielMartensson commented 7 months ago

@DanielMartensson I can create two functions: SAE_J1939_Send_Transport_Protocol_Request_To_Send(J1939 *j1939, uint8_t DA) SAE_J1939_Send_Transport_Protocol_Clear_To_Send(J1939 *j1939, uint8_t DA) or SAE_J1939_Send_Transport_Protocol_RTS(J1939 *j1939, uint8_t DA) SAE_J1939_Send_Transport_Protocol_CTS(J1939 *j1939, uint8_t DA)

It seems unnecessary to me to separate these two short functions into separate files. Moreover, these functions differ in only three lines.

@KomarovAlexandr

I like the function names that ends with CTS and RTS. Place them in the same .c file as the Send Transport Protocol

KomarovAlexandr commented 7 months ago

I ran into some problem while editing the functions. There are several other dependencies on other types of packets in the transport protocol. I don’t have the opportunity to check it at work right now. So I will add these changes later when it becomes possible to check You can leave this PR open and I will add changes to it later. Or you can close this PR and I’ll create a new one later

DanielMartensson commented 7 months ago

I ran into some problem while editing the functions. There are several other dependencies on other types of packets in the transport protocol. I don’t have the opportunity to check it at work right now. So I will add these changes later when it becomes possible to check You can leave this PR open and I will add changes to it later. Or you can close this PR and I’ll create a new one later

@KomarovAlexandr

Thank you. Yes, use refactoring if you want to rename the function names (If your IDE supporting that. This project has a lack of support for CMake, only support for MSBuild). The current SAE_J1939_Send_Transport_Protocol.c file is for RTS.

I think you could close this PR and open a new PR. It's much better if you can control and handle this process. Just ask me anything and I will help you with the code.

Best regards Daniel Mårtensson

DanielMartensson commented 7 months ago

@KomarovAlexandr How it is going with the PR?

KomarovAlexandr commented 7 months ago

Hi @DanielMartensson I'm sorry I didn't respond. Yes, let's close this PR for now. I don't have the opportunity to fully debug this node right now because I've noticed potential problems with other communication formats. I hope that in the near future the possibility of debugging will appear and I will create a new PR

DanielMartensson commented 7 months ago

@KomarovAlexandr

Hi! I have added code for CTS and RTS now. Can you try it. I haven't try it onto embedded systems. Only Windows.

Thank you