-
Notifications
You must be signed in to change notification settings - Fork 122
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
Refactor timer taxonomy #495
base: main
Are you sure you want to change the base?
Conversation
Marked as draft because I want to double-check that I didn't introduce any unintentional breakage (in particular in the v2 and l0 versions). |
This PR should now be ready for review ✔️ |
can't you already do this by unsafely casting a Tim1ch to a Tim4ch? For example, when you construct a SimplePwm, store it in IMO this is better than changing the registers to allow 4ch in the 1ch timers, because: There's 2 kinds of users of the PAC:
increasing the number of channels impacts end users (they can accidentally try using channels that don't exist), while requiring unsafe casting impacts only HAL authors, which are likely more "sophisticated" users. |
Sure, it's possible to treat any general-purpose timer as a
I agree that this change makes life slightly harder for people who use this crate directly, but it should make life significantly easier for people who use Embassy. In the current situation, we protect end users of the PAC crate from using non-existent channels in a timer (i.e., if they try to use channel 3 in a 2-channel timer, they get a compile-time or a runtime error), but we don't protect end users of Embassy from using the wrong timer type (e.g., if they try to use With the proposed changes, we won't be able to protect the users of the PAC from using more channels, but we will be able to ensure that users of Embassy use timer peripherals that actually support all operations that they need. In my opinion, it's a good trade-off: it's quite simple to understand that a 2-channel timer has only two channels, but understanding all types of STM32 timers and their features takes a few days of detailed studying of reference manuals. |
df76baa
to
ce4feb6
Compare
This PR accompanies embassy-rs/embassy#3123. It simplifies the register definitions for timers, unifies the naming of timers and supports the type-safe timer taxonomy from that PR.
However, as a necessary trade-off, it exposes registers and register fields for all channels even for timers that don't have the full number of channels: for example, all four CCR registers are exposed for
Tim1ch
. The idea here is thatTim1ch
represents a timer that is at least as capable as a 1-channel timer, but might have up to 4 channels. For example, this allows us to implement a PWM driver that works for all timers with channels (it requires only the functionality ofTim1ch
), but still supports up to 4 channels for timers that have them (so it can be used withTim4ch
orTimAdv2ch
).The idea is that the higher-level code ensures that only the available channels are used. For example, the
SimplePwm
driver ensures this by accessing only those channels that correspond to pins that belong to the timer peripheral: if there is a pin for channel 3, then the timer surely supports channel 3!