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

Fix kconfig mismatch breaking nightlies #18805

Merged
merged 1 commit into from
Oct 27, 2022
Merged

Fix kconfig mismatch breaking nightlies #18805

merged 1 commit into from
Oct 27, 2022

Conversation

MrKevinWeiss
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss commented Oct 27, 2022 •
edited
Loading

Contribution description

Nightlies are failing due to kconfig mismatch.
It would seem this is a result of bringing in the USB stuff. I assume that this uses ztimer periph_timer as a backend as periph_timer is already selected. However, kconfig only resolves one and not recursively making it hard to match. For not a hack is added to override for these boards.

Testing procedure

Fixed in murdock with NIGHTLY=1

Issues/PRs references

@github-actions github-actions bot added Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: timers Area: timer subsystems labels Oct 27, 2022
@MrKevinWeiss
Copy link
Contributor Author

Still running local tests... So far so good.

@MrKevinWeiss MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 27, 2022
@github-actions github-actions bot added the Area: CI Area: Continuous Integration of RIOT components label Oct 27, 2022
@MrKevinWeiss
Copy link
Contributor Author

If we really want to be efficient I can add in another commit for the other broken nightly:

diff --git a/pkg/tinyusb/Kconfig b/pkg/tinyusb/Kconfig
index c1224f5b59..7836aab807 100644
--- a/pkg/tinyusb/Kconfig
+++ b/pkg/tinyusb/Kconfig
@@ -32,6 +32,7 @@ menuconfig PACKAGE_TINYUSB
     select MODULE_TINYUSB_PORTABLE_SYNOPSYS_DWC2 if CPU_STM32 && CPU_FAM_L4
     select MODULE_TINYUSB_PORTABLE_STM32_FSDEV if CPU_STM32 && CPU_FAM_F0
     select MODULE_TINYUSB_PORTABLE_STM32_FSDEV if CPU_STM32 && CPU_FAM_F1
+    select MODULE_TINYUSB_PORTABLE_STM32_FSDEV if CPU_STM32 && CPU_FAM_F3
     select MODULE_TINYUSB_PORTABLE_STM32_FSDEV if CPU_STM32 && CPU_FAM_G4
     select MODULE_TINYUSB_PORTABLE_STM32_FSDEV if CPU_STM32 && CPU_FAM_L0
     select MODULE_TINYUSB_PORTABLE_STM32_FSDEV if CPU_STM32 && CPU_FAM_WB

@MrKevinWeiss
Copy link
Contributor Author

Anyone opposed?

@riot-ci
Copy link

riot-ci commented Oct 27, 2022 •
edited
Loading

Murdock results

✔️ PASSED

6b5c75c boards/blxxpill-128kib: Fix kconfig mismatch

Success Failures Total Runtime
1992 0 1992 06m:12s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@leandrolanzieri
Copy link
Contributor

If we really want to be efficient I can add in another commit for the other broken nightly:

Go ahead, just adapt the PR title and description

leandrolanzieri
leandrolanzieri reviewed Oct 27, 2022
View reviewed changes
boards/blackpill-128kib/Kconfig Outdated
# make will select timer backend, probably due to the USBUS
# and kconfig cannot select if something is already selected like make
choice CHOICE_ZTIMER_MSEC_BACKEND
default ZTIMER_MSEC_BACKEND_TIMER if MODULE_STDIO_UART_RX
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is due to USBUS why does it depend on uart rx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

USBUS_STDIO eventually selects this due to STDIN. I didn't dig down as this is a hack. I just noticed that without this conditional examples/hello-world fails. That somewhat clued me in that is is to do with the STDIN stuff.

I am hoping that this will eventually get cleaned up nicely making the hack obsolete. Maybe something @jue89 would be interested in taking on 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind it makes it fail again...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I was talking about STDIN and used something completely different. Local tests indicate this is fixed!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I'm far away from obtaining the black belt in Kconfig modeling :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I'm far away from obtaining the black belt in Kconfig modeling :D

In case you're interested ;) besides the ultimate word in this https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html I found useful https://docs.zephyrproject.org/latest/build/kconfig/tips.html

Many of the recommendations were taken early on in the design, many others had to be gradually discarded (e.g., select). Still it's a good read to understand the trade-offs that one makes when using one approach instead of the other.

@github-actions github-actions bot added the Area: pkg Area: External package ports label Oct 27, 2022
@MrKevinWeiss MrKevinWeiss changed the title boards/blxxpill-128kib: Fix kconfig mismatch Fix kconfig mismatch breaking nightlies Oct 27, 2022
leandrolanzieri
leandrolanzieri reviewed Oct 27, 2022
View reviewed changes
sys/ztimer/Kconfig Outdated Show resolved Hide resolved
@MrKevinWeiss
Copy link
Contributor Author

maybe @gschorcht can comment to indicate the correct behaviour for the nucleo-f303ze, should it be like make or like Kconfig?

@MrKevinWeiss
Copy link
Contributor Author

Murdock results

x FAILED

a4a9036 fixup! REMOVEME: Testin nightlies with boards

Build failures (4)

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

Good bot!

@MrKevinWeiss
Copy link
Contributor Author

oh god, not good.

@MrKevinWeiss
Copy link
Contributor Author

Ahh cool, if we have xtimer then make uses both ztimer_periph_rtt and ztimer_periph_timer or is it just a usec and msec interaction? Investigating...

gschorcht
gschorcht reviewed Oct 27, 2022
View reviewed changes
pkg/tinyusb/Kconfig Outdated
@@ -32,6 +32,7 @@ menuconfig PACKAGE_TINYUSB
select MODULE_TINYUSB_PORTABLE_SYNOPSYS_DWC2 if CPU_STM32 && CPU_FAM_L4
select MODULE_TINYUSB_PORTABLE_STM32_FSDEV if CPU_STM32 && CPU_FAM_F0
select MODULE_TINYUSB_PORTABLE_STM32_FSDEV if CPU_STM32 && CPU_FAM_F1
select MODULE_TINYUSB_PORTABLE_STM32_FSDEV if CPU_STM32 && CPU_FAM_F3
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I'm wondering why this wasn't catched by Murdock. Anyway, this selection is buggy and needs to be fixed, which is done by PR #18787.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will drop it then!

Copy link
Contributor

Choose a reason for hiding this comment

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

You can leave it for now, I don't know how long it will take to get PR #18787 merged.

@gschorcht
Copy link
Contributor

maybe @gschorcht can comment to indicate the correct behaviour for the nucleo-f303ze, should it be like make or like Kconfig?

Makefiles are correct, Kconfig was just missing the fix in ba9649b.

@gschorcht
Copy link
Contributor

It would seem this is a result of bringing in the USB stuff.

cpu/stm32/periph/usbdev_fs.c uses the ztimer_msec module to produce a small delay when pulling down the USB D+ line during reset. This is modeled in makefiles and Kconfig as well. It seems that the problem only occurs for the 128k variants of blxxxpill where the USB CDC ACM is used as stdio.

@github-actions github-actions bot removed the Area: pkg Area: External package ports label Oct 27, 2022
@MrKevinWeiss
Copy link
Contributor Author

Hm, I'm wondering why this wasn't catched by Murdock

Kconfig tests in the PR only test a subset of boards on all apps. #18803 may be of interest to you to locally test the full CPU.

@MrKevinWeiss
boards/blxxpill-128kib: Fix kconfig mismatch
6b5c75c 
Nightlies are failing due to kconfig mismatch.
It would seem this is a result of bringing in the USB stuff.
I assume that this uses ztimer periph_timer as a backend as periph_timer is already selected.
However, kconfig only resolves one and not recursively making it hard to match.
For not a hack is added to override for these boards.
@MrKevinWeiss
Copy link
Contributor Author

This seems to fix it only the nucleo is failing for an out of scope reason. Should I drop the REMOVEME?

...man I am pretty bad an managing multiple PRs at once...

@github-actions github-actions bot removed the Area: CI Area: Continuous Integration of RIOT components label Oct 27, 2022
@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 27, 2022
@benpicco benpicco merged commit 1481df8 into RIOT-OS:master Oct 27, 2022
@MrKevinWeiss MrKevinWeiss deleted the pr/fix/nightlybpfail branch October 31, 2022 05:10
@MrKevinWeiss
Copy link
Contributor Author

Thanks!

@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gschorcht gschorcht gschorcht left review comments

@jue89 jue89 jue89 left review comments

@benpicco benpicco benpicco left review comments

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2023.01
Development

Successfully merging this pull request may close these issues.
7 participants
@MrKevinWeiss @riot-ci @leandrolanzieri @gschorcht @benpicco @jue89 @kaspar030