Place FunctionalInterrupt into IRAM

[OttoWinter]

See #5779

I have yet to test this, so using the new draft PR feature.

Also made functional attachInterrupt functions usable in ISR (consistent with non-functional attachInterrupt)
Should cleanupFunctional also be placed in IRAM? (called from __attachInterrupt)

[devyte]

@hreintke could you please take a look at this?

[hreintke]

@OttoWinter @devyte
The observation is correct, interruptFunctional should be located in IRAM.

In order to make the implementation full consistent with other attachInterrupts,

• functional attachInterrupts
• attachScheduledInterrupt
• cleannupFuctional

should also get the IRAM attribute.

[hreintke]

@OttoWinter
You should also move void attachScheduledInterrupt() to IRAM

[OttoWinter]

@hreintke It already is? https://github.com/esp8266/Arduino/pull/5780/files#diff-9f051caf4fce72b674c996d1a5b58dfbR55

[hreintke]

@OttoWinter Sorry, overlooked that line.

[OttoWinter]

It should probably be written somewhere in the docs that the lambda expression must also be placed in iram to ensure correct operation if the compiler doesn't optimize it away;

attachInterrupt(pin, []() ICACHE_RAM_ATTR {
  // do something
}, mode);

Not sure how this is for std::bind. Also I'm a bit wary if the std::function call operator must also be placed in iram. The function is called through that operator so technically it must also be in iram.

[d-a-v]

Also I'm a bit wary if the std::function call operator must also be placed in iram. The function is called through that operator so technically it must also be in iram.

#5922 is merged.

Is there a MCVE to test this PR ?

[OttoWinter]

@d-a-v Yes, see attached issue - copying it here too:

void setup() {
  // doesn't work reliably, my guess the function is pre-cached somehow and so
  // no interrupt for reading it into cache needs to be called?
  // I don't have the experience to say.
  attachInterrupt(D1, [](){}, CHANGE);
}

and get a high frequency signal on that pin, for example 50% duty cycle PWM from another ESP.

[devyte]

If this is merged, then the std::function constructor and destructor need to be moved as well. Probably assignment too.

[d-a-v]

@OttoWinter I tried your sketch.
The issue is the lambda not being in iram.

See the below sketch, using ICACHE_RAM_ATTR for ISR.
When using a lambda (directly or in a variable), code stays is flash (then exception - sometimes).
When using a plain function, code is in iram (no exception).
(I added the assembly code to check the ISR address space for sure)

ICACHE_RAM_ATTR has no effect on lambdas. Maybe I don't know how to use it ?
And it wouldn't be wise to force ld to store all lambdas into iram (they can be recognized with their symbol name).

What can be easily done is calling panic() from attachInterrupt() when user function is not in iram.

#include <PolledTimeout.h>

#include <ESP8266WiFi.h>

esp8266::polledTimeout::periodicFastMs logger(1000);
unsigned long t0;
uint32_t x = 0;
void* currentpc;

// assembly to store a label address into global currentpc
#define ASM(func, label) \
  inline void func() __attribute__((always_inline)); \
  void func () { __asm__ __volatile__(label ": movi a6, " label " ; mov %0,a6":"=a"(currentpc)::"a6"); }

ASM(a1, ".L3333")
ASM(a2, ".L3334")
ASM(a3, ".L3335")

ICACHE_RAM_ATTR auto lambda  = []() ICACHE_RAM_ATTR {
  x++;
  a1();
};

ICACHE_RAM_ATTR void plainfunc () {
  x++;
  a2();
};

WiFiServer getout(23);

void test_attachInterrupt(uint8_t pin, void (*userFunc)(), int mode)
{
  Serial.printf("attach: %p\n", userFunc);
  attachInterrupt(pin, userFunc, mode);
}

void setup() {
  Serial.begin(115200);

  // using wifi server to shake flash cache
  // shaking: "while true; do echo a | nc 10.0.1.226 23; done"
  WiFi.mode(WIFI_STA);
  WiFi.begin(STASSID, STAPSK);
  while (WiFi.status() != WL_CONNECTED)
  {
    delay(500);
    Serial.print('.');
  }
  Serial.println("connected");
  
  getout.begin();
  t0 = millis();

  //test_attachInterrupt(D2, plainfunc, CHANGE); // shows in IRAM space
  // or:
  //test_attachInterrupt(D2, lambda, CHANGE); // shows in FLASH space
  // or:
  test_attachInterrupt(D2, []() ICACHE_RAM_ATTR { x++; a3(); }, CHANGE); // shows in FLASH space
}


void loop() {
  if (logger)
  {
    unsigned long t1 = millis();
    Serial.printf("%10lums %10u %10lu %p\n", t1 - t0, x, 1000 * x / (t1 - t0), currentpc);
  }

  if (getout.hasClient())
    getout.available().println("I am busy");
}

[dok-net]

@d-a-v Great, now, to keep the related information together in one place - one get's confused - is this the pertaining test for the currentpc value (taken from core_esp8266_wiring_digital.cpp)?

if ((uint32_t)currentpc >= 0x40200000)
{
	// currentpc not in IRAM
	::printf((PGM_P)F("currentpc not in IRAM!\r\n"));
} else
{
	::printf((PGM_P)F("currentpc in IRAM\r\n"));
}

[dok-net]

@d-a-v And if so, then

attachInterrupt(digitalPinToInterrupt(m_rxPin), [this]() { rxRead(this); a1(); }, CHANGE);
gives
currentpc not in IRAM!

Cross-check:

void ICACHE_RAM_ATTR SoftwareSerial::rxRead(SoftwareSerial * self) {
	a1();
...

gives
currentpc in IRAM

[dok-net]

@d-a-v In plain words, I don't understand what std::function<any(...)>::operator()() const is or does or what relevance the operator() has - the fact of the matter seems to be, that the lambda's code is not I IRAM, and isn't that what it is all about?

[d-a-v]

It seems we recognize only std::bind() and not pure lambda like you do, which is mangled differently.
In the example I'm trying to understand, I get: _ZNUlvE_4_FUNEv which is {lambda()#1}::_FUN() from auto lambda = []() {...
@earlephilhower

[d-a-v]

I put this symbol into the linker file and the lambda is moved to IRAM.
We could now find the correct matching generic string for those lambdas but that would move every single lambda right into IRAM and that's probably unwise given its size.
Would you try with std::bind for attachInterrupt() instead, or use a regular IRAM_ATTR function ?

[earlephilhower]

The linker moves lambda/std::bind into IRAM basically by a simple REGEX in the linker file. So for std::bind which has a limited set of signatures (mangled names, basically) we added those to the IRAM section.

Lambdas can probably be detected by name mangling rules and moved into IRAM, but the problem is that then all lambda functions, even ones used in regular app code (the vast majority) will be pushed into IRAM and you'll end up not being able to link anymore. I'm with @d-a-v, it's not a feasible option for the 8266.

The root of the problem is that in GCC you can't put a section on lambdas (well, you can, but GCC ignores it and doesn't emit code with the changed section).

Not sure there's much that can be done here.

[hreintke]

@earlephilhower @d-a-v
I've been checking too with RTTI enabled.
My current understanding is that the lambda name mangling is not defined and compiler dependent.