drivers/atwinc15x0: add Kconfig #17382

gschorcht · 2021-12-11T15:15:37Z

Contribution description

This PR adds Kconfig to the atwinc15x0 driver and contains the following changes:

Kconfig file is added to atwinc15x0. If the driver is selected, it automatically selects the required driver_atwinc15x0 package.This corresponds to the makefile dependencies.
An according Kconfig file is added to the test application tests/driver_atwinc15x0.
Since the Makefile of the test application includes tests/driver_netdev_common/Makefile that enables all required modules, a Kconfig file is also added to tests/driver_netdev_common by commit 6befb7d.
Commit ff37253 fixes the dependencies of the driver and the package in Kconfig. The driver_atwinc15x0 package is a helper package that is required for the atwinc15x0 driver module and cannot be used standalone. Therfore it should depend on the atwinc15x0 driver module and only be visible and force selected when the atwinc15x0 driver module is used.
~~feather-m0-wifi has an ATWINC15x0 module on-board. The atwinc15x0 driver is enable by default if module netdev_default is used (commit 49fb6fa). This corresponds to the makefile dependencies.~~
The feather-m0-wifi board is just a feather-m0 board with an additional ATWINC15x0 module. The feather-m0 is in fact a base board. The board definition of feather-m0 should be therefore reused. Kconfig should be defined on top of feather-m0 because it shares all its features (commit 06345e9). If this is agreed, the same should be done for feather- m0-lora.
The atwinc15x0 driver is enabled by default if a board has an ATWINC15x0 module (HAVE_ATWINC15X0) and module netdev_default is enabled (commit 57fc6f8).

Testing procedure

Compilation limited to the feather-m0-wifi board should succeed in CI.

Use command

TEST_KCONFIG=1 make BOARD=feather-m0-wifi -C tests/driver_atwinc15x0 info-modules

and check that the following modules are used:

atwinc15x0
driver_atwinc15x0
driver_atwinc15x0_common
driver_atwinc15x0_spi_flash

Use command

TEST_KCONFIG=1 make BOARD=feather-m0-wifi -C tests/driver_atwinc15x0 menuconfig

and check that

Drivers ---> Network Device Drivers ---> ATWINC15x0 WiFi module

is selected by default and can't be deselected.

-*- ATWINC15x0 WiFi module  --->

Go into the menu and check the input fields.

(RIOT_AP) WiFi SSID
[ ] Use WPA2 authentication

Check Use WPA2 authentication

(RIOT_AP) WiFi SSID
[*] Use WPA2 authentication
(This is RIOT-OS) Passphrase for WPA2 Personal Mode authentication (NEW)

Use command

TEST_KCONFIG=1 make BOARD=feather-m0-wifi -C tests/shell menuconfig

and check that

Drivers ---> Network Device Drivers ---> ATWINC15x0 WiFi module

is not selected and can be selected.

[ ] ATWINC15x0 WiFi module  ---

Issues/PRs references

Depends on PR #17381
Part of #16875

leandrolanzieri · 2021-12-12T10:38:36Z

boards/feather-m0-wifi/Kconfig

-    select HAS_PERIPH_UART
-    select HAS_PERIPH_USBDEV
-    select HAS_HIGHLEVEL_STDIO
+    select BOARD_FEATHER_M0


I'd preferably split common feather_m0 features into its own symbol in boards/feather-m0/Kconfig and select that instead. The BOARD_ symbols are usually used to identify which board is used and to set the default to BOARD. Although in this case BOARD should get the proper value because of the order of inclusion I find it cleaner if we want to factor this out.

Should we split the board Kconfig from this PR?

boards/feather-m0-wifi/Kconfig

pkg/driver_atwinc15x0/Kconfig

leandrolanzieri · 2021-12-12T10:45:28Z

tests/driver_atwinc15x0/Kconfig

+    imply MODULE_ATWINC15X0
+    imply DRIVER_NETDEV_COMMON


Application configurations of visible symbols are done through .config files. During migration we use a special name for testing.

RIOT/tests/driver_bmx280/app.config.test

Lines 1 to 6 in 85313cc

# this file enables modules defined in Kconfig. Do not use this file for

# application configuration. This is only needed during migration.

CONFIG_MODULE_BMX280=y

CONFIG_MODULE_BME280_I2C=y

CONFIG_MODULE_FMT=y

CONFIG_MODULE_XTIMER=y

I know. In the case of tests/driver_atwinc15x0, the Makefile is including a common Makefile.netdev.mk that is used by all netdev driver tests from tests/driver_netdev_common which enables all required modules.

Therefore, I was looking for a possibility to do the same in Kconfig. Since I found a Kconfig file in a number of tests, e.g. tests/periph_gpio, I thought that this could be the possible way because Kconfig can source other Kconfig files. Using app.config.test I have to enable all these modules, that have to enabled by each other netdev driver test application again and again:

CONFIG_MODULE_ATWINC15X0=y CONFIG_MODULE_SHELL=y CONFIG_MODULE_SHELL_COMMANDS=y CONFIG_MODULE_PS=y CONFIG_MODULE_GNRC=y CONFIG_MODULE_AUTO_INIT_GNRC_NETIF=y CONFIG_MODULE_GNRC_TXTSND=y CONFIG_MODULE_GNRC_PKTDUMP=y

Furthermore, with these definitions I get errors for all GNRC modules. That's why I was asking in comment #13135 (comment) how to enable GNRC modules in Kconfig for testing.

[genconfig.py]:ERROR-Treating Kconfig warnings as errors: [genconfig.py]:ERROR-=> /home/gs/src/RIOT-Xtensa-ESP.working/tests/driver_atwinc15x0/app.config.test:7: warning: attempt to assign the value 'y' to the undefined symbol MODULE_GNRC [genconfig.py]:ERROR-=> /home/gs/src/RIOT-Xtensa-ESP.working/tests/driver_atwinc15x0/app.config.test:8: warning: attempt to assign the value 'y' to the undefined symbol MODULE_AUTO_INIT_GNRC_NETIF [genconfig.py]:ERROR-=> /home/gs/src/RIOT-Xtensa-ESP.working/tests/driver_atwinc15x0/app.config.test:9: warning: attempt to assign the value 'y' to the undefined symbol MODULE_GNRC_TXTSND [genconfig.py]:ERROR-=> /home/gs/src/RIOT-Xtensa-ESP.working/tests/driver_atwinc15x0/app.config.test:10: warning: attempt to assign the value 'y' to the undefined symbol MODULE_GNRC_PKTDUMP

Application configurations of visible symbols are done through .config files.

If I understand it right

FEATURES_REQUIRED += xyz is modeled in app.config.test as CONFIG_MODULE_XYZ = y

FEATURES_OPTIONAL += xyz is modeled as part of config APPLICATION in Kconfig as imply MODULE_XYZ

USEMODULE += xyz is modeled in app.config.test as CONFIG_MODULE_XYZ = y

How could it be modeled that tests/driver_atwinc15x0/Makefile includes a common tests/driver_netdev_common/Makefile.netdev.mk that is used by all netdev driver tests to enable all required modules and to avoid to do it again and again for each netdev driver test? Would it be correct to model it like in 6581176 and d8b5a1c?

I think the way would be to have a .config file and add it to the loaded configurations by adding it to the list of the KCONFIG_ADD_CONFIG makefile variable.

leandrolanzieri · 2021-12-12T10:46:44Z

drivers/atwinc15x0/Kconfig

+    select MODULE_PERIPH_GPIO
+    select MODULE_PERIPH_GPIO_IRQ
+    select MODULE_PERIPH_SPI
+    select MODULE_NETDEV_ETH


I'm sorry but I think this is not modelled yet. That's the main reason we did not move yet to modelling networking drivers.

drivers/net/netdev seems not to be so much complicated. Since there were no error messages, maybe we should keep it so it won't be forgotten later?

drivers/atwinc15x0/Kconfig

Just for test compilations in CI.

aabadie · 2022-01-05T10:43:19Z

This PR contains lots of changes related to modeling netdev drivers in Kconfig that would be worth adding in a separate PR. For the moment they are in a test app but they should move to each related module Kconfig files. @gschorcht do you plan to add them ?

gschorcht · 2022-01-05T16:37:26Z

To be honest, I don't know what @leandrolanzieri's plans for netdev driver modeling are. I just made everything available that is needed by any netdev default in tests/driver_netdev_common to get Murdock happy. Most of them aren't needed for the ATWINC15x0 Kconfig. But tests/driver_netdev_common with all these definitions might help to start with adding real definitions in all these moduels and removing them there step by step.

aabadie · 2022-01-06T08:55:49Z

To be honest, I don't know what @leandrolanzieri's plans for netdev driver modeling are. I just made everything available that is needed by any netdev default in tests/driver_netdev_common to get Murdock happy.

I don't know either what are his plans but here I see that you made the Kconfig modeling job, it just needs to be moved to the right places. Once done, that would be a big step for the Kconfig migration, so very useful IMHO.

gschorcht · 2022-01-06T12:41:38Z

Kconfig modeling job, it just needs to be moved to the right places.

That's true for a number of these definitions. For others, however, it is necessary to model configuration parameters that may require more detailed knowledge about the according modules, e.g. nimble.

leandrolanzieri · 2022-01-18T09:16:38Z

cpu/nrf5x_common/radio/Kconfig.nrf5x

@@ -7,7 +7,7 @@

 config HAVE_NRF5X_RADIO
    bool
-    select NRF5X_RADIO if MODULE_NETDEV_DEFAULT
+    select NRF5X_RADIO if MODULE_NETDEV_DEFAULT && !PACKAGE_NIMBLE


@fjmolinas can you check if this is needed?

Nimble has not been modeled so far, but what should be done is change the default NRF5X_RADIO backend if using Nimble no? I think something like this:

diff --git a/cpu/nrf5x_common/radio/Kconfig.nrf5x b/cpu/nrf5x_common/radio/Kconfig.nrf5x index c98a0059b6..05ca54067b 100644 --- a/cpu/nrf5x_common/radio/Kconfig.nrf5x +++ b/cpu/nrf5x_common/radio/Kconfig.nrf5x @@ -22,6 +22,7 @@ if NRF5X_RADIO choice NRF5X_RADIO_BACKEND bool "nrf5x radio backend" + default MODULE_NRFBLE if PACKAGE_NIMBLE config MODULE_NRF802154 bool "Implementation of the IEEE 802.15.4 for nRF52 radio"

If when using nimble the backend is fixed, then we should also hide the prompt, so it cannot be changed.

leandrolanzieri · 2022-01-18T10:08:34Z

drivers/atwinc15x0/Kconfig

+config WIFI_SSID
+    string "WiFi SSID"
+    default "RIOT_AP"
+    help
+        SSID of the AP to be used. The SSID must not contain more
+        than 32 characters.
+
+config WIFI_USE_WPA2
+        bool "Use WPA2 authentication"
+        default n
+        help
+            Specify whether WPA2 authentication is used to connect to the
+            AP. If WPA2 authentication is enabled, the passphrase has to be
+            defined.
+
+config WIFI_PASS
+        string "Passphrase for WPA2 Personal Mode authentication"
+        depends on WIFI_USE_WPA2
+        default "This is RIOT-OS"
+        help
+            The passphrase is defined as a clear text string with a maximum
+            of 64 characters. It is used for WPA2 personal mode authentication.


Definitely outside the scope of this PR, but at some point we need to handle this system-wide and not driver-specific.

Hm, this seems to be a hen's egg problem because it is required for the compilation of the driver.

Yeah, as it is currently done. Perhaps we can think about some key management (something similar to credman?). Would it be possible (on a separate PR) to start unifying the macro name and have one system-wide for other modules (e.g., ESP)?

leandrolanzieri · 2022-01-18T10:09:22Z

drivers/atwinc15x0/Kconfig

+
+config WIFI_USE_WPA2
+        bool "Use WPA2 authentication"
+        default n


default n is the default behaviour, no need to specify it.

leandrolanzieri · 2022-01-18T10:11:06Z

drivers/atwinc15x0/doc.txt

+#WIFI_SSID | "RIOT_AP" | SSID of the AP to be used.
+#WIFI_PASS | -         | Passphrase used for the AP as clear text (max. 64 chars) [1].


I'd suggest to migrate these values to include the CONFIG_ prefix

My concern is that user applications may have already used these documented configuration settings. We can change them but we should have some deprecation mechanism which. My approach was to use it in that way

#ifndef WIFI_SSID #ifndef CONFIG_WIFI_SSID #define WIFI_SSID "RIOT_AP" #else /* CONFIG_WIFI_SSID */ #define WIFI_SSID CONFIG_WIFI_SSID #endif /* CONFIG_WIFI_SSID */ #endif /* WIFI_SSID */

which allows to use both definitions.

Sure, we should add a deprecation warning if we change them. It already has been done with other configurations (e.g., #13129).

leandrolanzieri · 2022-01-18T10:12:01Z

sys/random/Kconfig

@@ -16,7 +16,7 @@ if MODULE_RANDOM
 choice RANDOM_IMPLEMENTATION
    bool "PRNG Implementation"
    depends on TEST_KCONFIG
-    default MODULE_PRNG_HWRNG if HAS_PERIPH_HWRNG
+    default MODULE_PRNG_HWRNG if HAS_PERIPH_HWRNG && !MODULE_NETDEV_DEFAULT


To be honest, I don't remember but there was a problem when module netdev_default is used.

leandrolanzieri · 2022-01-18T10:15:57Z

tests/driver_netdev_common/Kconfig

+    select MODULE_PS
+
+    # gnrc is a meta module including all required, basic gnrc networking modules
+    select MODULE_GNRC


If we need the stack modelled to enable this application then we should do it properly first. As @aabadie mentions, all these modules should be on their own files. Networking is the next big step in the migration.

I wonder if it is actually possible to use the driver without GNRC, in that case we could test the driver modelling alone.

As @aabadie mentions, all these modules should be on their own files.

That is clear. It was just an attempt to test the netdev driver Kconfigs before the modeling of the network stack is complete. I was able to get green CI with them. As said, I thought that tests/driver_netdev_common with all these dummy definitions might help to model network related drivers and models step by step and remove them from tests/driver_netdev_common once they are modeled.

I wonder if it is actually possible to use the driver without GNRC, in that case we could test the driver modelling alone.

Since the feather-m0-wifi board uses the driver as netdev_default, which in turn is activated by all GNRC applications, you will not be able to successfully compile these applications into CI without it being modeled somehow, even if it is a dummy definition to begin with.

Since the feather-m0-wifi board uses the driver as netdev_default, which in turn is activated by all GNRC applications, you will not be able to successfully compile these applications into CI without it being modeled somehow, even if it is a dummy definition to begin with.

But couldn't we model just the driver + netdev without GNRC? We would not test GNRC-based applications but at least the one from the driver, right? AFAIU netdev_default should not pull the stack.

@gschorcht regarding this point, what I'm attempting for Ethernet (#17739) and IEEE 802.15.4 (#17789) is to change the tests to use the driver and netdev only, without pulling any stack.

leandrolanzieri · 2022-01-19T10:50:09Z

tests/driver_netdev_common/Kconfig

+config MODULE_NETDEV
+    bool
+    default y if BOARD_ATXMEGA_A1_XPLAINED && MODULE_NETDEV_DEFAULT
+    default y if BOARD_ATXMEGA_A3BU_XPLAINED && MODULE_NETDEV_DEFAULT
+    select MODULE_EUI_PROVIDER
+
+config MODULE_NETDEV_ETH
+    bool
+    default y if HAS_PERIPH_ETH
+    select MODULE_NATIVE_CLI_EUI_PROVIDER if BOARD_NATIVE
+    select MODULE_NETDEV
+    select MODULE_NETDEV_REGISTER if !MODULE_GNRC_NETIF_SINGLE
+
+    select MODULE_STM32_ETH if HAS_PERIPH_ETH && HAS_CPU_STM32 && !MODULE_ATWINC15X0
+    select MODULE_SAM0_ETH if HAS_PERIPH_ETH && HAS_CPU_SAMD5X && !MODULE_ATWINC15X0


I know this is here just for testing, but we can use it as a starting point.

As I understand the current modules, MODULE_NETDEV_DEFAULT will only enable networking devices that are declared by the board (via HAVE_ symbols). Each driver would select the required netdev module, which would in turn select the generic netdev module. I don't think there is need to actually add prompts to these netdev modules, as they are needed.

Perhaps we need one more abstraction between driver module and netdev module, as some devices work without netdev (e.g., they use the IEEE802154 radio HAL). In that case the device would select instead a HAVE_IEEE802154_DEVICE or so, and the selection of the netdev may be optional if the driver supports something else than the netdev interface.

Although highly outdated (both name schemes and some patterns), #14055 did some modelling in this direction, both netdev and IEEE802154-related GNRC modules. Note that the netdev modules had prompt in that proposal, but I'd not go with that now.

stale · 2022-09-21T05:49:17Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

gschorcht requested review from aabadie, cgundogan, fjmolinas, jia200x, leandrolanzieri, MichelRottleuthner and smlng as code owners December 11, 2021 15:15

github-actions bot added Area: boards Area: Board ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: tests Area: tests and testing framework labels Dec 11, 2021

gschorcht removed request for aabadie and MichelRottleuthner December 11, 2021 15:15

gschorcht force-pushed the drivers/atwinc15x0/Kconfig branch from 06345e9 to 0de32ca Compare December 12, 2021 06:55

gschorcht requested a review from kaspar030 as a code owner December 12, 2021 10:18

github-actions bot added the Area: CI Area: Continuous Integration of RIOT components label Dec 12, 2021

leandrolanzieri reviewed Dec 12, 2021

View reviewed changes

leandrolanzieri mentioned this pull request Dec 12, 2021

[Tracking]: modeling dependencies in Kconfig #16875

Closed

gschorcht force-pushed the drivers/atwinc15x0/Kconfig branch 2 times, most recently from e49d21d to d8b5a1c Compare December 24, 2021 12:35

github-actions bot removed the Area: boards Area: Board ports label Dec 24, 2021

gschorcht force-pushed the drivers/atwinc15x0/Kconfig branch 4 times, most recently from 766e792 to ca957cd Compare December 26, 2021 14:45

gschorcht added 10 commits December 27, 2021 15:51

tests/driver_atwinc15x0: add Kconfig

047c60a

fixup! drivers/atwinc15x0: add Kconfig

14a3792

fixup! drivers/atwinc15x0: add Kconfig

e21f575

fixup! drivers/atwinc15x0: add Kconfig

4091611

fixup! drivers/atwinc15x0: add Kconfig

57fc6f8

fixup! tests/driver_netdev_common: add Kconfig

9c4ee95

fixup! tests/driver_atwinc15x0: add Kconfig

bac0efb

fixup! drivers/atwinc15x0: add Kconfig

de96ed6

drivers/atwinc15x0: small doc improvements

29f8ab6

REMOVEME

c533b6c

gschorcht force-pushed the drivers/atwinc15x0/Kconfig branch from c274d0a to 617e793 Compare December 27, 2021 14:51

gschorcht added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 27, 2021

gschorcht force-pushed the drivers/atwinc15x0/Kconfig branch from 617e793 to c533b6c Compare December 27, 2021 17:01

github-actions bot removed the Area: CI Area: Continuous Integration of RIOT components label Dec 27, 2021

gschorcht requested a review from haukepetersen as a code owner December 29, 2021 10:46

github-actions bot added Area: cpu Area: CPU/MCU ports Area: sys Area: System Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Dec 29, 2021

REMOVEME

ae17fcc

Just for test compilations in CI.

gschorcht force-pushed the drivers/atwinc15x0/Kconfig branch from 036c60e to ae17fcc Compare December 29, 2021 12:09

leandrolanzieri requested changes Jan 18, 2022

View reviewed changes

leandrolanzieri reviewed Jan 19, 2022

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Sep 21, 2022

gschorcht removed the State: stale State: The issue / PR has no activity for >185 days label Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/atwinc15x0: add Kconfig #17382

drivers/atwinc15x0: add Kconfig #17382

gschorcht commented Dec 11, 2021 •

edited

Loading

leandrolanzieri Dec 12, 2021

gschorcht Dec 12, 2021

leandrolanzieri Dec 12, 2021

gschorcht Dec 12, 2021 •

edited

Loading

gschorcht Dec 24, 2021 •

edited

Loading

leandrolanzieri Jan 18, 2022

leandrolanzieri Dec 12, 2021

gschorcht Dec 12, 2021

aabadie commented Jan 5, 2022 •

edited

Loading

gschorcht commented Jan 5, 2022

aabadie commented Jan 6, 2022

gschorcht commented Jan 6, 2022

leandrolanzieri Jan 18, 2022

fjmolinas Jan 18, 2022

leandrolanzieri Jan 18, 2022

fjmolinas Jan 18, 2022

leandrolanzieri Jan 18, 2022

gschorcht Jan 18, 2022

leandrolanzieri Jan 18, 2022

leandrolanzieri Jan 18, 2022

leandrolanzieri Jan 18, 2022

gschorcht Jan 18, 2022 •

edited

Loading

leandrolanzieri Jan 18, 2022

leandrolanzieri Jan 18, 2022

gschorcht Jan 18, 2022

leandrolanzieri Jan 18, 2022

gschorcht Jan 18, 2022

leandrolanzieri Jan 18, 2022

leandrolanzieri Mar 11, 2022

leandrolanzieri Jan 19, 2022

leandrolanzieri Jan 19, 2022

stale bot commented Sep 21, 2022

	# this file enables modules defined in Kconfig. Do not use this file for
	# application configuration. This is only needed during migration.
	CONFIG_MODULE_BMX280=y
	CONFIG_MODULE_BME280_I2C=y
	CONFIG_MODULE_FMT=y
	CONFIG_MODULE_XTIMER=y

		#WIFI_SSID \| "RIOT_AP" \| SSID of the AP to be used.
		#WIFI_PASS \| - \| Passphrase used for the AP as clear text (max. 64 chars) [1].

drivers/atwinc15x0: add Kconfig #17382

Are you sure you want to change the base?

drivers/atwinc15x0: add Kconfig #17382

Conversation

gschorcht commented Dec 11, 2021 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschorcht Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

gschorcht Dec 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Jan 5, 2022 • edited Loading

gschorcht commented Jan 5, 2022

aabadie commented Jan 6, 2022

gschorcht commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschorcht Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Sep 21, 2022

gschorcht commented Dec 11, 2021 •

edited

Loading

gschorcht Dec 12, 2021 •

edited

Loading

gschorcht Dec 24, 2021 •

edited

Loading

aabadie commented Jan 5, 2022 •

edited

Loading

gschorcht Jan 18, 2022 •

edited

Loading