Expose attachInterruptArg in Arduino.h and updated base for functional interrupts

[dok-net]

A C++ std library functional/bind/lambdas based quite clean implementation [of the same functionality as in #6055 (closed) ]

Edit: Since May 4 2019, original PR date, a lot has changed, given the time for testing, input from project members, and general refactoring. So here's what this is today:

The interrupt handling code gets updated to use C++ std::function. C-style bindings are preserved as wrappers. This solves issues with leaking of attached arguments on detachInterrupt or re-attaching; is type-safe; makes std::function a first-class citizen for interrupt service routines; has been tested for code paths all being in IRAM, if std::bind is used, but not lambdas, which currently exhibit a compiler weakness in this area.

[dok-net]

@d-a-v : could #6039 please take into account all the work and research I've done on fixing the interrupt handling? Please remember, that at heart, #6038 is the cleanest ISR vs. ICACHE_RAM safe implementation at this present stage I have to offer, the caveat being that the user cannot mix functional and non-functional attaches and detaches without care. #6003 provides this extra ease, at the expense of copied struct definition, dependency inversion between FunctionalInterrupt and core_esp8266_wiring_digital and the extra functional field only for the purpose of supporting FunctionalInterrupt. My take is still to go for clean dependencies over ease of use :-)

[d-a-v]

@dok-net Thanks for your work,
Lots of nice improvement-PRs including yours are waiting for the bugfix-one-month-late core-2.5.1 release so they can be properly tested / reviewed.
2.5.1 is imminent now (well, after some neverending recent small but mandatory fixing merged commits).

[d-a-v]

@dok-net #6039 is orthogonal to your PRs #6003 #6038 #6047.
What do you mean with " take into account all the work and research" ? Am I stepping on your toes ?
(In the files you modified I fixed a static declared in a .h, not a real issue. On the other hand I added an IRAM_ATTR define which might please your esp8266/esp32 lib :)

[dok-net]

Dear @d-a-v , no, stepping on my toes is much to hard a word, I am just a little panicky that #6039 might cause a merge mess for me, I only wanted to point that out. As far a bug fix vs. feature is concerned, I've created issue report #6049 to hopefully motivate sufficiently why this PR is a bug fix and should be in the very next bugfix release.
SoftwareSerial is also pending release reliant on the availability of attachInterruptArg - of course I could back-port, but I'd rather have it all matching up nicely as it does in my own repos' configurations right now. Naturally, I can only try to show that this PR is a relatively minor fix to a real problem without expected side-effects...

[d-a-v]

@dok-net
There are #6048+#6055 and this one #6047.
Does OP states #6047 replace both ?

Also I understand #5922 should allow functional in IRAM and there are issues about that per @hreintke 's comment #6055 (comment) and it needs to be sorted out.

[dok-net]

@d-a-v

There are #6048+#6055 and this one #6047.
Does OP states #6047 replace both ?

Yes, it should work this way (in git).

Also I understand #5922 should allow functional in IRAM and there are issues about that per @hreintke 's comment #6055 (comment) and it needs to be sorted out.

I couldn't quite wrap my head around the irom.text etc. meddling, and unfortunately the above mentioned comment features incomplete code and I can't figure it quite out either - I would like to trust that the pertaining parts of std::function<void()> are either inlined or linker-edited into IRAM as per #5922, and that is sufficient for #6047, unless someone attaches/detaches ISRs from inside executing ISRs, on which opinions vary widely :-)

[hreintke]

@dok-net
I just updated the comment in #6055 to include a complete sketch. If anything still unclear, just let me know.

[dok-net]

@d-a-v @hreintke To keep you informed, with this std::function based ISR attach code, I see sporadic crashes after reset. So yes, there is still something not in order with the IRQ-safety of std::function and friends.

[hreintke]

@dok-net @d-a-v :
I can do further checking on this but am lost in the PR's and associated commits.
Do I need only git master + this PR or is it also dependent on others like the Ticker etc ?

[dok-net]

@hreintke I almost can't resist giving an answer that's just as cryptic :-)
No, don't worry, I am glad you care! I am putting a lot of work into keeping the PRs clear to merge with master each by themselves. If this doesn't work now and then, yes, me too, I am dissatisfied and disbelieving how long git and the GitHub PRs exist without proper dependency management for the inter-PR dependencies. Stuffing all into a single branch and thus one PR can't quite be the solution, either.
Anyway, Ticker has nothing to do with #6049, hence this PR stands alone. For differential diagnosis, you can try #6055 (this includes #6048), which are what I use while this #6047 unfortunately doesn't work.

[hreintke]

@dok-net @d-a-v @igrr
Is there a way that, for debug purposes, we can intercept the "missed cache interrupt" and then check whether we are in interrupt code. If no -> continue as usual, if yes -> panic() and output somehow the offending function address ?

[earlephilhower]

@hreintke, There's no cache miss handler code. Cache fetches are done completely in HW. If the HW state machine can't engage the SPI bus to do the read you get a nonrecoverable HW exception at that point and crash hard. If it can access the bus, it will work since interrupts are just normal codepaths and nothing fancy on these machines.

As a debug only method, you might be able to do something fishy to the SPI bus at the start of our interrupt dispatch code and then undo it on return. Then panics will occur should ISR code attempt to go out to flash. But that's debug only and it will break SPI, WiFi persistence, and anything else that access flash as a data store.

I think it's probably overkill, though. Even the ISR-in-RAM check is a bit pedantic since it is possible for an app to never access flash as data store, or an app could disable IRQs before doing so (and killing any IRQ generated stuff like PWM, etc.).

[dok-net]

@d-a-v It seems you've some time at your hands today to talk - I've a question: What is the state of the IRAM linker segment manipulations w.r.t. std::function now? I have hacked a fix for EspSoftwareSerial (master HEAD) to use that in the meantime until my bigger PRs get reviewed and merged, and somehow it's working using the FunctionalInterrupts API instead of the C-style attachInterruptArg (in my PRs). Before, it seemed unstable. Also, a standalone test in libraries/FunctionalInterrupt/ that I had seen failing, as I believed due to flash accessing during interrupt service - runs fine. As much as I understood it, std::function<void(void)> are OK, does this depend on the captures in any way?

[d-a-v]

As much as I understood it, std::function<void(void)> are OK, does this depend on the captures in any way?

Since #5922, all functional effective code is moved into IRAM.
Matching signature is supposed to be: std::function<any(...)>::operator()() const.
In this comment I didn't check the capture influence to the generated object name.
You can play with https://demangler.com/ and check if your symbol matches _ZNKSt8functionIF*EE*

You can also more easily check from a running sketch whether a lambda is linked IRAM with this trick.

[dok-net]

@d-a-v I believe, with the information you provided me, that this PR now contains a safe refactoring of the IRQ code that has the relevant code paths in IRAM. It certainly is a lot cleaner than #6055 .

[dok-net]

Update:
I've removed, for the time being, Delegate in favor of plain std::function.
Here are the build results, for a FunctionalInterrupt.h, attachScheduledInterrupt example:
Master:

ICACHE *: 32768           - flash instruction cache
IROM   *: 237528          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 27653   \ 32768 - code in IRAM          (IRAM_ATTR, ISRs...)
DATA   *: 1496  )         - initialized variables (global, static) in RAM\HEAP
RODATA *: 1000  ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 25888 )         - zeroed variables      (global, static) in RAM\HEAP

This PR:

ICACHE *: 32768           - flash instruction cache
IROM   *: 237720          - code in flash         (default or ICACHE_FLASH_ATTR)
IRAM   *: 27613   \ 32768 - code in IRAM          (IRAM_ATTR, ISRs...)
DATA   *: 1496  )         - initialized variables (global, static) in RAM\HEAP
RODATA *: 992   ) \ 81920 - constants             (global, static) in RAM\HEAP
BSS    *: 25944 )         - zeroed variables      (global, static) in RAM\HEAP

[dok-net]

Further info, performance of back-to-back IRQs, that is, connecting two pins, and let every change on pin A trigger an interrupt on pin B, where the ISR toggles the state of pin A.

Master @ 80MHz: IRQs/s = 136239
This PR @ 80MHz: IRQs/s = 141242

Sketch:

#include <atomic>

constexpr long unsigned MAXIRQS = 200000;

std::atomic<long unsigned> irqCount;
std::atomic<uint32_t> lastTime;
uint32_t prevTime;

IRAM_ATTR void triggered(void*)
{
	if (irqCount.load())
	{
		irqCount.store(irqCount.load() - 1);
		digitalWrite(D4, !digitalRead(D4));
	}
	else
	{
		lastTime.store(ESP.getCycleCount());
	}
}

void setup()
{
	Serial.begin(115200);
	while (!Serial);
	delay(100);
	pinMode(D4, OUTPUT);
	digitalWrite(D4, LOW);
	pinMode(D7, INPUT);
	attachInterruptArg(D7, triggered, nullptr, CHANGE);

	Serial.println("measuring IRQ latency");
	irqCount.store(MAXIRQS);
	prevTime = ESP.getCycleCount();
	digitalWrite(D4, !digitalRead(D4));
}

void loop()
{
	delay(1000);
	if (!irqCount.load())
	{
		Serial.print("IRQs/s = ");
		Serial.println(1000 * MAXIRQS / ((lastTime.load() - prevTime) / ESP.getCpuFreqMHz() / 1000));
		irqCount.store(MAXIRQS);
		prevTime = ESP.getCycleCount();
		digitalWrite(D4, !digitalRead(D4));
	}
}

[dok-net]

For further motivation, here are the results when substituting Delegate back in for std::function:
dok-net:all_in_functional_w_delegate@80MHz: IRQs/s = 144092

[mcspr]

For further motivation, here are the results when substituting Delegate back in for std::function:
dok-net:all_in_functional_w_delegate@80MHz: IRQs/s = 144092

Notable improvement is removal of if(functional), arg casts, which is mighty good thing for the Core's ISR handler speed.

With the PR though, doesn't std function's function still need to be explicitly marked as IRAM ? What about lambdas?
e.g. attachInterrupt(PIN, [some, captured, args]() { ... }, CHANGE); in the setup(), which then looks like std::_Function_handler<void (), setup::{lambda()#1}>::_M_invoke(std::_Any_data const&) in the objdump

I've tried some alternative approach at master...mcspr:isr-why
Looking at objdump (fairly weird) output, std::function:: _M_invoke is part of the std function operator() call tree (and as described here), so the branch experiments by moving it to IRAM via ldscript rule (but I'm pretty sure it needs to be something else for the sake of schedule_function / anything else using void() std funcs)

[dok-net]

All the compiler limitations with regard to attributes still apply.