Can my program be more efficient ?

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

Hey !

 

I made a program which prints a real time system clock on a terminal.

 

 

This function send bytes via serial connection

void transmitByte(uint8_t data) {
	/* Wait for empty transmit buffer */
	loop_until_bit_is_set(UCSR0A, UDRE0);
	UDR0 = data;                                            /* send data */
}

This one arranges the symbols for display.

void printTime(){
	transmitByte(13);						//carriage return
	transmitByte('0' + hour / 10);
	transmitByte('0' + hour % 10);
	transmitByte(':');
	transmitByte('0' + minute / 10);
	transmitByte('0' + minute % 10);
	transmitByte(':');
	transmitByte('0' + second / 10);
	transmitByte('0' + second % 10);

Can this arrangement be done in a more efficient way ?

 

 

Does it only seem so to me or are my questions really stupid ? Are you going to make memes about my stupidity ?

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

I'd just connect a single uart_sendchar() style routine to stdout using FDEV_SETUP_STREAM then you can just use printf or puts etc.

At the very least consider sprintf() then a single uart_puts()

The only downside of printf is that it can occupy 1K+ of flash but that's probably only an issue in tiny 1 or 2K chips

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

PS forgot to say that '\n' is perhaps more descriptive to the reader than just 13. Actually do you mean 10 or 13 anyway?

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

You may want probably to use a ringbuffer and an ISR which sends out the data 'on its own' so that your program flow does not blocked.

In the beginning was the Word, and the Word was with God, and the Word was God.

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

You need to define :

efficient way ?

size in flash

size in C

CPU time

etc. 

 

And add:

Do you need / and % somewhere else? (it's faster to sub with 100 until you cant any more, and  then with 10)

Last Edited: Sat. Dec 10, 2016 - 08:19 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You need to determine whether you actually need to optimize it to begin with, usually it's not necessary. However, since there's no hardware support for division or modulus it's fairly easy to "improve" upon what you have, depending on your definition of improve. There's an avr app note that gives much more efficient conversion for atmegas at http://www.atmel.com/images/doc0938.pdf, but is it worth losing the ease of readability in most cases? Probably not, especially given that your current implementation blocks while waiting for the uart to free up.

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

Thank you for answers, I use this function to print strings

#include <string.h>

void printString(char string[])
{
	/*pick up characters from string */
	uint16_t c;
	for(c = 0; c < strlen(string); c++){
		transmitByte(string[c]);					
	}
}

I was wandering if there was any way to print clock like if I using printf() function.

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

With printf you'd use:

printf("\n%02u:%02u:%02u", hour, minute, second);

 

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

Well, if you already have 3 separate counters for hours, mins and secs, you could do 6 counters for tens of hours, hours, tens of minutes, minutes, tens of seconds and seconds. This way you could avoid the divisions, at the cost of increasing complexity/time in other parts of the code.

 

Overall, I think it would be faster and maybe also smaller.

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

Maybe I should leave my program as it is. I just though that this printTime() function is too bulky. By the way the first byte (13) returns to the beginning of a current line, it doesn't start a new line. 

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

Hi Walrus,

Your code looks pretty much optimized for readability.

Doing magic with a USD 7 Logic Analyser: https://www.avrfreaks.net/comment/2421756#comment-2421756

Bunch of old projects with AVR's: http://www.hoevendesign.com

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

Thank you, you made me feel better :).

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

The condition in 7's for loop could be string[c].

That is how strlen works.

"SCSI is NOT magic. There are *fundamental technical
reasons* why it is necessary to sacrifice a young
goat to your SCSI chain now and then." -- John Woods

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

skeeve wrote:

The condition in 7's for loop could be string[c].

That is how strlen works.

 

What do you mean ?
 

Last Edited: Sun. Dec 11, 2016 - 10:50 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your printString method is not efficient as strlen counts the characters in the string then you output them. You don't need to count the characters, you only need to output them and a character of 0 tells you the end of the string.

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

He meant you could replace:

	for(c = 0; c < strlen(string); c++){
		transmitByte(string[c]);
	}

with:

	c = 0;
        while(string[c]){
		transmitByte(string[c++]);
	}

but you can actually get rid of 'c' all together:

void printString(char * string)
{
	while(*string){
		transmitByte(*string++);
	}
}

Last Edited: Sun. Dec 11, 2016 - 12:31 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

That helps a lot ! Thank you guys.