1. This forum section is a read-only archive which contains old newsgroup posts. If you wish to post a query, please do so in one of our main forum sections (here). This way you will get a faster, better response from the members on Motherboard Point.

Commentary on while loops in interrupts

Discussion in 'Embedded' started by John Smith, Jul 2, 2003.

  1. John Smith

    John Smith Guest

    Hi all

    On my current (maintenance) project, I have come across a couple of
    interrupt service routines (ISR) which have while loops inside them. An
    example follows.

    Personally I think this is rather dangerous, as if there is a hardware
    problem, the software will lock up inside the ISR and not permit other RTOS
    processes to execute. Do you agree, and what would your fix be? I think I
    would put in an iterator.

    Thanks
    R

    void ISR(void)
    {
    volatile int status;

    status = READ_HARDWARE();
    while (status)
    {
    /* handle the expected types of interrupt */
    if (status & 1)
    HANDLE_INT_TYPE_1();
    if (status & 2)
    HANDLE_INT_TYPE_2();

    /* re-read in case another type occurred simultaneously */
    status = READ_HARDWARE();

    }
    }
     
    John Smith, Jul 2, 2003
    #1
    1. Advertisements

  2. John Smith

    Morris Dovey Guest

    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...
     
    Morris Dovey, Jul 2, 2003
    #2
    1. Advertisements

  3. 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.
     
    John R. Strohm, Jul 2, 2003
    #3
  4. John Smith

    John Smith Guest

    Thanks for the input.

    R
     
    John Smith, Jul 2, 2003
    #4
  5. John Smith

    Bob Guest

    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
     
    Bob, Jul 2, 2003
    #5
  6. 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 :)
     
    Tanya Cumpston, Jul 3, 2003
    #6
  7. John Smith

    Dingo Guest

    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.
     
    Dingo, Jul 3, 2003
    #7
  8. John Smith

    John Smith Guest

    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?
     
    John Smith, Jul 4, 2003
    #8
  9. John Smith

    Paul Black Guest

    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).
     
    Paul Black, Jul 4, 2003
    #9
  10. John Smith

    John Smith Guest

    Exactly: reading hardware is such a case, so again, why NOT use volatile?
     
    John Smith, Jul 4, 2003
    #10
  11. John Smith

    Paul Black Guest

    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.
     
    Paul Black, Jul 4, 2003
    #11
  12. Because the variable "status" itself is not reading any hardware.
    It's a normal C variable in completely control of the compiler.
     
    Hans-Bernhard Broeker, Jul 4, 2003
    #12
  13. John Smith

    John Smith Guest

    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
     
    John Smith, Jul 4, 2003
    #13
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.