Race Condition in CAN_DataSend

[annego15]

Hi foxBMS team. I experienced issues with CAN messages with wrong data being sent. Every few minutes a message with senseless data is sent on valid CAN IDs.

The issue seems to come from the function CAN_DataSend (in the file src/app/driver/can/can.c) being called from multiple tasks, but this function is not threadsafe, leading to a race condition and corrupted data.
For example if a fatal error is detected the function CANTX_SendMessageFatalErrorCode is called which calls CAN_DataSend from a high priority task. However, it can happen that the periodic CAN send task was running and in the middle of executing CAN_DataSend. In this case two CAN messages are sent in the same mailbox and depending on when exactly the the lower priority task was preempted data is sent on a wrong ID or the data is completely messed.

I was able to fix the issue by wrapping the for loop in CAN_DataSend with OS_EnterTaskCritical(); and OS_ExitTaskCritical();

[foxBMS]

Dear @annego15,

thanks for your report. This is a bug and will be fixed in the next release.

The fix that will be shipped looks like this:

diff --git a/src/app/driver/can/can.c b/src/app/driver/can/can.c
index d6fe4c99..48d540d7 100644
--- a/src/app/driver/can/can.c
+++ b/src/app/driver/can/can.c
@@ -936,6 +936,7 @@ extern STD_RETURN_TYPE_e CAN_DataSend(CAN_NODE_s *pNode, uint32_t id, CAN_IDENTI
      *  In the HAL, message box numbers start from 1, not 0.
      */
     for (uint8_t messageBox = 1u; messageBox <= CAN_NR_OF_TX_MESSAGE_BOX; messageBox++) {
+        portDISABLE_INTERRUPTS();
         if (canIsTxMessagePending(pNode->canNodeRegister, messageBox) == 0u) {
             /* id shifted by 18 to use standard frame */
             /* standard frame: bits [28:18] */
@@ -958,8 +959,12 @@ extern STD_RETURN_TYPE_e CAN_DataSend(CAN_NODE_s *pNode, uint32_t id, CAN_IDENTI
             }
             canTransmit(pNode->canNodeRegister, messageBox, pData);
             result = STD_OK;
+            /* Re-enable interrupts if empty mailbox found */
+            portENABLE_INTERRUPTS();
             break;
         }
+        /* Re-enable interrupts if all mailboxes are full */
+        portENABLE_INTERRUPTS();
     }
     return result;
 }

Best regards,
The foxBMS Team

[annego15]

Thanks for fixing this!