FunctionalInterrupt/ScheduledFunctions should be in a library, not in core_esp8266_wiring_digital

[dok-net]

After mulling over the situation with the pending PR #6003 , I have rewritten the support for std::functional and scheduled interrupts to be less intrusive.
First, the support is revoked. Second, the refactored code is brought back as an example in libraries/esp8266/examples/GPIO/FunctionalInterrupt.
I have unified the code with the ESP32 version: espressif/arduino-esp32#2734

[dok-net]

@hreintke I should not have missed anything, so it's working as before, and bringing back the functional interrupts in one way or the other is basically a matter of changing the locations of the files, and altering #include's "" back to <>. What do you think? What about your review of my exposing attachInterruptArg(), that PR is still pending, causing me some daily grievance in my library work :-)

[hreintke]

@dok-net @devyte
I will review but, unless I missed something in my first quick review, this is not backward compatible with the current implementation.

Is is correct that in your implementation all support for functional and scheduled scheduled interrupt is removed from core and a "user" needs to create it's own ? (Eventually using the example code)

[dok-net]

Let me first put into perspective what I believe is the state in the present master branch:

• attachInterruptArg() is not exposed in the API, for good measure, because:
• __attachInterruptArg must not be used directly, as that would expose the bug where cleanupFunctional(void* arg), linked as external against the next best definition in the depths of the build tree, without checks, casts void* interrupt_handler_t::arg to ArgStructure* and deletes that ostensible object and the members thereof.
  Or putting more easily, FunctionalInterrupt.h is the only safe access to attachInterruptArg.

I have some reservations regarding the MT safety of the implementation of FunctionalInterrupt.h/ScheduledFunctions.h, especially on ESP32 where there is a possibility to run without core affinity. But even for single core all-userspace interrupts without atomic access, I am not so sure.

As regards your question concerning backward compatibility of this PR, only in the context of PR #6003 which fixes the interoperability between FunctionalInterrupt and direct use of attachInterruptArg(), without further work there is a need for the user to use, either one or the other, the attachInterrupt/attachInterrruptArg/detatchInterrupt functions from Arduino.h directly, or stick completely to the FunctionalInterrupt.h wrappers. This is because the interrupt handling in core now has no way of telling what type of objects are behind the arg pointer, thus in general, leaking them on detach.

Depending on the outcome of this discussion, it is my understanding, that there is nothing keeping us from creating a new library that provides FunctionalInterrupt and friends. Or if so decided, the files can be moved back into cores/esp8266 after addressing the mentioned shortcomings if any, and pointing the Arduino.h entry points in attachInterrupt et al at implementations in FunctionalInterrupt and "hiding" those in core_exp8266_wiring_digital.*. The same can be applied accordingly to ESP32.

My take on the matter is that right now I am perfectly happy with explicitly managing the argument objects's lifetimes in user code. Probably we could also use std::function objects in interrupt_handler_t for a cleaner solution than what I perceive is in core right now.

Let me ask a final question, I don't quite seem to understand how ScheduledFunctions works right now without the user code always having to call ScheduledFunctions::runScheduledFunctions() explicitly in loop()?

[d-a-v]

Let me ask a final question, I don't quite seem to understand how ScheduledFunctions works right now without the user code always having to call ScheduledFunctions::runScheduledFunctions() explicitly in loop()?

It is run there (meaning: at very arduino main loop())
(#6039 would like to change that: at every yield())

[d-a-v]

[...] there is a possibility to run without core affinity

Scheduled Functions are meant to be run outside from any callback / interrupt state.
This is to ensure "Atomic access" or any locking facility is not needed.

[hreintke]

@dok-net @d-a-v :
The files ScheduledFunctions.h and .cpp can/should be removed.
It is/was an attemp to generalize the current schedule_function(...) and by accident included in the ScheduledInterrupt patch.

[dok-net]

Any use of std::bind or std::functional objects is not ICACHE_RAM interrupt service routine policy compliant. The whole debate over FunctionalInterrupts, except the scheduled ones, is moot for this reason. Even if the check implemented in attachInterrupt doesn't catch the error, does not mean it's not inherent.
So, until lambdas, std::bind, std::function can handle the ICACHE_RAM attribute, the only "legal" use of the FunctionalInterrupt.h/cpp is for scheduled handler, which are only dropped into the scheduler, but get called outside of the ISR context. This is fine.
I vote for merging this PR instead of #6003, but certainly not doing anything about the situation seems completely wrong to me.

[dok-net]

Any use of std::bind, std::function or lambdas in the code-path of ISRs in fact is a case of ISR not in IRAM, just like this, only wrapped in some other functions.

[dok-net]

This attachScheduledInterrupt() is a valid use of std::bind, std::function or lambdas, since the actual call is made in the context of the task that runs the scheduler, not in a interrupt service routine.