Feature/embedded time

[wbuck]

Replaces the custom time module with embedded-time.

This change addresses both #191 and #185.

This change will allow folks to create custom device drivers without depending on a specific device crate for their custom duration/rate types.

[David-OConnor]

A step further is getting rid of .hz internally.

[wbuck]

@David-OConnor I was thinking the exact same thing.
I was wondering if using the Generic<u32> rate would be the way to go.

In that case interfaces would have to be changed (and not only the arguments).
The return types would need to be changed to return a Result in the event someone passed in a value that could not be safety converted into Hertz.

Unless panicking is acceptable in that case.

[strom-und-spiele]

I like the Result idea; it'd be nice if we could get rid of the (rather ugly) lines like Hertz::try_from(3u32.MHz()).unwrap() and still keep a nice way to catch conversion errors.

[wbuck]

@strom-und-spiele agreed, I didn't like Hertz::try_from(3u32.MHz()).unwrap() it felt ugly and non-intuitive.

Instead, something like this maybe?

pub fn sysclk( mut self, freq: Generic<u32> ) -> Result<Self, ConversionError> {
    self.sysclk = Some( Hertz::<u32>::try_from( f )?.0 );
    Ok( self )
}

This looks a little bit better.

let clocks = rcc
    .cfgr
    .use_hse( 8u32.MHz( ).into( ) )?
    .sysclk( 48u32.MHz( ).into( ) )?
    .pclk1( 24u32.MHz( ).into( ) )?
    .freeze( &mut flash.acr );

I can also use the Rate and TryInTo traits to constrain the parameter.
This provides a nicer interface for the user I think.

fn sysclk<R>( mut self, freq: R ) -> Result<Self, <R as TryInto<Hertz<u32>>>::Error> 
where
    R: Rate + TryInto<Hertz<u32>>
{
    let freq: Hertz = freq.try_into( )?;
    self.sysclk = Some( freq.0 );
    Ok( self )
}

let clocks = rcc
    .cfgr
    .use_hse( 8u32.MHz( ) )?
    .sysclk( 48u32.MHz( ) )?
    .pclk1( 24u32.MHz( ) )?
    .freeze( &mut flash.acr );

[strom-und-spiele]

Nice. Thanks for thinking this over and proposing the Rate and TryInto traits. Looks pretty ✨
I'll try to give the rest of your PR a read but I guess @Sh3Rm4n should also have a look, as he brought the idea forward (as you mentioned above).

[strom-und-spiele]

I'm (positively) surprised how little change is needed to adopt embedded-time. IMHO this speaks a lot for adopting it.
My change requests are minor nits and the conversion feature you proposed in #192 (comment).
I personally prefer having a lot of TODO boxes to check (and get a dopamine dose every time I do so) over missing on a change, so if I added redundant comments, the intention was to be helpful not to annoy. :)

[wbuck]

So I have started changing functions taking a frequency to what I proposed above.

fn func_taking_rate<R>( mut self, freq: R ) -> Result<Self, <R as TryInto<Hertz<u32>>>::Error> 
where
    R: Rate + TryInto<Hertz<u32>>

I'm lazy so I personally prefer calling this:

some_func( 8u32.MHz( ) ).unwrap( );

Over this:

some_func( Hertz::try_from( 8u32.MHz( ).unwrap( ) )

Or this:

some_func( 8u32.MHz( ).into( ) ).unwrap( );

[David-OConnor]

Is this finalized? Do you plan to make additional changes?

[wbuck]

@David-OConnor & @strom-und-spiele There is one last thing I was toying with and it has to do with the CountDown/Periodic timer. I wanted to see if I could find a more intuitive way to set the count value of the timer by using a Duration (milliseconds, microseconds, etc) instead of a Rate (hertz).

I'm really unsure about this though and maybe i'll just abandon that idea all together (in that case this PR is finalized).
I wrote a quick example of what I was thinking but I wanted some feedback on it.

This example was written in another toy application so I didn't have access to the crate private methods that are defined in the stm32f3xx-hal so I just hacked it for testing purposes.

This is only an example.

Timer / CountDown implementation

pub struct Timer<D, TIM> 
where
     D: Duration + FixedPoint<T = u32>,
{
    clocks: Clocks,
    tim: TIM,
    _pantom: PhantomData<D>,
}

impl<D> CountDown for Timer<D, TIM1>  
where
    D: Duration + FixedPoint<T = u32>,
{
     type Time = D;

    fn start<T>( &mut self, count: T )
    where
        T: Into<Self::Time> 
    {
        self.stop( );

        let frequency: Hertz = ( count.into( ) as D ).to_rate( ).unwrap( );
        let frequency = frequency.0;
        
        // Same as current impl.
    }

    fn wait( &mut self ) -> nb::Result<( ), void::Void> {
       // Same as current impl.
    }
}

// Timer impl same as current.

Instantiating the timer

let mut timer = Timer::tim1( p.TIM1, 10u32.microseconds( ), clocks, &mut rcc.apb2 );

Using the timer

timer.start( 50u32.microseconds( ) );

Maybe I've missed the point regarding the CountDown timer but when I'm setting a timer I usually like using a duration instead of a rate.

[David-OConnor]

Agree on duration instead of rate. The Hz abstraction doesn't even let you set a duration > 1s.

For an example of how to set duration, ref my PR here: https://github.com/stm32-rs/stm32f3xx-hal/pull/166/files

It doesn't use CountDown, but I think that would be nice. see set_period for an example on how to setup a time. Check the comments for caveats. I'm not clear on how the existing method works, and it doesn't produce accurate results for me.

                /// Set the timer period, in seconds. Overrides the period or frequency set
                /// in the constructor.
                /// This allows you to set periods greater than 1hz.
                pub fn set_period(&mut self, period: f32, clocks: &clocks::Clocks) -> Result<(), ValueError> {
                    // PSC and ARR range: 0 to 65535
                    // (PSC+1)*(ARR+1) = TIMclk/Updatefrequency = TIMclk * period
                    // APB1 (pclk1) is used by Tim2, 3, 4, 6, 7.
                    // APB2 (pclk2) is used by Tim8, 15-20 etc.
                    // todo: It appears there's a (fixed?) 2x multiplier on APB1
                    // timers; it's twice `pclk1`. See clocks diagram in RM, or `Clock Configuration`
                    // tool in STM32CubeIDE.
                    let tim_clk = clocks.calc_speeds().pclk1 * 1_000_000. * 2.;

                    // We need to factor the right-hand-side of the above equation (`rhs` variable)
                    // into integers. There are likely clever algorithms available to do this.
                    // Some examples: https://cp-algorithms.com/algebra/factorization.html
                    // We've chosen something quick to write, and with sloppy precision;
                    // should be good enough for most cases.

                    // - If you work with pure floats, there are an infinite number of solutions: Ie for any value of PSC, you can find an ARR to solve the equation.
                    // - The actual values are integers that must be between 0 and 65_536
                    // - Different combinations will result in different amounts of rounding errors. Ideally, we pick the one with the lowest rounding error.
                    // - The aboveapproach sets PSC and ARR always equal to each other.
                    // This results in concise code, is computationally easy, and doesn't limit
                    // the maximum period. There will usually be solutions that have a smaller rounding error.

                    let max_val = 65_535;
                    let rhs = tim_clk * period;

                    // todo: Round instead of cast?
                    let arr = (rhs.sqrt() - 1.) as u16;
                    let psc = arr;

                    if arr > max_val || psc > max_val {
                        return Err(ValueError {})
                    }


                    self.tim.arr.write(|w| unsafe { w.bits(u32::from(arr)) });
                    self.tim.psc.write(|w| unsafe { w.bits(u32::from(psc)) });

                    Ok(())
                }

                /// Reset the countdown; set the counter to 0.
                pub fn reset_countdown(&mut self) {
                    self.tim.cnt.write(|w| unsafe { w.bits(0) });
                }
            }

You would need to do 3 things:

• Change the period argument from seconds to Duration.
• Reformat into the CountDown EH trait.
• Pull the timer clock from the rcc::Clocks instead of the forked one I'm using.

[wbuck]

@David-OConnor Yeah I was looking at the issue you brought up regarding timer durations greater than 1 second.

Another idea I've been playing with (not for this PR) but isn't even close to prime time is creating a TimerBuilder.
The pattern would allow for both the creation of a simple timer (hiding all complex stuff) and a complicated timer.

The plan is to allow the user to configure the CountDown timer however they want.

Example of complex setup:

let mut timer = TimerBuilder::new( TIM1, &mut rcc )
    .repeating( )
    .prescaler( | p | { } )
    .duration( | d | { } )
     // More setup.
    .build( );

Example of a simple setup:

let mut timer = TimerBuilder::new( TIM1, &mut rcc )
      .duration( | d | { } )
      .build( );

This is still a pie in the sky idea, but I have started to try and map out the potential different branches one might take when building a CountDown timer.

[David-OConnor]

I like that. I like how the embedded-hal CountDown etc traits provide a basic abstraction that will have identical syntax anwhere: Across HALs, between timer, RTC and systick etc. It's deliberately limited in how you can configure it - that's why I agree with your idea of separating simple and complex.

Keep in mind the reason for simple though - having the standard CountDown API.

[wbuck]

@David-OConnor When you say "I'm not clear on how the existing method works, but it doesn't produce accurate results for me" are you talking about how the arr and psc registers are configured in the current start( hertz ) method?

I agree, I also like the simple abstractions; the timer returned from the builder would still implement CountDown.
To me timers aren't one-size-fits-all and I think there needs to be a way for people to construct timers to meet their needs (without having to use the PAC).

The TimerBuilder in my example would only allow the creation of timers that fit into the CountDown timer box.
Using this builder you would not be able to configure a timer for PWM for example.

[David-OConnor]

Yes - this part:

    let frequency = timeout.into().0;
                    let timer_clock = $TIMX::get_clk(&self.clocks);
                    let ticks = timer_clock.0 * if self.clocks.ppre1() == 1 { 1 } else { 2 }
                        / frequency;
                    let psc = crate::unwrap!(u16::try_from((ticks - 1) / (1 << 16)).ok());

                    // NOTE(write): uses all bits in this register.
                    self.tim.psc.write(|w| w.psc().bits(psc));

                    let arr = crate::unwrap!(u16::try_from(ticks / u32::from(psc + 1)).ok());

                    // TODO (sh3rm4n)
                    // self.tim.arr.write(|w| { w.arr().bits(arr) });
                    self.tim.arr.write(|w| unsafe { w.bits(u32::from(arr)) });

The formula that relates timer period(freq), ARR, PSC, and timer clocks is in a comment in my code above. I don't understand how this relates to it.

[wbuck]

@David-OConnor

I think there is a small issue in the set_period method.
PCLK1 is only doubled in the event that the APB1 prescaler (PPRE1) is set to anything above 3.

If (APB1 prescaler=1) x1 else x2

Also, switching the type for the start method from a Rate type (Hertz<u32>) to a Duration<u32> type isn't going to solve the issue you're having (periods > 1s).

I still convert the duration into a frequency Hertz<u32> within the start methods body.

let frequency: Hertz = ( timeout.into( ) as D ).to_rate( ).unwrap( );

Anything greater than 1 second will return 0 for the frequency.
So again, the same issue you're currently having would persist.

Really the only solution I can think of for periods > 1s would be to do what you have already done with the set_period method.

[David-OConnor]

Good catch on PCLK1. I can confirm from the Cube IDE that an APB1 prescaler above 1 causes the timer clock multiplier to be 2 instead of 1. Also, my code doesn't differentiate between timers using apb1 or apb2, although there's a comment indicating it. (These both work correctly in the original version)

I think the sqrt approach I made works for long durations, but may not be precise enough for short ones, where you'd need to have one of the values (PSC, ARR) smaller than the other.

I can't comment on the >1s issue, since I haven't looked at the Duration type you're using.

[David-OConnor]

edit: I understand your >1 s point. It's not about the Duraction struct; it's about the PSC and ARR calculation.

edit: I think it makes sense to split the PSC and ARR calculation into a standalone, pure function, then call it from CoutndownDown and set_period:

/// Calculate values required to set the timer period: `PSC` and `ARR`. This can be
/// used for initial timer setup, or changing the value later.
fn calc_period_vals(period: f32, clocks: &clocks::Clocks) -> Result<(u16, u16), ValueError> {
    // PSC and ARR range: 0 to 65535
    // (PSC+1)*(ARR+1) = TIMclk/Updatefrequency = TIMclk * period
    // APB1 (pclk1) is used by Tim2, 3, 4, 6, 7.
    // APB2 (pclk2) is used by Tim8, 15-20 etc.
    // todo: It appears there's a (fixed?) 2x multiplier on APB1
    // timers; it's twice `pclk1`. See clocks diagram in RM, or `Clock Configuration`
    // tool in STM32CubeIDE.
    let mult = if let clocks::ApbPrescaler::Div1 = clocks.apb1_prescaler {
        1.
    } else {
        2.
    }
    let tim_clk = clocks.calc_speeds().pclk1 * 1_000_000. * mult;

    // We need to factor the right-hand-side of the above equation (`rhs` variable)
    // into integers. There are likely clever algorithms available to do this.
    // Some examples: https://cp-algorithms.com/algebra/factorization.html
    // We've chosen something quick to write, and with sloppy precision;
    // should be good enough for most cases.

    // - If you work with pure floats, there are an infinite number of solutions: Ie for any value of PSC, you can find an ARR to solve the equation.
    // - The actual values are integers that must be between 0 and 65_536
    // - Different combinations will result in different amounts of rounding errors. Ideally, we pick the one with the lowest rounding error.
    // - The aboveapproach sets PSC and ARR always equal to each other.
    // This results in concise code, is computationally easy, and doesn't limit
    // the maximum period. There will usually be solutions that have a smaller rounding error.

    let max_val = 65_535;
    let rhs = tim_clk * period;

    let arr = (rhs.sqrt() - 1.).round() as u16;
    let psc = arr;

    if arr > max_val || psc > max_val {
        return Err(ValueError {})
    }

    Ok((psc, arr))
}

/// Set the timer period, in seconds. Overrides the period or frequency set
/// in the constructor.
pub fn set_period(&mut self, period: f32, clocks: &clocks::Clocks) -> Result<(), ValueError> {
   let (psc, arr) = calc_period_vals(period, clocks)?;

    self.tim.arr.write(|w| unsafe { w.bits(arr.into()) });
    self.tim.psc.write(|w| unsafe { w.bits(psc.into()) });

    Ok(())
}

[David-OConnor]

I think the timer clock computation makes more sense in the rcc or clocks module, so you can just do this:

let tim_clk = clocks.calc_speeds().timclock1 * 1_000_000.;

[David-OConnor]

I can confirm that the sqrt approach I took is imprecise at high frequencies, eg kHz range. The original algorithm fairs better there.

[wbuck]

@David-OConnor So I think I'm going to keep the CountDown timer the way it currently is (keep this PR fairly simple).

I would prefer to open a different PR for changing the timer.

I like your approach of allowing folks to adjust the period but what about the start method?.
Currently if someone calls set_period and then calls start the previously calculated ARR and PSC values would be blown away.

Someone would have to first call the ctor (which in turn calls start enabling the timer) then would have to call set_period.

But I guess given how general the CountDown trait is, there really is no other simple and clean solution.

[David-OConnor]

I think this goes back to your idea of simple and complex: Unfortunately, I don't think there's a way around that limitation with the CountDown impl. One way would be raw pointers. Another is to PR embedded-hal to add a change_period method, but they like to keep those traits as generic as possible.

I think the answer, as you said, is to leave CountDown in as a generic option, and add additional approaches to the HAL.

[Sh3Rm4n]

I have not gone through all comments and changes in detail, just wanted to ask if this PR is still kinda WIP or if it is worth to fully review it to get it into the master branch :)

[wbuck]

@Sh3Rm4n All work is complete and it’s ready for a full review.

[Sh3Rm4n]

Wow, this is pretty cleanly applicable without too much change.

I'm only unhappy about the Result change.

Regarding #192 (comment), I know that the current clocks generation results into ugly code, but this is on my todo list for a long time.

let clocks = rcc
    .cfgr
    .use_hse( 8u32.MHz( ).into( ) )?
    .sysclk( 48u32.MHz( ).into( ) )?
    .pclk1( 24u32.MHz( ).into( ) )?
    .freeze( &mut flash.acr );

which is your 2nd proposal, which does look pretty clean to me. It is not as clever as the TryInto constrain, but it also does not need to unnecessarily introduce a change in the function signature (aka introducing Result as the return type).

Thanks for your contribution! I think my suggestion is an easy fix. Otherwise, this looks pretty good ✨

[Sh3Rm4n]

Mostly ergonomic suggestions and pointing out some missed Result<Self,...> to Self reversions.

This looks promising. 👍

EDIT: Ah and before I forget: I'm always happy about a changelog entry. Note, that this is a breaking change.

[Sh3Rm4n]

Use missed reverting this part :)

[wbuck]

Oh, not sure how I missed these..

[wbuck]

Do you want to do a minor version bump since not only is this a breaking change to the public API but also changes within the implementation?
So v0.6.1 to 0.7.0?

[Sh3Rm4n]

Another note: If you are not comfortable applying my suggestion about the src/rcc.rs changes, I'm fine, if you leave it as is. I know my ways around this file quite well, so I can implement it afterwards. This shouldn't be a blocker for this PR to get merged.

[Sh3Rm4n]

Some minor nits, but this looks really good now overall! :)

[Sh3Rm4n]

Nice. Looks good to me! Thank you for your work and patience.