volatile variables, function problems - GCC bug??

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

Hi,

I'm fairly new to AVRs Interrupts, but have used C for a while. I've been bashing my head against this for 3 days / nights - another set of eyes would be welcome.

ATMega324P, UART0, 18.432 Mhz external. WinAVR20070525 (using eclipse / cygwin command line)

I was unable to find a UART library that supports ATMega324P / dual UARTs and compiles cleanly (no warning) with the current AVR-GCC, uses F_CPU and has commented bit value manipulation. I've therefore put together the following: (excuse the formatting, I've tried to condense a bit for posting)

#include 
#include 
#include 

#define UART0_BAUD 9600L

#define UART0_FORMAT_MODE 	( 0<<UMSEL01 | 0<<UMSEL01 ) 	// Async Mode
#define UART0_FORMAT_PARITY ( 0<<UPM01 | 0<<UPM00 ) 		// No parity
#define UART0_FORMAT_SIZE 	( 0<<UCSZ02 | 1<<UCSZ01 | 1<<UCSZ00 ) 	// 8 Bit 
#define UART0_FORMAT_STOP 	( 0<<USBS0 ) 					// 1 stop Bit 
#define UART0_FORMAT_CLK 	( 0<<UCPOL0 ) 					// Async clock
#define UART0_FORMAT ( UART0_FORMAT_MODE | UART0_FORMAT_PARITY | UART0_FORMAT_STOP | UART0_FORMAT_SIZE | UART0_FORMAT_CLK )

#define UART0_UDR		UDR0				// The actual char data
#define UART0_UBRR		UBRR0				// Baud / control byte
#define UART0_UCSRA		UCSR0A				// Control Reg A
#define UART0_UCSRB		UCSR0B				// Control Reg A
#define UART0_UCSRC		UCSR0C				// Control Reg A
#define UART0_RXEN		RXEN0				// Rx Enable Interrupt bit
#define UART0_RXCIE 	RXCIE0				// Rx Complete Interrupt bit
#define UART0_TXEN 		TXEN0				// Tx Enable Interrupt bit
#define UART0_UDRIE		UDRIE0				// Tx char complete Enable Interrupt bit
#define UART0_FE		FE0					// Frame Error bit
#define UART0_U2X		U2X0				// X2 bit Used for F_CPU < 2Mhz 

#define UART0_RXC_vect USART0_RX_vect		// Int Recieve vector
#define UART0_UDRE_vect USART0_UDRE_vect	// Int Transmit ready
#define UART0_TXFIFO_SIZE 4				// Bytes in the TX FIFO
#define UART0_RXFIFO_SIZE 4				// Bytes in the RX FIFO

void UART0_Init(void);				// Initialize UART and Flush FIFOs
void UART0_putc (uint8_t d);		// Put a byte into UART Tx FIFO
void UART0_puts(char *Ptr);			// Put a string into UART Tx FIFO
uint8_t UART0_txtest(void);			// Check number char data in UART Tx FIFO
uint8_t UART0_rxtest(void);			// Check number char data in UART Rx FIFO
uint8_t UART0_getc (void);			// Get a byte from UART Rx FIFO

typedef struct UART0_TXfifo {
	uint8_t	idx_w;
	uint8_t	idx_r;
	uint8_t	count;
	uint8_t buff[UART0_TXFIFO_SIZE];
} UART0_TXFIFO;

static volatile UART0_TXFIFO UART0_txfifo;

typedef struct UART0_RXfifo {
	uint8_t	idx_w;
	uint8_t	idx_r;
	uint8_t	count;
	uint8_t buff[UART0_RXFIFO_SIZE];
} UART0_RXFIFO;

static volatile UART0_RXFIFO UART0_rxfifo;

void UART0_Init(void) {
	UART0_txfifo.idx_r = 0;	UART0_txfifo.idx_w = 0;	UART0_txfifo.count = 0;
	UART0_rxfifo.idx_r = 0;	UART0_rxfifo.idx_w = 0;	UART0_rxfifo.count = 0;
	UART0_UBRR = (F_CPU / (16UL * UART0_BAUD)) - 1;
	UART0_UCSRC =  UART0_FORMAT;
	UART0_UCSRB = _BV(UART0_RXEN)|_BV(UART0_RXCIE)|_BV(UART0_TXEN);
	sei();
}

SIGNAL(UART0_UDRE_vect)
{
	uint8_t n, i;

	n = UART0_txfifo.count;
	if(n) {
		UART0_txfifo.count = --n;
		i = UART0_txfifo.idx_r;
		UART0_UDR = UART0_txfifo.buff[i++];
		if(i >= sizeof(UART0_txfifo.buff))
		i = 0;
		UART0_txfifo.idx_r = i;
	}

	if (n == 0) UART0_UCSRB &= ~_BV(UART0_UDRIE);  // If buffer empty, disable TX for a bit
}

void UART0_putc(uint8_t d) {
	uint8_t i;

	i = UART0_txfifo.idx_w;

	while (UART0_txfifo.count >= sizeof(UART0_txfifo.buff)); // Blocking

	UART0_txfifo.buff[i++] = d;

	cli();
	UART0_txfifo.count++;
	sei();

	UART0_UCSRB |= _BV(UART0_UDRIE) ; // Enable Tx

	if(i >= sizeof(UART0_txfifo.buff)) i = 0;
	UART0_txfifo.idx_w = i;
}

void UART0_puts(char *Ptr) {
        while (*Ptr != 0) {
                UART0_putc (*Ptr);
                Ptr++;
        }
}

uint8_t UART0_txtest(void) { return UART0_txfifo.count; }

SIGNAL(UART0_RXC_vect)
{
	uint8_t d, n, i;

	if (bit_is_clear(UART0_UCSRA, UART0_FE))
	{
		n = UART0_rxfifo.count;

		d = UART0_UDR;
		if(n < sizeof(UART0_rxfifo.buff)) {
			i = UART0_rxfifo.idx_w;
			UART0_rxfifo.buff[i++] = d;
			if(i >= sizeof(UART0_rxfifo.buff)) i = 0;
			UART0_rxfifo.idx_w = i;
			UART0_rxfifo.count = ++n;
		} // If buffer is full dump it siliently :-(
	} else {
		d = UART0_UDR;		// Dummy read of bad byte / frame error
	}
}

uint8_t UART0_rxtest(void) { return UART0_rxfifo.count; }

uint8_t UART0_getc(void) {
	uint8_t d, i;

	i = UART0_rxfifo.idx_r;
	while (UART0_rxfifo.count == 0) ; // Blocking
	d = UART0_rxfifo.buff[i++];
	cli();
	UART0_rxfifo.count--;
	sei();
	if(i >= sizeof(UART0_rxfifo.buff)) { i = 0; }
	UART0_rxfifo.idx_r = i;
	return d;
}

void Test(void) {
	uint16_t d;

	while ( (UART0_rxtest() > 0) ) {
		d=UART0_getc();
		UART0_putc(d);
	}
 }

int main (void)
{
	uint16_t d;
	
	UART0_Init();	UART0_puts("Hello World\r\n");

	for (;;) {
#if 1
		Test();
#else
		while ( (UART0_rxtest() > 0) ) {
				d=UART0_getc();
				UART0_putc(d);
		}
#endif
	}
}

// EOF

As supplied above, the AVR reboots after every few characters. Change the last #if from 1 to 0, recompile & flash - it then works fine. It is the same for 9600 and 115200 baud.

A few questions:

1) I don't think I need to worry about cli/sei inside interrupt (SIGNAL) handlers as they are atomic and can't be interrupted - is this correct?

2) I think I only need to protect the count up / down lines in the putc / getc functions (although I've tried a more generic protection - without success)

The TX functions seem fine, haven't got them to break, it is the RX function that is the problem, and my gut is something todo with the .count variable.

I'm quite happy to accept it is my programming mistake, but I don't understand why using a function makes such a difference. Memory shouldn't be an issue 2K RAM, code base should be VERY small. Any ideas please?

Thanks,

Carl
--
http://www.rvproject.gen.nz/

[Edit: Title change - learning more about the problem]

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

Quote:
1) I don't think I need to worry about cli/sei inside interrupt (SIGNAL) handlers as they are atomic and can't be interrupted - is this correct?

This is incorrect. First, SIGNAL is interruptible. You would want to use INTERRUPT instead. Second, if you compiled this without warning, then you are using a rather old version of avr-gcc as both SIGNAL and INTERRUPT have been deprecated. SIGNAL has been dropped entirely and INTERRUPT has been replaced with ISR.

Regards,
Steve A.

The Board helps those that help themselves.

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

Hi Steve,

I'm using AVR-GCC in the WINAVR 20070525 package.

$ avr-gcc -v
Using built-in specs.
Target: avr
Configured with: ../gcc-4.1.2/configure --prefix=/c/WinAVR --target=avr --enable-languages=c,c++ --w
ith-dwarf2 --enable-win32-registry=WinAVR-20070525 --disable-nls --with-gmp=/usr/local --with-mpfr=/
usr/local --enable-doc --disable-libssp
Thread model: single
gcc version 4.1.2 (WinAVR 20070525)

"avr-gcc -Wall" works fine with no warnings about use of SIGNAL. The (SIGNAL) generated asm and has

.global __vector_20
        .type   __vector_20, @function
__vector_20:
/* prologue: frame size=0 */
        push __zero_reg__
        push __tmp_reg__
        in __tmp_reg__,__SREG__
        push __tmp_reg__
        clr __zero_reg__
        push r18
        push r24
        push r25
        push r30
        push r31
/* prologue end (size=10) */

/* epilogue: frame size=0 */
        pop r31
        pop r30
        pop r25
        pop r24
        pop r18
        pop __tmp_reg__
        out __SREG__,__tmp_reg__
        pop __tmp_reg__
        pop __zero_reg__
        reti
/* epilogue end (size=10) */

Which looks like it is masking further interrupts and is identical to ISR version.

I'm keen to use latest best practice so have converted to using ISR(_vector_) instead but still have same issues.

I've started looking at the generated asm but I'm getting out of my depth.

Any more ideas why the use of a function call is causing the problems?

Carl

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

Koshchi confused SIGNAL and INTERRUPT. Using ISR is better anyway though.

Offhand I've got no idea what happens with your code, it doesn't look
completely wrong at a first glance.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

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

Hi,

OK - I've spent the another day on this - still the same problem, I've used a different UART library set and trimmed as much as possible to the bare bones. I had a typo on the original 8N1 reg config.

New version:

#include 
#include 
#include 

#define UART0_BAUD 115200L

#define UART0_FORMAT_MODE 	( 0<<UMSEL01 | 0<<UMSEL00 ) 	// Async Mode
#define UART0_FORMAT_PARITY 	( 0<<UPM01 | 0<<UPM00 ) 	// No parity
#define UART0_FORMAT_SIZE 	( 1<<UCSZ01 | 1<<UCSZ00 ) 	// 8 Bit 
#define UART0_FORMAT_STOP 	( 0<<USBS0 ) 			// 1 stop Bit 
#define UART0_FORMAT_CLK 	( 0<<UCPOL0 ) 			// Async clock
#define UART0_FORMAT ( UART0_FORMAT_MODE | UART0_FORMAT_PARITY | UART0_FORMAT_STOP | UART0_FORMAT_SIZE | UART0_FORMAT_CLK )

#define UART0_RXFIFO_SIZE 	4					// Bytes in the RX FIFO
#define UART0_RXFIFO_MASK 	(UART0_RXFIFO_SIZE-1)		// Mask (hence why size needs to be ^2)

static volatile 	uint8_t	idx_w;
static volatile 	uint8_t	idx_r;
static volatile 	uint8_t 	buff[UART0_RXFIFO_SIZE];

void UART0_Init(void);				// Initialize UART and Flush FIFOs
void UART0_putc (uint8_t d);			// Put a byte into UART Tx FIFO
uint8_t UART0_rxtest(void);			// Check number char data in UART Rx FIFO
uint8_t UART0_getc (void);			// Get a byte from UART Rx FIFO

void UART0_Init(void) {
	idx_r = 0;	idx_w = 0;
	UBRR0 = (F_CPU / (16UL * UART0_BAUD)) - 1;
	UCSR0C = UART0_FORMAT;
	UCSR0B = _BV(RXEN0)|_BV(RXCIE0)|_BV(TXEN0);
	sei();
}

// Really simple output
void UART0_putc(uint8_t d) {
	  loop_until_bit_is_set(UCSR0A, UDRE0);
	  UDR0 = d;
}

ISR(USART0_RX_vect) {
        unsigned char c;

        c = UDR0;						// Get received char
        buff[idx_w & UART0_RXFIFO_MASK] = c;	// put in buffer (no check for overflow)
        idx_w++;						// RMASK makes it unnecessary to range limit this
}

unsigned char UART0_rxtest(void) {
        return (idx_w - idx_r);
}

unsigned char UART0_getc(void) {
        unsigned char c;

        while(UART0_rxtest() == 0);			// wait for data. see also UART0_rxtest()

        c = buff[idx_r & UART0_RXFIFO_MASK];
        cli();
        idx_r++;
        sei();
        return(c);
}

__inline__ void Test1(void) {
	uint16_t d;

	while ( UART0_rxtest() > 0 ) {
		d=UART0_getc();
		UART0_putc(d);
	}
}

int main (void)
{
	UART0_Init();	UART0_putc('_');

	for (;;) {
#ifdef FAIL
		Test1();
#else
		uint16_t d;

		while ( UART0_rxtest() > 0 ) {
			d=UART0_getc();
			UART0_putc(d);
		}
#endif
	}
}

// EOF

Use a function - falls over, in the main - works fine. Even the .S asm looks 'sensible' - but not my forte.

Thanks for the pointers so far...

Carl

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

Quote:
Koshchi confused SIGNAL and INTERRUPT.

Which I believe was the reason for the switch in the first place :oops:

Regards,
Steve A.

The Board helps those that help themselves.

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

See - https://www.avrfreaks.net/index.p... for more details / fixes.