jandelgado / jled

Non-blocking LED controlling library for Arduino and friends.
MIT License
331 stars 55 forks source link

ESP-IDF component #86

Closed troky closed 2 years ago

troky commented 2 years ago

Removed Arduino.h dependency and used native functions so this lib can be used as native ESP-IDF component. CMakeLists.txt changed accordingly.

jandelgado commented 2 years ago

@troky, thanks for the contribution. Please check the CI and make sure that it still works with the Arduino Framework. Could you please also add a simple example showing how to use, similar to the Pico or Mbed examples?

troky commented 2 years ago

Don't have Arduino around as I don't use PlatformIO. Basically code is copied from Arduino library so it should work without any problems. No need for any special example - Arduino example is good enough just without "#include ". It took 15mins to make this pull request and I've spent few hours now trying to play along CI... without success. I give up.

jandelgado commented 2 years ago

Ok, understand. Thanks for the efforts. I'll have a look at it at the weekend

jandelgado commented 2 years ago

@troky The unit test are using mockups to simulate SDK API calls issued by jled. For example there are implementations for analogWrite, pinOut etc. For the new ESP32-HAL there are now some functions and dummy-Headers missing. I'll fix it in the next days

jandelgado commented 2 years ago

@troky where did you copy the code from the Arduino lib? Another problem I found is that the code does not compile with the Arduino framework, because the Arduino Framework seems to use an older ESP-IDF version, where e.g. the definition of struct ledc_timer_config_t is different, since later a ledc_clk_cfg_t clk_cfg attribute was introduced. This leads to compile errors:

in file included from lib/src/esp32_hal.cpp:23:0:
lib/src/esp32_hal.h: In constructor 'jled::Esp32Hal::Esp32Hal(jled::Esp32Hal::PinType, int, uint16_t)':
lib/src/esp32_hal.h:96:28: error: 'LEDC_AUTO_CLK' was not declared in this scope
                 .clk_cfg = LEDC_AUTO_CLK
                            ^
lib/src/esp32_hal.h:97:9: error: 'ledc_timer_config_t' has no non-static data member named 'clk_cfg'
         };
         ^
lib/src/esp32_hal.h:110:9: error: 'ledc_channel_config_t' has no non-static data member named 'flags'
         };
         ^
Compiling .pio/build/esp32dev/FrameworkArduino/Esp.cpp.o
*** [.pio/build/esp32dev/lib17c/src/esp32_hal.cpp.o] Error 1
In file included from lib/src/jled.h:45:0,
                 from /tmp/tmp1ra2t2y0/src/breathe.ino:4:
lib/src/esp32_hal.h: In constructor 'jled::Esp32Hal::Esp32Hal(jled::Esp32Hal::PinType, int, uint16_t)':
lib/src/esp32_hal.h:96:28: error: 'LEDC_AUTO_CLK' was not declared in this scope
                 .clk_cfg = LEDC_AUTO_CLK
                            ^
lib/src/esp32_hal.h:97:9: error: 'ledc_timer_config_t' has no non-static data member named 'clk_cfg'
         };
         ^
lib/src/esp32_hal.h:110:9: error: 'ledc_channel_config_t' has no non-static data member named 'flags'
         };
         ^

So we need to support two different esp32 HAL's: one for the Arduino Framework and one for the newer ESP-IDF SDK.

Another thing that is a (litte) problem is the usage of the designated initializers, which are part of the C++ 20 standard, which also seems not to be used in platformio (and ArduinoIDE):

https://github.com/troky/jled/blob/103dc5f79c9b63aa62c78ad57ccd7ba54db5119f/src/esp32_hal.h#L101-L110

troky commented 2 years ago

Yes, that could be the problem.. Arduino library doesn't follow esp-idf releases ususaly. Code is copied from https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/esp32-hal-ledc.c ledcSetup, ledcAttachPin, ledcWrite.

Maybe you are right about creating separate HALs. I tried to keep it simple :)

jandelgado commented 2 years ago

@troky fyi: I built an example using the esp-idf framework and will release a JLed version with an esp-idf ESP32 HAL shortly

troky commented 2 years ago

Cool! Thanks.

jandelgado commented 2 years ago

@troky could you please check if this version https://github.com/jandelgado/jled/tree/add_esp_idf_sdk_support works for you?

troky commented 2 years ago

Yes, it compiles and works as expected. Only change I had to do is change: ledc_channel.flags = { 0 }; to ledc_channel.flags.output_invert = 0; in esp32_hal.h because for some reason original line didn't initialize ledc_channels struct correctly.