Improve EIC_Hander to control when the interrupt is cleared

[coffeemachine]

I find the way EIC_Handler clears interrupt impractical. I have a use case (see below) where I want to clear the interrupt before executing the callback. Therefore I modified Winterrupts.c to allow manually clearing the interrupt earlier from the ISR.

My use case: I want to make sure that the value I read from the pin is eventually correct (after the pin voltage has become stable). If the interrupt is not enabled when I do the digitalRead, I may miss a state change.

I don't know if this integrates well with the general idea of this library (and other boards') and I'd appreciate to have feedback on that.

[AchimErk]

Hi, I experienced too the issue, of lost interrupts. I don't think, that allowing to clear interrupt by the additional function helps here.

Lets assume the follwoing senarios: A) one change, one interrupt

• Change of Digital In from 0 to 1
• ISR is triggered
• in ISR will read 1
• Interrupt is cleared by WInterrupts routine => everything works as expected B) two quick changes of input line
• Change of Digital in from 1 to 0
• ISR is triggered
• Change of Digital in from 0 to 1
• in ISR will read 1
• Interrupt is cleared by WInterrupts routine => only one interrupt, but value read is value at digital pin as pin changes before ISR was executed C) quick changes, but other timing
• Change of Digital in from 1 to 0
• ISR is triggered
• in ISR will read 1
• Change of Digital in from 0 to 1
• Interrupt is cleared by WInterrupts routine => only one interrupt, value read is NOT value at digital pin, but no next interrupt to handle second change

Even when having addional possibility to clear, this will not change. The only solution I see is, that in WInterrupts the interrupt need to be cleared before the attached ISR routine is called. (See my Issue 537).

When interrupt is cleared before calling ISR routing then everything would work fine: D) like C from timing

• Change of Digital in from 1 to 0
• ISR is triggered
• Interrupt will be cleared before call routing
• in ISR will read 1
• Change of Digital in from 0 to 1 (will trigger next Interrupt)
• return from ISR
• next ISR is triggered
• Change of Digital in from 1 to 0
• ISR is triggered
• Interrupt is cleared by WInterrupts routine
• in ISR will read 0 => two interrupts will be processed, after second value read is value of pin.

I can not see any disadvantage of this change.

best regards Achim

[coffeemachine]

@AchimErk if I understand everything correctly, your suggestion (also listed in #537), is equivalent to mine, when calling clearInterrupt() at the beginning of the ISR. The advantage of this solution is that it is backward compatible: if clearInterrupt() is not called in the ISR, the behavior remains unchanged and will not break existing code that relies on it.

I'd like to point out that "not losing" any interrupts is not possible. You may always have more than the chip can process. Be it your solution or mine, there's always a delay before the flag is cleared, which can be long if an ISR already being executed.

The solution I'm proposing here doesn't aim at not losing interrupts, rather it allows the developer to manually control when the interrupt flag is cleared. In my use case this allows to always read the last value of the pin or, if it changed inbetween, to trigger another ISR call.

[matthijskooijman]

Good analysis, it seems that clearing the flag first is good for the reasons already given: 1) makes sure that the value read in the ISR callback will always (eventually) be the last state, and 2) clearing it earlier reduces the window for missing the interrupts.

I wonder if the added complexity of allowing more control like tihs PR implements is really needed? Maybe just clearing the interrupt first be default would be better? This means that sketches do not have to do anything specific to get the "better" behaviour and might lead to less surprise. Another argument for this is that on the AVR Arduino core, which is still somewhat the reference platform, the interrupt is also cleared (by hardware) before running the ISR, so if the SAMD core would do the same, you could write portable code without needing to rely on a SAMD-specific clearInterrupt() function.

[coffeemachine]

@matthijskooijman indeed it would be simpler. However that would be a breaking API change in my view. Code that relies on the interrupt not being cleared before the ISR has ended would no longer work. I'm not sure what's Arduino's policy regarding breaking changes. If it's not an issue, I'd also prefer the simpler and more portable version.

[matthijskooijman]

It is a bit of a breaking change, but given the difference between AVR and SAMD now, one could also think of it as a bugfix. I also wonder how much code really cares about this at all, though.

In any case, to me it seems acceptable to just change this, but I guess the devs should comment on this (@aentinger and @facchinm seem to have worked on this repo most lately?).

[AchimErk]

@coffeemachine, Thanks for your comments. But I'm not sure, if you got me right. My issue is not "missing interrupts" at all, how the headline may have suggested. The goal is, that I would like to be informed MINIMUM once, after each change of the input pin. The value read should match value of the pin, when ISR has finished. Thats not true for scenario C above, and i faced this issue in reality (getting wong pinValue for a push button). Doing readPin and later "clear interrupt" by handler opens a time window. All changes to the pin happed in this window will not be reflected by my read (as happening after the read), but interrupt to inform is cleared immediately by handler without calling again the ISR. So value from readPin becomes wrong. Having the possibiltiy to call "ClearInterrupt" before read, does not change this, so does not solve my issue. I would need a function like "doNotClearInterruptLater". Of couse, when not calling clearInterrupt late, then it needs to be called early, so need some functionality for that too. Changing clear interrupt to be executed before calling ISR routine does close the time window as well, where pinChanges are lost (not causing another ISR call).

Yes, this is changeing behaviour. But I'm not sure, if this would call this a break of code. Break of code mean for me, some code works before, but not after the fix. I tried, but I could not find an example for breaking code. Can you provide an example, where code would not work any more, when clearInterrupt is executed early, but did work before?

All scenarios coming into my head do not behave wrong after change, and/or are still possible:

• less IRQ might happen before clear interrupt. Having little more or less should not make a difference, as no guaranteed timing is given. Maybe ISR is delayed by another ISR happing immediate before, so there might be many of these anyway.
• No change for : no interrupt of the ISR routing, when trigger is happening after clearing (as ISR will not interrupt ISR). It will call ISR second time after ISR has finished. (Which may happen as well when change of pin is happening immediately after clear ISR status.)
• in ISR I do not see any different behaviour, other than query ISR-Status register for this ISR. This is now cleared and would not show this IRQ is active. But by call this ISR you know implicit, that this Interrupt-Status was set. You do not need to query this register at all.

Fixing the issue by provide some new functions adds a lot of complexity for the user of these functions. So before going in this direction, we should be confident that there is a benefit provided by these functions (compared to clear Interrupt early). Here too some examples of breaking code may be helpful.

Until there is no example of breaking code, I would agree to matthijskooijman to see "to clear interrupt after ISR routine instead of clearing it before" as a bug.

best regards Achim

[coffeemachine]

@AchimErk I think I got you perfectly and this is also the reason why I created this PR.

Having the possibiltiy to call "ClearInterrupt" before read, does not change this, so does not solve my issue.

Yes it does. Because if the pin changes before you read it, it doesn't matter, you get the latest value. If it changes after, it doesn't matter either because it also means it changed after clearInterrupt and thus the ISR will be triggered again. This is perfectly equivalent to your solution, as the only difference is that you clear the interrupt before the ISR user code is called.

myISR() {
  clearInterrupt();
  readPin();
}

Regarding the discussion whether or not this function should be provided to the users, see my previous comment.

[matthijskooijman]

Yes it does.

@coffeemachine This is only true because your PR does not just allow clearInterrupt(), it also prevents clearing the interrupt after the ISR if clearInterrupt() was called by the ISR. I suspect this is the bit that @AchimErk was missing?

But regardless, I would be favor of just changing the behaviour to always clear before, as I mentioned in https://github.com/arduino/ArduinoCore-samd/pull/499#issuecomment-653876062

[AchimErk]

Hi coffeemahine,

you are right.

I did look at your comment, but I did not look into the code in detail. As it prevent clear interrupt after the ISR, it will solve the issue.

So sorry for this.

best regards Achim

[coffeemachine]

Hi coffeemahine,

you are right.

I did look at your comment, but I did not look into the code in detail. As it prevent clear interrupt after the ISR, it will solve the issue.

So sorry for this.

best regards Achim

No worries, I also didn't realise where the confusion was coming from ;)

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

coffeemachine seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}