GreyGnome / EnableInterrupt

New Arduino interrupt library, designed for Arduino Uno/Mega 2560/Leonardo/Due
329 stars 73 forks source link

FALLING function is never fired #57

Closed g105b closed 5 years ago

g105b commented 5 years ago

Thank you for your work on this library!

I'm using ATMEGA328P chip. Here is my setup function:

void setup() {
  pinMode(LED_BUILTIN, OUTPUT);
  Serial.begin(9600);
  while(!Serial);
  Serial.setTimeout(1000);

  pinMode(PIN_COUNTER_1, INPUT_PULLUP);
  digitalWrite(LED_BUILTIN, HIGH);
  enableInterrupt(PIN_COUNTER_1, c1_fall, FALLING);
  enableInterrupt(PIN_COUNTER_1, c1_rise, RISING);
}

Within c1_rise function, the c1_rstatus variable is incremented successfully.

Within c1_fall function, the c1_fstatus variable is never incremented.

I can't figure out what I'm doing wrong with the library.

Here's my full ino script:

#include <EnableInterrupt.h>

#define PIN_COUNTER_1 9
#define DEBOUNCE 10
#define COOLOFF 1000
#define DELAY_PRINT 50

volatile unsigned long c1 = 0;
volatile unsigned long last_c1 = 0;
volatile unsigned long c1_ftime = 0;
volatile unsigned long c1_rtime = 0;

volatile unsigned long c1_fstatus = 0;
volatile unsigned long c1_rstatus = 0;

unsigned int t = 0;
unsigned int last_t = 0;

void output() {
  if(t - last_t < DELAY_PRINT) {
    return;
  }

  if(c1 == last_c1) {
    return;
  }

  Serial.print("C1\t");
  Serial.println(c1);
  Serial.print("ftime, rtime: ");
  Serial.print(c1_ftime);
  Serial.print(", ");
  Serial.println(c1_rtime);
  Serial.print(c1_fstatus);
  Serial.print(", ");
  Serial.println(c1_rstatus);
  last_c1 = c1;
  last_t = t;
}

// Only register a falling edge if it's long enough away from the previous rising edge (debounce)
void c1_fall() {
  unsigned long itime = millis();

  c1_fstatus = 1;

  // If a falling edge has been registered
  // and the time since last falling edge is less than debounce time,
  // ignore!
  if(c1_ftime > 0
  && itime - c1_ftime < DEBOUNCE) {
    return;
  }

  c1_fstatus = 2;

  // If a rising edge has been registered
  // and the time since last rising edge is less than cooloff time,
  // ignore!
  if(c1_rtime > 0
  && itime - c1_rtime < COOLOFF) {
    return;
  }

  c1_fstatus = 3;

  // Register a successful falling edge.
  c1_ftime = itime;
}

void c1_rise() {
  unsigned long itime = millis();

  c1_rstatus = 1;

  // If a rising edge has been registered
  // and the time since last rising edge is less than debounce time,
  // ignore!
  if(c1_rtime > 0
  && itime - c1_rtime < DEBOUNCE) {
    return;
  }

  c1_rstatus = 2;

  // If the time since last falling edge is less than colloff time,
  // ignore!
  if(itime - c1_ftime < COOLOFF) {
    return; // Too soon - cool off!
  }

  c1_rstatus = 3;

  c1_rtime = itime;
  c1++;
}

void c1_change() {
  unsigned long itime = millis();

  c1_rstatus = 1;

  // If a rising edge has been registered
  // and the time since last rising edge is less than debounce time,
  // ignore!
  if(c1_rtime > 0
  && itime - c1_rtime < DEBOUNCE) {
    return;
  }

  c1_rstatus = 2;

  // If the time since last falling edge is less than colloff time,
  // ignore!
  if(itime - c1_ftime < COOLOFF) {
    return; // Too soon - cool off!
  }

  c1_rstatus = 3;

  c1_rtime = itime;
  c1++;
}

void setup() {
  pinMode(LED_BUILTIN, OUTPUT);
  Serial.begin(9600);
  while(!Serial);
  Serial.setTimeout(1000);

  pinMode(PIN_COUNTER_1, INPUT_PULLUP);
  digitalWrite(LED_BUILTIN, HIGH);
  enableInterrupt(PIN_COUNTER_1, c1_fall, FALLING);
  enableInterrupt(PIN_COUNTER_1, c1_rise, RISING);
}

void loop() {
  t = millis();
  output();
  delay(10);
}
jimbowley commented 5 years ago

Can you have more than one interrupt type on a single pin (at the same time)? I wouldn't have guessed that.

GreyGnome commented 5 years ago

Bowler, Bowley is correct. 1 interrupt type per pin please. You want CHANGE. You'll need to check the state of your pin, within the interrupt handler, to know if you rose or fell getting there. As the docs say, "These interrupts can be set to trigger on RISING, or FALLING, or both ("CHANGE") signal levels, or on LOW level..."

"OR' only- no "AND"s. So it's, "RISING" or "FALLING" or "CHANGE".

Good luck. Let me know how it goes.

On Fri, Aug 23, 2019 at 10:26 AM jimbowley notifications@github.com wrote:

Can you have more than one interrupt type on a single pin (at the same time)? I wouldn't have guessed that.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/GreyGnome/EnableInterrupt/issues/57?email_source=notifications&email_token=AA2KFGJC6OECXFC44SYLTSTQF76T5A5CNFSM4IPAUEU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5ARFRI#issuecomment-524358341, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2KFGKALDDIPQD4AYKOIS3QF76T5ANCNFSM4IPAUEUQ .

-- -Mike Schwager

GreyGnome commented 5 years ago

...or "LOW". I forgot the "LOW".

On Fri, Aug 23, 2019 at 3:55 PM Michael Schwager mschwage@gmail.com wrote:

Bowler, Bowley is correct. 1 interrupt type per pin please. You want CHANGE. You'll need to check the state of your pin, within the interrupt handler, to know if you rose or fell getting there. As the docs say, "These interrupts can be set to trigger on RISING, or FALLING, or both ("CHANGE") signal levels, or on LOW level..."

"OR' only- no "AND"s. So it's, "RISING" or "FALLING" or "CHANGE".

Good luck. Let me know how it goes.

On Fri, Aug 23, 2019 at 10:26 AM jimbowley notifications@github.com wrote:

Can you have more than one interrupt type on a single pin (at the same time)? I wouldn't have guessed that.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/GreyGnome/EnableInterrupt/issues/57?email_source=notifications&email_token=AA2KFGJC6OECXFC44SYLTSTQF76T5A5CNFSM4IPAUEU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5ARFRI#issuecomment-524358341, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2KFGKALDDIPQD4AYKOIS3QF76T5ANCNFSM4IPAUEUQ .

-- -Mike Schwager

-- -Mike Schwager

GreyGnome commented 5 years ago

Ugh, "LOW" is only on External Interrupts. That should be noted.

On Fri, Aug 23, 2019 at 3:56 PM Michael Schwager mschwage@gmail.com wrote:

...or "LOW". I forgot the "LOW".

On Fri, Aug 23, 2019 at 3:55 PM Michael Schwager mschwage@gmail.com wrote:

Bowler, Bowley is correct. 1 interrupt type per pin please. You want CHANGE. You'll need to check the state of your pin, within the interrupt handler, to know if you rose or fell getting there. As the docs say, "These interrupts can be set to trigger on RISING, or FALLING, or both ("CHANGE") signal levels, or on LOW level..."

"OR' only- no "AND"s. So it's, "RISING" or "FALLING" or "CHANGE".

Good luck. Let me know how it goes.

On Fri, Aug 23, 2019 at 10:26 AM jimbowley notifications@github.com wrote:

Can you have more than one interrupt type on a single pin (at the same time)? I wouldn't have guessed that.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/GreyGnome/EnableInterrupt/issues/57?email_source=notifications&email_token=AA2KFGJC6OECXFC44SYLTSTQF76T5A5CNFSM4IPAUEU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5ARFRI#issuecomment-524358341, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2KFGKALDDIPQD4AYKOIS3QF76T5ANCNFSM4IPAUEUQ .

-- -Mike Schwager

-- -Mike Schwager

-- -Mike Schwager

g105b commented 5 years ago

Thank you, that answered my issue and the project is working fine now.

Just to be pedantic,

"These interrupts can be set to trigger on RISING, or FALLING, or both ("CHANGE").

The sentence does literally say that they can be set to trigger on rising, falling or both. I fully understand the issue I was having now, and have to say that your documentation is actually really refreshingly well written, but that one sentence is obviously the one that caught me out. I read it as saying I could trigger on both.

Thanks for the library, and thanks for your answers! Have a great week.

GreyGnome commented 5 years ago

OK, I'll spell it out just a little bit more in the docs. Thanks for the feedback.

On Mon, Aug 26, 2019 at 10:22 AM Greg Bowler notifications@github.com wrote:

Thank you, that answered my issue and the project is working fine now.

Just to be pedantic,

"These interrupts can be set to trigger on RISING, or FALLING, or both ("CHANGE").

The sentence does literally say that they can be set to trigger on rising, falling or both. I fully understand the issue I was having now, and have to say that your documentation is actually really refreshingly well written, but that one sentence is obviously the one that caught me out. I read it as saying I could trigger on both.

Thanks for the library, and thanks for your answers! Have a great week.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/GreyGnome/EnableInterrupt/issues/57?email_source=notifications&email_token=AA2KFGN7OAH3XUA4FAHHV7DQGPYKTA5CNFSM4IPAUEU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5EWOLA#issuecomment-524904236, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2KFGMWHXLWZ5VJSZX4BJTQGPYKTANCNFSM4IPAUEUQ .

-- -Mike Schwager

g105b commented 5 years ago

May I suggest:

These interrupts can be set to trigger on one of three values: RISING, FALLING or CHANGE (for both).

GreyGnome commented 5 years ago

Nice, I like it!

On Tue, Aug 27, 2019 at 3:36 AM Greg Bowler notifications@github.com wrote:

May I suggest:

These interrupts can be set to trigger on one of three values: RISING, FALLING or CHANGE (for both).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/GreyGnome/EnableInterrupt/issues/57?email_source=notifications&email_token=AA2KFGLCASQKXVNCS75OL7DQGTRPHA5CNFSM4IPAUEU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5G63AI#issuecomment-525200769, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2KFGNRSUWDAB2VJASDSWDQGTRPHANCNFSM4IPAUEUQ .

-- -Mike Schwager