Added support for using variable period PWM timers by updating ARR in stead of CCR. #4143
Conversation
|
bender run |
|
@mickem needs one rustfmt fix, but CI should be unstuck now. |
|
Thanks for unsticking, formatting has been fixed... |
embassy-stm32/src/timer/low_level.rs
Outdated
| pub fn set_auto_reload_value(&self, value: u32) { | ||
| match T::BITS { | ||
| TimerBits::Bits16 => { | ||
| let value = unwrap!(u16::try_from(value)); |
There was a problem hiding this comment.
Do we want to have this unwrap here? Is this how other parts of the timer handle this?
We probably want to at least have a try version, or note in the function signature that this function WILL panic if its a 16-bit timer and you try to set a value above u16::MAX. Maybe we have an infallible version of the fn that takes a u16 and a fallible version that takes a u32?
There was a problem hiding this comment.
Ah, I see this is consistent with other methods. I don't love it, but it is at least consistent. I'd appreciate you adding a doc comment here, amd maybe on other methods like set_compare_value as well.
There was a problem hiding this comment.
I spent some time looking at different avenues.
The fundamental problem here is that various timer subclasses are not always compatible and do not expose a type.
There is a "bits" variable which can be used but I failed to use that on a macro to derive a type (that said I am new to macros).
But many of the register function could be helped by having a u16 or u32 depending on the type of counter.
Another option which I also explored was to return a Result<(),Error> that works but becomes a tad awkward as many functions which wrap these function are using u16 and thus we end up returning a "useless result" that has to be thrown away which is a tad error prone as unwrap!() will not reveal new possible errors in future enhancements (and would also break existing interfaces).
I ended up using your suggestion i.e. adding a comment and improving the error handling a bit yielding a proper panic.
For the future I think it would be good so look into how to handle timers as there is a lot of potential errors now where things are casted to various types which makes it hard to reason about the code. For instance what happens when I start a DMA transfer on a 32bit register with a pointer to a u16 or voce versa. I am a bit unsure. It would be easier to reason about this if the timers were typed to the correct type i.e. u16 for 16bit timers and u32 for 32 bit ones.
So exposing a type somewhere would make this easier I think (though that would likely require a lot of rearchitecting)...
Anyways, TLDR: I updated the PR with a comment and panic with error message.
This is a clone of the
waveform_chXfunctions but instead of settings CCR it sets ARR.The effect of this is that the waveform will have a variable period and a fixed pulse width.
I guess one could have added this to the existing macro but I felt this was cleaner.
Note 1: Apart from using arr this should be identical to the other method so I guess one could argue that a "super method" could be built or parts of the function could be reused but as this solved my problem and was quick and simple I went with it.
Note 2: I am unsure about the array type. The original waveform method has a u16 but I think it should be dynamic depending on the underlying timer used so it should be 32 bit for 32bit timers and 16 bit for 16 bit timers.
Note 3: The arr is I think "per timer" not "per channel" so it is possible that it might make more sense to use a single function and not one per channel.
So please do let me know if this makes sense and what makes the most sense regarding all the notes.