Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get ready for ESP-IDF v5 #180

Merged
merged 28 commits into from
Jan 17, 2023
Merged

get ready for ESP-IDF v5 #180

merged 28 commits into from
Jan 17, 2023

Conversation

jdoubleu
Copy link
Contributor

@jdoubleu jdoubleu commented May 8, 2022

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: espressif/esp-idf@9f55712

@martinberlin martinberlin self-assigned this May 9, 2022
@martinberlin martinberlin added the CI testing Continuous integration label May 9, 2022
@martinberlin martinberlin mentioned this pull request May 9, 2022
2 tasks
jdoubleu added a commit to jdoubleu/epdiy that referenced this pull request May 10, 2022
second-string added a commit to second-string/spot-check-firmware that referenced this pull request May 22, 2022
@second-string
Copy link
Contributor

second-string commented May 22, 2022

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: second-string/spot-check-firmware@65de40c

second-string added a commit to second-string/spot-check-firmware that referenced this pull request May 22, 2022
@second-string
Copy link
Contributor

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.

mickeprag and others added 4 commits May 30, 2022 21:30
* Add board HAL (Hardware abstraction layer)

Instead of using defines to set the board hardware definitions each
board get its own file with the routines to access the hardware
functions. This way also third party boards can be implemented
without being required to modify the epdiy source.

So far only logic from display_ops.c has been ported. Work still
also exists to optimize duplicated code.

Care has been taken to not break existing code bases. If a board is
defined in menuconfig this will still be used.

* Move content of config_reg_v4.h into epd_board_v5.c

The goal is to get rid of the config_reg_v*.h files

* Move content of config_reg_v4.h into epd_board_v4.c

* Move content of config_reg_v6.h into epd_board_v6.c

* Move content of config_reg_v2.h into epd_board_v2_v3.c

* Move content of config_reg_v2.h into epd_board_lilygo_t5_47.c

* Remove board specific defines from display_ops.h

* Free the i2s clock pin in i2s code

This should not be done for individual boards.

* Deprecate v6 board specific functions as generic function.

Boards specific code should be marked as such.

* Move temperature readings into board code

* Implement a control interface for gpios

Move signals oe, mode and stv to this
Implement start_frame using the ctrl interface

* Move logic of end_frame() from board to display_opts

* Remove uneccesary function latch_row()

* Move logic for latch_row from board to display_opts

* Remove warning about v6_wait_for_interrupt() is unused

* Optimize set_ctrl by supplying a mask with the changed signals

* Share the temperature readings between boards v2 to v5

* Deprecate epd_powerdown() and make it to a board specific function instead

* Read vcom voltage from epd_board_vcom_v6()

This makes vcom voltage available at runtime and not at compile time
Revert "disable CI runs on latest ESP-IDF for now"

This reverts commit 0e2bd15.
The drop-in replacement is portTICK_PERIOD_MS.

See espressif/esp-idf#51.
Some symbols and macros were not longer implicitly imported.
@jdoubleu
Copy link
Contributor Author

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

I'm not sure whether #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
Copy link
Collaborator

martinberlin commented May 31, 2022

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 martinberlin force-pushed the master branch 2 times, most recently from 3ffb83c to fd8a290 Compare June 2, 2022 03:19
@martinberlin
Copy link
Collaborator

#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 added a commit to jdoubleu/epdiy that referenced this pull request Jun 9, 2022
@jdoubleu
Copy link
Contributor Author

jdoubleu commented Jun 9, 2022

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
Copy link
Collaborator

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

jdoubleu added a commit to jdoubleu/epdiy that referenced this pull request Jun 11, 2022
@martinberlin
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

@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);
}
  1. 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

@vroland
Copy link
Owner

vroland commented Sep 2, 2022

Thanks a lot for the work you have done here, @dot4qu and @martinberlin !
Just from reading though, some thoughts:

  • I think for the last releases, espressif sometimes had many betas, some with breaking changes. So I would prefer merging it as soon as IDF 5.0 is released, but not sooner.
  • Keeping compatibility with IDF 4.X will make some things ugly, but as long as arduino-esp32 stays on 4.X we should keep compatibility, IMO.
  • Regarding the BLE issues: During a display update, epdiy takes a lot of resource because it has to feed the I2S while doing waveform calculation, etc. If no update is running, the epdiy tasks should pretty much just idle, though. If they are taking resources permanently, maybe something has changed about task management in IDF 5.0?

@martinberlin
Copy link
Collaborator

If no update is running, the epdiy tasks should pretty much just idle, though. If they are taking resources permanently, maybe something has changed about task management in IDF 5.0?

I'm not sure about this point but I managed yesterday after much effort to install IDF 4.4 and 5.0 at the same time after some hacking.
The fact is that I was getting the content-length and drawing a small progress bar each 10 BLE packets to make the wait a bit more visual. So in fact I was doing some I2S while BLE was at full data transmission. And that generated the WDT alerts. So I will check compiling the same Firmware with 4.4 and check if it happens also with this version. Glad to see you back around here

@second-string
Copy link
Contributor

I'm not sure about this point but I managed yesterday after much effort to install IDF 4.4 and 5.0 at the same time after some hacking.

Not exactly the point of this thread but I didn't have too much trouble with just cloning the esp-idf repo and checking out the different tags. Then just open a new shell window and run the ./install.sh script. It might take a little time the first time you do it for a specific version since it has to download the different toolchain versions, but after that it's not super difficult to switch. Just thought I'd mention it.

@second-string
Copy link
Contributor

Er, I just tried to run one of these examples on two of my boards/screens and I'm not seeing any screen updates at all. I'm going to dig more into this tomorrow but as far as I know I haven't changed anything, so if someone else could test on their screen really quick to make sure I'm not going crazy i would appreciate it!

@martinberlin
Copy link
Collaborator

Mh, is still working for me. Although didn't try to pull latest updates in IDF v. 5 branch in a while. Just in case share the last commit of your IDF to see if we are in the same page:

commit b66be87f886fdc1fcc97a1013a84fc59be1e5c7b (HEAD, tag: v5.0-beta1)
Merge: fde4afc67a e073fc4254
Author: Darian darian@espressif.com
Date: Fri Jul 29 21:23:43 2022 +0800

Did you make sure your IDF menuconfig points to the right board configuration? (Double check this!)
Because this happen to me also and it was mostly that I was building this with a different board and of course did not work at all, although compiles and flashes perfectly.

@second-string
Copy link
Contributor

I forgot I switched some pin mappings for my custom board and had removed those changes on this branch to push to the PR 🤦🏼‍♂️🤦🏼‍♂️🤦🏼‍♂️.

Just changed a couple defines in the correct boards/ file and now I'm all set. I did notice this new addition to the Kconfig for custom boards though

config EPD_BOARD_CUSTOM
.

It doesn't look like it's used anywhere, anyone know if there's a plan to allow a custom board file based on this? It would be nice to not change the source files in the future

@martinberlin
Copy link
Collaborator

I think that allows to define a custom board on initialization. Never used it so far but I know is possible after the boards refactoring done by @mickeprag
Maybe what it will be nice is to add an example and define a custom board for an existing one to show how is done.

@second-string
Copy link
Contributor

esp-idf v5.0 has been release. I just pushed a tiny change to switch to the release container now, but unless espressif changed things significantly from the 5.0 beta release it should be okay. Besides the merge conflict in font.c is there any other testing that needs to be done here before merging (finally)?

@martinberlin
Copy link
Collaborator

martinberlin commented Oct 27, 2022

Hi @dot4qu
Where is this announced? I still see along documentation it refers as stable to 4.x
And also in their official repository releases I don't see a signal about this. But maybe I'm wrong, please enlighten me!

@second-string
Copy link
Contributor

Interesting, it looks like they have a release/v5.0 tag for the repo, a docker container, and release/v5.0 documentation, but it's not listed officially as released anywhere. Sorry, I saw the repo tag when I was doing some other stuff so I just assumed it was fully released!

@vroland
Copy link
Owner

vroland commented Nov 7, 2022

I look forward to having this merged, too, since my ESP32-S3 branch is based on this branch ;) I would think it should be a matter of days now to the release, maybe some last-minute fixes?

@martinberlin
Copy link
Collaborator

I’m also a bit impatient about getting this merged. Don’t really know how long it will take but I guess they need to also pack another OS versions like windows, still didn’t read any announcements on when it will be finished. But seems like a big move…

@martinberlin
Copy link
Collaborator

martinberlin commented Dec 2, 2022

Wanted to comment that IDF version 5.0 was released today.

@vroland if you find some time it would be great to give this a last review and merge it!

@second-string
Copy link
Contributor

I merged in master to get rid of the the font.c merge conflict, but now there are two tests failing again. I'm not sure how that's possible since we should have been locked on the 5.0 release container (unless they made changes to that), but I don't have time to look at the failures right now.

I'll look into it sometime later this week or over the holidays.

@jdoubleu
Copy link
Contributor Author

jdoubleu commented Dec 28, 2022

It appears, that uint32_t has been changed from unsigned int (in IDF v4.4.2) to long unsigned int (in v5). That breaks a bunch of log statements which use printf under the hood. In v5, it seems, they've turned on errors for some warnings.

Unfortunately, I had to use the PRI macros from <inttypes.h> to still be compatible with the old alias in v4.4.x.

@mickeprag
Copy link
Contributor

I don't think uint32_t has changed. It should be 4 bytes regardless of idf version.
But maybe the printf formatters has changed?

Using the PRI macros makes it really ugly. A common practice is to cast it to unsigned for log messages:

uint32_t val = 42;
printf("Value: %u", (unsigned)val);

@jdoubleu
Copy link
Contributor Author

jdoubleu commented Dec 29, 2022

According to the migration guide, uint32_t has indeed been changed: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/migration-guides/release-5.x/5.0/gcc.html#int32-t-and-uint32-t-for-xtensa-compiler. Note that even unsigned long is still 4 bytes on Xtensa (something like ILP32).

I agree, the PRI macros are not that readable. I fear that simply casting to unsigned (int) might not work either, because uint32_t is an alias of unsigned long now, which requires %lu format specifier, not %u. While in v4.x its still unsigned int, requiring %u.

@martinberlin
Copy link
Collaborator

martinberlin commented Jan 14, 2023

Hi @dot4qu @jdoubleu @mickeprag and company :)
What is missing so we can merge this one?
Can I help somewhere? I had my head in a couple of different matters and I couldn't find the inner peace to focus on this again.

About the logging functions and integers, yes is a pain, no idea why this happened in IDF ver. 5. Every-time you print a integer, no matter if it's uint8_t or uint16_t you need to cast it as (int) in the logging functions.

But other than that was is missing so we can merge this?
Or if it's all in order and all checks pass let's merge it @vroland . I would prefer that Valentin does it since he has for sure at this point the code more fresh in his head than me :)

@second-string
Copy link
Contributor

Hi @dot4qu @jdoubleu @mickeprag and company :) What is missing so we can merge this one?

Nothing from my side! I've been using this branch in my personal code and it's been working without any errors for me

@vroland
Copy link
Owner

vroland commented Jan 17, 2023

Actually I used it as the base branch before as well, I just kind of forgot, sorry.

@vroland vroland merged commit 12c67ce into vroland:master Jan 17, 2023
@jdoubleu
Copy link
Contributor Author

I still get a few compiler warnings, which I'm trying to fix in another PR.

[830/898] Building C object esp-idf/epd_driver/CMakeFiles/__idf_epd_driver.dir/font.c.objIn file included from /project/thirdparty/epdiy/src/epd_driver/font.c:8:
/opt/esp/idf/components/esp_rom/include/esp32/rom/miniz.h:7:2: warning: #warning "{target}/rom/miniz.h is deprecated, please use (#include "miniz.h") instead" [-Wcpp]
    7 | #warning "{target}/rom/miniz.h is deprecated, please use (#include "miniz.h") instead"
      |  ^~~~~~~
[835/898] Building C object esp-idf/epd_driver/CMakeFiles/__idf_epd_driver.dir/rmt_pulse.c.objIn file included from /project/thirdparty/epdiy/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"
      |  ^~~~~~~
[837/898] Building C object esp-idf/epd_driver/CMakeFiles/__idf_epd_driver.dir/board/epd_board_common.c.objIn file included from /project/thirdparty/epdiy/src/epd_driver/board/epd_board_common.c:2:
/opt/esp/idf/components/driver/deprecated/driver/adc.h:19:2: warning: #warning "legacy adc driver is deprecated, please migrate to use esp_adc/adc_oneshot.h and esp_adc/adc_continuous.h for oneshot mode and continuous mode drivers respectively" [-Wcpp]
   19 | #warning "legacy adc driver is deprecated, please migrate to use esp_adc/adc_oneshot.h and esp_adc/adc_continuous.h for oneshot mode and continuous mode drivers respectively"
      |  ^~~~~~~
In file included from /project/thirdparty/epdiy/src/epd_driver/board/epd_board_common.c:3:
/opt/esp/idf/components/esp_adc/deprecated/include/esp_adc_cal.h:17:2: warning: #warning "legacy adc calibration driver is deprecated, please migrate to use esp_adc/adc_cali.h and esp_adc/adc_cali_scheme.h" [-Wcpp]
   17 | #warning "legacy adc calibration driver is deprecated, please migrate to use esp_adc/adc_cali.h and esp_adc/adc_cali_scheme.h"
      |  ^~~~~~~

@second-string
Copy link
Contributor

I wasn't aware of the miniz ROM one, but the deprecated drivers was known. There was no way to port the existing code without significant code and logical changes since the new drivers a structured differently and have different APIs. Now that this is merged they would be good things to tackle individually though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI testing Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Epdiy not compatible with ESP-IDF v5
6 participants