optimizing if statement infested code

i hooked up a sonar module, then i read the value and if it was in a certain range then it would play a tone according to how far away a object is, in this case my hand. :slight_smile: the spkr pin is connected to a LM386, and then to a 8ohm speaker.

  1. but the code has way too many if statements and that is a sign for bad coding. i wanted to use a “break” inside each “if” so that it would skip the rest of the "if"s and loop again, but they dont work in void loop.
    what is a good way to slim this code down?
#define trig 7 //trigger pin on sonar module
#define echo 6 //echo pin
#define spkr 4 //->lm386 amp->8ohm speaker
#define led 12
int dist; //how far the object is away from the module(cm)
float valueSensor=0;

//notes to play when sonar in certain range
float pitchTable[14] = {
  392.00, // G4
  415.30, // G#4/Ab4
  440.000000, // A
  466.163757, // A#/Bb
  493.83301, // B
  523.251160, // C
  554.365234, // C#/Db
  587.329529, // D
  622.253967, // D#/Eb
  659.255127, // E
  698.456482, // F
  739.988831, // F#/Gb
  783.990845, // G
  830.609375 // G#/Ab
};

void setup() 
{
  pinMode(echo, INPUT);
  pinMode(trig, OUTPUT);
  //  Serial.begin(9600);
}

void loop() 
{
  //send trigger pulse to sonar
  digitalWrite(trig, HIGH);
  delay(1);
  digitalWrite(trig, LOW);
  //recieve echo back
  valueSensor= pulseIn(echo, HIGH);
  dist= valueSensor/58;

  //  Serial.print("dist: ");
  //  Serial.println(dist);
  //  Serial.println("");

  //if distance is greater than 5 and less than 10 play tone 1 in array
  if(dist>5 && dist<10)
  {
    tone(spkr,pitchTable[1]);    
  }
  //...same but different values of sonar range to play tone
  if(dist>10 && dist<15)
  {
    tone(spkr,pitchTable[2]);
  } 

  //...more if statements with different tones and sonar ranges...

  if(dist>75)
  {
    noTone(spkr);
  }
  //delay(25);
}
  1. also another question. i cant get the LM386 to change the volume. how do i connect the pot? which of the 3 pins goes to the amp in, which comes from the arduino etc. pot looks like: and scheme for lm386 attached.
    (i know its the programming section, but no sense making two posts for another quick question)

  2. EDIT\\ this is really the last question. can i use comparing signs like < and > in switch case?

switch (dist)
  {
  case >5 && <10:
    tone(spkr,pitchTable[1]);
    break;

that doesnt work

FNI71U9FWMO3JOC.png

sirbow2:
i wanted to use a “break” inside each “if” so that it would skip the rest of the "if"s and loop again, but they dont work in void loop.

Use “return” instead of “break”.

can i use comparing signs like < and > in switch case

What happened when you tried? But anyway, the answer is “no”.

Re the pot, every pot I’ve seen has the wiper as the centre pin. So that goes to pin 3 on the opamp, the other two go to GND and VIN, electrically it doesn’t matter which is which although it will reverse the direction.

Re the code, I assume you have a lot of these

  if(dist>5 && dist<10)
  {
    tone(spkr,pitchTable[1]);    
  }

The three variables could be placed in an array and a loop used to do all the tests, like this

byte vals [][3] = {
   // lo, hi, pitch
   {5, 10, 1},
   .....
}

for (int i = 0; i < n_vals; i++) {
 
    if (dist > vals[i][0] && dist < vals[i][1]) {
       tone(spkr, pitchTable[vals[i][2]]); 
       return;
    }
}

One loop and one table no matter how many tests.


Rob

but the code has way too many if statements and that is a sign for bad coding. i wanted to use a “break” inside each “if” so that it would skip the rest of the "if"s and loop again, but they dont work in void loop.
what is a good way to slim this code down?

I disagree that a lot of if statements is a sign of bad coding. Your premise that you need a break is what is faulty. What you need is to use if/else if/else.

  if(dist>5 && dist<10)
  {
    tone(spkr,pitchTable[1]);    
  }
  //...same but different values of sonar range to play tone
  if(dist>10 && dist<15)
  {
    tone(spkr,pitchTable[2]);
  }

This code has some problems. What happens if dist IS 10? Nothing.

if(dist < 10)
   // Do the close thing
else if(dist < 15)
   // Do the not quite so close thing
else if(dist < 20)
   // Etc
else
   // "It" is a long way off

Each if statement will be evaluated UNTIL one becomes TRUE. The rest will then be skipped.

Notice that it is not necessary to test both ends of the range each time. If the distance is less than 10, that case will have been handled, so it is not necessary to test that the distance is greater than 10 and less than 15.

Re the pot, every pot I’ve seen has the wiper as the centre pin. So that goes to pin 3 on the opamp, the other two go to GND and VIN, electrically it doesn’t matter which is which although it will reverse the direction.

yeah thats how i have it hooked up, no volume control still.

byte vals [3] = {
// lo, hi, pitch
{5, 10, 1},

}

for (int i = 0; i < n_vals; i++) {

if (dist > vals_[0] && dist < vals*[1]) {_
_ tone(spkr, pitchTable[vals[2]]);
return;
}
}[/quote]
i dont understand this snippit. you seem to have only 5 and 10, the sonar can go up to 70cm. each 5cm range is a different tone. maybe im just looking at it wrong
thanks PaulS
i have this now, and it works alot better:
*_</em> <em><em>*if(dist<=10)   {     tone(spkr,pitchTable[1]);   } //more else if statements.   else if(dist<=15)   {     tone(spkr,pitchTable[2]);   }*</em></em> <em>_**
i added "="s, changed some to "else if"s, and removed the second test of each if._

you seem to have only 5 and 10, the sonar can go up to 70cm

That's why there's ...... in the array setup, you fill in the blanks. Pauls' code only goes to 20 but you've figured that out.

No matter, Pauls is right of course about not needing to test for both values and my loop could be changed to reflect that. I have a deep-seated hate of multiple if statements, it's not a technical issue just aesthetic, once things get to maybe 2-3 if elses I start looking for another way. If your steps are 5 and range 70 then you'll have ~14 if/elses. Too many for my liking but it will work.

If the value sent to pitchTable() is linear and the steps are linear you could probably do the whole thing algorithmically.


Rob

i got a video of my results here: http://dduino.blogspot.com/2011/10/ultrasonic-thermin.html

Very nice! But here:

//if distance is less than 10cm play tone 1 in array
  if(dist&lt;=10)
  {
    tone(spkr,pitchTable[1]);
  }

  // if not less than 10cm, and distance is less than 15cm,
  //play 2nd tone in array
  else if(dist&lt;=15)
  {
    tone(spkr,pitchTable[2]);
  }

You are out by one everywhere. The first tone is index 0, not 1 (and so on).

Also:

  tone(spkr,pitchTable[14]);

That entry doesn’t exist, hence the noise you are getting. They go from 0 to 13.

Wouldn’t this be a lot simpler? …

#define trig 7 //trigger pin on sonar module
#define echo 6 //echo pin
#define spkr 4
#define led 12
int dist; //how far the object is away from the module(cm)
float valueSensor=0;

//tones array which holds the frequency to play.
float pitchTable[] = {
  392.00, // G4
  415.30, // G#4/Ab4
  440.000000, // A
  466.163757, // A#/Bb
  493.83301, // B
  523.251160, // C
  554.365234, // C#/Db
  587.329529, // D
  622.253967, // D#/Eb
  659.255127, // E
  698.456482, // F
  739.988831, // F#/Gb
  783.990845, // G
  830.609375 // G#/Ab
};

#define NUMITEMS(arg) (sizeof (arg) / sizeof (arg [0]))

void setup()
{
  pinMode(echo, INPUT);
  pinMode(trig, OUTPUT);
}

void loop()
{
  //send a trigger signal
  digitalWrite(trig, HIGH);
  delay(1);
  digitalWrite(trig, LOW);
  //receive the trigger signal echo, and calculate cm to the object
  valueSensor= pulseIn(echo, HIGH);
  dist= valueSensor/58;

  // each 5 positions represents a different note
  byte i = dist / 5;
  
  // play note if in range
  if (i < NUMITEMS (pitchTable))
     tone(spkr,pitchTable[i]);
  else
    noTone(spkr);
  //delay(25);
}

yeah thats how i have it hooked up, no volume control still.

Then you haven’t got it wired like you show in that diagram. There is no way that will not change the volume unless you have the ground not connected. Or some other odd mistake.

is this right for the POT? http://1.bp.blogspot.com/-jXemtZEffu0/Tqxft58kp1I/AAAAAAAAA2M/jCVPFQO8FqM/s1600/Picture+002.jpg (note the whole pot is 4 bread board rows wide, so the pins dont line up, but i bent the middle one so that it lines up)

and i still get that weird clicking distortion when nothing is in front of the sensor, andi can also hear it when i play any note but the lowest note. i tried nick gammons code too. also its not pitch tone whatever related. if i play the lowest note->no distortions, but the next note up would distort. if i remove the lowest note from the array and play the lowest note(which was the "next note up" from the previous run) again->no distortions if i go back to my orignal code with only if statements, i dont have any distortions.

and uhh, can you explain the "#define NUMITEMS(arg) (sizeof (arg) / sizeof (arg [0]))" stuff or point me to a reference, i didnt see any of the arduino site

The NUMITEMS is just a define that works out the number of elements in an array. It divides the total size (eg. 20 bytes) by the size of one element (eg. 2) to give the number of items (eg. 10).

That way you can let the compiler work out for you how big the array is without having to count them up (and possibly getting it wrong).

thanks, i tinkered with my previous post a bit. re read if you wish. :)

is this right for the POT?

It's not clear (to me anyway) exactly where the left wire goes. To the 328 VCC pin I think.

What I don't see is any other connection to VCC for the processor, or is there one that's not clear?

There's also no decoupling directly near the processor.


Rob

if i look at the pot in the orientation of that pic, going L->R. digital4, LM386 pin3, gnd all the VCC for the processor are there.

There's also no decoupling directly near the processor. what? decoupling?

Yep, I see it now.

Still no decoupling although those breadboards have so much capacitance you can get away with it.

Can you give me the model/brand of that tiny crystal?


Rob

sirbow2: what? decoupling?

A 0.1 uF cap between pin 7 (VCC) and Gnd, preferably right next to the processor. Otherwise it might get a bit too excited. Also between pin 20 (AVCC) and Gnd, and pin 21 (AREF) and Gnd.

ECS D 160 http://www.ecsxtal.com/store/pdf/partnumber4.pdf

what? decoupling?

A 0.1uF capacitor across the power pins of the chip, as close as you can get.

Thanks for the XTAL info.


Rob

ok so with the sound distortion issue. my tests:
-not caused by the amp(hooked speaker directly to arduino to test)

-if have “if” code like this it works fine(if statements with both greater than and less than, NO “equal to”):

  if(dist>5 && dist<10)
  {
    tone(spkr,pitchTable[1]);
  }

  if(dist>10 && dist<15)
  {
    tone(spkr,pitchTable[2]);
  }

-if i have it like this it deosnt work(just if statements with only less-than-or-equal-to):

if(dist<=10)
  {
    tone(spkr,pitchTable[0]);
  }
  if(dist<=15)
  {
    tone(spkr,pitchTable[1]);
  }

-Nick Gammons code doesnt work:

#define NUMITEMS(arg) (sizeof (arg) / sizeof (arg [0]))

 // each 5 positions represents a different note
  byte i = dist / 5;

  if (i < NUMITEMS (pitchTable))
  {
    tone(spkr,pitchTable[i]);
  }
  else
  {
    noTone(spkr);
  }

-this doesnt work(if else with greater than and less than restrictions on distance):

if(dist<=10 && dist>=3)
  {
    tone(spkr,pitchTable[0]);
  }
  // if not less than 10cm, then if distance is less than 15cm,
  //play 2nd tone in array
  else if(dist<=15 && dist>10)
  {
    tone(spkr,pitchTable[1]);
  }

By “doesnt work”, i mean it has distortions

ahh i believe i solved it! tNick Gammons code is so efficient compared to my first code, that it repeats too quickly. i added a 100msec delay at the end of void loop, and it works great. but i still cant get that volume to work…Added a .1uf decoupling cap though…

Well, you are getting noise because you are changing the frequency too quickly.

Try this (works without the sensor):

#define trig 7 //trigger pin on sonar module
#define echo 6 //echo pin
#define spkr 4
#define led 12
int dist; //how far the object is away from the module(cm)
float valueSensor=0;

//tones array which holds the frequency to play.
float pitchTable[] = {
  392.00, // G4
  415.30, // G#4/Ab4
  440.000000, // A
  466.163757, // A#/Bb
  493.83301, // B
  523.251160, // C
  554.365234, // C#/Db
  587.329529, // D
  622.253967, // D#/Eb
  659.255127, // E
  698.456482, // F
  739.988831, // F#/Gb
  783.990845, // G
  830.609375 // G#/Ab
};

#define NUMITEMS(arg) (sizeof (arg) / sizeof (arg [0]))

void setup()
{
  pinMode(echo, INPUT);
  pinMode(trig, OUTPUT);
}

void loop()
{
  dist = random (0, 80);
  // each 5 positions represents a different note
  byte i = dist / 5;
  
  // play note if in range
  if (i < NUMITEMS (pitchTable))
     tone(spkr,pitchTable[i]);
  else
    noTone(spkr);
  delay(25);
}

That plays a lot of random beeps. Now comment out the final delay(25) and it just plays noise. So the code is OK, it just doesn’t handle very rapid switching.