boards/esp-based: Model features in Kconfig by leandrolanzieri · Pull Request #14223 · RIOT-OS/RIOT

leandrolanzieri · 2020-06-08T10:11:51Z

Contribution description

This models in Kconfig all the features of the esp8266 and esp32 boards:

esp32-heltec-lora32-v2
esp32-mh-et-live-minikit
esp32-olimex-evb
esp32-ttgo-t-beam
esp32-wemos-lolin-d32-pro
esp32-wroom-32
esp32-wrover-kit
esp8266-esp-12x
esp8266-olimex-mod
esp8266-sparkfun-thing

Testing procedure

Check the symbol organization
Green CI
tests/kconfig_features should pass for all boards. Which means that make info-features-provided and make dependency-debug-features-provided-kconfig should return the same list.

Issues/PRs references

Part of #14148

leandrolanzieri · 2020-06-17T09:13:57Z

Adapted to the new symbol classification

MrKevinWeiss

Looks good, lets see what murdock says. ACK!

CPU_MODEL issues

MrKevinWeiss · 2020-06-22T13:40:20Z

I guess the correct way to move forward is to set the CPU_MODEL correctly in the makefiles. It appears that CPU_MODEL is not being used for anything and should not change normal operation in RIOT.

leandrolanzieri · 2020-06-22T14:11:57Z

I guess the correct way to move forward is to set the CPU_MODEL correctly in the makefiles. It appears that CPU_MODEL is not being used for anything and should not change normal operation in RIOT.

Done

MrKevinWeiss

Much better, I ACK! If anyone has reservations speak up now, I don't want to leave this too long and it always needing rebasing.

MrKevinWeiss · 2020-06-23T07:08:42Z

Please squash

gschorcht

Please give me the chance to take a look. Sorry that I'm jumping into the review process so late but I was completely busy the last 3 month with preparing my courses for the ad-hoc online summer term. Now , I'm resuming slowly my work on RIOT. I will have a look in the next view days.

MrKevinWeiss · 2020-06-23T07:45:49Z

Yup and it looks like it needs another rebase. OK I will leave it in @gschorcht hands when he has time.

gschorcht · 2020-06-27T15:46:35Z

@leandrolanzieri Thank you for contributing this. What was the reason for introducing different CPU_MODEL. I am not really happy with it for the following reasons:

They don't have any impact on supported features.
The classification in CPU_MODEL_ESP32_D0WDQ6, CPU_MODEL_ESP32_WROOM_32, and CPU_MODEL_ESP32_WROVER isn't correct. WROOM-32 as well as WROVER are modules not CPUs. In fact they are nothing else than an ESP32 D0WDQ6 CPU plus flash and ram. That is the CPU_MODEL for all of them is the same is the same.
All the boards that I know use either the ESP32 D0WDQ6 or one of the modules with ESP32 D0WDQ6. All boards have 4 MB flash on board.
There is nothing in the source code that depends on the CPU model, neither in the SDK nor in RIOT port.

If you wan't to classify CPU modules, you would have to define

ESP32 D0WD (all ESP32 D0WD* are just different packages) dual-core and no flash
ESP32-D2WD dual core and 2 MB flash on chip
ESP32-U4WDH single core and 4 MB flash on chip
ESP32-S0WD single core and no fash
ESP32-PICO* dual core and 4 MB flash on chip

But, as I said, it has no consequences, the source code is the same for all versions of the CPU.

gschorcht · 2020-06-27T15:56:47Z

@leandrolanzieri The same for ESP8266. ESP12F is a module with CPU ESP8266 EX. The only difference is that the module already has some flash memory, a crystal and some capacitors and pull-ups. But the CPU model is the same and it doesn't have any impact on supported features.

gschorcht · 2020-06-27T16:02:13Z

cpu/esp_common/Kconfig

+config CPU_ARCH_XTENSA
+    bool
+    select HAS_ARCH_32BIT
+


Suggested change

select HAS_ARCH_ESP

see PR #14375

Why using a feature for that when CPU_ARCH could be checked?

gschorcht · 2020-06-27T16:14:12Z

cpu/esp32/Kconfig

+config HAS_PERIPH_ADC_CTRL
+    bool
+    help
+        Indicates that an ESP32 ADC controller peripheral is present.
+


ADC controller can't be configured by the user. It should be exposed in Kconfig.

Suggested change

config HAS_PERIPH_ADC_CTRL

bool

help

Indicates that an ESP32 ADC controller peripheral is present.

But this is a feature provided

As you can see the symbol has no prompt, meaning that the user can't change the value (it's an invisible symbol). It's here in order to indicate that the feature is provided, like indicated in Makefile.features.

I don't remember exactly, but providing it as a feature was a trick to compile cpu/esp32/periph/adc_ctrl.c when it is needed by other peripherals

RIOT/cpu/esp32/Makefile.dep

Lines 55 to 57 in ef4835a

ifneq (,$(filter periph_adc periph_dac,$(USEMODULE)))

FEATURES_REQUIRED += periph_adc_ctrl

endif

As I remember, the C files in cpu/*/periph are compiled based on used features.

As I remember, the C files in cpu/*/periph are compiled based on used features.

Yes. That's why this is included in the Kconfig. All features provided by Makefile.features should match these.

Hm, but periph_adc_ctrl is only used internally and shouldn't be exposed to the user. Isn't it possible to drop this feature from Kconfig?

As I mentioned, the user does not see this symbol and can't change the value of it directly (it has no prompt). It is there just to indicate that the feature is provided, just as FEATURES_PROVIDED += periph_adc_ctrl is there.

Edit: It is not a configurable parameter.

As I mentioned, the user does not see this symbol

It is shown in the feature list

-*- <HAS_PERIPH_ADC_CTRL>

In order to be able to see that, one needs to enable the show-all mode in menuconfig. That means that one will see symbols with and without prompt. You must have noticed also that one can't change the value of it. There is no way to make it not appear at all when using this mode.

leandrolanzieri · 2020-06-29T08:25:41Z

Thanks for the feedback @gschorcht

@leandrolanzieri Thank you for contributing this. What was the reason for introducing different CPU_MODEL. I am not really happy with it for the following reasons:

They don't have any impact on supported features.

That is the case usually so far with other CPU_MODELS, but we are trying to be as specific as possible with the categorization of CPUs.

The classification in CPU_MODEL_ESP32_D0WDQ6, CPU_MODEL_ESP32_WROOM_32, and CPU_MODEL_ESP32_WROVER isn't correct. WROOM-32 as well as WROVER are modules not CPUs. In fact they are nothing else than an ESP32 D0WDQ6 CPU plus flash and ram. That is the CPU_MODEL for all of them is the same is the same.

I understand what you mean. My though was that, usually a CPU model would define the amount of memory available so defining the modules as CPU model would make sense in that case. But we could go with CPU_MODEL_ESP32_D0WDQ6 if you think it's best.

All the boards that I know use either the ESP32 D0WDQ6 or one of the modules with ESP32 D0WDQ6. All boards have 4 MB flash on board.

Ok.

There is nothing in the source code that depends on the CPU model, neither in the SDK nor in RIOT port.

Same as above: we are trying to have all the information possible, it may not be used now, but I think that being specific does not hurt in this case.

If you wan't to classify CPU modules, you would have to define

ESP32 D0WD (all ESP32 D0WD* are just different packages) dual-core and no flash

ESP32-D2WD dual core and 2 MB flash on chip

ESP32-U4WDH single core and 4 MB flash on chip

ESP32-S0WD single core and no fash

ESP32-PICO* dual core and 4 MB flash on chip

Would you be OK with this?

leandrolanzieri · 2020-06-29T08:37:24Z

I rebased to solve conflict in the test Makefile. Also, I specified CPU_ARCH and CPU_FAM in Makefile.features, and selected the newly introduced arch_esp feature.

gschorcht · 2020-07-07T13:14:23Z

@leandrolanzieri PR #14117 will take longer to merge. How do we proceed? Shall we merge this PR first and PR #14117 has to be modified after a rebase. Or shall we apply the small cleanup from PR #14117 here as well?

leandrolanzieri · 2020-07-07T13:18:44Z

Or shall we apply the small cleanup from PR #14117 here as well?

Would this be to cherry-pick e74e062 ? Is it OK without the rest of the PR?

gschorcht · 2020-07-07T13:40:17Z

Or shall we apply the small cleanup from PR #14117 here as well?

Would this be to cherry-pick e74e062 ? Is it OK without the rest of the PR?

Yes and yes.

>All of them are features of each ESP SoC and have not to be configured by the board definition. Signed-off-by: Jean Pierre Dudey <jeandudey@hotmail.com> Co-authored-by: Gunar Schorcht <gunar@schorcht.net>

leandrolanzieri · 2020-07-07T14:07:06Z

@gschorcht done. I added the commit that moves the features. #14117 will still need to modify the Kconfig to add the definition and select esp_wifi_ap feature

gschorcht

Looks good now.

gschorcht · 2020-07-07T15:23:37Z

@leandrolanzieri Thanks for this contribution.

leandrolanzieri · 2020-07-07T19:44:04Z

Thanks for the review and the suggestions!

leandrolanzieri added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: Kconfig Area: Kconfig integration labels Jun 8, 2020

leandrolanzieri requested a review from gschorcht June 8, 2020 10:11

leandrolanzieri requested review from MichelRottleuthner, aabadie, cgundogan, fjmolinas, jia200x and smlng as code owners June 8, 2020 10:11

jia200x mentioned this pull request Jun 8, 2020

Tracker: Kconfig: Model features provided by CPU and Board #14148

Closed

leandrolanzieri force-pushed the pr/kconfig/esp_boards_symbols branch from 22ebcc8 to 1dbb1c8 Compare June 17, 2020 09:13

leandrolanzieri force-pushed the pr/kconfig/esp_boards_symbols branch from 1dbb1c8 to ef19441 Compare June 22, 2020 12:35

MrKevinWeiss previously approved these changes Jun 22, 2020

View reviewed changes

MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 22, 2020

leandrolanzieri added this to the Release 2020.07 milestone Jun 22, 2020

MrKevinWeiss approved these changes Jun 23, 2020

View reviewed changes

gschorcht requested changes Jun 23, 2020

View reviewed changes

leandrolanzieri force-pushed the pr/kconfig/esp_boards_symbols branch from 1154ba0 to 0d1ee5d Compare June 23, 2020 07:48

gschorcht requested changes Jun 27, 2020

View reviewed changes

leandrolanzieri force-pushed the pr/kconfig/esp_boards_symbols branch from 0d1ee5d to 840b9cc Compare June 29, 2020 08:36

boards/common/esp32: move ESP features to cpu/esp_common

3003cf7

>All of them are features of each ESP SoC and have not to be configured by the board definition. Signed-off-by: Jean Pierre Dudey <jeandudey@hotmail.com> Co-authored-by: Gunar Schorcht <gunar@schorcht.net>

leandrolanzieri force-pushed the pr/kconfig/esp_boards_symbols branch from 0cfe648 to 887ed43 Compare July 7, 2020 14:03

gschorcht approved these changes Jul 7, 2020

View reviewed changes

leandrolanzieri added 16 commits July 7, 2020 16:27

cpu/esp_common: Add Kconfig symbols

e6b698c

cpu/esp32: Add Kconfig symbols

0b0a03b

boards/common: Add Kconfig symbols

597c565

boards/esp32-heltec-lora32-v2: Add Kconfig symbols

689eea7

boards/esp32-mh-et-live-minikit: Add Kconfig symbols

c8428a1

boards/esp32-olimex-evb: Add Kconfig symbols

be5b169

boards/esp32-ttgo-t-beam: Add Kconfig symbols

595a433

boards/esp32-wemos-lolin-d32-pro: Add Kconfig symbols

46a2fd7

boards/esp32-wroom-32: Add Kconfig symbols

4804757

boards/esp32-wrover-kit: Add Kconfig symbols

6dd0d90

cpu/esp8266: Add Kconfig symbols

7a2e4c8

boards/common/esp8266: Add Kconfig symbols

85b674d

boards/esp8266-esp-12x: Add Kconfig symbols

d5bfce9

boards/esp8266-olimex-mod: Add Kconfig symbols

0b26d99

boards/esp8266-sparkfun-thing: Add Kconfig symbols

a4f9fbb

tests/kconfig_features: Add esp-based boards to whitelist

c6eaae9

leandrolanzieri force-pushed the pr/kconfig/esp_boards_symbols branch from 887ed43 to c6eaae9 Compare July 7, 2020 14:35

leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 7, 2020

gschorcht merged commit f320638 into RIOT-OS:master Jul 7, 2020

leandrolanzieri deleted the pr/kconfig/esp_boards_symbols branch July 8, 2020 06:24

gschorcht mentioned this pull request Jul 11, 2020

boards: add support for the ESP32-Ethernet-Kit board #14490

Merged

	ifneq (,$(filter periph_adc periph_dac,$(USEMODULE)))
	FEATURES_REQUIRED += periph_adc_ctrl
	endif

Conversation

leandrolanzieri commented Jun 8, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

leandrolanzieri commented Jun 17, 2020

Uh oh!

MrKevinWeiss left a comment

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss commented Jun 22, 2020

Uh oh!

leandrolanzieri commented Jun 22, 2020

Uh oh!

MrKevinWeiss left a comment

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss commented Jun 23, 2020

Uh oh!

gschorcht left a comment

Choose a reason for hiding this comment

Uh oh!

MrKevinWeiss commented Jun 23, 2020

Uh oh!

gschorcht commented Jun 27, 2020

Uh oh!

gschorcht commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leandrolanzieri commented Jun 29, 2020

Uh oh!

gschorcht commented Jul 7, 2020

Uh oh!

leandrolanzieri commented Jul 7, 2020

Uh oh!

gschorcht commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leandrolanzieri commented Jul 7, 2020

Uh oh!

gschorcht left a comment

Choose a reason for hiding this comment

Uh oh!

gschorcht commented Jul 7, 2020

Uh oh!

leandrolanzieri commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

gschorcht commented Jun 27, 2020 •

edited

Loading

leandrolanzieri Jun 30, 2020 •

edited

Loading

leandrolanzieri commented Jun 29, 2020 •

edited

Loading

gschorcht commented Jul 7, 2020 •

edited

Loading