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

Timers with period > 1s #190

Closed
David-OConnor opened this issue Jan 2, 2021 · 7 comments
Closed

Timers with period > 1s #190

David-OConnor opened this issue Jan 2, 2021 · 7 comments
Labels
enhancement New feature or request

Comments

@David-OConnor
Copy link
Contributor

David-OConnor commented Jan 2, 2021

Hi. Is it possible to implement timers with period > 1s? It seems that the .hz() api only takes integers.

If not, are you open to a PR?

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Jan 3, 2021

I'm not sure, if it is really possible, but it should.

Some pointers / ideas:

  1. Hardware: Does the timer support frequencies lower than 1 Hz?
  2. The Into<Hertz> has to be adjusted, as this currently limits the maximal timer period to 1 s.
    • Traits and From<...> conversions can be found here. Maybe extending the U32Ext implementation is useful?
    • This is possible not enough, and this constraint has to be reworked to also support values in the < 1 Hz / > 1 s range
  3. (maybe) The prescaler and auto-reload value calculation has to be adjusted
    • Where PSC is the prescaler scaling down the peripheral clock which is connected to the timer block
    • And ARR is the auto-reload value, which holds the amount of timer ticks until an event / interrupt fires
    • The maximum value for ARR is the maximum possible length for a timer period in Hz, assuming PSC scales the peripheral clock down to 1 Hz

@Sh3Rm4n Sh3Rm4n added the enhancement New feature or request label Jan 3, 2021
@David-OConnor
Copy link
Contributor Author

David-OConnor commented Jan 3, 2021

  1. Yep!

  2. I think we'd need to bypass the Into<Hertz>, or fork it; I think it's the root cause. I'd be happy setting period or frequency with a f32 argument instead, but that's a breaking change. Maybe with a set_period(f32) or set_freq(f32) method you could run after? Would be non-breaking.

  3. That's the core of it; PSC and ARR, in conjunction with clock speed are the only thing that determines period. For a given period, you set a combination of PSC and ARR. There is more than one combo for each period; we could pick arbitrarily. I need to dive into the docs more, but IIIRC the period is (ARR+1) * (PSC+1) * the timer's clock speed.

Some naive approaches: Keep PSC and ARR equal. Calculate it with sqrt of period. (Has resolution issues). Fix one and set the other. (Has resolution and range issues). I think the existing code is pretty good about setting the right values, if they're faster than 1hz, and you leave it in edge mode.

Related: The latest version of my fork has a working interface to set RTC wakeup timer, where you pass period in seconds as f32. This works in practice in a similar way to a timer, with an easy API. But the reg write process is different, and doesn't help here.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Jan 3, 2021

I think we'd need to bypass the Into, or fork it; I think it's the root cause

Definitely. This has to be reworked. I wouldn't worry about a breaking change too much.
Maybe it would be worth to get feedback from the other stm hal impls as well, as they have to same issue / design IIRC.

I'd be happy setting period or frequency with a f32 argument instead

Seems like a valid approach. I'm not sure though, if this does cause any issues. I wonder why the u32 approach with MegaHertz, Hertz, ... New-Type pattern is used everywhere. 🤔

@IamfromSpace
Copy link
Contributor

Just to chime in, this is similar to the discussion around clocks for PWM.

Ultimately, the only way to get full control over these things is to basically give the same interface as the registers. They use the divider -1 approach because all states are valid and you can get ever valid combo. However, it’s just terribly unfriendly to use.

It seems like there just needs to be multiple ways to configure these things, via trait or otherwise. Possibly structs that can produce the register values given a clock frequency.

That way most users can get more conceptually useful inputs (like setting an approximate resulting period) or drill down to get exactly what they need.

@David-OConnor
Copy link
Contributor Author

David-OConnor commented Jan 3, 2021

I've added the method (as to be non-breaking) to my fork. Here's the code. The comments provide some details, including how this approach is quick+dirty: Ie there are ways to get better precision, perhaps at the expense of some computation.

  /// 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) {
    // 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 * 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.

    let rhs = tim_clk * period;

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

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

We set psc and arr equal to each other, as the sqrt of the equation. You could come up with better approaches that come closer to the period you want.

I've tested this with a few values; seems to work, at least for the clock config and periods I used.

Here's the code from the current impl, using hz.

  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)) });

@David-OConnor
Copy link
Contributor Author

David-OConnor commented Jan 4, 2021

Probably should return a Result that fails if arr or psc is greater than 65k. edit: Added to fork:

let max_val = 65_535;
if arr > max_val || psc > max_val {
    return Err(ValueError {})
}

A little more info on the subjective part:

  • 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 above approach 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.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Aug 16, 2021

Closed by #267

@Sh3Rm4n Sh3Rm4n closed this as completed Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants