ESP32 - IRAM interrupts

[florentbr]

I noticed that the current implementation uses non IRAM functions in the ESP32 interrupts :

dimmable-light/src/hw_timer_esp32.cpp

Lines 39 to 56 in 21edb0d

	void ARDUINO_ISR_ATTR startTimerAndTrigger(uint32_t delay) {
	timerWrite(timer, 0);
	timerAlarmWrite(timer, delay, false);
	timerAlarmEnable(timer);
	timerStart(timer);
	}
	void ARDUINO_ISR_ATTR setAlarm(uint32_t delay) {
	timerAlarmWrite(timer, delay, false);
	// On core v2.0.0-2.0.1, the timer alarm is automatically disabled after triggering,
	// so re-enable the alarm
	timerAlarmEnable(timer);
	}
	void ARDUINO_ISR_ATTR stopTimer() {
	timerStop(timer);
	}

All the interrupt code needs to be placed into IRAM to avoid long latency or crash when the flash is locked.

https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/system/intr_alloc.html#iram-safe-interrupt-handlers

Either replace the timer functions with specific isr timer functions from timer.h:

timer_group_set_alarm_value_in_isr(...)
timer_group_set_counter_enable_in_isr(...)
timer_group_enable_alarm_in_isr(...)

or with inlined low level functions from timer_ll.h :

#define TIMER_NUM  ( 2 )
#define TIMER_HW   ( TIMER_LL_GET_HW(TIMER_NUM / SOC_TIMER_GROUPS) )
#define TIMER_ID   ( TIMER_NUM % SOC_TIMER_GROUP_TIMERS_PER_GROUP )

timer_ll_set_counter_value(TIMER_HW, TIMER_ID, 0);
timer_ll_set_alarm_value(TIMER_HW, TIMER_ID, 5000);
timer_ll_set_alarm_enable(TIMER_HW, TIMER_ID, true);

[fabianoriccardi]

Thanks for the observation. I had replaced the IRAM_ATTR with ARDUINO_ISR_IRAM Docs in the latest release because it doesn't make sense placing my library functions in IRAM if the Arduino HAL (which I rely on) is not there. Moreover, in the previous releases, from what I can understand, IRAM_ATTR wasn't providing a real benefit because I was registering interrupts without ESP_INTR_FLAG_IRAM.

Given the current implementation, I except possible "long latency" but no crashes. I don't know how much this latency can be.

As you point out, I may replace all Arduino HAL calls with "native" low-level calls about timers and GPIOs, but it will require some effort. I can consider it.

[florentbr]

Using IRAM_ATTR or ARDUINO_ISR_IRAM on an interrupt is pretty much useless if the interrupt itself is calling even a single non IRAM function. The ESP_INTR_FLAG_IRAM is there to place the interrupt handler in IRAM as well.

The doc says the ARDUINO_ISR_IRAM flag makes some HAL function, like digitalRead/Write and more to be loaded into IRAM. Looking at the Arduino sources, digitalWrite is indeed tagged as IRAM but somehow it rely on gpio_set_level which is not. There's a closed issue about it on github: espressif/esp-idf#2408 .

What's disturbing is that the hal functions timerWrite, timerAlarmWrite, timerAlarmEnable, timerStart from Arduino esp32-hal-timer.h are not tagged IRAM but the ones from IDF hal/timer_hal.h are.

All the code from Espressif are using interrupts with code executed exclusively from IRAM.
Since this library has no control over the rest of the code, it's probably best to implement the interrupts in IRAM as recommended. From Espressif:

It can also be useful to keep an interrupt handler in IRAM if it is called very frequently, to avoid flash cache misses.

Some discussions on the subject:
espressif/arduino-esp32#3697
https://esp32.com/viewtopic.php?t=4978

[fabianoriccardi]

The routine will be called 100 times/second if AC@50Hz, so it will stay in IRAM very probably and you won't have problems. The same for the other ISRs.

If your firmware performs a lot of flash operations will have sporadic huge latencies, and this is bad.
As you point out, there is not consistency in HAL IRAM_ATTR usage, so, the only way to completely solve this issue is to use registers or the lowest-level functions that are in IRAM.

About timer_ll.h gpio_ll.h
Those functions are inline, not explicitly in IRAM but as consequence they should be embedded in IRAM as the caller. Same for gpio_ll_set_level.

Hence, there is a chance to code an optimized version which stays in IRAM despite the Arduino config.

[florentbr]

About timer_ll.h gpio_ll.h
Those functions are inline, not explicitly in IRAM but as consequence they should be embedded in IRAM as the caller. Same for gpio_ll_set_level.

You're right, sorry i meant to say that one inherits the IRAM attribute while the other (Arduino) doesn't.

[fabianoriccardi]

Just for clarity, the issue #23 and #16 was related to your concerns, I will investigate it.

[mathieucarbou]

FYI I have extracted a ZCD I am using for YaSolR (solar diverter) to https://github.com/mathieucarbou/MycilaPulseAnalyzer

I've managed to recreated some inlined versions of gptimer calls: they are isolated in a header. (inlined_gptimer.h)

Thanks to that I am able to compile the examples with

  -D CONFIG_ARDUINO_ISR_IRAM=1
  -D CONFIG_GPTIMER_ISR_HANDLER_IN_IRAM=1
  -D CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM=1
  -D CONFIG_GPTIMER_ISR_IRAM_SAFE=1
  -D CONFIG_GPIO_CTRL_FUNC_IN_IRAM=1

And have all the ISR code loaded into IRAM.

The 2 examples are doing concurrent flash (nvs) operations to prove that there is no crash anymore (before, a crash would happen immediately or after a few seconds).

This is not as optimised as the code @florentbr has done (triac_timer.cpp, which inspried me a lot), but it it far less complex to use eventually in a project like dimmable light.

Also, using gpio_ll instead of arduino GPUI functions is quite svraitworward.

Note: my library only work with Arduino >= 3 and esp32 boards.