boards/esp32: changes the approach for configurations of ADC channels in board definitions #11289

gschorcht · 2019-03-27T08:16:19Z

Contribution description

This PR changes the approach of peripheral configurations for ADC in board definitions to the usual RIOT approach. With these changes, peripheral configurations use static const arrays in the boards/esp32*/periph_conf.h files and define the *_NUMOF macros using the size of these static array.

The static configuration arrays contain only definitions that can be changed by the board definition or the application. They do not contain any MCU implementation detail. The board definitions use preprocessor defines as before to fill these static configuration arrays. This makes it possible to override all configurations either with the make command or application specific configuration files.

Testing procedure

Compilation and test with the most common ESP32 board should be executed

make BOARD=esp32-wroom-32 -C tests/periph_adc flash test

Issues/PRs references

PRs #11289 #11290 #11291 #11292 #11293 #11294 are releated and should be merged together.

boards/esp32-olimex-evb/Makefile.features

leandrolanzieri

Just two comments, otherwise looks good to me.

cpu/esp32/periph/adc.c

leandrolanzieri

Tested the ADC and GPIOs on esp32-wroom-32 and everything is working fine. ACK but we should wait the other PRs.

leandrolanzieri · 2019-03-28T12:57:45Z

@gschorcht please squash

gschorcht · 2019-03-28T14:10:43Z

@leandrolanzieri Squashed.

Thanks a lot for reviewing and testing.

ACK but we should wait the other PRs.

Ok. When we start to merge, we should start with this PR so that I can rebase all related PRs before they are merged.

leandrolanzieri · 2019-03-29T14:22:32Z

Mmm @gschorcht murdock is failing. Maybe olimex_esp32_gateway is not in USEMODULE when Makefile.features is evaluated?

leandrolanzieri · 2019-03-29T14:54:05Z

Mmm @gschorcht murdock is failing. Maybe olimex_esp32_gateway is not in USEMODULE when Makefile.features is evaluated?

Nope, periph_adc passes. For some reason adc.c is tried to be compiled for all applications.

gschorcht · 2019-03-29T18:20:22Z

@leandrolanzieri Hm, ... periph/adc.c is part of module periph and is always compiled. For test/periph_adc the compilation in mrudock doesn't fail since it requires feature periph_adc which is not provided by esp32-olimex-evb. But for all other application periph_adc isn't required. That's why they fail in murduck with something like:

cpu/esp32/periph/adc.c:173:27: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits]
     CHECK_PARAM_RET (line < ADC_NUMOF, -1)

And now I remember that this was the reason why I had the variable adc_channel_num in addition to the macro ADC_NUMOF. The check against the varaiable doesn't fail during compilation.

gschorcht · 2019-03-29T18:25:32Z

@leandrolanzieri I can try to embed it in #if MODULE_PERIPH_ADC

leandrolanzieri · 2019-03-29T23:51:53Z

@leandrolanzieri Hm, ... periph/adc.c is part of module periph and is always compiled. For test/periph_adc the compilation in mrudock doesn't fail since it requires feature periph_adc which is not provided by esp32-olimex-evb. But for all other application periph_adc isn't required. That's why they fail in murduck with something like:
cpu/esp32/periph/adc.c:173:27: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits]
     CHECK_PARAM_RET (line < ADC_NUMOF, -1)
And now I remember that this was the reason why I had the variable adc_channel_num in addition to the macro ADC_NUMOF. The check against the varaiable doesn't fail during compilation.

Is there a way to change this dependency?

gschorcht · 2019-03-29T23:53:56Z

@leandrolanzieri

Is there a way to change this dependency?

Yes, I will provide a fix.

gschorcht · 2019-03-29T23:59:20Z

@leandrolanzieri Commit 66f7742 should fix the problem. Let's see what murdock says.
One comment. Even though the defines for DAC changes in commit 66f7742, I would like to leave it in this commit because it is necessary to fixes compile problems with ADC and it will prevent merge conflicts with the DAC PR #11290. Unfortunatly, ADCs and DACs have to be handled in same file since they use the same hardware unit.

gschorcht · 2019-03-30T01:35:31Z

@leandrolanzieri The last commit solved the problem. I required, I could squash again.

leandrolanzieri

@gschorcht Though last changes fix the compiling problem I'm not sure if this is the correct way to handle this dependency issue. Looking at other CPUs I can see that it is common in RIOT to include some periph module directly in the CPU Makefile.include, so it's clear that the issue is out of the scope of this PR.

We could merge this as a temporary solution while the dependencies are fixed. Maybe @haukepetersen @jcarrano or @kaspar030 have a better idea on how to proceed?

gschorcht · 2019-04-03T07:19:35Z

@leandrolanzieri @MrKevinWeiss The changes for all periphs are now tested and working. Do you think we should merge them before branch 2019.4 is created to release it with 2019.4? If so, we would have to do it at latest tomorrow since I'm not in the office from Friday to Thursday next week.

leandrolanzieri · 2019-04-03T08:30:35Z

@gschorcht I think we should address the peripheral dependency issue first. @cladmi has pointed out that the peripheral directory should be specified differently, by including include $(RIOTMAKE)/periph.mk in the Makefile instead of include $(RIOTBASE)/Makefile.base. That way only the needed drivers are compiled.

gschorcht · 2019-04-03T09:02:43Z

@leandrolanzieri

@cladmi has pointed out that the peripheral directory should be specified differently, by including include $(RIOTMAKE)/periph.mk in the Makefile instead of include $(RIOTBASE)/Makefile.base

I see, I have tried it and it compiles indeed only the periph/*.c that are required by modules. Unfortunatly, it would require some changes in all periph/*.c using #if MODULE_PERIPH_* more strictly. Otherwise there are linker errors.

How do we proceed. Should I provide a PR with required changes in periph/*.c that has to be merged before these PRs can be rebased?

leandrolanzieri · 2019-04-03T09:16:26Z

Unfortunatly, it would require some changes in all periph/*.c using #if MODULE_PERIPH_* more strictly. Otherwise there are linker errors.

board_common.c should also only print the configuration of the used modules.

How do we proceed. Should I provide a PR with required changes in periph/*.c that has to be merged before these PRs can be rebased?

Yes, please do that on a separate PR.

gschorcht · 2019-04-03T09:33:06Z

board_common.c should also only print the configuration of the used modules.

Exactly.

gschorcht · 2019-04-03T09:38:42Z

@leandrolanzieri Hm, ... there is one problem with the $(RIOTMAKE)/periph.mk. Since ADCs and DACs use the same hardware devices that need to be initialized if either ADCs or DACs are used, both of them are implemented in periph/adc.c. But periph/adc.c isn't compiled if only module periph_dac is used 😟 Even it would be possible to enable module periph_adc automatically if periph_dac is enabled, finaly we would have to use #if MODULE_PERIPH_ADC, #if ADC_NUM_OF ... wrappers inside periph/adc.c in same way.

leandrolanzieri · 2019-04-03T09:52:50Z

Since ADCs and DACs use the same hardware devices that need to be initialized if either ADCs or DACs are used

Is there a way to extract this common initialization?

gschorcht · 2019-04-03T09:57:38Z

Unfortunatly not without bigger structure changes.

gschorcht · 2019-04-03T23:06:05Z

@leandrolanzieri In PR #11337, you will find the changes for periph_* submodules as we discussed it here. But I'm not sure if these significant changes make really sense, just to solve the small problem in this PR. Maybe it would be better to continue with the workarround in this PR for the following reason:

PR cpu/esp32: fix of periph_* submodule compilation #11337 will definitely take some time before it can be merged.
PRs boards/esp32: changes the approach for configurations of ADC channels in board definitions #11289 to boards/esp32: changes the approach for configurations of UART interfaces in board definitions #11294 are tested and work. If we wait for PR cpu/esp32: fix of periph_* submodule compilation #11337, it might become necessary to review and test all of them again once they are rebased onto PR cpu/esp32: fix of periph_* submodule compilation #11337.

leandrolanzieri · 2019-04-04T06:40:39Z

@gschorcht

you will find the changes for periph_* submodules as we discussed it here

Thanks for addressing the issue so quickly.

But I'm not sure if these significant changes make really sense, just to solve the small problem in this PR

IMHO fixing these dependencies is important in general, not just for this PR, and should be addressed before this cleanup. @cladmi do you have any opinion on this?

PRs boards/esp32: changes the approach for configurations of ADC channels in board definitions #11289 to boards/esp32: changes the approach for configurations of UART interfaces in board definitions #11294 are tested and work. If we wait for PR cpu/esp32: fix of periph_* submodule compilation #11337, it might become necessary to review and test all of them again once they are rebased onto PR cpu/esp32: fix of periph_* submodule compilation #11337.

Aren't some changes going to be needed anyways if #11289 - #11294 are merged before #11337? I'm happy to review and test them again.

gschorcht · 2019-04-04T10:14:58Z

Aren't some changes going to be needed anyways if #11289 - #11294 are merged before #11337? I'm happy to review and test them again.

Yes, but it seems easier for me to keep my branches and the changes in PR #11289 - #11294 consistent if we merge the changes in this PR first and then rebase PR #11337 than to wait with all the different branches until PR #11337 is merged. I planned to continue with changes of peripherals that are not affected by PR #11337.

gschorcht · 2019-04-11T09:52:19Z

@leandrolanzieri How do we continue with the bunch of this PRs that are ready and PR #11371 that requires a review and is a structural change?

The GPIO definitions defined here are required in this file to be able to use them in peripheral configurations.

ADC pins are now configured using static arrays in header files instead of static variables in implementation to be able to define ADC_NUMOF using the size of these arrays instead of a variable.

leandrolanzieri

Ok, I tested again and everything seems fine. ACK.

gschorcht · 2019-05-06T11:26:25Z

@leandrolanzieri Thanks a lot.

gschorcht added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: boards Area: Board ports Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Mar 27, 2019

gschorcht requested a review from leandrolanzieri March 27, 2019 08:16

gschorcht changed the title ~~boards/esp32: changes the approach for peripheral configurations of ADC channels in ESP32 board definitions~~ boards/esp32: changes the approach for configurations of ADC channels in board definitions Mar 27, 2019

gschorcht added this to the Release 2019.04 milestone Mar 27, 2019

gschorcht mentioned this pull request Mar 27, 2019

boards/esp32: changes the approach for peripheral configurations in ESP32 board definitions #10644

Closed

leandrolanzieri reviewed Mar 27, 2019

View reviewed changes

boards/esp32-olimex-evb/Makefile.features Show resolved Hide resolved

leandrolanzieri requested changes Mar 27, 2019

View reviewed changes

cpu/esp32/periph/adc.c Outdated Show resolved Hide resolved

cpu/esp32/periph/adc.c Outdated Show resolved Hide resolved

leandrolanzieri added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Mar 27, 2019

gschorcht force-pushed the cpu/esp32/periph/conf/adc branch from 75db4ec to 983afdf Compare March 27, 2019 15:54

leandrolanzieri approved these changes Mar 28, 2019

View reviewed changes

leandrolanzieri added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Mar 28, 2019

gschorcht force-pushed the cpu/esp32/periph/conf/adc branch from 983afdf to e31c232 Compare March 28, 2019 14:05

leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 29, 2019

gschorcht force-pushed the cpu/esp32/periph/conf/adc branch 2 times, most recently from 5047048 to 66f7742 Compare March 30, 2019 00:58

leandrolanzieri requested changes Apr 1, 2019

View reviewed changes

gschorcht mentioned this pull request Apr 3, 2019

cpu/esp32: fix of periph_* submodule compilation #11337

Merged

gschorcht added 4 commits May 2, 2019 16:38

cpu/esp32: GPIO defs required for periph conf

e2abe00

The GPIO definitions defined here are required in this file to be able to use them in peripheral configurations.

cpu/esp32: ADC config approach changed

32d6c04

ADC pins are now configured using static arrays in header files instead of static variables in implementation to be able to define ADC_NUMOF using the size of these arrays instead of a variable.

boards/esp32: disable ADC for Olimex ESP32 EVB

0bab7e0

boards/esp32: ADC config approach changed

3db78e0

ADC pins are now configured using static arrays in header files instead of static variables in implementation to be able to define ADC_NUMOF using the size of these arrays instead of a variable.

gschorcht force-pushed the cpu/esp32/periph/conf/adc branch from 66f7742 to 3db78e0 Compare May 2, 2019 14:49

leandrolanzieri approved these changes May 6, 2019

View reviewed changes

leandrolanzieri merged commit 9fc9ff2 into RIOT-OS:master May 6, 2019

gschorcht deleted the cpu/esp32/periph/conf/adc branch May 6, 2019 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boards/esp32: changes the approach for configurations of ADC channels in board definitions #11289

boards/esp32: changes the approach for configurations of ADC channels in board definitions #11289

gschorcht commented Mar 27, 2019 •

edited

Loading

leandrolanzieri left a comment

leandrolanzieri left a comment

leandrolanzieri commented Mar 28, 2019

gschorcht commented Mar 28, 2019

leandrolanzieri commented Mar 29, 2019

leandrolanzieri commented Mar 29, 2019 •

edited

Loading

gschorcht commented Mar 29, 2019

gschorcht commented Mar 29, 2019

leandrolanzieri commented Mar 29, 2019

gschorcht commented Mar 29, 2019 •

edited

Loading

gschorcht commented Mar 29, 2019 •

edited

Loading

gschorcht commented Mar 30, 2019

leandrolanzieri left a comment

gschorcht commented Apr 3, 2019 •

edited

Loading

leandrolanzieri commented Apr 3, 2019

gschorcht commented Apr 3, 2019

leandrolanzieri commented Apr 3, 2019

gschorcht commented Apr 3, 2019

gschorcht commented Apr 3, 2019 •

edited

Loading

leandrolanzieri commented Apr 3, 2019

gschorcht commented Apr 3, 2019

gschorcht commented Apr 3, 2019 •

edited

Loading

leandrolanzieri commented Apr 4, 2019

gschorcht commented Apr 4, 2019

gschorcht commented Apr 11, 2019 •

edited

Loading

leandrolanzieri left a comment

gschorcht commented May 6, 2019

boards/esp32: changes the approach for configurations of ADC channels in board definitions #11289

boards/esp32: changes the approach for configurations of ADC channels in board definitions #11289

Conversation

gschorcht commented Mar 27, 2019 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

leandrolanzieri left a comment

Choose a reason for hiding this comment

leandrolanzieri left a comment

Choose a reason for hiding this comment

leandrolanzieri commented Mar 28, 2019

gschorcht commented Mar 28, 2019

leandrolanzieri commented Mar 29, 2019

leandrolanzieri commented Mar 29, 2019 • edited Loading

gschorcht commented Mar 29, 2019

gschorcht commented Mar 29, 2019

leandrolanzieri commented Mar 29, 2019

gschorcht commented Mar 29, 2019 • edited Loading

gschorcht commented Mar 29, 2019 • edited Loading

gschorcht commented Mar 30, 2019

leandrolanzieri left a comment

Choose a reason for hiding this comment

gschorcht commented Apr 3, 2019 • edited Loading

leandrolanzieri commented Apr 3, 2019

gschorcht commented Apr 3, 2019

leandrolanzieri commented Apr 3, 2019

gschorcht commented Apr 3, 2019

gschorcht commented Apr 3, 2019 • edited Loading

leandrolanzieri commented Apr 3, 2019

gschorcht commented Apr 3, 2019

gschorcht commented Apr 3, 2019 • edited Loading

leandrolanzieri commented Apr 4, 2019

gschorcht commented Apr 4, 2019

gschorcht commented Apr 11, 2019 • edited Loading

leandrolanzieri left a comment

Choose a reason for hiding this comment

gschorcht commented May 6, 2019

gschorcht commented Mar 27, 2019 •

edited

Loading

leandrolanzieri commented Mar 29, 2019 •

edited

Loading

gschorcht commented Mar 29, 2019 •

edited

Loading

gschorcht commented Mar 29, 2019 •

edited

Loading

gschorcht commented Apr 3, 2019 •

edited

Loading

gschorcht commented Apr 3, 2019 •

edited

Loading

gschorcht commented Apr 3, 2019 •

edited

Loading

gschorcht commented Apr 11, 2019 •

edited

Loading