Skip to content

esp32/Makefile: Include all driver/*.c in build. Fixes #4869 #4874

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

esp32/Makefile: Include all driver/*.c in build. Fixes #4869 #4874

wants to merge 1 commit into from

Conversation

nevercast
Copy link
Contributor

@nevercast nevercast commented Jun 25, 2019 •
edited
Loading

Small change to import all drivers in the ESP32 port. This allows writing modules, internally or externally with the cmodules functionality that can use these ESP32 drivers.

For me personally, this allows me to write a cmodule that uses the I2S peripheral without modifying the Makefile.

A snippet of the make process showing the files included:

CC /builder/esp-idf/components/driver/spi_master.c
CC /builder/esp-idf/components/driver/sigmadelta.c
CC /builder/esp-idf/components/driver/uart.c
CC /builder/esp-idf/components/driver/sdspi_transaction.c
CC /builder/esp-idf/components/driver/sdio_slave.c
CC /builder/esp-idf/components/driver/i2c.c
CC /builder/esp-idf/components/driver/periph_ctrl.c
CC /builder/esp-idf/components/driver/ledc.c
CC /builder/esp-idf/components/driver/sdmmc_transaction.c
CC /builder/esp-idf/components/driver/sdspi_host.c
CC /builder/esp-idf/components/driver/pcnt.c
CC /builder/esp-idf/components/driver/sdspi_crc.c
CC /builder/esp-idf/components/driver/timer.c
CC /builder/esp-idf/components/driver/rmt.c
CC /builder/esp-idf/components/driver/mcpwm.c
CC /builder/esp-idf/components/driver/spi_common.c
CC /builder/esp-idf/components/driver/can.c
CC /builder/esp-idf/components/driver/spi_slave.c
CC /builder/esp-idf/components/driver/gpio.c
CC /builder/esp-idf/components/driver/sdmmc_host.c
CC /builder/esp-idf/components/driver/i2s.c
CC /builder/esp-idf/components/driver/rtc_module.c
AR build/esp-idf/driver/libdriver.a

Here is a screenshot of me initializing the I2S hardware via MicroPython, possible due to this change. (Python & CModule library not provided)
Screenshot from 2019-06-25 13-37-25

Discussion and issue initially raised here: #4869

@nevercast nevercast changed the title ports/esp32: Include all driver/*.c in build. Fixes #4869 esp32/Makefile: Include all driver/*.c in build. Fixes #4869 Jun 25, 2019
@nevercast
Copy link
Contributor Author

It could be an improvement that more directories automatically include their .c/.o files. Not just ESPIDF_DRIVER, but I felt that was out of scope for this change. I want to keep this PR small since its blocking me downstream from being synced to #master.

We can discuss making this change more generic in a follow up issue, if that is of interest.

dpgeorge
dpgeorge reviewed Jun 25, 2019
View reviewed changes
ports/esp32/Makefile Outdated
rtc_module.o \
)
ESPIDF_DRIVER_C = $(wildcard $(ESPCOMP)/driver/*.c)
ESPIDF_DRIVER_O = $(ESPIDF_DRIVER_C:.c=.o)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about combining this into one declaration, to avoid the need to define ESPIDF_DRIVER_C?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I don't know how to get make to do that in one line. It seems to evaluate the replace before the wildcard, and just breaks.

If you know how, shoot. Otherwise, I'll look in to it further.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try using patsubst, see py/py.mk for examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its one line now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

patsubst needs the whole path to be satisfied which elongates the line by a substantial amount, even with the $(ESPCOMP) variable. subst, though not as safe, seems perfectly sufficient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it looks ok with subst

@nevercast nevercast force-pushed the feature/ports-esp32-all-drivers branch 2 times, most recently from e2610c5 to 49f2b09 Compare June 25, 2019 03:32
@nevercast nevercast force-pushed the feature/ports-esp32-all-drivers branch from 49f2b09 to 3a6fc47 Compare June 25, 2019 03:36
@nevercast
Copy link
Contributor Author

Whoops, misunderstanding regarding real name and real email. This PR should be all good.

@nevercast
Copy link
Contributor Author

A last minute thought, do you want me to rebase this on top of your travis changes so that we get an ESP32 build confirmation?

@dpgeorge
Copy link
Member

do you want me to rebase this on top of your travis changes so that we get an ESP32 build confirmation?

No it's ok, I'll test and rebase during merging.

@nevercast
Copy link
Contributor Author

No it's ok, I'll test and rebase during merging.

Great, thanks.

@dpgeorge
Copy link
Member

Thanks for the work, merged in 2f262d5

@dpgeorge dpgeorge closed this Jun 25, 2019
@nevercast nevercast deleted the feature/ports-esp32-all-drivers branch June 25, 2019 05:28
@nevercast
Copy link
Contributor Author

Thanks for reviewing and merging, I'm happy to say we are now tracking micropython#master again 👍

@dpgeorge dpgeorge mentioned this pull request Jul 11, 2019
Closed
tannewt added a commit to tannewt/circuitpython that referenced this pull request Jul 21, 2021
@tannewt
Fix Arduino RP2040 flash size
109bd28 
For RP2040 boards, we now change the default flash size based on
the configured flash. We will still try to read the size from the
flash first.

Fixes micropython#4874
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgeorge dpgeorge dpgeorge left review comments

Assignees
No one assigned
Labels
port-esp32
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nevercast @dpgeorge