Skip to content

boards/samr21-xpro: Model features in Kconfig#13404

Merged
MrKevinWeiss merged 8 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/boards/samr21-xpro/features_kconfig
Jun 3, 2020
Merged

boards/samr21-xpro: Model features in Kconfig#13404
MrKevinWeiss merged 8 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/boards/samr21-xpro/features_kconfig

Conversation

@leandrolanzieri
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri commented Feb 19, 2020

Contribution description

This PR models the FEATURES_PROVIDED as Kconfig symbols. The idea is, similarly to the current Makefile.features, to select the provided features at every level (i.e. board, CPU model, CPU series, etc.).

In cpu/Kconfig and board/Kconfig I declared common CPU and board symbols, which take their value depending on the selected CPU model and board respectively. Here I proposed to have BOARD, CPU_MODEL, CPU_SERIES, CPU_FAMILY and CPU_ARCH (more specific to less specific), plus CPU which matches the current CPU variable. We should evaluate if that cover our use cases properly.

The selection of symbols starts with the board and goes like:

BOARD_<board> --[select]--> CPU_MODEL_<model> --[select]--> CPU_SERIES_<series> --[select]--> CPU_FAMILY_<family> --[select]--> CPU_ARCH_<arch>

Although this chain of symbols tries to mirror the common symbols, nothing prevents use from using other intermediate symbols (e.g. here I added CPU_ARCH_CORTEX_M0PLUS which selects CPU_ARCH_CORTEX_M).

Done in #13715.

On features

I added cpu/Kconfig.features for the declaration of common feature-like symbols. For features specific to a CPU, board or architecture we can declare them directly on the corresponding Kconfig file. Here I used the HAS_ prefix to indicate features, mostly because it is customary in Kconfig files across projects, but we could use something else.

Done in #14151.

In the future, we could also indicate via this mechanism that, for instance, the CPU_MODEL_SAMD21G18A presents an at86rf233 radio.

Testing procedure

I added the debug-features-provided-kconfig target in dependencies_debug.inc.mk, which should print the same list as info-features-provided, and will allow us to test the migration process. To test it, make sure you set SHOULD_RUN_KCONFIG=1.

Also, one can run make menuconfig and either enable the 'show-all mode' (by pressing a) or search for a particular feature symbol (by presing /), like HAS_PERIPH_GPIO. Once the symbol is selected, one can see more information on the symbol (by pressing ?), like the description of the feature and who selects (provides) it (the board, the cpu model, etc.).

Edit: A test tests/kconfig_features has been added to check that boards provide the same features via Kconfig and via Makefile.features. This is useful during migration process to ensure sync.

Issues/PRs references

Depends on #14151

@leandrolanzieri leandrolanzieri added Type: new feature The issue requests / The PR implemements a new feature for RIOT TF: Config Marks issues and PRs related to the work of the Configuration Task Force Area: Kconfig Area: Kconfig integration labels Feb 19, 2020
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

@aabadie @fjmolinas @cgundogan @jia200x It would be really good to have some feedback on the approach here

@leandrolanzieri leandrolanzieri force-pushed the pr/boards/samr21-xpro/features_kconfig branch from 89faf74 to a3426b5 Compare March 3, 2020 11:11
@leandrolanzieri leandrolanzieri marked this pull request as ready for review March 3, 2020 11:11
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

Moved from draft

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

@jia200x suggested offline to split this PR. I will make one that only adds que common symbols. And this one will add the implementation for samr21-xpro.

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

@jia200x common part can be found here #13715

@leandrolanzieri leandrolanzieri added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 25, 2020
@leandrolanzieri leandrolanzieri removed the State: waiting for other PR State: The PR requires another PR to be merged first label May 15, 2020
@leandrolanzieri leandrolanzieri force-pushed the pr/boards/samr21-xpro/features_kconfig branch from a3426b5 to d6b8c6a Compare May 26, 2020 13:59
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

I'm splitting features declaration in #14151

@leandrolanzieri leandrolanzieri added the State: waiting for other PR State: The PR requires another PR to be merged first label May 26, 2020
@leandrolanzieri leandrolanzieri force-pushed the pr/boards/samr21-xpro/features_kconfig branch from d6b8c6a to c42ea45 Compare May 27, 2020 07:29
@leandrolanzieri leandrolanzieri force-pushed the pr/boards/samr21-xpro/features_kconfig branch 2 times, most recently from 6d2938a to a3ab0fd Compare May 27, 2020 09:42
@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels May 27, 2020
@leandrolanzieri leandrolanzieri force-pushed the pr/boards/samr21-xpro/features_kconfig branch from a3ab0fd to 6eb85e5 Compare May 27, 2020 10:02
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

HAS_WDT_WARNING_PERIOD will need to be renamed so it is not counted as a feature for now.

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

Rebased and renamed the wdt_warning_period feature to the one in master.

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jun 2, 2020

please squash

@leandrolanzieri leandrolanzieri force-pushed the pr/boards/samr21-xpro/features_kconfig branch from 4d12f4a to 7fa48d3 Compare June 2, 2020 15:37
@jia200x jia200x added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 2, 2020
@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jun 2, 2020

I approved the requested changes, but this doesn't count as an ACK because some commits are mine. I will block to avoid accidental merges

jia200x
jia200x previously requested changes Jun 2, 2020
Copy link
Copy Markdown
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

Intentional block to avoid accidental merges :)

@leandrolanzieri leandrolanzieri force-pushed the pr/boards/samr21-xpro/features_kconfig branch from 7fa48d3 to 42e521b Compare June 2, 2020 15:44
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Assuming documentation for HAS_ and MODULE_ get added to the documentation in a separate
PR, I ACK!

@MrKevinWeiss MrKevinWeiss dismissed jia200x’s stale review June 3, 2020 07:39

No need to block anymore.

@MrKevinWeiss MrKevinWeiss merged commit 972d944 into RIOT-OS:master Jun 3, 2020
@leandrolanzieri leandrolanzieri deleted the pr/boards/samr21-xpro/features_kconfig branch June 3, 2020 07:41
@leandrolanzieri leandrolanzieri added this to the Release 2020.07 milestone Jun 3, 2020
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

Thanks all for reviewing and testing!

@kaspar030
Copy link
Copy Markdown
Contributor

Edit: A test tests/kconfig_features has been added to check that boards provide the same features via Kconfig and via Makefile.features. This is useful during migration process to ensure sync.

this now forces all features to be added to both Makefiles and kconfig, right?

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

Yes, it is to keep sync with boards that already have the Kconfig symbols.

@fjmolinas
Copy link
Copy Markdown
Contributor

#13404 (comment) was this addressed? Is HAS reserver for features then?

Comment on lines +1 to +12
include ../Makefile.tests_common

BOARD_WHITELIST += samr21-xpro

kconfig-features:
@bash -c 'diff <($(MAKE) info-features-provided) \
<($(MAKE) dependency-debug-features-provided-kconfig) || \
(echo "ERROR: Kconfig features mismatch" && exit 1)'

all: kconfig-features

include $(RIOTBASE)/Makefile.include
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this test should be extended to cover other variables in makefiles/dependencies_debug.inc.mk that intrinsically related to provided variables, so from what is missing BOARD CPU CPU_MODEL CPU_ARCH CPU_FAM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR TF: Config Marks issues and PRs related to the work of the Configuration Task Force Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants