-
Notifications
You must be signed in to change notification settings - Fork 443
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
ifdef'ing out contents of driver source/header files when not used. #225
Conversation
This change wraps driver header/source files in the appropriate constants to effectively make them empty if that driver is not used in this build. The ESP-IDF build system is smart enough to conditionally build source files depending on project settings (see CMakeLists.txt in drivers folders). PlatformIO is not, and always builds every file. This change effectively makes those files no-ops which serves the same purpose and makes this project able to compile with PlatformIO (and other build systems). This addresses, but does not entirely fix, issue lvgl#220.
So something I don’t understand here, I use PlatformIO and esp-idf and I can build the demo project just fine with either. PlatformIO also now uses esp-idf for the esp32 so why does it behave differently than using esp-idf directly. I am not sure I am seeing that as they appear mostly identical to me (different output structure is about all that is different). Maybe it does compile all the source files but the linker seems to exclude the drivers not used. My concern is that by ifdef out all the drivers when you make a change that affects them all you are more likely to not notice a breaking change unless you configure and compile each one separately and that is going to be very painful. |
Hi David: When you say that you can "build the demo project" are you referring to lv_platformio (as described in the README), or are you referring to main/main.c in this project? |
@cmumford I am referring to building main/main.c in this project which builds all the drivers as well as several demo projects. I typically just load the project up in PIO and build it (after setting up my platformio.ini file). This is the method I used to work on the DMA SPI driver as well as the other display-specific drivers I was working on. I occasionally cross-check by building it with ESP-IDF too from the command line. I've never had an issue switching back and forth despite the output files being in different folders. In fact I regularly use the ESP-IDF command line to do the menuconfig stuff even while using PIO to build this project. It has been my experience that the interoperate well. As I mentioned, my concern is that if we if-def out all the drivers then go making changes to the core SPI driver, etc... it might set us up to not notice a broken build unless we go through a build cycle of configuring and building for each display and that is going to be painful when working on core common code. |
@davidjade I agree with your concern, and only went this route because I could see no alternative. I will build as you suggest above and see if I get the same results. I think the main difference between your approach and mine is that when you are building your project is lv_port_esp32, and when I'm building I'm just adding lv_port_esp32 as a dependency. I'm new to PIO, so I'm sure you're more experienced than I am. I would think that adding lv_port_esp32 as a dependency would be the preferred approach because it would obviate the need to add lv_conf.h, or any other file from this project, to the application project, and make it easier to upgrade/downgrade lv_port_esp32 versions. |
@cmumford Using as a component I'm not sure what the answer is. I'm not fully up to date on all aspects of this project but I thought there was an effort to separate the ESP32 drivers out so they could be used with the main LVGL codebase as a component. That effort I think is in another repo and I'm not sure of its status. @C47D likely knows that status of that effort and the direction it is going. That may be the ultimate answer here. I suspect the right thing to do is to structure things such that they follow the ESP-IDF separate component way of doing things as these should then work in PIO builds. Right now this project uses components but they are not separable. If things were separate components, in my mind there doesn't need to be anything done for PIO specifically since it uses ESP-IDF under the covers. Same goes for LVGL but right now, since this ESP32 code includes a copy of LVGL, etc... it is not really set up to be included as a separate component as you are finding. Sorry if I am complicating things but it seems to me that there is a structural issue that needs to be addressed to include this project as a component in both ESP-IDF and PIO projects and that effort may already be happening in another project/plan which likely impacts or maybe even negates these proposed changes |
@davidjade would you mind describing how you are building this project with PIO? My build is failing whether I create my own platformio.ini file, or copy the one from lv_platformio. |
@cmumford I don't think I did anything special. I use PIO with VSCode on Windows 10 and just added a platformio.ini file in the root folder. Then I open the folder in VSCode and used PIO to build. I will note that I am a couple of commits behind in my local project so maybe recent changes have broken something? Here's the commit that I just built 74ab198 (Oct 3rd, the last one I contributed DMA work to). Here's my platformio.ini file. You'll see that I use a board profile in each ENV section (board = esp-wrover-kit-v4) that is specific to my boards for debugging. ESP-IDF includes this profile but last I check PIO does not so I copied it into my PIO install manually. I think you will have to pick the appropriate profile for the ESP32 board you use.
|
Thx for your help @davidjade - I was able to build with both esp-idf and PlatformIO. I'm going to close this PR. I do still think that this project should modified to allow this project to be included into another PlatformIO project using lib_deps, but that's a larger discussion that needs to be had with the project owners. Thx again. |
This change wraps driver header/source files in the appropriate
constants to effectively make them empty if that driver is not
used in this build.
The ESP-IDF build system is smart enough to conditionally build
source files depending on project settings (see CMakeLists.txt
in drivers folders). PlatformIO is not, and always builds every
file. This change effectively makes those files no-ops
which serves the same purpose and makes this project able to
compile with PlatformIO (and other build systems).
This addresses, but does not entirely fix, issue #220.