Commentary on while loops in interrupts

John...

It depends on the product function and what kind of failure mode is wanted. If the hardware does a melt-down; you need to decide how much can reasonably be hoped for from the software...

RTOS

This kind of loop is perfectly reasonable in an ISR.

You loop on the interrupt status register, to be sure that you have serviced ALL pending interrupts from the device before you exit the ISR. If you have not serviced all pending interrupts, you will be interrupted again as soon as you re-enable interrupts on the way out.

In the event of a hardware failure that hangs an IRQ on your door that won't go away, this code will loop in the ISR. Code that didn't loop in the ISR would loop around the interrupt request/interrupt acknowledge/ISR. The net effect is the same: no other code runs.

FURTHER, this approach is actually preferable to non-looping code if it is even remotely possible that the device can stack multiple interrupt requests. The cost and latency of servicing another interrupt is MUCH higher than the cost of looping in the ISR.

serviced

have

won't

net

Thanks for the input.

RTOS

An iterator (that is: counting how many times you go through the while loop) may be a good idea. Compare the count to some *extremely* large number that should never happen with correctly functioning hardware.

If you reach that impossible count then : cleanup, report the error and mask off the interrupt so you never see it again. You must do all of these things or not bother with any of them. This leaves you with a crippled product but at least you continue to operate as best you can. Staying out of the "hang" state makes it possible to report the failure - otherwise neither you nor the cutomer gets a clue as to what went wrong.

The time to write this code is in development. I probably would *not* insert this into a "finished" product unless I had grounds for suspicion that it was actually needed. Is the product hanging mysteriously with this interrupt asserted?

Bob

You could always try something like:

#define COOLANT_LOW_PRESSURE_INTERRUPT_MASK 1 #define EXCESSIVE_TEMPERATURE_INTERRUPT_MASK 2

void ISR(void) { volatile int status;

while (status = READ_HARDWARE()) { /* handle & clear the expected types of interrupt */ if (status & COOLANT_LOW_PRESSURE_INTERRUPT_MASK) HANDLE_COOLANT_LOW_PRESSURE(); if (status & EXCESSIVE_TEMPERATURE_INTERRUPT_MASK) HANDLE_EXCESSIVE_TEMPERATURE();

/* handle any unexpected types of interrupt */ if (status & UNEXPECTED_INTERRUPTS_MASK) BOAKYAG(); } }

Tanya :-)

As noted by others the algorithm is a reasonable one as long as it fits the system design. But you should ditch the volatile specifier for 'status'. It has no purpose and it may be costing you valuable CPU cycles in the interrupt service routine.

RTOS

Why do you day that the volatile is unnecessary? Surely a good optimizing compiler will reuse values unless it realises that they can change between successive use, and thus remove the read inside the While loop?

volatile is a hint to the compiler that the value changes through "other" means and that such an optimisation should not be performed (i.e. every use should access memory rather than rely on the cached value in a register).

optimizing

between

Exactly: reading hardware is such a case, so again, why NOT use volatile?

Adding the volatile adds extra memory accesses. It's used where a location changes through means that the compiler can not detect: typically a hardware register or a location that other threads use and change. If nothing else uses a variable [concurrently], don't tell the compiler (through volatile) that it does.

Because the variable "status" itself is not reading any hardware. It's a normal C variable in completely control of the compiler.

volatile?

Yes, got it. Thanks. Me being extra dumb :-( The volatile part is (in my original example) READ_HARDWARE (or whatever I called it), if that was a 'variable' located at hardware. (Lousy name for a variable though :-)

Thanks again R