Can __HAL_LOCK in stm32 code cause race condition issues

[Kefei Yao]

Posted on March 19, 2018 at 10:24

The __HAL_LOCK macro is defined as in stm32f1xx_hal_def.h

#define __HAL_LOCK(__HANDLE__) \

do{ \

if((__HANDLE__)->Lock == HAL_LOCKED) \

{ \

return HAL_BUSY; \

} \

else \

{ \

(__HANDLE__)->Lock = HAL_LOCKED; \

} \

}while (0)

So the Lock is read first and modified if needed. Would it cause race condition while the task calling __HAL_LOCK is preempted by a higher priority task which call __HAL_LOCK subsequently.  

For example, task1 calls __HAL_LOCK and find the lock is open. task 1 is preempted by task2 whose priority is higher than task1 right before task1 write the Lock as HAL_LOCKED. task2 finds the lock is open and set the Lock as HAL_LOCKED. The task2 goes to block state to wait for some resource. Then task1 runs again and continue to get the lock. This will potentially corrupt the resources protect by the __HAL_LOCK for task2.

I would be happy if someone could correct me if I'm wrong since I could learn a lesson from it

[Darius B]

Posted on March 19, 2018 at 11:18

Hi,

It seem to me, that your are right with multitasking. This code   

__HAL_LOCK

is dangerous.  It must be used from 1 task.

If you want use in multitask like you, then need use critical sections (disable/enable interrupts)  example (not tested).

#define __HAL_LOCK(__HANDLE__) \

do{ \

    __disable_interrupts();

   if((__HANDLE__)->Lock == HAL_LOCKED) \

   { \

      /*

return HAL_BUSY; if we return, our task lost

*/  \

    __enable_interrupts();   /*let other task unlock */  

\

      

Os_delay(1); /*let other task unlock */  \

   } \

   else \

   { \

      (__HANDLE__)->Lock = HAL_LOCKED; \

      __enable_interrupts(); /*let other task unlock */  \

 

      break; \

   } \

}while (1)

BR.

Darius Babrauskas

[Kefei Yao]

Posted on March 19, 2018 at 12:03

Thanks for the reply. What do you mean by 'if we return, our task lost'. The original __HAL_LOCK macro return HAL_BUSY if it is already locked by others. Shoudn't the thread safe version __HAL_LOCK follow the same regulation as the original one?

[Darius B]

Posted on March 19, 2018 at 12:17

I mean, that we lost task (maybe better job). See code below from stm 

if in   HAL_ADC_Start     __HAL_LOCK(hadc);  return,   then   ADC not start measure.

HAL_StatusTypeDef HAL_ADC_Start(ADC_HandleTypeDef* hadc)

{

  HAL_StatusTypeDef tmp_hal_status = HAL_OK;

  ADC_TypeDef       *tmpADC_Master;

 

  /* Check the parameters */

  assert_param(IS_ADC_ALL_INSTANCE(hadc->Instance));

 

  /* Perform ADC enable and conversion start if no conversion is on going */

  if (ADC_IS_CONVERSION_ONGOING_REGULAR(hadc) == RESET)

  {

    /* Process locked */

    __HAL_LOCK(hadc);

.............................

BR.

Darius Babrauskas

[Morty Morty]

Posted on May 14, 2018 at 17:21

It's broken. See

https://community.st.com/0D50X00009XkeOGSAZ

for solutions.

[Darius B]

Posted on May 14, 2018 at 19:21

Thank you Morty.

I readed your link. But not sure that they are good solution. 

My solution are tested by me more times and worked Ok. 

BR.

Darius

[T J]

Posted on May 15, 2018 at 03:38

another reason to never use multi tasking.

I find it best to process all state machines within a single foreground loop.

that way, there is no issue of Printf being interrupted on your debug screen.

in this foreground loop we process each state machine sequentially, much like a Multitasking solution.

the down side is we never sleep, but in our case we have power to burn.

Our unit draws 6 Watts since we have real work to do, the processor only draws 60mA at 3.3V is 0.2Watts

Sleeping will drop that to about 1/3 (I guess), so we save 0.13Watts if we sleep.

not worth the huge debug issues when switching tasks in the middle of a print statement.

[AvaTar]

Posted on May 15, 2018 at 08:25

another reason to never use multi tasking.

IMHO a bit too broad of a statement.

There do exist processors with multiple physical cores, which are only properly exploited with a multitasking OS.

But for a plain Cortex M, I agree.

And comprehending multiple, concurrent processes (including it's consequences on data handling) is quite a task ...

[sudo112358]

Hi there,

correct, concurrency is the keyword. We just had the problem that while starting to send a message over CAN bus, a receive interrupt can always occur....finally, we found out about this problem by hard search and just after a module stopped receiving any messages due to this race condition...the receive interrupt was forever disabled....of course in a (prototype) system running in a customer's application.

So, like in the Linux kernel: "If it's not broken, don't fix it." (mind the inverse logic :)

[Darius B]

Hi,

Can you provide your code?

Darius

[sudo112358]

Hi,

I can't because it's part of our product. But the problem appears when we use the HAL library for CAN communication, transmit and receive in interrupt mode. When our module is just entering the transmit interrupt routine and simultaneously receives a message, the lock fails and as far as we could debug, the receive routine has already disabled the receive interrupt and will somehow not run through the receive callback which would enable the interrupt again. I can check in the source code tomorrow for details, but the principal behaviour is like this. We had the system running for weeks in an application at a customer's site and suddenly it did not "receive" any messages - in other words, the interrupt didn't fire anymore. In our lab, we built the same setup with a higher rate of sent and received messages and it just takes minutes, sometimes even some seconds until it fails. Manually re-setting the interrupt flag for reception in debug solves the problem until it crashes again. Our current workaround is quite ugly, but as one of the next steps, we will switch to the LL-lib or finally implement it ourselves..the HAL has several problems and a strange code quality imho...

Greetings,

Chris

[T J]

I use the Canbus without issue, but I dont use the CanTx interrupts, they are not enabled.

I use a CanTx message table and a CanRx message table both 64 frames long.

During the CanRx interrupt, I pull the new frame and save it in the table.

In the primary foreground process,

If there is a CanTx message frame ready to go, then it is sent without enabling any interrupt.

I only look at MsgIDs in the ranges 0x400 - 0x5FF and 0x600-0x6FF 
 
This is my Startup init code:
 
char canRxString [1024];
uint16_t canRxMsgID [64];
uint8_t canRxMsgLength [64];
uint32_t canRxMsgBottomWord [64];
uint32_t canRxMsgTopWord [64];
char 	 canRxHaveSegmentID [64];							
 
char canTxString[1024];
uint16_t canTxMsgID [64];
uint8_t canTxMsgLength [64];
uint32_t canTxMsgBottomWord [64];
uint32_t canTxMsgTopWord [64];
 
 
in HAL void CAN_Config() {
:
.
.
.
 
 //##-2- Configure the CAN Filter ###########################################
 sFilterConfig.FilterNumber 					= 0;
 sFilterConfig.FilterMode 					= CAN_FILTERMODE_IDMASK;
 sFilterConfig.FilterScale 					= CAN_FILTERSCALE_32BIT;
 sFilterConfig.FilterIdHigh 					= 0x400 << 5; //11-bit ID in top bits
 sFilterConfig.FilterIdLow 					= 0x0000;
 sFilterConfig.FilterMaskIdHigh	 	 = 0x600 << 5; 	// resolves as 0x0400 - 0x05FF
 sFilterConfig.FilterMaskIdLow				= 0x0000;
 sFilterConfig.FilterFIFOAssignment	 = 0;
 sFilterConfig.FilterActivation	 = ENABLE;
 sFilterConfig.BankNumber 	 			= 0;
 
 if (HAL_CAN_ConfigFilter(&hcan, &sFilterConfig) != HAL_OK)
 {
 // Filter configuration Error 
 _Error_Handler(__FILE__, __LINE__);
 char string[38];
 
 sprintf( string, "Can Filter configuration Error\n\r");
 puts1( string);
 }
	
 
 //##-2- Configure the 2nd CAN Filter ###########################################
 sFilterConfig.FilterNumber 			= 1;
 sFilterConfig.FilterMode 			= CAN_FILTERMODE_IDMASK;
 sFilterConfig.FilterScale 			= CAN_FILTERSCALE_32BIT;
 sFilterConfig.FilterIdHigh 			= 0x600 << 5; //11-bit ID in top bits		// command channel
 sFilterConfig.FilterIdLow 			= 0x0000;
 sFilterConfig.FilterMaskIdHigh		= 0x700 << 5; 	// resolves as 0x0600 - 0x06FF
 sFilterConfig.FilterMaskIdLow		= 0x0000;
 sFilterConfig.FilterFIFOAssignment	= 1;
 sFilterConfig.FilterActivation		= ENABLE;
 sFilterConfig.BankNumber 			= 1;
 
 if (HAL_CAN_ConfigFilter(&hcan, &sFilterConfig) != HAL_OK)
 {
 // Filter configuration Error 
 _Error_Handler(__FILE__, __LINE__);
 char string[38];
 sprintf( string, "Can Filter configuration Error\n\r");
 puts1( string);
 }
 
 
 
after HAL Can init code:
 
void initCairoCan(void) {
 CAN_Config();
 canRxpointerIN = 0;
 canRxMsgIN = 0;
 canRxMsgOUT = 0;
 canRxMsgTableEMPTY = true;
 canRxMsgTableFULL = false; 
 
 for (int i = 0; i < 16; i++)
 IOCanMsgFlag[i] = false;
 
			
	
 __HAL_CAN_ENABLE_IT(&hcan, CAN_IT_FMP1);
 __HAL_CAN_ENABLE_IT(&hcan, CAN_IT_FMP0);
		
 canTxMsgIN = 0;
 canTxMsgOUT = 0;
 canTxMsgTableEMPTY 		= true;
 canTxMsgTableFULL 		= false;
 canTxMsgTableOverflow = false;
 canTxMsgOverrun = false;
 blockCanTx = false;
 
}
 
I edited the interrupt code :
void CEC_CAN_IRQHandler(void) {
 /* USER CODE BEGIN CEC_CAN_IRQn 0 */
 CAN_IRQFlag = true;
 checkCanRxFifos();
 /* USER CODE END CEC_CAN_IRQn 0 */
 HAL_CAN_IRQHandler(&hcan);
 /* USER CODE BEGIN CEC_CAN_IRQn 1 */
 
 /* USER CODE END CEC_CAN_IRQn 1 */
}
 
 
void checkCanRxFifos(void) {
	
 int readCanBytecount;
	
 char canFifo1FullFlag = CAN->RF1R & CAN_RF1R_FMP1;
 if (canFifo1FullFlag) {
 {
 readCanBytecount = (CAN->sFIFOMailBox[1].RDTR & 0x0f);	
 
 canRxMsgBottomWord[canRxMsgIN]	= CAN->sFIFOMailBox[1].RDLR;
 canRxMsgTopWord[canRxMsgIN]	= CAN->sFIFOMailBox[1].RDHR;
 
 canRxMsgID[canRxMsgIN] = CAN->sFIFOMailBox[1].RIR >> 21;
 CAN->RF1R |= CAN_RF1R_RFOM1; 						// release FIFO 
 
 canRxMsgLength[canRxMsgIN] =	readCanBytecount; 
 
 canRxMsgIN++;
 canRxMsgIN &= 0x3F; 	// 64 entries only
 canRxMsgTableEMPTY = false;
 if (canRxMsgIN == canRxMsgOUT) canRxMsgTableFULL = true;
 }
 CAN->IER |= CAN_IER_FMPIE1; 		 				 // (11)		Set FIFO1 message pending IT enable 
 }
 
 char canFifo0FullFlag = CAN->RF0R & CAN_RF0R_FMP0;
 if (canFifo0FullFlag) {
 {
 readCanBytecount 		= (CAN->sFIFOMailBox[0].RDTR & 0x0f);	
 
 canRxMsgBottomWord[canRxMsgIN]	= CAN->sFIFOMailBox[0].RDLR;
 canRxMsgTopWord[canRxMsgIN]	= CAN->sFIFOMailBox[0].RDHR;
 
 uint32_t canRxmsgID = CAN->sFIFOMailBox[0].RIR >> 21;
 CAN->RF0R |= CAN_RF0R_RFOM0; 						// release FIFO 
 
 if(canRxMsgTableFULL) {	
 canRxMsgTableOverflow = true; // now dump new frame...
 }else {
 canRxMsgID[canRxMsgIN] = canRxmsgID;
 canRxMsgLength[canRxMsgIN] = readCanBytecount; 	
 canRxMsgIN++;
 canRxMsgIN &= 0x3F; 	// 64 entries only
 canRxMsgTableEMPTY = false;
 if (canRxMsgIN == canRxMsgOUT)
 canRxMsgTableFULL = true;
 }
			
 //length = sprintf(string + length,"%08X, %08X :W",canFifoBuf.d32[0],canFifoBuf.d32[1]);
 //			for( int i = 0; i < readCanBytecount; i++){			
 //				canRxBuffer[canRxpointerIN++] = canFifoBuf.d8[i];			
 //				canRxpointerIN &= 0xFF;
 //				if (canRxpointerIN == canRxpointerOUT )		CanRxbufferOverrun = true;
 //				//length += sprintf(string + length,"%02X, ",canFifoBuf.d8[i]);
 //			}
 //sprintf(string + length -2,"\n\r");		// remove 2 bytes, the last comma and space
 
 }
 CAN->IER |= CAN_IER_FMPIE0; 		// (11)		Set FIFO1 message pending IT enable 
 }
 //			if (length >0) puts(string);
 
}
inside my foreground process I call DoCan, which only checks all new RXframes before it leaves.
 
 
void DoCan(void) {
 char string[1024];
 int length = 0;
 if (canRxMsgTableOverflow) {
 canRxMsgTableOverflow = false;
 sprintf(string, "CanFlags Table has Overflowed, new frames are being dumped\n\r"); // now dump new frame...
 puts1(string);
 }
 if (canRxMsgTableFULL) {
 canRxMsgTableFULL = false;
 sprintf(string, "CanFlags Table is Full, probably missing frames now\n\r");
 puts1(string);
 }
			
 if (checkCanRxSensorFreshDataCompleted()) 
 showCanSensorADCArray();
 if (haveCanString)
 showCanString();
 
 			
 while (!canRxMsgTableEMPTY) {
 int canRxMsgBytecount = canRxMsgLength[canRxMsgOUT];
					
 int16_t dataChannel 	= canRxMsgID[canRxMsgOUT];
 canRxCmdFifoBuf.d32[0] 	= canRxMsgBottomWord[canRxMsgOUT];
 canRxCmdFifoBuf.d32[1] 	= canRxMsgTopWord[canRxMsgOUT];
 
 // zero data within RxCanFrameBuffer to make sure we only use valid data, but not necessary
 canRxMsgID[canRxMsgOUT] = 0; 
 canRxMsgBottomWord[canRxMsgOUT] = 0;
 canRxMsgTopWord[canRxMsgOUT] = 0;
 canRxMsgLength[canRxMsgOUT] = 0;				
 
 /*	if (dataChannel == 0x418) 
 		while(1);
 	if (dataChannel == 0x408) 
 		while(1);
 */
 canRxMsgTableFULL = false;
 canRxMsgOUT++;
 canRxMsgOUT &= 0x3f; 		// only 16 messages
 if(canRxMsgOUT == canRxMsgIN) canRxMsgTableEMPTY = true;
			.
			.
			.
			.
		process frame here
 
 
 
 
the transmit function
if ((CAN->TSR & CAN_TSR_TME0) == CAN_TSR_TME0) // (1) 
{
 
 CAN->sTxMailBox[0].TDTR = canTxMsgLength[canTxMsgOUT]; // (2) length
 CAN->sTxMailBox[0].TDLR = canTxMsgBottomWord[canTxMsgOUT]; // (3) 4bytes
 CAN->sTxMailBox[0].TDHR = canTxMsgTopWord[canTxMsgOUT]; // (3) 4bytes
 CAN->sTxMailBox[0].TIR = ((uint32_t)canTxMsgID[canTxMsgOUT] << 21 | CAN_TI0R_TXRQ); // (4) // send it now if the line is idle
 
 
}
 
if ((CAN->TSR & CAN_TSR_TME1) == CAN_TSR_TME1) // (1) 
 {
if ((CAN->TSR & CAN_TSR_TME2) == CAN_TSR_TME2) // (1) 
 {
 
 
 
void CAN1_RX0_IRQHandler(void)
{
 /* USER CODE BEGIN CAN1_RX0_IRQn 0 */
 //CAN_Rx0_IRQFlag = true;
 //CAN_IRQFlag = true;
 checkCanRxFifos();
 
 /* USER CODE END CAN1_RX0_IRQn 0 */
 HAL_CAN_IRQHandler(&hcan1);
 /* USER CODE BEGIN CAN1_RX0_IRQn 1 */
 
 /* USER CODE END CAN1_RX0_IRQn 1 */
}
 
/**
* @brief This function handles CAN1 RX1 interrupt.
*/
void CAN1_RX1_IRQHandler(void)
{
 /* USER CODE BEGIN CAN1_RX1_IRQn 0 */
// CAN_Rx1_IRQFlag = true;
// CAN_IRQFlag = true;
 checkCanRxFifos();
 
 /* USER CODE END CAN1_RX1_IRQn 0 */
 HAL_CAN_IRQHandler(&hcan1);
 /* USER CODE BEGIN CAN1_RX1_IRQn 1 */
 
 /* USER CODE END CAN1_RX1_IRQn 1 */
}