Need some help figuring out my code

Get rid of this and use millis()

It's basically the same. I just don't understand how to use an interrupt in here every x amount of ms to change the state of all the LEDs. That's the problem I have. (plus the button starting and stopping it all)

That's fine because you should not use an interrupt...

Something like this?

#incude "pitches.h"

const int buttonPin = 10;
int buttonState;
const int melodyPin = 11;
const int rLed1 = 1;
const int rLed2 = 2;
const int rLed3 = 3;
const int rLed4 = 4;
const int rLed5 = 5;
const int rLed6 = 6;
const int wLed1 = 7;
const int wLed2 = 8;
const int wLed3 = 9;
const int wLed4 = 12;
const int wLed5 = 13;
const int wLed6 = 0;

unsigned long previousMillis = 0;

int wish_melody[] = {

int wish_tempo[] = {
  4, 8, 8, 8, 8,
  4, 4, 4,
  4, 8, 8, 8, 8,
  4, 4, 4,
  4, 8, 8, 8, 8,
  4, 4, 8, 8,
  4, 4, 4,

void setup() {
  pinMode(rled1, OUTPUT); // all led pins = output
  pinMode(rled2, OUTPUT);
  pinMode(rled3, OUTPUT);
  pinMode(rled4, OUTPUT);
  pinMode(rled5, OUTPUT);
  pinMode(rled6, OUTPUT);
  pinMode(wled1, OUTPUT);
  pinMode(wled2, OUTPUT);
  pinMode(wled3, OUTPUT);
  pinMode(wled4, OUTPUT);
  pinMode(wled5, OUTPUT);
  pinMode(wled6, OUTPUT);
  pinMode(button, INPUT);
  pinMode(melodyPin, OUTPUT);


void loop() {
  int song = 1;

  void sing(int s) {
    song = s;
    int size = sizeof(wish_melody) / sizeof(int);
    for (int thisNote = 0; thisNote < size; thisNote++) {

      unsigned long currentMillis = millis();
      // to calculate the note duration, take one second
      // divided by the note type.
      //e.g. quarter note = 1000 / 4, eighth note = 1000/8, etc.
      int noteDuration = 1000 / wish_tempo[thisNote];

      buzz(melodyPin, wish_melody[thisNote], noteDuration);

      // to distinguish the notes, set a minimum time between them.
      // the note's duration + 30% seems to work well:
      long pauseBetweenNotes = noteDuration * 1.30;
      if (currentMillis - previousMillis >= pauseBetweenNotes) {
        previousMillis = currentMillis;

      // stop the tone playing:
      buzz(melodyPin, 0, noteDuration);



It's identical to the one I had before. Buzzer works fine I just can't for the life of me find a proper way to change the LEDs state during the song is playing since you said I should not use interrupts

Have a good long read here:

@thenoobjoker you might want to back up a bit so you don't make mistakes like trying to include a function within a function. It's C++, not Pascal!

void loop() {
  int song = 1;

  void sing(int s) {
    song = s;

Just wait 'til you get comfy and can share code that compiles. We here. We'll wait.



This is a way to use interrupt if you are only interested in the falling edge and do not expect EM interference. I know that debouncing and the correct method of reading buttons is complicated and you have to take less care if you do polling. In the case coded below, you do not poll an interrupt, you are "polling " a boolean variable.

How different approaches are, can be seen in the articles of hackaday
and others (e.g. the possibly most advanced paper

#include <arduino.h>
// EIFR = 0x01

const byte        ledPin = 13;
byte              ledState = LOW;

const byte        interruptPin = 2;
      byte        interruptUsed = 0;
volatile boolean  edgeDetected = false;
volatile boolean  ISRactive = true;
volatile long     lastEdgemillis = 0;

void EdgeISR() {
  edgeDetected   = true;

void enableButtonISR(){
  Serial.println("ISR Active");
  edgeDetected = false;
  ISRactive    = true;
  // Now we clear a possibly outstanding interrupt for interruptUsed before attaching.
  // This is necessary, because it is not done in attachInterrupt() ...
  EIFR |= (1 << interruptUsed);           
  attachInterrupt(interruptUsed, EdgeISR, FALLING);

void disableButtonISR(){
  ISRactive   = false;

void setup() {

  // Internal LED used just for visual feedback
  pinMode(ledPin, OUTPUT);
  digitalWrite(ledPin, ledState);  // not neccessary, but to be on the safe side

  // Button from interruptPin to ground
  pinMode(interruptPin, INPUT_PULLUP);
  // Recommended way to get the Interrupt No 
  interruptUsed = byte(digitalPinToInterrupt(interruptPin));
  // Attach Button to EdgeISR() and enable ISR

void loop() {
  // Loop until EdgeISR detects a falling edge
  if (edgeDetected){
    // In THIS example, we go into this part of if{} only once .. 
   // after one falling edge (and probably later again after re-enabling the button ISR)
    // but there are numerous ways to design the application
    // e.g. one could use a switch/case construct to do
    // different things or make it dependend on a counter or 
    // create a state machine ...
    edgeDetected = false;
    // This is where the "real" application would be going to start
    // In this example we do nothing than toggling the LED and ... 
    // ... wait, but could do a lot of things:
    lastEdgemillis = millis();
    Serial.println("ISR Inactive");
    ledState = !ledState;
    digitalWrite(ledPin, ledState);
    while(millis()-lastEdgemillis <= 200){}; 
    // This is the end of the application part
    // Now this is to call when we like to re-activate the button again 
    if (!ISRactive) {enableButtonISR();} 

Yeah… I was exhausted, I'm taking a look through this thread again. People kept saying I shouldn't use interrupts but don't explain what I should use. Do you know what they want me to use instead?

But you are still polling
The only reason you have a interrupt there is to catch the case where the falling edge is a fleeting event which wouldn't be caught by a sub-millisecond (to pluck a number from the ether) main polling loop.

We're talking about a switch here.
Operated by a human.

Just let that sink in.

@thenoobjoker Sry I don't have handy the three URLs always recommended in this circumstance. Perhaps someone will supply the classics at hand.

But google

Arduino switch denouncing


Arduino two things at the same time


Arduino blink without delay

Stay busy with those for some time. It's hard until it's easy. :expressionless:

As for interrupts, yes no you shouldn't even think about it. At a noob or even better level of skill, interrupts are a distraction. Interrupts have their place, but require some care and attention that is well served by a solid footing with non-interrupt regular programming.

I use interrupts when necessary. I hardly use them at all. Logical conclusion: for what I do, interrupts are rarely necessary.

Certainly not for what you want to be up to.

I too recommend although it may be more deeper and wider than you need or might be able to fathom.


Bunk. Reading and debouncing switches is dead simple. A monkey could do it. Detect a state change, register a switch event, do not poll again until the defined debounce period has elapsed.

Here is's take on it:

Alright, thanks a lot. Back to researching I go.



So we have a fascinating phenomenon here. Three days ago, a new poster joined the Arduino forums with a slightly confused observation about a particular simple module from the typical "beginner" kit.

He(?) has subsequently embarked on a persistent vendetta here as I first noted in #20, nine posts so far, to convince the OP to embark on a totally inappropriate diversion in his project.

Clearly "the blind leading the blind"! :crazy_face:

Advice to OP: Do not even read any such unsafe posts. :sunglasses:

Your assumption of my gender is correct.

What would you suggest I use then?

Kind regards

I think you're confused; you are not the subject of reply #61.

The suggestion is that you poll the switch.

...and stay away from Instructables

I wasn't guessing your gender, but the enthusiastic interjector. :grin:

This whole thread has been severely muddled by the unhelpful suggestions, 63 posts so far is just ridiculous. Let's get back to your OP.

Which is exactly what you have to do. :+1:

There are - as I understand it - three parts to your system. A song which is to be read from a table of notes (array), a LED pattern similarly read from an array, and a button to toggle the whole shebang on and off.

So your program loop needs to be a "state machine" involving three steps. Let's say the first step is to monitor the button. Has it just been pressed (that is, was previously not pressed and now is)? If so, a button timer (millis) is started. If it was started on a previous pass in the loop and is still pressed (otherwise it is cancelled), millis() is checked to see if it has been consistently pressed long enough (about 10 ms) to be a genuine press and if so, the "run/ stop" flag is toggled.

Next step: Check the song table. How long (millis) has the current note been playing? If not long enough, just keep playing it. If ready for the next note, OK, go to the next note.

Third step: Check the LED pattern table. How long (millis) has the current pattern been playing? If not long enough, just keep playing it. If ready for the next pattern, OK, go to the next pattern.

This loop() containing three steps keeps repeating forever. The second and third steps are of course dependent on the "run/ stop" flag; if it was set to "stop" then any note is switched off as is the LED pattern. You have an option here as to whether the button causes a pause or a re-start of the patterns.

Note that although there are other ways of doing it, the loop() runs extremely fast. It presumes you use the "tone" function to generate the sound independently of the main program and you only change the "tone" when you need to.

Great, thank you. I'm heading of to bed now but will attempt to try this tomorrow.

Kind regards

Nooobody expects the Spanish Inquisition...