Interrupt doesn´t trigger "if"statment.

Hello folks,

I have a super simple code. Basically i want to drive a LED (and eventually a MOSFET) with a pwm signal which is controlled with a poti. Furthermore i want a push button to trigger an interrupt which flags a variable and turns the LED shortly off and on again.
I have tested the interrupt and the pwm part alone but once i combine these two the LED "if"statemant does not work.

int pwmPin = 10;
int buttonPin = 45;
volatile int potiPin = A2;

int brightness = 0;
boolean flag = 0;
int debounceTime = 10;
long unsigned int lastPress;
boolean lastFlag = 0;



void ISR_FLAG(){
flag = !flag;
lastPress = millis();
}



void setup() {

  Serial.begin(9600);
  pinMode(buttonPin, INPUT);
  pinMode(pwmPin,OUTPUT);
  attachInterrupt(digitalPinToInterrupt(buttonPin), ISR_FLAG, RISING);
  }


void loop() {
  
   if ((millis() - lastPress) > debounceTime && lastFlag != flag){
    analogWrite(pwmPin, 0);
    delay(500);
    analogWrite(pwmPin, 255);
    delay(500);
  }
  
  Serial.print(flag);
  delay(200);
  Serial.print(brightness);
  brightness = analogRead(potiPin);
  brightness = map(brightness, 0, 1023, 0, 255);
  analogWrite(pwmPin, brightness);
  
 lastFlag = flag;
}
void ISR_FLAG(){
flag = !flag;
lastPress = millis();
}

These 2 variables get modified inside the ISR and therefore should be declared 'volatile'

long unsigned int lastPress;
boolean lastFlag = 0;

long unsigned int ? you mean unsigned long ?if ((millis() - lastPress) > debounceTime && lastFlag != flag){also there may be an issue with lastPress, being a a 4 byte variable you should turn interrupts 'off', copy it's value to another variable (or do the compare i suppose that doesn't matter) and turn the interrupts 'on' again, to prevent it's value being modified during the comparison, causing an anomaly.volatile int potiPin = A2;this is actually a constant, so doesn't need to be declared volatile.

Furthermore i want a push button to trigger an interrupt

Interrupts are not the correct tool to detect button presses. See under example / 02. Digital / debounce in the IDE. Also search this site for other examples as the question comes up a lot.

Hello you two,

im sorry thati didn´t reply any earlyer but i really couldn´t find the time.

Deva_Rishi:

Thanks for clearing up the variables i don´t really know what to use as you noticed.

also there may be an issue with lastPress, being a a 4 byte variable you should turn interrupts 'off', copy it's value to another variable (or do the compare i suppose that doesn't matter) and turn the interrupts 'on' again, to prevent it's value being modified during the comparison, causing an anomaly.

Do you mean like this?:

  noInterrupts();
  
   if ((millis() - lastPress) > debounceTime && lastFlag != flag){
    analogWrite(pwmPin, 0);
    delay(500);
    analogWrite(pwmPin, 255);
    delay(500);
  }

  interrupts();

It doesn´t solve the problem completly but it does enable the Serial.print() function to work. So the values are displayed in the monitor and the button press is detected but the if statement isn´t working.

@PerryBebbington

What is the correct way of reading a button? Everywhere i look it says pulling is not preffered and that i have to use interrputs. Is there another way? Maybe just give me a keyword and i´ll look for myself.

Cheers guys!

 noInterrupts();

if ((millis() - lastPress) > debounceTime && lastFlag != flag){
    analogWrite(pwmPin, 0);
    delay(500);
    analogWrite(pwmPin, 255);
    delay(500);
  }

interrupts();

That code is not great because you leave interrupts disabled for a whole second when the "if" statement evaluates TRUE. Interrupts will get missed if you delay their execution for too long.

This is better...

noInterrupts();
bool debounced = millis() - lastPress > debounceTime && lastFlag != flag;
interrupts();
if (debounced)
{
   ....
}

Or even better take a copy of your volatile variable(s)...

noInterrupts();
uint32_t copyLastPress = lastPress;
bool copyFlag = flag;
interrupts();
if (millis() - copyLastPress > debounceTime && lastFlag != copyFlag){
{
   ....
}

What is the correct way of reading a button? Everywhere i look it says polling (sp.) is not preferred and that i have to use interrupts. Is there another way? Maybe just give me a keyword and i´ll look for myself.

Where does it say that?

For Arduino platform polling, and not using delay() is the best way. The problem with interrupts alone is it doesn't make your program any more responsive to button presses. To do that you must eliminate blocking code, and once you have done that you might as well poll the button in the first place.

Farquarl:
@PerryBebbington

What is the correct way of reading a button? Everywhere I look it says pulling is not preferred and that i have to use interrupts. Is there another way? Maybe just give me a keyword and i´ll look for myself.

Hello again,
I can't imagine where you read that! Interrupts are not the correct tool for reading button presses and the subject comes up often on here always with the same answer. I've already said where to look to learn how to use polling to read buttons, here is a reminder:

See under example / 02. Digital / debounce in the IDE.

As pcbbc says, you have to get rid of delays and write non blocking code, these will help you:
Using millis for timing
Demonstration for several things at the same time

I´ve watched serveral youtube videos where they describe that and interrupt is a good alternative to polling because it doesn´t matter where you are in the loop that the press is recognized. But after googling polling vs interrupt with a button i realised you are right. I guess i just misjudged the time importance. I know the debouncing tutorial but i thought it was just to showcase exactly that and not a guide how to use buttons. Anyway thank you guys for sticking with me, hearing the same questions over and over again must be tiring. i really appreaciate your help.

I will rewrite the code without an interrupt and using the the proper millis() way.

Edit: I wanted to use delay because i didn´t want the pwm level of the poti to "overwrite" the full off and on time of the pwmPin. But i guess that my logic was flawed.

Farquarl:
I've watched several YouTube videos where they describe that and interrupt is a good alternative to polling because it doesn't matter where you are in the loop that the press is recognized

There's an awful lot of rubbish on YouTube!

Anyway thank you guys for sticking with me, hearing the same questions over and over again must be tiring. i really appreciate your help.

You're welcome :slight_smile:

Farquarl:
Edit: I wanted to use delay because i didn´t want the pwm level of the poti to "overwrite" the full off and on time of the pwmPin. But i guess that my logic was flawed.

Then the correct way to do that is either...
Constantly read the pot, but store the value to a temporary variable
When the PWM has completed a full on/off cycle, update the real variable from the temporary copy

Or...
Only read the pot value at the end of a complete on/off cycle
(There's no need reading it every time through loop if you aren't going to use it)

Deva_Rishi:

long unsigned int lastPress;

long unsigned int ? you mean unsigned long ?

FYI: It’s just a matter of style. The keywords ‘long’, ‘short’, and ‘unsigned’ can be in any order and they all imply ‘int’ and can be used with the keyword ‘int’ in any order. This sketch compiles without warnings or errors:

int a;
unsigned b;
long c;
short d;

int unsigned e;
int long f;
int short g;

unsigned int h;
long int i;
short int j;

unsigned long k;
long unsigned l;
int unsigned long m;
int long unsigned n;
unsigned int long o;
long int unsigned p;
unsigned long int q;
long unsigned int r;

unsigned short s;
short unsigned t;
int unsigned short u;
int short unsigned v;
unsigned int short w;
short int unsigned x;
unsigned short int y;
short unsigned int z;

void setup() {}
void loop() {}

Okay i have rewritten the code and it is now based on polling and millis() instead of delay.
Somehow the led doesn´t turn on at all. There must be a mistake i´ve made but i can´t find it.

Also is this the right way to use it? Or should i use while() in this case? (tried it with the same result)

int pwmPin = 10;
int buttonPin = 45;
int potiPin = A2;


int debounceTime = 20;
int halfCycle = 500;
int fullCycle = 1000;
int cycle = 0;
volatile long int brightness;
volatile boolean flag = 0;
int lastFlag;
volatile long int lastPress;



void setup() {
Serial.begin(9600);
pinMode(pwmPin, OUTPUT);
pinMode(buttonPin, INPUT);

}

void loop() {
  
  brightness = analogRead(potiPin); 
  brightness = map(brightness, 0, 1023, 0, 255);
  flag = digitalRead(buttonPin);
 
  if(cycle =  0){
     analogWrite(pwmPin, brightness);
   
  }
  
    if(flag != lastFlag){
    lastPress = millis();
    lastFlag = flag;
    
    if((millis() - lastPress) >= debounceTime){
      cycle = 1;
      int currentMillis = millis();
      
      if((millis() - currentMillis) <= halfCycle){
        digitalWrite(pwmPin, LOW);
      }
      if((millis() - currentMillis) <= fullCycle){
        digitalWrite(pwmPin, HIGH);
      }
      if((millis() - currentMillis) > fullCycle){
      cycle = 0;
      }
    }
      
      
  }
     
  }

Edit:
Okay i found the mistake of the pwm signal not comming thourgh
if(cycle = 0) must be if(cycle ==0);

if(cycle =  0){oops !

There is no inline documentation (comments) in your code, so please explain how it works.

aarg:
There is no inline documentation (comments) in your code, so please explain how it works.

Sure thing

i want to control a mosfet via a poti and pwm. When i press a button the pwmPin should be LOW half a second an 100% dutycycle for half a second. Afterwards the pwm level that the poti dictates should take over again.

Therefore i first read the poti and maps its values to be compatible with pwm

 brightness = analogRead(potiPin); 
  brightness = map(brightness, 0, 1023, 0, 255);

Im polling the button:

flag = digitalRead(buttonPin);

Checking if the button has been pressed and debouncing:

if(flag != lastFlag){
    lastPress = millis();
    lastFlag = flag;
    if((millis() - lastPress) >= debounceTime)

When im sure the button is properly pressed i change the “cycle” variable to 1 to stop the pwmPin outputting the “brightness” level.
Afterwards the pwmPin should be low for 500 millis and on for 500 millis.

   cycle = 1;
      int currentMillis = millis();
      
      while((millis() - currentMillis) <= halfCycle){
        digitalWrite(pwmPin, LOW);
      }
      if((millis() - currentMillis) <= fullCycle){
        digitalWrite(pwmPin, HIGH);
      }
      if((millis() - currentMillis) > fullCycle){
      cycle = 0;

After a the duration of an full "on-off"cycle the pwm value should return to the poti level.

cycle = 0;

Look at it more like a state machine. You are either doing normal PWM, in the OFF portion after a button press or the ON portion of a button press

const int pwmPin = 10;
const int buttonPin = 45;
const int potiPin = A2;

const unsigned long OffTime = 500;
const unsigned long OnTime = 1000;

int cycle = 0;
int lastFlag;
unsigned long startTime;

void setup() {
  Serial.begin(9600);
  pinMode(pwmPin, OUTPUT);
  pinMode(buttonPin, INPUT);
}

void loop() {

  int brightness = analogRead(potiPin);
  brightness = map(brightness, 0, 1023, 0, 255);
  int flag = digitalRead(buttonPin);

  if (flag != lastFlag) {
    // button changed state
    if ( flag == HIGH ) {
      // button was pushed, start the cycle
      startTime = millis();
      cycle = 1;
      digitalWrite(pwmPin, LOW);
    }
    delay(50);  // debounce
  }
  lastFlag = flag;

  switch (cycle) {
    case 0: // normal PWM
      analogWrite(pwmPin, brightness);
      break;

    case 1:  // LED Off
      if (millis() - startTime >= OffTime) {
        // transistion to On time
        startTime = millis();
        cycle = 2;
        digitalWrite(pwmPin, HIGH);
      }
      break;

    case 2: // LED on
      if (millis() - startTime >= OnTime) {
        // transistion back to normal
        cycle = 0;
      }
      break;
  }
}

You code requires a pull-down resistor on your button. Do you have one installed? If not, it is better to declare the button pin as INPUT_PULLUP and wire one side of the button to the pin and the other side to ground. This changes the logic of the pin (LOW == pressed) so the code will need to change.

Farquarl:
Sure thing

i want to control a mosfet via a poti and pwm. When i press a button the pwmPin should be LOW half a second an 100% dutycycle for half a second. Afterwards the pwm level that the poti dictates should take over again.

Therefore i first read the poti and maps its values to be compatible with pwm

 brightness = analogRead(potiPin); 

brightness = map(brightness, 0, 1023, 0, 255);




Im polling the button:



flag = digitalRead(buttonPin);




Checking if the button has been pressed and debouncing: 


if(flag != lastFlag){
    lastPress = millis();
    lastFlag = flag;
    if((millis() - lastPress) >= debounceTime)




When im sure the button is properly pressed i change the "cycle" variable to 1 to stop the pwmPin outputting the "brightness" level.
Afterwards the pwmPin should be low for 500 millis and on for 500 millis.



cycle = 1;
      int currentMillis = millis();
     
      while((millis() - currentMillis) <= halfCycle){
        digitalWrite(pwmPin, LOW);
      }
      if((millis() - currentMillis) <= fullCycle){
        digitalWrite(pwmPin, HIGH);
      }
      if((millis() - currentMillis) > fullCycle){
      cycle = 0;




After a the duration of an full "on-off"cycle the pwm value should return to the poti level.



cycle = 0;

FFS your code works flawless. I have a Pulldown resistor installed. You use a delay() in your debounce function. Isn´t that “bad practise”? Or is it okay because its justa short duration and for this purpose it doesn´t matter?

Thanks alot!!

Farquarl:
FFS your code works flawless. I have a Pulldown resistor installed. You use a delay() in your debounce function. Isn´t that “bad practise”? Or is it okay because its justa short duration and for this purpose it doesn´t matter?

Thanks alot!!

Flawlessly. It is a bad practice for a final, working sketch. It’s fine for demo’ing something, because most loops don’t run anything that can’t be ignored for 50ms or so. It will crash some hardware, though. It’s very easy to modify to make it non-blocking, so people often post solutions that use a short delay, just to avoid clutter and to avoid getting sidetracked about the original problem.

Farquarl:
You use a delay() in your debounce function. Isn´t that "bad practise"? Or is it okay because its just a short duration and for this purpose it doesn't matter?

It matters. What people forget is this; maybe in well written code loop goes round in 1ms, that's 1000 times per second. A 19ms delay does not seem like much but it's not 19ms, it's 19ms every time around loop. So, the loop that was going round in 1ms without delay is now taking 20ms, so instead of 1000 times per second it is going round only 50 times per second with 95% of that time taken up in delay, so that 19ms delay is actually wasting 95% of the available processor time. Think what useful things you could be doing with all that wasted processor time.