Split from: atmega 32 receive buffer

Go To Last Post
7 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
#define DATA_PORT PORTB            //Defining LCD PORTS(my made library)
#define ThreePIN PORTA
#define RS 5
#define RW 6
#define E  7
#include<avr/io.h>
#include<util/delay.h>
#include<avr/lcd.h>
#include<stdio.h>
unsigned char received_data[8],i,j,k,check;      // variable declaration, here i made an array for 8 character
int main()                                       //You can change for your desired length
{
    DDRB=0xff;
    DDRA=0xff;
    UBRRL = 6;                       //--- Baud Rate Set as 9600 by Data Sheet
    UCSRB = (1<<TXEN) | (1<<RXEN);      //--- Transmit and Receive Enable
    UCSRC = (1<<URSEL) | (3<<UCSZ0);
    lcd_init();
    send_a_string("Hello");             // To send Hello to LCD
   while(1)
   {
      send_a_command(0x01);
      for(k=0;k<='Y';k++)                //LOOP to check condition because we need to check until enter pressed
      {
      while(!(UCSRA&(1<<RXC)));          // receiving command
      {
	 check=UDR;
	 if(check!='\r')                   //Command to check if enter is pressed or not
	 {
	 received_data[i]=check;           // storing character by character in array
	 i++;
	 }
         else
	 {
	  k='Y';                          //Condition to terminate loop
	 }
      }
      }
      for(j=0;j<=i;j++)                  //another loop to display data on LCD
      {
      send_a_character(received_data[j]);    //Printing received data on LCD
	 received_data[j]='\0';
      }
      _delay_ms(2000);
      i=0;
      j=0;
   }
}
	    //Hope i helped someone with this Thanks

This is the code to receive UART data in buffer,

 

Point is how can i make it more reliable and short.

 

Kindly have a look.

Thanks

 

I MADE SOME CHANGES LOOK OVER IT.

#define DATA_PORT PORTB
#define ThreePIN PORTA
#define RS 5
#define RW 6
#define E  7
#include<avr/io.h>
#include<util/delay.h>
#include<avr/lcd.h>
#include<stdio.h>
unsigned char received_data[8],i,j,k,check;
int main()
{
    DDRB=0xff;
    DDRA=0xff;
    UBRRL = 6;                       //--- Baud Rate Set as 9600 by Data Sheet
    UCSRB = (1<<TXEN) | (1<<RXEN);      //--- Transmit and Receive Enable
    UCSRC = (1<<URSEL) | (3<<UCSZ0); 
    lcd_init();
    send_a_string("Hello");
   while(1)
   {
      send_a_command(0x01);
     // for(k=0;k<='Y';k++)
      do
      {
      while(!(UCSRA&(1<<RXC)));
      {  
	 check=UDR;
	 received_data[i]=check;
	 i++;
      }
      }while(check!='\r');   
      for(j=0;j<=i;j++)
      {
      send_a_character(received_data[j]);
	 received_data[j]='\0';
      } 
      _delay_ms(2000);
      i=0;
      j=0;
   } 
}
	    

 

Manish verma

Last Edited: Tue. Feb 6, 2018 - 04:15 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@Manish verma,

 

If you want us to comment on your code, you should post a new topic.  Tacking your code onto the end of a 7 year old post is probably not going to get the result you want.

 

Greg Muth

Portland, OR, US

Xplained/Pro/Mini Boards mostly

 

Make Xmega Great Again!

 

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

Agree with Greg but in the short term things like this:

	 received_data[i]=check;           // storing character by character in array
	 i++;

look very dangerous indeed. What protection do you have here against 'i' getting so large it writes beyond the bounds of received_data[] ? Once it reaches 8 you are in trouble.

Last Edited: Tue. Feb 6, 2018 - 03:50 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
look very dangerous indeed. What protection do you have here against 'i' getting so large it writes beyond the bounds of received_data[] ? Once it reaches 8 you are in trouble.

And in very short order?  Am I missing something, or is the poster reading UDR and storing the value as fast as practical -- when there is NO valid character to read?

 

      while(!(UCSRA&(1<<RXC)));
      {  
	 check=UDR;
	 received_data[i]=check;
	 i++;
      }

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Lee, there is a semi-colon lurking in there!

 

(and yes this is about the most misleading piece of code I have seen in a long long time !!)

 

PS and the indentation adds further to the  misleading view as it's difficult to see where the do's, while's and other stuff actually start/end.

 

Maybe this is a little clearer

   while(1)
   {
      send_a_command(0x01);
      do
      {
         while(!(UCSRA&(1<<RXC)));

         {
           check=UDR;
           received_data[i]=check;
           i++;
         }
      } while(check!='\r');   

      for(j=0;j<=i;j++)
      {
        send_a_character(received_data[j]);
        received_data[j]='\0';
      }
      _delay_ms(2000);
      i=0;
      j=0;
   } 

But that still raises the question of what the braces are doing around the innermost block. So perhaps:

   while(1)
   {
      send_a_command(0x01);
      do
      {
         while(!(UCSRA & (1 << RXC)));

         check = UDR;
         received_data[i] = check;
         i++;
      } while(check != '\r');   

      for(j = 0; j <= i; j++)
      {
        send_a_character(received_data[j]);
        received_data[j] = '\0';
      }
      _delay_ms(2000);
      i = 0;
      j = 0;
   } 

Last Edited: Tue. Feb 6, 2018 - 04:25 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello clawson sir,

 

PS and the indentation adds further to the  misleading view as it's difficult to see where the do's, while's and other stuff actually start/end.


I appologise for that.

 

And yes you are right these braces must be like this -

 


But that still raises the question of what the braces are doing around the innermost block. So perhaps:

   while(1)
   {
      send_a_command(0x01);
      do
      {
         while(!(UCSRA & (1 << RXC)));

         check = UDR;
         received_data[i] = check;
         i++;
      } while(check != '\r');   

      for(j = 0; j <= i; j++)
      {
        send_a_character(received_data[j]);
        received_data[j] = '\0';
      }
      _delay_ms(2000);
      i = 0;
      j = 0;
   } 

 

Manish verma

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

Well the code is still dangerous for the point that nothing prevents i++ incrementing past the end of received_data. You probably want something like:

      do
      {
         while(!(UCSRA & (1 << RXC)));

         check = UDR;
         received_data[i] = check;
         i++;
         if (i >= 7) {
            break;
         }
      } while(check != '\r')

But always remember the 0x00 at the end of the string! That takes a byte.