Add new Espressif target esp32s3 for tinyUSB #783

alisitsyn · 2021-04-12T16:02:11Z

What happened in this PR

This PR adds support of a new Espressif chip ESP32-S3 into tinyusb project.
The chip peripheral is very similar compare to ESP32-S2 target and support is added as a family driver to allow following extension of the family and group the controllers with similar USB peripheral.

Update the device controller driver for S family esp32sx:
tinyusb/src/portable/espressif/esp32sx/dcd_esp32sx.c
Add two family folders hw/bsp/esp32s2 and hw/bsp/esp32s3/ (better from support standpoint) and new esp32s3 based board in hw/bsp/esp32s3/boards/espressif_addax_1/*.
The board specifics are defined in board cmake file hw/bsp/esp32s3/boards/espressif_addax_1/board.cmake (example for esp32s3).
The cmake wrapper hw/bsp/esp32s3/family.mk is updated to specify target chip and add new make targets to flash the board using dfu-flash utility.
The following examples were fixed hid_composite_freertos, cdc_msc_freertos, board_test to allow selection of different target boards.
The compilation is performed using the same make BOARD=espressif_addax_1 command.
The compiled example can be flashed into the target board using the commands below:
make BOARD=espressif_addax_1 dfu - generates the dfu image for software
make BOARD=espressif_addax_1 dfu-flash -flashes the dfu image into target board

dfu-flash documentation for ESP32-S2

The soc/hal layers in ESP-IDF are already updated in esp-idf tinyusb support on esp32s3 to support the new chip.

me-no-dev · 2021-04-13T07:46:37Z

Hi @alisitsyn :) There seem to be changes included that need to be reverted, like submodules being removed. Also to make the changes more clear, is it possible to not include the new example?

alisitsyn · 2021-04-13T16:15:46Z

Hi @me-no-dev,

There seem to be changes included that need to be reverted, like submodules being removed.

The branch update is in progress (rebase to the latest master) and will be ready after testing soon.

Also to make the changes more clear, is it possible to not include the new example?

My intention is not addition of a new example but update the existing espressif examples to be able to compile them for new added board based on new esp32s3 chip - esp32s3_addax_1.

hathach

Thank you very much for your PR. It is great to have the new ESP32-S3 chip added. The changes look good, though I have a few feedback

Please revert the submodules removal. They are important and needed to build for other platforms :)
Can you move the esp32s3 into it own family hw/bsp/esp32s3 similar to esps2, it will help to group similar driver such as board_init() and ws2812 driver.

CI also needs to be update to build with esp32s3, though I could help with that.

Since PR status is WIP, I converted it to Draft. Feel free to push new update, I will try to follow up with feedback. Whenever you think it is ready for a merge, just Mark is as ready to covert it back.

hathach · 2021-04-13T17:17:38Z

examples/rules.mk


-all:
-	idf.py -B$(BUILD) -DBOARD=$(BOARD) build
+all flash monitor dfu-flash dfu :


nice, I didn't know we could do this

hathach · 2021-04-13T17:22:53Z

examples/rules.mk

+ifeq ($(findstring esp32s, $(BOARD)), esp32s)
+$(info Vendor: $(VENDOR), family: $(CHIP_FAMILY), target: $(CHIP_TARGET))


not all board has esp32s2 or esp32s3 in their name, we should just use the CHIP_FAMILY. It is good enough.

The commit I was based on contains the names of the espressif boards started with target prefix like esp32s2_saola_1. This looked useful for me. This was updated accordingly as it is in latest master.

name like Adafruit funhouse or tinyS2 etc. won't help esp32s2 or s3. It will cause issue with other boards in the future. The $(VENDOR) variable will probably be removed as well. I don't see the use of it in the make. Board name is already uniquely defined.

Ah, I see now. The unique board name is enough to determine all required information as $(CHIP_TARGET), $(FAMILY) and so on.

hathach · 2021-04-13T17:34:41Z

hw/bsp/esp32s2_kaluga_1/board.mk

+CFLAGS += \
+    "-DCFG_TUSB_MCU=OPT_MCU_ESP32S2"


seem like this line isn't needed, I didn't see it on other board.mk

alisitsyn · 2021-04-13T19:58:39Z

@hathach,

Thank you for your feedback and advices.

Please revert the submodules removal. They are important and needed to build for other platforms.

It seems that submodules were removed incidentally by script.

Can you move the esp32s3 into it own family hw/bsp/esp32s3 similar to esps2, it will help to group similar driver such as board_init() and ws2812 driver.

I am going to move it into the separated family folder hw/bsp/esp32sx how it was done in the latest master. All examples will inherit the same standard board functionality and ws2812 driver (exists on esp32s3_addax_1 board also).

CI also needs to be updated to build with esp32s3, though I could help with that.

I updated the CI scripts to work with esp32sx family instead of esp32s2 based on latest master. The branch will be updated soon. I will need your advises soon.

hathach · 2021-04-14T06:47:43Z

It seems that submodules were removed incidentally by script.

no problem at all, just make sure you revert it

I am going to move it into the separated family folder hw/bsp/esp32sx how it was done in the latest master. All examples will inherit the same standard board functionality and ws2812 driver (exists on esp32s3_addax_1 board also).

I actually think it is better to keep family separated as esp32s2 and esp32s3 (rather than esp32sx), although IDF does an great job of driver abstraction, It is still useful and more obvious to end-user. Yeah, I know there is a bit of code duplication but I am fine with it. Other mcu such as stm32 f2 f3 f4 is actually quite similar, but separated them make it easier to maintain and manage. Hope this makes sense

I updated the CI scripts to work with esp32sx family instead of esp32s2 based on latest master. The branch will be updated soon. I will need your advises soon.

Great, I am looking forward your next push.

alisitsyn · 2021-04-14T11:42:03Z

I actually think it is better to keep family separated as esp32s2 and esp32s3 (rather than esp32sx), although IDF does an great job of driver abstraction, It is still useful and more obvious to end-user. Yeah, I know there is a bit of code duplication but I am fine with it. Other mcu such as stm32 f2 f3 f4 is actually quite similar, but separated them make it easier to maintain and manage. Hope this makes sense

Actually the ESP32S (like F4 for STM) is the family and ESP32-S2, ESP32-S3 are actual chips inside this family. I personally think that this is logically correct. It is not an issue to follow your approach from make standpoint. As I understand the structure will be like below. Is this correct?

tinyusb_esp\hw\bsp\
                    esp32s2\ < --- esp32s2 family boards
                        boards\
                            adafruit_feather_esp32s2
                            adafruit_magtag_29gray
                            adafruit_metro_esp32s2
                            espressif_kaluga_1
                            espressif_saola_1
                    esp32s3 < --- esp32s2 family boards
                        boards\
                            espressif_addax_1
tinyusb_esp\src\portable\espressif\ < ---- dcd driver
                                    esp32s2\
                                    esp32s3\

The CI test for espressif_addax_1 board is failed because the actual update of soc/hal drivers in ESP-IDF is not merged.

alisitsyn · 2021-04-14T12:31:17Z

@me-no-dev , Unfortunately I can not add you to reviewers. Could you take a look to update and give me your comments? Thank you.

hathach · 2021-04-14T14:53:03Z

Actually the ESP32S (like F4 for STM) is the family and ESP32-S2, ESP32-S3 are actual chips inside this family. I personally think that this is logically correct. It is not an issue to follow your approach from make standpoint. As I understand the structure will be like below. Is this correct?
tinyusb_esp\hw\bsp\
                    esp32s2\ < --- esp32s2 family boards
                        boards\
                            adafruit_feather_esp32s2
                            adafruit_magtag_29gray
                            adafruit_metro_esp32s2
                            espressif_kaluga_1
                            espressif_saola_1
                    esp32s3 < --- esp32s2 family boards
                        boards\
                            espressif_addax_1can temporarily

Yeah, this is correct. Normally MCUs within families is only slightly different in ram & rom, pinout (and maybe an extra peripheral). I thought esp32s3 is totally different with extra core. Though you guys at espressif apparently understand it better, if you think their cores and peripheral is similar enough to be in the same group. So yes, please go ahead with esp32sx

tinyusb_esp\src\portable\espressif\ < ---- dcd driver
esp32s2
esp32s3\

In both cases, the dcd file should only be one file dcd_esp32sx.c, dcd is only different if mcus use totally different USB IP. In this case, both s2 and s3 (and other stm32) share the same synopsys IP.

The CI test for espressif_addax_1 board is failed because the actual update of soc/hal drivers in ESP-IDF is not merged.

This wouldn't an issue, can you tell me at which branch it works, I will see if I could quickly modify ci to get it build (also have some local testing as well). If it is not easy, we could temporarily disable its build.

hathach · 2021-04-14T14:54:26Z

@me-no-dev , Unfortunately I can not add you to reviewers. Could you take a look to update and give me your comments? Thank you.

Hmm, that is weird, I am sure he is one of repo contributors, in any case, I have added him as reviewer.

hathach

This looks good already, please keep it that way. No need to separate s2 and s3. Ignore my previous comment, since I thought s2 and s3 are totally different due to addition of new core and ble 😅 . This look great already. I will try to have some hand-on testing and will provide more feedback soon.

alisitsyn · 2021-04-15T08:22:46Z

@hathatch,

Normally MCUs within families is only slightly different in ram & rom, pinout (and maybe an extra peripheral). I thought esp32s3 is totally different with extra core. Though you guys at espressif apparently understand it better, if you think their cores and peripheral is similar enough to be in the same group. So yes, please go ahead with esp32sx

You are right about the ESP32-S3. It has enough differences (cores, different peripheral and memory changes) and I can see the reason to divide them into two different family folders. I think it is better to get team opinion about this, cc @me-no-dev.

In both cases, the dcd file should only be one file dcd_esp32sx.c, dcd is only different if mcus use totally different USB IP. In this case, both s2 and s3 (and other stm32) share the same synopsys IP.

The USB IP are almost the same, so agree it is better to keep the one device controller driver for them. I also saw the #382 but it is better to keep existing driver for now.

This wouldn't an issue, can you tell me at which branch it works, I will see if I could quickly modify ci to get it build (also have some local testing as well). If it is not easy, we could temporarily disable its build.

The branch with the required update is in the internal system. I can provide the patch to fix the actual hal/soc in esp-idf master.
I will contact our team to provide the better way to solve this and will contact you then.

Thank you for your comments. I will contact you ASAP.

hathach · 2021-04-15T08:41:47Z

You are right about the ESP32-S3. It has enough differences (cores, different peripheral and memory changes) and I can see the reason to divide them into two different family folders. I think it is better to get team opinion about this, cc @me-no-dev.

Actually, it is not a big deal, this bsp family folder thing is just an way to organize boards for testing stock examples in tinyusb repo. It should not make much of differences. Since most end-user people will just IDF platform, arduino or micropython/circuitpython. It is not really that important and can be easily changes later on if needed.

One of the tip to see if we should split or not is when you see lots of #ifdef in source/cmake etc .. I will leave it up to you to decide, I am fine with either way.

The USB IP are almost the same, so agree it is better to keep the one device controller driver for them. I also saw the #382 but it is better to keep existing driver for now.

Yup, later on I would like to abstract the synopsys driver enough so that stm and esp32s can share functions (much like ehci, ohci). Currently stm32f receive lots of improvement and enhancement regarding iso, fifo scheme/optimization. But it doesn't apply to esp32s just yet due to lack of time for testing. But that is for later PRs.

The branch with the required update is in the internal system. I can provide the patch to fix the actual hal/soc in esp-idf master.
I will contact our team to provide the better way to solve this and will contact you then.

Thank you for your comments. I will contact you ASAP.

Actually, I am fine with testing it, we can just comment out the build for esp32s3 in ci matrix board build for now, so that ci won't block other PRs. We could always enable later on by a flip of the comment. Our focus should only on the file changes. Give me a bit of time, I am still catching up with review :)

add support of new esp32s3 target and the board update the roles.mk wrapper to allow dfu flashing of espressif chip update examples to allow compilation for esp32s3_addax_1 board once the code is tested the PR to original tinyusb repo will be submitted

`hw\bsp` separate one family folder to esp32s2, esp32s3 add board specific board.cmake file to override board specific options(features) fix examples and test scripts to use new family approach

alisitsyn · 2021-04-16T14:12:11Z

Actually, it is not a big deal, this bsp family folder thing is just an way to organize boards for testing stock examples in tinyusb repo. It should not make much of differences. Since most end-user people will just IDF platform, arduino or micropython/circuitpython. It is not really that important and can be easily changes later on if needed.

I agree with your opinion and pushed new commit to update PR according to our discussion. Please take a look when possible.

Yup, later on I would like to abstract the synopsys driver enough so that stm and esp32s can share functions (much like ehci, ohci). Currently stm32f receive lots of improvement and enhancement regarding iso, fifo scheme/optimization. But it doesn't apply to esp32s just yet due to lack of time for testing. But that is for later PRs.

This is a good idea and I can take a look to update later in order to apply changes to espressif driver.

We could always enable later on by a flip of the comment. Our focus should only on the file changes. Give me a bit of time, I am still catching up with review

I rebased on master and pushed new commit to this PR to group boards using the target name as family. The esp32s3 based board was commented out in board matrix. Still have issues with raspberry_pi_pico but the failures are not related to update.
Please review the changes when you have time.

Thanks you!

hathach

Great work, everything look good now. I have a couple of minor comments. Since I have do a merge from (master) to fix rp2040 build, I take the chance to update the PR according to the comment as well. I think this PR is ready to merge, unless you still plan to push more update ( let me know if you do ).

Update: look like rp2040 still failed, I think we can safely ignore this. Will fix it later if it still failed when merged.

examples/make.mk

examples/device/board_test/CMakeLists.txt

alisitsyn · 2021-04-18T19:17:16Z

I take the chance to update the PR according to the comment as well. I think this PR is ready to merge, unless you still plan to push more update ( let me know if you do ).

The updates look good to me do not have anything to add. Please merge this PR ASAP in spite of rp2040 failures. My esp-idf MR will be merged then. Thank you.

hathach · 2021-04-19T06:02:14Z

Merged now, thank you very much for your PR 👍 👍

alisitsyn · 2021-04-19T06:35:19Z

Merged now, thank you very much for your PR 👍 👍

Thank you very much for your support. 👍

hathach · 2021-04-19T06:38:38Z

Merged now, thank you very much for your PR +1 +1

Thank you very much for your support.

I am the one to say thank :)

remove unused submodules rebese `merge pull request hathach#783`: Add new Espressif target esp32s3 for tinyUSB (esp-based_on_334e95f) These submodules are not needed when TinyUSB is built as an ESP-IDF component. We remove them to avoid fetching extra submodules when ESP-IDF is cloned with --recursive flag. Note: this is a local change, not for upstream.

alisitsyn · 2021-05-18T07:54:38Z

Hello @hathach,

There was the delay with merging of esp-idf tinyusb support on esp32s3 , but it was finally merged. Could you help to reenable CI test support for esp32s3_addax_1 board? Please let me know if you have any questions. Thank you.

hathach · 2021-05-19T04:15:26Z

@alisitsyn no problem, done via #840

alisitsyn · 2021-05-19T07:36:21Z

@hathach,

Thank you very much! 👍

hathach · 2021-05-19T07:42:36Z

No problem, you're welcome

hathach requested changes Apr 13, 2021

View reviewed changes

hathach marked this pull request as draft April 13, 2021 17:58

alisitsyn requested a review from hathach April 13, 2021 20:03

alisitsyn force-pushed the esp-based_on_334e95f branch 3 times, most recently from 49c9f8a to 5873201 Compare April 14, 2021 11:12

hathach requested a review from me-no-dev April 14, 2021 14:40

hathach reviewed Apr 14, 2021

View reviewed changes

alisitsyn force-pushed the esp-based_on_334e95f branch 2 times, most recently from c96f189 to 10cd6fd Compare April 16, 2021 13:31

alisitsyn added 2 commits April 16, 2021 15:41

espressif: group boards using target name as a family

f5e02e7

`hw\bsp` separate one family folder to esp32s2, esp32s3 add board specific board.cmake file to override board specific options(features) fix examples and test scripts to use new family approach

espressif: temporarily remove the espressif_addax_1 board from test

91e5740

alisitsyn force-pushed the esp-based_on_334e95f branch from 10cd6fd to 91e5740 Compare April 16, 2021 13:50

hathach added 2 commits April 18, 2021 01:30

Merge branch 'master' into alisitsyn-esp-based_on_334e95f

38cc459

update per review

803b755

hathach marked this pull request as ready for review April 17, 2021 18:54

hathach approved these changes Apr 17, 2021

View reviewed changes

examples/make.mk Outdated Show resolved Hide resolved

examples/device/board_test/CMakeLists.txt Outdated Show resolved Hide resolved

hathach merged commit 01801c8 into hathach:master Apr 19, 2021

hathach changed the title ~~WIP: Add new Espressif target esp32s3 for tinyUSB~~ Add new Espressif target esp32s3 for tinyUSB Apr 19, 2021

hathach mentioned this pull request May 19, 2021

enable ci for esp32s3 boards #840

Merged

hathach mentioned this pull request Sep 17, 2021

Move to IDF 4.4 for ESP32-S3 support adafruit/tinyuf2#149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Espressif target esp32s3 for tinyUSB #783

Add new Espressif target esp32s3 for tinyUSB #783

alisitsyn commented Apr 12, 2021 •

edited

Loading

me-no-dev commented Apr 13, 2021

alisitsyn commented Apr 13, 2021

hathach left a comment •

edited

Loading

hathach Apr 13, 2021

hathach Apr 13, 2021

alisitsyn Apr 13, 2021 •

edited

Loading

hathach Apr 14, 2021 •

edited

Loading

alisitsyn Apr 14, 2021 •

edited

Loading

hathach Apr 13, 2021

alisitsyn commented Apr 13, 2021

hathach commented Apr 14, 2021

alisitsyn commented Apr 14, 2021 •

edited

Loading

alisitsyn commented Apr 14, 2021

hathach commented Apr 14, 2021 •

edited

Loading

hathach commented Apr 14, 2021

hathach left a comment

alisitsyn commented Apr 15, 2021 •

edited

Loading

hathach commented Apr 15, 2021 •

edited

Loading

alisitsyn commented Apr 16, 2021

hathach left a comment •

edited

Loading

alisitsyn commented Apr 18, 2021 •

edited

Loading

hathach commented Apr 19, 2021

alisitsyn commented Apr 19, 2021

hathach commented Apr 19, 2021

alisitsyn commented May 18, 2021

hathach commented May 19, 2021

alisitsyn commented May 19, 2021

hathach commented May 19, 2021

		ifeq ($(findstring esp32s, $(BOARD)), esp32s)
		$(info Vendor: $(VENDOR), family: $(CHIP_FAMILY), target: $(CHIP_TARGET))

		CFLAGS += \
		"-DCFG_TUSB_MCU=OPT_MCU_ESP32S2"

Add new Espressif target esp32s3 for tinyUSB #783

Add new Espressif target esp32s3 for tinyUSB #783

Conversation

alisitsyn commented Apr 12, 2021 • edited Loading

What happened in this PR

me-no-dev commented Apr 13, 2021

alisitsyn commented Apr 13, 2021

hathach left a comment • edited Loading

Choose a reason for hiding this comment

hathach Apr 13, 2021

Choose a reason for hiding this comment

hathach Apr 13, 2021

Choose a reason for hiding this comment

alisitsyn Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

hathach Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

alisitsyn Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

hathach Apr 13, 2021

Choose a reason for hiding this comment

alisitsyn commented Apr 13, 2021

hathach commented Apr 14, 2021

alisitsyn commented Apr 14, 2021 • edited Loading

alisitsyn commented Apr 14, 2021

hathach commented Apr 14, 2021 • edited Loading

hathach commented Apr 14, 2021

hathach left a comment

Choose a reason for hiding this comment

alisitsyn commented Apr 15, 2021 • edited Loading

hathach commented Apr 15, 2021 • edited Loading

alisitsyn commented Apr 16, 2021

hathach left a comment • edited Loading

Choose a reason for hiding this comment

alisitsyn commented Apr 18, 2021 • edited Loading

hathach commented Apr 19, 2021

alisitsyn commented Apr 19, 2021

hathach commented Apr 19, 2021

alisitsyn commented May 18, 2021

hathach commented May 19, 2021

alisitsyn commented May 19, 2021

hathach commented May 19, 2021

alisitsyn commented Apr 12, 2021 •

edited

Loading

hathach left a comment •

edited

Loading

alisitsyn Apr 13, 2021 •

edited

Loading

hathach Apr 14, 2021 •

edited

Loading

alisitsyn Apr 14, 2021 •

edited

Loading

alisitsyn commented Apr 14, 2021 •

edited

Loading

hathach commented Apr 14, 2021 •

edited

Loading

alisitsyn commented Apr 15, 2021 •

edited

Loading

hathach commented Apr 15, 2021 •

edited

Loading

hathach left a comment •

edited

Loading

alisitsyn commented Apr 18, 2021 •

edited

Loading