LED and MUSIC and BUTTONS: Turning them On and Off in certain orders

Hello I am new to arduino electronics. I haven't used a development board or program electronics in years. So I decided to jump back into the game and see what advances there was. And there were tons. So I was playing with some of the examples and was trying to do a particular thing. And I ran into some trouble and hoping to get some help and see what my problems are.

I am trying to use 2 buttons. One to turn the LED on and off. Which I was able to do. the other was to play Music or tones through the piezo buzzer. I got that playing. But What I ran into was when trying to combine them together. I searched google and this forum and tried so many different if, while, and for statement but could not get it to work the way I wanted. And I am not too great with C++ so that was an issue too.

What I wanted to do was have one button to turn on and off the LED. But I want the LED to turn off after a few seconds if I do not turn it off manually. I tried to add delay in my codes but it didnt work out the way I want it. And I want the music to play and loop when another button is press and to turn off when i press the button again. But it doesn't loop, I have to hold the button down for it to loop. I hope i explained everything correctly.

#include "pitches.h"


//variables

const int buttonPin = 7;
const int buttonPin2 = 4;
const int ledPin = 11;
boolean ledOn = false;
boolean currentButton = LOW;
boolean lastButton = LOW;


void setup()
{
  pinMode(ledPin, OUTPUT);
  pinMode(buttonPin, INPUT);
  pinMode(buttonPin2, INPUT);
}

void loop()
{   
    music();

    ledTime();
}
  
void ledTime(){
currentButton = debounce(lastButton);
if (lastButton == LOW && currentButton == HIGH)
    {
      ledOn = !ledOn;
     lastButton = HIGH;
  } 
    lastButton = currentButton; 
    digitalWrite(ledPin, ledOn); 
  }

boolean debounce(boolean last)
{
  boolean current = digitalRead(buttonPin);
  if (last != current)
  { delay(5);
  current = digitalRead(buttonPin);
  }
  return current;
}

void music(){
  // notes in the melody:
int melody[] = {
  NOTE_C4, NOTE_G3,NOTE_G3, NOTE_A3, NOTE_G3,0, NOTE_B3, NOTE_C4};

int noteDurations[] = {
  4, 8, 8, 4,4,4,4,4 };

while (digitalRead(buttonPin2) == HIGH)
{
  for (int thisNote = 0; thisNote < 8; thisNote++) {

    int noteDuration = 1000/noteDurations[thisNote];
    tone(8, melody[thisNote],noteDuration);

    int pauseBetweenNotes = noteDuration * 1.30;
    delay(pauseBetweenNotes);
    // stop the tone playing:
   //noTone(8);
  }
}
}

Any help is appreciated and thank you all in advance.

Don't use delays. You shouldn't use them for this, especially if you want the LED and song to work independently. Delay is a blocking function that will cause the microcontroller to do nothing until time expires.

There are two example sketches you should take a look at. Debounce will show you a better way of debouncing a switch than you currently do. The other, more important one, is BlinkWithoutDelay. Both are in the Digital subfolder of examples. Use that method to time your notes.

Finally, your song isn't repeating because you haven't coded it to repeat. Once the for loop runs, it's done and you have to retrigger it, it won't run forever.

Have your play button, properly debounced, toggle a boolean "playing_music" global variable. If your music subroutine, check that value; if it's true, play the music timing the notes with the BlinkWithoutDelay method. If false, do noTone.

I've actually done something almost exactly like this before, except more complicated with an LCD screen to choose from about 11 songs (all video game BGMs that I had to type in the pitches and durations myself). So I kinda like music a little bit. :slight_smile: Once you've rewritten the code after taking a look at those examples, PM me and I'll take a second look.

Finally, use INPUT_PULLUP for your buttons, not just INPUT. If you're just connecting the button to to pin without an external pullup, there's a chance that it might not trigger right. It's best to do it properly.

Thank you for the quick reply. I just been trying some more things to my code. Which now i will need to try again with the examples you told me.

I will see what I can do with that. But I am not looking to make it blink. Just to turn on and turn off. and if it is on for too long that it would turn off itself.

And wow you made 11 songs from those pitch list. that is amazing. I am still trying to figure out how that works exactly. LCD, I don't think I am ready for that yet. could barely get a music playing in a loop haha.

for the INPUT_PULLUP just change it to that or instead of the INPUT?

I will go ahead and look through those examples and see if i can paste things together. THanks

tanik1:
Thank you for the quick reply. I just been trying some more things to my code. Which now i will need to try again with the examples you told me.
basicall
I will see what I can do with that. But I am not looking to make it blink. Just to turn on and turn off. and if it is on for too long that it would turn off itself.

And wow you made 11 songs from those pitch list. that is amazing. I am still trying to figure out how that works exactly. LCD, I don't think I am ready for that yet. could barely get a music playing in a loop haha.

for the INPUT_PULLUP just change it to that or instead of the INPUT?

I will go ahead and look through those examples and see if i can paste things together. THanks

From the Digital Pins page: http://arduino.cc/en/Tutorial/DigitalPins

Arduino (Atmega) pins default to inputs, so they don't need to be explicitly declared as inputs with pinMode(). Pins configured as inputs are said to be in a high-impedance state. One way of explaining this is that input pins make extremely small demands on the circuit that they are sampling, say equivalent to a series resistor of 100 megohm in front of the pin. This means that it takes very little current to move the input pin from one state to another, and can make the pins useful for such tasks as implementing a capacitive touch sensor, reading an LED as a photodiode, or reading an analog sensor with a scheme such as RCTime.

This also means however, that input pins with nothing connected to them, or with wires connected to them that are not connected to other circuits, will report seemingly random changes in pin state, picking up electrical noise from the environment, or capacitively coupling the state of a nearby pin.

Use it in place of INPUT, like this:

pinMode(buttonPin, INPUT);

Have fun.

void ledTime(){
currentButton = debounce(lastButton);
if (lastButton == LOW && currentButton == HIGH)
    {
      ledOn = !ledOn;
     lastButton = HIGH;
  } 
    lastButton = currentButton; 
    digitalWrite(ledPin, ledOn); 
  }

Nothing that this function does suggests that the name is anywhere close to reasonable. A better name is certainly in order.

Nothing about this function suggests that the indentation is even close to reasonable. Use Tools + Auto Format to make this code more readable. Consistent placement of { would help, too. Sometimes they go on the statement line; sometimes they go on a new line. Pick a style (I prefer the new line style), and stick to it.

  { delay(5);

Regardless of the style, NOTHING goes on the line after the {.

And, yeah, the delays have got to go.

jiggy-ninja: Do i still need to use the PULLUP if I have a external PULLDOWN resistor? I'm just looking into those examples and trying to make sense of it all. I see that the debounce one was much different than what I was doing. But I am still trying to make it turn off after a few seconds on its own. So ill see what I can do.

PaulS: I am sorry that the code is messy. But I can always clean up the codes later which I was planning to do after. And that code was just a quick debounce I found online and it did what i needed it to do at the moment. And jiggy-ninja had already pointed me to another example. So I will go base off that.

But I can always clean up the codes later

When you cook, do you leave dirty dishes all over the place? Or, do you try to keep the kitchen relatively neat as you go?

"Cleaning up the code later" is NOT a viable option, in my opinion. Keep it neat. Keep it clean. As you go. Not later.

PaulS:
When you cook, do you leave dirty dishes all over the place? Or, do you try to keep the kitchen relatively neat as you go?

Honestly, yes thats how I cook. Everyone has their own ways of doing thing. And sorry that my way isn't like that. And I do not do too much programming. And maybe in the future I will be a little cleaner. But right now I am just looking for a possible solution for something I am trying to do. Thank you.

Programming is all about thinking logically, and organizing code in appropriate ways. Messy coding makes that much more difficult, and is a large part, in my opinion, why some people struggle. People who organize there code so that it is readable, use appropriate names, and consistently follow conventions generally have a far easier time getting from point A (a general idea what the program should do) to point B (a working program with no obvious bugs).

Of course, you are free to work with poorly laid out code, guessing which } goes with which {, etc. If you want people to help you though, neat code is more pleasant to look at.

And, given the Tools + Auto Format function isn't ALL that hard to do.

Hi everyone, I have another part that I would like to get some help on. So I read this page about millis(). heres the link. http://www.gammon.com.au/forum/?id=11411

So i was trying to combine that with the debounce in the example. I know I am doing something wrong but I dont know where exactly it is. I am trying to toggle the led on and off with a button and if the led has been on for more than 5s to turn it off. The toggle works but not the off parts. ;(

Now i got it to turn off but it wont turn back on.

const int buttonPin = 2;    // the number of the pushbutton pin
const int ledPin = 13;      // the number of the LED pin

// Variables will change:
int ledState = LOW;         // the current state of the output pin
int buttonState;             // the current reading from the input pin
int lastButtonState = HIGH;   // the previous reading from the input pin

// the following variables are long's because the time, measured in miliseconds,
// will quickly become a bigger number than can be stored in an int.
long lastDebounceTime = 0;  // the last time the output pin was toggled
long debounceDelay = 50;    // the debounce time; increase if the output flickers

const unsigned long redLEDinterval = 5000;
unsigned long redLEDtimer;
const unsigned long buttonInterval = 0;
unsigned long buttonTimer;

void setup() {
  pinMode(buttonPin, INPUT);
  pinMode(ledPin, OUTPUT);
  redLEDtimer = millis ();
  buttonTimer = millis();

}

void loop() {
    if ((millis() - buttonTimer) >= buttonInterval)
    {
    // read the state of the switch into a local variable:
    int reading = digitalRead(buttonPin);

    // check to see if you just pressed the button 
    // (i.e. the input went from LOW to HIGH),  and you've waited 
    // long enough since the last press to ignore any noise:  

    // If the switch changed, due to noise or pressing:
    if (reading != lastButtonState) {
      // reset the debouncing timer
      lastDebounceTime = millis();
    } 

    if ((millis() - lastDebounceTime) > debounceDelay) {
      // whatever the reading is at, it's been there for longer
      // than the debounce delay, so take it as the actual current state:

      // if the button state has changed:
      if (reading != buttonState) {
        buttonState = reading;

        // only toggle the LED if the new button state is HIGH
        if (buttonState == HIGH) {
          ledState = !ledState;
        }
      }
    }

    // set the LED:
    digitalWrite(ledPin, ledState);

    // save the reading.  Next time through the loop,
    // it'll be the lastButtonState:
    lastButtonState = reading;
  }
  if ( (millis () - redLEDtimer) >= redLEDinterval) 
    ledTimer ();
}

void ledTimer ()
{
while (1) //added this in and now it turn off but i cant turn it back on.
{
  if (digitalRead (ledPin) == HIGH)
    digitalWrite (ledPin, LOW);
    redLEDtimer = millis (); 
} 
}  // end of toggleRedLED
unsigned long buttonTimer;

millis() is not a timer. The variables involved should, in my opinion, not imply that it is.

You are recording when an event occurred. Using event in the variable name makes it clear which event the time (NOT timer) is for.

int buttonState;             // the current reading from the input pin
int lastButtonState = HIGH;   // the previous reading from the input pin

My preference is for names that "match", like currState and prevState. I think that it is helpful when writing the if statements and the assignment statements, to assure that the right things are being compared/saved.

    int reading = digitalRead(buttonPin);

What the heck is this? You only need to know the current state and the previous state. You do NOT need three variables to determine if a state change occurred, and if it did, was it too close to another state change to matter.

Your state change detection and debouncing code is far too complex.

What kind of switch are you using? A 50 millisecond debounce time far longer than most switches bounce.

This code really doesn't need the complexity of using millis() to debounce the switch. A simple delay(10); when the state change is detected is hardly going to affect the responsiveness of the LED to the switch, and results in far simpler code.

Even if you want to continue using millis() to debounce the switch, in pseudo code, the process looks like this:
read current switch state
if current state not equal previous state:
if switch is now pressed
if time since last press is greater than debounce time
record current stat change time
toggle output pin state
record pin state change time
else
nothing to do
else
nothing to do
else
nothing to do
set previous state to current state

Like i mentioned before. I am not great at coding. These are all example codes that came with the arduino and I am learning base on these examples. Why it need that third variable? I thought it needs it because it was in the example. And i just bought an arduino uno r3 starter kit. And I am just trying to figure out how I would get this to work how I want it. So I am doing one part at a time. And I am going with millis and not delay is because people have said that using delay will block other code from running until it finish its delay. and what I plan to do is:

be able to turn the led on and off while playing music through the piezo buzzer. And being able to turn the piezo buzzer on and off while the led is on. and if the led is on for a certain amount of time to turn it off automatically. Same goes for the music if its on for a certain amount of time also turn off. but to have them separately from each other.

So what I was trying to do is maybe get one part working then do another part and hopefully merge them all together and pray that they work.

Your pseudo code makes perfect sense and I have been trying to write the actual codes from the examples found at arduino and other sources online. and it works and all only when I put things together they fall apart. But slowly learning.

Why it need that third variable?

It doesn't. I said that it doesn't in my reply.

And I am going with millis and not delay is because people have said that using delay will block other code from running until it finish its delay.

That's true. But, there is a huge difference between a 10 millisecond delay to debounce a switch (only when the transition to pressed happens) and delay(86400000UL);.

So what I was trying to do is maybe get one part working then do another part and hopefully merge them all together and pray that they work.

I'm a coding atheist. I don't believe that praying works. Understanding does.

Start with state change detection, using delay() when the state changes to pressed for debouncing. Only if the 10 millisecond delay is a problem would I bother with trying to get rid of it. And, in your description, I don't think it will be.

ok after a while i finally got the lights to do what I wanted. And I was also able to get the music to to play with a press of the button by following the same rules as the debounce and blinkwithoutdelay example.

But now my problem is I cannot do anything with the lights until the music finish. What could be my potential problem?

#include "pitches.h"


const int buttonPin = 2;
const int buttonPin2 = 4;
const int ledPin = 13;
const int piezo = 8;


int ledState = LOW;
int currentState;
int prevState = HIGH;
int currentState2;
int prevState2 = HIGH;

long lastDebounce = 0;
long lastDebounce2 = 0;
long debounceDelay = 5;
long currentMillis; 
long prevMillis = 0;

const long timeForOff = 10000;
long autoOff;

// notes in the melody:
int melody[] = {
  NOTE_C4, NOTE_G3,NOTE_G3, NOTE_A3, NOTE_G3,0, NOTE_B3, NOTE_C4};

// note durations: 4 = quarter note, 8 = eighth note, etc.:
int noteDurations[] = {
  4, 8, 8, 4,4,4,4,4 };

void setup(){
  pinMode(buttonPin, INPUT);  
  pinMode(ledPin, OUTPUT);    
  pinMode(buttonPin2, INPUT);
}

void loop(){
  //LED debounce steps working code
  currentState = digitalRead(buttonPin);
  if (currentState != prevState){
    if (currentState == HIGH)
      lastDebounce = millis();
    if (millis() - lastDebounce >= debounceDelay){
      ledState = !ledState;
      lastDebounce = millis();
      digitalWrite(ledPin, ledState);
      autoOff = millis();  
    }
  }
  prevState = currentState;
  //auto off timer for when LED is on
  if (millis() - autoOff >= timeForOff){
    if (ledState = HIGH)
      digitalWrite(ledPin, LOW);
    ledState = !ledState; 
  }


  currentState2 = digitalRead(buttonPin2);
  if (currentState2 != prevState2){
    if (currentState2 == HIGH)
      lastDebounce2 = millis();
    if (millis() - lastDebounce2 >= debounceDelay){
      lastDebounce2 = millis();
      int i = 0; 
      while(i<8){
        int noteDuration = 1000/noteDurations[i];
        int noteDelay = (noteDuration * 1.30);
        currentMillis = millis();
        if (millis() - prevMillis >= noteDelay){
          tone(8, melody[i], noteDuration);
          prevMillis = currentMillis;
          i++;
        }
      }
    }
  }

  prevState2 = currentState2;
}

Hi Jiggy-Ninja,

You think you can help show me how you store your pitch array using the PROGMEM? I am not sure if I am storing it correct and calling it back correctly. This is how i was trying to store them. But I think its not correct.
thanks

PROGMEM prog_int16_t melody_0[] = { NOTE_C3, NOTE_C3, NOTE_G3, NOTE_G3, NOTE_A3, NOTE_A3, NOTE_G3, NOTE_F3, NOTE_F3, NOTE_E3, NOTE_E3, NOTE_D3, NOTE_D3, NOTE_C3, NOTE_G3, NOTE_G3, NOTE_F3, NOTE_F3, NOTE_E3, NOTE_E3, NOTE_D3, NOTE_G3,NOTE_G3, NOTE_F3, NOTE_F3, NOTE_E3, NOTE_E3, NOTE_D3, NOTE_C3, NOTE_C3, NOTE_G3, NOTE_G3, NOTE_A3, NOTE_A3, NOTE_G3, NOTE_F3, NOTE_F3, NOTE_E3, NOTE_E3, NOTE_D3, NOTE_D3, NOTE_C3};
PROGMEM prog_int16_t melody_1[] = { NOTE_G3, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_E4, NOTE_E4, NOTE_D4, NOTE_E4, NOTE_C4, NOTE_C4, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_F4, NOTE_A4, NOTE_A4, NOTE_G4, NOTE_F4, NOTE_E4, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_F4, NOTE_A4, NOTE_A4, NOTE_G4, NOTE_F4, NOTE_E4, NOTE_C4, NOTE_G3,;
PROGMEM prog_int16_t melody_2[] = { NOTE_G3, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_E4, NOTE_E4, NOTE_D4, NOTE_E4, NOTE_C4, NOTE_C4, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_F4, NOTE_A4, NOTE_A4, NOTE_G4, NOTE_F4, NOTE_G3, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_F4, NOTE_D4, NOTE_C4, NOTE_E4, NOTE_C4};


PROGMEM const int *melody_table[] = 
{
  melody_0,
  melody_1,
  melody_2,
};

tanik1:
ok after a while i finally got the lights to do what I wanted. And I was also able to get the music to to play with a press of the button by following the same rules as the debounce and blinkwithoutdelay example.

But now my problem is I cannot do anything with the lights until the music finish. What could be my potential problem?

The problem is that you're stuck in that while loop until the melody finishes. I think you're the one I gave my sketch to to look at, but I'll post the relavent section here again.

void play_notes()
{
  unsigned long t_now = millis();
  
  if( t_now>note_end )
  {
    noTone( outPin );
    unsigned int pitch;
    unsigned int length;
    
    //pitch = pgm_read_word_near( note+(play_intro?intro:melody) );
    //length = pgm_read_word_near( note+1+(play_intro?intro:melody) );
    if( play_intro==1 )
    {
      pitch = pgm_read_word_near(intro+note);
      length = pgm_read_word_near(intro+note+1);
    }
    else
    {
      pitch = pgm_read_word_near(melody+note);
      length = pgm_read_word_near(melody+note+1);
    }
    
    if( length!=0 )
    {
      unsigned long time = (length*beat)/6;
      tone( outPin, pitch, time );
      note_end = t_now + (time*1.1);
      note += 2;
    }
    else
    {
      play_intro = 0;
      note = 0;
    }
  }
}

It does a couple things wrong (I wrote this a while ago), but you can refine the basic idea. If I am playing a song, this function is called repeatedly in loop(). You can put something like this in there:

if( play_melody )
{
  play_notes();
}

The first thing it does is test if the proper time for the note has passed. If the note should still be playing, it doesn't do anything. If it should be done, it stops the note and gets the pitch and length variables from the program memory. The next thing it does is this test:

if( length!=0 )

Because my sketch has many songs of different lengths, I chose to use a sentinel to signify the end of the song, rather than keeping track of how many notes it has. Because a note of 0 length is nonsense, I chose to use that value to signify the end of the melody. In my case, I restart from the beginning. You can choose whether to stop or repeat if you want.

If it's not 0 length, it plays the next note like normal and advances the note index by two (I stored the pitch and duration in the same array).

tanik1:
Hi Jiggy-Ninja,

You think you can help show me how you store your pitch array using the PROGMEM? I am not sure if I am storing it correct and calling it back correctly. This is how i was trying to store them. But I think its not correct.
thanks

PROGMEM prog_int16_t melody_0[] = { NOTE_C3, NOTE_C3, NOTE_G3, NOTE_G3, NOTE_A3, NOTE_A3, NOTE_G3, NOTE_F3, NOTE_F3, NOTE_E3, NOTE_E3, NOTE_D3, NOTE_D3, NOTE_C3, NOTE_G3, NOTE_G3, NOTE_F3, NOTE_F3, NOTE_E3, NOTE_E3, NOTE_D3, NOTE_G3,NOTE_G3, NOTE_F3, NOTE_F3, NOTE_E3, NOTE_E3, NOTE_D3, NOTE_C3, NOTE_C3, NOTE_G3, NOTE_G3, NOTE_A3, NOTE_A3, NOTE_G3, NOTE_F3, NOTE_F3, NOTE_E3, NOTE_E3, NOTE_D3, NOTE_D3, NOTE_C3};

PROGMEM prog_int16_t melody_1[] = { NOTE_G3, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_E4, NOTE_E4, NOTE_D4, NOTE_E4, NOTE_C4, NOTE_C4, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_F4, NOTE_A4, NOTE_A4, NOTE_G4, NOTE_F4, NOTE_E4, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_F4, NOTE_A4, NOTE_A4, NOTE_G4, NOTE_F4, NOTE_E4, NOTE_C4, NOTE_G3,;
PROGMEM prog_int16_t melody_2[] = { NOTE_G3, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_E4, NOTE_E4, NOTE_D4, NOTE_E4, NOTE_C4, NOTE_C4, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_F4, NOTE_A4, NOTE_A4, NOTE_G4, NOTE_F4, NOTE_G3, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_F4, NOTE_D4, NOTE_C4, NOTE_E4, NOTE_C4};

PROGMEM const int *melody_table[] =
{
  melody_0,
  melody_1,
  melody_2,
};

I didn't put my melody_table into progmem, though it's possible to do so. I can't remember the page I found that showed how to do that. Here's how I did it for mine.

prog_int16_t celadonIntro[] PROGMEM =
{
  NOTE_D3,  6,
  NOTE_CS3, 6,
  NOTE_D3,  6,
  NOTE_E3,  6,
  NOTE_FS3, 6,
  NOTE_E3,  6,
  NOTE_FS3, 6,
  NOTE_G3,  6,
  NOTE_A3,  48,

  0, 0
};
prog_int16_t* introArray[] = { 
  surfIntro, hyruleIntro, palletIntro, underworldIntro, pacmanIntro, pokecenterIntro, route1Intro, oaksLabIntro, sproutTowerIntro, goldenrodIntro, celadonIntro };

You can see how to access them in the play_notes function I posted above.