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

stm32: Add the ability to center-align timers #1991

Merged
merged 6 commits into from
Oct 20, 2023

Conversation

diondokter
Copy link
Contributor

Draft because I still need to test it on hardware.

Added it as a nice enum type in place of just the direction.
This is the most straightforward implementation I think.

The reason for adding this is because it enables more fancy PWM usages using the existing simple and complementary pwm implementations.

I've made a variant for each of the interrupt settings for the center-aligned mode. Should that be separated? Or is this combination fine?

@jr-oss
Copy link
Contributor

jr-oss commented Oct 2, 2023

I would prefer separate parameters, since you need the edge/center aligned mode for frequency calculation.

I also have a toy project, that requires center aligned mode.
You can find my extension to SimplePwm here: main...jr-oss:embassy:simple_pwm_with_center_aligned_mode.

In edge aligned mode a cycle corresponds to the reload value, while in center aligned mode each cycle counts up and down, i.e. you need to half the reload value for the same frequency.

I chose a generic parameter that contains the configuration and frequency multiplier as const to configure center vs. edge aligned mode which avoids storage and runtime costs.
In my application center/edge aligned mode is also a compile time decision.

Disclaimer: I haven't run any unit tests nor have I fully understood all the trait dependencies, and so my solution might also be invalid.

Usage:

    let mut buzzer_pwm = simple_pwm::SimplePwm::<'_,  _, simple_pwm::CmsCenterAlignedMode1>::new(
        p.TIM1,
        Some(PwmPin::new_ch1(p.PE9, OutputType::PushPull)),
        None,
        Some(PwmPin::new_ch3(p.PE13, OutputType::PushPull)),
        None,
        Hertz::hz(3300),
    );

@diondokter
Copy link
Contributor Author

In edge aligned mode a cycle corresponds to the reload value, while in center aligned mode each cycle counts up and down, i.e. you need to half the reload value for the same frequency.

Ah good point, yeah it changes the frequency of the PWM. The frequency of the timer clock remains the same. This feels like something the PWM should fix, not the underlying timer.

I like your solution too with it being a typestate param. Though it makes the SimplePWM a bit less simple haha.

@Dirbaio
Copy link
Member

Dirbaio commented Oct 2, 2023

Yes, we should avoid typestates if the feature can be implemented with plain old enums.

@jr-oss
Copy link
Contributor

jr-oss commented Oct 2, 2023

I am also fine with enums.
If I were fully convinced by my version, I would have started a PR ;-)

Just wondering, which is the right approach to include the frequency factor.

  • add it to SimplePwm, making in non-ZST
  • read and match cr1.cms each time the frequency is set

If the frequency is only set initially, the latter approach is a good solution, as the configuration is already known.
When the frequency changes during runtime, it comes at a cost.

@diondokter
Copy link
Contributor Author

diondokter commented Oct 2, 2023

Yeah, do we sacrifice a byte of ram or a runtime memory read?

Although thinking about it, reading the enum from ram is pretty much the same as reading it from a register. So I guess keeping a ZST is still fine.

Btw, personally, I'm changing the frequency multiple times per second :P

@diondokter
Copy link
Contributor Author

Hmmm, what I dislike is that the basic timer doesn't have a center-aligned mode, but the frequency function is on the basic timer trait and so is not able account for the alignment. And Rust doesn't give us the ability to override a default fn with another one.

So the frequency as defined on the trait will always be the frequency from one extreme to the other extreme, instead of the frequency from 0 to 0. That might be fine idk... Maybe putting a doc comment on the function explaining this is enough?

@diondokter diondokter marked this pull request as ready for review October 20, 2023 12:44
@diondokter
Copy link
Contributor Author

Confirmed working on my STM32G0

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thanks!

@Dirbaio Dirbaio added this pull request to the merge queue Oct 20, 2023
Merged via the queue into embassy-rs:main with commit b1d0947 Oct 20, 2023
@diondokter diondokter deleted the center-align branch October 20, 2023 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants