-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
WIP: esp32: Add support to build using IDF cmake, with MicroPython as a cmake component #6473
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
Conversation
@tve any thoughts/comments on this approach? |
Wowza @dpgeorge - DID NOT SEE THIS COMING! |
This should allow building for esp32s2, but I didn't try that yet (did get it running on a standard TinyPICO tho). |
So there is no more "must be on IDF XXX commit" anymore? I can just grab latest and follow those steps ? I just branched and added this PR - want to give it a whirl. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Really like this.
Tested to build micropython with the docker image espressif/idf:release-v4.1
despite the mentioned change this works.
ports/esp32/main/CMakeLists.txt
Outdated
|
||
# Include core source components. | ||
include(${MPY_DIR}/py/micropy_py.cmake) | ||
include(${MPY_DIR}/py/micropy_extmod.cmake) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be:
include(${MPY_DIR}/extmod/micropy_extmod.cmake)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! now fixed.
I just pushed an approach to specify the board with something like that: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/build-system.html#custom-sdkconfig-defaults |
Ok, interesting. I was thinking to parse the existing mpconfigboard.mk, so there was only one "source of truth", but it is probably better to have a dedicated mpconfigboard.cmake file per board, with any required settings (like SDKCONFIG_DEAFULT). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems an unavoidable change in the long run. I'm not a fan of the esp-idf config/make system, but it's what everyone using the esp32 kind'a expects. The less MP reinvents the better in general...
Comments from doing a test run with this PR branch:
The fact that one gets 59% through the compilation and then it complains about mpy-cross isn't very user-friendly:
[ 59%] Generating ../../genhdr/qstrdefs.generated.h
[ 59%] Generating ../../frozen_content.c
mpy-cross not found at /home/src/esp32/mpy-esp-idf/micropython/mpy-cross/mpy-cross, please build it
first
make[2]: *** [esp-idf/main/CMakeFiles/__idf_main.dir/build.make:89: frozen_content.c] Error 1
The compilation seems to spit out a lot more warnings than when using the old makefile. I guess that's good, but only really if they're going to be addressed?
After a make
running ESPBAUD=921600 make flash
is very slow because it re-generates qstr headers, and it's awfully verbose (I just wanted to flash what's already built and not see the full list of what didn't get rebuilt...).
With the old makefile, if I run touch main.c; make
it takes just shy of 12 seconds on my machine. With the new stuff touch main.c; make
takes 26 seconds. Not great.
I suppose to really make MP a component of esp-idf the config stuff needs to be addressed too, right?
@@ -68,28 +70,28 @@ STATIC void machine_uart_print(const mp_print_t *print, mp_obj_t self_in, mp_pri | |||
if (self->invert) { | |||
mp_printf(print, ", invert="); | |||
uint32_t invert_mask = self->invert; | |||
if (invert_mask & UART_INVERSE_TXD) { | |||
if (invert_mask & UART_SIGNAL_TXD_INV) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with esp-idf pre-4.1? Or is the idea to deprecate the use of older esp-idf versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get that far, I just wanted to get something working and get feedback (will add WIP to the PR title).
We can continue to support older IDF versions if it's not difficult, but I'd rather just support the latest (easier to test, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NP, I just wanted to flag it.
@@ -0,0 +1,114 @@ | |||
# Set location of base MicroPython directory, and this port's directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this file have to be in a subdirectory? (I know nothing about cmake)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Or at least I couldn't work out how to put it in ports/esp32/
directly...
@@ -491,7 +490,7 @@ STATIC mp_obj_t esp_ifconfig(size_t n_args, const mp_obj_t *args) { | |||
tcpip_adapter_ip_info_t info; | |||
tcpip_adapter_dns_info_t dns_info; | |||
tcpip_adapter_get_ip_info(self->if_id, &info); | |||
tcpip_adapter_get_dns_info(self->if_id, TCPIP_ADAPTER_DNS_MAIN, &dns_info); | |||
tcpip_adapter_get_dns_info(self->if_id, ESP_NETIF_DNS_MAIN, &dns_info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compat with older esp-idf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, but easy to make it compat.
I feel the same, I'd rather pure make but I think we need to move to cmake. I tried updating the IDF to v4.1 with the existing Makefile approach but could not get it working after about 2 hours (eventually ended up with a link error for libc/libm), and then thought time would be better spent looking into cmake, hence this PR. For esp32s2 support (and other esp32 series chips) I think the cmake approach is necessary (just too complex otherwise, time gets wasted on duplicating effort already done by the IDF).
Can be fixed by a Makefile wrapper that checks for mpy-cross (or a cmake rule...).
Not sure why they are popping up but, yes, they'd need to be dealt with (silenced or fixed).
Yes... the verbosity is part of cmake I guess and cannot be changed. The qstr regeration is a pain and I could not work out how to fix it, it might not be possible to fix because cmake has a more restricted concept of dependencies than make.
That's mostly down to qstr regeneration, again.
The build system doesn't need to use menuconfig, it can continue to have sdkconfig fragments, it just needs to be set up that way. In the end it's a choice between:
We could support both make and cmake.... and maybe that's a solution for now, until cmake has proven itself (and users wanting to use cmake/esp32s2 can). From my point of view, I have a preference to switch to cmkae completely and remove make support (although have a simple Makefile to wrap cmake so existing |
It would be really good to fix the qstr issue.. Ugh. I agree with your overall comments. I would switch completely and only bring back the Makefile if there are real issues uncovered. Just because of less maintenance... |
Agreed... I already spent way too long trying to fix this, but my cmake skills are low so I could be missing something simple.
Yes, I'd rather spend time working on real issues (eg uevent, ssl, features, etc) than fixing and maintaining the build. So, I think that's a plan then, to move fully to cmake... (if anyone else has an opinion, please raise it). |
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
This is updated to: 1) fix qstr generation so it's faster, now very similar to how it works with normal make; 2) remove build warnings. |
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Do you mean just for the esp32 port or for all ports? Zephyr already uses cmake and I've been working on a refactoring in #6392 |
Aah, sorry, I meant just for the esp32 port. But of course other ports like Zephyr that have an SDK using cmake could also switch to use cmake for everything, if that makes sense. |
Is it possible to use CMake under Windows with Espressif Standard Setup of Toolchain for Windows? |
Tested successfully on esp-idf 4.2. Also, I added support for ubluetooth module. |
I pulled in pumelo's https://github.com/pumelo/micropython/tree/esp32-cmake and change MPY_ to MICROPY_ for the prefixes to match the rest. Works pretty well for me so far... I also pulled in and hacked on dpgeorge#14 to get BLE to work. For me this ends up overflowing iram0_0_seg by 3808 bytes, which is a significant amount. This using the esp-idf v4.1 tag. I'm not sure what to do about this. I increased the iram segment size by 4KB, but that's not exactly satisfying. But it allowed me to do a make size-components... I tried the release/v4.2 branch and that results in a pile of errors saying that Is there any hope to get this stuff finalized anytime soon? Edit:
|
FYI, I took a stab at CI: tve@c42bff4 |
I was able to build this PR using IDF v4.0.2 and basic smoke tests pass, even connecting to the WiFi. |
Wi-Fi autoconnect and title bar status
This PR adds a set of cmake files to allow building the esp32 port as a cmake component in the ESP-IDF. The existing Makefile is left untouched and can still be used.
The PR also updates the code to work with IDF v4.1, and the latest esp32 toolchain.
To build and flash to a device:
TODO: