More robust HAL USART implementation for RF module AT communication

[Thomas LB]

Hello,

My message is too long so I put the question first and context after:

Any advice regarding the most robust way to implement HAL_UART_ErrorCallback() ?

For now I have:

// UART Error Handle
void HAL_UART_ErrorCallback(UART_HandleTypeDef *huart)
{	
	HAL_UART_AbortReceive(&huart2);
	HAL_UART_DeInit(&huart2);
	MX_USART2_UART_Init();
	Ringbuf_Init(); // restart HAL_UARTEx_ReceiveToIdle_DMA(&UART, aRXBufferUser, RxBuf_SIZE);
}

I tried "upseting" the UART by sending data with wrong baudrate and it seems able to recover.

---------    Please don't tell me to trash HAL and go for LL :) you are probably right but it's working well (simple project, no multiple thread) ------------

I have a working configuration based on https://github.com/STMicroelectronics/STM32CubeL4/tree/master/Projects/NUCLEO-L476RG/Examples/UART/UART_ReceptionToIdle_CircularDMA

1) For transmission I use this function:

void send_uart_cellular(char * buffer)
{	
	uint8_t cpt = 0;
	transmit_done = 0;
	memset(msg, 0, tx_buffer_size); // Reset transmit buffer
	snprintf(msg, tx_buffer_size, "%s\r\n", buffer); // Add CRLF after command
	HAL_UART_Transmit_DMA(&huart2, (uint8_t *)msg, strlen(msg));
	while((transmit_done != 1) && (cpt < 200)){ // 200ms timeout
		cpt++;
		HAL_Delay(1);
	}
}

 with "msg" declared global as

char msg[512] = {0};

and "transmit_done" declared global as

volatile uint8_t transmit_done = 0;

and being modified by

void HAL_UART_TxCpltCallback(UART_HandleTypeDef *huart)  
{
	if (huart->Instance == USART2){ // Current UART
		transmit_done = 1;
	}
}

2) For reception I use these functions:

void Ringbuf_Init (void)
{	
	pBufferReadyForReception = aRXBufferA;
  pBufferReadyForUser      = aRXBufferB;
  uwNbReceivedChars        = 0;
	old_pos 				 				 = 0;
	
	memset(MainBuf, '\0', MainBuf_SIZE);
	memset(aRXBufferUser, '\0', RxBuf_SIZE);
	memset(aRXBufferA, '\0', RxBuf_SIZE);
	memset(aRXBufferB, '\0', RxBuf_SIZE);

	Data_received_available_index = 0;
	Data_processed_available_index = 0;

	HAL_UARTEx_ReceiveToIdle_DMA(&UART, aRXBufferUser, RxBuf_SIZE) == HAL_OK);
}

void HAL_UARTEx_RxEventCallback(UART_HandleTypeDef *huart, uint16_t Size)
{
  uint8_t *ptemp;
  uint8_t i;

  /* Check if number of received data in recpetion buffer has changed */
  if (Size != old_pos)
  {
    /* Check if position of index in reception buffer has simply be increased
       or if end of buffer has been reached */
    if (Size > old_pos)
    {
      /* Current position is higher than previous one */
      uwNbReceivedChars = Size - old_pos;
      /* Copy received data in "User" buffer for evacuation */
      for (i = 0; i < uwNbReceivedChars; i++)
      {
        pBufferReadyForUser[i] = aRXBufferUser[old_pos + i];
      }
    }
    else
    {
      /* Current position is lower than previous one : end of buffer has been reached */
      /* First copy data from current position till end of buffer */
      uwNbReceivedChars = RxBuf_SIZE - old_pos;
      /* Copy received data in "User" buffer for evacuation */
      for (i = 0; i < uwNbReceivedChars; i++)
      {
        pBufferReadyForUser[i] = aRXBufferUser[old_pos + i];
      }
      /* Check and continue with beginning of buffer */
      if (Size > 0)
      {
        for (i = 0; i < Size; i++)
        {
          pBufferReadyForUser[uwNbReceivedChars + i] = aRXBufferUser[i];
        }
        uwNbReceivedChars += Size;
      }
    }
    /* Process received data that has been extracted from Rx User buffer */
    UserDataTreatment(huart, pBufferReadyForUser, uwNbReceivedChars);

    /* Swap buffers for next bytes to be processed */
    ptemp = pBufferReadyForUser;
    pBufferReadyForUser = pBufferReadyForReception;
    pBufferReadyForReception = ptemp;
  }
  /* Update old_pos as new reference of position in User Rx buffer that
     indicates position to which data have been processed */
  old_pos = Size;

}

void UserDataTreatment(UART_HandleTypeDef *huart, uint8_t* pData, uint16_t Size)
{
		if (Data_received_available_index + Size >= MainBuf_SIZE)  // If Data_received_available_index + new data size is greater than the main buffer size
		{
			uint16_t nb_datatocopy = MainBuf_SIZE - Data_received_available_index;  // Find out how much space is left in the main buffer
			
			memcpy ((uint8_t *)MainBuf + Data_received_available_index, pData, nb_datatocopy);  // In that remaining space in MainBuf, copy nb data, from RxBuf + Previous_DrA_RxBuf_index (only new data received)
			memcpy ((uint8_t *)MainBuf, pData + nb_datatocopy, Size - nb_datatocopy);  // Then copy at the beginning of MainBuf the rest of data (len_RxBuf_data - nb_datatocopy), from RxBuf + Previous_DrA_RxBuf_index + nb_datatocopy
			Data_received_available_index = Size - nb_datatocopy;  // Update the position of Data_received_available_index to (len_RxBuf_data - nb_datatocopy)
		}

		else
		{
			memcpy ((uint8_t *)MainBuf + Data_received_available_index, pData, Size); // Copy new received data at the current position in MainBuf from RxBuf + Previous_DrA_RxBuf_index
			Data_received_available_index = Data_received_available_index + Size; // Update the position of Data_received_available_index to Data_received_available_index + len_RxBuf_data
		}
		
		new_data_received++; // Flag to notify app that new data is available
}

With following global variables:

uint8_t aRXBufferUser[RxBuf_SIZE]; // 512
uint8_t aRXBufferA[RxBuf_SIZE]; // 512
uint8_t aRXBufferB[RxBuf_SIZE]; // 512
uint8_t MainBuf[MainBuf_SIZE]; // 1024

volatile uint32_t uwNbReceivedChars;
uint8_t *pBufferReadyForUser;
uint8_t *pBufferReadyForReception;

volatile uint16_t old_pos = 0;
volatile uint16_t Data_received_available_index = 0;
volatile uint16_t Data_processed_available_index = 0;
volatile uint16_t Data_processed_available_index_saved = 0;

volatile uint8_t new_data_received = 0;

/* Timeout is in milliseconds */
volatile int32_t TIMEOUT = 0;

and SysTick_Handler being modified in stm32l0xx_it.c to make TIMEOUT variable decrement by 1 every ms.

void SysTick_Handler(void)
{
  /* USER CODE BEGIN SysTick_IRQn 0 */

  /* USER CODE END SysTick_IRQn 0 */
  HAL_IncTick();
  /* USER CODE BEGIN SysTick_IRQn 1 */
	TIMEOUT--;
  /* USER CODE END SysTick_IRQn 1 */
}

I also have functions for handling answers from the module:

/* Waits for a particular string to arrive in the incoming buffer... It also increments the Data_processed_available_index
 * returns 1, if the string is detected
 * return 0, in case of timeout
 */
uint8_t waitFor (char *string, uint32_t Timeout)
{
	uint16_t string_len = strlen(string); // Get string length	
	uint8_t string_found = 0;							// Flag to notify that string is found
	
	TIMEOUT = Timeout;										// Initialize TIMEOUT (decreases by one each ms (see SysTick_Handler in stm32l0xx_it.c) to Timeout (parameter of this waitFor function)	
	new_data_received = 0;								// Reset new_data_received flag
	
	uint16_t Instant_Data_received_available_index = Data_received_available_index;
	uint16_t Instant_Data_processed_available_index = Data_processed_available_index; 	

	if (((Data_processed_available_index < Instant_Data_received_available_index) && ((Instant_Data_received_available_index - Data_processed_available_index) >= string_len)) || ((Data_processed_available_index > Instant_Data_received_available_index) && ((MainBuf_SIZE - Data_processed_available_index + Instant_Data_received_available_index) >= string_len))){
		string_found = search_string_in_Mainbuf(string, &Data_processed_available_index, Instant_Data_received_available_index); // Look for string in this unprocessed data
		if(0 == string_found){															// If not found reset Data_processed counter to check again after new data received (buffer may have been full and HAL_UARTEx_RxEventCallback is trigged by Transfert Completed event, or data of interest might be preceded by idle event with data of no interest)
			Data_processed_available_index = Instant_Data_processed_available_index;
		}
	}
	
	while ((0 == string_found) && (TIMEOUT > 0)){						// While string is not found in MainBuf (no more than Timeout)
		while ((0 == new_data_received) && (TIMEOUT > 0)){		// Wait for new_data_received flag (no more than Timeout)
			HAL_Delay(1);
		}
		if(TIMEOUT > 0){																			// If Timeout not reached means new data has been received
			Instant_Data_received_available_index = Data_received_available_index;
			new_data_received = 0;															// Reset new_data_received flag
			string_found = search_string_in_Mainbuf(string, &Data_processed_available_index, Instant_Data_received_available_index); // Look for string in this new available data
			if(0 == string_found){															// If not found reset Data_processed counter to check again after new data received (buffer may have been full and HAL_UARTEx_RxEventCallback is trigged by Transfert Completed event, or data of interest might be preceded by idle event with data of no interest)
				Data_processed_available_index = Instant_Data_processed_available_index;
			}
		}
	}
	
	return string_found;
}

uint8_t search_string_in_Mainbuf(char *string, volatile uint16_t *index, uint16_t index_end)
{	
	uint16_t so_far = 0;													// Reset "so_far" variable that will be used as index for string buffer
	uint16_t string_len = strlen(string);					// Get string length
	
	while(*index != index_end){
		while ((MainBuf[*index] != string[so_far]))	// Peek in MainBuf to see if we get the first character of string
		{
			*index = *index + 1;											// While not, increment index
			if (*index == MainBuf_SIZE) *index = 0;		// If index reaches MainBuf_SIZE, index is reset
			if (*index == index_end) return 0; 				// If index_end reached, return 0
		}

		while (MainBuf[*index] == string[so_far]) 	// While character in MainBuf matches character in string
		{
			so_far++;																	// Increment index in string buffer
			*index = *index + 1;											// Increment index in MainBuf
			if (*index == MainBuf_SIZE) *index = 0;		// If index reaches MainBuf_SIZE, index is reset
			if (so_far == string_len) return 1;				// If end of string is reached, string is found in MainBuf so return 1
		}
		so_far = 0;																	// If first characters in MainBuf was matching string but not entirely, reset string index and look again for string in MainBuf
	}
	
	return 0;
}

Any comments? Hints? Recommandation?

[Karl Yamashita]

You're doing too much processing in HAL_UARTEx_RxEventCallback. Make a ring buffer queue to hold each message/packet and after each interrupt, you increment the pointer and call HAL_UARTEx_ReceiveToIdle_DMA pointing to the next queue. That way you don't have to memcpy from one buffer to another buffer. You just have HAL_UARTEx_ReceiveToIdle_DMA  write to the appropriate queue for you. Then in a main while loop you can check each queue for a new message. 

You can avoid having to monitor the Size and old_pos. Assuming you receive in packets with some idle time in between, that is. Lets say your longest packet will ever receive will be 128 bytes, when you call HAL_UARTEx_ReceiveToIdle_DMA, you pass an argument twice the size or 256. So when you get 128 bytes or less, it'll interrupt at half callback or idle interrupt. Full callback isn't used.. 

Also, i don't see where you call HAL_UARTEx_ReceiveToIdle_DMA again? So after your first interrupt, you're not going to get any more future interrupts.

You don't check HAL status when you call HAL_UART_Transmit_DMA so if HAL returns HAL_BUSY, then your transmit failed and so you may think the packet was sent successfully. So you continue sending the next packet. 

The same goes for HAL_UARTEx_ReceiveToIdle_DMA. If it returns HAL_BUSY, you don't do anything to recover. It's the same as not calling it, you're no longer going to get an interrupt.

[Thomas LB]

Hello, 
Thank you for answering my question.
1) "You're doing too much processing" current code is working 99.99% of time so I'm not sure this is the root cause here. Can you tell what is the limit you consider OK?

2) I understand your idea but I don't think this could work as I'm receiving data of unknown length, and might receive two part of a single interest moment in two part for exemple I send to the module "AT?" he answer "AT" then "OK" and I need to check if "AT OK" is receive. I'm pretty convinced that there is no other way than to copy the 2 alternatively receiving buffers to 1 big buffer on which I can process the data. But if you have a solution with new detailed constrain let me know :)

3) HAL_UARTEx_ReceiveToIdle_DMA is always on in circular mode so no need to send again when everything is OK

4) I did implement some version of checking the HAL_status like this:

uint8_t check_UART2(uint8_t timeout_ms){
	uint8_t status = 0;
	uint8_t cpt = 0;
	while ( ((HAL_UART_GetState(&huart2) != (HAL_UART_STATE_READY)) && (HAL_UART_GetState(&huart2) != (HAL_UART_STATE_BUSY_RX))) && (cpt < timeout_ms) ){
		cpt++;
		HAL_Delay(1);
	}
	if (cpt < timeout_ms){
		status = 1;
	}
	return status;
}

but I see no difference in "error" rate (<0.01% but not 0%) so I decided to remove it. My though is that I'm not part of a complexe system with several tasks, threads, etc. It's juste me in "raw C" so if I see no reason that HAL_UART_Transmit_DMA would not be ready, if I always check that previous transmission is finished.

What I call "error" is actually very strange behavior:

My entire code is frozen (timeout on uart functions don't free me) in current state until MainBuf is full 0.0

I've ready many many posts about HAL_Lock mecanism being broken but I don't see the issus In my case with latest drivers. I've seen that the idle detection mode can be "canceled" in case of error So I guess this is related to my issue but I don't see how my timeout can be inefficient in such a situation. 
Any idea?

Thank you

[Danish1]

Re "doing too much" in an ISR - essentially all the time you're in that ISR, processing or whatever, you cannot receive another character. (I suppose strictly the UART hardware can be busy receiving one character, but if your processing takes longer than that, you risk having overrun errors and losing characters).

Personally I do not like the "wait until the UART is idle" approach taken by many people on this forum for something like AT commands. Where there are events happening quickly, it is possible that a second AT command will start to be received before your idle-time has expired. For something like an AT command, which will always be terminated by <CR> or <LF>, I much prefer to note a request-to-process-up-to-this-point-in-the-buffer whenever a <CR> or <LF> comes in.

[Bob S]

See @Tilen MAJERLE github repos:

UART DMA Rx/Tx: https://github.com/MaJerle/stm32-usart-uart-dma-rx-tx (I agree with @Danish1 - get away from "rx until idle" nonsense, it just complicates things)

lwcell (lightweight cell modem AT command parsing): https://github.com/MaJerle/lwcell  -  Note that he has a couple other AT parsers, look through all his repos.

[Karl Yamashita]

Because you have HAL_UARTEx_ReceiveToIdle_DMA in circular mode, you've limited yourself to a single buffer. So when you get an interrupt, you have to figure out if it was a Half Complete callback or a Full Complete callback. Then you have to copy 1 half of the buffer into another array.

Now if you have it normal mode, you can actually have the DMA save to a buffer queue at any index. That's where a ring buffer helps. And using DMA to IDLE works excellent for your case with AT commands that send in several packets. 

You don't even have to check for a CR/LF at all. All AT commands will return some kind of reply like "\r\nOK\r\n", "\r\nERROR\r\n", etc... Those are your keywords and are pretty standard. And later you'll find out how so. But notice they have CR/LF in front of them? Now you have to write more code figuring out if the CR/LF is the start or end of the reply.

So your concern with the "AT" command and it echoes back "AT" and "OK" as separate packets. You think you have to append them into one bigger buffer inside the interrupt, but you don't. That can be done outside of the interrupt with ease. 

Take a look at this Wiki on github that i wrote. I explain how to use Half Full callback or IDLE only, and never have to worry about the Full Complete callback, nor copying from array to array. I instead just increment a index pointer and point the DMA to the next queue to save new data to. https://github.com/karlyamashita/Nucleo-G431RB_Three_UART/wiki

Once your get the ideal how DMA to IDLE is your friend, then i have another project that uses that same way of saving packets of AT command echo and reply. I then combine the packets into one message that you can parse, all outside of the interrupt. I've already uploaded the project to github but i'm still working on writing the document. 

[magene]

Are you using a STM32L7xx MCU?  If so, it looks like that family implements the character match (CM) in hardware functionality.  If you're messages are terminated with a known character, like CR or LF, you can set up a DMA (or IT probably) transfer to terminate when the termination character is received.  I use this functionality with the STM32H7A3 family and so far it has solved my issued with messages of unknown length over serial ports.

I wrote my own fairly lean FIFO queue class (actually, I found it on the web https://www.techiedelight.com/queue-implementation-using-templates-cpp/ ).  When the CM IRQ handler is called, all it does is enqueue the message and a timestamp in the FIFO and exits.  When the non-IRQ part of the code sees a queue count > 0, it dequeues the message and processes it.

[Thomas LB]

Thank you for trying to help here,

No I'm using STM32L0xx really rare bug but non-zero is too much for this application ^^

[Thomas LB]

Bug found in search_string_in_Mainbuf the second inner_loop can prevent the outer_loop from exiting so need to add the exit condition like this:

uint8_t search_string_in_Mainbuf(char *string, volatile uint16_t *index, uint16_t index_end) // Search string in Mainbuf from index to index_end (and increment index accordingly to the researched data)
{	
	uint16_t so_far = 0;												// Reset "so_far" variable that will be used as index for "string" buffer
	uint16_t string_len = strlen(string);				// Get "string" length
	
	while(*index != index_end){
		while (MainBuf[*index] != string[so_far])	// Peek in MainBuf to see if we get the first character of string
		{
			*index = *index + 1;										// While not, increment index
			if (*index == MainBuf_SIZE) *index = 0;	// If index reaches MainBuf_SIZE, index is reset
			if (*index == index_end) return 0; 			// If index_end reached, return 0
		}

		while (MainBuf[*index] == string[so_far]) // While character in MainBuf matches character in string
		{
			so_far++;																// Increment index in string buffer
			*index = *index + 1;										// Increment index in MainBuf
			if (*index == MainBuf_SIZE) *index = 0;	// If index reaches MainBuf_SIZE, index is reset
			if (so_far == string_len) return 1;			// If end of string is reached, string is found in MainBuf so return 1
			if (*index == index_end) return 0; 			// If index_end reached, return 0
		}
		so_far = 0;																// If first characters in MainBuf was matching string but not entirely, reset string index and look again for string in MainBuf
	}
	
	return 0;
}

[Tilen MAJERLE]

The correct way of receiving data with UART, in AT applications, is to use a circular buffer and rely on HT, TC and idle interrupts.

• First 2 save you from overflowing data in the rx dma buffer
• IDLE saves you from missing received data if DMA HT/TC interrupts didn't occur
• Circular buffer saves you from race condition that happens in normal DMA mode when receiving data.

AT applications have URC messages that can come asynchronously, and DMA may miss them. This is solved by circular buffer mode.

Working principle with explanation: https://github.com/MaJerle/stm32-usart-uart-dma-rx-tx