Fix attach_interrupt(...) for esp-idf framework

[mmakaay]

What does this implement/fix?

The attach_interrupt(...) implementation for the esp32 component uses ESP_INTR_FLAG_LEVEL5 for gpio_install_isr_service(...). This interrupt level requires handlers that are written in assembly. Therefore C-code handler are not acceptable and using those results in a failed interrupt setup.

Additionally, the return value of gpio_install_isr_service(...) was not checked, resulting in a LoadProhibited abort, because of a NULL pointer exception. further down the road. I added a check to see if it actually returns ESP_OK.

Note:
Adding some logging in case of a fail would be a nice to have, but this code currently lives in the gpio_idf.h file and not the gpio_idf.cpp file. That makes the structure different from the other .h files and I shouldn't define a TAG in this .h file for logging purposes.
Is there a reason why this code is in the .h file or is it okay if I split declaration and implementation between respectively the .h and the .cpp file here?

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Other

Related issue or feature (if applicable): fixes esphome/issues#2483

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#

Test Environment

• ESP32 (unicore, not 100% sure if that matters)
• ESP8266

Example entry for config.yaml:

# Example config.yaml
esp32:
  board: esp32doit-devkit-v1
  framework:
    type: esp-idf
    sdkconfig_options:
      CONFIG_FREERTOS_UNICORE: y

# Just an example, I saw the behavior with all code that attached a pin interrupt.
sensor:
  - platform: rotary_encoder
    name: "Rotary Encoder"
    pin_a: GPIO16
    pin_b: GPIO17

Checklist:

• The code change is tested and works locally.
• Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated in esphome-docs.

[probot-esphome]

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (esp32) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[mmakaay]

Tomorrow, I'll dig up a normal dual core ESP32 to see if the fix is only applicable to ESP32 unicore.
If this is the case, then I'll implement something to differentiate the handling between unicore and dual core.

[mmakaay]

Okay, this PR is useful for a regular dual core EPS32 as well. I tried flashing one with the rotary encoder for testing the interrupt setup, and this is what I ended up with:

[12:50:26]I (407) cpu_start: Starting scheduler on PRO CPU.
[12:50:26]I (0) cpu_start: Starting scheduler on APP CPU.
[12:50:26][I][app:029]: Running through setup()...
[12:50:26][C][rotary_encoder:127]: Setting up Rotary Encoder 'Rotary Encoder'...
[12:50:26][D][esp-idf:000]: I (210) gpio: GPIO[16]| InputEn: 1| Guru Meditation Error: Core  1 panic'ed (LoadProhibited). Exception was unhandled.
[12:50:26]
[12:50:26]Core  1 register dump:
[12:50:26]PC      : 0x4014e123  PS      : 0x00060533  A0      : 0x800e2b0c  A1      : 0x3ffbf180  
[12:50:26]A2      : 0x00000000  A3      : 0x00000000  A4      : 0x00000010  A5      : 0x3ffb0020  
[12:50:26]A6      : 0x00000028  A7      : 0x00020000  A8      : 0x800e2809  A9      : 0x3ffbf150  
[12:50:26]A10     : 0x3ffb0024  A11     : 0x00000010  A12     : 0x00000010  A13     : 0x00060523  
[12:50:26]A14     : 0x00000001  A15     : 0x00000011  SAR     : 0x00000010  EXCCAUSE: 0x0000001c  
[12:50:26]EXCVADDR: 0x00000000  LBEG    : 0x4000c2e0  LEND    : 0x4000c2f6  LCOUNT  : 0xffffffff  
[12:50:26]
[12:50:26]Backtrace:

0x4014e120: esp_intr_get_cpu at /home/mmakaay1/.platformio/packages/framework-espidf/components/esp_system/intr_alloc.c line 719
0x400e2b09: gpio_isr_handler_add at /home/mmakaay1/.platformio/packages/framework-espidf/components/driver/gpio.c line 484
0x400d5c3b: esphome::esp32::IDFInternalGPIOPin::attach_interrupt(void (*)(void*), void*, esphome::gpio::InterruptType) const at src/esphome/components/esp32/gpio_idf.h line 83
0x400d77ad: esphome::rotary_encoder::RotaryEncoderSensor::setup() at src/esphome/core/gpio.h line 80
0x4014dded: esphome::Component::call_setup() at src/esphome/core/component.cpp line 67
0x4014de65: esphome::Component::call() at src/esphome/core/component.cpp line 78
0x400db955: esphome::Application::setup() at src/esphome/core/application.cpp line 38
0x400dcb01: setup() at src/main.cpp line 174
0x400d5b87: esphome::loop_task(void*) at src/esphome/components/esp32/core.cpp line 71

So the same symptom here.
I'll flag the PR as ready for review, since no further differentiation is required.

[OttoWinter]

Is there a reason why this code is in the .h file or is it okay if I split declaration and implementation between respectively the .h and the .cpp file here?

no, only laziness 😝

[OttoWinter]

LGTM. let me know if you want to move it to cpp, otherwise we can also merge now

[mmakaay]

I will move it to the .cpp file. Moved the PR to draft for the time being.

[mmakaay]

Alright, moved the code to the .cpp file, and added an error log now I had access to TAG.

[mmakaay]

Ow, checks failing, that's what you get for committing at 3am 😄 I'll fix the issues tomorrow, and give the code a spin on a few of my devices for testing.

[mmakaay]

Okay, the code should be ready now. The checks are running, fingers crossed 🤞