 |
 |
| View previous topic :: View next topic |
| Author |
Message |
MiniMe
Joined: 17 Nov 2009
Posts: 50
|
| Is it my bad programming? |
 Posted: Fri Jan 07, 2011 10:38 am |
 |
|
Hi,
I have a relatively simple and small program that takes 5% of ROM (1616 bytes), I believe it should take less that that. Is there something that i do very wrong or it is normal that code like that take up so much ROM?
| Code: | #include <18F4550.h>
#fuses HSPLL,NOWDT,NOPROTECT,NOLVP,NODEBUG,USBDIV,PLL1,CPUDIV1,VREGEN
#use delay(clock=48000000)
#include <flex_lcd.c>
int1 select, exit, up, down, ad, timer0_tick;
char n2dalap2ev_nr_2_char(int8 nr) //just a function that other function uses
{
char a;
switch(nr) { //Kui muutuja asendadada erinevate returnidega =< ROM
case 0: a = 'E';
break;
case 1: a = 'E';
break;
case 2: a = 'T';
break;
case 3: a = 'K';
break;
case 4: a = 'N';
break;
case 5: a = 'R';
break;
case 6: a = 'L';
break;
case 7: a = 'P';
break;
}
return a;
}
int8 aeg_muutmine(int8 a,int8 b,int8 c,int8 koht,int1 A_D)//The problem function
{
int8 loendur;
//printf("\n\ra%u b%u c%u",a,b,c);
while((!select) && (!exit)) {
//printf("\n\rkoht %u",koht);
if(timer0_tick) {
timer0_tick = FALSE;
loendur++;
}
if(loendur < 5) {
if(A_D){
if(koht == 1) {
lcd_gotoxy(1,1);
lcd_putc(' ');
}
else if(koht == 2){
lcd_gotoxy(3,1);
lcd_putc(' ');
lcd_putc(' ');
}
else{
lcd_gotoxy(6,1);
lcd_putc(' ');
lcd_putc(' ');
}
}
else{
if(koht == 1) {
lcd_gotoxy(1,1);
lcd_putc(' ');
lcd_putc(' ');
}
else if(koht == 2){
lcd_gotoxy(4,1);
lcd_putc(' ');
lcd_putc(' ');
}
else{
lcd_gotoxy(7,1);
lcd_putc(' ');
lcd_putc(' ');
}
}
}
else if(loendur > 10){
loendur = 0;
}
else{
if(A_D){
if(koht == 1){
lcd_gotoxy(1,1);
lcd_putc((char)n2dalap2ev_nr_2_char(a));
}
else if(koht == 2){
lcd_gotoxy(3,1);
printf(lcd_putc,"%02u",b);
}
else{
lcd_gotoxy(6,1);
printf(lcd_putc,"%02u",c);
}
}
else{
if(koht == 1){
lcd_gotoxy(1,1);
printf(lcd_putc,"%02u",a);
}
else if(koht == 2){
lcd_gotoxy(4,1);
printf(lcd_putc,"%02u",b);
}
else{
lcd_gotoxy(7,1);
printf(lcd_putc,"%02u",c);
}
}
}
//printf("\n\ra%u b%u c%u",a,b,c);
if(up) {
up = FALSE;
loendur = 5;
if(A_D){
if(koht == 1){
if(a != 7 ) {
a++;
}
}
else if(koht == 2){
if(b != 23 ) {
b++;
}
}
else{
if(c != 59 ) {
c++;
}
}
}
else{
if(koht == 1){
if(a != 31 ) {
a++;
}
}
else if(koht == 2){
if(b != 12 ) {
b++;
}
}
else{
if(c != 99 ) {
c++;
}
}
}
}
if(down) {
down = FALSE;
loendur = 5;
if(A_D){
if(koht == 1){
if(a >= 2 ) {
a--;
}
}
else if(koht == 2){
if(b !=0 ) {
b--;
}
}
else{
if(c !=0 ) {
c--;
}
}
}
else{
if(koht == 1){
if(a !=0 ) {
a--;
}
}
else if(koht == 2){
if(b !=0 ) {
b--;
}
}
else{
if(c !=0 ) {
c--;
}
}
}
}
}
if(koht == 1){
return a;
}
else if(koht == 2){
return b;
}
else{
return c;
}
}
int main(){
lcd_init();
for(;;) {
aeg_muutmine(11, 22,33,2,TRUE);
}
return 0;
} |
Thank you in advance! |
|
 |
Ttelmah
Joined: 11 Mar 2010
Posts: 19377
|
|
| Posted: Fri Jan 07, 2011 11:02 am |
|
|
You should be able to make quite a significant reduction quite simply.
Look at this, and then the replacement:
| Code: |
if(A_D){
if(koht == 1) {
lcd_gotoxy(1,1);
lcd_putc(' ');
}
else if(koht == 2){
lcd_gotoxy(3,1);
lcd_putc(' ');
lcd_putc(' ');
}
else{
lcd_gotoxy(6,1);
lcd_putc(' ');
lcd_putc(' ');
}
}
//The last lcd_putc, is common to all choices, so:
//Simple change:
if(A_D){
if(koht == 1) {
lcd_gotoxy(1,1);
}
else if(koht == 2){
lcd_gotoxy(3,1);
lcd_putc(' ');
}
else{
lcd_gotoxy(6,1);
lcd_putc(' ');
}
lcd_putc(' ');
}
|
The same applies to each of the subsequent operations, and _particularly_ a bit latter, to the ones involving printf. Printf, is always bulky. Involves repeated division by ten....
I'd suspect you could reduce the size by perhaps 30% by just doing this style of change.
Best Wishes |
|
|
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Fri Jan 07, 2011 11:09 am |
|
|
| Quote: | I have a relatively simple and small program that takes 5% of ROM (1616 bytes).
|
I compiled it with vs. 4.116 and I got 1328 bytes of Flash memory.
You have about 200 lines of code. Look at the .LST file. Each line takes
about 3 instructions. Each instruction takes 2 bytes. So 200 x 3 x 2 =
1200 bytes. That's about right.
| Quote: | int main(){
lcd_init();
for(;;) {
aeg_muutmine(11, 22,33,2,TRUE);
}
return 0;
}
|
main() doesn't return to anything. There is no O/S to return to.
Here's the .LST file for the end of main(). It doesn't return. It executes a
Sleep instruction at the end of main and puts the PIC into sleep mode.
| Quote: |
.... return 0;
052A: MOVLW 00
052C: MOVWF 01
.... }
052E: SLEEP
|
|
|
|
MiniMe
Joined: 17 Nov 2009
Posts: 50
|
|
| Posted: Fri Jan 07, 2011 1:03 pm |
|
|
Thank You for replies.
I used all the suggestion and tips you gave me and I've managed to lower ROM usage from 1616 to 1496 bytes. ~8%
Version of my compiler is 4.106 ... I guess upgrading it might save much more ROM than minor changes in code.
| Code: |
#include <18F4550.h>
#fuses HSPLL,NOWDT,NOPROTECT,NOLVP,NODEBUG,USBDIV,PLL1,CPUDIV1,VREGEN
#use delay(clock=48000000)
#include <flex_lcd.c>
int1 select, exit, up, down, timer0_tick;
char n2dalap2ev_nr_2_char(int8 nr) //just a function that other function uses
{
char a;
switch(nr) { //Kui muutuja asendadada erinevate returnidega =< ROM
case 0: a = 'E';
break;
case 1: a = 'E';
break;
case 2: a = 'T';
break;
case 3: a = 'K';
break;
case 4: a = 'N';
break;
case 5: a = 'R';
break;
case 6: a = 'L';
break;
case 7: a = 'P';
break;
}
return a;
}
void print_function(int8 a) //PCM programmer suggestion
{
//printf(lcd_putc,"%02u",a); // saved 6 bytes of ROM
int b;
b = a/10;
lcd_putc(b+48); // by divide 10 you can get Example: 9 from 97
b = a%10;
lcd_putc(b+48); // by modulo 10 you can get Example: 7 from 97
// by using modulo 72 byes saved 72 vs 6
}
int8 aeg_muutmine(int8 a,int8 b,int8 c,int8 koht,int1 A_D)//The problem function
{
int8 loendur, d; //d to replase 3X returns 2 byte saved
//printf("\n\ra%u b%u c%u",a,b,c);
while((!select) && (!exit)) {
//printf("\n\rkoht %u",koht);
if(timer0_tick) {
timer0_tick = FALSE;
loendur++;
}
if(loendur < 5) {
if(A_D){
if(koht == 1) {
lcd_gotoxy(1,1);
}
else if(koht == 2){
lcd_gotoxy(3,1);
lcd_putc(' ');
}
else{
lcd_gotoxy(6,1);
lcd_putc(' ');
}
}
else{
if(koht == 1) {
lcd_gotoxy(1,1);
}
else if(koht == 2){
lcd_gotoxy(4,1);
}
else{
lcd_gotoxy(7,1);
}
lcd_putc(' '); //tTelmah seuggested change 46 bytes saved
}
lcd_putc(' '); //tTelmah seuggested change 46 bytes saved
}
else if(loendur > 10){
loendur = 0;
}
else{
if(A_D){
if(koht == 1){
lcd_gotoxy(1,1);
lcd_putc((char)n2dalap2ev_nr_2_char(a));
}
else if(koht == 2){
lcd_gotoxy(3,1);
print_function(b);
}
else{
lcd_gotoxy(6,1);
print_function(c);
}
}
else{
if(koht == 1){
lcd_gotoxy(1,1);
print_function(a);
}
else if(koht == 2){
lcd_gotoxy(4,1);
print_function(b);
}
else{
lcd_gotoxy(7,1);
print_function(c);
}
}
}
//printf("\n\ra%u b%u c%u",a,b,c);
if(up) {
up = FALSE;
loendur = 5;
if(A_D){
if(koht == 1){
if(a != 7 ) {
a++;
}
}
else if(koht == 2){
if(b != 23 ) {
b++;
}
}
else{
if(c != 59 ) {
c++;
}
}
}
else{
if(koht == 1){
if(a != 31 ) {
a++;
}
}
else if(koht == 2){
if(b != 12 ) {
b++;
}
}
else{
if(c != 99 ) {
c++;
}
}
}
}
if(down) {
down = FALSE;
loendur = 5;
if(A_D){
if(koht == 1){
if(a >= 2 ) {
a--;
}
}
else if(koht == 2){
if(b !=0 ) {
b--;
}
}
else{
if(c !=0 ) {
c--;
}
}
}
else{
if(koht == 1){
if(a !=0 ) {
a--;
}
}
else if(koht == 2){
if(b !=0 ) {
b--;
}
}
else{
if(c !=0 ) {
c--;
}
}
}
}
}
if(koht == 1){
a = d;
}
else if(koht == 2){
b = d;
}
else{
c = d;
}
return d;
}
int main(){
lcd_init();
for(;;) {
aeg_muutmine(11, 22,33,2,TRUE);
}
//No need return because on no OS.
} |
|
|
|
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Fri Jan 07, 2011 1:14 pm |
|
|
Your program gives this warning message when compiled:
| Quote: | | Function not void and does not return a value MAIN |
To remove this warning message, you can declare main() to
return a 'void' as shown below.
| Quote: | void main(){
lcd_init();
for(;;) {
aeg_muutmine(11, 22,33,2,TRUE);
}
while(1);
} |
Also, if you don't want the PIC to go to sleep, you can put a continuous
loop at the end of main() as shown above. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19377
|
|
| Posted: Fri Jan 07, 2011 3:01 pm |
|
|
As a further comment, look at the div function.
When you perform a division, the code internally has the result, and the remainder. The normal division, doesn't give you this, but the div function returns both. Used with the 'print function', it should help a little further.
You might also simply try adjusting the opt level. While this can cause code to become unreliable at the highest settings, increasing it a little might well help.
Best Wishes
Last edited by Ttelmah on Sat Jan 08, 2011 5:30 am; edited 1 time in total |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Sat Jan 08, 2011 3:19 am |
|
|
Another comment is not related to reducing the code size but maintainability:
Just looking at the code I have no clue as to what it is doing or supposed to do... 
Good code is 'self documenting', i.e. the code contains comments and uses meaningful variable and function names. The use of variable names like 'a', 'b', 'c' should only be used for 'simple' counters, in all other cases the use of more descriptive names will make your code easier to read.
Another tip is to use the concept of 'state machines'. Your code probably works as intended but the long list of if-else constructs makes it difficult to follow. A State Machine design will allow you to divide the program into 'logical' blocks that are easier to comprehend and hence will contain fewer bugs. |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
