Skip to content

make: reorganize makefile features#7880

Merged
haukepetersen merged 18 commits intoRIOT-OS:masterfrom
kaspar030:reorganize_makefile_features
Nov 2, 2017
Merged

make: reorganize makefile features#7880
haukepetersen merged 18 commits intoRIOT-OS:masterfrom
kaspar030:reorganize_makefile_features

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 commented Oct 26, 2017

This PR implements a chain Makefile.features taking any cpu, shared board and shared cpu file into account.

@kaspar030 kaspar030 added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Oct 26, 2017
haukepetersen
haukepetersen requested changes Oct 27, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

Nice one, I like it, and I would say let's roll with this one!

From my perspective I would love to see the full adaption of this PR so we can do a full review. As everything is WIP here, consider my comments more as a 'don't forget' flag instead of a real review...

boards/arduino-atmega-common/Makefile.features Outdated
FEATURES_PROVIDED += periph_spi
FEATURES_PROVIDED += periph_timer
FEATURES_PROVIDED += periph_uart
FEATURES_PROVIDED += periph_pm
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Oct 27, 2017

Choose a reason for hiding this comment

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

should be moved to the (possible shared) CPU features file (as implemented for all atmega CPUs, right?)

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Oct 28, 2017

Choose a reason for hiding this comment

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

done

boards/f4vi1/Makefile.features
# Put defined MCU peripherals here (in alphabetical order)
FEATURES_PROVIDED += periph_timer
FEATURES_PROVIDED += periph_uart
FEATURES_PROVIDED += periph_gpio
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Oct 27, 2017

Choose a reason for hiding this comment

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

We could also move periph_gpio feature entries from all stm or even cortexm based boards to the CPU files, as these are not dependent on any board specific configuration and they are implemented for all these CPUs

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Oct 28, 2017

Choose a reason for hiding this comment

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

My understanding was that the feature being available also depends on the board's configuration in periph_conf.h and thus might override a CPU definition.

cpu/cortexm_common/Makefile.features
@@ -0,0 +1 @@
FEATURES_PROVIDED += cpp
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Oct 27, 2017

Choose a reason for hiding this comment

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

Then this PR is missing the removal of all the cpp entries from all the cortexm boards, but this is part of the WIP right?

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Oct 28, 2017

Choose a reason for hiding this comment

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

correct!

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Oct 28, 2017

Choose a reason for hiding this comment

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

done

@kaspar030 kaspar030 force-pushed the reorganize_makefile_features branch from fddafc5 to cf6a4a2 Compare October 28, 2017 16:06
@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 Oct 28, 2017
@kaspar030 kaspar030 force-pushed the reorganize_makefile_features branch 2 times, most recently from 0865ee8 to cc8a322 Compare October 28, 2017 18:44
@kaspar030 kaspar030 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 28, 2017
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Oct 28, 2017

Ready for review.

@kaspar030 kaspar030 mentioned this pull request Oct 28, 2017
Merged
jnohlgard
jnohlgard previously requested changes Oct 29, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

It seems cpuid and hwrng are missing from kinetis_common

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Oct 30, 2017

It seems cpuid and hwrng are missing from kinetis_common

Yes, thanks. Fixed.

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Oct 30, 2017

Just wrote and used this test to check for changes in supported boards due to feature changes:
https://github.com/haukepetersen/riotsandbox/blob/master/tools/suptest/suptest.py
(Running it for master and for this branch, dumping the output in two files and looking at the diff of the two files)

Found that this PR introduces a changed output for 'make info-boards-supported' for a number of tests. See this gist for the diff: https://gist.github.com/haukepetersen/d295ff0bd76bd876a1ab38fd8930681c

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Oct 30, 2017

ups, the diff does not show the test names, so here is the raw data: https://gist.github.com/haukepetersen/fb6b50cff132e99790929976a6b4b8a7

@kaspar030 kaspar030 force-pushed the reorganize_makefile_features branch from 9aca9c9 to 50dcd09 Compare October 30, 2017 11:46
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Oct 30, 2017

I think I got them all. Actually, some more tests are build now (probably some features were not specified before.

btw, I used

#!/bin/sh

for dir in examples/* tests/*; do
    [ -d "$dir" ] && {
        for board in $(make --no-print-directory -C $dir info-boards-supported || echo ERROR); do
            echo $dir $board
        done
    }
done

on both master and this branch, and compared the output with

diff -U0 master.txt reorganize_makefile_features.txt

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Nov 2, 2017

notes for follow ups (do not need to be addressed in this PR):

  • eliminate FEATURES_MCU_GROUP defines (or at least move them to CPU families)
  • move FEATURES_PROVIDED += periph_gpio to CPU

haukepetersen
haukepetersen approved these changes Nov 2, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

Looked through all the changes once more, everything looks valid to me. Also analyzed the diff between info-boards-supported for all tests/examples between master and this branch. All differences lead to more boards being tested now, so no problem here (actually in the contrary...).

-> ACK

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Nov 2, 2017

Oh: and you might want to sqash?! :-)

@kaspar030 kaspar030 force-pushed the reorganize_makefile_features branch from 7c63219 to 0898bf5 Compare November 2, 2017 11:59
@kaspar030 kaspar030 force-pushed the reorganize_makefile_features branch from 0898bf5 to f7f3b68 Compare November 2, 2017 11:59
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Nov 2, 2017

  • squashed

@haukepetersen I kept the cpu reorganiation in seperate commits. That fine with you?

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Nov 2, 2017

It seems cpuid and hwrng are missing from kinetis_common

This issue is addressed. So @gebart hope you are not mad that I dismiss your review :-)

@haukepetersen haukepetersen dismissed jnohlgard’s stale review November 2, 2017 12:10

comment is addressed

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Nov 2, 2017

All good -> go

@haukepetersen haukepetersen merged commit 36dd0c9 into RIOT-OS:master Nov 2, 2017
@haukepetersen haukepetersen deleted the reorganize_makefile_features branch November 2, 2017 12:10
@kaspar030 kaspar030 mentioned this pull request Nov 2, 2017
Closed
@haukepetersen haukepetersen mentioned this pull request Nov 3, 2017
Merged
@basilfx basilfx mentioned this pull request Nov 5, 2017
Merged
@cladmi cladmi mentioned this pull request Nov 16, 2017
Closed
@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haukepetersen haukepetersen haukepetersen approved these changes

@jnohlgard jnohlgard jnohlgard left review comments

Assignees

@haukepetersen haukepetersen

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2018.01

Development

Successfully merging this pull request may close these issues.

4 participants

@kaspar030 @haukepetersen @jnohlgard @aabadie