cpu/stm32_common: fix source selection declared as module dependencies (broken)#10153
Merged
vincent-d merged 2 commits intoRIOT-OS:masterfrom Oct 11, 2018
Merged
Conversation
`cpu/stm32_common/Makefile.dep` was never included by the global `Makefile.dep`, so declaring this `USEMODULE +=` here was never shown to the build system and never exported as `MODULE_PERIPH_I2C_X` variables. In fact, `USEMODULE` was only used to trigger the `periph.mk`/`SUBMODULES` mechanism to add matching source files names to `SRC` and so select the implementation for each CPU type. It is replaced by just explicitly selecting the right source file.
The `periph_flash_common` feature was only defined here to trigger inclusion of a source file with common functions. It even only defines private symbols `_lock` and `_unlock` so no reason to expose it to the build system. And in practice, all stm cpus providing `periph_flashpage` or `periph_eeprom` were required to provide `periph_flash_common` to allow including it. The previous implementation was only parsing in the modules were in `FEATURES_REQUIRED` wich did not take cases of `FEATURES_OPTIONAL` into account. And also, in the same time, as the dependencies was declared in `Makefile.include` it was processed before `Makefile.dep` so never handled cases where a module could depend on `periph_flashpage` or `periph_eeprom` feature. It is replaced by selecting the common source file when module using it are included. The now useless feature `periph_flash_common` is removed from `FEATURES_PROVIDED`.
vincent-d
approved these changes
Oct 11, 2018
Member
vincent-d
left a comment
There was a problem hiding this comment.
Looks good and works as expected.
ACK
Member
|
And go |
Contributor
Author
|
@vincent-d Thank you for reviewing |
This was referenced Oct 17, 2018
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution description
This fixes the handling of
periph_flashpageforstm32_common. If a module would have requiredperiph_flashpageit would not have worked and if an application would have said declared an optional feature it would not have worked either.The does not fix the global handling of CPU dependencies but replaces using modules/periphs to declare module internal only selection of source files to only source file inclusion.
The fact that they are described as
moduleis never used in the code.This also fixes the
i2chandling the same way.More description in the commit messages.
Testing procedure
Not used that they are declared as modules:
There is no output for
Flashpage handling
Flashpage uses
flash_commonThis does not break master handling for
periph_flashpage:Both with master and this PR,
flash_commonis usedHandling dependency in
Makefile.depWith this diff applied, as if a module depended on it in
Makefile.dep, it now works with this PR.Compiling in
examples/hello-worldwith the same test command it succeeds with this PR.With master we get
ERROR_no_flash_commonFlashpage works with FEATURES_OPTIONAL
With this PR, it also works with
FEATURES_OPTIONAL += periph_flashpagein an application:With master we get
ERROR_no_flash_commoneeprom handling
The same tests can be run by using
periph_eepromfeature, BOARDb-l072z-lrwan1andtests/periph_eeprom.i2c handling
The same
i2cimplementation is still used with this PR as with master.Issues/PRs references
Previous attempt to fix it:
#9892
#9913 (by fixing dependency handling globally)
#10146
Required for PRs
#9969
#8774