Please assist me in optimizing my (beginner) code

system · December 8, 2010, 4:46pm

I have made the following code (its 16 blinkm's connected to 16 pushbuttons with midi-in, midi-out will be added soon) - everything works fine so far except the code is not very elegant

I want to start using for loops and functions but i can't wrap my head around how to start.

I am not looking for a direct coding solution but more advice as to where to go from here...

Maybe some helpfull tips so i can actually learn something here.
Thank you good people

#include "Wire.h"
#include "BlinkM_funcs.h"

// I2C Adres voor alle blinkm's in het netwerk:

byte blinkm_all = 0x00; 

// Variabelen voor LED'S (I2C adressen) :

byte LED1 = 9;
byte LED2 = 10;
byte LED3 = 11;
byte LED4 = 12;
byte LED5 = 13;
byte LED6 = 14;
byte LED7 = 15;
byte LED8 = 16;
byte LED9 = 17;
byte LED10 = 18;
byte LED11 = 19;
byte LED12 = 20;
byte LED13 = 21;
byte LED14 = 22;
byte LED15 = 23;
byte LED16 = 24;

// Variabele voor MIDI in:

byte incomingByte; 

const int PUSH_BUTTON_INPUT_PIN = 2;
const int PUSH_BUTTON_INPUT_PIN2 = 3; 
const int PUSH_BUTTON_INPUT_PIN3 = 4;
const int PUSH_BUTTON_INPUT_PIN4 = 5; 
const int PUSH_BUTTON_INPUT_PIN5 = 6;
const int PUSH_BUTTON_INPUT_PIN6 = 7; 
const int PUSH_BUTTON_INPUT_PIN7 = 8;
const int PUSH_BUTTON_INPUT_PIN8 = 9; 
const int PUSH_BUTTON_INPUT_PIN9 = 10;
const int PUSH_BUTTON_INPUT_PIN10 = 11; 
const int PUSH_BUTTON_INPUT_PIN11 = 12; 
const int PUSH_BUTTON_INPUT_PIN12 = 16; 
const int PUSH_BUTTON_INPUT_PIN13 = 17; 
const int PUSH_BUTTON_INPUT_PIN14 = 18; 
const int PUSH_BUTTON_INPUT_PIN15 = 19; 
const int PUSH_BUTTON_INPUT_PIN16 = 22; 

int PUSH_BUTTON_STATE1 = 0; 
int PUSH_BUTTON_STATE2 = 0;
int PUSH_BUTTON_STATE3 = 0; 
int PUSH_BUTTON_STATE4 = 0;
int PUSH_BUTTON_STATE5 = 0; 
int PUSH_BUTTON_STATE6 = 0;
int PUSH_BUTTON_STATE7 = 0; 
int PUSH_BUTTON_STATE8 = 0;
int PUSH_BUTTON_STATE9 = 0; 
int PUSH_BUTTON_STATE10 = 0;
int PUSH_BUTTON_STATE11 = 0;
int PUSH_BUTTON_STATE12 = 0;
int PUSH_BUTTON_STATE13 = 0;
int PUSH_BUTTON_STATE14 = 0;
int PUSH_BUTTON_STATE15 = 0;
int PUSH_BUTTON_STATE16 = 0;

int PUSH_BUTTON_PRESSED;

void setup() {
     
    Serial.begin(31250);
    BlinkM_begin();

  pinMode(PUSH_BUTTON_INPUT_PIN, INPUT);     
  pinMode(PUSH_BUTTON_INPUT_PIN2, INPUT);  
  pinMode(PUSH_BUTTON_INPUT_PIN3, INPUT);     
  pinMode(PUSH_BUTTON_INPUT_PIN4, INPUT);
  pinMode(PUSH_BUTTON_INPUT_PIN5, INPUT);     
  pinMode(PUSH_BUTTON_INPUT_PIN6, INPUT);
  pinMode(PUSH_BUTTON_INPUT_PIN7, INPUT);     
  pinMode(PUSH_BUTTON_INPUT_PIN8, INPUT);
  pinMode(PUSH_BUTTON_INPUT_PIN9, INPUT);     
  pinMode(PUSH_BUTTON_INPUT_PIN10, INPUT);
  pinMode(PUSH_BUTTON_INPUT_PIN11, INPUT);
  pinMode(PUSH_BUTTON_INPUT_PIN12, INPUT);
  pinMode(PUSH_BUTTON_INPUT_PIN13, INPUT);
  pinMode(PUSH_BUTTON_INPUT_PIN14, INPUT);
  pinMode(PUSH_BUTTON_INPUT_PIN15, INPUT);
  pinMode(PUSH_BUTTON_INPUT_PIN16, INPUT);
  
}

void loop() {
  
  

    PUSH_BUTTON_STATE1 = digitalRead(PUSH_BUTTON_INPUT_PIN);
    PUSH_BUTTON_STATE2 = digitalRead(PUSH_BUTTON_INPUT_PIN2);
    PUSH_BUTTON_STATE3 = digitalRead(PUSH_BUTTON_INPUT_PIN3);
    PUSH_BUTTON_STATE4 = digitalRead(PUSH_BUTTON_INPUT_PIN4);
    PUSH_BUTTON_STATE5 = digitalRead(PUSH_BUTTON_INPUT_PIN5);
    PUSH_BUTTON_STATE6 = digitalRead(PUSH_BUTTON_INPUT_PIN6);
    PUSH_BUTTON_STATE7 = digitalRead(PUSH_BUTTON_INPUT_PIN7);
    PUSH_BUTTON_STATE8 = digitalRead(PUSH_BUTTON_INPUT_PIN8);
    PUSH_BUTTON_STATE9 = digitalRead(PUSH_BUTTON_INPUT_PIN9);
    PUSH_BUTTON_STATE10 = digitalRead(PUSH_BUTTON_INPUT_PIN10);
    PUSH_BUTTON_STATE11 = digitalRead(PUSH_BUTTON_INPUT_PIN11);
    PUSH_BUTTON_STATE12 = digitalRead(PUSH_BUTTON_INPUT_PIN12);
    PUSH_BUTTON_STATE13 = digitalRead(PUSH_BUTTON_INPUT_PIN13);
    PUSH_BUTTON_STATE14 = digitalRead(PUSH_BUTTON_INPUT_PIN14);
    PUSH_BUTTON_STATE15 = digitalRead(PUSH_BUTTON_INPUT_PIN15);
    PUSH_BUTTON_STATE16 = digitalRead(PUSH_BUTTON_INPUT_PIN16);
    
    if (PUSH_BUTTON_STATE1 == HIGH) {  // Knop ingedrukt..
    if(PUSH_BUTTON_PRESSED == 0){      // Checken of knop niet toevallig net al was ingedrukt..

    BlinkM_setRGB(LED1,0x00,0x00,0xff);    // LED1 wordt blauw.
    PUSH_BUTTON_PRESSED = 1;             // Wordt 1 want er is net een knop ingedrukt..
    }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;  // Counter gaat terug naar af..
    BlinkM_setRGB(LED1,0x00,0x00,0x00); // LED1 wordt weer zwart want knop is niet meer ingedrukt..
  }
    if (PUSH_BUTTON_STATE2 == HIGH) {  
   if(PUSH_BUTTON_PRESSED == 0){   
    // turn LED on:    
    BlinkM_setRGB(LED2,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1; 
   }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0; 
    BlinkM_setRGB(LED2,0x00,0x00,0x00);
  }
  if (PUSH_BUTTON_STATE3 == HIGH) {   
  if(PUSH_BUTTON_PRESSED == 0){  
    // turn LED on:    
    BlinkM_setRGB(LED3,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
    }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0; 
    BlinkM_setRGB(LED3,0x00,0x00,0x00);
  }
    if (PUSH_BUTTON_STATE4 == HIGH) { 
  if(PUSH_BUTTON_PRESSED == 0){     
    // turn LED on:    
    BlinkM_setRGB(LED4,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
    }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0; 
    BlinkM_setRGB(LED4,0x00,0x00,0x00);
  }
  if (PUSH_BUTTON_STATE5 == HIGH) { 
    
    if(PUSH_BUTTON_PRESSED == 0){     
    // turn LED on:    
    BlinkM_setRGB(LED5,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
    }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0; 
    BlinkM_setRGB(LED5,0x00,0x00,0x00);
  }
    if (PUSH_BUTTON_STATE6 == HIGH) {
      if(PUSH_BUTTON_PRESSED == 0){       
    // turn LED on:    
    BlinkM_setRGB(LED6,0x00,0x00,0xff);
  PUSH_BUTTON_PRESSED = 1;
      }  
  } 
  else {
    PUSH_BUTTON_PRESSED = 0; 
    BlinkM_setRGB(LED6,0x00,0x00,0x00);
  }
  if (PUSH_BUTTON_STATE7 == HIGH) { 
    if(PUSH_BUTTON_PRESSED == 0){     
    // turn LED on:    
    BlinkM_setRGB(LED7,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
    }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED7,0x00,0x00,0x00);
  }
    if (PUSH_BUTTON_STATE8 == HIGH) {  
   if(PUSH_BUTTON_PRESSED == 0){   
    // turn LED on:    
    BlinkM_setRGB(LED8,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
   }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED8,0x00,0x00,0x00);
  }
  if (PUSH_BUTTON_STATE9 == HIGH) { 
    if(PUSH_BUTTON_PRESSED == 0){    
    // turn LED on:    
    BlinkM_setRGB(LED9,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
    }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED9,0x00,0x00,0x00);
  }
    if (PUSH_BUTTON_STATE10 == HIGH) {  
     if(PUSH_BUTTON_PRESSED == 0){    
    // turn LED on:    
    BlinkM_setRGB(LED10,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
     }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED10,0x00,0x00,0x00);
  }
  if (PUSH_BUTTON_STATE11 == HIGH) {   
    if(PUSH_BUTTON_PRESSED == 0){    
    // turn LED on:    
    BlinkM_setRGB(LED11,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
    }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED11,0x00,0x00,0x00);
  }
    if (PUSH_BUTTON_STATE12 == HIGH) { 
      if(PUSH_BUTTON_PRESSED == 0){      
    // turn LED on:    
    BlinkM_setRGB(LED12,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
      }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED12,0x00,0x00,0x00);
  }
    if (PUSH_BUTTON_STATE13 == HIGH) {  
       if(PUSH_BUTTON_PRESSED == 0){    
    // turn LED on:    
    BlinkM_setRGB(LED13,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
       }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED13,0x00,0x00,0x00);
  }
    if (PUSH_BUTTON_STATE14 == HIGH) {
     if(PUSH_BUTTON_PRESSED == 0){        
    // turn LED on:    
    BlinkM_setRGB(LED14,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
     }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED14,0x00,0x00,0x00);
  }
    if (PUSH_BUTTON_STATE15 == HIGH) {   
      if(PUSH_BUTTON_PRESSED == 0){  
    // turn LED on:    
    BlinkM_setRGB(LED15,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
      }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED15,0x00,0x00,0x00);
  }
    if (PUSH_BUTTON_STATE16 == HIGH) {
      if(PUSH_BUTTON_PRESSED == 0){       
    // turn LED on:    
    BlinkM_setRGB(LED16,0x00,0x00,0xff); 
    PUSH_BUTTON_PRESSED = 1;
      }
  } 
  else {
    PUSH_BUTTON_PRESSED = 0;
    BlinkM_setRGB(LED16,0x00,0x00,0x00);
  }
  
  // MIDI-IN CODE >>>>>>>>>>>>
  
if (Serial.available() > 0) {
            
            incomingByte = Serial.read();

if (incomingByte== 144){ // Note on midi channel 1
      BlinkM_setRGB(LED1,0x00,0x00,0xff);
    }
    else if (incomingByte== 128){ // Note off midi channel 1
    BlinkM_setRGB(LED1,0x00,0x00,0x00);
   }
}
if (incomingByte== 145){ // Note on midi channel 2
      BlinkM_setRGB(LED2,0x00,0xff,0x00);
    }
    else if (incomingByte== 129){ // Note off midi channel 2
    BlinkM_setRGB(LED2,0x00,0x00,0x00);
      }
if (incomingByte== 146){ // Note on midi channel 3
      BlinkM_setRGB(LED3,0xff,0x00,0x00);
    }
    else if (incomingByte== 130){ // Note off midi channel 3
    BlinkM_setRGB(LED3,0x00,0x00,0x00);
      }
}

system · December 8, 2010, 4:59pm

I want to start using for loops and functions but i can't wrap my head around how to start.

Even before that, you should learn how to use arrays. Arrays are used to group like objects, like pin numbers, pin states, etc.

byte LED1 = 9;
byte LED2 = 10;
byte LED3 = 11;
byte LED4 = 12;
byte LED5 = 13;
byte LED6 = 14;
byte LED7 = 15;
byte LED8 = 16;
byte LED9 = 17;
byte LED10 = 18;
byte LED11 = 19;
byte LED12 = 20;
byte LED13 = 21;
byte LED14 = 22;
byte LED15 = 23;
byte LED16 = 24;

becomes

byte LEDs[] = {9, 10, 11, 12, 13, 14, 15, 16 ,17, 18, 19, 20, 21, 22, 23, 24};

Shorter already.

Do the same for the other like things.

Then, you can loop through the array, and turn all the LEDs on like so:

for(byte i=0; i<16; i++)
{
   digitalWrite(LEDs[i], HIGH);
}

You can read all the switches in a loop the same way.

system · December 9, 2010, 2:15am

sm,

Paul's suggestion is an excellent one. I would add a couple of things:

Variables are not usually named with all capital (uppercase) letters. Some people make the first letter of the name uppercase to show that it's a global variable.
Use the 'sizeof()' operator instead of magic number like '16'.

Incorporating these two suggestions:

byte leds[] = {9, 10, 11, 12, 13, 14, 15, 16 ,17, 18, 19, 20, 21, 22, 23, 24};

for(byte i=0; i<sizeof(leds); i++)
{
   digitalWrite(leds[i], HIGH);
}

Regards,

-Mike

system · December 9, 2010, 2:23am

In my opinion the sizeof operator is not an optimization in readability, extensibility or in execution time.

Consider creating a const variable that indicates the length of the array.

const byte NUMBER_OF_LEDS = 16;

lloyddean · December 9, 2010, 6:38am

No, but it increases maintainability and reduces debug time because the length of the array is figured out at compile time by the compiler and not something the programmer has to remember to change if array entries are added or deleted.

system · December 9, 2010, 7:32am

Until the time comes to refactor the code and make it readable in terms of a clear chain of command and responsibilities. Then you introduce the magic of functions, and then. Where is your sizeof god then?

system · December 9, 2010, 4:11pm

However you implement it, I think we can all agree that magic numbers are bad, mmmmkay?

-Mike

robtillaart · December 9, 2010, 9:56pm

However you implement it, I think we can all agree that magic numbers are bad, mmmmkay?

not when these magic numbers come from the requirements / real world / hardware restrictions etc (in short from outside the program) . I think more in AlphaBeta's direction, but I would use a #define

#define NUMBER_OF_LEDS 16

byte leds[NUMBER_OF_LEDS] = {9, 10, 11, 12, 13, 14, 15, 16 ,17, 18, 19, 20, 21, 22, 23, 24};

for(byte i=0; i<NUMBER_OF_LEDS; i++)
{
   digitalWrite(leds[i], HIGH);
}

As the array is declared 16 in size if will give an error at compile time if I try to stuff too many values in it. Too few is no problem.

error: too many initializers for 'byte [16]'

update: if I need a runtime dynamic array I wrap it in a class

system · December 10, 2010, 9:23am

Too few is no problem

Until you try to debug why pin zero keeps getting written to / read from!

robtillaart · December 10, 2010, 5:30pm

Until you try to debug why pin zero keeps getting written to / read from!

"there is no silver bullet", Mythical Man Month, Fred Brooks

system · December 10, 2010, 6:16pm

Mythical Man Month, Fred Brooks

...was one of my college text books.
Defensive programming techniques have advanced greatly in the last thirty years.

system · December 10, 2010, 11:26pm

robtillaart,

I think the #define or 'const' are both good ways to implement constant values. The important thing is that they document the meaning of the constant. Your example does that.

This example does not:#define SIXTEEN 16

Also, in the code:foo(i,j,16);
the 16 is a "magic number" because you have to be a wizard to understand its meaning. If one does it as you do, and writes

foo(i,j,NUMBER_OF_LEDS);

then even muggles can understand it.

(I know you understand all this, robtillaart, but some readers may not.)

Regards,

-Mike

system · December 11, 2010, 11:06am

Thank you PaulS and everyone else who replied (by all means carry on the discussion ;))
Had a little help from a friend and this is what I got now:

#include "Wire.h"
#include "BlinkM_funcs.h"
#include <MIDI.h>

int number_of_pushpins = 16;
boolean pushbutton_pressed = false;

byte leds[] = {9, 10, 11, 12, 13, 14, 15, 16 ,17, 18, 19, 20, 21, 22, 23, 24}; 
int pushbutton_pins[] = {2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 16, 17, 18, 19, 22};
boolean pushbutton_state[] = {false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false};

void setup() {
   
    Serial.begin(31250);
    BlinkM_begin();
    
    for(int i=0; i< number_of_pushpins; i++){
       pinMode(pushbutton_pins[i], INPUT);
    }
 
}

void loop() {
  
  for(int i=0; i< number_of_pushpins; i++){
       pushbutton_state[i] = digitalRead(pushbutton_pins[i]);
    }


  for(int i=0; i< number_of_pushpins; i++){
       if( pushbutton_state[i] == HIGH) {
         if(pushbutton_pressed == false){ 
           BlinkM_setRGB(leds[i],0x00,0x00,0xff);  
           pushbutton_pressed = true; 
           
         }
       }else{
          pushbutton_pressed = false; 
          BlinkM_setRGB(leds[i],0x00,0x00,0x00);
       }
    }
}

I guess this

 boolean pushbutton_state[] = {false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false};

can be changed into something easier.. not sure yet how.
This has been a great learning exercise so far..awesome

system · December 11, 2010, 1:54pm

Regardless of the method you use to implement the magic number...
For the love of whatever you worship! COMMENT IT!
No matter the method you use, it is a constant, plain and simple (I wouldn't use sizeof, because it may not get compiled into a constant in the final code.. which would be a loss of efficiency, while a comment should've removed the need for sizeof)

As for the pushbuttons, it appears they aren't debounced?
If they really aren't, you could look into creating your own button library (I know, there are multiple floating about allready.. but it isn't too hard to make, and simple enough to use as practice) to fix that, while keeping your current sketch more or less the same.

system · December 11, 2010, 2:15pm

The type of a given variable is determined by how it is used. The variable pushbutton_state is used to hold the output of digitalRead. The digitalRead function returns an int, not a boolean.

When an array is populated by the code before the values in the array are used, as is the case with pushbutton_state, it is not necessary to provide initial values for the array.

When it is necessary to provide initial values, and all the values are the same, provide those values in a for loop in setup().

system · December 11, 2010, 4:18pm

PaulS: i should change it to this?

int pushbutton_state[] = {};

Imahilus: thanks, i will get back to tomorrow

Another question about this part:

}else{
          pushbutton_pressed = false; 
          BlinkM_setRGB(leds[i],0x00,0x00,0x00);
       }

How would i go about making sure that this else part in the loop only runs once? Do i need to do the same as above and use some kind of counter? I only want it run once because i want to implement midi note off there and its enough if it sends a noteoff once.

Same goes with the BlinkM_setRGB(leds*,0x00,0x00,0x00);*
statement, it turns my leds off (black) - but really i don't need it to keep on running forever do i?

system · December 11, 2010, 4:25pm

i should change it to this?

Code:

int pushbutton_state[] = {};

No.

int pushbutton_state[number_of_pushpins];

Also, this:

int number_of_pushpins = 16;

should be this:

[glow]const[/glow] int number_of_pushpins = 16;

This prevents the sketch from altering the value in number_of_pushpins.

system · December 11, 2010, 4:28pm

Do i need to do the same as above and use some kind of counter? I only want it run once because i want to implement midi note off there and its enough if it sends a noteoff once.

Either a counter or a simple flag, midiOffSent, initialized to false, and set false again whenever a MIDI note is sent. Then, test midiOffSent before sending a MIDI off note. Only send the off note if one has not been sent.

system · December 11, 2010, 6:01pm

Thanks, now i'm confused though.
Can you tell in english what this means:

int pushbutton_state[number_of_pushpins];

something like: define an array of type integer that holds the value of the number_of_pushpins (=16) ? i dont get the logic behind it... :-?

system · December 11, 2010, 6:19pm

The value in the [] is the number of elements in the array. When no value is present, the initializer list is used to determine the number of elements.

When there is no initializer list, the number of elements in the array must be explicitly defined.

Topic		Replies	Views
How to optimize this code + setting pins to LOW INPUT before OUTPUT Programming	6	553	May 6, 2021
BlinkM arduino code Syntax & Programs	5	2872	May 6, 2021
Maybe i could get you all to look at my code General Guidance	9	70	January 31, 2025
Coding Guru Needed! Syntax & Programs	4	1194	May 6, 2021
Optimzing Code and pushbutton Resistors General Guidance	9	642	May 5, 2021

Please assist me in optimizing my (beginner) code

Related topics