boards: espressif: esp32s3_devkitc: introduce board variants

[JarmouniA]

Introduce board variants for Espressif's ESP32S3-DevKitC board to support the different SPI Flash & PSRAM sizes of ESP32-S3-WROOM module based on https://docs.espressif.com/projects/esp-dev-kits/en/latest/esp32s3/esp32-s3-devkitc-1/user_guide.html#ordering-information

To maintain the same user experience as before this change, the ESP32-S3-DevKitC-1-N8 variant is kept as the default and will be selected automatically when no variant is given in the build command; i.e. west build -p always -b esp32s3_devkitc/esp32s3/procpu samples/hello_world --sysbuild

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[JarmouniA]

CI failure unrelated to PR

west build -p -b esp32s3_devkitc/esp32s3/procpu tests/drivers/counter/counter_basic_api -T drivers.counter.basic_api

... 
/__w/zephyr/zephyr/drivers/counter/counter_esp32_tmr.c:55:12: error: 'counter_esp32_init' defined but not used [-Werror=unused-function]
   55 | static int counter_esp32_init(const struct device *dev)
      |            ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

[JarmouniA]

Cc @rftafas

[marekmatej]

While I agree that we should find a better way to deal with the multiple FLASH/PSRAM sizes, I don't agree with the presented changes. It introduces unnecessary complexity that is not reflected in terms of maintainability or ease of use. The reasoning in the issue is wrong as the module (or SIP) is not the SOC.

If existing devkits are not addressing user needs, there are several existing options: Define their boards (based on devkits), or add configurations into a prj.conf, or create overlays, if they choose to depend on the existing devkits. Therefore, -1

[JarmouniA]

The reasoning in the issue is wrong as the module (or SIP) is not the SOC.

I am well aware of that, but I used the terms SoC/Series according to how things are actually configured in Zephyr

zephyr/soc/espressif/esp32s3/Kconfig.soc

Line 10 in 11c543e

config SOC_ESP32S3_R2

zephyr/soc/espressif/esp32s3/Kconfig

Line 4 in 11c543e

config SOC_SERIES_ESP32S3

[JarmouniA]

It introduces unnecessary complexity that is not reflected in terms of maintainability or ease of use.

I am unable to see what "unnecessary complexity" is being introduced with this change, we are using standard Zephyr hardware model tools to describe different board variants that are widely available, and this is done in many other boards in-tree already & is well understood by Zephyr users.

[marekmatej]

It introduces unnecessary complexity that is not reflected in terms of maintainability or ease of use.

I am unable to see what "unnecessary complexity" is being introduced with this change, we are using standard Zephyr hardware model tools to describe different board variants that are widely available, and this is done in many other boards in-tree already & is well understood by Zephyr users.

Using board variants to implement the PSRAM & FLASH sizes is not a good idea for these reasons:

1. It increases the number of files in the board support folder

2. It introduces this hard-to-read configuration of mashed potatoes here
   c5c307d#diff-2d5cfecfb100c18a12de360be08a7a1e15b497f2788fbff2cfdce9e3a7a47a00R5-R32
   and here
   c5c307d#diff-14856756005b905167ba94b5afb62b679daec9558d334052bbf8513ac0896addR7-R31

3. You did not take into account the PSRAM MODE and SPEED configs. That would make things even worse (more files and more cryptic configs). Leading to target names like esp32s3_devkitm/esp32s3/procpu/usb/n4r2/oct :-)

4. You supported just a single SOC and a single devkit. If we are going to accept changes like this, they need to cover all Espressif supported boards.

I think introducing the ESP32 related snippets is more straightforward, and it comes with much less maintenance costs. Which always needs to be considered.

To be able to use something like west build -b esp32s3_devkitm/esp32s3/procpu -p -DSNIPPETS="esp-psram-2M esp-flash-8M" would be more pleasant to use and maintain too!

[JarmouniA]

It introduces unnecessary complexity that is not reflected in terms of maintainability or ease of use.

I am unable to see what "unnecessary complexity" is being introduced with this change, we are using standard Zephyr hardware model tools to describe different board variants that are widely available, and this is done in many other boards in-tree already & is well understood by Zephyr users.

Using board variants to implement the PSRAM & FLASH sizes is not a good idea for these reasons:

1. It increases the number of files in the board support folder

That I concede, but there is no way around it with the tools we have rn.

2. It introduces this hard-to-read configuration of mashed potatoes here
   c5c307d#diff-2d5cfecfb100c18a12de360be08a7a1e15b497f2788fbff2cfdce9e3a7a47a00R5-R32
   and here
   c5c307d#diff-14856756005b905167ba94b5afb62b679daec9558d334052bbf8513ac0896addR7-R31

It's not my fault that Hardware Model V2 works that way, if you have a better & cleaner method in mind to support board variants, please propose it. Plus there is no need for it to be super readable, it's just a config file that will be setup one time & not modified ever again except to add another variant or to migrate to the next hardware model....

3. You did not take into account the PSRAM MODE and SPEED configs. That would make things even worse (more files and more cryptic configs). Leading to target names like esp32s3_devkitm/esp32s3/procpu/usb/n4r2/oct :-)

RAM & Flash sizes are fundamental characteristics of a given SoC/board, that are closely linked to the build system, & influence what app can be build for/run on a given board, it's not a hardware feature that can enabled/disabled, unlike other hardware characteristics that can be changed quickly in devicetree... that don't really merit to have their own variants.

4. You supported just a single SOC and a single devkit. If we are going to accept changes like this, they need to cover all Espressif supported boards.

That was intentional, to validate the idea first and get community feedback :)

I think introducing the ESP32 related snippets is more straightforward, and it comes with much less maintenance costs. Which always needs to be considered.

I don't think the changes proposed here add any meaningful amount of maintenance, they are written once & not touched again, the default config (N8) is still the same.

To be able to use something like west build -b esp32s3_devkitm/esp32s3/procpu -p -DSNIPPETS="esp-psram-2M esp-flash-8M" would be more pleasant to use and maintain too!

I will look into it, thank you for your valuable feedback!

[marekmatej]

@JarmouniA PTAL here for the inspiration #87432