Skip to content

cpu: efm32: throw error on including rtc and rtt together.#8890

Merged
kaspar030 merged 1 commit intoRIOT-OS:masterfrom
basilfx:feature/efm32_rtt_rtc
Apr 12, 2018
Merged

cpu: efm32: throw error on including rtc and rtt together.#8890
kaspar030 merged 1 commit intoRIOT-OS:masterfrom
basilfx:feature/efm32_rtt_rtc

Conversation

@basilfx
Copy link
Copy Markdown
Member

@basilfx basilfx commented Apr 5, 2018 •
edited
Loading

Contribution description

On the EFM32, RTT and RTC use the same hardware peripheral. It doesn't make sense to include both of them, especially if #8543 gets merged.

This PR will throw an error if you include both the RTT and RTC driver.

To test, create/modify an application which includes both, then verify that it will not build.

Issues/PRs references

#8543

@basilfx basilfx added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Apr 5, 2018
@basilfx basilfx requested review from smlng and vincent-d April 5, 2018 18:42
@basilfx basilfx added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 5, 2018
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 5, 2018

Maybe just set FEATURES_CONFLICT += periph_rtc:periph_rtt?

@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Apr 5, 2018

I didn't know of that.

However, it seems that it doesn't work in combination with FEATURES_OPTIONAL. For instance, examples/default includes RTC as optional, and settings FEATURES_REQUIRED to include RTT doesn't throw an error. If I check riotbuild.h, both are included.

@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Apr 10, 2018

@kaspar030 Any idea?

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 10, 2018

@kaspar030 Any idea?

I think we need to fix the feature conflict stuff... @cladmi maybe you could take a look?
@basilfx Is this issue very pressing for you? I propose we keep the PR open, and if feature conflicts are not fixed until beginning of next week, we merge this PR as workaround.

@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Apr 10, 2018

It would become a minor issue once #8543 gets merged. So this can wait for now.

@basilfx basilfx added this to the Release 2018.04 milestone Apr 11, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Apr 11, 2018

I think the problem is that the check is using FEATURES_REQUIRED and not FEATURES_USED. I will verify it would solve it.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Apr 11, 2018

Yes that's it, I will do a PR.

@cladmi cladmi mentioned this pull request Apr 11, 2018
Merged
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Apr 11, 2018

With #8925 you will be able to use FEATURES_CONFLICT and FEATURES_CONFLICT_MSG to show a warning.

They are used for stm32f4discovery for reference https://github.com/RIOT-OS/RIOT/blob/master/boards/stm32f4discovery/Makefile.features#L19

@basilfx basilfx force-pushed the feature/efm32_rtt_rtc branch from 9730855 to bbb0606 Compare April 11, 2018 16:40
@basilfx
Copy link
Copy Markdown
Member Author

basilfx commented Apr 11, 2018

I have updated/rebased this PR to use FEATURES_CONFLICT. It should work whenever #8925 gets merged.

@basilfx basilfx requested a review from cladmi April 11, 2018 16:41
@cladmi cladmi added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 12, 2018
@kaspar030 kaspar030 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 12, 2018
@kaspar030 kaspar030 merged commit 80f97b0 into RIOT-OS:master Apr 12, 2018
maxvankessel pushed a commit to maxvankessel/RIOT that referenced this pull request Apr 20, 2018
@kaspar030 @maxvankessel
Merge pull request RIOT-OS#8890 from basilfx/feature/efm32_rtt_rtc
a454a00 
cpu: efm32: throw error on including rtc and rtt together.
@vincent-d vincent-d mentioned this pull request Jul 10, 2018
Merged
@basilfx basilfx deleted the feature/efm32_rtt_rtc branch January 14, 2020 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vincent-d vincent-d Awaiting requested review from vincent-d

@smlng smlng Awaiting requested review from smlng

1 more reviewer

@cladmi cladmi cladmi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2018.04

Development

Successfully merging this pull request may close these issues.

3 participants

@basilfx @kaspar030 @cladmi