Receiving whole string by UART doesn't work.

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

Hello, on atmegea328P using UART I want receive whole string. I use external oscillator 16MHz and speed of my UART is 9600. My functions responsible for receiving char, sending char and sending string work perfectly. My idea is to receive single character and put it into table. At the end my function returns pointer to the first element of the table. Function sometimes gives me correct output but sometimes it returns double letters or string is totally cut. Function uart_getc() returns 0 if any error occurs. I've added new line to break the while loop when it detects an error and now funciton works almost perfect but it is missing first character. What I do wrong in my function?

 

char* uart_get_string()
{
	static char result[32];
	char data = uart_getc();
	uint8_t i = 0;

	while(data != '\n') {
		if(data == 0) break;
		data = uart_getc();
		result[i++] = data;
	}
	result[i] = '\r';

	return result;
}

 

This topic has a solution.
Last Edited: Wed. Jun 29, 2022 - 03:19 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What you describe sounds very much like the baud rate is slightly off. So, a question: what are you using as the clock source for the micro? Possibilities include:

 

(1) Internal oscillator

 

(2) External crystal or ceramic resonator

 

(3) External oscillator.

 

Note: External oscillator requires its own power and is typically a 4-pin device. A crystal has two pins, usually, while a ceramic resonator has 3 or 4 pins but requires no independent power.

 

The usual solution with a Mega328 is to use a crystal when serial communication is needed. You will also need two small capacitors, typically 15pf or 18pf.

 

The issue is that the Mega328 internal oscillator is just not good enough for reliable UART-based communication.

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

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

It seems that the 1st char goes nowhere  (before the while loop)

 

However I don't quite understand the code well. sad

John Samperi

Ampertronics Pty. Ltd.

https://www.ampertronics.com.au

* Electronic Design * Custom Products * Contract Assembly

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

Function uart_getc() returns 0 if any error occurs.

But you said that part works perfectly--

My functions responsible for receiving char, sending char and sending string work perfectly -why do you have an error?

  Is that working or not??? 

returns 0 if any error occurs.

Such as what?  How did you know you have an error?

 

You need to take things more -step-by-step.

 

The first thing you might have done is simply sent every rcvd char back out  (to a terminal)...does everything look proper?

Worry about storing, arrays, tables, etc only after that works 100%

while(data != '\n') {
		if(data == 0) break;

This arrangement doesn't make a whole lot of sense

 

Note that your return does not return the entire result array, just a pointer to it (not sure how you are making use of the return)

Also, you need to abort/return/wrapaound, whatever, if you exceed your 32 char limit...put this safety into the program!!

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

tonn05 wrote:
but it is missing first character.

Well that is exactly how the code is written to behave. There is no bug. You intentionally dump the first character.

 

Try this. The while statement may look odd to the uninitiated; but It is an often used pattern in C for getting a value from a function and testing it's value as a loop control variable:

 

char* uart_get_string()
{
	static char result[32];
	char data;
	uint8_t i = 0;

	while ((data = uart_getc()) != '\n') {
		if (data == 0) break;
		if (i > (sizeof(result) - 2)) break;
		result[i++] = data;
	}
	result[i++] = '\r';
	result[i]   = 0; // Terminate Result String

	return result;
}

 

Last Edited: Mon. Jun 27, 2022 - 07:24 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

When you want to read at least one then usually a do{}while() is a better option than a while(){} as it defers the termination test to the END of the loop. So:

char* uart_get_string()
{
	static char result[32];
	char data;
	uint8_t i = 0;

	do {
                data = uart_getc();
		if(data == 0) break;
		result[i++] = data;
	} while (data != '\n');
	result[i++] = '\r';
        result[i] = 0x00; // VERY IMPORTANT - there must be 0x00 at the end of a "string"

	return result;
}

But another thought for you - do you really want to embed the [32] limit in this function? Why not have the caller allocate there own buffer and pass in a pointer and length?

char* uart_get_string(char * result, int len)
{
	char data;
	uint8_t i = 0;

	do {
        data = uart_getc();
		if(data == 0) {
		    break;
		}
		result[i++] = data;
		if (i > len) {
		    break;
		}
	} while (data != '\n');

	result[i++] = '\r';
    result[i] = 0x00; // VERY IMPORTANT - there must be 0x00 at the end of a "string"

	return result;
}

...
    char buf[12];
    printf("result = %s", uart_get_string(buf, 12));
...
    char txt[23];
    uatr_get_string(txt, 23);

But really why not actually do:

// this just wraps the interface that FDEV_SETUP needs around the existing uart_getc()..
int uart_fdev(FILES * stream_unused) {
    char c;

    c = uart_getc();
    return c;
}

uart_str = FDEV_SETUP_STREAM(NULL, uart_fdev, _FEDV_SETUP_READ);

int main(void) {
    char buf[20];

    stdin =  &uart_str;
    gets(buf);
}

As soon as you have single character uart_getc() and uart_putc() then you don't actually need to write anything else - you attach them to file streams for input/output and then can use all the methods in <stdio.h> like putchar(), printf(), scanf(), getchar(), gets(), etc. - you don't have to write all this stuff yourself - you can use the standard stuff that is tried and tested. (in the same way that you probably wouln't write memcpy() or strstr() etc. yourself either.

Last Edited: Mon. Jun 27, 2022 - 08:33 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
But really why not actually.......

 

Just buy a CodeVision license and never have to write these functions again?

 

"We're off to see the WIZARD.....The wonderful WIZARD of....." winkdevil

 

Jim

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

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

I've little modified this code and it now it works perfectly. Thank you for all for help.

 

char* uart_get_string()
{
	static char result[32];
	char data;
	uint8_t i = 0;

	while ((data = uart_getc()) != '\n') {
		if (data == 0) break;
		if (i > (sizeof(result) - 2)) break;
		result[i++] = data;
	}
	result[i++] = data;
	result[i++]   = '\r';
	result[i] = 0; // Terminate Result String

	return result;
}

 

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

So you are sticking with the fixed [32] rather than taking onboard the advice I gave?

 

Also what about using FDEV and stdio ?