pkg/tinyusb: fix Kconfig problems

[gschorcht]

Contribution description

This PR tries to fix the problems with tinyUSB in nightlies.

• The tinyusb_device feature had to be moved from boards/common/arduino-zero definition to the boards/arduino-zero definition because the common arduino-zero features are also used by wemos-zero which uses the highlevel_stdio feature via the stdio_cdc_acm module.
• The tinyusb_device feature had to be removed from Kconfig of board stm32f429i-disco since it uses the highlevel_stdio feature via the stdio_cdc_acm module.

Testing procedure

Green CI

Issues/PRs references

[benpicco]

Do we still need this with #19007?

[gschorcht]

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.

[riot-ci]

Murdock results

✔️ PASSED

b805709 boards/stm32f429i-disc1: disable tinyusb feature for stm32f429i-disco

Success	Failures	Total	Runtime
2006	0	2006	04m:11s

Artifacts

• Documentation preview

[gschorcht]

@benpicco I also had to split boards/stm32f429i-disco1/Makefile.features into a file common to stm32f429i-disco as well as stm32f429i-disc1, and files specific to each of these boards. Since the board definitions are just different variants of the same board, I used a file boards/stm32f429i-disc1/Makefile.features.common for the common part instead of defining boards/common/stm32f429i-discx.

[gschorcht]

@benpicco It seems that the last change fixes the Kconfig problem for now? May I squash?

[benpicco]

Sure, but static tests now have some issues

[gschorcht]

Sure, but static tests now have some issues

Ah yes, the static test that allows FEATURES_PROVIDED only in files named exactly Makefile.features 😟 which seems to be too hard a restriction. Then I don't see no other than to define a common board for the stm32f429i-discx boards or we find another solution.

[gschorcht]

@benpicco It seems that changing the approach of how the stdio_cdc_acm and usbus* modules are enabled by default in PR #18998 when TEST_KCONFIG is defined solves the problem for the stm32f429i-disco board as well, and allows stdio_tinyusb_cdc_acm to be used when tinyusb_device is enabled.

[maribu]

please squash :)

[gschorcht]

please squash :)

Unfortunately, the static tests fail because I placed with commit e181c61 the FEATURES_PROVIDED definitions shared by the stm32f429i-disco and stm32f429i-disc1 revisions of the same board in a Makfile.features.common file instead of a Makefile.features file. I thought I could solve the problem this way without defining completely new boards/common/stm32f429t-discx for only two revisions of the same board that differ in only one feature.

[gschorcht]

The problem is that the ST-Link UART interface of the stm32f429i-disco revision seems to be buggy and the highlevel_stdio feature has to be used which in turn doesn't allow to enable the tinyusb_device feature as long as PR #18998 is not merged.

What I am trying to fix is actually the following

--- a/boards/stm32f429i-disc1/Makefile.features
+++ b/boards/stm32f429i-disc1/Makefile.features
@@ -11,4 +11,7 @@ FEATURES_PROVIDED += periph_usbdev
 
 # Put other features for this board (in alphabetical order)
 FEATURES_PROVIDED += riotboot
-FEATURES_PROVIDED += tinyusb_device
+
+ifeq (,$(filter highlevel_stdio,$(FEATURES_PROVIDED)))
+  FEATURES_PROVIDED += tinyusb_device
+endif

but this is also not allowed by the sanity check of the make system 😟

The only way to solve this problem seems to be to disable the tinyusb_device feature for stm32f429i-discx boards for now.

[maribu]

The problem is that the ST-Link UART interface of the stm32f429i-disco revision seems to be buggy

Is there an errata for that? Otherwise maybe it was just one flaky board or a missing update of the st-link firmware...

[gschorcht]

The problem is that the ST-Link UART interface of the stm32f429i-disco revision seems to be buggy

Is there an errata for that? Otherwise maybe it was just one flaky board or a missing update of the st-link firmware...

@benpicco's statement in PR 14122 doesn't sound like a flaky board.

[maribu]

Interesting. It seems that the disc1 is using the typical STM32F103 for an ST-LINK v2.1 interface, the disco is using a different chip that does not support modern features like dragand drop programming and UART. So indeed not just a flaky board or an outdated firmware :-(

[gschorcht]

Interesting. It seems that the disc1 is using the typical STM32F103 for an ST-LINK v2.1 interface, the disco is using a different chip that does not support modern features like dragand drop programming and UART.

Yes, unfortunately uses the Rev. B01 of the board a STM32F103C8T6 instead of a STM32F103CBT6 as Rev C01 or up which only differ in flash 😟 Obviously the ST-Link V2.1 firmware requires more than 64 kByte Flash.

But do two revisions of the same board justify the introduction of a boards/common/stm32f429i-discx board definition just because the sanity check of the make system prevents conditional FEATURES_PROVIDED?

Perhaps the sanity check should at least allow features to depend on other features, especially because the make system is end-of-life in the long term and Kconfig allows this. Otherwise we have to disable the tinyusb_device feature until PR #18998 is merged to fix the compilation problem in master.

[maribu]

I think it is fine to create a common board for both. My personal mind model of boards has the assumption build in that changing one board won't affect another, so IMO it is actually less surprising to create a common folder - even if the diff between the two boards is trivial.

I wonder if the disco version has the correct PCB lines. The STM32F103C8 often actually has 128 KiB flash. I have like 40 STM32F103C8 lying around and 30 of them do flash fine with a firmware larger than 64 KiB (and I also verified that the contents in the flash actually read back correctly). I am not sure if the 10 that don't are actually clones (the do have an STM logo, though, so they would be the nasty kind of clones with fake labels...). Those 10 were all on the same charge of cheap Blue Pills, so fake clones is actually quite a plausible explanation.

I doubt that the ST-Link updater would just ignore the chip ID and write more data than the official storage capacity, though. But at least a black magic probe firmware (which also requires more than 64 KiB in recent versions) should work just fine and would allow UART :)

[gschorcht]

Finally, I found a hack with commit b805709 on how to disable the tinyusb_device feature for the stm32f429i-disco board revision. The sanity check allows to use the BOARD variable to enable features.

I squashed it directly because the change is small enough and to avoid an extra CI compilation round.

[gschorcht]

@maribu I tried to start a full build to be sure that the PR is completely compilable, but after one hour of collecting jobs, I stopped the full build. I have no idea what the problem was. Let's see what happens when bors executes the full build.

[gschorcht]

It seems that the nightlies fail only because of tinyUSB. Could we try to merge this PR to fix it.

[maribu]

bors merge

[bors]

Build succeeded:

• Murdock
• static-tests
• tools-build

[gschorcht]

Thanks