.murdock: Add nucleo boards to kconfig test #16845
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is just a check to see how much is needed for the other stms... I expect some errors. |
f4ddf85 to
65e9b0c
Compare
|
currently there are differences with the clock in the nucleo-wx boards. TEST_KCONFIG=0 TEST_KCONFIG=1 However a simple test shows that no difference. It must be something else. |
|
Think I found it... |
|
Hmmm so here are the issues I see with this PR:
It might be good to get an opinion from @aabadie regarding from BOARD_HAS_HSE to HAVE_CLK_HSE or something... |
|
Regarding the errors in the diff --git a/drivers/periph_common/Kconfig.spi b/drivers/periph_common/Kconfig.spi
index 05252b45bc..eca91e5dca 100644
--- a/drivers/periph_common/Kconfig.spi
+++ b/drivers/periph_common/Kconfig.spi
@@ -20,13 +20,25 @@ config MODULE_PERIPH_SPI_RECONFIGURE
bool "Pin reconfiguration support"
depends on HAS_PERIPH_SPI_RECONFIGURE
-# TODO: this module is actually just an artifact from the way periph_init_%
-# modules are handled in Makefile. We need to define it to keep the list the
-# same for now. We should be able to remove it later on.
+config MODULE_PERIPH_SPI_GPIO_MODE
+ bool "Support initializing SPI pins with adapted GPIO modes"
+ depends on HAS_PERIPH_SPI_GPIO_MODE
+ help
+ Say y to call `spi_init_with_gpio_mode`, which allows to initialize the SPI pins in
+ with an specific GPIO mode.
+
+# TODO: these modules are actually just artifacts from the way periph_init_%
+# modules are handled in Makefile. We need to define them to keep the list the
+# same for now. We should be able to remove them later on.
config MODULE_PERIPH_INIT_SPI_RECONFIGURE
bool "Auto initialize SPI pin reconfiguration support"
default y if MODULE_PERIPH_INIT
depends on MODULE_PERIPH_SPI_RECONFIGURE
+config MODULE_PERIPH_INIT_SPI_GPIO_MODE
+ bool
+ default y if MODULE_PERIPH_INIT
+ depends on MODULE_PERIPH_SPI_GPIO_MODE
+
endif # MODULE_PERIPH_SPI |
|
And the RTC mem module is missing: diff --git a/drivers/periph_common/Kconfig.rtc b/drivers/periph_common/Kconfig.rtc
index 45bd98fe3b..c45ebb4752 100644
--- a/drivers/periph_common/Kconfig.rtc
+++ b/drivers/periph_common/Kconfig.rtc
@@ -10,10 +10,29 @@ config MODULE_PERIPH_RTC
depends on HAS_PERIPH_RTC
select MODULE_PERIPH_COMMON
+if MODULE_PERIPH_RTC
+
config MODULE_PERIPH_INIT_RTC
bool "Auto initialize RTC peripheral"
default y if MODULE_PERIPH_INIT
- depends on MODULE_PERIPH_RTC
+
+config MODULE_PERIPH_RTC_MEM
+ bool "Low-Power RTC Memory"
+ depends on HAS_PERIPH_RTC_MEM
+ help
+ Provides an interface to access low-power memory present on some RTCs.
+ This memory is retained even when the rest of the system is powered off.
+
+# TODO: this module is actually just an artifact from the way periph_init_%
+# modules are handled in Makefile. We need to define it to keep the list the
+# same for now. We should be able to remove it later on.
+
+config MODULE_PERIPH_INIT_RTC_MEM
+ bool
+ default y if MODULE_PERIPH_INIT
+ depends on MODULE_PERIPH_RTC_MEM
+
+endif # MODULE_PERIPH_RTC
# Include CPU specific configurations
osource "$(RIOTCPU)/$(CPU)/periph/Kconfig.rtc"
diff --git a/tests/periph_rtc/Kconfig b/tests/periph_rtc/Kconfig
new file mode 100644
index 0000000000..6384225e04
--- /dev/null
+++ b/tests/periph_rtc/Kconfig
@@ -0,0 +1,11 @@
+# Copyright (c) 2021 HAW Hamburg
+#
+# This file is subject to the terms and conditions of the GNU Lesser
+# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+#
+
+config APPLICATION
+ bool
+ default y
+ imply MODULE_PERIPH_RTC_MEM
diff --git a/tests/periph_rtc/Makefile b/tests/periph_rtc/Makefile
index c717733c39..eef8bb11ac 100644
--- a/tests/periph_rtc/Makefile
+++ b/tests/periph_rtc/Makefile
@@ -8,4 +8,7 @@ DISABLE_MODULE += periph_init_rtc
USEMODULE += xtimer
+# avoid running Kconfig by default
+SHOULD_RUN_KCONFIG ?=
+
include $(RIOTBASE)/Makefile.include
diff --git a/tests/periph_rtt/Kconfig b/tests/periph_rtt/Kconfig
index a2d35dbf35..adca5a3148 100644
--- a/tests/periph_rtt/Kconfig
+++ b/tests/periph_rtt/Kconfig
@@ -9,4 +9,6 @@ config APPLICATION
bool
default y
imply MODULE_PERIPH_RTT_SET_COUNTER
+ imply MODULE_PERIPH_RTC
+ imply MODULE_PERIPH_RTC_MEM
depends on TEST_KCONFIG
diff --git a/tests/periph_rtt/Makefile b/tests/periph_rtt/Makefile
index 75b7c29ac3..288072512b 100644
--- a/tests/periph_rtt/Makefile
+++ b/tests/periph_rtt/Makefile
@@ -7,6 +7,9 @@ FEATURES_OPTIONAL += periph_rtc_mem
DISABLE_MODULE += periph_init_rtt
+# avoid running Kconfig by default
+SHOULD_RUN_KCONFIG ?=
+
include $(RIOTBASE)/Makefile.include
# Put board specific dependencies hereStill, the binary is not equal at least for the nucleo-l073rz, could it be something with the clocks as well? |
|
Now that things are passing the CI we can discuss if it is better to have the board/cpu specific module requirements in the selection of the board/cpu or if they should be flagged and done in the module. If everyone is happy then I will cleanup the commits (maybe split into other PRs). Note that the nucleo-wl55 is not included as the clocks need to be modelled still. |
fjmolinas
reviewed
Sep 23, 2021
| @@ -23,6 +23,8 @@ config BOARD_NUCLEO_L4R5ZI | |||
| select HAS_PERIPH_TIMER | |||
| select HAS_PERIPH_UART | |||
|
|
|||
| select MODULE_PERIPH_LPUART if MODULE_STDIO_UART && HAS_PERIPH_LPUART | |||
There was a problem hiding this comment.
Is the && HAS_PERIPH_LPUART needed here? Its already selected above?
There was a problem hiding this comment.
I suspect this will also lead to uart and lpuar being included.
There was a problem hiding this comment.
The && doesn't need to be added as the board config is where the HAS_PERIPH_LPUART is.
I wanted to add that to make it a bit more verbose as it is a requirement, though the requirement is both given and consumed within the one symbol.
PERIPH_UART must be selected anyway for the MODULE_PERIPH_LPUART to even be exposed...
Would that be an additional requirement, or, since it is 'if'ed out it would throw an error if the MODULE_PERIPH_LPUART was trying to be selected but there was no MODULE_PERIPH_UART?
and I guess this is where things get confusing.
MODULE_PERIPH_LPUART depends on MODULE_PERIPH_UART
MODULE_STDIO_UART selects MODULE_PERIPH_UART
therefore if MODULE_STDIO_UART selects MODULE_PERIPH_LPUART, we can assume MODULE_PERIPH_UART is selected.
However, if, for some reason, MODULE_STDIO_UART no longer selects MODULE_PERIPH_UART that would break that dependency.
I don't think there is a super clean solution.
I can remove the HAS_PERIPH_LPUART as it is declared above and this should only exist in boards that PERIPH_LPUART anyways and we can trust MODULE_STDIO_UART to being in all required modules and dependencies?
3f783ce to
6311be3
Compare
sys/ztimer/Kconfig
Outdated
Comment on lines
78
to
80
| !CPU_COMMON_SAM0 && \ | ||
| !CPU_COMMON_EFM32 && \ | ||
| !CPU_FAM_F1 | ||
| !CPU_STM32 |
There was a problem hiding this comment.
I don't think we need these, now ztimer_sec is modelled in Makefile.dep. Native should be kept though AFAIU.
There was a problem hiding this comment.
Hmm so it seems that this was here to prevent the selection of RTC in platforms where it conflicts with RTT. As we have now 4 occurrences of this, maybe make a hidden symbol out of it instead of just enlarging this list?
afb3854 to
283f5eb
Compare
|
May I squash? |
12c6682 to
9b6a63a
Compare
|
Anything missing here @leandrolanzieri ? |
✅ frdm-k22f
✅ nucleo-f091rc
✅ nucleo-f303re
✅ same54-xpro
✅ nucleo-f767zi
✅ nucleo-l152re
✅ saml21-xpro
✅ arduino-mega2560
✅ nucleo-l432kc
✅ hifive1b
✅ saml10-xpro
✅ saml11-xpro
✅ samr30-xpro
✅ samr21-xpro
✅ samr34-xpro
✅ nrf52dk
✅ remote-revb
✅ nucleo-f207zg
✅ nucleo-l073rz
✅ slstk3401a
✅ nucleo-g474re
✅ nucleo-f411re
✅ nucleo-f103rb
✅ frdm-kw41z
✅ esp8266-esp-12x
✅ esp32-wroom-32
✅ arduino-due
✅ slstk3400a
✅ stk3200
|
✅ frdm-k22f
✅ nucleo-f091rc
✅ nucleo-f303re
✅ same54-xpro
✅ nucleo-f767zi
✅ nucleo-l152re
✅ saml21-xpro
✅ nucleo-l432kc
✅ hifive1b
✅ saml10-xpro
✅ saml11-xpro
✅ samr30-xpro
✅ samr21-xpro
✅ samr34-xpro
✅ nrf52dk
✅ remote-revb
✅ nucleo-f207zg
✅ nucleo-l073rz
✅ slstk3401a
✅ nucleo-g474re
✅ nucleo-f411re
✅ nucleo-f103rb
✅ frdm-kw41z
✅ esp8266-esp-12x
✅ esp32-wroom-32
✅ z1
✅ arduino-due
✅ slstk3400a
✅ stk3200
|
fjmolinas
approved these changes
Oct 29, 2021
|
rerunning murdock just in case |
leandrolanzieri
approved these changes
Nov 1, 2021
There was a problem hiding this comment.
Murdock seems happy and the code looks good, nice step here @MrKevinWeiss ! ACK
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
Add kconfig testing for most series of stm32-*.
A number of fixes also occur, some smaller such as adding a few missing symbols like on the arduino_pwm and spi, some larger like the rtt/rtc fix.
A possibly controversial change is introduced which is the selection of LPUART module in the board selection and the PERIPH_RTC module in the cpu selection. The LPUART being conditionally brought in by STDIO depending on the board configuration is required. This because the STDIO port may be connected to the lpuart in the periph conf. As we only bring in the LPUART module if the STDIO module is selected this inherits the TEST_KCONFIG requirement, thus not needing it explicitly written...
The PERIPH_RTC_MEM bringing in PERIPH_RTC is requirement only by the STM32 CPU, this is due to needing to lock the RTC when writing to the RTC memory registers.
The possible alternative would be to use HAVE_* flags selected in the board/cpu then checked in the module (ie. STDIO or PERIPH_RTC_MEM). As this adds a level of indirection it is probably not the best solution.
Other possibilities would be to conditionally include kconfig source files, for example, a common RTC and a STM RTC kconfig file. However this means handling of multiple of the same symbol. Though there are ways of doing this I like keeping it clean and being able to search for
config MY_MODULEand only having one result. Otherwise I would have to resolve which module is being used for the specific case.Testing procedure
Murdock all green
Issues/PRs references