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

Add set_frequency method on PWM channels #174

Merged
merged 1 commit into from
Jun 14, 2021
Merged

Conversation

azerupi
Copy link
Contributor

@azerupi azerupi commented Jun 11, 2021

Following discussion in #172 this adds a method to PWM channels to change the underlying timer's frequency. This allows to dynamically change the PWM frequency when we have already associated a channel with a pin. Otherwise this required unsafe to work around a partial move error because the channel was moved out of the timer struct.

In the implementation I changed clock_frequency on the Instance to not take &self. This allows it to be used in the channels. But this is not required, we could access the RCC register directly to avoid making a breaking change?

Compared to the timer's implementation we don't stop and start the timer before changing the frequency. I tested it on my project and it seems to work flawlessly. I'm not sure why that was required in the timer's implementation? If it needs to be added, is there a way to reuse the timer's implementation without copy-pasting?

Let me know if anything can be improved! 🙂

This adds a method to PWM channels to change the underlying timer's
frequency. This allows to dynamically change the PWM frequency when
we have associated a channel with a pin. Otherwise this required
unsafe to work around a partial move error because the channel was
moved out of the timer struct.

See stm32-rs#172 for more information
@azerupi
Copy link
Contributor Author

azerupi commented Jun 11, 2021

The clippy failure is the same as what I got in #173, it has been fixed there.

Copy link
Contributor

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you, @azerupi!

@hannobraun hannobraun merged commit 45e9349 into stm32-rs:master Jun 14, 2021
@azerupi
Copy link
Contributor Author

azerupi commented Jun 14, 2021

Great, thanks!

Just to make sure, you did see this part?

In the implementation I changed clock_frequency on the Instance to not take &self.

Because I think that is a breaking change since Instance is public. 🙂

@dbrgn
Copy link
Contributor

dbrgn commented Jun 14, 2021

Breaking changes are fine, we have breaking changes all the time. However, maybe you could send a PR that updates the CHANGELOG?

@hannobraun
Copy link
Contributor

I did see that, but thanks for pointing it out again. I could have been a bit less terse 🙂

While it is strictly speaking a breaking change, I don't think there's much reason for end users to touch Instance except in trait bounds. Its methods are more of an internal API, although Rust won't let us express that. So I think this change has pretty low impact, and given that, I believe that using unsafe to conjure up an Rcc is not worth it in this case.

Also, what @dbrgn said, we break things all the time 😄 In a perfect world, we'd take more steps to avoid/alleviate that, but we have to work with the resources we've got (on the maintainer and contributor side).

azerupi added a commit to azerupi/stm32l0xx-hal that referenced this pull request Jun 14, 2021
dbrgn pushed a commit that referenced this pull request Jun 25, 2021
jamwaffles pushed a commit to jamwaffles/stm32l0xx-hal that referenced this pull request Jul 26, 2021
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