Go Down

Topic: Newlines in code affecting interrupt functionality? (Read 2875 times) previous topic - next topic

concat

Hi everyone!

I've been trying to implement the interrupt library PinChangeInt (https://code.google.com/p/arduino-pinchangeint/) for multiple pins and ran into a very strange problem...

I currently have an Arduino Mega sending 1us pulses to an Arduino Pro Mini 5V 16MHz every 20ms: 32 in a row to pin 2 and 96 in a row to pin 3, the goal being to signal an analog measurement during the 'off' periods of a PWM signal (I know that these are also hardware interrupt pins anyways but I plan to expand to the rest of the pins later).

I tried coding from the ground up with some help from the quick and dirty code provided by the library creator, but was never able to capture both interrupts. Here was my original code:

Code: [Select]
#include <EEPROM.h>
#include <PinChangeInt.h>
uint8_t latest_interrupt;
uint8_t interrupt_count[20]={0}; // 20 possible arduino pins
uint8_t channel = 5;
uint8_t addr = 0;
boolean ADCflag = false;
uint8_t cur[8];
#define PIN1 2
#define PIN2 3
#define PIN3 4

void setup() {
 pinMode(4, OUTPUT);
 Serial.begin(9600);
 pinMode(PIN1, INPUT); digitalWrite(PIN1, HIGH);
 PCintPort::attachInterrupt(PIN1, &quicfunc, FALLING);  // add more attachInterrupt code as required
 pinMode(PIN2, INPUT); digitalWrite(PIN2, HIGH);
 PCintPort::attachInterrupt(PIN2, &quicfunc, FALLING);
 ADCSRA = B11001111; //prescaler = 128
 ADCSRB = B00000000;
 ADMUX = B01100000;
 ADMUX |= channel;
}
ISR(ADC_vect) {
 cur[channel] = ADCH;
 ADCSRA |= B11000000;
}
void quicfunc() {
 latest_interrupt=PCintPort::arduinoPin;
 interrupt_count[latest_interrupt]++;
};
void loop() {
 if(addr<512) {
   if(interrupt_count[2]>0) {
     EEPROM.write(addr, constrain(cur[channel],1,254));
     interrupt_count[2]--;
     addr++;
     digitalWrite(4, HIGH);
   }
   if(interrupt_count[3]>0) {
     EEPROM.write(addr, 255);
     interrupt_count[3]--;
     addr++;
     digitalWrite(13, HIGH);
   }
   if(ADCflag) {
     EEPROM.write(addr, 0);
     ADCflag = false;
     addr++;
   }
 }
}


I identified that neither LED - one of which I hooked up to pin 4 - lit up. I then tried to mould the code to an example I had tried and had worked from http://www.hackmaine.org/forums/viewtopic.php?f=6&t=13 until, after a lot of edits, I had arrived at this essentially identical script:

Code: [Select]
#include <PinChangeInt.h>
#define PIN1 2
#define PIN2 3
#define PIN3 4

uint8_t latest_interrupted_pin;
uint8_t interrupt_count[20]={0}; // 20 possible arduino pins
void quicfunc() {
 latest_interrupted_pin=PCintPort::arduinoPin;
 interrupt_count[latest_interrupted_pin]++;
};
void pin3func() {
 Serial.print("Pin "); Serial.print(PIN3, DEC); Serial.println("!");
}

void setup() {
 pinMode(PIN1, INPUT); digitalWrite(PIN1, HIGH);
 PCintPort::attachInterrupt(PIN1, &quicfunc, FALLING);  // add more attachInterrupt code as required
 pinMode(PIN2, INPUT); digitalWrite(PIN2, HIGH);
 PCintPort::attachInterrupt(PIN2, &quicfunc, FALLING);
 pinMode(PIN3, INPUT); digitalWrite(PIN3, HIGH);
 PCintPort::attachInterrupt(PIN3, &pin3func, CHANGE);
 Serial.begin(115200);
 Serial.println("---------------------------------------");
}
uint8_t i;
void loop() {
 uint8_t count;
 Serial.print(".");
 delay(1000);
 for (i=0; i < 20; i++) {
   if (interrupt_count[i] != 0) {
     count=interrupt_count[i];
     interrupt_count[i]=0;
     Serial.print("Count for pin ");
     if (i < 14) {
       Serial.print("D");
       Serial.print(i, DEC);
     } else {
       Serial.print("A");
       Serial.print(i-14, DEC);
     }
     Serial.print(" is ");
     Serial.println(count, DEC);
   }
 }
}


It still only captured one interrupt. However, adding a newline before uint8_t i; made it capture both interrupts successfully! What could be causing this behavior?? I hope I'm not overlooking something insanely silly :~

lar3ry

Let me see if I have this straight...

Before making the change, your program had this...

Code: [Select]

  Serial.println("---------------------------------------");
}uint8_t i;
void loop() {


Is that correct?

concat

Thanks for the quick reply!

Not quite: it had
Code: [Select]
}
uint8_t i;
void loop() {


and the code that worked had the newline:
Code: [Select]
}

uint8_t i;
void loop() {


I've also noticed that if I take out the interrupt on pin 4 by removing
Code: [Select]
pinMode(PIN3, INPUT); digitalWrite(PIN3, HIGH);
 PCintPort::attachInterrupt(PIN3, &quicfunc, FALLING);
and/or removing
Code: [Select]
#define PIN3 4 has an identical effect: only registering the interrupt on pin 3. Without removing the seemingly extraneous interrupt, it works like a charm.

Might a 1us pulse be too short to initialize an interrupt?

concat

Aha!!  :)

Changing the pulse width to 5us and it makes worlds of difference. I've got two interrupts clocking in basically perfectly. Still, I don't understand how these changes in code structure could cause the program to neglect the interrupt from one of the pins? At this point, I'm curious about how far the pulse width can be pushed shorter...

lar3ry


Robin2

Am I correct in thinking you have Serial.print[ln]() commands within your interrupt function?

Why?

Interrupt functions should have the absolute minimum in them and Serial.print functions have a huge overhead.

Make the interrupt set a flag and/or save a value and return from the interrupt as quickly as possible and elsewhere in your code get the Serial.print commands to work depending on the value of the flag.

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

MarkT

Firstly the first argument to attachInterrupt is _not_ the pin number, its the interrupt
number, and must be 0 for pin 2 and 1 for pin 3 on the Uno, Mini, Nano, Pro Mini etc.

Secondly if you call Serial.print in an ISR the whole system will freeze up.  You mustn't
call any function that waits on interrupts (Serial calls, delay()).  Global interrupts are
disabled automatically on entering an ISR, so nothing dependent on other interrupts
will work until you either leave the ISR or enable them.

Lastly you shouldn't take very long inside any ISR (this usually means a few microseconds),
just handle the immediate condition and update some volatile variables.  All variables
shared between ISRs and the main loop should be declared volatile or the compiler
may optimize away the loads and stores that are needed.
[ I will NOT respond to personal messages, I WILL delete them, use the forum please ]

PaulS

Quote
Firstly the first argument to attachInterrupt is _not_ the pin number, its the interrupt
number, and must be 0 for pin 2 and 1 for pin 3 on the Uno, Mini, Nano, Pro Mini etc.

OP is using pin change interrupts, which play by different rules.

What isn't clear to me is what value is in PCintPort::arduinoPin when the interrupt fires. Using that as an index into an array, where the index usually starts at 0 strikes me as a bad idea.

Quote
Secondly if you call Serial.print in an ISR the whole system will freeze up.

Where do you see that being done? The ISR is:
Code: [Select]
void quicfunc() {
  latest_interrupt=PCintPort::arduinoPin;
  interrupt_count[latest_interrupt]++;
};


Quote
Lastly you shouldn't take very long inside any ISR

I can't really see a way to make the ISR much shorter.
The art of getting good answers lies in asking good questions.

lar3ry


Quote
Secondly if you call Serial.print in an ISR the whole system will freeze up.

Where do you see that being done? The ISR is:
Code: [Select]
void quicfunc() {
  latest_interrupt=PCintPort::arduinoPin;
  interrupt_count[latest_interrupt]++;
};



It's in the PinChange Iterrrupt...
Code: [Select]

void pin3func() {
  Serial.print("Pin "); Serial.print(PIN3, DEC); Serial.println("!");
}

concat


It's in the PinChange Iterrrupt...
Code: [Select]

void pin3func() {
  Serial.print("Pin "); Serial.print(PIN3, DEC); Serial.println("!");
}



Sorry if I wasn't more specific: pin3func() is never called. Pin 4 is not connected to anything (in retrospect, I should have grounded it, but still, "Pin 4!" never showed up in the Serial monitor); I kept the function there in fear that removing it would cause the program to break.


What isn't clear to me is what value is in PCintPort::arduinoPin when the interrupt fires. Using that as an index into an array, where the index usually starts at 0 strikes me as a bad idea.


PCintPort::arduinoPin corresponds to the *pin number* on the Arduino, so 0 for RX, 13 for D13 and 14 for A0. The value shouldn't ever exceed 20 on the Uno or Duemillanove and 22 on the Mini (which raises a good point... I should change the array size to 22 since I am using a Mini, thanks for that!). What other repercussions could there be for using PCintPort::arduinoPin as an array index?


Have a look at Nick Gammon's excellent Interrupts tutorial at http://gammon.com.au/interrupts



I did skim through it before but I guess now's a good time to buckle down and actually absorb some information!

PaulS

Quote
What other repercussions could there be for using PCintPort::arduinoPin as an array index?

If the values do range from 0 to 22, then using it as an index is fine.
The art of getting good answers lies in asking good questions.

Vaclav


Firstly the first argument to attachInterrupt is _not_ the pin number, its the interrupt
number, and must be 0 for pin 2 and 1 for pin 3 on the Uno, Mini, Nano, Pro Mini etc.

Secondly if you call Serial.print in an ISR the whole system will freeze up.  You mustn't
call any function that waits on interrupts (Serial calls, delay()).  Global interrupts are
disabled automatically on entering an ISR, so nothing dependent on other interrupts
will work until you either leave the ISR or enable them.

Lastly you shouldn't take very long inside any ISR (this usually means a few microseconds),
just handle the immediate condition and update some volatile variables.  All variables
shared between ISRs and the main loop should be declared volatile or the compiler
may optimize away the loads and stores that are needed.



And here is a quote form another source telling the real story.

Quote
Interrupts events (that is, noticing the event) can occur at any time, and most are remembered by setting an "interrupt event" flag inside the processor. If interrupts are disabled, then that interrupt will be handled when they are enabled again, in priority order.


Would it be too offensive to all your " gurus "  to quit spreading this nonsense about "short" interrupts?
Or at least read the often referenced article about the interrupts and since you love including stuff in quotes - quote it right?
Arduino has its limits, but you guys just love to make it look worst than it is.
Off my soap box. Happy New year.

AWOL


Would it be too offensive to all your " gurus "  to quit spreading this nonsense about "short" interrupts?
Or at least read the often referenced article about the interrupts and since you love including stuff in quotes - quote it right?
Arduino has its limits, but you guys just love to make it look worst than it is.


From you Vaclav, yes it would be offensive.
Don't put serial prints in interrupts - if the processor is in a busy wait for some space to come free in the TX buffer inside an ISR, with interrupts disabled, then those interrupts aren't going to come, ever, are they?

Go Up