simavr icon indicating copy to clipboard operation
simavr copied to clipboard

Premature interrupt cause flag clearance

Open djfd opened this issue 5 years ago • 1 comments

Hi,

Please correct me if I am wrong, but it seems there is an issue of too early resetting the bit of interrupt source. This affects at least pin-change and external interrupts.

The code of avr_service_interrupts:

...
                if (vector->trace)
			printf("IRQ%d calling\n", vector->vector);
		_avr_push_addr(avr, avr->pc);
		avr_sreg_set(avr, S_I, 0);
		avr->pc = vector->vector * avr->vector_size;

		avr_raise_irq(vector->irq + AVR_INT_IRQ_RUNNING, 1);
		avr_raise_irq(table->irq + AVR_INT_IRQ_RUNNING, vector->vector);
		if (table->running_ptr == ARRAY_SIZE(table->running)) {
			AVR_LOG(avr, LOG_ERROR, "%s run out of nested stack!", __func__);
		} else {
			table->running[table->running_ptr++] = vector;
		}
               avr_clear_interrupt(avr, vector);
...

This last statement clears the source bit:

if (vector->raised.reg && !vector->raise_sticky)
avr_regbit_clear(avr, vector->raised);

even if we did not step into ISR itself yet, just changed PC for future iterations.

However, the documentation states (eg see at-tiny-24 manual)

The flag is cleared when the interrupt routine is executed. Alter- natively, the flag can be cleared by writing a logical one to it.

That is not before, not during, but only when interrupt routine is executed (past tense, passive voice, right?)

So for me it looks more correct to perform this job piece upon RETI instruction, eg.like this:

--- a/sim_interrupts.c	2019-04-21 16:35:22.245170807 +1000
+++ b/sim_interrupts.c	2019-04-21 16:36:32.601994196 +1000
@@ -219,6 +219,7 @@
 	avr_int_table_p table = &avr->interrupts;
 	if (table->running_ptr) {
 		avr_int_vector_t * vector = table->running[--table->running_ptr];
+		avr_clear_interrupt(avr, vector);
 		avr_raise_irq(vector->irq + AVR_INT_IRQ_RUNNING, 0);
 	}
 	avr_raise_irq(table->irq + AVR_INT_IRQ_RUNNING,
@@ -288,7 +289,6 @@
 		} else {
 			table->running[table->running_ptr++] = vector;
 		}
-		avr_clear_interrupt(avr, vector);
 	}
 }
 

Am I missing something?

Thanks

djfd avatar Apr 21 '19 06:04 djfd

avr_interrupt_reti is called after the interrupt completes by calling reti.

the interrupt is set to start just a few lines before... looking at the code I may see a possibility of what you are saying. do you have a test to show this?

bsekisser avatar Oct 11 '20 02:10 bsekisser