PaulStoffregen / Encoder

Quadrature Encoder Library for Arduino
http://www.pjrc.com/teensy/td_libs_Encoder.html
561 stars 243 forks source link

ESP8266/ESP32: Use FunctionalInterrupt or attachInterruptArg by default #39

Open mcspr opened 5 years ago

mcspr commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

tablatronix commented 5 years ago

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

tablatronix commented 5 years ago

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

mcspr commented 5 years ago

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)

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