Go Down

Topic: Please assist me in optimizing my (beginner) code (Read 3575 times) previous topic - next topic

stereomuffin

Dec 08, 2010, 05:46 pm Last Edit: Dec 08, 2010, 05:49 pm by stereomuffin Reason: 1
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 :)

Code: [Select]

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

PaulS

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

Code: [Select]
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
Code: [Select]
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:
Code: [Select]
for(byte i=0; i<16; i++)
{
  digitalWrite(LEDs[i], HIGH);
}


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

Mike Murdock

sm,

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

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

2. Use the 'sizeof()' operator instead of magic number like '16'.

Incorporating these two suggestions:
Code: [Select]
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

AlphaBeta

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.
Code: [Select]
const byte NUMBER_OF_LEDS = 16;

lloyddean

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.

AlphaBeta

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? ;)

Mike Murdock

#6
Dec 09, 2010, 05:11 pm Last Edit: Dec 09, 2010, 05:12 pm by mfm9 Reason: 1
However you implement it, I think we can all agree that magic numbers are bad, mmmmkay? :)

-Mike

robtillaart

#7
Dec 09, 2010, 10:56 pm Last Edit: Dec 09, 2010, 10:59 pm by robtillaart Reason: 1
Quote
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

Code: [Select]

#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
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

AWOL

Quote
Too few is no problem

Until you try to debug why pin zero keeps getting written to / read from!
"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.

robtillaart

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


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

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

AWOL

Quote
Mythical Man Month, Fred Brooks

...was one of my college text books.
Defensive programming techniques have advanced greatly in the last thirty years.
"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.

Mike Murdock

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:
Code: [Select]
#define SIXTEEN 16:)

Also, in the code:
Code: [Select]
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

Code: [Select]
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

stereomuffin

#12
Dec 11, 2010, 12:06 pm Last Edit: Dec 11, 2010, 01:36 pm by stereomuffin Reason: 1
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:

Code: [Select]

#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

Code: [Select]
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  :)

Imahilus

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.

PaulS

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().

Go Up