Skip to content

cpu/stm32_common: fix source selection declared as module dependencies (broken) #10153

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.

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

Merged
merged 2 commits into from
Oct 11, 2018

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Oct 11, 2018

Contribution description

This fixes the handling of periph_flashpage for stm32_common. If a module would have required periph_flashpage it 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 module is never used in the code.

This also fixes the i2c handling the same way.

More description in the commit messages.

Testing procedure

Not used that they are declared as modules:

There is no output for

git grep -e MODULE_PERIPH_I2C_1 -e MODULE_PERIPH_I2C_2 -e MODULE_PERIPH_FLASH_COMMON

Flashpage handling

Flashpage uses flash_common

This does not break master handling for periph_flashpage:

Both with master and this PR, flash_common is used

cd tests/periph_flashpage
make BOARD=iotlab-m3 clean all &>/dev/null && test -f bin/iotlab-m3/stm32_common_periph/flash_common.o && echo flash_common_compiled || echo ERROR_no_flash_common
flash_common_compiled
Handling dependency in Makefile.dep

With this diff applied, as if a module depended on it in Makefile.dep, it now works with this PR.

diff --git a/Makefile.dep b/Makefile.dep
index 4c55e2e27..92b939f42 100644
--- a/Makefile.dep
+++ b/Makefile.dep
@@ -788,6 +788,8 @@ ifneq (,$(filter periph_gpio_irq,$(USEMODULE)))
   FEATURES_REQUIRED += periph_gpio
 endif

+FEATURES_REQUIRED += periph_flashpage
+
 # always select gpio (until explicit dependencies are sorted out)
 FEATURES_OPTIONAL += periph_gpio

Compiling in examples/hello-world with the same test command it succeeds with this PR.
With master we get ERROR_no_flash_common

Flashpage works with FEATURES_OPTIONAL

With this PR, it also works with FEATURES_OPTIONAL += periph_flashpage in an application:

cd examples/hello-world
FEATURES_OPTIONAL=periph_flashpage make BOARD=iotlab-m3 clean all &>/dev/null && test -f bin/iotlab-m3/stm32_common_periph/flash_common.o && echo flash_common_compiled || echo ERROR_no_flash_common
flash_common_compiled

With master we get ERROR_no_flash_common

eeprom handling

The same tests can be run by using periph_eeprom feature, BOARD b-l072z-lrwan1 and tests/periph_eeprom.

i2c handling

The same i2c implementation is still used with this PR as with master.

cd tests/periph_i2c
make BOARD=nucleo-f091rc clean all &>/dev/null; ls bin/nucleo-f091rc/stm32_common_periph/i2c_1.o
bin/nucleo-f091rc/stm32_common_periph/i2c_1.o
make BOARD=iotlab-m3 clean all &>/dev/null; ls bin/iotlab-m3/stm32_common_periph/i2c_2.o
bin/iotlab-m3/stm32_common_periph/i2c_2.o

Issues/PRs references

Previous attempt to fix it:

#9892
#9913 (by fixing dependency handling globally)
#10146

Required for PRs

#9969
#8774

`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`.
@cladmi cladmi added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Oct 11, 2018
@cladmi cladmi requested review from vincent-d, aabadie and kYc0o October 11, 2018 13:47
Copy link
Member

@vincent-d vincent-d left a comment

Choose a reason for hiding this comment

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

Looks good and works as expected.

ACK

@vincent-d
Copy link
Member

And go

@vincent-d vincent-d merged commit 746d3e1 into RIOT-OS:master Oct 11, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Oct 11, 2018

@vincent-d Thank you for reviewing

@cladmi cladmi deleted the pr/stm32/fix_source_file_selection branch October 15, 2018 14:59
@cladmi cladmi added this to the Release 2018.10 milestone Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants