Optimization

Using Arduino Programming Questions
#1

Hey guys. Still very new to the C language. My program is working, and working exactly the way I expected. However, I can’t help but feel there has to be a way to clean up the code and make it shorter with better use some of C++'s internal functions.
The project: I am using a 6 position switch to select what “loop” it is going to do… each loop does different LED tricks
The code:

// Name LED pins
int led1 = 3;
int led2 = 5;
int led3 = 6;
int led4 = 7;
int led5 = 8;
int led6 = 9;
int led7 = 10;
int led8 = 11;

// Setup LED pins for loops

int ledArray[] = {3, 5, 6, 7, 8, 9, 10, 11};
int count = 0;
int brightness = 0;

void setup() {
  // Define Inputs 
  pinMode(0, INPUT);
  pinMode(1, INPUT);
  pinMode(2, INPUT);
  pinMode(4, INPUT);
  pinMode(12, INPUT);
  pinMode(13, INPUT);
  // Define Outputs
  pinMode(3, OUTPUT); //PWM
  pinMode(5, OUTPUT); //PWM
  pinMode(6, OUTPUT); //PWM
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(9, OUTPUT); //PWM
  pinMode(10, OUTPUT); //PWM
  pinMode(11, OUTPUT); //PWM  
}

void loop() {
  
  //Read Switch Position
  int switchPos1 = digitalRead(0);
  int switchPos2 = digitalRead(1);
  int switchPos3 = digitalRead(2);
  int switchPos4 = digitalRead(4);
  int switchPos5 = digitalRead(12);
  int switchPos6 = digitalRead(13);
  
  // Read the pots
  int leftPot = analogRead(A0);
  int rightPot = analogRead(A1);
  
  // Read current from speaker
  int speaker = analogRead(A2);
  
  if (switchPos1 == LOW) {  // Check if in position 1
  
            /* Knight Rider Effect - with delay: (LED Chaser)
            The left pot will control delay for each light.
            The right pot will control delay after each cycle.  */
            
            
            leftPot = map(leftPot, 0, 1023, 5, 75);
            rightPot = map(rightPot, 0, 1023, 10, 50);
            for (count=0;count<7;count++) {
             digitalWrite(ledArray[count], HIGH);
             delay(leftPot);
             digitalWrite(ledArray[count + 1], HIGH);
             delay(leftPot);
             digitalWrite(ledArray[count], LOW);
             delay(rightPot);
            }
            for (count=7;count>0;count--) {
             digitalWrite(ledArray[count], HIGH);
             delay(leftPot);
             digitalWrite(ledArray[count - 1], HIGH);
             delay(leftPot);
             digitalWrite(ledArray[count], LOW);
             delay(rightPot);
            }

     
  } else if (switchPos2 == LOW) { // Check if in position 2
 
         /* Alt Chaser
         if the Pot on the right is turned CCW,
         then the LEDs chase left, CW they chase
         right, in the middle, chase from
         inside out */
         
          leftPot = map(leftPot, 0, 1023, 10, 75);
          rightPot = map(rightPot, 0, 1023, 1, 100);
          if (rightPot <= 30) {
          for (count=0;count<7;count++) {
           digitalWrite(ledArray[count], HIGH);
           delay(leftPot);
           digitalWrite(ledArray[count + 1], HIGH);
           delay(leftPot);
           digitalWrite(ledArray[count], LOW);
           delay(leftPot);
          }
          } else if(rightPot >= 61) {
          for (count=7;count>0;count--) {
           digitalWrite(ledArray[count], HIGH);
           delay(leftPot);
           digitalWrite(ledArray[count - 1], HIGH);
           delay(leftPot);
           digitalWrite(ledArray[count], LOW);
           delay(leftPot);
          }
          } else {
            digitalWrite(led4, HIGH);
            digitalWrite(led5, HIGH);
            delay(leftPot * 4);
            digitalWrite(led4, LOW);
            digitalWrite(led5, LOW);
            digitalWrite(led3, HIGH);
            digitalWrite(led6, HIGH);
            delay(leftPot * 4);
            digitalWrite(led3, LOW);
            digitalWrite(led6, LOW);
            digitalWrite(led2, HIGH);
            digitalWrite(led7, HIGH);
            delay(leftPot * 4);
            digitalWrite(led2, LOW);
            digitalWrite(led7, LOW);
            digitalWrite(led1, HIGH);
            digitalWrite(led8, HIGH);
            delay(leftPot * 4);
            digitalWrite(led1, LOW);
            digitalWrite(led8, LOW);
            
            
          }
  
  } else if (switchPos3 == LOW) { // Check if in position 3
          // Just a goofy
          leftPot = map(leftPot, 0, 1023, 5, 75);
          rightPot = map(rightPot, 0, 1023, 1, 100);
          if (rightPot <= 30) { // One Direction
          digitalWrite(led1, HIGH);
          delay(leftPot);
          digitalWrite(led2, HIGH);
          delay(leftPot);
          digitalWrite(led3, HIGH);
          delay(leftPot);
          digitalWrite(led4, HIGH);
          delay(leftPot);
          digitalWrite(led5, HIGH);
          delay(leftPot);
          digitalWrite(led6, HIGH);
          delay(leftPot);
          digitalWrite(led7, HIGH);
          delay(leftPot);
          digitalWrite(led8, HIGH);
          delay(leftPot);
          digitalWrite(led1, LOW);
          delay(leftPot);
          digitalWrite(led2, LOW);
          delay(leftPot);
          digitalWrite(led3, LOW);
          delay(leftPot);
          digitalWrite(led4, LOW);
          delay(leftPot);
          digitalWrite(led5, LOW);
          delay(leftPot);
          digitalWrite(led6, LOW);
          delay(leftPot);
          digitalWrite(led7, LOW);
          delay(leftPot);
          digitalWrite(led8, LOW);
          delay(leftPot);
          
          } else if(rightPot >= 61) { // The other direction
          digitalWrite(led8, HIGH);
          delay(leftPot);
          digitalWrite(led7, HIGH);
          delay(leftPot);
          digitalWrite(led6, HIGH);
          delay(leftPot);
          digitalWrite(led5, HIGH);
          delay(leftPot);
          digitalWrite(led4, HIGH);
          delay(leftPot);
          digitalWrite(led3, HIGH);
          delay(leftPot);
          digitalWrite(led2, HIGH);
          delay(leftPot);
          digitalWrite(led1, HIGH);
          delay(leftPot);
          digitalWrite(led8, LOW);
          delay(leftPot);
          digitalWrite(led7, LOW);
          delay(leftPot);
          digitalWrite(led6, LOW);
          delay(leftPot);
          digitalWrite(led5, LOW);
          delay(leftPot);
          digitalWrite(led4, LOW);
          delay(leftPot);
          digitalWrite(led3, LOW);
          delay(leftPot);
          digitalWrite(led2, LOW);
          delay(leftPot);
          digitalWrite(led1, LOW);
          delay(leftPot);
          } else { // inside to out
          digitalWrite(led4, HIGH);
          delay(leftPot);
          digitalWrite(led5, HIGH);
          delay(leftPot);
          digitalWrite(led3, HIGH);
          delay(leftPot);
          digitalWrite(led6, HIGH);
          delay(leftPot);
          digitalWrite(led2, HIGH);
          delay(leftPot);
          digitalWrite(led7, HIGH);
          delay(leftPot);
          digitalWrite(led1, HIGH);
          delay(leftPot);
          digitalWrite(led8, HIGH);
          delay(leftPot);
          digitalWrite(led4, LOW);
          delay(leftPot);
          digitalWrite(led5, LOW);
          delay(leftPot);
          digitalWrite(led3, LOW);
          delay(leftPot);
          digitalWrite(led6, LOW);
          delay(leftPot);
          digitalWrite(led2, LOW);
          delay(leftPot);
          digitalWrite(led7, LOW);
          delay(leftPot);
          digitalWrite(led1, LOW);
          delay(leftPot);
          digitalWrite(led8, LOW);
          delay(leftPot);
          }
#2

The Code (continued):

} else if (switchPos4 == LOW) { // Check if in position 4

          leftPot = map(leftPot, 0, 1023, 5, 75);
          rightPot = map(rightPot, 0, 1023, 10, 50);
    
          digitalWrite(led1, HIGH);
          digitalWrite(led8, HIGH);
          delay(leftPot);
          digitalWrite(led1, LOW);
          digitalWrite(led8, LOW);
          digitalWrite(led2, HIGH);
          digitalWrite(led7, HIGH);
          delay(leftPot);
          digitalWrite(led2, LOW);
          digitalWrite(led7, LOW);
          digitalWrite(led3, HIGH);
          digitalWrite(led6, HIGH);
          delay(leftPot);
          digitalWrite(led3, LOW);
          digitalWrite(led6, LOW);
          digitalWrite(led4, HIGH);
          digitalWrite(led5, HIGH);
          delay(leftPot);
          digitalWrite(led4, LOW);
          digitalWrite(led5, LOW);
          digitalWrite(led3, HIGH);
          digitalWrite(led6, HIGH);
          delay(leftPot);
          digitalWrite(led3, LOW);
          digitalWrite(led6, LOW);
          digitalWrite(led2, HIGH);
          digitalWrite(led7, HIGH);
          delay(leftPot);
          digitalWrite(led2, LOW);
          digitalWrite(led7, LOW);
          digitalWrite(led1, HIGH);
          digitalWrite(led8, HIGH);
          delay(leftPot);
          digitalWrite(led1, LOW);
          digitalWrite(led8, LOW);
    

  } else if (switchPos5 == LOW) { // Check if in position 5
  
         // Random Light Show
         randomSeed(analogRead(A5));
         
         int randNumber = random(8);
         leftPot = map(leftPot, 0, 1023, 5, 200);
         rightPot = map(rightPot, 0, 1023, 10, 100);
         digitalWrite(ledArray[randNumber], HIGH);
         delay(leftPot);
         digitalWrite(ledArray[randNumber], LOW);
         delay(rightPot);
   
  
  } else if (switchPos6 == LOW) { // Check if in position 6
  
          /* VU METER
          Using a little speaker to in front om my subs
          port to generate a small amount of current, 
          and make the lights flash to the bass */
          
          // Adjust speaker and pwm sensitivity
          int speakerVal = map(speaker, 0, 1023, 0, 254);
          int leftVal = map(leftPot, 0, 1023, 1, 20);
          speaker = (speakerVal * leftVal);
          int rightVal = map(rightPot, 0, 1023, 1, 20);
          speakerVal = (speakerVal * rightVal);
          
          if (speaker >= 10) {
            digitalWrite(led4, HIGH);
            digitalWrite(led5, HIGH);
              if (speaker >= 25 ) {
               analogWrite(led3, speakerVal);
               analogWrite(led6, speakerVal);
                 if (speaker >= 50 ) {
                   analogWrite(led2, speakerVal);
                   analogWrite(led7, speakerVal);
                     if (speaker >= 100 ) {
                       analogWrite(led1, speakerVal);
                       analogWrite(led8, speakerVal);
                     }
                 }
              }   
            }    
            }
            
            if (speaker == 0) {
            digitalWrite(led3, LOW);
            digitalWrite(led4, LOW);
            analogWrite(led1, 0);
            analogWrite(led2, 0);
            analogWrite(led5, 0);
            analogWrite(led6, 0);
            analogWrite(led7, 0);
            analogWrite(led8, 0);
            }

  }
#3 
// Name LED pins
int led1 = 3;
int led2 = 5;
int led3 = 6;
int led4 = 7;
int led5 = 8;
int led6 = 9;
int led7 = 10;
int led8 = 11;

// Setup LED pins for loops

int ledArray[] = {3, 5, 6, 7, 8, 9, 10, 11};

  // Define Outputs
  pinMode(3, OUTPUT); //PWM
  pinMode(5, OUTPUT); //PWM
  pinMode(6, OUTPUT); //PWM
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(9, OUTPUT); //PWM
  pinMode(10, OUTPUT); //PWM
  pinMode(11, OUTPUT); //PWM

Name those pins, but don't use the names. OK. Whatever.

  } else if (switchPos5 == LOW) { // Check if in position 5
  
         // Random Light Show
         randomSeed(analogRead(A5));

The randomSeed() function should be called once, not every pass through loop.

However, I can't help but feel there has to be a way to clean up the code and make it shorter with better use some of C++'s internal functions.

Not really. You have an array of pin numbers that you never use, which could shorten the code a little bit. You could create some functions for each of the blocks of code (one for each switch), but that would reduce the amount of code. It would make it easier to read/maintain, though.

#4

The random part, I changed it to better suit what I was looking for, and don't use randomSeed() at all now.

int randNumber;
         int n;
         leftPot = map(leftPot, 0, 1023, 0, 200);
         rightPot = map(rightPot, 0, 1023, 0, 1000);
         
         for(n=random(8)+1;n>0,n--;) {
           randNumber = random(8);
           delay(leftPot);
           digitalWrite(ledArray[randNumber], HIGH);

         }
          delay(rightPot);

And

Name those pins, but don't use the names. OK. Whatever.

} else {
            digitalWrite(led4, HIGH);
            digitalWrite(led5, HIGH);
            delay(leftPot * 4);
            digitalWrite(led4, LOW);
            digitalWrite(led5, LOW);
            digitalWrite(led3, HIGH);
            digitalWrite(led6, HIGH);
            delay(leftPot * 4);
            digitalWrite(led3, LOW);
            digitalWrite(led6, LOW);
            digitalWrite(led2, HIGH);
            digitalWrite(led7, HIGH);
            delay(leftPot * 4);
            digitalWrite(led2, LOW);
            digitalWrite(led7, LOW);
            digitalWrite(led1, HIGH);
            digitalWrite(led8, HIGH);
            delay(leftPot * 4);
            digitalWrite(led1, LOW);
            digitalWrite(led8, LOW);
            
            
          }

Oops. I guess I do use them... I could change that code to (ledArray[?], HIGH); but that seems to be just more typing to the same effect. As for the functions, I just saw an example of that and will try that out. Thanks for the reply. The function thing will rock if I get it working right.

#5

The random part, I changed it to better suit what I was looking for, and don't use randomSeed() at all now.

If what you were looking for was the same set of random numbers to be generated, you have definitely achieved that goal. If not, you want to call randomSeed(), once, at some point.

Oops. I guess I do use them... I could change that code to (ledArray[?], HIGH); but that seems to be just more typing to the same effect.

If you are not going to use the array, why define it? I don't see that it is all that useful with such a small number of elements. On the other hand, all the constant pin numbers, in the pinMode and digitalRead statements have go to go. What happens if you need to change a pin?

The function thing will rock if I get it working right.

Yes, it will.

#6

mabvs:
However, I can’t help but feel there has to be a way to clean up the code and make it shorter with better use some of C++'s internal functions.

Exactly - you’re thinking of writing a helper function to manage the clutter. It’s no problem, just look at what you want to do on many of the lines:

  • turn on or off as many as four items at a time
  • delay (usually)

So what if you had a function like this:

void digitalDelayAndSet(long delayTime,int a1,int h1=HIGH,int a2=-1,int h2=HIGH,int a3=-1,int h3=HIGH,int a4=-1,int h4=HIGH)
{
  int array[]={a1,a2,a3,a4};
  int hiLo[]={h1,h2,h3,h4};
  for (int i=0;i<sizeof(array)/sizeof(array[0]);++i)
  {
    if (array[i]<0)
      break;
    digitalWrite(array[i],(HIGH==hiLo[i]?HIGH:LOW));
  }    
  if (delayTime>0)
    delay(delayTime);
}

It looks complicated, but all it does is take in a delay and up to four digitalWrite() values, and then writes them and then delays.

So for example, where you have:

digitalWrite(led4, LOW);
digitalWrite(led5, LOW);
digitalWrite(led3, HIGH);
digitalWrite(led6, HIGH);
delay(leftPot * 4);

You would instead call:

digitalDelayAndSet( (leftPot * 4), led4,LOW, led5,LOW, led3,HIGH, led6,HIGH );

The reason the delay is in front instead of behind is because of the odd “int h1=HIGH” style parameters in the function call, these are called default parameters, and are passed in if you don’t enter anything. So for example, where you call:

digitalWrite(led1, HIGH);
digitalWrite(led8, HIGH);
delay(leftPot);

You only need to enter two values - the rest are automatically filled in and ignored by the function:

digitalDelayAndSet( leftPot, led1,HIGH, led8,HIGH );

The only exception is the delay - you always have to fill that in, but if you don’t need it, just use 0 - for example,

digitalWrite(led1, LOW);
digitalWrite(led8, LOW);

becomes this:

digitalDelayAndSet( 0, led1,LOW, led8,LOW );

I’ve written and compiled it on the arduino, but haven’t tested it - so if you do use the function, change over just a few lines at a time and see if it works as you expect.

In any case, whether you use it or not, your instincts were correct - C/C++ gives you a way to group repetitive items into a function, (hopefully) making your code easier to use.

#7

You can also use Tabs in your .pde file, spreads the code in chunks horizontally, so you don't need to spend so much time scrolling up & down the screen. Makes it easier for you to edit, call the tabs something like: a-presetup - define your pin assignments & variables here b-setup - put pinModes, Serial.begin, etc here c-loop-a - 1st third of code, split up as convenient for you d-loop-b - 2nd third of code, or maybe this is wher all your functions are e-loop-c - final-third of code, etc.

I have a 15000 byte program spread across 7 tabs, works out well.

#8

wow… GREAT info utopia. I could not even finish your post without having my project open and starting it. Exciting!
And Crossroad… wow… I wish I had figured that out before I wore out 50% of my mouse wheels life. That does make it so much quicker and just better organized.

And paul… I do appreciate the help, but as for halving the “same random set of numbers”… I agree that according to the documentation on the site, randomSeed is a must, however, the flashing LEDs I am staring at as I write this seem to disagree. I added the randomSeed(A5) above the void setup() where I declared all those “unnecessary” ints. It made no difference, and the LEDs seem to flash very randomly with or without it…The only thing I wish I know how to do better would be that if it picked say 5 (to light up 5 LEDs), that it could not pick the same LED in that five (meaning that it could not pick ledArray[0], ledArray[3], ledarray[0], ledarray[7], ledarray[4]).

#9

@ utopia,

Wow, quite a generic function you proposed, some remarks,

  • make the delayTime unsigned long => test >0 not needed
  • the HighLOW test is not needed
  • name should be SetAndDelay to reflect behaviour (OK a bit purist)
  • move the break test into the for clause.
  • as the array is fixed size you could hard code its size in the for clause (i < 4 ; not done)
void digitalSetAndDelay(unsigned long delayTime,int a1,int h1=HIGH,int a2=-1,int h2=HIGH,int a3=-1,int h3=HIGH,int a4=-1,int h4=HIGH)
{
  int array[]={a1,a2,a3,a4};
  int hiLo[]={h1,h2,h3,h4};
  for (int i=0;i<sizeof(array)/sizeof(array[0]) && array[i] >= 0; ++i)   //
  {
   digitalWrite(array[i], hiLo[i]);
  }
  delay(delayTime);
}

recognizing repeating parts to isolate reusable functions is a good thing. If you look closer to the code of the OP you see a repeating pattern in post #3 - mabvs else part

            digitalWrite(led4, HIGH);
            digitalWrite(led5, HIGH);
            delay(leftPot * 4);
            digitalWrite(led4, LOW);
            digitalWrite(led5, LOW);

then the same with led3 and 6
then led2 and 7
then led1 and 8

which makes this function:

void LED(int l1, int l2, int del)
{
  digitalWrite(l1, HIGH);
  digitalWrite(l2, HIGH);
  delay(del);
  digitalWrite(l1, LOW);
  digitalWrite(l2, LOW);
}

Not as powerfull/flexibel as yours but one that does the job

#10

It made no difference, and the LEDs seem to flash very randomly with or without it.

If you were to print out the values, though, you'd see that every time the Arduino was restarted, without the call to randomSeed(), the same set of random numbers was generated.

Most of the time, it doesn't matter. Who's going to notice? But, if you are developing a game that relied on random(), without the use of randomSeed(), the game would always play the same way.

#11

Well actually, what I am trying to figure out now is how to create a function where I call on it, and it would do a loop with however many argument I send.

i.e.

ledFunct(led1, led8);
// OR
ledFunct(led2, led5, led6);

void ledFunct() {  // 
//do
 digitalWrite(ledArray[?], HIGH); // for each arg passed,
delay(leftPot *4);
// then 
digitalWrite(ledArray[?], LOW; // for every HIGH

EDIT, Paul.. I see what your saying. Not critical in my project, but good habit as it may be needed for another project. Added for sake of doing shit right :D

#12

But, if you are developing a game that relied on random(), without the use of randomSeed(), the game would always play the same way.

Not necessary true, even without using randomSeed() you can introduce randomness in a game IIF 1) you have to wait for userinput at a certain moment and 2) can allow to do it in a polling way.

You need to include something like the snippet below at every user interaction. The predefined sequence will be broken in a undefined way as the # calls to random() becomes less defined unless the user has microsecond timing. This trick makes the calls to random() at the relevant places in the game in essence non-deterministic. OK there is a performance penalty.

Serial.println("press key");
while(digitalRead(KEY) == HIGH) random();

A "no delay" version is possible, but it will call random() probably less often than in the active waiting version, ==> introducing less entropy, so chances of repeating numbers is greater.

if (digitalRead(KEY) == LOW)
{
  ... do things
} else random();
#13

There was another interesting posting thread yesterday dealing with finding a optimum source for use with the randomSeed() function. Seems the consensus so far is that there is yet to be identified a 'good enough' source for automatic means of generating the initial seed value that doesn't rely on a users manually 'hit any key' type of function.

To paraphrase Coding Badly, the search continues...........

A lot depends on one's own definition of what 'good enough' means of course.

Lefty

#14

A lot depends on one's own definition of what 'good enough' means of course.

Agree, but "good enough" often tends towards mediocracy, see - http://lib.store.yahoo.net/lib/demotivators/mediocritydemotivationalposter.jpg - ;)

#15

mabvs:
wow… GREAT info utopia. I could not even finish your post without having my project open and starting it. Exciting!

I’m delighted I could help (note: sorry for the confusion in names here, I just changed from ‘utopia’ to my real name)

Just some other comments:

#1: On the topic of random, I was asking a similar question on another part of the forum:

http://arduino.cc/forum/index.php/topic,66060.0.html

As retrolefty mentioned, they came up with some great ideas, and I summarized the best one on my site:

http://www.utopiamechanicus.com/77/better-arduino-random-numbers/

It was so good it replaced the one I was originally using - it’s really simple to implement, and for your own projects, quite a good range of values.

In your case, just randomSeed() in the setup function, and then use random() from then on.

#2: For your problem of non-repeating items, one way is to use an array of items, and then shuffle them randomly. Once that is done, then you are guaranteed that the values are all unique (sort of like shuffling cards in a deck). So for example:

int pins[]={ led1, led2, led3, led4, led5, led6, led7 };
int total=sizeof(pins)/sizeof(pins[0]);
int i=0, k=0, temp=0;
for (i=0;i<total;++i)
{
  // get random index
  k=random(0,total);
  // swap pick, item i
  temp=pins[i];
  pins[i]=pins[k];
  pins[k]=temp;
}

by the end, the pins array is shuffled, with no duplicates. Use them in order (pins[0] then pins[1] etc) and you end up with the lights coming on in random order.

#16