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.

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

boards/weact-f4x1cx: enable tinyUSB #19275

Closed

Conversation

benpicco
Copy link
Contributor

Contribution description

The MCU already has tinyUSB support, we only need to mark it as available in the board, together with periph_usbdev. (Ideally this should just be a single USB port feature on which both periph_usbdev and tinyusb_device depend).

Testing procedure

Issues/PRs references

@benpicco benpicco added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Feb 14, 2023
@benpicco benpicco requested review from gschorcht and maribu February 14, 2023 12:44
@github-actions github-actions bot added Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration labels Feb 14, 2023
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration labels Feb 14, 2023
@@ -16,6 +16,7 @@ config BOARD_COMMON_WEACT_F4X1CX
select HAS_PERIPH_TIMER
select HAS_PERIPH_UART
select HAS_PERIPH_USBDEV
select HAS_TINYUSB_DEVICE
Copy link
Contributor

@gschorcht gschorcht Feb 14, 2023

Choose a reason for hiding this comment

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

Kconfig won't be happy with this. This board and all the other boards with a signle USB interface were part of PR #18998 where I also changed the approach of the STDIO backend selection for Kconfig in commit b2c855a.

But with the current approach of STDIO selection we would have to use the selection as for stm32f4discovery or esp32s-wemos-mini dependent on what the default selection should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah true it's the problem again that would require a weact-f4x1cx.conf just to make Kconfig CI happy but without solving the issue on the Kconfig side.

@riot-ci
Copy link

riot-ci commented Feb 14, 2023

Murdock results

✔️ PASSED

f62aeff boards/weact-f4x1cx: enable tinyUSB

Success Failures Total Runtime
6865 0 6865 13m:55s

Artifacts

@benpicco
Copy link
Contributor Author

closed in favor of #18998

@benpicco benpicco closed this Feb 16, 2023
@benpicco benpicco deleted the boards/weact-f4x1cx-tinyusb branch February 16, 2023 16:15
@gschorcht
Copy link
Contributor

Why did you close it in favour of PR #18998? I don't think that PR #18998 will get merged as it is as long as the STDIO selection problem is not solved in Kconfig.

@benpicco
Copy link
Contributor Author

I can always just set CONTINUE_ON_EXPECTED_ERRORS=1, that's preferable to dealing with another Kconfig issue 😅

@gschorcht
Copy link
Contributor

Why not to solve it for now like for esp32s2_wemos_mini?

       select HAS_HIGHLEVEL_STDIO
+     select MODULE_USBUS_CDC_ACM if TEST_KCONFIG && MODULE_USBUS
+     select PACKAGE_TINYUSB if TEST_KCONFIG && !MODULE_USBUS
+
+ choice STDIO_IMPLEMENTATION
+     default MODULE_STDIO_CDC_ACM if MODULE_USBUS
+     default MODULE_STDIO_TINYUSB_CDC_ACM if PACKAGE_TINYUSB
+ endchoice

@benpicco
Copy link
Contributor Author

My only concern is that we will also have to clean up all those one-off selections once we have a general solution…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants