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

Implement full embedded_hal::Pwm #176

Merged
merged 36 commits into from
Mar 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
ed4f6f0
Add a MicroSecond time unit
justacec Jan 21, 2020
5050dda
Implement Into<Hertz> for the different time types
justacec Jan 21, 2020
f37b17c
Adjust whitespace
justacec Jan 21, 2020
8d0012c
Change the Pwm struct to be PwmChannel to make room for a new Pwm struct
justacec Jan 21, 2020
5a2f9f6
Make PwmChannel Copy
justacec Jan 21, 2020
bb0113d
Actually add the Copy/Clone implementations for the specific types
justacec Jan 21, 2020
4dfe267
Add new Pwm struct and fix return types
justacec Jan 21, 2020
a275dc1
Make Pwm Deref and DerefMut for backwards compatability
justacec Jan 21, 2020
69235c7
Add the core::ops Deref and DerefMut modules
justacec Jan 21, 2020
8d26086
Implement the new Pwm class to be compliant with embedded_hal::Pwm
justacec Jan 21, 2020
4a0a15f
Update example for the Pwm
justacec Jan 21, 2020
7175ebe
Reduce complexity of macro call
justacec Jan 21, 2020
0a555c9
Merge pull request #1 from stm32-rs/master
justacec Jan 21, 2020
89ef755
Merge branch 'master' into justacec_implement_full_pwm_hal
justacec Jan 21, 2020
a6bbac1
Remove accedental merge conflict messages
justacec Jan 21, 2020
06f5a47
Correct typo in code comment
justacec Jan 22, 2020
6a6b08a
Simplify the get_period function
justacec Jan 22, 2020
0cbc67f
Temporarally remove the Clone and Copy instances
justacec Jan 22, 2020
a77eea3
Remove PhantomData from the _channels field of the Pwm struct
justacec Jan 22, 2020
b22ff3b
Add an `into_channels(&self)` method to split out the channels
justacec Jan 22, 2020
a6f21b4
Update the Deref and DerefMut impl
justacec Jan 22, 2020
ca3b1a9
Remove unecessary [#derive] and [#allow] directives
justacec Jan 22, 2020
87179b1
Corrected the order for milli and micro (cant beleive I did that)
justacec Jan 22, 2020
cf5a152
Update to @burrbull suggested techniques
justacec Jan 25, 2020
62ec7bd
Shift comments to correct spot and update indent
justacec Jan 25, 2020
b2b3a07
Update Pwm examples to use new format
justacec Jan 25, 2020
aa3c331
Removed old commented code
justacec Jan 25, 2020
4c4c2d1
Change check_enabled to check_used and return Channel vice Option
justacec Jan 25, 2020
1098ab2
Use channel C3 in the code to match the comments
justacec Jan 25, 2020
a1c54ff
Merge remote-tracking branch 'upstream/master'
justacec Mar 14, 2020
cedb98f
Merge branch 'master' into justacec_implement_full_pwm_hal
justacec Mar 14, 2020
bca666d
Did not actually commit the merge fixes..
justacec Mar 15, 2020
16431d3
Re-adjust the return types of the pwm methods
justacec Mar 15, 2020
013478a
Added changelog message for Pwm implementation
justacec Mar 30, 2020
7904998
Merge remote-tracking branch 'upstream/master'
justacec Mar 31, 2020
abb1a17
Merge branch 'master' into justacec_implement_full_pwm_hal
justacec Mar 31, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

- Extend the Pwm implementation to cover the full embedded_hal::Pwm API
- Replace default blocking spi Write implementation with an optimized one
- Use `Deref` for SPI generic implementations instead of macros
- Make traits `rcc::Enable` and `rcc::Reset` public, but `RccBus` sealed
Expand Down
51 changes: 42 additions & 9 deletions examples/pwm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ use stm32f1xx_hal::{
prelude::*,
pac,
timer::{Tim2NoRemap, Timer},
time::U32Ext,
pwm::Channel
};
use cortex_m_rt::entry;

Expand Down Expand Up @@ -49,25 +51,56 @@ fn main() -> ! {
// let c4 = gpiob.pb9.into_alternate_push_pull(&mut gpiob.crh);

let mut pwm = Timer::tim2(p.TIM2, &clocks, &mut rcc.apb1)
.pwm::<Tim2NoRemap, _, _, _>(pins, &mut afio.mapr, 1.khz())
.2;
.pwm::<Tim2NoRemap, _, _, _>(pins, &mut afio.mapr, 1.khz());

//// Operations affecting all defined channels on the Timer

// Adjust period to 0.5 seconds
pwm.set_period(500.ms());

asm::bkpt();

// Return to the original frequency
pwm.set_period(1.khz());

asm::bkpt();

let max = pwm.get_max_duty();

pwm.enable();
//// Operations affecting single channels can be accessed through
//// the Pwm object or via dereferencing to the pin.

// Use the Pwm object to set C3 to full strength
pwm.set_duty(Channel::C3, max);

asm::bkpt();

// Use the Pwm object to set C3 to be dim
pwm.set_duty(Channel::C3, max / 4);

asm::bkpt();

// Use the Pwm object to set C3 to be zero
pwm.set_duty(Channel::C3, 0);

asm::bkpt();


// Extract the PwmChannel for C3
let mut pwm_channel = pwm.split().2;

// full
pwm.set_duty(max);
// Use the PwmChannel object to set C3 to be full strength
pwm_channel.set_duty(max);

asm::bkpt();

// dim
pwm.set_duty(max / 4);
// Use the PwmChannel object to set C3 to be dim
pwm_channel.set_duty(max / 4);

asm::bkpt();

// zero
pwm.set_duty(0);
// Use the PwmChannel object to set C3 to be zero
pwm_channel.set_duty(0);

asm::bkpt();

Expand Down
21 changes: 12 additions & 9 deletions examples/pwm_custom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,28 +33,31 @@ fn main() -> ! {
let p0 = pb4.into_alternate_push_pull(&mut gpiob.crl);
let p1 = gpiob.pb5.into_alternate_push_pull(&mut gpiob.crl);

let mut pwm = Timer::tim3(p.TIM3, &clocks, &mut rcc.apb1)
let pwm = Timer::tim3(p.TIM3, &clocks, &mut rcc.apb1)
.pwm((p0, p1), &mut afio.mapr, 1.khz());

let max = pwm.0.get_max_duty();
let max = pwm.get_max_duty();

pwm.0.enable();
pwm.1.enable();
let mut pwm_channels = pwm.split();

// Enable the individual channels
pwm_channels.0.enable();
pwm_channels.1.enable();

// full
pwm.0.set_duty(max);
pwm.1.set_duty(max);
pwm_channels.0.set_duty(max);
pwm_channels.1.set_duty(max);

asm::bkpt();

// dim
pwm.1.set_duty(max / 4);
pwm_channels.1.set_duty(max / 4);

asm::bkpt();

// zero
pwm.0.set_duty(0);
pwm.1.set_duty(0);
pwm_channels.0.set_duty(0);
pwm_channels.1.set_duty(0);

asm::bkpt();

Expand Down
152 changes: 140 additions & 12 deletions src/pwm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
*/

use core::marker::PhantomData;
use core::marker::{Copy};
justacec marked this conversation as resolved.
Show resolved Hide resolved
use core::mem;

use crate::hal;
Expand All @@ -68,6 +69,7 @@ use crate::afio::MAPR;
use crate::bb;
use crate::gpio::{self, Alternate, PushPull};
use crate::time::Hertz;
use crate::time::U32Ext;
use crate::timer::Timer;

pub trait Pins<REMAP, P> {
Expand All @@ -76,6 +78,26 @@ pub trait Pins<REMAP, P> {
const C3: bool = false;
const C4: bool = false;
type Channels;

fn check_used(c: Channel) -> Channel {
if (c == Channel::C1 && Self::C1)
|| (c == Channel::C2 && Self::C2)
|| (c == Channel::C3 && Self::C3)
|| (c == Channel::C4 && Self::C4)
{
c
} else {
panic!("Unused channel")
Copy link
Member

Choose a reason for hiding this comment

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

Still not a fan of this panic, and I'm not sure if check_used is even nessecary anymore

Copy link
Contributor Author

@justacec justacec Jan 29, 2020

Choose a reason for hiding this comment

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

@TheZoq2 It took me a bit to understand as this was a @burbull construction, but the function seems to make sense to me. Without the function there is no way for the per-channel functions to know if that channel is, in-fact, being used. As an example, lets say that I configure the Timer to use only Channel 3 passing a single pin. But then I call something like:

mypwn.enable(Channel::C1);

Without the check_used method, the function would go ahead and set the bit. Although, I am not sure that are any real negative consequences of this.

This was the real reason behind me using a macro in my original incarnation of this functionality. The macro expanded to only include channels that were active and would default to doing nothing through a default match. In this incantation, this has been shifted to be run-time vice compile-time.

Now, concerning the panic. Although I had originally default to no action, I actually see no issue with this panic as it will aggressively inform the user that he/she has something amis. I kind of feel that this is a better approach than doing nothing. In the do nothing variant, I could mis-type something and work for hours trying to figure out why my channel duty is not acting correctly; only to find out that I had put the wrong channel in there. If there was a panic, the compiler would yell at me (I think). If I am mis-understanding that it would occur at the compiler phase and it would be runtime, I would have a potential backtrace which would point to the exact place that needed to be checked.

@burrbull I am cc'ing you here to see if you have any specific comments on this above what I have put here.

Copy link
Member

@TheZoq2 TheZoq2 Jan 29, 2020

Choose a reason for hiding this comment

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

Now, concerning the panic. Although I had originally default to no action, I actually see no issue with this panic as it will aggressively inform the user that he/she has something amis.

Agreed, panic is better than doing nothing

If there was a panic, the compiler would yell at me (I think). If I am mis-understanding that it would occur at the compiler phase and it would be runtime,

Panics happen at runtime, if possible I would like to move this to compile time but that is a difficult task.

If I am mis-understanding that it would occur at the compiler phase and it would be runtime, I would have a potential backtrace which would point to the exact place that needed to be checked.

Yes, but backtraces can be a bit of a pain to get a hold of in embedded contexts

But again, we can probably take care of this in the future in another PR, so unless burbull has additional comments, I think this is good to go

Copy link
Member

Choose a reason for hiding this comment

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

I don't know how to do this check compile time because channel can be not constant.

In fact we need possibility to write const fn in trait implementation and variant of assert which fails in compile when it is possible and fail in runtime in other cases.

}
}
}

#[derive(Clone, Copy, PartialEq)]
pub enum Channel {
C1,
C2,
C3,
C4,
}

use crate::timer::sealed::{Ch1, Ch2, Ch3, Ch4, Remap};
Expand All @@ -89,7 +111,7 @@ macro_rules! pins_impl {
$($PINX: $TRAIT<REMAP> + gpio::Mode<Alternate<PushPull>>,)+
{
$(const $ENCHX: bool = true;)+
type Channels = ($(Pwm<TIM, $ENCHX>),+);
type Channels = ($(PwmChannel<TIM, $ENCHX>),+);
}
)+
};
Expand All @@ -115,7 +137,7 @@ pins_impl!(

#[cfg(any(feature = "stm32f100", feature = "stm32f103", feature = "stm32f105",))]
impl Timer<TIM1> {
pub fn pwm<REMAP, P, PINS, T>(self, _pins: PINS, mapr: &mut MAPR, freq: T) -> PINS::Channels
pub fn pwm<REMAP, P, PINS, T>(self, _pins: PINS, mapr: &mut MAPR, freq: T) -> Pwm<TIM1, REMAP, P, PINS>
where
REMAP: Remap<Periph = TIM1>,
PINS: Pins<REMAP, P>,
Expand All @@ -133,7 +155,7 @@ impl Timer<TIM1> {
}

impl Timer<TIM2> {
pub fn pwm<REMAP, P, PINS, T>(self, _pins: PINS, mapr: &mut MAPR, freq: T) -> PINS::Channels
pub fn pwm<REMAP, P, PINS, T>(self, _pins: PINS, mapr: &mut MAPR, freq: T) -> Pwm<TIM2, REMAP, P, PINS>
where
REMAP: Remap<Periph = TIM2>,
PINS: Pins<REMAP, P>,
Expand All @@ -147,7 +169,7 @@ impl Timer<TIM2> {
}

impl Timer<TIM3> {
pub fn pwm<REMAP, P, PINS, T>(self, _pins: PINS, mapr: &mut MAPR, freq: T) -> PINS::Channels
pub fn pwm<REMAP, P, PINS, T>(self, _pins: PINS, mapr: &mut MAPR, freq: T) -> Pwm<TIM3, REMAP, P, PINS>
where
REMAP: Remap<Periph = TIM3>,
PINS: Pins<REMAP, P>,
Expand All @@ -162,7 +184,7 @@ impl Timer<TIM3> {

#[cfg(feature = "medium")]
impl Timer<TIM4> {
pub fn pwm<REMAP, P, PINS, T>(self, _pins: PINS, mapr: &mut MAPR, freq: T) -> PINS::Channels
pub fn pwm<REMAP, P, PINS, T>(self, _pins: PINS, mapr: &mut MAPR, freq: T) -> Pwm<TIM4, REMAP, P, PINS>
where
REMAP: Remap<Periph = TIM4>,
PINS: Pins<REMAP, P>,
Expand All @@ -175,7 +197,26 @@ impl Timer<TIM4> {
}
}

pub struct Pwm<TIM, CHANNEL> {
pub struct Pwm<TIM, REMAP, P, PINS>
where
REMAP: Remap<Periph = TIM>,
PINS: Pins<REMAP, P>
{
clk: Hertz,
_pins: PhantomData<(TIM, REMAP, P, PINS)>,
}

impl<TIM, REMAP, P, PINS> Pwm<TIM, REMAP, P, PINS>
where
REMAP: Remap<Periph = TIM>,
PINS: Pins<REMAP, P>
{
pub fn split(self) -> PINS::Channels {
unsafe { mem::MaybeUninit::uninit().assume_init() }
justacec marked this conversation as resolved.
Show resolved Hide resolved
}
}

pub struct PwmChannel<TIM, CHANNEL> {
_channel: PhantomData<CHANNEL>,
_tim: PhantomData<TIM>,
}
Expand All @@ -193,7 +234,7 @@ macro_rules! hal {
_pins: PINS,
freq: Hertz,
clk: Hertz,
) -> PINS::Channels
) -> Pwm<$TIMX, REMAP, P, PINS>
where
REMAP: Remap<Periph = $TIMX>,
PINS: Pins<REMAP, P>,
Expand Down Expand Up @@ -240,10 +281,97 @@ macro_rules! hal {
.set_bit()
);

unsafe { mem::MaybeUninit::uninit().assume_init() }
Pwm {
clk: clk,
_pins: PhantomData
}
}

/*
The following implemention of the embedded_hal::Pwm uses Hertz as a time type. This was choosen
because of the timescales of operations being on the order of nanoseconds and not being able to
efficently represent a float on the hardware. It might be possible to change the time type to
a different time based using such as the nanosecond. The issue with doing so is that the max
delay would then be at just a little over 2 seconds because of the 32 bit depth of the number.
Using milliseconds is also an option, however, using this as a base unit means that only there
could be resolution issues when trying to get a specific value, because of the integer nature.

To find a middle ground, the Hertz type is used as a base here and the Into trait has been
defined for several base time units. This will allow for calling the set_period method with
something that is natural to both the MCU and the end user.
*/
impl<REMAP, P, PINS> hal::Pwm for Pwm<$TIMX, REMAP, P, PINS> where
REMAP: Remap<Periph = $TIMX>,
PINS: Pins<REMAP, P>,
{
type Channel = Channel;
type Duty = u16;
type Time = Hertz;

fn enable(&mut self, channel: Self::Channel) {
match PINS::check_used(channel) {
Channel::C1 => unsafe { bb::set(&(*$TIMX::ptr()).ccer, 0) },
Channel::C2 => unsafe { bb::set(&(*$TIMX::ptr()).ccer, 4) },
Channel::C3 => unsafe { bb::set(&(*$TIMX::ptr()).ccer, 8) },
Channel::C4 => unsafe { bb::set(&(*$TIMX::ptr()).ccer, 12) }
}
}

fn disable(&mut self, channel: Self::Channel) {
match PINS::check_used(channel) {
Channel::C1 => unsafe { bb::clear(&(*$TIMX::ptr()).ccer, 0) },
Channel::C2 => unsafe { bb::clear(&(*$TIMX::ptr()).ccer, 4) },
Channel::C3 => unsafe { bb::clear(&(*$TIMX::ptr()).ccer, 8) },
Channel::C4 => unsafe { bb::clear(&(*$TIMX::ptr()).ccer, 12) },
}
}

fn get_duty(&self, channel: Self::Channel) -> Self::Duty {
match PINS::check_used(channel) {
Channel::C1 => unsafe { (*$TIMX::ptr()).ccr1.read().ccr().bits() },
Channel::C2 => unsafe { (*$TIMX::ptr()).ccr2.read().ccr().bits() },
Channel::C3 => unsafe { (*$TIMX::ptr()).ccr3.read().ccr().bits() },
Channel::C4 => unsafe { (*$TIMX::ptr()).ccr4.read().ccr().bits() },
}
}

fn set_duty(&mut self, channel: Self::Channel, duty: Self::Duty) {
match PINS::check_used(channel) {
Channel::C1 => unsafe { (*$TIMX::ptr()).ccr1.write(|w| w.ccr().bits(duty)) },
Channel::C2 => unsafe { (*$TIMX::ptr()).ccr2.write(|w| w.ccr().bits(duty)) },
Channel::C3 => unsafe { (*$TIMX::ptr()).ccr3.write(|w| w.ccr().bits(duty)) },
Channel::C4 => unsafe { (*$TIMX::ptr()).ccr4.write(|w| w.ccr().bits(duty)) },
}
}

fn get_max_duty(&self) -> Self::Duty {
unsafe { (*$TIMX::ptr()).arr.read().arr().bits() }
}

fn get_period(&self) -> Self::Time {
let clk = self.clk;
let psc: u16 = unsafe{(*$TIMX::ptr()).psc.read().psc().bits()};
let arr: u16 = unsafe{(*$TIMX::ptr()).psc.read().psc().bits()};

// Length in ms of an internal clock pulse
(clk.0 / u32(psc * arr)).hz()
}

fn set_period<T>(&mut self, period: T) where
T: Into<Self::Time> {
let clk = self.clk;

let ticks = clk.0 / period.into().0;
let psc = u16(ticks / (1 << 16)).unwrap();
let arr = u16(ticks / u32(psc + 1)).unwrap();
unsafe {
(*$TIMX::ptr()).psc.write(|w| w.psc().bits(psc));
(*$TIMX::ptr()).arr.write(|w| w.arr().bits(arr));
}
}
}

impl hal::PwmPin for Pwm<$TIMX, C1> {
impl hal::PwmPin for PwmChannel<$TIMX, C1> {
type Duty = u16;

fn disable(&mut self) {
Expand All @@ -267,7 +395,7 @@ macro_rules! hal {
}
}

impl hal::PwmPin for Pwm<$TIMX, C2> {
impl hal::PwmPin for PwmChannel<$TIMX, C2> {
type Duty = u16;

fn disable(&mut self) {
Expand All @@ -291,7 +419,7 @@ macro_rules! hal {
}
}

impl hal::PwmPin for Pwm<$TIMX, C3> {
impl hal::PwmPin for PwmChannel<$TIMX, C3> {
type Duty = u16;

fn disable(&mut self) {
Expand All @@ -315,7 +443,7 @@ macro_rules! hal {
}
}

impl hal::PwmPin for Pwm<$TIMX, C4> {
impl hal::PwmPin for PwmChannel<$TIMX, C4> {
type Duty = u16;

fn disable(&mut self) {
Expand Down
Loading