Skip to content
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

pkg/tinyusb: fix Kconfig problems #19023

Merged
merged 2 commits into from
Dec 13, 2022
Merged

pkg/tinyusb: fix Kconfig problems #19023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b1b7e8
boards/arduino-zero: move tinusb_device feature
gschorcht Dec 7, 2022
b805709
boards/stm32f429i-disc1: disable tinyusb feature for stm32f429i-disco
gschorcht Dec 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions boards/arduino-zero/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ config BOARD
config BOARD_ARDUINO_ZERO
bool
default y
select HAS_TINYUSB_DEVICE
select BOARD_COMMON_ARDUINO_ZERO

source "$(RIOTBOARD)/common/arduino-zero/Kconfig"
2 changes: 2 additions & 0 deletions boards/arduino-zero/Makefile.features
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
include $(RIOTBOARD)/common/arduino-zero/Makefile.features

FEATURES_PROVIDED += tinyusb_device
1 change: 0 additions & 1 deletion boards/common/arduino-zero/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ config BOARD_COMMON_ARDUINO_ZERO
select HAS_PERIPH_USBDEV
select HAS_ARDUINO
select HAS_ARDUINO_PWM
select HAS_TINYUSB_DEVICE

select HAVE_SAUL_GPIO

Expand Down
1 change: 0 additions & 1 deletion boards/common/arduino-zero/Makefile.features
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,3 @@ FEATURES_PROVIDED += periph_usbdev
# Various other features (if any)
FEATURES_PROVIDED += arduino
FEATURES_PROVIDED += arduino_pwm
FEATURES_PROVIDED += tinyusb_device
2 changes: 1 addition & 1 deletion boards/stm32f429i-disc1/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ config BOARD_STM32F429I_DISC1

# Put other features for this board (in alphabetical order)
select HAS_RIOTBOOT
select HAS_TINYUSB_DEVICE
select HAS_TINYUSB_DEVICE if !BOARD_STM32F429I_DISCO
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this with #19007?

Copy link
Contributor Author

@gschorcht gschorcht Dec 7, 2022 •
edited

Choose a reason for hiding this comment

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

Hm, unfortunatly it seems so. If you take a look into the Nightlies output , the compilation of tests/pkg_tinyusb_cdc_acm_stdio fails for stm32f429i-disco because usbus* modules are enabled in Kconfig due to the unconditional settings in the application configuration file boards/stm32f429i-disco/stm32f429i-disco.config

RIOT/boards/stm32f429i-disco/stm32f429i-disco.config

Lines 1 to 3 in 2274a56

CONFIG_MODULE_USBUS=y
CONFIG_MODULE_USBUS_CDC_ACM=y
CONFIG_MODULE_STDIO_CDC_ACM=y

That is, even if we enable MODULE_STDIO_TINYUSB_CDC_ACM, all MODULE_USBUS* symbols are unconditionally enabled. The only solution for now is to disable the tinyUSB device feature for this board.

This is exactly the same problem I have with all boards that use the single USB interface as highlevel_stdio and usb_board_reset to control the bootloader mode, for example boards/common/arduino-mkr. I can't enable the tinyUSB feature for these boards, although we support stdio_tinyusb_cdc_acm and usb_board_reset in tinyUSB, because the symbols MODULE_STDIO_CDC_ACM and MODULE_USBUS* are unconditionally enabled by such application configuration files, for example boards/common/samdx1-arduino-bootloader/samdx1-arduino-bootloader.config

RIOT/boards/common/samdx1-arduino-bootloader/samdx1-arduino-bootloader.config

Lines 1 to 3 in 2274a56

CONFIG_MODULE_USBUS=y
CONFIG_MODULE_USBUS_CDC_ACM=y
CONFIG_MODULE_STDIO_CDC_ACM=y

I have been trying and thinking about how to solve this problem for days, because this problem is exactly what prevents PR #18998 from being compilable. We could enable the tinyUSB feature for all these boards, but the unconditional activation of MODULE_STDIO_CDC_ACM and MODULE_USBUS* prevents this.

I have already discussed the problem a bit with @MrKevinWeiss. The reason is that the logic of when MODULE_STDIO_CDC_ACM and MODULE_USB* are enabled in Kconfig is the reverse of the logic in the Makefile dependencies for these modules. In the Makefile dependencies stdio_cdc_acm and thus also the usbus* modules are enabled automatically when needed. It is easy to define an additional prerequisite for them that the tinyusb_device is not enabled. In Kconfig, the user has to enable MODULE_USBUS first, then he can enable MODULE_USBUS_CDC_ACM and then MODULE_STDIO_CDC_ACM or even MODULE_USB_BOARD_RESET afterwards. The only way to enable them by default if needed is their unconditional activation in such application configuration files which are included in Makfile.features for test compilation at the moment.

I personally don't like this logic since the the application/user has to enable them explicitly even if they are required to flash the board at all.

Any suggestion how to solve this problem would be welcome, otherwise we can't enable the tinyUSB feature for a large couple of boards.


# Clock configuration
select BOARD_HAS_HSE
Expand Down
5 changes: 4 additions & 1 deletion boards/stm32f429i-disc1/Makefile.features
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,7 @@ FEATURES_PROVIDED += periph_usbdev

# Put other features for this board (in alphabetical order)
FEATURES_PROVIDED += riotboot
FEATURES_PROVIDED += tinyusb_device

ifneq (,$(filter stm32f429i-disc1,$(BOARD)))
FEATURES_PROVIDED += tinyusb_device
endif