sys/usbus: changed approach of default activation of stdio_cdc_acm in Kconfig #19038
+3
−112
Conversation
This file contains 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
If `TEST_KCONFIG` is set and the board provides the `highlevel_stdio` feature, the `stdio_cdc_acm` module is enabled by default in Kconfig. This setting was previously done unconditionally either by a `.config` file of the board or the bootloader `.config` file (`boards/common/samx1d-arduino-booloader.config`, `nrf52_bootloader.confg`). Changing this approach became necessary to allow enabling other `stdio_*` modules or other USB stacks like tinyUSB for these boards.
Default configuration is now realized in Kconfig files.
This file is not needed any longer with the conditional enable of ´stdio_cdc_acm` and `usbus_*` modules.
gschorcht
requested review from
leandrolanzieri,
jia200x,
MrKevinWeiss,
aabadie and
haukepetersen
as code owners
github-actions
bot
added
Area: boards
gschorcht
added
Type: enhancement
There was a problem hiding this comment.
I am a bit worried that we are somewhat shooting ourselves in the foot with matching the constraints of make and constraints of kconfig... I don't know if this is the proper overall modelling of STDIO but I imagine it is a step in the right direction. Maybe @leandrolanzieri can confirm...
| @@ -11,6 +11,7 @@ menu "Standard Input/Output (STDIO)" | |||
| choice STDIO_IMPLEMENTATION | |||
| bool "STDIO implementation" | |||
| default MODULE_STDIO_NATIVE if CPU_ARCH_NATIVE | |||
| default MODULE_STDIO_CDC_ACM if TEST_KCONFIG && HAS_HIGHLEVEL_STDIO | |||
There was a problem hiding this comment.
We don't need the TEST_KCONFIG as it is defined above in the menu.
There was a problem hiding this comment.
To me this reads as "If I want to use STDIO and I have HIGHLEVEL_STDIO then I would like to use the STDIO_CDC_ACM" that choice STDIO_IMPLEMENTATION` can be overridden in the future or for specific boards to say:
choice STDIO_IMPLEMENTATION
default MODULE_STDIO_MY_BOARD_SPECIFIC_CHOICE
so I guess that works
| @@ -9,6 +9,7 @@ menuconfig MODULE_USBUS | |||
| bool "USB Unified Stack (USBUS)" | |||
| depends on TEST_KCONFIG | |||
| depends on HAS_PERIPH_USBDEV || MODULE_USBDEV_MOCK | |||
| default y if TEST_KCONFIG && HAS_HIGHLEVEL_STDIO | |||
There was a problem hiding this comment.
Again, no need to add the TEST_KCONFIG
There was a problem hiding this comment.
Also, logically this doesn't make sense. Wouldn't I only bring in the MODULE_USBUS if something needed it, for example MODULE_STDIO_CDC_ACM probably needs it... Just having a provided feature of HAS_HIGHLEVEL_STDIO probably shouldn't select it.
Though I understand that we assume everything we build has some STDIO or console or something.
| @@ -49,6 +49,7 @@ endif # KCONFIG_USEMODULE_USBUS_CDC_ACM | |||
| config MODULE_USBUS_CDC_ACM | |||
| bool "USB CDC ACM support" | |||
| depends on MODULE_USBUS | |||
| default y if TEST_KCONFIG && HAS_HIGHLEVEL_STDIO | |||
There was a problem hiding this comment.
Also don't need TEST_KCONFIG here as it is already in MODULE_USBUS. Maybe here would be better to select though @leandrolanzieri will probably disagree (or maybe imply or something).
bors bot
added a commit
that referenced
this pull request
May 31, 2023
19086: Remodel the USB in Kconfig r=aabadie a=MrKevinWeiss
### Contribution description
#### The issues with current architecture
Generally there has been some confusion on how to manage KConfig with respect to the board selection of default STDIO backends, specifically for boards that require a USB based backend as there are possible stacks to use.
The `<BOARD>.config` way of selecting cannot handle conditional selects.
The issues is more with boards such as `esp32s2-wemos-mini`, currently some USB stack will be selected regardless of overridding the preferred STDIO.
Selecting a USB stack directly with `STDIO_USB*` creates some circular dependency issues with kconfig and is hard to manage.
We also have a mutually exclusive USB stacks, TINYUSB or USBUS which should probably be a choice.
#### Desired behaviour
1. Ideally we want a board to default to the most obvious STDIO implementation, for example, if I have nucleo, it uses a UART, for some ESPs, USB is the default way to communicate.
2. These backends could always be overridden though, for example, I may just connect directly to a UART and want my STDIO there, or maybe use a ble based STDIO.
3. The next condition would be specifically for boards with a USB based STDIO. Since we have a TINYUSB stack and a USBUS stack we would want to use the associated STDIO depending on the stack the application selects.
4. However, if nothing is selected by the application, than bring in a USB stack (board based preference) unless there is a specific non-USB based STDIO is selected. For these boards that have this requirement, we DO NOT want to bring in the USB stack if the STDIO is specifically overridden (important for kconfig).
#### Update kconfiglib package to RIOT-OS org managed one
There is a problem with the upstreamed Kconfiglib implementation and the maintainer is not responsive to the fix. The issue is to do with `menuconfig`s in choices and has been fixed with the RIOT-OS based fork. This PR requires this fix.
#### Changes to the USB stack
A new entry point is introduced `USB_DEVICE` which indicates wanting a USB device but not caring which stack is used. This allows making a `choice` between the `TINYUSB` and `USBUS` stack allowing mutual exclusivity.
Making the USB stack a `choice` means that a specific stack cannot be selected from non-board/non-cpu/non-application based symbols. Thus the `REQUIRES_` design pattern is used for a module to indicate a specific stack should be selected. This is needed for the `MODULE_TINYUSB_NETDEV` in this case.
#### Changes to USB STDIO implementations
The `MODULE_STDIO_CDC_ACM` and `MODULE_STDIO_TINYUSB_CDC_ACM` are both depends on now, using a `REQUIRES_USB_STDIO` to select the dependencies.
This means we do not have to use `select PACKAGE_TINYUSB if TEST_KCONFIG && !MODULE_USBUS` in the board select.
##### Why not just select the USB from STDIO_USB
Issue with using select for STDIO choices is that we cannot check which stack we are using to default the STDIO to that, breaking desired behaviour 3.
#### The `FORCE_USB_STDIO`
Desired behaviour 4 means that we do not want to bring in the USB stack if we override, say, to the UART STDIO backend. Due to the limitations of Kconfig, this is my solution to prevent the USB from being brought in if there is an STDIO that doesn't need it. It is only for the `esp32s2-wemos-mini` board and would not be used in other places and would only need to be explicitly disabled for applications requiring different STDIO backend and no USB. It is not perfect but I think the best solution and fairly understandable...
<details><summary><h4>Issues with Kconfig</h4></summary>
When using a `choice` and having conditional defaults, for example:
```kconfig
choice IMPL
default FOO if CHOOSE_FOO
default BAR
```
there is a limitation of the level of the level of knowledge that can be expected from Kconfig, a limitation on circular dependencies, and a limitation that the dependencies only get resolved once.
For example, if ` BAR` selects something that would eventually select `CHOOSE_FOO`, then the default should be `FOO` and which would no longer select `BAR` preventing the select `CHOOSE_FOO`... Messy stuff and we would want an error saying no no no.
What Kconfig cannot handle is something like:
```kconfig
choice IMPL
bool "Implementation"
default FOO if CHOOSE_FOO
default BAR
config FOO
bool "Foo"
config BAR
bool "Bar"
endchoice
config CHOOSE_FOO
bool
config SYMBOL
bool
select CHOOSE_FOO if !BAR
```
`SYMBOL` causes a circular dependency in Kconfig even though the only possible outcome for the `choice` selection would be static. If we select `BAR` then `CHOOSE_FOO` would not be selected and we stay with `BAR`. If we select `FOO` than `CHOOSE_FOO` will be selected which stays with `FOO`. Everything should be fine, but isn't because Kconfig does not resolve to that degree, it simply sees that there is a dependency of the `IMPL` choice outcome (ie. `if !BAR`) that is a condition for a dependency of the `IMPL` choice selection (ie. ` if CHOOSE_FOO`).
This is a limitation of the Kconfig what what makes this problem so challenging, with Make we say "select some sort of USB backend if no other stdio is specifically requested" and it will.
</details>
An attempt at remodelling the dependencies of the USB stack in Kconfig.
Currently there are some issues, especially with the integration of TinyUSB package as a backend.
This will require a kconfiglib package fix though...
### Testing procedure
`TEST_KCONFIG=1 BOARD=reel make menuconfig -C examples/hello-world`
### Issues/PRs references
Requires ulfalizer/Kconfiglib#123 to be merged upstream or fork for RIOT
Relates maybe to #18998 and #19038
19672: pkg/micropython: model in Kconfig r=aabadie a=aabadie
Co-authored-by: MrKevinWeiss <[email protected]>
Co-authored-by: Alexandre Abadie <[email protected]>
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
This PR is a split-off of PR #18998 and changes the approach how the highlevel STDIO as well as required USBUS modules are enabled in Kconfig if the
TEST_KCONFIGvariable is set.Background
The logic for when the highlevel STDIO and the required USBUS modules are enabled in Kconfig is inverse to the logic in makefile dependencies:
stdio_cdc_acmand thus also theusbus*modules are automatically enabled if no otherstdio_*module is enabled and thehighlevel_stdiofeature is provided.MODULE_USBUS, then he can enableMODULE_USBUS_CDC_ACMandMODULE_STDIO_CDC_ACMor evenMODULE_USB_BOARD_RESETafterwards. The only way to enable them by default for testing is to enable them unconditionally in an application configuration file.configfor boards with the highlevel STDIO feature, and then to include this.configit inMakfile.featuresof the board for test compilation ifTEST_KCONFIGis set. See for exampleRIOT/boards/common/nrf52/nrf52_bootloader.config
Lines 1 to 4 in 7c7aede
RIOT/boards/nrf52840-mdk-dongle/Makefile.features
Lines 11 to 15 in 7c7aede
The problem of the Kconfig approach is that it is not possible to use other
stdio_*modules ifTEST_KCONFIGis set since the modules in.configare enabled unconditionally. This prevents for example enabling the tinyUSB feature and using thestdio_tinyusb_cdc_acmmodule for such boards.Another problem is that a Kconfig file is included in a
Makefile.featuresfile with this approach which seems not to be consistent.Proposed Change
Commit 21eee37 is a very small change that changes the Kconfig approach according to the Makefile dependencies approach if and only if the variable
TEST_KCONFIGis set. That is,MODULE_STDIO_CDC_ACM,MODULE_USBUSandMODULE_USBUS_CDC_ACMare automatically enabled if theTEST_CONFIGvariable is set and the board provides thehighlevel_stdiofeature. Since the boards that previously used the.configfile are exactly the same boards that provide thehighlevel_stdiofeature, this change is functionally the same.The other commits just remove the
.configfiles and their include inMakefile.featuresof these boards.Testing procedure
Green CI
Issues/PRs references
Prerequisite for PR #18998