Broken code

Hello, I have another issue. I cannot get this code to work-

#include <Servo.h>
Servo servol;
const int motorPin = 11;
const int buzzerPin = 9;



 int songLength = 18;



char notes[] = "cdfda ag cdfdg gf "; 

int beats[] = {1,1,1,1,1,1,4,4,2,1,1,1,1,1,1,4,4,2};



int tempo = 150;


void setup() 
{
  pinMode(motorPin, OUTPUT);
  pinMode(buzzerPin, OUTPUT);
  servol.attach(10);
}


void loop() 
{
  int position;

int g = 0;
for (g = 0; g < 5; g++) {
 servol.write(180);   

  delay(1000);         

  servol.write(0);     

  delay(1000); 
}
  int i =0;
  for (i = 0; i < songLength; i++) // step through the song arrays
  {
   int duration = beats[i] * tempo;  // length of note/rest in ms
    
    if (notes[i] == ' ')          // is this a rest? 
    {
      delay(duration);            // then pause for a moment
    }
    else                          // otherwise, play the note
    {
      int frequency(char note) ;
      tone(buzzerPin, frequency(notes[i]), duration);
      delay(duration);            // wait for tone to finish
    }
    delay(tempo/10);              // brief pause between notes
  }
  
  // We only want to play the song once, so we'll pause forever:
  while(true){}
  // If you'd like your song to play over and over,
  // remove the above statement
}


int frequency(char note) 
{
  // This function takes a note character (a-g), and returns the
  // corresponding frequency in Hz for the tone() function.
  
  int i;
  const int numNotes = 8;  // number of notes we're storing
  
  // The following arrays hold the note characters and their
  // corresponding frequencies. The last "C" note is uppercase
  // to separate it from the first lowercase "c". If you want to
  // add more notes, you'll need to use unique characters.

  // For the "char" (character) type, we put single characters
  // in single quotes.

  char names[] = { 'c', 'd', 'e', 'f', 'g', 'a', 'b', 'C' };
  int frequencies[] = {262, 294, 330, 349, 392, 440, 494, 523};
  
  // Now we'll search through the letters in the array, and if
  // we find it, we'll return the frequency for that note.
  
  for (int i = 0; i < numNotes; i++)  // Step through the notes
  {
    if (names[i] == note)         // Is this the one?
    {
      return(frequencies[i]);     // Yes! Return the frequency
    }
  }
  return(0);  // We looked through everything and didn't find it,
              // but we still need to return a value, so return 0
  int position;
}
int songlength2 = 23;
char notes2[] = "a g f e efffefffa g f e fff agfe";
int beats2[] = {1,1,1,1,.5,.5,1,2,.5,.5,1,2,1,1,1,1,.5,.5,1,6,8,8,8};
int tempo2 = 104;
for(h=0; h <= 255; h++){
    
    int duration = beats2[h] * tempo;  // length of note/rest in ms
    
    if (notes[i] == ' ')          // is this a rest? 
    {
      delay(duration);            // then pause for a moment
    }
    else                          // otherwise, play the note
    {
      tone(buzzerPin, frequency(notes[h]), duration);
      delay(duration);            // wait for tone to finish
    }
    delay(tempo/10);              // brief pause between notes
  }
  
  // We only want to play the song once, so we'll pause forever:
  while(true){}
  // If you'd like your song to play over and over,
  // remove the above statement



int frequency(char note) 
{
  // This function takes a note character (a-g), and returns the
  // corresponding frequency in Hz for the tone() function.
  
  
  const int numNotes = 8;  // number of notes we're storing
  
  // The following arrays hold the note characters and their
  // corresponding frequencies. The last "C" note is uppercase
  // to separate it from the first lowercase "c". If you want to
  // add more notes, you'll need to use unique characters.

  // For the "char" (character) type, we put single characters
  // in single quotes.

  char names[] = { 'c', 'd', 'e', 'f', 'g', 'a', 'b', 'C' };
  int frequencies[] = {262, 294, 330, 349, 392, 440, 494, 523};
  
  // Now we'll search through the letters in the array, and if
  // we find it, we'll return the frequency for that note.
  
  for (j = 0; j < numNotes; j++)  // Step through the notes
  {
    if (names[j] == note)         // Is this the one?
    {
      return(frequencies[j]);     // Yes! Return the frequency
    }
  }
  return(0);  // We looked through everything and didn't find it,
              // but we still need to return a value, so return 0
  int position;
}

The errors I am receiving are these.

Arduino_1:105: error: expected unqualified-id before 'for'
Arduino_1:105: error: expected constructor, destructor, or type conversion before '<=' token
Arduino_1:105: error: expected constructor, destructor, or type conversion before '++' token

bmeatball333:
Hello, I have another issue. I cannot get this code to work-

The for-loop code in line 105 (and following) of your file is NOT within a function.
But is has to be.

This section of code:

int songlength2 = 23;
char notes2[] = "a g f e efffefffa g f e fff agfe";
int beats2[] = {1,1,1,1,.5,.5,1,2,.5,.5,1,2,1,1,1,1,.5,.5,1,6,8,8,8};
int tempo2 = 104;
for(h=0; h <= 255; h++){
    
    int duration = beats2[h] * tempo;  // length of note/rest in ms
    
    if (notes[i] == ' ')          // is this a rest? 
    {
      delay(duration);            // then pause for a moment
    }
    else                          // otherwise, play the note
    {
      tone(buzzerPin, frequency(notes[h]), duration);
      delay(duration);            // wait for tone to finish
    }
    delay(tempo/10);              // brief pause between notes
  }
  
  // We only want to play the song once, so we'll pause forever:
  while(true){}
  // If you'd like your song to play over and over,
  // remove the above statement

is hanging outside any function. You are not allowed to have control keywords (e.g., for, while, if, etc.) outside of a function. My best guess is that this code belongs "inside" some function for which you forgot to include while writing it.

Also, why define position at the bottom of frequency(), but never use it? Defining duration inside a for loop that has 255 iterations is also a bad idea...move it outside the for loop. Finally, if you want something to run only once, put it in setup() and get rid of the while(true){} construct.

You appear to have defined

int frequency(char note)

twice but have never properly called it.
You need to call it by

X = frequency(Y);

Where Y is the note and X is the holder for the int that frequency returns.

Ok, so I fixed the code, but now I have another issue. What I'm trying to do is have a servo move back and forth, after which play some music then some more music. What is happening though is the servo moves then I hear two short tones play but not the music I want. Thanks for the help!

You can see your code.
We can't

Sorry, here is my code.

#include <Servo.h>
Servo servol;
const int motorPin = 11;
const int buzzerPin = 9;

// We'll set up an array with the notes we want to play
// change these values to make different songs!

// Length must equal the total number of notes and spaces 

 int songLength = 18;

// Notes is an array of text characters corresponding to the notes
// in your song. A space represents a rest (no tone)

char notes[] = "cdfda ag cdfdg gf "; // a space represents a rest

// Beats is an array values for each note and rest.
// A "1" represents a quarter-note, 2 a half-note, etc.
// Don't forget that the rests (spaces) need a length as well.

int beats[] = {1,1,1,1,1,1,4,4,2,1,1,1,1,1,1,4,4,2};

// The tempo is how fast to play the song.
// To make the song play faster, decrease this value.

int tempo = 150;


void setup() 
{
  pinMode(motorPin, OUTPUT);
  pinMode(buzzerPin, OUTPUT);
  servol.attach(10);
}


void loop() 
{
  int position;

int g = 0;
for (g = 0; g < 5; g++) {
 servol.write(180);   

  delay(1000);         

  servol.write(0);     

  delay(1000); 
}
  int i =0;
  for (i = 0; i < songLength; i++) // step through the song arrays
  {
   int duration = beats[i] * tempo;  // length of note/rest in ms
    
    if (notes[i] == ' ')          // is this a rest? 
    {
      delay(duration);            // then pause for a moment
    }
    else                          // otherwise, play the note
    {
      int frequency(char note) ;
      tone(buzzerPin, frequency(notes[i]), duration);
      delay(duration);            // wait for tone to finish
    }
    delay(tempo/10);              // brief pause between notes
  }
  
  // We only want to play the song once, so we'll pause forever:
  while(true){}
  // If you'd like your song to play over and over,
  // remove the above statement
}


int frequency(char note) 
{
  // This function takes a note character (a-g), and returns the
  // corresponding frequency in Hz for the tone() function.
  
  int i;
  const int numNotes = 8;  // number of notes we're storing
  
  // The following arrays hold the note characters and their
  // corresponding frequencies. The last "C" note is uppercase
  // to separate it from the first lowercase "c". If you want to
  // add more notes, you'll need to use unique characters.

  // For the "char" (character) type, we put single characters
  // in single quotes.

  char names[] = { 'c', 'd', 'e', 'f', 'g', 'a', 'b', 'C' };
  int frequencies[] = {262, 294, 330, 349, 392, 440, 494, 523};
  
  // Now we'll search through the letters in the array, and if
  // we find it, we'll return the frequency for that note.
  
  for (int i = 0; i < numNotes; i++)  // Step through the notes
  {
    if (names[i] == note)         // Is this the one?
    {
      return(frequencies[i]);     // Yes! Return the frequency
    }
  
  return(0);  // We looked through everything and didn't find it,
  }
delay(1000);  // but we still need to return a value, so return 0
  int position;


int songlength2 = 23;
char notes2[] = "a g f e efffefffa g f e fff agfe";
int beats2[] = {1,1,1,1,.5,.5,1,2,.5,.5,1,2,1,1,1,1,.5,.5,1,6,8,8,8};
int tempo2 = 104;
for(int h=0; h <= 255; h++){
    
    int duration = beats2[h] * tempo;  // length of note/rest in ms
    
    if (notes[i] == ' ')          // is this a rest? 
    {
      delay(duration);            // then pause for a moment
    }
    else                          // otherwise, play the note
    {
      tone(buzzerPin, frequency(notes[h]), duration);
      delay(duration);            // wait for tone to finish
    }
    delay(tempo/10);              // brief pause between notes
  }
  
  // We only want to play the song once, so we'll pause forever:
  while(true){}
  // If you'd like your song to play over and over,
  // remove the above statement
}


int frequency2(char note) 
{
  // This function takes a note character (a-g), and returns the
  // corresponding frequency in Hz for the tone() function.
  
  
  const int numNotes = 8;  // number of notes we're storing
  
  // The following arrays hold the note characters and their
  // corresponding frequencies. The last "C" note is uppercase
  // to separate it from the first lowercase "c". If you want to
  // add more notes, you'll need to use unique characters.

  // For the "char" (character) type, we put single characters
  // in single quotes.

  char names[] = { 'c', 'd', 'e', 'f', 'g', 'a', 'b', 'C' };
  int frequencies[] = {262, 294, 330, 349, 392, 440, 494, 523};
  
  // Now we'll search through the letters in the array, and if
  // we find it, we'll return the frequency for that note.
  
  for (int j = 0; j < numNotes; j++)  // Step through the notes
  {
    if (names[j] == note)         // Is this the one?
    {
      return(frequencies[j]);     // Yes! Return the frequency
    }
  }
  return(0);  // We looked through everything and didn't find it,
              // but we still need to return a value, so return 0
  int position;
}

bmeatball333:
Ok, so I fixed the code, but now I have another issue. What I'm trying to do is have a servo move back and forth, after which play some music then some more music. What is happening though is the servo moves then I hear two short tones play but not the music I want. Thanks for the help!

I don't see your fixed code.

But when I look up the loop() function of the broken code, I can see lots and lots of delay(), delay() and delay().

Delay means always: STOP THE PROGRAM EXECUTION AT ONCE UNTIL TIME IS OVER!

So delay() is only usable for "single task" programs with sequential control.

For pseudo-multitasking programs, doing several actions in (nearly) the same time, delay() cannot be used.

For what you are planning, a "two-task" program perhaps delay would be possible, if you could synchronize the two actions in some way.

When the two tasks are "playing notes" and "rotating a servo", you would have the possibility to synchronize either:

  1. each time a note starts or stops playing, a servo command would be possible to execute
    or:
  2. each time a new servo position is set, a note can start or stop playing
    In both cases it would be able to keep delay() in your code and synchronize the two tasks.

If you want either a third task or you want running the servos and the notes not synchronized, you cannot use delay() and need a compleeeetely different programming logic.

In order to " be broken " it has to compile first.
It does not.

And here is why

 else                          // otherwise, play the note
    {
      int frequency(char note) ;
      tone(buzzerPin, frequency(notes[i]), duration);
      delay(duration);            // wait for tone to finish
    }
    delay(tempo / 10);            // brief pause between notes
  }

Have you set you compiler options to "verbose" ???

After it compiles , clean it up.
Variables should be defined once( loop, for etc.) , but compiler MAY fix that, so no biggie.
They should be defined BEFORE they are used, compiler MAY complain.

It is more logical to define local loop variable inside loop conditions, unless you NEED it as function variable.

Not this
int i= 0; //i is function variable
for(i = 0;...)

But this
for(int i = 0;....) // i is local for variable

Actually the "new standard" won't let you use variable defined inside loop without redefinition again.
In some way nice - you can reuse same variable name.

Moderator edit: Code tags. Again. {sigh}

Vaclav:
In order to " be broken " it has to compile first.
It does not.

And here is why

else // otherwise, play the note
{
int frequency(char note) ;
tone(buzzerPin, frequency(notes*), duration);*

  • delay(duration); // wait for tone to finish*
  • }*
  • delay(tempo / 10); // brief pause between notes*
  • }*
    Have you set you compiler options to "verbose" ???
    After it compiles , clean it up.
    Variables should be defined once( loop, for etc.) , but compiler MAY fix that, so no biggie.
    They should be defined BEFORE they are used, compiler MAY complain.
    It is more logical to define local loop variable inside loop conditions, unless you NEED it as function variable.
    Not this
    int i= 0; //i is function variable
    for(i = 0;...)
    But this
    for(int i = 0;....) // i is local for variable
    Actually the "new standard" won't let you use variable defined inside loop without redefinition again.
    In some way nice - you can reuse same variable name.
    [/quote]
    I don't understand, the Arduino software says it is compiling. How am I supposed to tell its not?

Have you set you compiler options to "verbose" ?

@Vaclav: There's some good and bad points using this construct:

for(int i = 0;....) // i is local for variable

Good: it encapsulates i at the statement block level.
Bad: it encapsulates i at the statement block level.

For example, if I am looking to copy and then terminate a character array and make it a string when I find a certain character (e.g., '#') in the array, then:

for (int i = 0; array[i] != '#'; i++) {
   buffer[i] = array[i];
}
buffer[i] = '\0';

won't work. Admittedly, this is a trumped-up example, but there are times when it's necessary to know the ending index value outside of the control block in which it is defined. Also, placing data definitions in one place often helps document the function. Find a duplicate-definition error is pretty simple if all data definitions are in one place.

It's a little bit of a personal preference thing more than a dogmatic statement.

econjack:
@Vaclav: There's some good and bad points using this construct:

for(int i = 0;....) // i is local for variable

Good: it encapsulates i at the statement block level.
Bad: it encapsulates i at the statement block level.

For example, if I am looking to copy and then terminate a character array and make it a string when I find a certain character (e.g., '#') in the array, then:

for (int i = 0; array[i] != '#'; i++) {

buffer[i] = array[i];
}
buffer[i] = '\0';




won't work. Admittedly, this is a trumped-up example, but there are times when it's necessary to know the ending index value outside of the control block in which it is defined. Also, placing data definitions in one place often helps document the function. Find a duplicate-definition error is pretty simple if all data definitions are in one place.

It's a little bit of a personal preference thing more than a dogmatic statement.

econjack:
@Vaclav: There's some good and bad points using this construct:

for(int i = 0;....) // i is local for variable

Good: it encapsulates i at the statement block level.
Bad: it encapsulates i at the statement block level.

For example, if I am looking to copy and then terminate a character array and make it a string when I find a certain character (e.g., '#') in the array, then:

for (int i = 0; array[i] != '#'; i++) {

buffer[i] = array[i];
}
buffer[i] = '\0';




won't work. Admittedly, this is a trumped-up example, but there are times when it's necessary to know the ending index value outside of the control block in which it is defined. Also, placing data definitions in one place often helps document the function. Find a duplicate-definition error is pretty simple if all data definitions are in one place.

It's a little bit of a personal preference thing more than a dogmatic statement.

Totally agree, but since that is not the OP problem I just suggested it as "clean-up" item.
When I first run into this, I really did not understood why the compiler does not like it.

As far as duplicate definitions I find it odd that the (IDE) verbose output will give you error location but you cannot "click" on the line number to get to the editor window to fix it.
But if one uses "blink without delay" ( correctly) such feature is not needed ( Sarcasm)

Cheers Vaclav

Vaclav:
Have you set you compiler options to "verbose" ?

How would I do this exactly?

How would I do this exactly?

Open up the IDE and follow the sequence:

File --> Preferences

and then check the verbose checkbox.

Im guessing that there is some thing wrong with these.
http://pastebin.com/ftABxrxw(Code)

http://pastebin.com/tp8QNJ9W(Log)

bmeatball333:
Im guessing that there is some thing wrong with these.
http://pastebin.com/ftABxrxw(Code)

What do you think is wrong with that?

The code compiles fine, without any errors.

If you are not sure how copy-and-paste is done correctly, always use the "RAW Paste Data" from the pastebin.com website.

jurs:
What do you think is wrong with that?

The code compiles fine, without any errors.

If you are not sure how copy-and-paste is done correctly, always use the "RAW Paste Data" from the pastebin.com website.

That's not the issue. My code is moving my servo then playing two short notes. What it's supposed to do is move the servo then play two songs

bmeatball333:
What it's supposed to do is move the servo then play two songs

So where is the problem?

The program posted (and compiling without any errors) is a pretty ugly "beginner type" of program using all that silly "delay()" function for blocking the program execution every now and then.

But that's absolutely OK for a "single task" and strictly "sequential control" type of program.

So if you want the programming logic "move the servo then play two songs", why don't you create that program, writing

  • a function that can move the servo
  • a function that can play a song

And then put it together in a loop function that looks like

void loop()
{
  moveMyServo();
  playMeTheSong(notes, beats, songLength);
  playMeTheSong(notes2, beats2, songLength2);
}

Which one is the problem for you?
Writing a "moveMyServo()" function?
Writing a "playMeTheSong()" function?
Or both of them?

You have a function called "frequency2" that duplicates quite a bit of the function "frequency", but you never call it.
Is that intentional?