-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
[WIP] esp32/Makefile: Move to v4.1 #6413
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
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.
Thanks, this is a great start! I did have a couple of questions, though...
@@ -260,7 +265,7 @@ CFLAGS_COMMON = -Os -ffunction-sections -fdata-sections -fstrict-volatile-bitfie | |||
-Wno-error=unused-variable -Wno-error=deprecated-declarations \ | |||
-DESP_PLATFORM | |||
|
|||
CFLAGS_BASE = -std=gnu99 $(CFLAGS_COMMON) -DMBEDTLS_CONFIG_FILE='"mbedtls/esp_config.h"' -DHAVE_CONFIG_H | |||
CFLAGS_BASE = -std=gnu11 $(CFLAGS_COMMON) -DMBEDTLS_CONFIG_FILE='"mbedtls/esp_config.h"' -DHAVE_CONFIG_H |
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.
Can you please explain the need for this change?
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 compile flag is only required for one file:
$(ECHO) "GEN $@" | ||
$(Q)$(MKDIR) -p $(dir $@) | ||
$(Q)$(PYTHON) $(ESPIDF)/tools/kconfig_new/confgen.py \ | ||
$(Q)$(PYTHON2) $(ESPIDF)/tools/kconfig_new/confgen.py \ |
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.
Why this change, seems something more related to your build env as opposed to esp-idf v4.0.1 vs. v4.1?
@@ -562,7 +577,7 @@ ESPIDF_BT_NIMBLE_O = $(patsubst %.c,%.o,\ | |||
endif | |||
|
|||
$(BUILD)/$(ESPCOMP)/esp_eth/src/esp_eth_mac_dm9051.o: CFLAGS += -fno-strict-aliasing | |||
ESPIDF_ESP_ETH_O = $(patsubst %.c,%.o,$(wildcard $(ESPCOMP)/esp_eth/src/*.c)) | |||
ESPIDF_ESP_ETH_O = $(patsubst %.c,%.o,$(filter-out %_openeth.c,$(wildcard $(ESPCOMP)/esp_eth/src/*.c))) |
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.
Why does this have to be excluded?
@@ -89,7 +91,7 @@ else ifeq ($(ESPIDF_CURHASH),$(ESPIDF_SUPHASH_V4)) | |||
$(info Building with ESP IDF v4) | |||
|
|||
PYPARSING_VERSION = $(shell python3 -c 'import pyparsing; print(pyparsing.__version__)') | |||
ifneq ($(PYPARSING_VERSION),2.3.1) | |||
ifeq ($(filter 2.3% 2.2% 2.1%,$(PYPARSING_VERSION)),) |
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.
Is it really a good idea to allow all these older versions? Did you test against each one? Do we really want to test against each one?
@osctobe thanks for the contribution here. Are you able to keep working on this, or shall someone else pick it up? |
This is more of a general question than related to the upgrade to v4.1: Is there a specific reason that micropython does not build as a component to esp-idf? See e.g. that the compile flags are changed for only one file https://github.com/espressif/esp-idf/blob/94cb7e8b8f6da8abf2ca5c6117c81eaac92d90ee/components/driver/component.mk#L11. I know that there have been discussions about moving the esp32 port to cmake, but that's not what I'm talking about. As long as esp-idf does support the "legacy make" build system this should work quite well. I have a working poc which builds micropython as a component to esp-idf with quite minimal change to the micropython code base. It uses the approch described here https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/build-system-legacy.html#fully-overriding-the-component-makefile. I'll be happy to share this poc if interest is available. |
Because it was done that way from the beginning, to have full control over the build, which is mainly to get the qstr generation done correctly. It also makes it more like other ports. But these are not strong reasons to do it that way.
I agree.
The legacy make support in the IDF may disappear in the future, so I'd say it's better to just go straight to cmake if anything. See #6473 for a PR to do that. |
See #6906 . |
ESP IDF v4.1.1 is now supported, as of d191d88 |
…n-main Translations update from Hosted Weblate
Fixup build system for ESP-IDF v4.1. This only makes SDK parts build, there are API changes that need adjusting to in the code.
Issue #6412 .