Calling attachinterrupt() generates an interrupt

Hi!
I am using an Arduino Nano in a project, part of which requires to PWM control a DC brush motor (through a transistor). Two keys control for control are connected to the two interrupt pins and their other terminals are connected to a digital pin. When I press the button, the motor should run for a certain time at certain speeds, then stop. I have written two interrupt handlers which basically set a flag whenever any of the keys is pressed, which is then detected in the main loop and reset when required.

The problem is, when the DC brush motor is operated at high power, it produces lots of electrical noise which generates interrupts on the keyboard handlers. To avoid these erroneous interrupts I am trying to disable interrupts in the main routine once the interrupt flag was set, and then re-enable them when motor is OFF.

However, I am witnessing a strange misbehavior: as soon as I call attachinterrupt() in the main code to re-enable interrupts, this generates an external interrupt and the handler is executed for the second time. I.e. when I push the button, the algorithm is executed twice. This happens exactly when attachinterrupt() is called: when I put a delay just before calling this function, the second (erroneous) run of the routine is delayed by the specified time period.

Please help me figure out how I can avoid this problem. I am attaching part of my code below (irrelevant parts trimmed):

/* Arduino keys
2	Int, Kbd1
3	Int, Kbd2
4	KbdCommon
5	PWM	        Fan
*/

#define OFF 0
#define ON 1

unsigned long Tstart=0, currentmillis=0; // time=0, 
// Tstart - the moment when the key was pressed
byte duration=5; // Motor operation in seconds
int seconds = 0; // number of seconds since the button was pushed
int count=0; // loop cycles counter

boolean state = OFF; // flag, if ON, the motor must be running

const byte FAN_PWM = 5; // Motor is connected to pin 5

const byte keyboardPin = 4;         // common pin for the key buttons
const byte key1InterruptPin = 2;  // key1 pin
const byte key2InterruptPin = 3;  // key2 pin
const byte key1Interrupt = 0;        // key1 attached to int0
const byte key2Interrupt = 1;        // key1 attached to int0

byte currentFanPower=0;

void setFanPower (int power) {
  if (power>255) power=255;
  if (power<0) power=0;
  analogWrite(FAN_PWM, power);
}

// Interrupt handler if Key 1 is pressed
void key1pressed (){ 
    seconds = 0;
    state = ON;
    Tstart=millis();  // note the time when the key was pressed
}

// Interrupt handler if Key 2 is pressed (currently both keys doing the same)
void key2pressed (){
    seconds = 0;
    state = ON;
    Tstart=millis();   // note the time when the key was pressed
}

void setup() {
// initialize the keyboard  
  pinMode(keyboardPin, OUTPUT); 
  digitalWrite(keyboardPin, LOW); // connect the common pin for two buttons to ground

  pinMode(key1InterruptPin, INPUT);
  digitalWrite(key1InterruptPin, HIGH); // pull up
  pinMode(key2InterruptPin, INPUT);
  digitalWrite(key2InterruptPin, HIGH); // pull up  
  attachInterrupt(key1Interrupt, key1pressed, FALLING); 
  attachInterrupt(key2Interrupt, key2pressed, FALLING);
  
//initialize PWM pin  
  pinMode(FAN_PWM, OUTPUT);
  analogWrite(FAN_PWM, 0);
}

void loop() {
  if (state) {
    detachInterrupt(key1Interrupt);  // disable interrupts on key1 
    detachInterrupt(key2Interrupt);  // disable interrupts on key2 
    currentmillis=millis();
    seconds=(currentmillis-Tstart)/1000;
    if (seconds<=duration){
       currentFanPower=92; // set FAN power, 0...255 (92 = 18.5V)
    } else {
      state=OFF;
      seconds=0;
      currentFanPower=0; // Turn OFF the motor
      attachInterrupt(key1Interrupt, key1pressed, FALLING);  // re-enable interrupt on key1
      attachInterrupt(key2Interrupt, key2pressed, FALLING);  // re-enable interrupt on key1
    }
  }
   setFanPower(currentFanPower); // enable actual PWM output on the pin
}

In the future, post code that compiles.

Your code has race conditions. You are much better off leaving interrupts always enabled. It will make it easier to eliminate the race conditions. I think this will work...

/* Arduino keys
2	Int, Kbd1
3	Int, Kbd2
4	KbdCommon
5	PWM	        Fan
*/

#define OFF 0
#define ON 1

unsigned long Tstart=0, currentmillis=0; // time=0, 
// Tstart - the moment when the key was pressed
byte duration=5; // Motor operation in seconds
int seconds = 0; // number of seconds since the button was pushed
int count=0; // loop cycles counter

boolean state = OFF; // flag, if ON, the motor must be running

const byte FAN_PWM = 5;

const byte keyboardPin = 4;
const byte key1InterruptPin = 2;
const byte key2InterruptPin = 3;
const byte key1Interrupt = 0;
const byte key2Interrupt = 1;

byte currentFanPower=0;
boolean Error=0;

void setFanPower (int power) {
  if (power>255) power=255;
  if (power<0) power=0;
  analogWrite(FAN_PWM, power);
}

// Interrupt handler if Key 1 is pressed
void key1pressed ()
{ 
  if ( ! state )
  {
    seconds = 0;
    state = ON;
    Tstart=millis();
  }
}

// Interrupt handler if Key 2 is pressed (currently both keys doing the same)
void key2pressed (){
  if ( ! state )
  {
    seconds = 0;
    state = ON;
    Tstart=millis();
  }
}

void setup() {
// initialize the keyboard  
  pinMode(keyboardPin, OUTPUT); 
  digitalWrite(keyboardPin, LOW); // connect the common pin for two buttons to ground

  pinMode(key1InterruptPin, INPUT);
  digitalWrite(key1InterruptPin, HIGH); // pull up
  pinMode(key2InterruptPin, INPUT);
  digitalWrite(key2InterruptPin, HIGH); // pull up  
  attachInterrupt(key1Interrupt, key1pressed, FALLING); 
  attachInterrupt(key2Interrupt, key2pressed, FALLING);
  
//initialize PWM pins  
  pinMode(FAN_PWM, OUTPUT);
  analogWrite(FAN_PWM, 0);
}

void loop() {
  if (state) {
    currentmillis=millis();
    seconds=(currentmillis-Tstart)/1000;
    if (seconds<=duration){
       currentFanPower=92; // 0...255 92 = 18.5V as in the original
    } else {
      state=OFF;
      seconds=0;
      currentFanPower=0; // Turn OFF the motor
    }
  }
   setFanPower(currentFanPower); // enable actual PWM output on the pin
}

Coding Badly, sorry, I left a couple of extra brackets when trimming the code and didn't check before posting. Won't do that again.
Thanks for the code suggestions. I'll try your code and will post the results.

So, is it considered normal that disabling and re-enabling interrupts may generate erroneous interrupts?
Actually, when I just started working on this code I was facing this problem also within setup(): as soon as I called attachinterrupt() for the first time, this used to generate a false interrupt, I even tried to write a routine attempting to ignore the first call of the interrupt. And then somehow this was solved - don't know what has fixed this initial error...

vahegan:
So, is it considered normal that disabling and re-enabling interrupts may generate erroneous interrupts?

No. It's the result of a bug in attachInterrupt. The discussion and solution are available in this forum.

And then somehow this was solved - don't know what has fixed this initial error...

Probably enabling the internal pull-ups before calling attachInterrupt.

Posting #8 in External interrupt fires early? - Microcontrollers - Arduino Forum

seems to be relevant here - attachInterrupt allows the normal hardware response
as it simply enables the interrupt, so any pending interrupt flags get responded to.

The code fragment in the above posting disables global interrupts then enables the
interrupt then clears the relevant flag, then re-enables global interrupts.

If you don't want to mess with attachInterrupts all you need to do is guard your
interrupt routine with a boolean:

volatile boolean int0_guard = false ;

void my_int0 ()
{
  if (!int0_guard)   // unless the guard is true simply ignore this interrupt
    return ;
  ..
  ..
}

void setup ()
{
  ..
  attachInterrupt (0, my_int0, FALLING) ;
  int0_guard = true ;   // only set the guard variable after attachInterrupt
  ..
}

I think, MarkT is right in that

MarkT:
attachInterrupt allows the normal hardware response as it simply enables the interrupt, so any pending interrupt flags get responded to.

I compiled your code suggestion. Without checking status in handler, if I press the button while the motor is running, the motor timer resets to zero and starts counting again.

If I add the status check to interrupt handler, button press while the motor is running does not affect the seconds counter and the motor continues to run until the specified duration. However it remembers that a pending interrupt flag was set and, upon completion of the cycle it generates an interrupt and then runs the motor for the second cycle.

vahegan:
I compiled your code suggestion. Without checking status in handler, if I press the button while the motor is running, the motor timer resets to zero and starts counting again.

You modified the code?

Finally, it worked, by utilization of both suggested methods simultaneously: checking the machine state in the interrupt handler, along with interrupt guards:

void key1pressed () {
  if (!key1int_guard) return;
  if (state==0) {  
  seconds = 0;
  state = 1;
  Tstart=millis();
  key2int_guard=false;
  }

Thank you, everyone, for your invaluable contributions!
P.S. I think, I'll move away from using interrupts on the keyboard towards just checking the keyboard pins on every cycle of the main loop. I hope this way I'll be able to use the keys even when the motor is running. Will see...