How to stop sending the last received char of UART echo function?

Question

I developed a code for uart based ISR with linear buffers, the problem is that it keep sending the last received char in Arduino IDE.

Here's the application code:

#include "uart_new.h"

void setup() {
uart_init_new(103);
DDRB = 0xff;
}

void loop() {
uint16_t c;
c = get_char();
if (c & NO_DATA)
{
  PORTB |= (1<<PB5);
  _delay_ms(500);
  PORTB &= ~(1<<PB5);
  _delay_ms(500);
}
else
{
  put_char(c);
}
}

The header file:

#ifndef uart_new_H_
#define uart_new_H_

#define NO_DATA     0x0100
#define Buf_OF      0x0200
#define FR_Er       0x0400
#define DOR_Er      0x0800
#define PARTY_Er    0x1000

#define buf_s 32
#define buf_mask Buf_s - 1
#define baud_rate 9600

void uart_init_new(uint16_t ubrr);
void put_char(uint8_t data);
uint16_t get_char(void);
void put_s(uint8_t *str);

#endif // uart_new_H_

The source code:

#include <avr/io.h>
#include <Arduino.h>
#include "uart_new.h"

/*******************************************
If you want to prevent reading a string
backwards then you need FIFO implementation
*******************************************/

static volatile uint8_t rx_buf[32],tx_buf[32];
static volatile uint8_t rx_p,tx_p,rx_cnt,tx_cnt;
static volatile uint8_t rx_errors,tx_sBuf,rx_sBuf;

void uart_init_new(uint16_t ubrr)
{
    rx_p=0,tx_p=0,rx_cnt=0,tx_cnt=0;
    UBRR0H = (uint8_t)(ubrr>>8);
    UBRR0L = (uint8_t)ubrr;
    UCSR0B = (1<<RXEN0)|(1<<TXEN0)|(1<<RXCIE0); //Enable Rx and Tx & Receive interrupt
    UCSR0C = (1<<USBS0)|(3<<UCSZ00);            //Set frame format: 8data, 2stop bit
}

uint16_t get_char(void)
{
    if (rx_cnt == 0)
        return NO_DATA;
    else if (rx_p < rx_cnt)
        return rx_buf[rx_p++];
}

void put_char(uint8_t data)
{
    if (tx_cnt < 32)                        // start count the input data
        tx_buf[tx_cnt++] = data;            // 1st input location tx_cnt

        UCSR0B |= (1<<UDRIE0);
}

void put_s(uint8_t *str)
{
    while (*str)
        put_char(*str++);
}

ISR(USART_RX_vect)
{
    if (rx_cnt < 32)                        // start count the received data
        rx_buf[rx_cnt++]=UDR0;              // 1st output location rx_cnt
    else
        rx_cnt = 0;                         // when the counter reached max value, reset it, overwrite everything

if (!(UCSR0A & (1<<RXC0)))
        UCSR0B &= ~(1<<UDRIE0);
}
ISR(USART_UDRE_vect)
{
    uint8_t i;
    if (tx_p < tx_cnt)                      // check if pointer < counted data
        UDR0 = tx_buf[tx_p++];
        else
        {
            for (i=0;i<tx_cnt;i++)
                tx_buf[i]=0;
            tx_p = 0;                       // when the pointer reach max value, reset it
            tx_cnt= 0;                      // also reset counter for new transmission
            UCSR0B &= ~(1<<UDRIE0);         // disable empty data register interrupt
        }
}

If the there's a received char, then ISR is executed, the function exit the LED blink loop, but when the receive stops and there're no more data it doesn't return the blink state and keeps sending the last received char. How to solve this problem?

Answer 1

There are many, many problems with this code. The most fundamental flaw is that it uses linear buffers, where you should be using circular buffers instead. I'll come to this later. Now, if we look at the details, in the header file:

#define Buf_OF      0x0200

This, and a few other macros, serve no purpose. Do not put "TODO" items in the code: put them in a TODO file, or as comments. Strive to always have your code in a clean state. More so when you post it in a public place.

#define buf_mask Buf_s - 1

This is presumably intended to be used as a bit mask, but it will not work reliably. For example, if you try to complement the mask as ~buf_mask, the preprocessor will expand that to ~Buf_s - 1, and the compiler will interpret it as (~Buf_s) - 1, which is not what you want. You should always parenthesize any expression you use in a macro, as

#define buf_mask (Buf_s - 1)

Later,

void put_s(uint8_t *str);

If you want to be able to issue calls like put_s("Hello!");, then you should declare the function as:

void put_s(const char *str);

Now, in the implementation, you have:

static volatile uint8_t rx_errors,tx_sBuf,rx_sBuf;

All those are unused. Remove them. Again, this is supposed to be a program, not a TODO list.

Later,

uint16_t get_char(void)
{
    if (rx_cnt == 0)
        return NO_DATA;
    else if (rx_p < rx_cnt)
        return rx_buf[rx_p++];
}

The compiler should have warned you about this: what if both conditions are false? In this case the function will return by reaching its end, with no specified return value. This is undefined behavior. You should probably return NO_DATA in this case. But then, that's what you should return every time you are not returning valid data, so the function can be simply written as:

uint16_t get_char(void)
{
    if (rx_p < rx_cnt)
        return rx_buf[rx_p++];
    else
        return NO_DATA;
}

Later, in ISR(USART_RX_vect),

if (rx_cnt < 32)
    rx_buf[rx_cnt++]=UDR0;
else
    rx_cnt = 0;

If you set rx_cnt to zero without updating rx_p, then the subsequent characters that you will put in the buffer will be ignored by get_char() until you fill the buffer up to rx_p. Thus you presumably want to set rx_p to zero also. And if you do this at the start if the ISR, you ensure that at least the current character is not lost:

// when the counter reached max value, reset the buffer
if (rx_cnt >= 32) {
    rx_p = 0;
    rx_cnt = 0;
}
rx_buf[rx_cnt++]=UDR0;

Further in the ISR:

if (!(UCSR0A & (1<<RXC0)))
    UCSR0B &= ~(1<<UDRIE0);

This makes no sense: if you have no pending character in the receive register (which you should not have, as you have just read that register), then you disable the ISR that serves the transmitter. The transmitter and the receiver should be able to work independently: remove those lines.

Then, in ISR(USART_UDRE_vect):

for (i=0;i<tx_cnt;i++)
    tx_buf[i]=0;

This serves no purpose. If you handle your buffers properly, you should never access uninitialized data in them. Thus you will never read the zeroes you are writing here. Again, remove those lines.

If you implement all the fixes above, you should end up with a code that “kind of” works. There are still a couple of issues though:

Race conditions

If get_char() gets interrupted by ISR(USART_RX_vect), you can get what is known as a “race condition”, which can lead to incoherent behavior or data corruption. For example, testing rx_p<rx_cnt is not atomic: the CPU has to read rx_cnt from RAM, then read rx_p, then compare them. If the interrupt fires between the two memory reads, the ISR can set both values to zero, then the test will compare the old rx_cnt with the new rx_p (which is now zero) and incorrectly assume there is valid data to be returned.

There are other places where this can byte you. For example, rx_p++ is not atomic either, it's a read-modify-write sequence that can very well be interrupted in the middle. And there are similar issues if put_char() gets interrupted by ISR(USART_UDRE_vect).

The fix for these issues is to disable interrupts while you are in the “critical sections” that access data shared with the ISRs. This can be done using noInterrupts()/interrupts() pairs or, if you prefer the avr-libc style, with atomically executed code blocks. For example:

#include <util/atomic.h>

uint16_t get_char(void)
{
    uint16_t result = NO_DATA;
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        if (rx_p < rx_cnt)
            result = rx_buf[rx_p++];
    }
    return result;
}

A similar protection is needed in put_char().

Linear buffers

This is the main flaw, because it will require you to rethink a significant part of your code. Imagine what can happen if the UART transmitter lags slightly behind your program calling put_char(). Say you reach tx_p = 30 and tx_cnt = 32. Then your buffer has 30 “empty” cells (from 0 to 29 the space is available, as that data has already been sent) and two cells with bytes pending to be sent (at positions 30 and 31). If you call now put_char() to send an extra byte, that byte will be lost (because tx_cnt<32 is false). This is unacceptable: you should not loose data when the buffer is almost empty!

The solution is to put the extra byte at position 0, the next one at position 1, etc. But this creates a complication: now the pending bytes are at positions 30, 31, 0, 1... At this point your buffer becomes a circular buffer. It's not a big deal, but you must read about the subject before you try to implement it.

Note that the same issue applies to the receive buffer: both buffers should be made circular.