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

Pwm channels #34

Merged
merged 78 commits into from
Dec 2, 2019
Merged

Pwm channels #34

merged 78 commits into from
Dec 2, 2019

Conversation

IamfromSpace
Copy link
Contributor

This supports 95% of all device/channel/pin combinations. Those that are missing seem to not yet have register support from the underlying stm32f3 crate and mostly just stm32f373 is affected.

As noted from the previous issue, this interface is a bit different from the implementation in the f1xx-hal. The key thing is that the pins are provided after the channels are returned.

In the f1xx approach there's a combinatoric explosion of possible pin combos. Just counting common pins for TIM2 there are nearly 2^11 pin combos--so a macro approach is intractable. Instead, the API enables a bring-your-own-impl for any desired pin combo working. This a clever approach and can often be quite useful, but it's also overhead for a user. This alternative approach "just works."

There are also has numerous guard rails in place while remaining intuitive. Timers/channels/pins not supported by a device cannot be accessed. Channels cannot be enabled until they have pins. Invalid pins cannot be supplied to a channel. Pins cannot be modified or shared after usage. A channel may use regular or complementary pins but not both.

When using type inference, the mechanisms behind these invariants is fairly invisible.

This is a large PR, so I'm happy to take the time to address any and all feedback. I mostly wanted to show that this approach could in fact cover all cases.

Closes #23

… TIM8 because a trait doesn't make sense. This should still be moved to the pwm module.
…annel config into the pin configuration step, and add support for TIM16 (which has only one channel) to prove out this functionality.
…pins, and use typestates to ensure a channel uses one or the other, allow TIM8_CH3 to output to PB1 to use this feature.
…ry for when true complementary PWM is implemented.
@IamfromSpace
Copy link
Contributor Author

After merging in the most recent PR for f303 features, it now fails to build (all other devices work). Error is that port D/E/F aren't available, but that doesn't quite make sense to me. Hoping someone might have better insight as to how the previous changes might have affected this!

@strom-und-spiele
Copy link
Collaborator

This is due to different functionalities on the different sub-versions of the f303.
@russellmcc wrote:

One subtlety here is backwards compatibility, since previously selecting "stm32f303" would give you access to, e.g., the whole E port, but now you don't get access to it until you select a specific chip like "stm32f303xd" which has the E port. I think this is okay since the E port doesn't work on all stm32f303 chips, but just wanted to mention it.

see: #32

@IamfromSpace
Copy link
Contributor Author

@strom-und-spiele Ah, thanks for the clarification. Was looking in the wrong spots before! I'm a bit dubious about the ability to support sub-devices, but I see how the USB features don't leave any other option, so this makes sense. Fixed now!

@strom-und-spiele
Copy link
Collaborator

@IamfromSpace hth,
I'm not so sure on how to approach this issue with sub-devices either.
You looking around in the wrong places makes me suggest to really break backwards compatibility and force the usage of the correct subversion. After all (when using this crate), this is a thing you figure out once, put it in your Cargo file and never think about again. And imho it's not harder to determine the full version than just the main one.

The crass alternative (not supporting sub-devices) is something I think would hold back the development of this crate.

@dfrankland
Copy link
Member

I am all up for breaking backwards compatibility. This crate is no where near 1.0.0 so I think it's natural to make some changes for the better.

@IamfromSpace
Copy link
Contributor Author

@strom-und-spiele @dfrankland agreed all around. Supporting subdevices seems like a must, just quite a challenge today. Having been working with a the device difference a lot lately, I’ve got some crazy ideas I’ll open a separate issue for—assuming I can get it concrete enough.

For this PR these changes were simple enough and are definitely the right way to go!

Copy link
Member

@dfrankland dfrankland left a comment

Choose a reason for hiding this comment

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

I was able to build all device features and ran a simple regression test without issues 👍

src/pwm.rs Show resolved Hide resolved
Co-Authored-By: Dylan Frankland <dfrankland@users.noreply.github.com>
@dfrankland
Copy link
Member

Awesome, thank you!

@dfrankland dfrankland merged commit 4fd3ae6 into stm32-rs:master Dec 2, 2019
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.

Support PWM Output
3 participants