Go Down

Topic: Help reducing code size (Read 474 times) previous topic - next topic

Joy

I have written a code whose size is 7292 bytes..
as I am new to coding I may have done some things in a long way.
Friends please help and guide me in places where the code can be made shorter to reduce the size, as I am planning to write this code to an Atmega 88..

Code: [Select]
#include <IRremote.h>
#include <EEPROM.h>
#include "Seg7.h"
int pin1 = 8;
int pin3 = 9;
int RECV_PIN = 11;
int fadered = 25;
int pin1State = LOW;
int pin2State = LOW;
int val;
int state;
int state1;
int btn1_Cnt = LOW;
int btn2_Cnt;
Seg7 s7;
int digit;

IRrecv irrecv(RECV_PIN);
decode_results results;

void setup()
{
   s7.attach(0, 1, 2, 3, 4, 5, 6, 7);
  s7.write(digit);
  irrecv.enableIRIn();
  pinMode(pin1, OUTPUT);
  pinMode(pin3, OUTPUT);
  fadered = EEPROM.read(8);

  /********** THIS IS TO READ WHAT WAS THE STATE OF PIN3 SAVED LAST TIME ***********/

  btn2_Cnt = EEPROM.read(4);

  if(btn2_Cnt % 2 == 0){
    analogWrite(pin3, 0);
}
  else{
    analogWrite(pin3, fadered);
  }
   
   s7.write(digit);


  /********** THIS IS TO READ WHAT WAS THE STATE OF PIN1 SAVED LAST TIME ***********/

  btn1_Cnt = EEPROM.read(0);

  if(btn1_Cnt % 2 == 0){
    digitalWrite(pin1, LOW);
  }
  else{
    digitalWrite(pin1, HIGH);
  }
 
      digit = map(fadered,24, 249, 0, 9);
    s7.write(digit);


}
void loop() {
  if (irrecv.decode(&results)) {

    /********** THIS PART IS FOR PIN1 ************/

    if(results.value == 0xffb24d){
      btn1_Cnt++;
      if(btn1_Cnt % 2 == 0)
        digitalWrite(pin1, LOW);
      else
        digitalWrite(pin1, HIGH);

    }

    /********** THIS PART IS FOR PIN3 ************/

    if(results.value == 0xff7887 && fadered <= 230){
      fadered = fadered + 25;
      analogWrite(pin3, fadered);
    }

    if(results.value == 0xff50af && fadered >= 25){
      fadered = fadered - 25;
      analogWrite(pin3, fadered);
    }
   
   
        if(results.value == 0xff38c7){
      btn2_Cnt++;
      if(btn2_Cnt % 2 == 0)
        analogWrite(pin3, 0);
      else
        analogWrite(pin3, fadered);
    }
    digit = map(fadered,24, 249, 0, 9);
    s7.write(digit);

    /********** THIS IS FOR SAVING THE BRIGHTNESS VALUE IN EEPROM ************/

    val = fadered;
    if(val != EEPROM.read (8)){
      EEPROM.write(8, val);
     
    }

    /********** THIS IS TO SAVE THE LAST STATE OF THE PIN3 ************/

    state = btn2_Cnt;
    if(state != EEPROM.read (4)){
      EEPROM.write(4, state);
   
    }

    /********** THIS IS TO SAVE THE LAST STATE OF THE PIN1 ************/

    state1 = btn1_Cnt;
    if(state1 != EEPROM.read (0)){
      EEPROM.write(0, state1);
 
    }
   
   
    irrecv.resume();
  }
}



AWOL

#1
Aug 18, 2011, 04:02 pm Last Edit: Aug 18, 2011, 04:16 pm by AWOL Reason: 1
Can you see how this:
Code: [Select]
   state = btn2_Cnt;
   if(state != EEPROM.read (4)){
     EEPROM.write(4, state);

looks a lot like this:
Code: [Select]
   state1 = btn1_Cnt;
   if(state1 != EEPROM.read (0)){
     EEPROM.write(0, state1);
?
Quite often that says "time for a function".
Won't always make your code smaller, but it'll make it easier to debug.

(it is also worth pointing out that "btn1_Cnt" is an "int", and doesn't fit in an EEPROM byte)

"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

wildbill

There are a number of small changes you can make - variables like state that aren't necessary, AWOL's function, transforming the if statements that digitalwrite HIGH or LOW to a single digitalwrite that uses the ifs condition directly. These aren't going to get you much though - the libraries you are using are likely the cause of the size of your code.

I see that the 88 has 8K available - how much does the bootloader occupy and so how much do you need to reduce it?

Graynomad

Overall I think it's pretty good, just a few comments

Code: [Select]
   val = fadered;
    if(val != EEPROM.read (8)){
      EEPROM.write(8, val);
     
    }


val isn't used anywhere else, no point assigning fadered to it just for this piece of code.

Code: [Select]
 
    if(fadered != EEPROM.read (8)){
      EEPROM.write(8, fadered);
     
    }


Likewise with "state1" and "state".

This

Code: [Select]
fadered = fadered + 25;
.....
fadered = fadered - 25;


could be

Code: [Select]
fadered += 25;
.....
fadered -= 25;


No smaller but slight simpler source code.

Code: [Select]
int RECV_PIN = 11;

This is a constant, either use "const" or "#define".

______
Rob



Rob Gray aka the GRAYnomad www.robgray.com

AWOL

Code: [Select]
int fadered = 25;
....
 fadered = EEPROM.read(8);

Two bytes could be saved by not initialising "fadered" when declared!

Make the pin constants constant.
"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

Aeturnalus

Hmm... you could also try changing the if/else chain to a switch/case, which may reduce size ( I believe switch/case compiles to a jump table, whereas if/else compiles to a bunch of jump-if-compare or jump-if-not-compare statements ).

Since you're storing an on/off value for pin1 and pin3, you can also use a single byte for both of them. You also set pin1 and pin3 multiple times without changing the values, which is a waste - just set it once at the end. Moreover, there is repeated code between setup and loop - encapsulate that into a function to save space.

Code: [Select]

#include <IRremote.h>
#include <EEPROM.h>
#include "Seg7.h"
const int pin1 = 8;
const int pin3 = 9;
const int RECV_PIN = 11;
uint8_t fadered = 25;

Seg7 s7;
int digit;

IRrecv irrecv(RECV_PIN);
decode_results results;

uint8_t stateReg;
const int PIN1_MSK = ( 1 << 0 );
const int PIN3_MSK = ( 1 << 1 );

const int FADE_REG = 8;
const int STATE_REG = 4;

void setup()
{
  s7.attach(0, 1, 2, 3, 4, 5, 6, 7);
  s7.write(digit);
  irrecv.enableIRIn();
  pinMode(pin1, OUTPUT);
  pinMode(pin3, OUTPUT);
 
  fadered = EEPROM.read( FADE_REG );
  stateReg = EEPROM.read( STATE_REG );
 
  setStates();

  digit = map(fadered, 24, 249, 0, 9);
  s7.write(digit);
}

void setStates() {
// set pin3
if ( stateReg & PIN3_MSK ) {
analogWrite( pin3, 0 );
} else {
analogWrite( pin3, fadered );
}
// set pin1
digitalWrite( pin1, ( stateReg & PIN1_MSK ) );
}

void loop() {
  if (irrecv.decode(&results)) {

switch ( results.value ) {
case 0xffb24d: // pin1
stateReg ^= PIN1_MSK;
break;
case 0xff38c7: // pin3
stateReg ^= PIN3_MSK;
break;
case 0xff7887:
if ( fadered <= 230 ) {
fadered += 25;
}
break;
case 0xff50af:
if ( fadered >= 25 ) {
fadered -= 25;
}
break;
}

    digit = map(fadered, 24, 249, 0, 9);
    s7.write(digit);

setStates(); // set the state of pin1 and pin3

    if ( fadered != EEPROM.read ( FADE_REG ) ) {
      EEPROM.write( FADE_REG, fadered );
    }
if ( stateReg != EEPROM.read( STATE_REG ) ) {
  EEPROM.write( STATE_REG, stateReg );
}
   
    irrecv.resume();
  }
}



Note: I have no clue if this code compiles, since I don't have Seg7.h. But it should work, and the concepts are all thee.

Joy


Hmm... you could also try changing the if/else chain to a switch/case, which may reduce size ( I believe switch/case compiles to a jump table, whereas if/else compiles to a bunch of jump-if-compare or jump-if-not-compare statements ).

Since you're storing an on/off value for pin1 and pin3, you can also use a single byte for both of them. You also set pin1 and pin3 multiple times without changing the values, which is a waste - just set it once at the end. Moreover, there is repeated code between setup and loop - encapsulate that into a function to save space.

Code: [Select]

#include <IRremote.h>
#include <EEPROM.h>
#include "Seg7.h"
const int pin1 = 8;
const int pin3 = 9;
const int RECV_PIN = 11;
uint8_t fadered = 25;

Seg7 s7;
int digit;

IRrecv irrecv(RECV_PIN);
decode_results results;

uint8_t stateReg;
const int PIN1_MSK = ( 1 << 0 );
const int PIN3_MSK = ( 1 << 1 );

const int FADE_REG = 8;
const int STATE_REG = 4;

void setup()
{
  s7.attach(0, 1, 2, 3, 4, 5, 6, 7);
  s7.write(digit);
  irrecv.enableIRIn();
  pinMode(pin1, OUTPUT);
  pinMode(pin3, OUTPUT);
 
  fadered = EEPROM.read( FADE_REG );
  stateReg = EEPROM.read( STATE_REG );
 
  setStates();

  digit = map(fadered, 24, 249, 0, 9);
  s7.write(digit);
}

void setStates() {
// set pin3
if ( stateReg & PIN3_MSK ) {
analogWrite( pin3, 0 );
} else {
analogWrite( pin3, fadered );
}
// set pin1
digitalWrite( pin1, ( stateReg & PIN1_MSK ) );
}

void loop() {
  if (irrecv.decode(&results)) {

switch ( results.value ) {
case 0xffb24d: // pin1
stateReg ^= PIN1_MSK;
break;
case 0xff38c7: // pin3
stateReg ^= PIN3_MSK;
break;
case 0xff7887:
if ( fadered <= 230 ) {
fadered += 25;
}
break;
case 0xff50af:
if ( fadered >= 25 ) {
fadered -= 25;
}
break;
}

    digit = map(fadered, 24, 249, 0, 9);
    s7.write(digit);

setStates(); // set the state of pin1 and pin3

    if ( fadered != EEPROM.read ( FADE_REG ) ) {
      EEPROM.write( FADE_REG, fadered );
    }
if ( stateReg != EEPROM.read( STATE_REG ) ) {
  EEPROM.write( STATE_REG, stateReg );
}
   
    irrecv.resume();
  }
}



Note: I have no clue if this code compiles, since I don't have Seg7.h. But it should work, and the concepts are all thee.



Thank you Aeturnalus.. :)

Your code just worked fine... :)
it was really written in a clean way and in short..

I am actually learning a lot for ur code pattern..
I am still going through the code and trying to understand how it is actually working...:)

Go Up