Skip to content

Conversation

@gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Mar 27, 2019

Contribution description

This PR changes the approach of peripheral configurations for DAC channels 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.

Please note that commit 8b48dfd is in also in related PRs to get each PR compilable separately.

Testing procedure

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

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

Issues/PRs references

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

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 28, 2019
@leandrolanzieri
Copy link
Contributor

Tested on esp32-wroom-32 and works fine. I would still move the esp32-olimex-evb change to a different commit as it actually is not so related to the change of the way it is configured.

@gschorcht
Copy link
Contributor Author

@leandrolanzieri I reverted the commit with esp32-olimex-evb and splitted it.

@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 29, 2019
@leandrolanzieri
Copy link
Contributor

@gschorcht please squash

Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

ACK

@leandrolanzieri leandrolanzieri added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 29, 2019
@gschorcht gschorcht force-pushed the cpu/esp32/periph/conf/dac branch from 43bec86 to bd64629 Compare March 29, 2019 12:13
@gschorcht
Copy link
Contributor Author

@leandrolanzieri Thanks for reviewing and testing. Squashed.

@leandrolanzieri 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 gschorcht force-pushed the cpu/esp32/periph/conf/dac branch from bd64629 to 372f97c Compare May 2, 2019 14:49
@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 2, 2019
@gschorcht gschorcht force-pushed the cpu/esp32/periph/conf/dac branch from 98053e1 to 7e52c57 Compare May 2, 2019 17:37
@leandrolanzieri leandrolanzieri dismissed their stale review May 2, 2019 20:05

Need to retest after review

@leandrolanzieri
Copy link
Contributor

Need to retest after review

I meant rebase >:(

@gschorcht
Copy link
Contributor Author

@leandrolanzieri All PRs are rebased. I'm not sure about the order in which we should merge them. All PRs are compilable and are working separatly. However, all of them require the changes in commit 8ede5be and include this commit therefore.

Therefore, I would suggest to start with PR #11289. After that, I will rebase all of them again.

gschorcht added 4 commits May 6, 2019 13:29
DAC pins are now configured using static arrays in header files instead of static variables in implementation to be able to define DAC_NUMOF using the size of these arrays instead of a variable.
DAC pins are now configured using static arrays in header files instead of static variables in implementation to be able to define DAC_NUMOF using the size of these arrays instead of a variable.
@gschorcht gschorcht force-pushed the cpu/esp32/periph/conf/dac branch from 7e52c57 to 7e8a1c1 Compare May 6, 2019 11:31
@gschorcht
Copy link
Contributor Author

@leandrolanzieri I rebased PRs #11290 to #11294.

Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

ACK

@leandrolanzieri leandrolanzieri merged commit 9075e1d into RIOT-OS:master May 7, 2019
@gschorcht
Copy link
Contributor Author

@leandrolanzieri Thanks.

@gschorcht gschorcht deleted the cpu/esp32/periph/conf/dac branch May 9, 2019 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants