CODEWITHSUNDEEP
cfirmware
Andre Ahmed
Andre Ahmed

Reputation: 1891

Writing safe UART interrupt buffer

I'm aware of volatile keyboard and it doesn't ensure synchronization safety.

The following code is used inside an interrupt routine, and I'm using heavily the function

GetCharUART inside the main loop.

Is it safe to and stable to write code like that ? or I have to go to low level synchronization like a mutex, if that's true, how would I protect that rbuf ?

 volatile char rbuf[5][UART_BUFSIZE];
vu16 rin[5] = { 0, 0, 0, 0, 0 };
vu16 rout[5] = { 0, 0, 0, 0, 0 };
    void USART_IRQHandler(char Channel, USART_TypeDef *USARTx)
    {
        volatile unsigned int IIR;
     
        IIR = USARTx->SR;
        if (IIR & USART_FLAG_RXNE)
        {                  // read interrupt
          USARTx->SR &= ~USART_FLAG_RXNE;             // clear interrupt
     
          rbuf[Channel][rin[Channel]] = USART_ReceiveData(USARTx);
          rin[Channel]++;
          if(rin[Channel]>=UART_BUFSIZE) rin[Channel]=0;
        }
     
        if (IIR & USART_FLAG_TXE)
        {
          USARTx->SR &= ~USART_FLAG_TXE;              // clear interrupt
        }
    }   
     
     
    int GetCharUART (char Channel)
    {
      int result;
     
      if (rin[Channel]==rout[Channel]) {
            return EMPTY;
        }
     
      result=rbuf[Channel][rout[Channel]];
      rout[Channel]++;
      if(rout[Channel]>=UART_BUFSIZE) 
            rout[Channel]=0;
     
      return result;
    }

Modified code:

void USART_IRQHandler(char Channel, USART_TypeDef *USARTx)
{
    volatile unsigned int IIR;

    IIR = USARTx->SR;
    if (IIR & USART_FLAG_RXNE)
    {                  // read interrupt
      USARTx->SR &= ~USART_FLAG_RXNE;             // clear interrupt

      rbuf[Channel][rin[Channel]% UART_BUFSIZE] = USART_ReceiveData(USARTx);
      rin[Channel]++;
    }

    if (IIR & USART_FLAG_TXE)
    {
      USARTx->SR &= ~USART_FLAG_TXE;              // clear interrupt
    }
}
/******************************************************************************/
int GetCharUART (char Channel)
{
  int result;

  if (rin[Channel]==rout[Channel]) {
        return EMPTY;
    }

  result=rbuf[Channel][rout[Channel]% UART_BUFSIZE];
  rout[Channel]++;

  return result;
}

Upvotes: 0

Views: 982

Answers (2)

Tyler Kropp
Tyler Kropp

Reputation: 573

Rather than trying to make data structures that would work synchronously work with interrupts, it's best to use a different data structure that actually is safe for use in interrupts.

It is common for circular/ring buffers to be used for data structures that need to be shared between regular code and interrupt code.

https://en.wikipedia.org/wiki/Circular_buffer

A circular buffer is implemented with two indices, and each side is only modifying one of the indices, which has a sort of reader/writer style. This allows it to be pushed onto and popped off by concurrent executions.

Because of the dual indices and the fact the indices are always incremented in one direction, the worst case that could happen is that the interrupt dropped a byte when the buffer is almost full because it didn't pick up on a recent change to the other index, which is not a big deal.

If you make your own implementation, remember to test it, because it can be easy to mess up.

Upvotes: 0

4386427
4386427

Reputation: 44329

Your code has a functional bug.

In the ISR you don't check for a "buffer full" condition. The code just increments rin[Channel] without looking at rout[Channel]. So a whole buffer of data can be lost.

Example: If rout[Channel] equals zero and rin[Channel] equals UART_BUFSIZE-1 then the ISR will set rin[Channel] to zero. In other words the buffer will now appear empty and data is lost.

So the first step is to fix the code.

Upvotes: 1

Related Questions