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.

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

periph_common: add as dependency to periph drivers #9906

Merged
merged 1 commit into from Jun 3, 2019
Merged

periph_common: add as dependency to periph drivers #9906

merged 1 commit into from Jun 3, 2019

Conversation

smlng
Copy link
Member

@smlng smlng commented Sep 7, 2018 •
edited

Contribution description

Rational: the periph_common module is included by each and every CPU/MCU
via one way or the other, hence it can be activated by default. With this
PR its added to the list of default modules.

Actually, the periph_common was missing for CPUs esp8266 and fe310 which would result in I2C or SPI not working for instance, because they rely on periph auto init by periph_common. This is fixed with this PR implicitly -> but requires #9905.

Testing procedure

Issues/PRs references

based on #9905

@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 7, 2018
@smlng smlng removed the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 10, 2018
@kaspar030 kaspar030 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 Sep 10, 2018
@smlng
Copy link
Member Author

smlng commented Sep 10, 2018

rebased, bc #9905 was merged

@smlng
Copy link
Member Author

smlng commented Sep 14, 2018

ola, any comments on this generalisation?

@PeterKietzmann
Copy link
Member

Mabye from @haukepetersen?

@smlng smlng requested review from cladmi and jia200x October 5, 2018 12:52
@cladmi
Copy link
Contributor

cladmi commented Oct 5, 2018

By default it only always adds the definition of periph_init as all the other modules are protected by #ifdef on things that should be defined by the periph config.

In general it makes sense as the dependencies have been implemented using headers macros instead of the build system.
I would add a comment on top of the DEFAULT_MODULE += periph_common about that.

Actually, the periph_common was missing for CPUs esp8266

It is there

RIOT/cpu/esp8266/Makefile.include

Line 122 in e4ca897

USEMODULE += periph_common
and it is using periph_init

Actually, the periph_common was missing for CPUs (…) fe310

It is not using periph_init function so is currently not relying on it.
If periph_init is now included by default, it should be addressed before.

@smlng
Copy link
Member Author

smlng commented Dec 14, 2018

@cladmi rebase and added comment as suggested

@smlng
Copy link
Member Author

smlng commented Dec 19, 2018

ping!

@smlng
Copy link
Member Author

smlng commented Jan 10, 2019

ping again

@smlng
Copy link
Member Author

smlng commented Mar 29, 2019

@cladmi what was the problem here, i.e. are any changes required. Do you agree or disagree with making periph_common a default module?

@smlng smlng mentioned this pull request Mar 29, 2019
Merged
@kaspar030
Copy link
Contributor

It might make more sense to make periph_* depend on periph_common.
Or even be explicit. As far as I can see, only hwrng, i2c, rtc, rtt, spi, timer, pm, cpuid, flashpage, eeprom would make use of this module.

@smlng
Copy link
Member Author

smlng commented Mar 29, 2019 •
edited

It might make more sense to make periph_* depend on periph_common.
Or even be explicit. As far as I can see, only hwrng, i2c, rtc, rtt, spi, timer, pm, cpuid, flashpage, eeprom would make use of this module.

mhm, sounds reasonable, too - my observation/point was that it doesn't make sense to have USEMODULE += periph_common explicitly added by all CPUs independently and rather move it to a central location. However, you're right instead of making it a default module we could add a rule as you stated above more [i.e. use] periph_% to pull in periph_common.

@smlng
Copy link
Member Author

smlng commented Mar 29, 2019

@kaspar030 add a fixup with your suggestion, need to reword the commit message when squashing

@kaspar030
Copy link
Contributor

Now there might be a problem when no periph_ is actually used, as then theres no periph_init(), but that is hard-called in all architectures. Unfortunately the call cannot easily be moved into e.g., kernel_init(), as some platforms don't call it last in their startup sequence...

Maybe that is not a problem. It is not with the current codebase.

@smlng
Copy link
Member Author

smlng commented Mar 29, 2019

well that hard-coded call of periph_init (and other stuff) is a problem in itself I'd say and should be fixed/changed in the long run, too. But for now, RIOT always pulls in some periph_ module (eg timer) and hence also periph_common.

So I would go for this solution, also (thanks to you suggestion to make it a dependency) its much cleaner and more appropriate way to add USEMODULE += periph_common in Makefile.dep compared to adding it by each and every CPU.

@kaspar030
Copy link
Contributor

So I would go for this solution

+1, one step at a time. :)

@smlng
Copy link
Member Author

smlng commented Mar 29, 2019

can/shall I squash?

@kaspar030
Copy link
Contributor

yes please. @cladmi wanna take another look?

@smlng
Copy link
Member Author

smlng commented Mar 29, 2019

done, ready to merge ...

@cladmi
Copy link
Contributor

cladmi commented Apr 4, 2019

Currently it makes that periph_common depends on periph_common but should not break anything :D
This could be changed in another PR by renaming it to periph-common instead this way it could differentiate from periph_ pseudomodules. It will be easier as there should be only one place using the name periph_common after this PR.

I agree with he given justification in the commit message and the current changes done.

However, I think that now there are other places that might need the change too:

git grep 'USEMODULE += periph_common'
Makefile.dep:  USEMODULE += periph_common
cpu/esp8266/Makefile.include:USEMODULE += periph_common
cpu/fe310/Makefile.include:USEMODULE += periph_common
makefiles/arch/atmega.inc.mk:USEMODULE += periph_common

@smlng smlng modified the milestone: Release 2019.04 Apr 8, 2019
@smlng
Copy link
Member Author

smlng commented Apr 8, 2019

@cladmi comments addressed, squash?

@smlng smlng force-pushed the pr/default_periph_common branch from 66a2cb1 to 13a6735 Compare May 29, 2019 09:23
@smlng
Copy link
Member Author

smlng commented May 29, 2019

@cladmi how to proceed? Can I squash?

@cladmi
Copy link
Contributor

cladmi commented Jun 3, 2019

Please squash, it got lost in my mailbox.

@smlng
periph_common: add as dependency to periph drivers
2de4b30 
Rational: the periph_common module is required by (most) other periph drivers
and also during startup of the CPU/MCU to run periph_init. The latter is only
required if other periph drivers are used, hence periph_common should be a
depency of periph_* modules and *not* of the CPU/MCU. This PR fixes that
by making periph_common a depency of periph_* and removing the explicit
include in the CPU/MCU implementation.
@smlng smlng force-pushed the pr/default_periph_common branch from 9c9f130 to 2de4b30 Compare June 3, 2019 11:44
@smlng
Copy link
Member Author

smlng commented Jun 3, 2019

done!

@cladmi cladmi changed the title periph_common: add as default module periph_common: add as dependency to periph drivers Jun 3, 2019
cladmi
cladmi approved these changes Jun 3, 2019
View reviewed changes
Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

ACK.

Many periphs use the periph_common implementation without setting the dependency.

Also, even if periph_init is called by cpu it makes sense that it is a dependency of periph anyway.

@cladmi cladmi merged commit 9bbc652 into RIOT-OS:master Jun 3, 2019
@cladmi
Copy link
Contributor

cladmi commented Jun 3, 2019

Forgot in the Approve comment, I checked that the branch rebased on master only had this dependency definition to periph_common:

git grep 'USEMODULE += periph_common'
Makefile.dep:  USEMODULE += periph_common

@MrKevinWeiss MrKevinWeiss added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cladmi cladmi cladmi approved these changes

@jnohlgard jnohlgard Awaiting requested review from jnohlgard

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@jia200x jia200x Awaiting requested review from jia200x

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@smlng @PeterKietzmann @cladmi @kaspar030 @MrKevinWeiss