Circular uart buffer freezes

Go To Last Post
23 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello,

I wrote a simple uart library not using any ISR's and got it working well with printf. I then started working on a a more optimized ISR version but it has some strange errors I cannot solve.

ISR(USART0_RX_vect)
{

	// Store new index
  RxHead = (RxHead + 1) % UART_RX_BUFFER_SIZE;

	// Check for transmission errors
	RxError = ( UCSR0A & (_BV(FE0)|_BV(DOR0)) );
  if(RxError) {
		// If transmission error do not store wonky data
		// Set the approreate RxError 
	}

	// Check if buffer is full
  else if( RxHead == RxTail ) {
  	// RxBuf is full! If we want to overwrite then 
		// inc tail so we dont grab new then old data!
		RxTail = ( RxTail + 1) % UART_RX_BUFFER_SIZE;
		RxError = UART_BUFFER_OVERFLOW;
  }

	// If no probelms then insert new data into RxBuf
	else {
		// Put recieved char into buffer
		RxBuf[RxHead] = UDR0;
		// Everything OK!
  	RxError = UART_OK;  
	}
};

This code when I write past the head the program freaks out. The for(;;) loop in my main() stops running and the if( RxHead == RxTail ) block runs infinity.

I am going to wrap that overflow check with #ifdef's so I can choose between allowing overflow and stopping the storage of the new char and raising a flag. I can get the desired code to run allowing overflow while increasing the tail so my get() function always returns the oldest data but when trying to block the write everything freezes.

Also which buffer loop is less cpu intensive & memory using? I'm not sure how the mod is derived in hardware.

RxHead = (RxHead + 1) % UART_RX_BUFFER_SIZE;

OR

if( (RxHead + 1) >= UART_RX_BUFFER_SIZE )
     RxHead = 0;

My head hurts and I'm out of ideas, can a fresh pair of eyes see what I did wrong? Thanks!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
This code when I write past the head the program freaks out. The for(;; ) loop in my main() stops running and the if( RxHead == RxTail ) block runs infinity.
Because you don't read UDR0 in that execution path in the ISR (also not in case of RxError), so the interrupt triggers again and again.

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

sternst wrote:
Quote:
This code when I write past the head the program freaks out. The for(;; ) loop in my main() stops running and the if( RxHead == RxTail ) block runs infinity.
Because you don't read UDR0 in that execution path in the ISR (also not in case of RxError), so the interrupt triggers again and again.

I also face to this problem, Could you show more clearly how to solve it. Thanks

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Without seeing your code, how can we solve it? Are the shared variables declared 'volatile'?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

Could you show more clearly how to solve it.

char Rxchar = UDR0;

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Sorry. My program have several module. Here's code of usart module

usart.c


  

#include 
#include 
#include 
#include "uartlib.h"
unsigned long time_out=0;

/*
 *  constants and macros
 */

/* size of RX/TX buffers */
#define UART_RX_BUFFER_MASK ( UART_RX_BUFFER_SIZE - 1)
#define UART_TX_BUFFER_MASK ( UART_TX_BUFFER_SIZE - 1)

#if ( UART_RX_BUFFER_SIZE & UART_RX_BUFFER_MASK )
#error RX buffer size is not a power of 2
#endif
#if ( UART_TX_BUFFER_SIZE & UART_TX_BUFFER_MASK )
#error TX buffer size is not a power of 2
#endif

#if defined(__AVR_AT90S2313__) \
 || defined(__AVR_AT90S4414__) || defined(__AVR_AT90S4434__) \
 || defined(__AVR_AT90S8515__) || defined(__AVR_AT90S8535__) \
 || defined(__AVR_ATmega103__)
 /* old AVR classic or ATmega103 with one UART */
 #define AT90_UART
 #define UART0_RECEIVE_INTERRUPT   UART_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  UART_UDRE_vect
 #define UART0_STATUS   USR
 #define UART0_CONTROL  UCR
 #define UART0_DATA     UDR
 #define UART0_UDRIE    UDRIE
#elif defined(__AVR_AT90S2333__) || defined(__AVR_AT90S4433__)
 /* old AVR classic with one UART */
 #define AT90_UART
 #define UART0_RECEIVE_INTERRUPT   UART_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  UART_UDRE_vect
 #define UART0_STATUS   UCSRA
 #define UART0_CONTROL  UCSRB
 #define UART0_DATA     UDR
 #define UART0_UDRIE    UDRIE
#elif  defined(__AVR_ATmega8__)  || defined(__AVR_ATmega16__) || defined(__AVR_ATmega32__) \
  || defined(__AVR_ATmega323__)
  /* ATmega with one USART */
 #define ATMEGA_USART
 #define UART0_RECEIVE_INTERRUPT   USART_RXC_vect
 #define UART0_TRANSMIT_INTERRUPT  USART_UDRE_vect
 #define UART0_STATUS   UCSRA
 #define UART0_CONTROL  UCSRB
 #define UART0_DATA     UDR
 #define UART0_UDRIE    UDRIE
#elif  defined(__AVR_ATmega8515__) || defined(__AVR_ATmega8535__)
  /* ATmega with one USART */
 #define ATMEGA_USART
 #define UART0_RECEIVE_INTERRUPT   USART_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  USART_UDRE_vect
 #define UART0_STATUS   UCSRA
 #define UART0_CONTROL  UCSRB
 #define UART0_DATA     UDR
 #define UART0_UDRIE    UDRIE
#elif defined(__AVR_ATmega163__)
  /* ATmega163 with one UART */
 #define ATMEGA_UART
 #define UART0_RECEIVE_INTERRUPT   UART_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  UART_UDRE_vect
 #define UART0_STATUS   UCSRA
 #define UART0_CONTROL  UCSRB
 #define UART0_DATA     UDR
 #define UART0_UDRIE    UDRIE
#elif defined(__AVR_ATmega162__)
 /* ATmega with two USART */
 #define ATMEGA_USART0
 #define ATMEGA_USART1
 #define UART0_RECEIVE_INTERRUPT   USART0_RXC_vect
 #define UART1_RECEIVE_INTERRUPT   USART1_RXC_vect
 #define UART0_TRANSMIT_INTERRUPT  USART0_UDRE_vect
 #define UART1_TRANSMIT_INTERRUPT  USART1_UDRE_vect
 #define UART0_STATUS   UCSR0A
 #define UART0_CONTROL  UCSR0B
 #define UART0_DATA     UDR0
 #define UART0_UDRIE    UDRIE0
 #define UART1_STATUS   UCSR1A
 #define UART1_CONTROL  UCSR1B
 #define UART1_DATA     UDR1
 #define UART1_UDRIE    UDRIE1
#elif defined(__AVR_ATmega64__) || defined(__AVR_ATmega128__)
 /* ATmega with two USART */
 #define ATMEGA_USART0
 #define ATMEGA_USART1
 #define UART0_RECEIVE_INTERRUPT   USART0_RX_vect
 #define UART1_RECEIVE_INTERRUPT   USART1_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  USART0_UDRE_vect
 #define UART1_TRANSMIT_INTERRUPT  USART1_UDRE_vect
 #define UART0_STATUS   UCSR0A
 #define UART0_CONTROL  UCSR0B
 #define UART0_DATA     UDR0
 #define UART0_UDRIE    UDRIE0
 #define UART1_STATUS   UCSR1A
 #define UART1_CONTROL  UCSR1B
 #define UART1_DATA     UDR1
 #define UART1_UDRIE    UDRIE1
#elif defined(__AVR_ATmega161__)
 /* ATmega with UART */
 #error "AVR ATmega161 currently not supported by this libaray !"
#elif defined(__AVR_ATmega169__)
 /* ATmega with one USART */
 #define ATMEGA_USART
 #define UART0_RECEIVE_INTERRUPT   USART0_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  USART0_UDRE_vect
 #define UART0_STATUS   UCSRA
 #define UART0_CONTROL  UCSRB
 #define UART0_DATA     UDR
 #define UART0_UDRIE    UDRIE
#elif defined(__AVR_ATmega48__) ||defined(__AVR_ATmega88__) || defined(__AVR_ATmega168__) || \
      defined(__AVR_ATmega48P__) ||defined(__AVR_ATmega88P__) || defined(__AVR_ATmega168P__) || \
      defined(__AVR_ATmega328P__)
 /* TLS-Added 48P/88P/168P/328P */
 /* ATmega with one USART */
 #define ATMEGA_USART0
 #define UART0_RECEIVE_INTERRUPT   USART_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  USART_UDRE_vect
 #define UART0_STATUS   UCSR0A
 #define UART0_CONTROL  UCSR0B
 #define UART0_DATA     UDR0
 #define UART0_UDRIE    UDRIE0
#elif defined(__AVR_ATtiny2313__)
 #define ATMEGA_USART
 #define UART0_RECEIVE_INTERRUPT   USART_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  USART_UDRE_vect
 #define UART0_STATUS   UCSRA
 #define UART0_CONTROL  UCSRB
 #define UART0_DATA     UDR
 #define UART0_UDRIE    UDRIE
#elif defined(__AVR_ATmega329__) ||\
      defined(__AVR_ATmega649__) ||\
      defined(__AVR_ATmega325__) ||defined(__AVR_ATmega3250__) ||\
      defined(__AVR_ATmega645__) ||defined(__AVR_ATmega6450__)
  /* ATmega with one USART */
  #define ATMEGA_USART0
  #define UART0_RECEIVE_INTERRUPT   USART0_RX_vect
  #define UART0_TRANSMIT_INTERRUPT  USART0_UDRE_vect
  #define UART0_STATUS   UCSR0A
  #define UART0_CONTROL  UCSR0B
  #define UART0_DATA     UDR0
  #define UART0_UDRIE    UDRIE0
#elif defined(__AVR_ATmega3290__) ||\
      defined(__AVR_ATmega6490__) ||
  /* TLS-Separated these two from the previous group because of inconsistency in the USART_RX */
  /* ATmega with one USART */
  #define ATMEGA_USART0
  #define UART0_RECEIVE_INTERRUPT   USART_RX_vect
  #define UART0_TRANSMIT_INTERRUPT  USART0_UDRE_vect
  #define UART0_STATUS   UCSR0A
  #define UART0_CONTROL  UCSR0B
  #define UART0_DATA     UDR0
  #define UART0_UDRIE    UDRIE0
#elif defined(__AVR_ATmega2560__) || defined(__AVR_ATmega1280__) || defined(__AVR_ATmega640__)
/* ATmega with two USART */
  #define ATMEGA_USART0
  #define ATMEGA_USART1
  #define UART0_RECEIVE_INTERRUPT   USART0_RX_vect
  #define UART1_RECEIVE_INTERRUPT   USART0_UDRE_vect
  #define UART0_TRANSMIT_INTERRUPT  USART1_RX_vect
  #define UART1_TRANSMIT_INTERRUPT  USART1_UDRE_vect
  #define UART0_STATUS   UCSR0A
  #define UART0_CONTROL  UCSR0B
  #define UART0_DATA     UDR0
  #define UART0_UDRIE    UDRIE0
  #define UART1_STATUS   UCSR1A
  #define UART1_CONTROL  UCSR1B
  #define UART1_DATA     UDR1
  #define UART1_UDRIE    UDRIE1
#elif defined(__AVR_ATmega644__)
 /* ATmega with one USART */
 #define ATMEGA_USART0
 #define UART0_RECEIVE_INTERRUPT   USART0_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  USART0_UDRE_vect
 #define UART0_STATUS   UCSR0A
 #define UART0_CONTROL  UCSR0B
 #define UART0_DATA     UDR0
 #define UART0_UDRIE    UDRIE0
#elif defined(__AVR_ATmega164P__) || defined(__AVR_ATmega324P__) || defined(__AVR_ATmega644P__)
 /* ATmega with two USART */
 #define ATMEGA_USART0
 #define ATMEGA_USART1
 #define UART0_RECEIVE_INTERRUPT   USART0_RX_vect
 #define UART1_RECEIVE_INTERRUPT   USART0_UDRE_vect
 #define UART0_TRANSMIT_INTERRUPT  USART1_RX_vect
 #define UART1_TRANSMIT_INTERRUPT  USART1_UDRE_vect
 #define UART0_STATUS   UCSR0A
 #define UART0_CONTROL  UCSR0B
 #define UART0_DATA     UDR0
 #define UART0_UDRIE    UDRIE0
 #define UART1_STATUS   UCSR1A
 #define UART1_CONTROL  UCSR1B
 #define UART1_DATA     UDR1
 #define UART1_UDRIE    UDRIE1
#else
 #error "no UART definition for MCU available"
#endif


/*
 *  module global variables
 */
static volatile unsigned char UART_TxBuf[UART_TX_BUFFER_SIZE];
static volatile unsigned char UART_RxBuf[UART_RX_BUFFER_SIZE];
static volatile unsigned char UART_TxHead;
static volatile unsigned char UART_TxTail;
static volatile unsigned char UART_RxHead;
static volatile unsigned char UART_RxTail;
static volatile unsigned char UART_LastRxError;



ISR(UART0_RECEIVE_INTERRUPT)
/*************************************************************************
Function: UART Receive Complete interrupt
Purpose:  called when the UART has received a character
**************************************************************************/
{
    unsigned char tmphead;
    unsigned char data;
    unsigned char usr;
    unsigned char lastRxError;


    /* read UART status register and UART data register */
    usr  = UART0_STATUS;
    data = UART0_DATA;


    /* */
#if defined( AT90_UART )
    lastRxError = (usr & (_BV(FE)|_BV(DOR)) );
#elif defined( ATMEGA_USART )
    lastRxError = (usr & (_BV(FE)|_BV(DOR)) );
#elif defined( ATMEGA_USART0 )
    lastRxError = (usr & (_BV(FE0)|_BV(DOR0)) );
#elif defined ( ATMEGA_UART )
    lastRxError = (usr & (_BV(FE)|_BV(DOR)) );
#endif

    /* calculate buffer index */
    tmphead = ( UART_RxHead + 1) & UART_RX_BUFFER_MASK;
    UART_RxHead = tmphead;
    if ( tmphead == UART_RxTail )
    {
    	/* error: receive buffer overflow */
        lastRxError = UART_BUFFER_OVERFLOW >> 8;
    }
    UART_RxBuf[tmphead] = data;
    UART_LastRxError = lastRxError;
}


ISR(UART0_TRANSMIT_INTERRUPT)
/*************************************************************************
Function: UART Data Register Empty interrupt
Purpose:  called when the UART is ready to transmit the next byte
**************************************************************************/
{
    unsigned char tmptail;
    if ( UART_TxHead != UART_TxTail) {
        /* calculate and store new buffer index */
        tmptail = (UART_TxTail + 1) & UART_TX_BUFFER_MASK;
        UART_TxTail = tmptail;
        /* get one byte from buffer and write it to UART */
        UART0_DATA = UART_TxBuf[tmptail];  /* start transmission */
    }else{
        /* tx buffer empty, disable UDRE interrupt */
        UART0_CONTROL &= ~_BV(UART0_UDRIE);
    	//UART0_DATA =0;
    }
}


/*************************************************************************
Function: uart_init()
Purpose:  initialize UART and set baudrate
Input:    baudrate using macro UART_BAUD_SELECT()
Returns:  none
**************************************************************************/
void uart_init(unsigned int baudrate)
{
    UART_TxHead = 0;
    UART_TxTail = 0;
    UART_RxHead = 0;
    UART_RxTail = 0;

#if defined( AT90_UART )
    /* set baud rate */
    UBRR = (unsigned char)baudrate;

    /* enable UART receiver and transmmitter and receive complete interrupt */
    UART0_CONTROL = _BV(RXCIE)|_BV(RXEN)|_BV(TXEN);

#elif defined (ATMEGA_USART)
    /* Set baud rate */
    if ( baudrate & 0x8000 )
    {
    	 UART0_STATUS = (1<<U2X);  //Enable 2x speed
    	 baudrate &= ~0x8000;
    }
    UBRRH = (unsigned char)(baudrate>>8);
    UBRRL = (unsigned char) baudrate;

    /* Enable USART receiver and transmitter and receive complete interrupt */
    UART0_CONTROL = _BV(RXCIE)|(1<<RXEN)|(1<<TXEN);

    /* Set frame format: asynchronous, 8data, no parity, 1stop bit */
    #ifdef URSEL
    UCSRC = (1<<URSEL)|(3<<UCSZ0);
    #else
    UCSRC = (3<<UCSZ0);
    #endif

#elif defined (ATMEGA_USART0 )
    /* Set baud rate */
    if ( baudrate & 0x8000 )
    {
   		UART0_STATUS = (1<<U2X0);  //Enable 2x speed
   		baudrate &= ~0x8000;
   	}
    UBRR0H = (unsigned char)(baudrate>>8);
    UBRR0L = (unsigned char) baudrate;

    /* Enable USART receiver and transmitter and receive complete interrupt */
    UART0_CONTROL = _BV(RXCIE0)|(1<<RXEN0)|(1<<TXEN0);

    /* Set frame format: asynchronous, 8data, no parity, 1stop bit */
    #ifdef URSEL0
    UCSR0C = (1<<URSEL0)|(3<<UCSZ00);
    #else
    UCSR0C = (3<<UCSZ00);
    #endif

#elif defined ( ATMEGA_UART )
    /* set baud rate */
    if ( baudrate & 0x8000 )
    {
    	UART0_STATUS = (1<<U2X);  //Enable 2x speed
    	baudrate &= ~0x8000;
    }
    UBRRHI = (unsigned char)(baudrate>>8);
    UBRR   = (unsigned char) baudrate;

    /* Enable UART receiver and transmitter and receive complete interrupt */
    UART0_CONTROL = _BV(RXCIE)|(1<<RXEN)|(1<<TXEN);

#endif

}/* uart_init */


/*************************************************************************
Function: uart_getc()
Purpose:  return byte from ringbuffer
Returns:  lower byte:  received byte from ringbuffer
          higher byte: last receive error
**************************************************************************/
unsigned int uart_getc(void)
{
    unsigned char tmptail;
    unsigned char data;

    if ( UART_RxHead == UART_RxTail ) {
        return UART_NO_DATA;   /* no data available */
    }

    /* calculate /store buffer index */
    tmptail = (UART_RxTail + 1) & UART_RX_BUFFER_MASK;
    UART_RxTail = tmptail;

    /* get data from receive buffer */
    data = UART_RxBuf[tmptail];

    return (UART_LastRxError << 8) + data;

}/* uart_getc */


/*************************************************************************
Function: uart_putc()
Purpose:  write byte to ringbuffer for transmitting via UART
Input:    byte to be transmitted
Returns:  none
**************************************************************************/
void uart_putc(unsigned char data)
{
    unsigned char tmphead;


    tmphead  = (UART_TxHead + 1) & UART_TX_BUFFER_MASK;
    while ( tmphead == UART_TxTail )
    {
      //  ;/* wait for free space in buffer */
    }
    UART_TxBuf[tmphead] = data;
    UART_TxHead = tmphead;

    /* enable UDRE interrupt */
    UART0_CONTROL    |= _BV(UART0_UDRIE);



}/* uart_putc */


/*************************************************************************
Function: uart_puts()
Purpose:  transmit string to UART
Input:    string to be transmitted
Returns:  none
**************************************************************************/
void uart_puts(const char *s )
{
    while (*s)
      uart_putc(*s++);

}/* uart_puts */


/*************************************************************************
Function: uart_puts_p()
Purpose:  transmit string from program memory to UART
Input:    program memory string to be transmitted
Returns:  none
**************************************************************************/
void uart_puts_p(const char *progmem_s )
{
    register char c;

    while ( (c = pgm_read_byte(progmem_s++)) )
      uart_putc(c);

}/* uart_puts_p */



/*************************************************************************
Function: uart_available()
Purpose:  Determine the number of bytes waiting in the receive buffer
Input:    None
Returns:  Integer number of bytes in the receive buffer
**************************************************************************/
int uart_available(void)
{
        return (UART_RX_BUFFER_MASK + UART_RxHead - UART_RxTail) % UART_RX_BUFFER_MASK;
}/* uart_available */



/*************************************************************************
Function: uart_flush()
Purpose:  Flush bytes waiting the receive buffer.  Acutally ignores them.
Input:    None
Returns:  None
**************************************************************************/
void uart_flush(void)
{
        UART_RxHead = UART_RxTail;
}/* uart_flush */



  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

But that code does NOT suffer from what's being described above? The receive interrupt always does:

    usr  = UART0_STATUS;
    data = UART0_DATA;

that is the correct procedure. I'm not entirely surprised, Petere Fleury certainly knows how to write a UART library.

In fact Peter Fleury's code (you deleted the copyright notice and GPL license by the way!) is so good that it always just works. If it doesn't work for you then it's almost certainly FAQ#3 at play.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Surely you just use a respected library as-is.

If you find a problem with the 'official' release of that library, you devise some test routines. Publish your test code with a link to the 'release version' of the library.

Fellow readers can help to find a solution, and often the original author joins in too!

Of course, most code has always got possible improvements. So publish your suggestions with suitable test code. Your name gains worldwide admiration.

It is always better to post a link to the original library source. And for you to publish your 'diff'.
Pasting vast reams of modified code with no mention of your modifications is not helpful. Why should I have to find the original and 'diff' it myself?

If you are claiming some dramatic increase in performance, I might be interested to investigate.

If you have simply broken a working library, the solution is to use the original version. You can do the "diff broken.c original.c" for yourself.

This is an excellent exercise for you. Note that you can also do "diff brokenfiles... original_distribution_directory"

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

diff broken.c original.c

I just tried that. It is a REAL shame that OP deleted the comment block at the top of the file as this contained the version number. But as far as I can determine the copy of the code on Fleury's website is a much newer version with a some additional support for other AVRs and some bug fixes. For example this (under 164/324/644):

 #define UART1_RECEIVE_INTERRUPT   USART0_UDRE_vect
 #define UART0_TRANSMIT_INTERRUPT  USART1_RX_vect

is now fixed as:

 #define UART1_RECEIVE_INTERRUPT   USART1_RX_vect
 #define UART0_TRANSMIT_INTERRUPT  USART0_UDRE_vect

(the 0's and 1's and RX/UDRE were the wrong way round!). That alone would seem to me to be reason enough to ditch the code being used and take the latest from the Fleury website:

http://homepage.hispeed.ch/peter...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

My head hurts and I'm out of ideas, can a fresh pair of eyes see what I did wrong?

Are interrupts temporary disabled while the main program is accessing the buffer?

All explained in the atomic section of the manual.
http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
Are interrupts temporary disabled while the main program is accessing the buffer?
This was my thought as well.

I haven't read the code but you have to assume that the write pointer/index used for storing chars changes while you are reading from the buffer. If your input isr also caters for queue wrapping then you have to assume that the output pointer/index changes while you are reading chars.

neither of these situations is complex but some thought is needed.

regards
Greg

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
Surely you just use a respected library as-is.

Cliff said:
Quote:

(the 0's and 1's and RX/UDRE were the wrong way round!). That alone would seem to me to be reason enough to ditch the code being used and take the latest from the Fleury website:

http://homepage.hispeed.ch/peter... ... tware.html

It really is wise to use a proven library. As Cliff has discovered, occasionally even the 'experts' make mistakes. But if you were to quote your experience with the associated links, others can help.

Cliff may have diff'ed your code. I will not unless you provide the links.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'm now using some spinoff from this original library on ATMega1284, in which i added  snd_available functions as I'm using both usarts simultaneously and transmitting more data in packets.

 

/*************************************************************************
Function: uart0_snd_available()
Purpose:  Determine the number of bytes free in send buffer
Input:    None
Returns:  Integer number of free bytes in the send buffer
**************************************************************************/
uint8_t uart0_snd_available(void)
{
    if(UART_TxHead==UART_TxTail){
        return UART_TX0_BUFFER_SIZE;
    }
    if(((UART_TxHead+1) & UART_TX0_BUFFER_MASK)==UART_TxTail){
        return 0;
    }
    if(UART_TxHead<UART_TxTail){
        return UART_TX0_BUFFER_SIZE - UART_TxTail + UART_TxHead;
    }else{
        return UART_TX0_BUFFER_SIZE - UART_TxHead + UART_TxTail;
    }
} /* uart0_snd_available */

 

Today I stumbled upon similar issue when reducing buffer sizes.  Mentioned loop causes deadlock on the system - at least this is prime suspect.

 

My suspicion is that because i use both usarts simultaneously, and timer to trigger some events, ATOMIC_BLOCK becomes too general disabling ALL interrrupts.

 

I probabbly need to rework locks so that they affect only local interrupts that might actually change those values or introduce flags to cope with this in more transparent ways.

 

Unless there is ready library that works exactly as mentioned :)

Intelligence is not everything, wisdom is more than that.

Last Edited: Sat. Nov 30, 2019 - 07:28 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Actually i thought about sth like this on volatile flag , i'll test it.

 

#define FLAG_LOCK_WAIT(flag) do{ do{{_asm nop _endasm}}while(flag==1); flag=1; } while (0)
#define FLAG_UNLOCK(flag) flag=0

 

That is actually WRONG - would work in multithreaded environment ;)

 

I probably should allocate temporary buffer that i can read to from interrupt level, if main buffer is unavailable not to create colission, and then include that buffer in aftercheck from main thread.

 

It is tricky :D

Intelligence is not everything, wisdom is more than that.

Last Edited: Sat. Nov 30, 2019 - 08:10 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Instead of going from library to library until you find something that works, just do it yourself. Its not that hard, and in the process you get an understanding of what is needed, what works best for you, what can be changed, how to change, etc. There may be libraries that are preferable to just use because of complexity, but a buffer is not really one of them and is simple enough to do on your own. When you start doing the simple yourself, you get a better idea of what kind of problems you can run into, how to think about these problems and come up with solutions, and when it comes time to using other libraries you have a head start on using and understanding them. 

 

This is a c translation-

https://godbolt.org/z/2z_apN

of this c++ buffer I created for myself-

https://github.com/cv007/Avr0PlusPlus/blob/master/Buffer.hpp

 

There is really not much code, and you should be able to follow it and understand what is happening. Then you can do something similar, or take the ideas and make something better.

 

Using a count and pointers to the buffer seem to work better than using indexes (imho), but either way works. I also interrupt protect everything that could be interrupted and cause problems. which also means any interrupt code may unnecessarily go through the sreg save/restore, or may be protecting an 8bit access which does not need protecting (until you change it to a 16bit, then forget it now needs protection). So what. The advantage is you no longer have to figure out when you do or don't have to protect any buffer functions/data.

 

Last Edited: Sat. Nov 30, 2019 - 08:45 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What is the strategy when the send buffer is full? Does adding more chars block or does it overwrite?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

>Does adding more chars block or does it overwrite?

 

You do not overwrite the buffer when appending/sending. The caller has to check the return value when appending to the buffer, and if was full (returned false) will have to try again. Its up to the caller to figure these things out, and it becomes blocking at that point if you put yourself in a loop waiting to get a byte into the buffer.

 

On the receive side, if you have a rx isr adding to a buffer, then either choice is bad but one of them has to be made. I prefer to just drop the latest incoming byte and set an overflow flag so whoever uses the buffer knows there will be some missing data. The other option I think is worse- overwrite the oldest, then you just corrupted any good data that was being processed. Maybe there are better choices when not using fixed size buffers (such as heap allocated buffers), but I imagine you still have decisions to make that are the same (allocation can fail).

 

 

Last Edited: Sat. Nov 30, 2019 - 09:00 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Two ways to handle rx buffer full, either hw (CTS) or sw (xon/xoff) handshaking. Either should signal the sender to stop before the buffer fills up!

 

jim

 

Click Link: Get Free Stock: Retire early! PM for strategy

share.robinhood.com/jamesc3274
get $5 free gold/silver https://www.onegold.com/join/713...

 

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Actually solved my problem in other way - Code works properly without atomic locs as it is all 8 bit and i rearranged it so pointer move memory write is at the end. Real culprit was : unknown really ...TADA !

 

I had debug code in interrupt (every 10 ms) causing sending large (once in a second after timeout - so not sure why it affected first write... and contents were larger than buffer) packet to debug usart, so.. it started looping in ISR waiting for buffer to become available.After talking to my dad/friends i suspected stack overflow but i was not sure so I even put a flag to isr call so that once it is stuck in logic block it will just skip it and release call, but it was not enough (I'm not sure why though).

 

Only when i moved all heavier logics and usart transfer to main loop triggered by flag from isr timer, then everything unfroze.

 

both buffers 64B [tx==rx]

 

previous outcome on usart1 :

[frame baked]
[FRAME from <0x1234> to <0x0001> port 0 PN = 0 frame_index = 0 size = 16 usart avail : 64 to transmi

 

proper output on usart1 :

[frame baked]
[FRAME from <0x1234> to <0x0001> port 0 PN = 0 frame_index = 0 size = 16 usart avail : 64 to transmit : 16]
[handle_frame_event - sent timeouted frame from <0x1234> to <0x0001> port 0 PN = 0 frame_index = 16 size = 16 D0 :  SYS : 1 ACK : 1]

 

proper output on usart 0

AA 01 00 34 12 1B 00 00 00 00 00 00 00 00 00 92

 

I'm not sure it was overflow, or what it actually was.. but now everything works as intended and debug suggests that buffer is indeed full and blocking safeguards are working.

 

All usarts have full isr drivers, the same goes for timer flags and one wire handling. Maybe when i'll advance more i might find out exactly what happened.

Intelligence is not everything, wisdom is more than that.

Last Edited: Sun. Dec 1, 2019 - 07:43 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

bajtec wrote:
Today I stumbled upon similar issue when reducing buffer sizes.

    if(((UART_TxHead+1) & UART_TX0_BUFFER_MASK)==UART_TxTail){
        return 0;
    }

I hope your reduction was still a power of two or the above code will fail.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

N.Winterbottom wrote:

bajtec wrote:

Today I stumbled upon similar issue when reducing buffer sizes.

 

    if(((UART_TxHead+1) & UART_TX0_BUFFER_MASK)==UART_TxTail){
        return 0;
    }

I hope your reduction was still a power of two or the above code will fail.

 

 

Library has checks against it ;)

 


#if (UART_RX0_BUFFER_SIZE & UART_RX0_BUFFER_MASK)
	#error RX0 buffer size is not a power of 2
#endif
#if (UART_TX0_BUFFER_SIZE & UART_TX0_BUFFER_MASK)
	#error TX0 buffer size is not a power of 2
#endif

#if (UART_RX1_BUFFER_SIZE & UART_RX1_BUFFER_MASK)
	#error RX1 buffer size is not a power of 2
#endif
#if (UART_TX1_BUFFER_SIZE & UART_TX1_BUFFER_MASK)
	#error TX1 buffer size is not a power of 2
#endif

#if (UART_RX2_BUFFER_SIZE & UART_RX2_BUFFER_MASK)
	#error RX2 buffer size is not a power of 2
#endif
#if (UART_TX2_BUFFER_SIZE & UART_TX2_BUFFER_MASK)
	#error TX2 buffer size is not a power of 2
#endif

 

Intelligence is not everything, wisdom is more than that.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

>Real culprit was : unknown

 

That is never a good place to be, but it happens.

 

 

I don't know if you are using the code previously in the thread and something like-

  https://github.com/alx741/avr_uart/blob/master/uart.c

but uart_putc is blocking-

while ( tmphead == UART_TxTail )

so if you use uart_putc or anything that uses it like uart_puts, in an interrupt, you are stuck there when the buffer is full as UART_TxTail is not going to change until the usart tx isr runs, and that is not going to run because you are waiting around in an interrupt and interrupts are currently disabled.

 

>I had debug code in interrupt (every 10 ms)

 

So its not too hard to imagine if you were sending debug info out the uart (via buffer), you could get stuck there on the uart_putc.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@curtvm - yes it happens :) As you rightfully pointed out there was atomic block on that part (which is not on library you posted, I actually reworked it a bit stripping 16 bit buffers and atomic blocks making sure pointer move is always at the end of writing so that it is ISR safe AND non blocking while using single core and one IRS/ buffer. i made it strictly avr 8bit related so no 16 bit calculation can extend processing times) but still, even if i had not disabled interrupts it was freezing. I even added security flag on IRS so that it quit almost immediately if previous IRS execution has not finished.

 

Code I'm writing is proprietary and quite large atm so pasting it would be bad for those two reasons, but if i ever find out what was wrong actually, then i'll post it here :)

Intelligence is not everything, wisdom is more than that.