solve issue #199, improve issue #200 in Winterrupts #201
Conversation
line 127 fixes an issues when changing trigger mode during runtime rest is to improve speed of interrupt from 6us for an extint14 to below 2us now. The interrupt handler is now based on a first come first serve policy which serves first the ISR that was attached first and it only tries to service ISRs that were actually attached.
corrected issue arduino#199 and improve arduino#200
|
The rationale and the patches look good, I'd only make some slight adjustments.
|
reset sense bits before or-writing the new sense signature
change the way ISRs are stored. Store now in a list on a ‘first come first served basis’, the first interrupt that gets attached will be served first. This improves the speed of the ISR calling from about 6us to 2us for a single interrupt on extint14
|
I am quite inexperienced with using Github for collaborative projects so excuse if I am doing this wrong. I reverted the changes and made them in 2 separate commit steps, one solving issue #199 and the other bigger change for improving #200. I removed the debug code which also means Winterrups.h is not changed. |
More accurate approximation for multiplication factor of PLL to make 48Mhz.
| //check if we already have this interrupt | ||
| int id=-1; | ||
| for (uint32_t i=0; i<nints; i++){ | ||
| if (ISRlist[i]==in) id=in; |
There was a problem hiding this comment.
The condition here should be id = i I guess?
| //and take it out of the ISR list | ||
| int id=-1; //find its location first | ||
| for (uint32_t i=0; i<nints; i++){ | ||
| if (ISRlist[i]==in) id=in; |
| } | ||
| //and remove the top item | ||
| ISRlist[nints]=0; | ||
| ISRcallback[nints]=NULL; |
There was a problem hiding this comment.
Is erasing the list element really needed? IMHO just decreasing nints should be enough, new elements added later will overwrite the old value.
| if ((EIC->INTFLAG.reg & 1<<ISRlist[i]) != 0) | ||
| { | ||
| ISRcallback[i]();// Call the callback function if assigned | ||
| EIC->INTFLAG.reg = 1<<ISRlist[i]; // Clear the interrupt |
There was a problem hiding this comment.
I'm wondering if we can gain something if we store 1 << ISRlist[i] instead of ISRlist[i].
|
great! glad somebody is fixing this and apologies for my cumbersome interaction with the github process
regards,
Jo
|
|
Followups on #261 |
#199 corrected an error in the behaviour of attachinterrupt when changing the trigger mode of an interrupt. I now first erase the relevant trigger mode bits, before OR-writing a new setting. (this erasing was missing, causing unpredictable behaviour when calling attachinterrupt more than once for the same pine and with different trigger modes.
#200 I noticed that the EIC_Handler had to loop over all possible external interrupts and check if an interrupt occurred even if we didn't enable those interrupt. This resulted in up to 6us wait times before e.g. EXTINT14 was serviced. I rewrote the handler to keep a list of all attached interrupts (and a handle to their ISRs) with a first come first serve policy, meaning that the first attached interrupt is treated first. This improves the wait time for a single interrupt with edge triggering to slightly less than 2us. This is still long, but already a clear improvement. My guess is that the jump to an ISR with 32 bit address that is stored in RAM is now the main culprit of the remaining time, but this would probably need some inline assembly or a better grasp of how the compiler turns this code into seemingly inefficient assembly code...but hey a factor of 3 is already something. Remember that speed might be an important reason why people decided to buy the Zero (or variants) in the first place.