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

RFC: docs and examples for custom selection of PWM channels #60

Merged
merged 4 commits into from
May 25, 2019
Merged

RFC: docs and examples for custom selection of PWM channels #60

merged 4 commits into from
May 25, 2019

Conversation

geomatsi
Copy link
Contributor

Hi all,

In practical applications various selections of PWM channels may be needed. For instance, one or two channels attached to gpio pins for non-default remapping. It is possible to keep adding implementations of the 'Pins' trait into pwm module for all the needed channel/pin combinations. However it does not look practical unless there exist some clever way to describe all possible combinations w/o boilerplate code.

Another option is to keep only basic implementations in pwm module, e.g. 'all the channels for default pin remapping'. As for the custom channel selections, it is possible to use newtype pattern to implement 'Pins' trait in the user application. This PR adds documentation and example demonstrating the use of newtype pattern for creation of custom PWM channel configuration.

Thoughts ? Comments ?

Regards,
Sergey

@TheZoq2
Copy link
Member

TheZoq2 commented May 4, 2019

I like the idea of this, however i'm not super familiar with how the pwm module works so I don't think I can give a ton of feedback on it.

Would you be able to add some documentation to the example explaining how to set the REMAP and C<1-4> values in the newtype?

@geomatsi
Copy link
Contributor Author

geomatsi commented May 5, 2019

Hi @TheZoq2 ,
Good point. I will add a more detailed explanation with references to MCU datasheet.

Regards,
Sergey

@therealprof
Copy link
Member

@geomatsi Very nice, I like it. Can't test it myself at the moment, though.

@geomatsi
Copy link
Contributor Author

geomatsi commented May 7, 2019

Hi all,

I added a bit more docs and examples in order to clarify how to define custom channel selections and how to implement Pins trait for them. Suggested approach has been tested using on stm32f101 hardware, e.g. here is a single channel example.

By the way, I would appreciate if you could take a look at docs phrasing and check if anything needs correction from the language point of view.

Regards,
Sergey

@kinnison
Copy link

A brief skim of this looks good from a language perspective. I can confirm that this clarified things to the point that I was able to implement the Pwm capability I desired for my STM32F103 based firmware.

👍

src/pwm.rs Outdated
@@ -126,7 +196,7 @@ impl Pins<TIM4>
PB9<Alternate<PushPull>>,
)
{
const REMAP: u8 = 0b0;
const REMAP: u8 = 0b00;

Choose a reason for hiding this comment

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

I think this change is wrong. TIM4_REMAP is, afaict, only one bit.

Zero means PB6,7,8,9 ånd One means PD12,13,14,15

Choose a reason for hiding this comment

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

I mean, the change is harmless enough, but it could lead somene to think that the value was wider than one bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch ! I will push a fixup.

Copy link
Member

@TheZoq2 TheZoq2 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I added two nitpicky comments that would make it perfect, after those are fixed I'd be happy to merge it!

src/pwm.rs Outdated Show resolved Hide resolved
src/pwm.rs Outdated Show resolved Hide resolved
Provide documentaion and example for custom channel configurations
using newtype pattern to implement Pins trait.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
Add more details and examples for custom channel selection.
Clarify where to get REMAP and Cx values.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
TIM4_REMAP is only one bit:
0b0 -> PB6, PB7, PB8, PB9
0b1 -> PD12, PD13, PD14, PD15

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
- phrasing
- specify AFIO/TIM section from TRM

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
@geomatsi
Copy link
Contributor Author

Hi @TheZoq2
PR has been updated with fixes according to your comments.

BTW, I am not sure why TravisCI tries to build the same PR changes twice...

Regards,
Sergey

@TheZoq2
Copy link
Member

TheZoq2 commented May 25, 2019

Nicely done, thanks!

@TheZoq2 TheZoq2 merged commit 2e15add into stm32-rs:master May 25, 2019
@geomatsi geomatsi deleted the tim3-remap branch May 26, 2019 19:57
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.

None yet

4 participants