STM32 SimplePwm: Fix regression and re-enable output pin #2670
Conversation
|
I left comments in Issue #2409 with my understanding what happened. Since this is the second time, that enable_outputs() got removed from SimplePwm, I added a lengthy comment. |
Ah, I'm terribly sorry for this mess. I didn't notice that You approach is correct on both old and new timer hierarchy. It requires a empty implement |
embassy-stm32/src/timer/mod.rs
Outdated
|
|
||
| /// Enable timer outputs. | ||
| fn enable_outputs(&self) { | ||
| Self::regs_1ch_cmp().bdtr().modify(|w| w.set_moe(true)); | ||
| } |
There was a problem hiding this comment.
could you restore this method? MOE existed on all TIM_1CH_CMP TIM_2CH_CMP and TIM_ADV.
There was a problem hiding this comment.
Or even better, rename this method to something like fn set_moe(&self, enable: bool), and call this method in public trait implement.
If you do so, please don't forget to adjust the doc for the new method.
There was a problem hiding this comment.
Replaced enable_outputs()by fn set_moe(&self, enable: bool)
embassy-stm32/src/timer/mod.rs
Outdated
| // SimplePwm<'d, T> is implemented for T: CaptureCompare16bitInstance | ||
| // Advanced timers implement this trait, but the output needs to be | ||
| // enabled explicitly. | ||
| // To support basic and advanced timers, this function is added |
There was a problem hiding this comment.
It better to say
"To support general-purpose and advanced timer, ..."
since there is already another timer called BasicInstance
There was a problem hiding this comment.
Good point. I changed the comment accordingly
embassy-stm32/src/timer/mod.rs
Outdated
| /// Enable timer outputs. | ||
| fn enable_outputs(&self) { | ||
| use crate::timer::sealed::AdvancedControlInstance; | ||
| let r = Self::regs_advanced(); | ||
| r.bdtr().modify(|w| w.set_moe(true)); |
There was a problem hiding this comment.
You might need to copy this code (or .set_moe(true)) to other 2 blocks: ($inst:ident, timer, TIM_1CH_CMP, UP, $irq:ident), ($inst:ident, timer, TIM_2CH_CMP, UP, $irq:ident), to make sure they also enable MOE.
There was a problem hiding this comment.
Thanks, I would have missed these locations.
Added fn enable_outputs(&self) to these timers, also.
|
Thanks for your fast response and don't worry about the mess. I can squash the commits, if you agree to the current state. |
Yeah! It looks good to me! And I also test PWM output on my dev board with TIM1(ADV), TIM15(2CH_CMP), TIM16(1CH_CMP), they all output correct waveform. |
PR embassy-rs#2499 implemented timer hierarchy, but removed enable_outputs() from trait CaptureCompare16bitInstance and from SimplePwm. This functions is required for advanced timers to set bit BDTR.MOE and to enable the output signal.
jr-oss
force-pushed
the
fix_regression_simple_pwm_output_enable
branch
from
d96926f to
b7bb4b2
Compare
|
Thanks. I squashed the commits. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
PR #2499 implemented timer hierarchy, but removed enable_outputs() from trait CaptureCompare16bitInstance and from SimplePwm.
This functions is required for advanced timers to set bit BDTR.MOE and to enable the output signal.