Encoder icon indicating copy to clipboard operation
Encoder copied to clipboard

ESP8266/ESP32: Use FunctionalInterrupt or attachInterruptArg by default

Open mcspr opened this issue 6 years ago • 5 comments

supercede #15 take 2 on #38

Smaller Encoder::attach_interrupt, encoder state arg is stored by the Core instead of us Two possible implementations, as FunctionalInterrupt status in current Core is uncertain. attachInterruptArg directly exposes that functionality

Note, that only ESP8266 Core >= 2.5.1 places functional interrupt handler in IRAM

Some questions: Should esp platform check go into a separate header? like interrupt_config.h Is there a reason we must have .cpp file to build this code as stub library?

edit after latest push

mcspr avatar May 01 '19 08:05 mcspr

It is still not buildable though. I raised a question in espressif/arduino-esp32 gitter chat about this issue. https://gitter.im/espressif/arduino-esp32?at=5cc95f25990feb451831fdb9

Linking .pioenvs/lolin32/firmware.elf
.pioenvs/lolin32/src/Basic.pde.cpp.o: In function `Encoder::update(Encoder_internal_state_t*)':
Basic.pde.cpp:(.iram1[Encoder::update(Encoder_internal_state_t*)]+0x3d): dangerous relocation: l32r: literal placed after use: .literal._ZN7Encoder6updateEP24Encoder_internal_state_t

Otherwise, idk of a fix other than moving Encoder::update to a global function.

mcspr avatar May 01 '19 09:05 mcspr

Important thing about examples and ESP8266/ESP32 - we cannot use the pins 6-11 (inclusive). From wroom datasheet:

  • Pins SCK/CLK, SDO/SD0, SDI/SD1, SHD/SD2, SWP/SD3 and SCS/CMD, namely, GPIO6 to GPIO11 are connected to the integrated SPI flash integrated on the module and are not recommended for other uses.

Same applies for most ESP8266 boards. But, in some configurations it is possible to use pins 9&10.

mcspr avatar May 04 '19 13:05 mcspr

Surely there is a better way to implement this PR.. still touches lot of code

tablatronix avatar Aug 15 '19 13:08 tablatronix

Maybe this should be PR to esp8266 branch and merged so we could at least test it easier ?

tablatronix avatar Aug 15 '19 14:08 tablatronix

How though? My main interest was to have compatible solution for both boards, but we are at a standstill with update() move for some reason (I have not changed it at all, just made global method instead of static class one)

  • we need IRAM_ATTR / ICACHE_RAM_ATTR somewhere. In hypothetical case, where Encoder class is declared and only then implemented, implementation can ignore the attr:
class Encoder {
...
    static void IRAM_ATTR update(...);
...
};

void Encoder::update(...) {
...
}
  • due to the linker problems, which I have not dug into to much, we can't use IRAM_ATTR on static class method.... unless there are -mauto-litpools -mtext-section-literals in cxxflags. esp8266 arduino platform flags have it -> https://github.com/esp8266/Arduino/blob/8f45a0fb91621de2c8bedb5723f2eb3b527706c4/platform.txt#L61 My understanding is that we need to ask this question via https://esp32.com forums to figure out a reason why those are missing with IDF / arduino-esp32, since this specific issue had no interest so far.

Sidenote there is also an arduino-devel mailing list discussion regarding attribute use: https://groups.google.com/a/arduino.cc/forum/#!topic/developers/56VPC9t2MPc

mcspr avatar Aug 16 '19 14:08 mcspr