ISOTP conflicts when multiple CAN busses are enabled

[fredmcc]

Found a problem when I have more than 1 CAN bus enabled messages from all buses are sent to the single ISOTP instance and the messages get jumbled with each other and a lot of messages are lost. I will submit a pull request with a change that I am using to solve the problem. It filters for the same rx bus as the isotp write bus and ignores other buses that may be enabled. I can't imagine a situation where having multiple buses inputting to a single ISOTP message would be wanted. Not sure if my solution is the best or most elegant one but it does seem to work. If there is a better way of doing it I am happy to implement it.

[tonton81]

uint8_t getBusNumber() { return busNumber; } in the templated library "_bus" returns the bus, so you can also do: uint8_t getBusNumber() { return _bus; } no need to store a variable

well, it will return CAN1, CAN2, or CAN3 as the enum, which actually stores the hardware address and not a 1,2,3 value

There is only one bus on esp32 so that's fine

[tonton81]

You could actually also just need only to patch isotp, since you are adding in the bus to write to, you can condition check the read bus with write bus from the handler..., this wouldn't need FlexCAN_T4 to be adapted since isotp.h is an independant file

The objects that call the background handler are _CAN1 _CAN2 _CAN3, which is a FlexCAN_T4_Base pointer to whatever you named your bus (Can0?)

[fredmcc]

I originally tried to do it all in isotp but I couldn't work out how to get the bus number assigned to the FlexCAN_T4_Base as I think that is assigned in the derived class eg. FlexCAN_T4 and even then it's a private variable. Also not sure how I can determine the pointer of the FlexCAN_T4_Base that called the isotp function to check that against the write bus pointer. Do you know how I could do that?

[tonton81]

comparisons between &_CAN1 and &CAN1, i'm pretty sure i did this back when IFCT came before FlexCAN_T4

i took a quick peek: if ( this != &Can0 ) sequence

the "this" pointer will be either &_CAN1 or &_CAN2 or &_CAN3 for the background handler in isotp, you could probably even compare it against the writing pointer you set to see if it matches

[fredmcc]

If I was to use "this" within isotp wouldn't this be a pointer to isotp not FlexCAN_T4_Base? What i need is to identify the CAN bus the message came from into isotp. The only thing I can find right now is msg.bus which is a uint8_t. I still cant work out how to make the change in isotp only as I need to expose the bus number for the write bus to compare it.

It's possible I'm just missing something.

[tonton81]

the handler is in the isotp.tpp file for FlexCAN_T4. the _CANx and this pointer as well as the uint8_t can all be in there, if you want to check it against an isotp pointer just create a flexcan base pointer in the isotp class and use it in the background handler for comparison. the write function can be used to set the pointer, actually, you don't need to create a pointer since the isotp write pointer is of same base class, you can use it in the background handler just be sure to check for nullptr before comparison or an incomming frame will crash teensy before you set the writebus method

[fredmcc]

By background handler do you mean the callback _isotp_handler? I think this is only called after the messages have been assembled and are complete.

I'm not trying to determine which bus the isotp message came from. The issue is that messages are coming from multiple buses into _process_frame_data(const CAN_message_t &msg) . So as an example

7E8 10 14 49 02 01 31 47 54 comes in from CAN1 7E8 23 6c 00 00 aa aa aa aa comes in from CAN2 // Not part of the isotp message on CAN1 7E8 21 55 39 46 45 54 58 4d comes in from CAN1 7E8 22 5a 33 31 33 33 37 36 comes in from CAN1

The frame from CAN2 is not a part of the isotp message CAN1 is sending but because isotp does not check which bus the massages are coming from it processes it as part of the message it already started reading from CAN1 and this leads to it not being read properly. So I think I have to do the check in _process_frame_data(const CAN_message_t &msg) and discard any frames not coming from the bus that is set as the write bus.

[tonton81]

that is another way yes, using msg.bus to identify the bus in isotp handler. but you still need to identify it how with the isotp object? okay just compare in your isotp handler, along with a nullptr check, something like: ` if ( _ISOTP_OBJ ) { // nullptr check

if ( &_ISOTP_OBJ != &_CAN1 ) return;

else if ( &_ISOTP_OBJ != &_CAN2 ) return;

else ( &_ISOTP_OBJ != &_CAN3 ) return;

} else return; ` at the top of the isotp handler. Since it is isotp handler you are using you can replace _ISOTP_OBJ with "this", same thing

[fredmcc]

Not sure how comparing the address of the isotp object to a CAN address will help.

I think I can do something like this
if ( &_isotp_busToWrite != &_CAN1) return; But I dont know how doing that will help either as we need to know where the CAN frame came from. The msg doesnt specify which CAN address it came from. So comparing _isotp_busToWrite with the CAN addresses doesnt help. Only the address from which the frame came from and I dont think that is available in isotp.

The more I look into it the more I think the way I implemented it is probably the best. Modify FlexCAN_T4 to expose the bus number that it puts in the message object and use that to do the filtering.

Do you see an issue in my pull request?

[tonton81]

no i don't see an issue but in my case on my car over 2 busses i don't have frame collision because all IDs are different.

You should be able to get the msg.bus and object comparisons at the end of the tpp file

void ext_output2(const CAN_message_t &msg) { _ISOTP_OBJ->_process_frame_data(msg); }

Not sure how comparing the address of the isotp object to a CAN address will help.

if you compare all 3 objects, and it doesn't return, thats the bus that the isotp object will use

that or you can do a comparison in the set write pointer to identify the bus as a uint8_t and then compare that with the msg.bus in ext_output2

[fredmcc]

OK. So here is how I managed to contain it within isotp. I think this is what you are meaning. Not sure its a better solution than my original one because I think it will make future maintenance a bit more tricky because it is duplicating the logic of setting the bus number from the CANX from Flexcan_t4, but if you prefer I can submit a pull request for this way of doing it and not touch Flexcan_t4.

What do you think?

isotp.h

if defined(TEENSYDUINO) // Teensy

void setWriteBus(FlexCAN_T4_Base* _busWritePtr) { 
  _isotp_busToWrite = _busWritePtr; 
  #if defined(__IMXRT1062__)
    if ( _isotp_busToWrite == _CAN1 ) readBus = 1;    
    if ( _isotp_busToWrite == _CAN2 ) readBus = 2;
    if ( _isotp_busToWrite == _CAN3 ) readBus = 3;
  #endif
  #if defined(__MK20DX256__) || defined(__MK64FX512__) || defined(__MK66FX1M0__)
    if ( _isotp_busToWrite == _CAN0 ) readBus = 0;    
    if ( _isotp_busToWrite == _CAN1 ) readBus = 1;
  #endif
}

elif defined(ARDUINO_ARCH_ESP32) //ESP32

void setWriteBus(ESP32_CAN_Base* _busWritePtr) { _isotp_busToWrite = _busWritePtr; }

endif

isotp.tpp

if defined(TEENSYDUINO)

if ( msg.bus != readBus ) return;

endif

[tonton81]

yup that looks good, and isolated to the plugin end. Later on if we deprecate the setwritebus and move the bus to the constructor, we can instantiate an isotp per bus :)

[fredmcc]

Yeah sounds good. Would be good to have multiple isotp instances for sure.

Submitted the pull request.

Really like your Flexcan library by the way. Been using it a lot.

[fredmcc]

Just noticed the same change probably should also be made to isotp_server. Do you want me to submit a pull request for that also?

[tonton81]

ok