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

boards/nucleo-f429zi: Provide 2nd timer #19447

Merged
merged 2 commits into from
Apr 17, 2023
Merged

boards/nucleo-f429zi: Provide 2nd timer #19447

merged 2 commits into from
Apr 17, 2023

Conversation

maribu
Copy link
Member

@maribu maribu commented Apr 4, 2023

Contribution description

  • Add a common timer config with two (instead of only one) timer using TIM2 + TIM5
    • Mostly copy-pasting the cfg_timer_tim2.h and cfg_timer_tim5.h together
  • Make use of that for the nucleo-f429zi

Testing procedure

E.g. tests/periph_timer, but also grepping for TIM2 and TIM5 in boards/nucleo-f429zi/include/periph_conf.h to detect any conflict e.g. between PWM and timer config.

Issues/PRs references

None

@github-actions github-actions bot added the Area: boards Area: Board ports label Apr 4, 2023
@benpicco benpicco added 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 and removed Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Apr 4, 2023
@riot-ci
Copy link

riot-ci commented Apr 4, 2023 •
edited
Loading

Murdock results

✔️ PASSED

d7923f4 boards/nucleo-f429zi: Use common TIM5 + TIM2 timer conf

Success Failures Total Runtime
6882 0 6882 09m:38s

Artifacts

@gschorcht
Copy link
Contributor

Would this change also be valid for stm32f429i-disc1?

kaspar030
kaspar030 reviewed Apr 17, 2023
View reviewed changes
boards/common/stm32/include/cfg_timer_tim2_and_tim5.h Outdated
},
{
.dev = TIM5,
.max = 0xffffffff,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for the sorting?
If TIM5 is always available and always 32bit, it should be the first in the array, to trickle down as default for ztimer_used.

Copy link
Member Author

Choose a reason for hiding this comment

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

At least on the L4 it indeed seems to be available and 32 bit. I will swap the two.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should have looked more closely. The exceptions are L0 and L1 where TIM2 is not 32 bit, not all STM32L families. Let me check for those as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

RIOT/boards/nucleo-l152re/doc.txt

Line 29 in 1961274

| Timers | 12 (8x 16-bit, 1x 32-bit [TIM5], 1x Systick, 2x watchdog) |

Yep, at least on STM32L1 the sorting helps ztimer

@maribu
Copy link
Member Author

maribu commented Apr 17, 2023

Would this change also be valid for stm32f429i-disc1?

Likely, but I don't have the hardware to test. I intentionally split out the timer config as separate header file so that using it for other boards is a one-line-diff PR.

@maribu maribu force-pushed the boards/nucleo-f429zi/2nd_timer branch 2 times, most recently from a5241b7 to bb571db Compare April 17, 2023 08:07
@maribu
Copy link
Member Author

maribu commented Apr 17, 2023

Maybe I should also call the file tim5_and_tim2 then with the new sorting

@maribu maribu force-pushed the boards/nucleo-f429zi/2nd_timer branch 2 times, most recently from 7f77ed2 to b3d6eaa Compare April 17, 2023 08:14
@maribu
boards/common/stm32: Add timer conf for TIM5 + TIM2
190cfdb 
This adds a common configuration file that provides two periph timers
using TIM5 and TIM2.
@maribu
boards/nucleo-f429zi: Use common TIM5 + TIM2 timer conf
d7923f4
@maribu maribu force-pushed the boards/nucleo-f429zi/2nd_timer branch from b3d6eaa to d7923f4 Compare April 17, 2023 10:25
@maribu
Copy link
Member Author

maribu commented Apr 17, 2023

CI is hopefully happy now :)

@gschorcht
Copy link
Contributor

Would this change also be valid for stm32f429i-disc1?

Likely, but I don't have the hardware to test. I intentionally split out the timer config as separate header file so that using it for other boards is a one-line-diff PR.

I could test it, I have the hardware. But since stm32f429i-disc doesn't provide a PWM config, we don't have to expect conflicts.

@maribu
Copy link
Member Author

maribu commented Apr 17, 2023

All seems to be green :)

benpicco
benpicco approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

bors merge

@bors
Copy link
Contributor

bors bot commented Apr 17, 2023

Build succeeded:

@bors bors bot merged commit 023cf78 into RIOT-OS:master Apr 17, 2023
@maribu maribu deleted the boards/nucleo-f429zi/2nd_timer branch April 18, 2023 06:13
@maribu
Copy link
Member Author

maribu commented Apr 18, 2023

Thx :)

@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 kaspar030 left review comments

@benpicco benpicco benpicco approved these changes

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

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2023.07
Development

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