Xmega USART Receiving(Rx) issue due to buffer overflow

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

Hey Professionals, 

I need your support regarding the communication between Xmega and PC using USART (RS-232). I could manage and write the write code for Xmega to PC transmission but when I tried the other way, Xmega to PC, I had a big an incomplete data. I added a check loop for buffer overflow and I found that I had each time, the buffer overflow that made the message sent from PC missing information. 

I would be very grateful if someone would help me to get from this. Once again thank you for the support, find below the code that I made for sending and recieving

 

//USART sending function

void SendUART_RS232(char *text)
{
    while(*text)
    {
        while( !(USARTC0_STATUS & USART_DREIF_bm) ); 
        
        USARTC0_DATA =(*text++);
    }
    while( !(USARTC0_STATUS & USART_DREIF_bm) ); 
    USARTC0.DATA= '\n';     
}

 

// The sending function

char RecUART_RS232()
{
    int dd;
   char data1[]="";
    while( (USARTC0_STATUS & USART_RXCIF_bm) )
    {
        
        dd= USARTC0_DATA;
        sprintf(data1,"%s%c",data1,dd);
       
    while( (USARTC0_STATUS & USART_BUFOVF_bm) )    
    {
        SendUART_RS232("Buffer overflow");
    }
        
    }
        
    SendUART_RS232(data1);// This function is added to monitor what Xmega was receiving 
    return data1;   
    
}

 

 

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

A_BY wrote:

   char data1[]="";

That creates a zero-length string - so there is no room to put any received characters into it!

 

  sprintf(data1,"%s%c",data1,dd);

That is a very heavy-weight way to append a single character to a string!!

 

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

Hey Awneil, thank you for your reply. 

Actually I am using char data1[]="";  to make it empty for each new loop of reception and it is actually working when I do use   sprintf(data1,"%s%c",data1,dd);

I slightly changed the code by removing   sprintf(data1,"%s%c",data1,dd); and just adding a counter just to check if the system was reading the same number of times than the number of digit I was sending from my PC but the counter was showing a number less than the number of digit I have sent. Is it a buffer issue?

 

find bellow the reviewed code

 

// The sending function

char RecUART_RS232()
{
    int dd;

    uint8_t j =0;
   char data1[]="";
    while( (USARTC0_STATUS & USART_RXCIF_bm) )
    {
        
        dd= USARTC0_DATA;
        //sprintf(data1,"%s%c",data1,dd);
       j=j+1;

    }
     sprintf(data1,"%s%d",data1,j);       
    SendUART_RS232(data1);// This function is added to monitor what Xmega was receiving 
    return data1;   
    
}

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

The red highlighting makes your text almost illegible.

A_BY wrote:
it is actually working

But only by luck!

 

You are running off the end of the array - it is pure luck that this is not trashing anything and causing havoc in your program!

 

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

Most of us would have a RX_ISR to buffer incoming chars, then do any processing in main.  With serial comms, you only have one char time to process it before an overrun condition will happen, anything longer then one char time will be a problem, unless the USART has an internal buffer.  the faster the baud rate the less time you have to handle the received char.

 

Jim

 

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

A_BY wrote:
and it is actually working
No it isn't!! You really need to learn a bit more about C memory allocation. It's true that in C++ a class such as string has the ability to dynamically extend but in C a pointer to char needs the space for the chars allocated at the time of creation (or later using malloc). The line of C:

char data1[] = "";   

creates a 1 byte array. There is not enough storage set aside to hold any characters you may write to it. So when you use something like:

sprintf(data1, "something");

it is not going to magically allocate the 10 bytes that would be required to hold that (9 characters and terminating 0). So you will actually be writing into other variables when you do this!

 

Even if you did allocate some space with something like:

char data1[20];  

the routine is still wrong because you do:

    return data1;
}

that is actually wrong in two ways:

 

1) you cannot return a pointer to a local, automatic, stack frame variable because the local IS just created on the stack and will be deallocated as the function exits. So you then return a pointer to data that is no longer valid (and will be over-written as soon as the stack is used again for any reason).

 

2) You cannot return data1 from this routine anyway because the routine is defined as:

char RecUART_RS232()
{

that tells C you will be returning a single character from this routine. Not a pointer to an array of char (which is what data1 is). I am astonished that the C compiler did not warn you about this.

 

The way most people would do what you are trying to achieve here is:

// The sending function- "sending"?? This is receiving?!?!
char * RecUART_RS232() {
    uint8_t j =0;
    static char data1[32];

    while (1) {
        // wait until a character is received
        while( !(USARTC0_STATUS & USART_RXCIF_bm) );
        // then collect/store it
        data1[j] = USARTC0_DATA;
        j = j + 1; // surely "j++"?

        if (j > 31) break; // protection against over-writing beyond array
        // may want some other terminating condition such as:
        if data1[j-1] == '\n') break;
    }
    SendUART_RS232(data1);// This function is added to monitor what Xmega was receiving
    return data1;
}

While that's not perfect I'd suggest it's a little better than your original attempt. The array is declared "static" so it exists even when the function exits (as a .bss array). That means that a pointer to it can be returned and it will still exist when control gets back to the caller. I changed your condition to wait until RXCIF is set (you may need to clear it - I don't know Xmega well). It then stores the received character and moves on. If the index increments beyond the end of the array it returns. But that then raises the question of what is the "stopping condition". Say someone sends "hello\n" do you want it to return when it gets to '\n' ? Maybe you want to use something else as an "early terminator" like a ',' character? You could implement the while(1) I have used as while(not terminating character) instead. Or perhaps a do { } while (not terminating character) ?

Last Edited: Mon. Jan 9, 2017 - 02:17 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

char data1[] = "";   

creates a 0 byte array.

Doesn't it create a 1-byte array - containing just the terminating NUL ?

 

Aside from that, the rest of the argument holds!

 

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

ki0bk wrote:
With serial comms, you only have one char time to process it before an overrun condition will happen

Especially when a PC is the sender - as they are quite capable of sending large bursts of "back-to-back" characters.

 

 

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

awneil wrote:
Doesn't it create a 1-byte array - containing just the terminating NUL ?
Rats! You caught that before I edited it...

~$ cat test17.c
#include <stdio.h>

char data1[] = "";

int main(void) {
    printf("size = %ld\n", sizeof(data1));
    return 0;
}
$ gcc -Os test17.c -o test17
$ ./test17
size = 1

So, yes, 1 byte for the 0x00.

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

Not to worry - I wan't entirely certain!

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

awneil wrote:
I wan't entirely certain!
Me neither. I initially wrote "0 byte" then when I re-read the post I thought "is that right?" hence my test17.c test to check - at which point I edited the post.

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

Looks like I've also forgotten how to spell "wasn't"...