Problem with generated CAN_LDD driver code for FlexCAN

[Laartoor]

When I look at the generated code for sending a frame, I wonder whether there is a race condition in it.

The code looks like this:

  uint32_t StatusReg = CAN_PDD_GetStatusInterruptFlags1(CAN0_BASE_PTR); /* Read content of the status register */
  .... // other error checks
  if ((StatusReg & (CAN_PDD_RECEIVING_MESSAGE | CAN_PDD_TRANSMITTING_MESSAGE)) != 0x00U) {
    return ERR_BUSY;                   /* If yes then error */
  }
  /* {MQXLite RTOS Adapter} Critical section begin (RTOS function call is defined by MQXLite RTOS Adapter property) */
  _int_disable();
  CAN_PDD_SetMessageBufferCode(CAN0_BASE_PTR, BufferIdx, CAN_PDD_MB_TX_NOT_ACTIVE); /* Set TX Buffer Inactive */
  .... /// Filling the buffer
  CAN_PDD_SetMessageBufferCode(CAN0_BASE_PTR, BufferIdx, TxMBCode); /* Set code for Tx buffer of the message */
  /* {MQXLite RTOS Adapter} Critical section ends (RTOS function call is defined by MQXLite RTOS Adapter property) */
  _int_enable();
  return ERR_OK;

I see the following problems with it:

a) the code can be interrupted in any place between lines 1 and 7, meaning that with a sufficiently long delay the message buffer will be updated while the module is active.

b) the code checks the global activity flag, instead of checking the buffer to be updated. With a high load on the CAN bus, this will lead to extra delays.

c) the code disables interrupts while it updates the buffer content. Is this really necessary given that FlexCAN will not touch buffers marked as inactive?

Would this at least be correct?

  uint32_t StatusReg;
  .... // other error checks
  _int_disable();
  StatusReg = CAN_PDD_GetStatusInterruptFlags1( CAN0_BASE_PTR);
  if ((StatusReg & (CAN_PDD_RECEIVING_MESSAGE | CAN_PDD_TRANSMITTING_MESSAGE)) != 0x00U) {
    _int_enable();
    return ERR_BUSY;
  }
  CAN_PDD_SetMessageBufferCode( CAN0_BASE_PTR, BufferIdx, CAN_PDD_MB_TX_NOT_ACTIVE); 
  _int_enable();
  .... // Fill the buffer
  CAN_PDD_SetMessageBufferCode( CAN0_BASE_PTR, BufferIdx, TxMBCode);
  return ERR_OK;

I still wonder whether the delay between checking the CAN activity and updating the buffer code is guaranteed to be short enough to not cause any interference.

[Petr_H · Accepted solution]

Hi,

I have received the following comments from our development team:

ad a)  the code can be interrupted in any place between lines 1 and 7, meaning that with a sufficiently long delay the message buffer will be updated while the module is active.

Yes, we confirm this issue, the state checking of CAN device is not in critical section, which provides a place for possible HW state change before message buffer starts to be filled.

ad b) the code checks the global activity flag, instead of checking the buffer to be updated. With a high load on the CAN bus, this will lead to extra delays.

Yes, you are also right in this point. We are changing this to check the state of each TX buffer individually.

ad c) the code checks the global activity flag, instead of checking the buffer to be updated. With a high load on the CAN bus, this will lead to extra delays.

We think if should be in "critical section". It could happen that filling of message buffer is interrupted by some event code, which could fill data into the buffer and after returning, the same message buffer would be overwritten.

The points a) and b) are fixed for next update of Processor Expert Driver Suite 10.4 and CodeWarrior 10.6. 

As an immediate workaround, please follow these steps:

1) Configure the CAN component according to your needs

2) In the pop-up menu of the CAN_LDD component select the option Code Generation > Don't Write Generated Component Modules

3) int the same pop-up menu select Open File > Generated_code/Can1.c (or what is your name of c file of the CAN component) and open this file

4) Replace the content of the SendFrame function with the following:

LDD_TError CAN1_SendFrame(LDD_TDeviceData *DeviceDataPtr, LDD_CAN_TMBIndex BufferIdx, LDD_CAN_TFrame *Frame) 
{ 
  CAN1_TDeviceData *DeviceDataPrv = (CAN1_TDeviceData *)DeviceDataPtr; 
  LDD_CAN_TBufferMask BufferMask;      /* Bitmask of the requested message buffer */ 
  uint8_t TxMBCode = 0x00U;            /* Temporary value of MB code */ 
  uint8_t DataIndex; 

  if (BufferIdx > DeviceDataPrv->MaxBufferIndex) { /* Is BufferIdx greater than MaxBuffers? */ 
    return ERR_PARAM_RANGE;            /* If yes then error */ 
  } 
  BufferMask = (LDD_CAN_TBufferMask)(0x01UL << BufferIdx); 
  if ((BufferMask & DeviceDataPrv->TxBufferMask) == 0x00U) { /* Is used buffer defined of BufferIdx for transmit? */ 
    return ERR_PARAM_INDEX;            /* If no then error */ 
  } 
  if (Frame->Length > DeviceDataPrv->MaxDataLen) { /* Is number of data greater than MaxDataLen? */ 
    return ERR_PARAM_LENGTH;           /* If yes then error */ 
  } 
  if (Frame->FrameType > LDD_CAN_RESPONSE_FRAME) { /* Is FrameType other than LDD_CAN_DATA_FRAME_STD, LDD_CAN_DATA_FRAME_EXT or LDD_CAN_REMOTE_FRAME? */ 
    return ERR_PARAM_ATTRIBUTE_SET;    /* If yes then error */ 
  } 
  /* {Default RTOS Adapter} Critical section begin, general PE function is used */ 
  _int_disable(); 
  if (CAN_PDD_GetMessageBufferCode(CAN0_BASE_PTR, BufferIdx) != CAN_PDD_MB_TX_NOT_ACTIVE) { /* Is Tx buffer inactive */ 
    /* {Default RTOS Adapter} Critical section end, general PE function is used */ 
    _int_enable(); 
    return ERR_BUSY;                   /* If no then error */ 
  }         
  if ((Frame->MessageID & LDD_CAN_MESSAGE_ID_EXT) != 0x00U) { /* Is the frame configured as Extended ID? */ 
    CAN_PDD_SetMessageBufferID(CAN0_BASE_PTR, BufferIdx, CAN_PDD_BUFFER_ID_EXT, (Frame->MessageID)&~(LDD_CAN_MESSAGE_ID_EXT)); /*Assign extended ID to buffer */ 
    CAN_PDD_EnableMessageBufferIDExt(CAN0_BASE_PTR, BufferIdx, PDD_ENABLE); /*Set ID extended */ 
  } else { 
    CAN_PDD_SetMessageBufferID(CAN0_BASE_PTR, BufferIdx, CAN_PDD_BUFFER_ID_STD, Frame->MessageID); /*Assign Standard ID to buffer */ 
    CAN_PDD_EnableMessageBufferIDExt(CAN0_BASE_PTR, BufferIdx, PDD_DISABLE); /*Set ID standard */ 
  } 
  if ((Frame->FrameType == LDD_CAN_DATA_FRAME)||(Frame->FrameType == LDD_CAN_RESPONSE_FRAME)) { /* Is it a data or WaitOnRemote frame? */ 
    for (DataIndex = 0x00U; DataIndex < Frame->Length; DataIndex++) { /* Fill message buffer data array */ 
      CAN_PDD_SetMessageBufferData(CAN0_BASE_PTR, BufferIdx, DataIndex, Frame->Data[DataIndex]); 
    } 
    CAN_PDD_SetMessageBufferDataLength(CAN0_BASE_PTR, BufferIdx, Frame->Length); /* Set the length of the message */ 
    CAN_PDD_EnableMessageBufferRTR(CAN0_BASE_PTR, BufferIdx, PDD_DISABLE); /* Clear RTR bit */ 
    CAN_PDD_EnableMessageBufferSRR(CAN0_BASE_PTR, BufferIdx, PDD_DISABLE); /* Clear SRR bit */ 
    if (Frame->FrameType == LDD_CAN_DATA_FRAME) { 
      TxMBCode = CAN_PDD_MB_TX_DATA_FRAME; /* Set buffer as a transmit buffer */ 
    } else { 
      TxMBCode = CAN_PDD_MB_TX_RESPONSE_FRAME; /* Set buffer as a response transmit buffer for remote frames */ 
    } 
  } else {                             /* Remote frame */ 
    TxMBCode = CAN_PDD_MB_TX_REMOTE_FRAME; /* Set Tx buffer for remote frames */ 
    CAN_PDD_SetMessageBufferDataLength(CAN0_BASE_PTR, BufferIdx, 0x00U); /* Set the length of the message */ 
    CAN_PDD_EnableMessageBufferRTR(CAN0_BASE_PTR, BufferIdx, PDD_ENABLE); /* Set the message as a remote frame */ 
    if ((Frame->MessageID & LDD_CAN_MESSAGE_ID_EXT) != 0x00U) { /* Extended frame */ 
      CAN_PDD_EnableMessageBufferSRR(CAN0_BASE_PTR, BufferIdx, PDD_ENABLE); /* Set SRR bit */ 
    } else {                           /* Standard frame */ 
      CAN_PDD_EnableMessageBufferSRR(CAN0_BASE_PTR, BufferIdx, PDD_DISABLE); /* Clear SRR bit */ 
    } 
  } 
  CAN_PDD_SetMessageBufferCode(CAN0_BASE_PTR, BufferIdx, TxMBCode); /* Set code for Tx buffer of the message */ 
  /* {Default RTOS Adapter} Critical section end, general PE function is used */ 
  _int_enable(); 
  return ERR_OK; 
} 

Best regards

Petr Hradsky

Processor Expert Support Team

[Vagni]

I ported my K60-based application from CW 10.4 + MQX 4.0.2.2 to KDS 1.1.1 + MQX 4.1.1.

My BSP is based on PEx and my application use both FlexCAN channels. For each of them I use a CAN_LDD driver.

I see that in KDS PEx the SendFrame function is already updated in the generated code. But my test code on both channels does not work any more.

The generated CAN_LDD driver code differs from my previous I got with CW 10.4 essentially on the SendFrame function (the FlexCAN peripheral is initialized in the same mode).

I attach my test code and the ProcessorExpert project of my BSP.

On my board one CAN channel port is connected with the other CAN channel port and my code does the following:

1. Init CAN_LDD driver on both channels in the same mode.
2. Send a packet from the first CAN port and, after 300 msec, check its reception on the second CAN port.
3. Send a packet from the second CAN port and, after 300 msec, check its reception on the first CAN port.

With my firmware compiled with CodeWarrior the test result is ok (so my hardware is good).

The test fails with my new firmware compiled with KDS.

Some notes:

• All the other function of my KDS firmware are working properly (serial ports, ADC, SPI, USB, etc.) as with my CW firmware. So I can say my BSP library is good.
• I tried to incremet up to 2 sec the reception waiting time, but this does not fix the issue.
• All the CAN_LDD driver functions return without errors (except when ReadFrame returns ERR_RXEMPTY).
• With my scope I see that no packet is output from both the CAN channels when I call the SendFrame function.
• My test passes only if I execute my application with the debugger and with a breakpoint set at the following line of the SendFrame function (in both drivers):

CAN_PDD_SetMessageBufferCode(CAN1_BASE_PTR, BufferIdx, TxMBCode); /* Set code for Tx buffer of the message */

How can the debugger breakpoint unblock the frame transmission on the CAN driver?

Can you help me to fix this issue?

Best Regards,

Alessandro

[Laartoor]

Thank you for your response. I see your point about keeping the lock longer, it ensures correctness if you are using multiple threads to update one buffer, and given that the update will take only a couple dozens of cycles, the performance benefit would not be worth it to complicate the interface.