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

[1/3] Timer refactor: system timer #2576

Merged
merged 6 commits into from
Nov 22, 2024
Merged

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Nov 20, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

A fairly radical change, but I realized that it's basically impossible to treat the Unit's as a resource and allow users to actually make modifications (FrozenUnit prevented this before). Unit operations now go through the SystemTimer struct, and are all unsafe with safety comments explaining to the user how to use it correctly. Opening early for feedback before I update all the examples.

TL;DR:

  • Removes all type params on Alarm
  • Systimer no longer implements peripheral ref, the peripheral ref
    pattern is instead intended to be used on the higher level timer
    drivers (OneShot, Periodic)
  • Removed Unit as a type, in favour of an enum
  • Alarms are back in the main SystemTimer "driver"
  • Made all Unit modification methods unsafe, it's not possible to
    modify the Unit's safely whilst timers and or the time::now API is
    in use

@MabezDev MabezDev changed the title system timer simplfication [1/N] Timer refactor: system timer Nov 20, 2024
@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 21, 2024

I welcome the simplification here especially since we are encouraging users to just use the higher-level timers and use the hw-timers to back them

I don't think users did anything but use split before in which case all the generics were just annoying best case.

Not sure if even the now unsafe functionality is useful, but it probably doesn't hurt to have it (while it would hurt to use it wrong - that's why it's unsafe)

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 21, 2024

Unrelated but I think the cycles function in examples/src/lib.rs is unused and t.b.h. it should - the examples should be self contained (I remember the hate the example-simplification-macros in esp-wifi got back then)

@MabezDev
Copy link
Member Author

Another question I'm not sure about, should Alarm's be ZSTs? I.e I erased the alarm number to simplify AnyTimer, but maybe that was a step too far for a "dumb timer"?

@Dominaezzz
Copy link
Collaborator

Another question I'm not sure about, should Alarm's be ZSTs? I.e I erased the alarm number to simplify AnyTimer, but maybe that was a step too far for a "dumb timer"?

The goal is simplicity right? ZSTs add more type complexity, which goes against that.
Unless there's something you're concerned about losing by not using ZSTs.

@MabezDev MabezDev force-pushed the dumb-timer/systimer branch 2 times, most recently from a922002 to ba41b9c Compare November 21, 2024 12:30
@MabezDev MabezDev added the skip-changelog No changelog modification needed label Nov 21, 2024
@MabezDev
Copy link
Member Author

Skip changelog for embassy and esp-wifi changes, whilst technically breaking, they're both tied to esp-hal releases so it won't be breaking when they upgrade

@MabezDev MabezDev changed the title [1/N] Timer refactor: system timer [1/4] Timer refactor: system timer Nov 21, 2024
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@playfulFence playfulFence left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

A few very nitpicky comments which you can feel free to ignore, but otherwise LGTM

examples/src/lib.rs Outdated Show resolved Hide resolved
esp-hal/src/timer/systimer.rs Outdated Show resolved Hide resolved
esp-hal/src/timer/systimer.rs Outdated Show resolved Hide resolved
esp-hal/src/timer/systimer.rs Outdated Show resolved Hide resolved
esp-hal/src/timer/systimer.rs Outdated Show resolved Hide resolved
esp-hal/src/timer/systimer.rs Outdated Show resolved Hide resolved
* Removes _all_ type params on Alarm
* Systimer no longer implements peripheral ref, the peripheral ref
  pattern is instead intended to be used on the higher level timer
  drivers
* Removed `Unit` as a type, in favour of an enum
* Alarms are back in the main `SystemTimer` "driver"
* Made all `Unit` modification methods unsafe, it's not possible to
  modify the `Unit`'s safely whilst timers and or the `time::now` API is
  in use
Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

Thanks

@bugadani bugadani added this pull request to the merge queue Nov 22, 2024
Merged via the queue into esp-rs:main with commit 60a2c76 Nov 22, 2024
28 checks passed
@MabezDev MabezDev changed the title [1/4] Timer refactor: system timer [1/3] Timer refactor: system timer Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants