Is IRAM_ATTR really necessary for interrupts?

[platisd]

I want to add support for ESP32 boards to my library and I am trying to keep the code as platform-agnostic as possible.
In regards to ISR's, the documentation is clear in regards to the use of IRAM_ATTR in the signature of the interrupt handlers:

Interrupt handlers must be placed into IRAM if ESP_INTR_FLAG_IRAM is used when registering the interrupt handler. In this case, ISR may only call functions placed into IRAM or functions present in ROM.

However, I do not see any difference when omitting ISR_ATTR. For example:

// Odometer.hpp
struct Odometer {
    void update();
    volatile unsigned long counter { 0 };
};

// Odometer.cpp
void Odometer::update() {
    counter++;
}

// MySketch.ino
Odometer odometer;

void setup() {
    attachInterrupt(1, [](){ odometer.update(); }, RISING);
}

void loop() { }

Without doing a really thorough investigation, seems to work as fine as the following does:

// Odometer.cpp
void ISR_ATTR Odometer::update() {
    counter++;
}

Is the wording (i.e. the "must" part) on the Espressif documentation misleading or will this potentially cause problems?

[atanisoft]

If the method is not tagged with IRAM_ATTR it will NOT be called during operations involving flash read/write. There may be other cases as well.

Also note that using c++ binding for ISRs can lead to other issues as the c++ binding code won't be tagged as IRAM_ATTR.

You will also need debounce checks for pin interrupts like you show in your example code.

[platisd]

If the method is not tagged with IRAM_ATTR it will NOT be called during operations involving flash read/write. There may be other cases as well.

If I understood this correctly, since I in my case I am not doing any flash operations, it is fine?

Also note that using c++ binding for ISRs can lead to other issues as the c++ binding code won't be tagged as IRAM_ATTR.

OK good to know. I tried declaring new/static functions in my .ino sketch with the IRAM_ATTR tag and also could not detect any different behavior.

You will also need debounce checks for pin interrupts like you show in your example code.

Yes there are, but I didn't wanna put irrelevant parts of the library here. :)

[atanisoft]

If I understood this correctly, since I in my case I am not doing any flash operations, it is fine?

It really depends on how the users of your library will use the ESP32. If they use SPIFFS and a webserver on the ESP32 then there is a very high chance of the ESP32 crashing while serving up web content IF your ISR is triggered/running at the time of flash access.

[platisd]

Oh, that is indeed a possible scenario! 😱

Also note that using c++ binding for ISRs can lead to other issues as the c++ binding code won't be tagged as IRAM_ATTR.

I'l think this through, but so far it does not seem like I will be able to find a way to get to use the library in the same way in all platforms. 😞

[atanisoft]

I guess the problem I am facing is related to #2748

Yes, that could be related. In general I would recommend using IRAM_ATTR AND have the ISR be VERY simple in that it would simply wake up a background task that is blocked on a FreeRTOS queue/semaphore/etc so that the ISR finishes as quickly as possible. If an ISR runs for more than ~300usec it will trigger the WDT.

[platisd]

Sorry I removed my previous response, but good to know about the 300uS limit.
I believe following this example should solve my problems so I am leaving it here for future reference.

Thanks for your help!

[chegewara]

You can make function with IRAM_ATTR conditionally on compile with #if defined ESP32 or something like that.

[platisd]

You can make function with IRAM_ATTR conditionally on compile with #if defined ESP32 or something like that.

I know, but I consider this a code smell. Application logic (e.g. my library) should not depend or care about domain logic (e.g. ESP32).

[chegewara]

Why? This is common use case in libraries that are working on esp8266 and esp32 (not IRAM_ATTR, but other functions).
It wont be esp32 dependent, will add IRAM_ATTR only for esp32 and can prevent crash in cases Mike described earlier.

[platisd]

Yes, it is common for libraries working for those two platforms but I don't see it as a case for libraries that are supposed to work on any platform and potentially not even microcontrollers. :)

The problem is that "having the ISR in RAM and not in flash" is an espressif-specific detail that will (conceptually) have to leak into the application logic.
To make matters worse, anything called within the ISR (ie. I am reading another pin to determine the direction the wheel is spinning) also has to be "in RAM and not in flash" from what I understand. The "smell" gets stronger in other words. 😢

[chegewara]

Of course its your library and you will do what you think is the best for you, but here is another example library that is not esp8266 or esp32 aimed:
https://github.com/FastLED/FastLED/blob/c563be6a37d9d561eb53f49a233fafd5c8c8b44a/fastled_progmem.h#L72-L76

I think you may know this library because its very popular.

[platisd]

Solving this on a technical level is not difficult and the link you provided shows how it would be done.
My problem is on an API-design/platform/conceptual level.

Let me explain: In my library, I am trying to keep the business logic as hardware-agnostic as possible. Therefore, I use an abstraction layer which is essentially a wrapper around "Arduino" (or potentially any other runtime environment), which includes GPIO read/writes, setting interrupts etc and all platform-specific details are taken care by it.

Anything that is "above" this layer (i.e. uses this layer), should not care at all about the platform-specific details. For example, if I am creating an Odometer class, I should not care about how IO is done in all the different platforms the code may run. The Odometer class should not care about how the interrupt mechanism works internally, but merely provide a function as a callback and that callback to be invoked upon the arrival of a pulse.

API-design wise, the fact that the callback must be in RAM and not in flash memory, is an implementation detail that is leaked through the arduino-esp32 API, which is a bad practice. Please note that I am not judging whether there is a better way of doing it. It is how it is at the moment.
To make matters worse, the API can be misused (i.e. passing a callback that is not in RAM) without this being prevented in a fool-proof (i.e. during compilation) manner. #2748 tried to point out the misuse problem.

[liebman]

In your abstraction layer for the esp32 you can create a task that waits on some queue. In the interrupt routine only post to the queue and return. The task you created then will wake up and call the callback. This is what I have done on a few projects and it worked very well.

[platisd]

In your abstraction layer for the esp32 you can create a task that waits on some queue. In the interrupt routine only post to the queue and return. The task you created then will wake up and call the callback. This is what I have done on a few projects and it worked very well.

Sounds potentially interesting but I have not used FreeRTOS before. Do you have any code examples that implement this?

[atanisoft]

Sounds potentially interesting but I have not used FreeRTOS before. Do you have any code examples that implement this?

You might look at ulTaskNotifyTake and xTaskNotifyFromISR, these are very light weight and can be used to wake up a running task (waiting on ulTaskNotifyTake) from an ISR. If you need/want to pass a value from the ISR to the task you can use vTaskNotifyGiveFromISR (in ISR) and xTaskNotifyWait (in task).

[me-no-dev]

I've been thinking to make the IRAM requirement optional in the menu. It is there for various reasons, but can now be made optional. There are performance consequences when you ISR not in IRAM.

[dpharris]

This would be good, as long as there is a good explaination of the consequences, and the requirements. Not all ISRs need to be extremely fast. David

…

On Sat., Feb. 1, 2020, 4:56 a.m. Me No Dev, ***@***.***> wrote: I've been thinking to make the IRAM requirement optional in the menu. It is there for various reasons, but can now be made optional. There are performance consequences when you ISR not in IRAM. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3697?email_source=notifications&email_token=AAEDQSWPFZQTGCONN7FBFELRAVWQTA5CNFSM4KOFHATKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKQ4PQA#issuecomment-581027776>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEDQSRCONJARLR35O37DODRAVWQTANCNFSM4KOFHATA> .

[platisd]

Sounds potentially interesting but I have not used FreeRTOS before. Do you have any code examples that implement this?

You might look at ulTaskNotifyTake and xTaskNotifyFromISR, these are very light weight and can be used to wake up a running task (waiting on ulTaskNotifyTake) from an ISR. If you need/want to pass a value from the ISR to the task you can use vTaskNotifyGiveFromISR (in ISR) and xTaskNotifyWait (in task).

@atanisoft tried to look into this but to no avail. As I mentioned, my FreeRTOS experience is very limited. 😺
For now I am going with the FunctionalInterrupt way and depending on what platform the library's users are aiming for, they can either use a lambda or std::bind.

I've been thinking to make the IRAM requirement optional in the menu. It is there for various reasons, but can now be made optional. There are performance consequences when you ISR not in IRAM.

@me-no-dev do you mean there may be a way for an ISR that is not in RAM to not cause a fatal problem (i.e. crash during flash read/write)? If that's the case, it would increase the API's correctness, since as of its current state it may be easily misused.

This would be good, as long as there is a good explaination of the consequences, and the requirements. Not all ISRs need to be extremely fast. David

@dpharris +1 to that. 👍

[atanisoft]

@platisd a lambda and std::bind result in the same signature so they will both fail with FunctionalInterrupt. That code is inherently broken as referenced in the other issue since it is not in IRAM and will lead to very odd crashes if it fires at the same time as any flash access is active. The only solution that will work reliably is a FreeRTOS method to push a message to a blocked task which can then call your callback safely, perhaps this is a solution for FunctionalInterrupt.

[platisd]

So FunctionalInterrupt and its example which uses std::bind are inherently broken atm?! 😢
Oh well, back to the drawing board it is. 😔

[atanisoft]

It could be fixed without modifications to usages likely. But be warned that it has stability issues due to flash access during the ISR firing.

[platisd]

Just for reference for others in the same situation, I am scraping std::bind and going only lambdas (as initially planned) with:

const uint8_t odometerPin   = 2;
const unsigned long pulsesPerMeter = 400;

DirectionlessOdometer odometer(
    odometerPin, []() { odometer.update(); }, pulsesPerMeter);

Where DirectionlessOdometer::update() has STORED_IN_RAM in its definition. STORED_IN_RAM is a macro that depending on the platform (ESP32/ESP8266/Other) gets the appropriate value (it's empty for AVRs for example).

The class will be provided along with a warning to the library users that if they see random crashes when they are doing flash intensive operations on Espressif platforms (e.g. SPIFFS and a server for hosting websites etc), they should additionally do:

const uint8_t odometerPin   = 2;
const unsigned long pulsesPerMeter = 400;

DirectionlessOdometer odometer(
    odometerPin, []() { odometer.update(); }, pulsesPerMeter);

void IRAM_ATTR updateOdometer() {
    odometer.update();
}

void setup() {
    attachInterrupt(digitalPinToInterrupt(odometerPin), updateOdometer, RISING);
}

This, at least until I figure out how to do the FreeRTOS magic. :)

[trycoon]

As a newbie this is a bit much to comprehend, currently I have code like:

#include <FunctionalInterrupt.h>
attachInterrupt(sonarFront.sense_pin, std::bind(&Sonar::onPing, this), CHANGE);

void IRAM_ATTR Sonar::onPing() {
   ....
}

and

    sensorReadingTicker.attach_ms<IO_Accelerometer *>(
        20, [](IO_Accelerometer *instance) {
          instance->getReadings();
        },
        this);

And do use WiFi and also read/write to Preferences.h, so this could potentially cause stability issues ??

Could anyone with more knowledge than me provide me with some example code of how this should be done correctly? Or give me a link to a library/application that does everything in the correct manner. Maybe an example using vTaskNotifyGiveFromISR?

Thanks!

[trycoon]

Does this mean that this example is broken if we access Flash and WiFi?
https://github.com/espressif/arduino-esp32/blob/master/libraries/ESP32/examples/GPIO/FunctionalInterrupt/FunctionalInterrupt.ino
Since it relies on std::bind.

[platisd]

Does this mean that this example is broken if we access Flash and WiFi?
https://github.com/espressif/arduino-esp32/blob/master/libraries/ESP32/examples/GPIO/FunctionalInterrupt/FunctionalInterrupt.ino
Since it relies on std::bind.

That was my understanding unfortunately as well.