What I learned 2day? May be a bad idea to call functions & pinwrites within ISR

So for the last 2 days I was chasing a problem.

I'm using interrupt to pick up a trigger on the pin. Falling edge, etc.

Whenever the interrupt gets triggered it fires off another pin (1 of 4) after some processing. I have some LED hooked up to those pins to show when the event is happening. They cycle through from 1 to 4.

For the life of me i couldn't figure out why it would sometimes freeze for a couple milliseconds then the LEDs would resume normal pattern. It would run ok for maybe 2 seconds then freezes for about 50ms then resume.

Using micros and some variables to count elapsed time during each function it would seem as if the MCU itself is not aware of the freeze. :o

I then removed that digitalWrite function call from within the ISR function and instead used a boolean flag which is reset to false outside of the ISR routine.

//digitalWrite(nextToFire,LOW); //I no longer use this,
	fireNow = true;	//signals that we can fire the plug now

Another function watches for fireNow to change to true for it to call the actual FireNow();
At the end of FireNow I reset fireNow to false;

So can someone say whether or not they have seen this phenomenon? It certain did work satisfactorily on my end to call the digitalWrite from within the interrupt called function.

I ended up using a regular function and a loop to pic

digitalWrite() within an ISR is OK.
Using millis()/micros() in an ISR is also OK providing you don't wait in the ISR for these to change value.
It is not clear from your description if you are using pin 1 of the arduino, but this is, say on a Uno, also used by the Serial functions.
Post the whole original code for a possible explanation.

The digitalWrite command performs a cli / sei sequence when setting or clearing the pin. This does not bode well when done inside an ISR. Check out the digitalWrite function definition in the wiring_digital.c file.

Don't use digitalWrite() in an interrupt routine -- use direct port I/O. It is much, much faster and cannot cause problems with interrupts.

To set a pin high or low (assuming PORTD, for example) use

PORTD |= (1<<bit_number); //HIGH on selected pin
PORTD &= ~(1<<bit_number); //LOW on selected pin

Of course the pin must have been selected OUTPUT previously. To read pin(s) of PORTx use PINx.

Jremington, I can't remember right now but iirc the direct port write was the last thing that I tested and it still caused problems.

Only thing that worked was placing the call it in the loop.

I'll double check later and post a response.

Having an interrupt simply set a flag is, in my view, the best way of handling interrupts; especially using multiple state machines that must respond to interrupts and when using micro-controllers that can generate multiple interrupts per microsecond.

And, best of all, much easier to debug software/hardware issues :slight_smile:

iirc the direct port write was the last thing that I tested and it still caused problems.

Then something else is wrong.

DBMcDonald:
The digitalWrite command performs a cli / sei sequence when setting or clearing the pin. This does not bode well when done inside an ISR. Check out the digitalWrite function definition in the wiring_digital.c file.

Can you explain your reasoning there, please?

OK So the results are in.

P̶O̶R̶T̶D̶ ̶/̶ ̶P̶O̶R̶T̶B̶ ̶w̶o̶r̶k̶s̶,̶ ̶e̶v̶e̶n̶ ̶w̶h̶e̶n̶ ̶r̶e̶f̶e̶r̶e̶n̶c̶e̶ ̶i̶n̶ ̶a̶n̶o̶t̶h̶e̶r̶ ̶f̶u̶n̶c̶t̶i̶o̶n̶.̶
My mistake, sketch wasn't uploaded properly, was uploaded to another on another usb port.
direct port doesn't work either.

DigitalWrite does not work well all the time, causes hiccups.

Just one final thing, can you double check this for me? Is the following code correct to set individual pins LOW? I want to make sure i'm hitting the right port.

 if (nextToSwitch == 7)//Pin 7
 {
 PORTD = PORTD &  B01111111; //Sets pin 7 LOW, without affecting other pins?
 }
 else if (nextToSwitch == 8)
 {
 PORTB = PORTB& B11111110; //Sets pin 8 LOW, etc?
 }
 else if (nextToSwitch == 9)
 {
 PORTB = PORTB & B11111101;
 }
 else if (nextToSwitch == 10)
 {
 PORTB = PORTB & B11111011;
 }

Jremington, thanks for the explanation, i did check the code and I saw the cli(;

AWOL,
Code within an ISR routine that stops other interrupts from occurring, especially in a multiple interrupt driven construct, could cause unpredictable behaviour. Simple sketches would probably not be impacted, but as code becomes more complex (for me, that implies multiple state machines with multiple interrupts from timers and external signals), it becomes problematic.

But you want your manipulation of the port register to be atomic, don't you?

DBMcDonald:
Code within an ISR routine that stops other interrupts from occurring, especially in a multiple interrupt driven construct, could cause unpredictable behaviour.

Nonsense.

Inside the ISR IRQs are normally disabled and stay that way until return from ISR.

AWOL and Whandall,
Sorry, I must apologize for my ignorance. After reading more I now know that the when an interrupt is triggered, all other interrupts are disabled.

That being said, it's still critical that as little time as possible be spent in an interrupt to prevent unpredictable behaviour as a consequence of other critical interrupts not being serviced and the implementation of a simple boolean flag accomplishes this.

DBMcDonald:
..and the implementation of a simple boolean flag accomplishes this.

The PORTD didn't work so this is what i'm continuing with. Thanks all!

digitalWrite() or direct port writes in an ISR is fine. The reason it isn't working is a bug elsewhere in the code which you have not posted (you should always post a complete sketch that demonstrates the problem - the fact that you are asking for help demonstrates that you are not qualified to say where the problem is - if you were, you would be able to solve it without asking for help, as 9 times out of 10, finding the problem is harder than fixing it). My suspicion is that something that should be declared volatile isn't.

digitalWrite() is slow, but it's not so slow that using it within an ISR is problematic (though of course port writes are preferable).
There is no reason not to twiddle a pin (especially using direct port writes) within an ISR; setting a flag in the ISR instead of doing the work there is great practice, but it's not significantly faster than a direct port write; there is no need to set a flag that you later check for when all you want to do is write a pin.

digitalWrite() uses cli() to disable interrupts, but does not use sei() to turn them back on (this could be problematic if you called digitalWrite from an ISR or when interrupts were disabled for some other reason, as it would reenable interrupts when the rest of the code wasn't expecting it) - it saves SREG before doing cli() and then restores it after the relevant register writes - thus re-enabling interrupts if and only if they were previously enabled. This is the correct way to disable/enable interrupts in a function to make an operation atomic if the function may be called from within an interrupt or other situation where interrupts are already disabled.

I'd also be curious to see the code that gave rise to the error.
I'd be looking for things initially like correct usage of volatiles, wrapping manipulation of multi byte data types outside the ISR (but set in the ISR) with cli/sei; divide by 0; signed number overflows etc. etc.

DrAzzy:
digitalWrite() or direct port writes in an ISR is fine. The reason it isn't working is a bug elsewhere in the code which you have not posted (you should always post a complete sketch that demonstrates the problem - the fact that you are asking for help demonstrates that you are not qualified to say where the problem is - if you were, you would be able to solve it without asking for help, as 9 times out of 10, finding the problem is harder than fixing it). My suspicion is that something that should be declared volatile isn't.

digitalWrite() is slow, but it's not so slow that using it within an ISR is problematic (though of course port writes are preferable).
There is no reason not to twiddle a pin (especially using direct port writes) within an ISR; setting a flag in the ISR instead of doing the work there is great practice, but it's not significantly faster than a direct port write; there is no need to set a flag that you later check for when all you want to do is write a pin.

digitalWrite() uses cli() to disable interrupts, but does not use sei() to turn them back on (this could be problematic if you called digitalWrite from an ISR or when interrupts were disabled for some other reason, as it would re enable interrupts when the rest of the code wasn't expecting it) - it saves SREG before doing cli() and then restores it after the relevant register writes - thus re-enabling interrupts if and only if they were previously enabled. This is the correct way to disable/enable interrupts in a function to make an operation atomic if the function may be called from within an interrupt or other situation where interrupts are already disabled.

Thanks for your reply. But I can assure you all variables accessed in the ISR and the function called by it were marked volatile. All time calc variables were unsigned long.

The direct port write didn't work either. In fact, it didn't fire any at all from the ISR! (now this needs some explanation)

-DigitalWrite worked but buggy,
-Direct port didn't work AT ALL (didn't fire), but fired once included in the function that is called from the main loop.
-The flag worked smoothly

I cannot post the entire sketch because it's part of something bigger.

Ps., it is also possible that whatever I was doing in the other areas of my code be utilizing too much of the microprocessor

PPS, here is another user that observed an irregular delay in digitalwrite
https://forum.arduino.cc/index.php?topic=16804.0

Another topic but maybe related: I've also noticed similar random lag with a simple PWM sketch that changes the frequency based on the read analogPin level. Randomly while increasing the frequency you will get a cutout. I had to write my own PWM function by using _delay_us(1) and putting it in a for loop that delays a total of the frequency pulsewidth. If I find both codes I start another thread.

I cannot post the entire sketch because it's part of something bigger.

So post a cut down version that shows the problem.

Arm waving explanations don't cut it when talking about software, it is all in the actual code. I know your instinct is to say the machine dosen't work, we see people say that every day, they are very very seldomly correct.

Exactly and if you don't find the root cause of the problem, this "bigger" system will risk being flaky as a result of your contribution to it.