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

cpu/stm32: fix KConfig modeling for STM32F1 / usbdev_synopsys_dwc2 #18741

Merged
merged 1 commit into from
Oct 14, 2022
Merged

cpu/stm32: fix KConfig modeling for STM32F1 / usbdev_synopsys_dwc2 #18741

merged 1 commit into from
Oct 14, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented Oct 14, 2022

Contribution description

This fixes incorrect module selection for STM32F1 boards with feature periph_usbdev, a regression introduced by #17812

Testing procedure

In master
~/Repos/software/RIOT $ BOARD=bluepill TEST_KCONFIG=0 make info-modules -C tests/sys_fido2_ctap > ~/mods-no-k 
~/Repos/software/RIOT $ BOARD=bluepill TEST_KCONFIG=1 make info-modules -C tests/sys_fido2_ctap > ~/mods-k 
~/Repos/software/RIOT $ diff ~/mods-no-k ~/mods-k 
--- /home/maribu/mods-no-k
+++ /home/maribu/mods-k
@@ -1,4 +1,6 @@
 make: Entering directory '/home/maribu/Repos/software/RIOT/tests/sys_fido2_ctap'
+=== [ATTENTION] Testing Kconfig dependency modelling ===
+=== [ATTENTION] Testing Kconfig dependency modelling ===
 auto_init
 auto_init_random
 auto_init_ztimer
@@ -85,6 +87,7 @@
 stm32_vectors
 sys
 tsrb
+usbdev_synopsys_dwc2
 usbus
 usbus_hid
 xtimer
This PR
~/Repos/software/RIOT $ BOARD=bluepill TEST_KCONFIG=0 make info-modules -C tests/sys_fido2_ctap > ~/mods-no-k 
~/Repos/software/RIOT $ BOARD=bluepill TEST_KCONFIG=1 make info-modules -C tests/sys_fido2_ctap > ~/mods-k 
~/Repos/software/RIOT $ diff ~/mods-no-k ~/mods-k 
--- /home/maribu/mods-no-k
+++ /home/maribu/mods-k
@@ -1,4 +1,6 @@
 make: Entering directory '/home/maribu/Repos/software/RIOT/tests/sys_fido2_ctap'
+=== [ATTENTION] Testing Kconfig dependency modelling ===
+=== [ATTENTION] Testing Kconfig dependency modelling ===
 auto_init
 auto_init_random
 auto_init_ztimer

Issues/PRs references

#17812

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 14, 2022
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 14, 2022
@maribu maribu enabled auto-merge October 14, 2022 01:59
@maribu maribu mentioned this pull request Oct 14, 2022
Merged
@riot-ci
Copy link

riot-ci commented Oct 14, 2022 •
edited
Loading

Murdock results

✔️ PASSED

ea53b35 cpu/stm32: fix KConfig modeling for STM32F1 / usbdev_synopsys_dwc2

Success Failures Total Runtime
1983 0 1983 06m:44s

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.

kaspar030
kaspar030 approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

gschorcht
gschorcht reviewed Oct 14, 2022
View reviewed changes
cpu/stm32/Makefile.dep Outdated
Comment on lines 12 to 14
ifneq (,$(filter stm32f103%,$(CPU_FAM)))
USEMODULE += usbdev_synopsys_dwc2
endif
ifneq (,$(filter stm32f105%,$(CPU_FAM)))
USEMODULE += usbdev_synopsys_dwc2
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ifneq (,$(filter stm32f103%,$(CPU_FAM)))
USEMODULE += usbdev_synopsys_dwc2
endif
ifneq (,$(filter stm32f105%,$(CPU_FAM)))
USEMODULE += usbdev_synopsys_dwc2
endif
ifneq (,$(filter stm32f105% stm32f107%,$(CPU_FAM)))
USEMODULE += usbdev_synopsys_dwc2
endif

???

Copy link
Contributor

Choose a reason for hiding this comment

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

STM32F103 has to use periph/usbdev_fs driver.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it also has to be CPU_MODEL not CPU_FAM :/ Another CI round 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it also has to be CPU_MODEL not CPU_FAM

Oops, yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like I'm getting erratic. But this time I think the fix is correct.

@maribu maribu force-pushed the cpu/stm32/KConfig branch from 63a5754 to 763bb58 Compare October 14, 2022 07:31
@maribu
cpu/stm32: fix KConfig modeling for STM32F1 / usbdev_synopsys_dwc2
ea53b35 
This fixes incorrect module selection for STM32F1 boards with feature
periph_usbdev, a regression introduced by
RIOT-OS#17812
@maribu maribu force-pushed the cpu/stm32/KConfig branch from 763bb58 to ea53b35 Compare October 14, 2022 08:05
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 14, 2022
@leandrolanzieri leandrolanzieri added this to the Release 2022.10 milestone Oct 14, 2022
gschorcht
gschorcht reviewed Oct 14, 2022
View reviewed changes
cpu/stm32/periph/Kconfig
# MODULE_USBDEV_SYNOPSYS_DWC2
select MODULE_USBDEV_SYNOPSYS_DWC2 if MODULE_PERIPH_USBDEV && !HAS_CPU_STM32WB && !HAS_CPU_STM32F3 && !HAS_CPU_STM32F1
# NOTE: In STM32F1 family STM32F105xx and STM32F107xx also use
# MODULE_USBDEV_SYNOPSYS_DWC2. Add those MCUs once the are added to
Copy link
Contributor

@gschorcht gschorcht Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

At least STM32F105xC and STM32F107xC seem to be modeled already. They are not used by any board for the moment.

benpicco
benpicco reviewed Oct 14, 2022
View reviewed changes
cpu/stm32/Makefile.dep
# In STM32F1 family STM32F105xx and STM32F107xx also use synopsys_dwc2
ifneq (,$(filter stm32f105% stm32f107%,$(CPU_MODEL)))
USEMODULE += usbdev_synopsys_dwc2
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Uh cpu/stm32/periph/Makefile will also need that fix

@maribu
Copy link
Member Author

maribu commented Oct 14, 2022

Let's do the remaining issues as follow up. Right now there are no boards with STM32F105xx and STM32F107xx MCU in RIOT, so this will not cause any issues for now.

I will open a PR to address #18741 (comment) and #18741 (comment) right after the feature freeze.

@maribu maribu mentioned this pull request Oct 14, 2022
Merged
@maribu maribu changed the title cpu/stm32: Fix KConfig modeling for STM32F1 / usbdev_synopsys_dwc2 cpu/stm32: fix KConfig modeling for STM32F1 / usbdev_synopsys_dwc2 Oct 14, 2022
@maribu
Copy link
Member Author

maribu commented Oct 14, 2022

Actually, no need to wait: Follow up is #18742

@maribu maribu merged commit ef8bd7c into RIOT-OS:master Oct 14, 2022
@maribu maribu deleted the cpu/stm32/KConfig branch October 14, 2022 10:48
@maribu
Copy link
Member Author

maribu commented Oct 14, 2022

Thx :)

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

@benpicco benpicco benpicco left review comments

@kaspar030 kaspar030 kaspar030 approved these changes

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

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

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

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

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2022.10
Development

Successfully merging this pull request may close these issues.
6 participants
@maribu @riot-ci @benpicco @kaspar030 @gschorcht @leandrolanzieri