get ready for ESP-IDF v5

[jdoubleu]

Fixes #156

WIP: Pleas do not merge, yet!

~At the moment, this PR is based on works in #177, so the CI is running.~ CI doesn't run, because the GitHub CI config has not been merged into master, yet.

Issues

Some symbols were no longer implicitly imported, so I included some rather specific headers.

However, there are a few issues remaining:

1. Some functions, like rtc_clk_apll_enable changed their signature. They are currently used in i2s_data_bus.c.
2. The RMTMEM global variable, used by rmt_pulse.c does no longer exist. Some HAL stuff seems to have been refactored: https://github.com/espressif/esp-idf/commit/9f55712c0312f05f79893fa8d8972b0b7e13df13

[second-string]

Thanks for starting this work! I fixed up the remaining few issues to at least get epdiy compiling with esp-idf v5. It still spits some warning for future deprecation of modules like the legacy RMT peripheral and the periph_ctrl.h header, but this at least unblocks development for me and maybe others. I haven't tested these changes on an actual screen yet, although they're super minor so I'd be shocked if they caused issues.

I didn't want to fork your fork to build on top of it so I just copied your fork on this esp-idf-latest branch into my components directory. The changes you can see in this commit is the remaining diff on top of the work you already did: https://github.com/dot4qu/spot-check-firmware/commit/65de40cc2602862f55681a298021bb809b939db7

[second-string]

I just confirmed that these changes do work properly to drive a screen. Tested with an ED060SC4 driven by my custom board using the changes linked in my commit above. I was able to clear the screen, draw a box, and draw text.

[jdoubleu]

Thanks for your work, @dot4qu! I'll try to include your changes in this PR as well.

I'm not sure whether https://github.com/vroland/epdiy/pull/168 has any impact on this. I know there have been quiet some changes.

I still think this needs extensive testing and approval from at least two maintainers.

[martinberlin]

In my opinion to fully test this after #183 master should be merged back into this branch. At least i2s_data_bus.c was also touched by 168 update. When that is done I will take some time to test it but also another developers with V6 as @mickeprag or V5 as @mcer12 should give a try to this update using latest IDF. Thanks for your collaboration @jdoubleu

[martinberlin]

#183 is merged. This can go forward when you find time for it @jdoubleu Please merge back master and fix the conflicts so you can add this V5 support on the top of @mickeprag new boards HAL.

[jdoubleu]

Hi, I've incorporated @dot4qu's changes and merged the current master into this branch, for now. There were some merge conflicts, however. I'm not sure why.

[martinberlin]

No problem, please check the test results so you can see where the build is failing. Thanks a lot for your contribution!

[martinberlin]

It seems there issues only compiling in IDF latest release:

 In file included from /app/src/epd_driver/display_ops.c:1:
/app/src/epd_driver/display_ops.h: In function 'fast_gpio_set_hi':
/app/src/epd_driver/display_ops.h:12:3: error: 'GPIO' undeclared (first use in this function)
   12 |   GPIO.out_w1ts = (1 << gpio_num);

And I don't really get why it's seeing duplicate functions between the display_ops header and C file.

[second-string]

It seems there issues only compiling in IDF latest release:

 In file included from /app/src/epd_driver/display_ops.c:1:
/app/src/epd_driver/display_ops.h: In function 'fast_gpio_set_hi':
/app/src/epd_driver/display_ops.h:12:3: error: 'GPIO' undeclared (first use in this function)
   12 |   GPIO.out_w1ts = (1 << gpio_num);

And I don't really get why it's seeing duplicate functions between the display_ops header and C file.

I remember looking into the GPIO macro definition a little bit, but I don't remember if I changed anything there or not. I'm traveling internationally right now so I most likely won't be able to take a look at this until around July 1st when I return.

It looks like the tests are all passing though - is this still an issue?

[martinberlin]

No rush, just take a detailed look when you are back.

It looks like the tests are all passing though - is this still an issue?

There are 8 failing test and I think all of them are with latest IDF release.

[martinberlin]

Hi!

It looks like the tests are all passing though - is this still an issue?

Yes there is still 8 tests that are not passing. We should take a deeper look to see what is going on. Currently busy with another projects so sadly I don't have much time left for testing but just ping me on SLACK if you need anything. Would be nice to merge this one and that EPDiy is working also with ESP-IDF v5

[second-string]

@jdoubleu there are two changes that need to happen to the code merged from the BSP PR:

1. In display_ops.h, we can't access the GPIO macro directly anymore. Add these two includes

#include "hal/gpio_ll.h"
#include "soc/gpio_struct.h"

and change the two fast_gpio functions to the following

inline void fast_gpio_set_hi(gpio_num_t gpio_num) {
    gpio_dev_t *device = GPIO_LL_GET_HW(gpio_num);
    device->out_w1ts   = (1 << gpio_num);
}

inline void fast_gpio_set_lo(gpio_num_t gpio_num) {
    gpio_dev_t *device = GPIO_LL_GET_HW(gpio_num);
    device->out_w1tc   = (1 << gpio_num);
}

2. Add one include to lut.c

#include "esp_timer.h"

Compiling most of the examples works for me now, but the tests will tell for sure

[second-string]

Ah dang looks like the jpeg decoder is having some issues as well, I'll take a look later this week

[jdoubleu]

Thanks for your contributions!

There are also some missing dependencies (e.g. libsodium). I wonder whether there's a mechanism to detect the ESP-IDF version in CMake. It seems like libsodium is not missing/required in ESP-IDF v4.

I've invited you to my fork, so you can also push into the esp-idf-latest branch directly, if you want.

[jdoubleu]

I noticed a few more issue:

There are a lot of warnings, which should probably be fixed. However, that is out of the scope of this PR. I'm not even sure if it's worth fixing them, since these are all examples and I hope nobody uses them in production.

Next, I probably found some license issues. Most of the examples contain third-party code. So does the terminal example. There's a comment in a source file referencing a license file. Unfortunately, there is no license file present for that example. The closest origin I could find was this fork: https://github.com/BBaoVanC/st

[second-string]

The last issue seems to be a weird problem with esp-idf's new component manager. It can't decide the right libsodium to pull in from the component list when using the component manager on v4.4. I opened an issue here https://github.com/espressif/esp-idf/issues/9309

The good news is that I can manually build fine on esp-idf 4.4 still. The issue seems to be in the CMakeLists.txt check that disables the component manager for versions < 5.0.0. I don't think it's working properly since the dependency lock file is still being generated in that output log.

So regardless of what the fix for the actual idf-component-manager issue is, all that needs to be figured out here is how to fix the check in CMakeLists.txt for the mpd_status project that correctly disables the component manager for versions < 5.0.0. Not sure where it's going wrong or what version it's getting from the docker image right now that's not entering that conditional correctly though

Edit: something else that works is removing the libsodium component from the new idf_components.yml file and just leaving it in the main/CMakeLists.txt idf_component_register file like it's always been. That breaks v5.0 though (obviously), so it don't know which location it's going to be easier to conditionally flag it off. Unless espresso fixes their side quickly ¯\_(ツ)_/¯

Edit x2: It's because the release docker images have different version numbers than non-release version. If you checkout the v4.4 tag for esp-idf, git describe gives you the expected v4.4 and that's it. That's why your check in CMakeLists.txt works locally for me. If you checkout what the image is checking out, which is release/v4.4, git describe gives you version + git SHA. More specifically, v4.4.1-185-gc5deb7a971.

Could you update that CMakeLists.txt check to not just run to the -1 index @jdoubleu? If you don't have time I can get to it this weekend

[jdoubleu]

Exactly! Only the check for the v4.4.1 version inside the container does not work. Interestingly it did work for v4.3.

The current approach is quiet hacky. Unfortunately, at that point, I do not have access to the IDF_VERSION_MAJOR property, yet. I assumed, that IDF_VER would point to the correct version. Thanks for investigating the issue and finding out that it uses release/v prefix inside the container.

It is really annoying that both packages collide in v5. Even when I explicitly used the package, i.e. espressif__libsodium.

EDIT: It becomes even more complicated: The version inside the container is actually release-4.4.1-472-gc9140caf8c. I'm not sure, how CMake's VERSION_LESS behaves in this situation.

[jdoubleu]

The tests pass 🎉

Now we need to check whether the examples actually run on hardware.

[martinberlin]

I would also like that someone tests a 6” display ED060XC3 or similar with a V5 board and the grayscale example (both in v4.4 and v5 IDF).

Not exactly related to this update but last time I tried got wrong results (check Slack) only with this size (9.7” works perfectly)

[second-string]

Just pushed a small tweak to the display_ops functions after testing on-device - it should have been using the GPIO port number, not the pin number, to get the LL device driver.

I tried to test all of the examples and most worked. The ones that worked and displayed correctly on my ED060SC4 w/ custom board were:

• calibration_helper
• demo
• dragon
• grayscale_test
• terminal

Examples that weren't tested fully:

• mpd_status: builds and flashes fine, ~~but there are a lot of warnings around args. I think these are easy fixes, I just didn't have the time to go further. See screenshot for list of warnings:~~ and I pushed a small change to fix the warnings. I don't think these had anything to do with our changes, my guess is that they've been in the example for a while. I did not test the actual behavior of this example because I don't know what it's supposed to do

• weather: fails to build with error: Failed to resolve component 'arduino'. It's been removed from esp-idf v5 CI tests, so I'm assuming it's acceptable since it depends on the arduino framework

• www-image: one log format specifier broken but I pushed a fix. ~~Unfortunately this fixes it for v5 but breaks it again for v4.X. Not sure how the best way to handle this as it's a breaking change between esp-idf versions~~

I did not test lily-t5-47-epd-platformio.

[second-string]

Edit: It seems as though this doesn't fix the issue for v4.3, just 4.4. The error seems to point to no support for the rules directive in the yaml file, I wonder if that was only added in esp-idf 4.4 I reopened the espressif issue, but we might just want to go back to the regex solution.

Edit x2: According to espressif:

@dot4qu IDF release-v4.3 docker image wasn't rebuilt for a while due to another issue. It's already fixed and now passing our internal CI. Should be updated soon So should be good to go as soon as they push out an update to the release-4.3 image 🤞

Original comment: I also just pushed a commit to remove the fancy regex in the mpd_status CMakeLists.txt to differentiate between esp-idf v4.X and v5.X. I fixed up the mpd_status/main/idf_component.yml file according to advice from the espressif folks to correctly exclude libsodium as an external component for any versions below 5.0.

Sorry to get rid of your hard work on that regex @jdoubleu 😄

[jdoubleu]

Great work @dot4qu !

but there are a lot of warnings around args. [..] I don't think these had anything to do with our changes, my guess is that they've been in the example for a while.

I found some of them very worrying, because there were a lot warnings related to pointer-to-pointer and pointer type discrepancies. As stated before, I don't think it's in the scope of this PR to fix all of them.

fails to build with error: Failed to resolve component 'arduino'. It's been removed from esp-idf v5 CI tests, so I'm assuming it's acceptable since it depends on the arduino framework

Exactly. The arduino component is not yet available for v5. I propose we wait for it and add support of the examples in a future PR?

www-image: one log format specifier broken but I pushed a fix.

Interesting that the CI didn't catch it. It only produced a warning, I guess? The version switch you used in d347a9956caa7e196d7df8f6aa1e4dc3c4b9ef58 looks good.

I also just pushed a commit to remove the fancy regex in the mpd_status CMakeLists.txt to differentiate between esp-idf v4.X and v5.X. I fixed up the mpd_status/main/idf_component.yml file according to advice from the espressif folks to correctly exclude libsodium as an external component for any versions below 5.0.

I think that's the better approach. It not only removes the fragile and somewhat complex CMake regex hack but at the same time expresses the dependency more explicitly.

Unfortunately, I couldn't find anything about the rules: directive in the documentation. Could you maybe point me to the discussion where you asked?

We should wait until espressif updates their espressif/idf:release-v4.3 docker image. Then, some of the maintainers should be able to re-run the pipeline.

I would also like that someone tests a 6” display ED060XC3 or similar with a V5 board and the grayscale example (both in v4.4 and v5 IDF).

@martinberlin I don't have a 6" display at hand. Maybe we can ask folks in the Slack?

We also need to test the examples on idf v4.4 (and maybe v4.3?) and those special cases with platformIO and arduino, right? As long as platformIO does support idf v5 already.

[second-string]

I also couldn't find anything on rules:. They might not have updated their new idf-component docs to include it yet 🤷 .

Here is where it was mentioned: https://github.com/espressif/esp-idf/issues/9309#issuecomment-1182083341

[jdoubleu]

When I'm building my personal project with these changes, I get lots of warnings from the epd_driver component. I'm not sure, whether they just increased the warnings level, but I didn't get any warning with v4.4.

I think we should fix them.

Here's the output form the ESP-IDF / build (latest, demo) run:

show

/app/src/epd_driver/render.c:125:29: warning: ignoring attribute 'section (".iram1.15")' because it conflicts with previous 'section (".iram1.5")' [-Wattributes]
  125 |                             const EpdWaveform *waveform) {
      |                             ^~~~~
In file included from /app/src/epd_driver/render.c:3:
/app/src/epd_driver/include/epd_driver.h:458:29: note: previous declaration here
  458 | enum EpdDrawError IRAM_ATTR epd_draw_base(EpdRect area,
      |                             ^~~~~~~~~~~~~
/app/src/epd_driver/display_ops.c:15:1: warning: ignoring attribute 'section (".iram1.16")' because it conflicts with previous 'section (".iram1.0")' [-Wattributes]
   15 | void IRAM_ATTR busy_delay(uint32_t cycles) {
      | ^~~~
In file included from /app/src/epd_driver/display_ops.c:1:
/app/src/epd_driver/display_ops.h:35:16: note: previous declaration here
   35 | void IRAM_ATTR busy_delay(uint32_t cycles);
      |                ^~~~~~~~~~
/app/src/epd_driver/display_ops.c:107:1: warning: ignoring attribute 'section (".iram1.17")' because it conflicts with previous 'section (".iram1.2")' [-Wattributes]
  107 | void IRAM_ATTR epd_skip() {
      | ^~~~
In file included from /app/src/epd_driver/display_ops.c:1:
/app/src/epd_driver/display_ops.h:70:16: note: previous declaration here
   70 | void IRAM_ATTR epd_skip();
      |                ^~~~~~~~
/app/src/epd_driver/display_ops.c:117:1: warning: ignoring attribute 'section (".iram1.18")' because it conflicts with previous 'section (".iram1.1")' [-Wattributes]
  117 | void IRAM_ATTR epd_output_row(uint32_t output_time_dus) {
      | ^~~~
In file included from /app/src/epd_driver/display_ops.c:1:
/app/src/epd_driver/display_ops.h:67:16: note: previous declaration here
   67 | void IRAM_ATTR epd_output_row(uint32_t output_time_dus);
      |                ^~~~~~~~~~~~~~
/app/src/epd_driver/display_ops.c:168:1: warning: ignoring attribute 'section (".iram1.19")' because it conflicts with previous 'section (".iram1.4")' [-Wattributes]
  168 | void IRAM_ATTR epd_switch_buffer() { i2s_switch_buffer(); }
      | ^~~~
In file included from /app/src/epd_driver/display_ops.c:1:
/app/src/epd_driver/display_ops.h:82:16: note: previous declaration here
   82 | void IRAM_ATTR epd_switch_buffer();
      |                ^~~~~~~~~~~~~~~~~
/app/src/epd_driver/display_ops.c:169:1: warning: ignoring attribute 'section (".iram1.20")' because it conflicts with previous 'section (".iram1.3")' [-Wattributes]
  169 | uint8_t IRAM_ATTR *epd_get_current_buffer() {
      | ^~~~~~~
In file included from /app/src/epd_driver/display_ops.c:1:
/app/src/epd_driver/display_ops.h:75:20: note: previous declaration here
   75 | uint8_t IRAM_ATTR *epd_get_current_buffer();
      |                    ^~~~~~~~~~~~~~~~~~~~~~
/app/src/epd_driver/font.c: In function 'uncompress':
/app/src/epd_driver/font.c:103:68: warning: passing argument 3 of 'tinfl_decompress' from incompatible pointer type [-Wincompatible-pointer-types]
  103 |     tinfl_status decomp_status = tinfl_decompress(&decomp, source, &source_size, dest, dest, &uncompressed_size, TINFL_FLAG_PARSE_ZLIB_HEADER | TINFL_FLAG_USING_NON_WRAPPING_OUTPUT_BUF);
      |                                                                    ^~~~~~~~~~~~
      |                                                                    |
      |                                                                    uint32_t * {aka long unsigned int *}
In file included from /app/src/epd_driver/font.c:8:
/opt/esp/idf/components/esp_rom/include/esp32/rom/miniz.h:597:92: note: expected 'size_t *' {aka 'unsigned int *'} but argument is of type 'uint32_t *' {aka 'long unsigned int *'}
  597 | tinfl_status tinfl_decompress(tinfl_decompressor *r, const mz_uint8 *pIn_buf_next, size_t *pIn_buf_size, mz_uint8 *pOut_buf_start, mz_uint8 *pOut_buf_next, size_t *pOut_buf_size, const mz_uint32 decomp_flags);
      |                                                                                    ~~~~~~~~^~~~~~~~~~~~
/app/src/epd_driver/font.c:103:94: warning: passing argument 6 of 'tinfl_decompress' from incompatible pointer type [-Wincompatible-pointer-types]
  103 |     tinfl_status decomp_status = tinfl_decompress(&decomp, source, &source_size, dest, dest, &uncompressed_size, TINFL_FLAG_PARSE_ZLIB_HEADER | TINFL_FLAG_USING_NON_WRAPPING_OUTPUT_BUF);
      |                                                                                              ^~~~~~~~~~~~~~~~~~
      |                                                                                              |
      |                                                                                              uint32_t * {aka long unsigned int *}
In file included from /app/src/epd_driver/font.c:8:
/opt/esp/idf/components/esp_rom/include/esp32/rom/miniz.h:597:165: note: expected 'size_t *' {aka 'unsigned int *'} but argument is of type 'uint32_t *' {aka 'long unsigned int *'}
  597 | tinfl_status tinfl_decompress(tinfl_decompressor *r, const mz_uint8 *pIn_buf_next, size_t *pIn_buf_size, mz_uint8 *pOut_buf_start, mz_uint8 *pOut_buf_next, size_t *pOut_buf_size, const mz_uint32 decomp_flags);
      |                                                                                                                                                             ~~~~~~~~^~~~~~~~~~~~~
/app/src/epd_driver/lut.c:69:1: warning: ignoring attribute 'section (".iram1.15")' because it conflicts with previous 'section (".iram1.8")' [-Wattributes]
   69 | void IRAM_ATTR write_row(uint32_t output_time_dus) {
      | ^~~~
In file included from /app/src/epd_driver/lut.c:1:
/app/src/epd_driver/lut.h:53:16: note: previous declaration here
   53 | void IRAM_ATTR write_row(uint32_t output_time_dus);
      |                ^~~~~~~~~
/app/src/epd_driver/lut.c:75:1: warning: ignoring attribute 'section (".iram1.16")' because it conflicts with previous 'section (".iram1.9")' [-Wattributes]
   75 | void IRAM_ATTR skip_row(uint8_t pipeline_finish_time) {
      | ^~~~
In file included from /app/src/epd_driver/lut.c:1:
/app/src/epd_driver/lut.h:54:16: note: previous declaration here
   54 | void IRAM_ATTR skip_row(uint8_t pipeline_finish_time);
      |                ^~~~~~~~
/app/src/epd_driver/lut.c:86:1: warning: ignoring attribute 'section (".iram1.17")' because it conflicts with previous 'section (".iram1.5")' [-Wattributes]
   86 | void IRAM_ATTR reorder_line_buffer(uint32_t *line_data) {
      | ^~~~
In file included from /app/src/epd_driver/lut.c:1:
/app/src/epd_driver/lut.h:18:16: note: previous declaration here
   18 | void IRAM_ATTR reorder_line_buffer(uint32_t *line_data);
      |                ^~~~~~~~~~~~~~~~~~~
/app/src/epd_driver/lut.c:314:1: warning: ignoring attribute 'section (".iram1.28")' because it conflicts with previous 'section (".iram1.7")' [-Wattributes]
  314 | void IRAM_ATTR provide_out(OutputParams *params) {
      | ^~~~
In file included from /app/src/epd_driver/lut.c:1:
/app/src/epd_driver/lut.h:50:16: note: previous declaration here
   50 | void IRAM_ATTR provide_out(OutputParams *params);
      |                ^~~~~~~~~~~
/app/src/epd_driver/lut.c:488:1: warning: ignoring attribute 'section (".iram1.30")' because it conflicts with previous 'section (".iram1.10")' [-Wattributes]
  488 | void IRAM_ATTR busy_delay(uint32_t cycles);
      | ^~~~
In file included from /app/src/epd_driver/lut.c:2:
/app/src/epd_driver/display_ops.h:35:16: note: previous declaration here
   35 | void IRAM_ATTR busy_delay(uint32_t cycles);
      |                ^~~~~~~~~~
/app/src/epd_driver/lut.c:535:1: warning: ignoring attribute 'section (".iram1.31")' because it conflicts with previous 'section (".iram1.6")' [-Wattributes]
  535 | void IRAM_ATTR feed_display(OutputParams *params) {
      | ^~~~
In file included from /app/src/epd_driver/lut.c:1:
/app/src/epd_driver/lut.h:49:16: note: previous declaration here
   49 | void IRAM_ATTR feed_display(OutputParams *params);
      |                ^~~~~~~~~~~~
In file included from /app/src/epd_driver/i2s_data_bus.c:2:
/opt/esp/idf/components/driver/deprecated/driver/periph_ctrl.h:7:2: warning: #warning driver/periph_ctrl.h header is no longer used, and will be removed in future versions. [-Wcpp]
    7 | #warning driver/periph_ctrl.h header is no longer used, and will be removed in future versions.
      |  ^~~~~~~
/app/src/epd_driver/i2s_data_bus.c:85:1: warning: ignoring attribute 'section (".iram1.5")' because it conflicts with previous 'section (".iram1.0")' [-Wattributes]
   85 | volatile uint8_t IRAM_ATTR *i2s_get_current_buffer() {
      | ^~~~~~~~
In file included from /app/src/epd_driver/i2s_data_bus.c:1:
/app/src/epd_driver/i2s_data_bus.h:55:29: note: previous declaration here
   55 | volatile uint8_t IRAM_ATTR *i2s_get_current_buffer();
      |                             ^~~~~~~~~~~~~~~~~~~~~~
/app/src/epd_driver/i2s_data_bus.c: In function 'i2s_get_current_buffer':
/app/src/epd_driver/i2s_data_bus.c:86:53: warning: return discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
   86 |   return current_buffer ? i2s_state.dma_desc_a->buf : i2s_state.dma_desc_b->buf;
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
/app/src/epd_driver/i2s_data_bus.c: At top level:
/app/src/epd_driver/i2s_data_bus.c:89:1: warning: ignoring attribute 'section (".iram1.6")' because it conflicts with previous 'section (".iram1.3")' [-Wattributes]
   89 | bool IRAM_ATTR i2s_is_busy() {
      | ^~~~
In file included from /app/src/epd_driver/i2s_data_bus.c:1:
/app/src/epd_driver/i2s_data_bus.h:72:16: note: previous declaration here
   72 | bool IRAM_ATTR i2s_is_busy();
      |                ^~~~~~~~~~~
/app/src/epd_driver/i2s_data_bus.c:94:1: warning: ignoring attribute 'section (".iram1.7")' because it conflicts with previous 'section (".iram1.1")' [-Wattributes]
   94 | void IRAM_ATTR i2s_switch_buffer() {
      | ^~~~
In file included from /app/src/epd_driver/i2s_data_bus.c:1:
/app/src/epd_driver/i2s_data_bus.h:62:16: note: previous declaration here
   62 | void IRAM_ATTR i2s_switch_buffer();
      |                ^~~~~~~~~~~~~~~~~
/app/src/epd_driver/i2s_data_bus.c:102:1: warning: ignoring attribute 'section (".iram1.8")' because it conflicts with previous 'section (".iram1.2")' [-Wattributes]
  102 | void IRAM_ATTR i2s_start_line_output() {
      | ^~~~
In file included from /app/src/epd_driver/i2s_data_bus.c:1:
/app/src/epd_driver/i2s_data_bus.h:67:16: note: previous declaration here
   67 | void IRAM_ATTR i2s_start_line_output();
      |                ^~~~~~~~~~~~~~~~~~~~~
In file included from /app/src/epd_driver/rmt_pulse.c:2:
/opt/esp/idf/components/driver/deprecated/driver/rmt.h:18:2: warning: #warning "The legacy RMT driver is deprecated, please use driver/rmt_tx.h and/or driver/rmt_rx.h" [-Wcpp]
   18 | #warning "The legacy RMT driver is deprecated, please use driver/rmt_tx.h and/or driver/rmt_rx.h"
      |  ^~~~~~~
/app/src/epd_driver/rmt_pulse.c:60:32: warning: ignoring attribute 'section (".iram1.8")' because it conflicts with previous 'section (".iram1.2")' [-Wattributes]
   60 |                                uint16_t low_time_ticks, bool wait) {
      |                                ^~~~~~~~
In file included from /app/src/epd_driver/rmt_pulse.c:1:
/app/src/epd_driver/rmt_pulse.h:39:16: note: previous declaration here
   39 | void IRAM_ATTR pulse_ckv_ticks(uint16_t high_time_us, uint16_t low_time_us,
      |                ^~~~~~~~~~~~~~~
/app/src/epd_driver/rmt_pulse.c:86:29: warning: ignoring attribute 'section (".iram1.9")' because it conflicts with previous 'section (".iram1.0")' [-Wattributes]
   86 |                             bool wait) {
      |                             ^~~~
In file included from /app/src/epd_driver/rmt_pulse.c:1:
/app/src/epd_driver/rmt_pulse.h:24:16: note: previous declaration here
   24 | void IRAM_ATTR pulse_ckv_us(uint16_t high_time_us, uint16_t low_time_us,
      |                ^~~~~~~~~~~~
/app/src/epd_driver/rmt_pulse.c:90:1: warning: ignoring attribute 'section (".iram1.10")' because it conflicts with previous 'section (".iram1.1")' [-Wattributes]
   90 | bool IRAM_ATTR rmt_busy() { return !rmt_tx_done; }
      | ^~~~
In file included from /app/src/epd_driver/rmt_pulse.c:1:
/app/src/epd_driver/rmt_pulse.h:29:16: note: previous declaration here
   29 | bool IRAM_ATTR rmt_busy();
      |                ^~~~~~~~

[jdoubleu]

I've resolved a few warnings. I'm pretty sure, they're either not shown or the warning level is way lower in v4.3 and v4.4.

• Some types mismatched. Maybe due to some changes in the toolchain. E.g. size_t is not uint32_t.
• The headers used the IRAM_ATTR macro in their function declaration. Internally it uses a counter somehow to create the sections. The definitions also used IRAM_ATTR, causing the declaration and definition to mismatch. I've removed the IRAM_ATTR macro from the headers, because it is required only by the actual definition. I haven't verified yet, whether the linker puts it into the correct section, though.
• Some headers are deprecated, but can be easily replaced.

For the RMT driver, the current header used is backwards compatible, but produces a warning. However, since the driver has been refactored, we need to adopt the new model. See https://docs.espressif.com/projects/esp-idf/en/latest/esp32/migration-guides/peripherals.html#rmt-driver for more information.

[second-string]

Re-ran CI since espressif pushed a new release_v4.3 docker image 2 days ago and the 4.3 mpd_status build is now passing. There's somehow now a bunch of issues with v5 though 🤦🏼‍♂️. The latest docker image for v5 might be too unstable to use until they release 5.0 and pin an image. Or we could pin our own to prevent any breaking changes but that would require a separate docker hub account.

[martinberlin]

When I try to compile this branch I'm getting a:

Failed to resolve component 'esp_adc_cal' 
Additionally does not find driver/i2c

Although I see this has been added to the REQUIRES section as esp_adc_cal but it seem in latest edition is called esp_adc. Added the solution in the review comments reading the deprecations list. Maybe this is the reason that it fails in esp-idf latest? Can we push this forward and merge it whenever there is some time?

[second-string]

tl;dr espressif deprecated the esp_adc_cal component in 5.X and has a new interface for instantiating and reading values (this is what happened with the RMT periph, it was just easier to hack in a couple externs/changes to work with the deprecated driver). The only code that needs to be updated to work with the new adc interface is in board/board_common.c.

I just cleaned and compiled the demo example successfully on esp-idf repo commit 341162bb3d28d1fb9efa5e2c811d2460f89c0411 on May 22 2022.

I also pulled the very latest esp-idf code as of today, Aug 24 2022, which is commit ae256e9ca23a85e05f6a9ff31805028d3e823f4d, rebuilt the toolchain with ./esp-idf/install.sh, and get the same error when compiling. It looks like esp_adc_cal has been moved from the default components into the component manager for esp-idf 5.1 and on (the latest HEAD is now on 5.1). I couldn't actually get it to work correctly using idf.py add-component esp_adc_cal: it adds to a new idf_component.yml file, but still fails to find a version. It's not listed on https://components.espressif.com either, so it might be too new of a change in esp-idf to be fully supported yet (the same thing happened with libsodium in the mpd_status example originally).

Compiling also fails when I check out the v5.0-beta1 tag. The esp_adc_cal functionality got rolled into the esp_adc component and a new header, esp_adc/adc_cali.h. The epdiy board/esp_board_common.c file needs to be updated to use the new ADC interface: documentation can be found here.

I spend 15 minutes looking into it and it doesn't seem that difficult to port - all of the functionality still exists it's just some different handle typedefs and function calls. I just don't have time to do it right now. Maybe @mickeprag would be willing to take it on, I believe they were the one to add the new HAL code? I pushed an update to the CMakeLists.txt file to support the component changes that espressif did, but now we just have to utilize the esp_adc component instead of the old esp_adc_cal one.

Also one last thing - there's now a 5.X idf docker container tag: v5.0-beta1. I recommend we switch the CI container to build against this rather than latest to prevent issues that keep coming up with the most recent changing esp-idf code. Happy to make this change if we're okay with it. esp-idf container tags listed here.

[martinberlin]

Hello @dot4qu, Thanks a lot for the corrections.

Compiling also fails when I check out the v5.0-beta1 tag. The esp_adc_cal functionality got rolled into the esp_adc component and a new header, esp_adc/adc_cali.h.

This is very strange. Using latest v5.0-beta1 as is, note that I completely wiped out and install it with git clone fetching directly that branch, it builds, compiles and flashes correctly (But attention on the Lilygo EPD047, esp32 version) Maybe your results is because you where using another board. It was too quick for a change to work, and right now I don't really know if this is used by all boards or only for some of them. But I will also try V5 tomorrow and grab the V6 that Micke sent to me, to try compile this also against v6 when I find some time. I would really like to get this tested on all possible boards and have an stable component that compiles on 4.2, 4.4 and 5.

Question though: Can we add a REQUIRES item but only for one specific IDF version?

Failed to resolve component 'esp_adc'.

Otherwise this will fail when is being build for IDF <= 5. This is the reason why tests are failing right now. Where mostly the latest are running OK.

[second-string]

Interesting, my board is set as an ED060SC4. I wonder if the Lilygo board doesn't have a temp sensor so that those adc functions are compiled out?

I'm not sure how to differentiate between versions for the esp_adc component in the CMakeLists file. You can see an example of libsodium moving to the new component manager in the mpd_status example. It remains in the idf_component_register call in CMakeLists, but is added for specific esp-idf versions in main/idf_component.yml.

I don't think the esp_adc component is available in the new component system, so I think it has to be flagged on and off in the CMakeLists file itself. Something like

idf_component_register(SRCS ${app_sources}
                       INCLUDE_DIRS "include"
                       REQUIRES esp_timer
#if IDF_VERSION > 5.0
                       driver esp_adc
#else
                       esp_adc_cal
#endif
)

I'm just making up that macro and comparison I don't remember the specifics of how it's set. I also have no idea if that will even work or not, but it's the only way I can think to do it