Pages: [1]   Go Down
Author Topic: Help reducing code size  (Read 374 times)
0 Members and 1 Guest are viewing this topic.
INDIA
Offline Offline
Sr. Member
****
Karma: 0
Posts: 373
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
#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();
  }
}


Logged

Global Moderator
UK
Offline Offline
Brattain Member
*****
Karma: 238
Posts: 24353
I don't think you connected the grounds, Dave.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Can you see how this:
Code:
   state = btn2_Cnt;
    if(state != EEPROM.read (4)){
      EEPROM.write(4, state);
looks a lot like this:
Code:
   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)

« Last Edit: August 18, 2011, 09:16:31 am by AWOL » Logged

"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.

New Jersey
Offline Offline
Faraday Member
**
Karma: 48
Posts: 3417
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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?
Logged

nr Bundaberg, Australia
Online Online
Tesla Member
***
Karma: 121
Posts: 8447
Scattered showers my arse -- Noah, 2348BC.
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Code:
   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:
 
    if(fadered != EEPROM.read (8)){
      EEPROM.write(8, fadered);
     
    }

Likewise with "state1" and "state".

This

Code:
fadered = fadered + 25;
.....
fadered = fadered - 25;
 

could be

Code:
fadered += 25;
.....
fadered -= 25;
 

No smaller but slight simpler source code.

Code:
int RECV_PIN = 11;

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

______
Rob


 
Logged

Rob Gray aka the GRAYnomad www.robgray.com

Global Moderator
UK
Offline Offline
Brattain Member
*****
Karma: 238
Posts: 24353
I don't think you connected the grounds, Dave.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
int fadered = 25;
....
  fadered = EEPROM.read(8);
Two bytes could be saved by not initialising "fadered" when declared!

Make the pin constants constant.
Logged

"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.

0
Offline Offline
Sr. Member
****
Karma: 0
Posts: 360
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
#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.
Logged

INDIA
Offline Offline
Sr. Member
****
Karma: 0
Posts: 373
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
#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.. smiley

Your code just worked fine... smiley
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...smiley
Logged

Pages: [1]   Go Up
Jump to: