Go Down

Topic: Help reducing code size (Read 556 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
 


Please enter a valid email to subscribe

Confirm your email address

We need to confirm your email address.
To complete the subscription, please click the link in the email we just sent you.

Thank you for subscribing!

Arduino
via Egeo 16
Torino, 10131
Italy