pkg/uwb*: add Kconfig dependency modelling by fjmolinas · Pull Request #16780 · RIOT-OS/RIOT

fjmolinas · 2021-08-27T08:12:18Z

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)

leandrolanzieri

Some initial comments

leandrolanzieri · 2021-08-27T08:19:00Z

cpu/nrf5x_common/Kconfig

+if TEST_KCONFIG
+
+config MODULE_CPU_COMMON
+    bool "Link nrf5x common code"


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

Suggested change

bool "Link nrf5x common code"

bool

leandrolanzieri · 2021-08-27T08:19:58Z

cpu/nrf5x_common/periph/Kconfig.nrf5x

+    depends on HAS_VDD_LC_FILTER_REG0
+    default y if HAS_VDD_LC_FILTER_REG0


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

leandrolanzieri · 2021-08-27T08:20:08Z

cpu/nrf5x_common/periph/Kconfig.nrf5x

+config MODULE_VDD_LC_FILTER_REG1
+    bool
+    depends on HAS_VDD_LC_FILTER_REG1
+    default y if HAS_VDD_LC_FILTER_REG1


Suggested change

default y if HAS_VDD_LC_FILTER_REG1

default y

leandrolanzieri · 2021-08-27T08:22:12Z

pkg/mynewt-core/Kconfig

+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"


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?

pkg/mynewt-core/Kconfig

pkg/uwb-core/Kconfig

pkg/uwb-dw1000/Kconfig

fjmolinas · 2021-08-27T09:24:23Z

Will trigger murdock once

fjmolinas · 2021-08-27T22:21:34Z

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 · 2021-08-31T08:19:29Z

ping @leandrolanzieri

leandrolanzieri · 2021-09-06T09:51:00Z

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 · 2021-09-06T17:00:56Z

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?

fjmolinas · 2021-09-30T08:35:37Z

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

fjmolinas · 2021-09-30T11:52:11Z

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

sema currently does not depend on xtimer, but on xtimer or ztimer

fjmolinas · 2021-10-27T08:51:08Z

@leandrolanzieri ok to squash?

fjmolinas · 2021-10-27T08:51:24Z

@leandrolanzieri ok to squash?

Anything pending to get this in?

MrKevinWeiss · 2021-10-29T09:58:21Z

as the fixup is pretty small please squash!

MrKevinWeiss · 2021-10-29T11:00:38Z

examples/twr_aloha/app.config.test

@@ -0,0 +1,13 @@
+CONFIG_PACKAGE_UWB-CORE=y
+CONFIG_PACKAGE_MYNEWT-CORE=y


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

leandrolanzieri

Looks good, thanks @fjmolinas for this one! ACK

fjmolinas · 2021-11-01T09:06:27Z

Thanks for the review @leandrolanzieri!!

fjmolinas added the Area: Kconfig Area: Kconfig integration label Aug 27, 2021

fjmolinas requested review from MrKevinWeiss, aabadie and leandrolanzieri August 27, 2021 08:12

fjmolinas requested review from cgundogan, haukepetersen, jia200x and kaspar030 as code owners August 27, 2021 08:12

fjmolinas mentioned this pull request Aug 27, 2021

cpu/cc2538: Add Kconfig support #16719

Merged

leandrolanzieri reviewed Aug 27, 2021

View reviewed changes

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 added 3 commits October 6, 2021 17:59

sys/sema/Kconfig: fix inclusion of xtimer

9cb27b9

sema currently does not depend on xtimer, but on xtimer or ztimer

pkg/mynewt-core: add Kconfig module dependency

b21191f

pkg/uwb-core: add Kconfig module dependency

a04b892

fjmolinas force-pushed the pr_kconfig_uwb branch from 87d1654 to 475cedf Compare October 6, 2021 15:59

pkg/uwb-dw1000: add Kconfig module dependency

cbd1277

fjmolinas force-pushed the pr_kconfig_uwb branch from 8b73c86 to 3d38765 Compare October 29, 2021 10:24

MrKevinWeiss reviewed Oct 29, 2021

View reviewed changes

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

examples/twr_aloha: add Kconfig

8efe255

fjmolinas force-pushed the pr_kconfig_uwb branch from 5377f28 to 8efe255 Compare October 29, 2021 11:12

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 approved these changes Nov 1, 2021

View reviewed changes

leandrolanzieri added this to the Release 2022.01 milestone Nov 1, 2021

leandrolanzieri merged commit 4011bd9 into RIOT-OS:master Nov 1, 2021

fjmolinas deleted the pr_kconfig_uwb branch November 1, 2021 09:06

		depends on HAS_VDD_LC_FILTER_REG0
		default y if HAS_VDD_LC_FILTER_REG0

		@@ -0,0 +1,13 @@
		CONFIG_PACKAGE_UWB-CORE=y
		CONFIG_PACKAGE_MYNEWT-CORE=y

Conversation

fjmolinas commented Aug 27, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Aug 27, 2021

Uh oh!

fjmolinas commented Aug 27, 2021

Uh oh!

fjmolinas commented Aug 31, 2021

Uh oh!

leandrolanzieri commented Sep 6, 2021

Uh oh!

fjmolinas commented Sep 6, 2021

Uh oh!

fjmolinas commented Sep 30, 2021

Uh oh!

fjmolinas commented Sep 30, 2021

Uh oh!

fjmolinas commented Oct 27, 2021

Uh oh!

fjmolinas commented Oct 27, 2021

Uh oh!

MrKevinWeiss commented Oct 29, 2021

Uh oh!

MrKevinWeiss Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Nov 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants