Bugfix: attach interrupt (#6049)

[dok-net]

Fixes: #6049

• BUG FIX: __attachInterruptArg is known in the field, but it's use fails due to incorrect delete of arg pointer via cleanupFunctional. This PR guards against this case.
• Original author commented that ScheduledFunctions.(h|cpp) were added by mistake, and should be removed.
• ICACHE_RAM_ATTR was previously applied to attachInterrupt() / detachInterrupt() functions, but reviewer of Fixes and implementation to expose attachInterruptArg in Arduino.h #6003 commented that this can be reverted to save some precious IRAM, as attaching / detachInterrupt from inside ISR are corner cases.
• Properly expose attachInterruptArg() via Arduino.h

(Reference: PR #6047 implement all-C++ functional based interrupt service routine handling. Caveat: ICACHE_RAM_ATTR attributes maybe don't quite work on functional instances, like lambdas, yet)

[d-a-v]

This PR shows lots of changes because of tab/space/style updates, making It hard to read.
Approving because

• it fixes a bug (for release 2.5.1?)
• (and if) restyle core and libraries with Allman #5726/prepare allman style #5774 will come back right after 2.5.1 release so that readability issue will happen no more.

[dok-net]

@d-a-v quote: "shows lots of changes because of tab/space/style updates" - yes, except that it doesn't :-)
Wrt. #5774, if you trust that 57bdbe1a9522c5091020da85290bb54d26257d7f does exactly that and nothing but, the changes are quite easily digestible.

[d-a-v]

the changes are quite easily digestible

Indeed and I took time to digest them.
It is far more easy to check when style is not changed in a PR so the diff is clear to read.

#5774 is not meant to be used on some files but on all files at once (and will be checked by CI from that moment). In the meantime, doing that in a PR is not nice for reviewers.

[dok-net]

@d-a-v I am really aware of that and quite sorry, but please trust me that all these smart code editors I use keep changing layouts all the time and having three proposals in the game, it began to seem to make sense to me to use the styling provided for in the repo sources already. The examples even get
rejected when they are not astyled - that might be for ESP32, though, things are getting confusing :-)
I hope you will find diff-reading #6049 to #6047 or #6038 to be a more pleasant experience for it.

[dok-net]

@d-a-v This bug-fixing PR should be readable now (please use diff settings: hide whitespace changes).

[d-a-v]

@dok-net
I understand this PR is the first of your PR serie. I think it can be merged, however I question myself about your removal of ICACHE_RAM_ATTR from __detachInterrupt. I think it is legal for an interrupt handler to detach itself no ?

[dok-net]

@d-a-v If deleting
struct FunctionInfo
{
std::function<void(void)> reqFunction = nullptr;
std::function<void(InterruptInfo)> reqScheduledFunction = nullptr;
};
is guaranteed to not cause flash operations? std::function<void(InterruptInfo)> looks suspicious to me.
And I haven't even begun complaining about such long-running operations inside world-stopping ISRs :-)

[d-a-v]

You are right, cleanupFunctional needs to be in iram too, and probably something like #5922 would need to put functional destructors into iram.
@earlephilhower what do you think ?

And I haven't even begun complaining about such long-running operations inside world-stopping ISRs :-)

Well it is true in such case where a handler stopping itself needs to destroy everything.

The other way would be to use a Scheduled function to do that job but then it would have to be managed with a bool in the interrupt handler to check wether the handler is in a destroy state.

I vote for putting iram back in detachInterrupt (not all interrupts have args, let's not break them too) and maybe creating a new "defer detachInterrupt in a Scheduled function" issue to address this case.

[dok-net]

I think I reconsider my opinion on supporting detachInterrupt() from inside executing ISRs - does this not lead to object destruction on functional etc. occurring while the same are in the middle of executing? Is that well specified? From reading up on the IRAM size of the ESP8266, it would also seem to be unwise pinning all this code there out of convenience, given that the resulting inability to run applications at all will be most inconvenient ;-)

[dok-net]

@earlephilhower So here I am asking you to speed up acceptance of this PR in order for me to update EspSoftwareSerial. It's getting quite tedious switching between my esp8266 branches, especially due to the SoftwareSerial submodule that keeps changing back and forth all the time due to this dependency.
I firmly believe in adding publicly the attachInterruptArg(), plus the bugfixes to detachInterrupt() that I have implemented. The chances in this PR are all rather pedestrian.

[devyte]

@dok-net please open a gitter account and let me know here. I'll send an invite to discuss directly there.

[dok-net]

@devyte I have an account there now.

[dok-net]

@d-a-v @devyte At the speed of Schedule merges, I'm getting jealous :-)

I vote for putting iram back in detachInterrupt (not all interrupts have args, let's not break them too) [...]

I've done this now. This one PR really doesn't change all that much, it just enables me to publish the proper next EspSoftwareSerial and we could go forward with discussing more seriously my other PRs after this.

[d-a-v]

I think we can merge. @devyte was requesting a chat, I'll let him merge.

[dok-net]

@devyte I've a bug report against EspSoftwareSerial that I expect to fix within 24h, which will bring a new release of the package into focus. Is there anything standing in the way of a positive review and merge of this mostly-pure-bugfix PR? The additional exposing of attachInterruptArg() is just alignment with ESP32 and is also due, I'd like to add.

[dok-net]

@devyte I was asked by d-a-v to close the lesser of those PRs fixing the same problem. #6047 supersedes this one, but is much more complex, where this here PR is just a few lines. So please understand that I personally would not close it and hope for an iterative approach - rebasing #6047 on this PR is not challenging.