2

I'd like to start by saying that I have only been dabbling in electronics (not just Arduino) for about 3 days now.

I am trying to write a sketch which changes an RGB LED when a push switch is pressed.

There are 6 modes -

0 = off
1 = dim
2 = mid brightness
3 = full brightness
4 = cycle through RGB colours
5 = pause the colour cycle at the current colour

I have managed to get this working using the code below this explanation. What I would like to know is this - Since the LED only changes state when the button is pressed, is it possible to avoid the use of the main 'loop()' entirely?

Will work fine for modes 0, 1, 2 and 3, but the interrupt cannot fire until mode 4 has finished 1 entire colour cycle.

/*
 * LED MODES
 * 0 = off
 * 1 = dim
 * 2 = mid
 * 3 = bright
 * 4 = colour cycle
 * 5 = colour select
 */
volatile int ledMode = 0; // off
const int maxLEDMode = 5;
volatile bool buttonPressed = false;

int rPin = 9;
int gPin = 10;
int bPin = 11;
int ledPin = 5;
int buttonPin = 2;
int currentVals[] = {0, 0, 0};


void setup() {
  Serial.begin(9600);
  pinMode(rPin, OUTPUT);
  pinMode(gPin, OUTPUT);
  pinMode(bPin, OUTPUT);
  pinMode(ledPin, OUTPUT);
  pinMode(buttonPin, INPUT);

  attachInterrupt(digitalPinToInterrupt(buttonPin), buttonPress, FALLING);
}


void loop() {
  setBrightness(); // try and move outside of main loop
}


// Ensure that buttonPress does not trigger twice
void buttonPress(){
  if(digitalRead(buttonPin) == 0){
    delay(10);
    if(digitalRead(buttonPin) == 0){

      setMode();
      buttonPressed = true;
      Serial.println(ledMode);
    }
  } 
}


//Increment LED Mode until max level is reached
void setMode(){
  if(ledMode == maxLEDMode){
    ledMode = 0;
  } else {
    ledMode++;
  }
}


//Set LED colours depending on ledMode
void setBrightness(){
  buttonPressed = false;

  if(ledMode == 0){
    mode0();
  }

  if(ledMode == 1){
    mode1();
  }

  if(ledMode == 2){
    mode2();
  }

  if(ledMode == 3){
    mode3();
  }

  if(ledMode == 4){
    mode4();
  }

  if(ledMode == 5){
    mode5();
  }
}


//Fade to off
void mode0 (){
  fadeTo(0, 0, 0, 10);
}


// Fade to dim light
void mode1 (){
  int val = 255/5;
  fadeTo(val, val, val, 100);
}


// Fade to medium light
void mode2 (){
  int val = (255/3);
  fadeTo(val, val, val, 100);
}


// Fade to bright light
void mode3 (){
  fadeTo(255, 255, 255, 100);
}


// Endlessly cycle through RGB colours
void mode4(){
    fadeToWithCheck(255, 0, 0, 500);
    fadeToWithCheck(255, 255, 0, 500);
    fadeToWithCheck(0, 255, 0, 500);
    fadeToWithCheck(0, 255, 255, 500);
    fadeToWithCheck(0, 0, 255, 500);
    fadeToWithCheck(255, 0, 255, 500);
}


// Pause LED on colour inherited from Mode4
void mode5(){
  int rExisting = currentVals[0];
  int gExisting = currentVals[1];
  int bExisting = currentVals[2];
  fadeTo(rExisting, gExisting, bExisting, 1);
}


// Fade to given colour, with awareness of mode4 toggles
void fadeToWithCheck(int r, int g, int b, int interval){
  if(ledMode == 4){
    int rExisting = currentVals[0];
    int gExisting = currentVals[1];
    int bExisting = currentVals[2];

    for(int i = 0; i <= interval; i++){
      analogWriteInverted(rPin, map(i, 0, interval, rExisting, r));
      analogWriteInverted(gPin, map(i, 0, interval, gExisting, g));
      analogWriteInverted(bPin, map(i, 0, interval, bExisting, b));

      if(buttonPressed == true){
        Serial.println("stopping loop");
        currentVals[0] = map(i, 0, interval, rExisting, r);
        currentVals[1] = map(i, 0, interval, gExisting, g);
        currentVals[2] = map(i, 0, interval, bExisting, b);
        break;
      }
      delay(10);
    }

    if(buttonPressed == true){
      buttonPressed = false;
    } else {
      currentVals[0] = r;
      currentVals[1] = g;
      currentVals[2] = b;
    }
  }
}


// Fade to given colour
void fadeTo(int r, int g, int b, int interval){
  int rExisting = currentVals[0];
  int gExisting = currentVals[1];
  int bExisting = currentVals[2];

  for(int i = 0; i <= interval; i++){
    analogWriteInverted(rPin, map(i, 0, interval, rExisting, r));
    analogWriteInverted(gPin, map(i, 0, interval, gExisting, g));
    analogWriteInverted(bPin, map(i, 0, interval, bExisting, b));
    delay(10);
  }
  currentVals[0] = r;
  currentVals[1] = g;
  currentVals[2] = b;
}


// Invert given value due to cathode/anode LED
void analogWriteInverted( byte pin, byte value)
{
  analogWrite(pin, 255-value);
}

Any help or advice at all would be appreciated! Many thanks, Rob

UPDATE - Working solution

Following on from the advice given by Michel, the following code meets my needs perfectly.

#include "TimerOne.h"

/*
 * LED MODES
 * 0 = off
 * 1 = dim
 * 2 = mid
 * 3 = bright
 * 4 = colour cycle
 * 5 = colour select
 */
volatile int ledMode = 0; // off
const int maxLEDMode = 5;
volatile bool buttonPressed = false;
unsigned long lastDebounceTime = -1;
unsigned long debounceDelay = 200;
int lastButtonState = HIGH;
int buttonState;
long secondTimer = 1000000;

int rPin = 6;
int gPin = 5;
int bPin = 3;
int buttonPin = 2;
int currentVals[] = {0, 0, 0};
int mode4Vals[] = {0, 0, 0};
int mode4Stage = 0;

long fadeInterval = 0;
long lastSeenInterval;


void setup() {
  Serial.begin(9600);
  pinMode(rPin, OUTPUT);
  pinMode(gPin, OUTPUT);
  pinMode(bPin, OUTPUT);
  pinMode(buttonPin, INPUT);

  Timer1.initialize(secondTimer / 1000);
  attachInterrupt(digitalPinToInterrupt(buttonPin), buttonPress, FALLING);

  turnOff();
}


void loop() {
  //setBrightness(); // try and move outside of main loop

}

void turnOff(){
  analogWriteInverted(rPin,0);
  analogWriteInverted(gPin,0);
  analogWriteInverted(bPin,0);

  currentVals[0] = 0;
  currentVals[1] = 0;
  currentVals[2] = 0;
}


// Ensure that buttonPress does not trigger twice
void buttonPress(){
  int reading = digitalRead(buttonPin);

  if (lastDebounceTime < 0 || (millis() - lastDebounceTime) > debounceDelay) {
    lastDebounceTime = millis();
    if(ledMode != 4){
        clearTimer();
    }
        setMode();
        buttonPressed = true;
        Timer1.attachInterrupt(incrementGlobalFadeInterval);
      }
    }


//Increment LED Mode until max level is reached
void setMode(){
  if(ledMode == maxLEDMode){
    ledMode = 0;
  } else {
    ledMode++;
  }
}


//Set LED colours depending on ledMode
void setBrightness(){
  buttonPressed = false;

  switch(ledMode){
    case 0: mode0(); break;
    case 1: mode1(); break;
    case 2: mode2(); break;
    case 3: mode3(); break;
    case 4: mode4(); break;
    case 5: mode5(); break;
    default: abort();
  }

}


//Fade to off
void mode0 (){
  fadeTo(0, 0, 0, 400);
}


// Fade to dim light
void mode1 (){
  int val = 255/5;
  fadeTo(val, val, val, 50);
}


// Fade to medium light
void mode2 (){
  int val = (255/3);
  fadeTo(val, val, val, 50);
}


// Fade to bright light
void mode3 (){
  fadeTo(255, 255, 255, 50);
}


// Endlessly cycle through RGB colours
void mode4(){
  switch(mode4Stage){
    case 0: fadeToWithCheck(255, 0, 0, 2000); break;
    case 1: fadeToWithCheck(255, 255, 0, 2000); break;
    case 2: fadeToWithCheck(0, 255, 0, 2000); break;
    case 3: fadeToWithCheck(0, 255, 255, 2000); break;
    case 4: fadeToWithCheck(0, 0, 255, 2000); break;
    case 5: fadeToWithCheck(255, 0, 255, 2000); break;
    default: abort();
  }
}


// Pause LED on colour inherited from Mode4
void mode5(){
  currentVals[0] = mode4Vals[0];
  currentVals[1] = mode4Vals[1];
  currentVals[2] = mode4Vals[2];

  int rExisting = currentVals[0];
  int gExisting = currentVals[1];
  int bExisting = currentVals[2];
  fadeTo(rExisting, gExisting, bExisting, 1);
}


// Fade to given colour, with awareness of mode4 toggles
void fadeToWithCheck(int r, int g, int b, int interval){
  if(ledMode == 4){
    int rExisting = currentVals[0];
    int gExisting = currentVals[1];
    int bExisting = currentVals[2];

    if(fadeInterval <= interval && lastSeenInterval != fadeInterval){
      lastSeenInterval = fadeInterval;
      analogWriteInverted(rPin, map(fadeInterval, 0, interval, rExisting, r));
      analogWriteInverted(gPin, map(fadeInterval, 0, interval, gExisting, g));
      analogWriteInverted(bPin, map(fadeInterval, 0, interval, bExisting, b));

      mode4Vals[0] = map(fadeInterval, 0, interval, rExisting, r);
      mode4Vals[1] = map(fadeInterval, 0, interval, gExisting, g);
      mode4Vals[2] = map(fadeInterval, 0, interval, bExisting, b);
    } else {
      currentVals[0] = map(fadeInterval, 0, interval, rExisting, r);
      currentVals[1] = map(fadeInterval, 0, interval, gExisting, g);
      currentVals[2] = map(fadeInterval, 0, interval, bExisting, b);
      fadeInterval = 0;

      if(mode4Stage == 5){
        mode4Stage = 0;
      } else {
        mode4Stage++;
      }
    }
  }
}


// Fade to given colour
void fadeTo(int r, int g, int b, int interval){
  int rExisting = currentVals[0];
  int gExisting = currentVals[1];
  int bExisting = currentVals[2];

  if(fadeInterval <= interval && fadeInterval != lastSeenInterval){
    lastSeenInterval = fadeInterval;
    analogWriteInverted(rPin, map(fadeInterval, 0, interval, rExisting, r));
    analogWriteInverted(gPin, map(fadeInterval, 0, interval, gExisting, g));
    analogWriteInverted(bPin, map(fadeInterval, 0, interval, bExisting, b));
  } else {
    clearTimer();
    currentVals[0] = r;
    currentVals[1] = g;
    currentVals[2] = b;
  }
}

void incrementGlobalFadeInterval(){
  fadeInterval++;
  setBrightness();
}

void clearTimer(){
  Timer1.detachInterrupt();
  fadeInterval = 0;
  lastSeenInterval = -1;
}

// Invert given value due to cathode/anode LED
void analogWriteInverted( byte pin, byte value)
{
  analogWrite(pin, 255-value);
}
  • Try switch function in C++. You'll find the info via a search engine. – MichaelT Jun 4 at 8:34
  • I'm guessing it will perform the same way as the switch in C#. That would tidy up my setBrightness function a lot. Good thinking! I'm not too clear on how this would avoid the use of the main loop function though? – Rob Jeffrey Jun 4 at 8:41
  • One other problem I see is you use delay, and a delay should not be used inside an interrupt. – Michel Keijzers Jun 4 at 8:50
  • Thanks Michel, is this because it is not best practice or can it cause fundamental issues? I put this in place because, when I pressed the button, the ISR was triggering a random number of times, sometimes once, sometimes up to 3 times. Have you seen an issue like that before? Sorry if this should be another post entirely. – Rob Jeffrey Jun 4 at 8:56
  • upvote for a well written post – jsotola Jun 4 at 15:26
1

Keep interrupt shorts

Inside an interrupt, do not use long pauses, thus not a delay. As chrisl notes below (see comment) delay does not even work inside an interrupt, since it is based on millis(), which are not updated within an interrupt.

The reason is that interrupts should be as short as possible (so even if you would use a for loop with micros() or another way to hold the execution time), because it can cause new interrupts not to be called (during an interrupt, no new interrupts will be called, at least not with the same/lower priority). So always keep code within interrupts very short, especially delay's are violating this, but also serial.print statements.

I see you use this function which will be called if you put everything in the interrupt handler:

// Fade to given colour
void fadeTo(int r, int g, int b, int interval){
  int rExisting = currentVals[0];
  int gExisting = currentVals[1];
  int bExisting = currentVals[2];

  for(int i = 0; i <= interval; i++){
    analogWriteInverted(rPin, map(i, 0, interval, rExisting, r));
    analogWriteInverted(gPin, map(i, 0, interval, gExisting, g));
    analogWriteInverted(bPin, map(i, 0, interval, bExisting, b));
    delay(10);
  }
  currentVals[0] = r;
  currentVals[1] = g;
  currentVals[2] = b;
}

Removing delays

To remove the delay, is a bit tricky though. What you need to do is (approximately):

  • Create a global variable instead of the loop variable (i, but give it a better name)
  • Calling an additional interrupt based on 10 ms (the delay).
  • In that interrupt, handle the next iteration (so increase the global variable i (but the new name)
  • Handle post processing (e.g. if i == interval, execute the last three statements of the function (and possibly other statements from called functions in the original code).

Multiple if to one switch statement

Also you can use a switch statement.. Instead of

 if(ledMode == 0){
    mode0();
  }

  if(ledMode == 1){
    mode1();
  }

  if(ledMode == 2){
    mode2();
  }

  if(ledMode == 3){
    mode3();
  }

  if(ledMode == 4){
    mode4();
  }

  if(ledMode == 5){
    mode5();
  }
}

Use

switch (ledMode)
{
case 0: 
  mode0();
  break;

case 1:
  mode1();
  break;

..

case 5:
  mode5();
  break;

default:
   abort(); // Error condition
}

Since the cases are so simple I put them normally on one line:

switch (ledMode)
{
case 0: mode0(); break;
case 1: mode1(); break;
..
case 5: mode5(); break;
default: abort();
}

Button debouncing

In your remark you say after a button press, sometimes the function is called multiple times. This is caused by button bouncing. When you press a button, for a short time, the contact is somewhere between on and off. During this time, it can change a few times between on and off, thus you get multiple calls, mostly very short after each other. To prevent this, use button debouncing (check the Arduino site for a Button Debounce example). It means that you check if a certain state has been stable for some time, and than perform the action, instead of immediately.

  • Great answer Michel, I'm pretty sure that all makes sense to me. I will give it a go and get back to you with my findings. – Rob Jeffrey Jun 4 at 9:00
  • Good luck, if it helps you, please consider upvoting/accepting the answer. – Michel Keijzers Jun 4 at 9:04
  • 1
    I thought, that delay() does not work at all inside of an ISR, because it relies on millis(), which in turn relies on interrupt for counting time. – chrisl Jun 4 at 9:26
  • @chrisl you are right, mostly, you can use milis(), but it is not updated, so the use of delay doesn't work indeed. I will adapt the answer. Thanks! – Michel Keijzers Jun 4 at 9:34
  • Thanks for the help Michel! Not sure if my solution is exactly what you imagined, but it certainly does the trick. I have updated my question with the working code. – Rob Jeffrey Jun 6 at 6:39
0

Remove the buttonPressed = false; from fadeToWithCheck. As in mode4 this function is called four times, only the current call is skipped if the button is pressed. The following calls are not skipped, because your reset buttonPressed back to false.

There is no need for this, as it's already reset back to false at the beginning of setBrightness.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.