Skip to content

pkg/uwb*: add Kconfig dependency modelling #16780

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.

Sign up for GitHub

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
Merged
merged 6 commits into from
Nov 1, 2021
Merged

pkg/uwb*: add Kconfig dependency modelling #16780

merged 6 commits into from
Nov 1, 2021

Conversation

fjmolinas
Copy link
Contributor

Contribution description

This PR models dependency in Kconfig related to uwb-core/uwb-dw1000 package, since this runs only on dwm1001 so far this PR goes throgh modelling dependencies for:

  • mynewt-core
  • uwb-core
  • uwb-dw1000
  • nrf52 (excluding netif related dependencies)

FEEDBACK wanted:

  • I abused select for the packages, I think its fine since those titely coupled
  • the sema case its not too clear, there is no clear dependency in the current modeling, but the code would have an issue if neither xtimer or ztimer are included, should I leave it as is? Maybe add a comment?
  • not sure if everything is in place for murdock to kconfig test....

Testing procedure

Binaries are the same:

  • kconfig
   text	   data	    bss	    dec	    hex	filename
  50688	    776	   8552	  60016	   ea70	/home/francisco/workspace/RIOT/examples/twr_aloha/bin/dwm1001/twr-aloha.elf
  • make:
   text	   data	    bss	    dec	    hex	filename
  50688	    776	   8552	  60016	   ea70	/home/francisco/workspace/RIOT/examples/twr_aloha/bin/dwm1001/twr-aloha.elf

Issues/PRs references

Depends on #16719 (for the DEVELHELP commit)

@fjmolinas fjmolinas added the Area: Kconfig Area: Kconfig integration label Aug 27, 2021
@github-actions github-actions bot added Area: boards Area: Board ports Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications Area: pkg Area: External package ports Area: sys Area: System Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Aug 27, 2021
@fjmolinas fjmolinas mentioned this pull request Aug 27, 2021
Merged
leandrolanzieri
leandrolanzieri reviewed Aug 27, 2021
View reviewed changes
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Some initial comments

cpu/nrf5x_common/Kconfig Outdated
if TEST_KCONFIG

config MODULE_CPU_COMMON
bool "Link nrf5x common code"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that this needs a prompt, as the user would not disable this one:

Suggested change
bool "Link nrf5x common code"
bool

cpu/nrf5x_common/periph/Kconfig.nrf5x Outdated
Comment on lines 16 to 17
depends on HAS_VDD_LC_FILTER_REG0
default y if HAS_VDD_LC_FILTER_REG0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
depends on HAS_VDD_LC_FILTER_REG0
default y if HAS_VDD_LC_FILTER_REG0
depends on HAS_VDD_LC_FILTER_REG0
default y

cpu/nrf5x_common/periph/Kconfig.nrf5x Outdated
config MODULE_VDD_LC_FILTER_REG1
bool
depends on HAS_VDD_LC_FILTER_REG1
default y if HAS_VDD_LC_FILTER_REG1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
default y if HAS_VDD_LC_FILTER_REG1
default y

pkg/mynewt-core/Kconfig Outdated
Comment on lines 19 to 36
config MODULE_MYNEWT-CORE
bool "Apache MyNewy mynewt-core RIOT implementation"

config MODULE_AUTO_INIT_MYNEWT-CORE
bool "Auto-initialize the mynewt-core package"
default y
depends on MODULE_AUTO_INIT

config MODULE_MYNEWT-CORE_OS
bool "mynewt-core kernel module"

config MODULE_MYNEWT-CORE_UTIL
bool "mynewt-core utilities modules"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all these modules optional? Meaning the user should be able to opt-out? or is it contrib? Also, maybe we could make PACKAGE_MYNEWT-CORE a menuconfig and make all these depend on it? would it be correct from the dependency point of view?

@fjmolinas
Copy link
Contributor Author

Will trigger murdock once

@fjmolinas fjmolinas added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 27, 2021
@fjmolinas
Copy link
Contributor Author

There is one dependency I'm not sure how to map since mtd_% select mtd, should I remove them?

ifneq (,$(filter mtd,$(USEMODULE)))
  USEMODULE += mtd_spi_nor
endif

@fjmolinas
Copy link
Contributor Author

ping @leandrolanzieri

@leandrolanzieri
Copy link
Contributor

There is one dependency I'm not sure how to map since mtd_% select mtd, should I remove them?

ifneq (,$(filter mtd,$(USEMODULE)))
  USEMODULE += mtd_spi_nor
endif

It could be that the current modelling is not correct. Is the user supposed to select the mtd interface type (SPI NOR, SD-Card, etc.), or just the mtd module and the interface is deduced from the board?

@fjmolinas
Copy link
Contributor Author

There is one dependency I'm not sure how to map since mtd_% select mtd, should I remove them?

ifneq (,$(filter mtd,$(USEMODULE)))
  USEMODULE += mtd_spi_nor
endif

It could be that the current modelling is not correct. Is the user supposed to select the mtd interface type (SPI NOR, SD-Card, etc.), or just the mtd module and the interface is deduced from the board?

@benpicco @bergzand you have touched mtd If I recall correctly, any opinions?

@github-actions github-actions bot removed Area: drivers Area: Device drivers Area: tests Area: tests and testing framework Area: boards Area: Board ports Area: CI Area: Continuous Integration of RIOT components labels Sep 30, 2021
@fjmolinas
Copy link
Contributor Author

Rebased and squashed... maybe you can give it a clean look @leandrolanzieri?

@fjmolinas
Copy link
Contributor Author

The sema issue relates to #16891, will think of a solution for that...

@fjmolinas
Copy link
Contributor Author

@leandrolanzieri ok to squash?

@fjmolinas
Copy link
Contributor Author

@leandrolanzieri ok to squash?

Anything pending to get this in?

@MrKevinWeiss
Copy link
Contributor

as the fixup is pretty small please squash!

MrKevinWeiss
MrKevinWeiss reviewed Oct 29, 2021
View reviewed changes
examples/twr_aloha/app.config.test Outdated
@@ -0,0 +1,13 @@
CONFIG_PACKAGE_UWB-CORE=y
CONFIG_PACKAGE_MYNEWT-CORE=y
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need this? the Makefile doesn't have it and it seems to be brought in anyways

@fjmolinas fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 29, 2021
@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 29, 2021
leandrolanzieri
leandrolanzieri approved these changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@leandrolanzieri leandrolanzieri 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, thanks @fjmolinas for this one! ACK

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Nov 1, 2021
@leandrolanzieri leandrolanzieri added this to the Release 2022.01 milestone Nov 1, 2021
@leandrolanzieri leandrolanzieri merged commit 4011bd9 into RIOT-OS:master Nov 1, 2021
@fjmolinas fjmolinas deleted the pr_kconfig_uwb branch November 1, 2021 09:06
@fjmolinas
Copy link
Contributor Author

Thanks for the review @leandrolanzieri!!

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

@MrKevinWeiss MrKevinWeiss MrKevinWeiss left review comments

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

@aabadie aabadie Awaiting requested review from aabadie

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner

@smlng smlng Awaiting requested review from smlng

Assignees
No one assigned
Labels
Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Milestone
Release 2022.01
Development

Successfully merging this pull request may close these issues.
3 participants
@fjmolinas @leandrolanzieri @MrKevinWeiss