Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 7 commits into from
May 1, 2019
Merged

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

merged 7 commits into from
May 1, 2019

Conversation

gschorcht
Copy link
Contributor

Contribution description

This PR changes the handling of periph_* submoduls during compilation for ESP32. Until now, all periph_* submodules were always compiled. #if MODULE_PERIPH_* wrappers were used instead for the following reason:

ADC and DAC channels are implemented by the same hardware controller. Therefore, both peripherals were implemented in the same file periph/adc.c, which in turn prevented the use of $(RIOTMAKE)/periph.mk and periph_ * submodules.

With this PR

  • separate submodules and source files are introduced for ADC and DAC,
  • functionality which is required by ADC and DAC is moved to a new submodule periph_adc_ctrl,
  • required dependencies are added to the makefiles, and
  • print board configuration is adapted accordingly.

Testing procedure

Since the implementation of peripherals did not change with exception of ADC and DAC, successful compilation of tests that use peripherals should be enough.

Due to the changes of ADC and DAC implementation structure, tests/periph_adc and tests/periph_dac should be executed on any ESP32 board with ADC and DAC channels, e.g., esp32-wroom-32.

Issues/PRs references

This PR was initiated by the discussion in PR #11289.

@gschorcht
cpu/esp32: use periph.mk in perpih Makefile
3bad3e6 
Includes now $(RIOTMAKE)/periph.mk instead of $(RIOTBASE)/Makefile.include to control compilation of periph submodules.
@gschorcht gschorcht added Area: cpu Area: CPU/MCU ports Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Apr 3, 2019
@gschorcht gschorcht mentioned this pull request Apr 3, 2019
Merged
leandrolanzieri
leandrolanzieri requested changes Apr 15, 2019
View reviewed changes
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.

Just a few comments, it looks good. I will be testing this shortly.

@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 labels Apr 15, 2019
@leandrolanzieri
Copy link
Contributor

I tested the ADC and DAC on esp32-wroom-32 and works fine.

@leandrolanzieri leandrolanzieri added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Apr 15, 2019
@gschorcht
Copy link
Contributor Author

@leandrolanzieri I addressed all change requests.

leandrolanzieri
leandrolanzieri approved these changes Apr 30, 2019
View reviewed changes
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good. ACK. Please @gschorcht squash!

@leandrolanzieri leandrolanzieri added 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 Apr 30, 2019
@gschorcht gschorcht force-pushed the cpu/esp32/periph/submodules branch from da08c46 to 94a1af3 Compare May 1, 2019 07:40
@gschorcht
Copy link
Contributor Author

@leandrolanzieri Squashed

@gschorcht gschorcht added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 1, 2019
@leandrolanzieri leandrolanzieri merged commit 7c14ff4 into RIOT-OS:master May 1, 2019
@gschorcht
Copy link
Contributor Author

@leandrolanzieri Thanks for reviewing and merging it.

@gschorcht
Copy link
Contributor Author

@leandrolanzieri Next, I will rebase PRs #11289 to #11294

@gschorcht gschorcht deleted the cpu/esp32/periph/submodules branch May 2, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gschorcht @leandrolanzieri