1

I am trying to implement a function queue scheduling system. INT1 is connected to a button and int1task causes an LED to flash.

typedef void (*funcptr)(void);
TPrioQueue *queue = NULL;

void int1ISR() {
  if (debounce(&int1time)) // Checks if the interrupt is caused by switch bouncing
    enq(queue, (void*) int1task, 1);
}

Next, I execute the function in loop() by:

typedef void (*funcptr)(void);

void loop(){
  funcptr func = (funcptr) deq(queue);
  if (func != NULL)
    (*func)(); // Stuck in a loop
}

For some reason, the LED keeps flashing even though I have not pressed the button. Why does this happen?

When I remove the line below, the ISR works properly.

enq(queue, (void*) int1task, 1)

The relevant sections of the code:

int debounce(unsigned long *debTimer) {
  unsigned long tmp = *debTimer;
  unsigned long currTime = millis();

  if ((currTime-tmp) > 500){
    *debTimer = currTime;
    return 1;
  }
  else
    return 0;
}

// Flashes LED at pin 6 five times at 2 Hz
void int1task(){
  for(int i=0; i<5; i++){
    digitalWrite(6, HIGH);
    delay(250);
    digitalWrite(6, LOW);
    delay(250);
  }
}

void setup() {
  pinMode(6, OUTPUT);
  attachInterrupt(1, int1ISR, RISING);
  Serial.begin(115200);
}

Code I used for the queue

Here is the schematic I am using:

Enter image description here

  • 1
    If debounce() calls millis(), then it will not work, as millis() do not change within an ISR. Can yoi show the code of debounce()? – jfpoilpret Feb 16 '15 at 5:32
  • 1
    I don't see any code that toggles the led. I don't see any code initializing the interrupts. – Gerben Feb 16 '15 at 19:38
  • 1
    I suspect a concurrent access problem (by loop() and the ISR). I think you should post the code of enq, deq and also the TPrioQueue or a link to the code if it is a 3rd-party library. – jfpoilpret Feb 21 '15 at 14:15
  • 1
    The queue has to be initialized with makeQueue(). – Edgar Bonet Feb 28 '15 at 15:00
  • 1
    Your snippets are still lacking a few important lines of code: we would like to see the declaration of all variables: queue, int1time. Also, did you check what @EdgarBonet mentioned in a previous comment, ie initialize queue with makeQueue()? That seems a real important point here! – jfpoilpret Mar 4 '15 at 8:57
2

After checking the queue functions that you use in your sketch, I see there is no protection against concurrent accesses in the enq and deq functions, which is a must-have if you use any of these functions from within an ISR.

I would suggest the simplest changes to these functions as follows:

void enq(TPrioQueue *queue, void *item, int priority)
{
    TPrioQEntry *newEntry=(TPrioQEntry *) malloc(sizeof(TPrioQEntry));

    if(newEntry != NULL)
    {
        newEntry->item=item;
        newEntry->prio=priority;
        newEntry->prev=NULL;
        newEntry->next=NULL;

        // Protect against concurrent accesses starting here
        uint8_t sreg = SREG;
        cli(); 
        if(queue->head==NULL)
            queue->head=newEntry;
        else
        {
            // Find insertion point.

            // First special case: Item at head has lower priority
            if(queue->head->prio > newEntry->prio)
            {
                // Insert item at the start of the queue.
                newEntry->next=queue->head;
                queue->head->prev=newEntry;
                queue->head=newEntry;
            }
            else
            {
                // Find your insertion point
                TPrioQEntry *trav= queue->head;

                while(trav->next!=NULL && trav->prio < newEntry->prio)
                    trav=trav->next;

                newEntry->prev=trav;
                newEntry->next=trav->next;
                if(trav->next != NULL)
                    trav->next->prev=newEntry;

                trav->next=newEntry;
            }
        }

        queue->itemCount++;
        // Protect against concurrent accesses stops here
        SREG = sreg;
    }   
}

void *deq(TPrioQueue *queue)
{
    // Protect against concurrent accesses starting here
    uint8_t sreg = SREG;
    cli(); 
    // Return NULL if queue is empty
    if(queue->head == NULL)
    {
        // Protect against concurrent accesses stops here
        SREG = sreg;
        return NULL;
    }

    void *ret=queue->head->item;
    TPrioQEntry *tmp=queue->head;

    if(queue->head->next!=NULL)
        queue->head->next->prev=NULL;

    queue->head=queue->head->next;
    free(tmp);

    queue->itemCount--;

    // Protect against concurrent accesses stops here
    SREG = sreg;

    return ret;
}
  • The above code did not work, the LED still kept flashing – sharon Feb 25 '15 at 10:06
  • If this still doesnot work, I suspect you may have some bad wiring or interferences somewhere in your circuit. Could you show a clear picture of your circuit? A schematoc would also be nice to have. – jfpoilpret Feb 25 '15 at 11:34
  • 1
    The simplest change would be to #include <util/atomic.h> and call deq() from an ATOMIC_BLOCK. – Edgar Bonet Feb 28 '15 at 14:55
  • @jfpoilpret [schematic] (dropbox.com/s/51f8174815zycrg/schematic.pdf?dl=0) – sharon Mar 4 '15 at 7:47
  • Isn't declaring some of the variables volatile required for this to work reliably? – Peter Mortensen Apr 7 '15 at 21:21
1

Although you did not clearly mention nor show your wiring, it is possible the problem comes from here.

Barring real concurrency issues with your queue implementation, which I already mentioned in another answer, for which fixes have to be implemented anyway, there are two potential wiring problems in your circuit:

  1. You use INT1 interrupt; that means you must connect your button to PIN 3 for a UNO, any other pin won't work.
  2. You did not specify pinMode(3, INPUT_PULLUP) in your setup(), which means you MUST use an external pullup resistor to force a HIGH value, and connect the other pin of the button to GND (or conversely: put a pulldown resistor and wire the other pin to 5V); otherwise, while the button is left unpressed, you will have a floating input, which means its value will vary randomly between HIGH and LOW, triggering your int1ISR many times when you don't expect it.

The simplest way to solve point 2 (floating input) is to use internal pullup resistors:

void setup() {  
  pinMode(3, INPUT_PULLUP);
  pinMode(6, OUTPUT);
  attachInterrupt(1, int1ISR, RISING);   
  Serial.begin(115200);
}

and connect one button pin to Arduino PIN 3, and the other button pin to GND.

That will remove floating input and its consequences.

0

ISRs have flags, typically an enable flag and a pending flag.

The enable flag needs to be set true for the interrupt to occur at all.

The pending flag is set when the interrupt event occurs.

Within the interrupt function, the pending flag must be cleared,

otherwise the the interrupt function will immediately be executed again (and again and again and ...)

  • 1
    AVR resets the interrupt flag once the ISR returns. – Ignacio Vazquez-Abrams Feb 16 '15 at 3:40
  • I am relatively new to arduino. Could you elaborate more about the flags? Are they like cli() and sei()? – sharon Feb 16 '15 at 5:25
  • My mistake, INTFn is cleared when the ISR is executed. – Ignacio Vazquez-Abrams Feb 16 '15 at 6:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.