Improve time keeping with Timer and RTC

[thalesfragoso]

Closes #89 and closes #41.

Right now it would be ok to leave the URS bit set, since the HAL has ownership over the registers and we don't implement anything which requires URS bit cleared. However, I decided to clear the URS bit after the update event to prevent future side effects, but I don't have strong feelings about that...

[thalesfragoso]

I just realized that the counter value isn't very useful without the prescaler and auto-reload value. What should we do about that ?

[TheZoq2]

Thanks for the PR, at a quick glance, it looks good to me.

I just realized that the counter value isn't very useful without the prescaler and auto-reload value. What should we do about that ?

That's a good question... The most straight forward approach is probably to make it return a suitable time unit, i.e. count_microseconds().

Another option might also be to return a struct which contains the relevant values which has method for conversion into real units.

struct TimerCount { arr, prescaler }

impl TimerCount {
    pub fn seconds() {..}
    pub fn milliseconds() {..}
    ...
}

cc: @mjepronk. Since you opened #89, you may be interested in this

[mjepronk]

Yes, It's interesting, thanks for the heads-up.

I thought however that cnt register would be incremented by the Hertz value that you specified when instantiating the Timer. This would make it obvious for the programmer to know that it would be in ms if the timer was created with 1_000.hz(). I have to admit that I haven't tried this...

[TheZoq2]

Yea, you have a point about the programmer knowing it, though I would prefer a static check because it avoids accidental pitfalls

[mjepronk]

Yes, of course. Personally I prefer the methods on the timer, instead of the struct. I find it a little bit harder to discover from the API documentation on how to use it.

[TheZoq2]

Yea, that's a very good point

[thalesfragoso]

That's a good question... The most straight forward approach is probably to make it return a suitable time unit, i.e. count_microseconds().

How about this:

/// Returns the number of microseconds since the last update event.
/// *NOTE:* This method is not a very good candidate to keep track of time, because
/// it is very easy to lose an update event.
pub fn micros_since(&self) -> u32 {
    let timer_clock = $TIMX::get_clk(&self.clocks).0;
    let psc = u32(self.tim.psc.read().psc().bits());
    
    // freq_divider is always bigger than 0, since (psc + 1) is always less than
    // timer_clock
    let freq_divider = u64(timer_clock / (psc + 1));
    let cnt = u64(self.tim.cnt.read().cnt().bits());
    
    // It is safe to make this cast, because the maximum timer period in this HAL is
    // 1s (1Hz), then 1 second < (2^32 - 1) microseconds
    u32(1_000_000 * cnt / freq_divider).unwrap()
}

I tried to get rid of the u64 arithmetic but couldn't find a good solution, maybe someone can help.
I'm not sure about the real use of this, it's just too error prone IMO, but with the right documentation I don't see a problem. I also couldn't think of a good name for the method, I just hope that no one mistakes this with some arduino's micro function.
I would like that someone takes a look in my assumptions to this code, I did some quick math, but it would be nice to have some confirmation.

Yes, It's interesting, thanks for the heads-up.

I thought however that cnt register would be incremented by the Hertz value that you specified when instantiating the Timer. This would make it obvious for the programmer to know that it would be in ms if the timer was created with 1_000.hz(). I have to admit that I haven't tried this...

The counter goes from zero to the auto-reload value every timer period, and after that it restarts from zero, so you wouldn't be able to get more than 1ms (with 1kHz timer) information that way. What people usually do is increment a counter variable inside the interrupt routine. If you wish to create a crate with time sensitive stuff, I recommend that you take a look at https://github.com/jonas-schievink/rubble, they found a good solution for that without the need of a RTOS.

[TheZoq2]

What people usually do is increment a counter variable inside the interrupt routine.

My imagined use case for this is for measuring time that is much shorter than the timer period, but yes, in general the counter variable is a more general solution.

I'm not sure about the real use of this, it's just too error prone IMO, but with the right documentation I don't see a problem

@mjepronk you originally suggested this, do you have insights here. You're right about it being somewhat error prone, but only if overflow occurs, right? If you use it to check time to a timeout or something like that, it should be fine I think.

// It is safe to make this cast, because the maximum timer period in this HAL is
// 1s (1Hz), then 1 second < (2^32 - 1) microseconds

I don't like this, at some point I would like to extend the range of allowable time periods beyond one hertz.

[mjepronk]

Yes, my use-case was to implement a timeout without the need for an interrupt. (I wanted to use it to implement the timeout for your ESP01 crate). Like you say, it would only be useful if we could use it beyond 1 Hz (up to several minutes), and with millisecond precision (1 kHz).

[thalesfragoso]

I don't like this, at some point I would like to extend the range of allowable time periods beyond one hertz.

Well, you're still safe up to 4294 seconds, which already sounds a bit absurd for a timer period.

Extending the timers to subHz range would need some big refactoring, since all the calculations are made in Hz. The larger period for the SYST would be ~4.2 seconds, so we would have some API differences if we would allow arbitrary large timer period for the vendor's timers. It's somehow doable, just not sure how useful that would really be, I would go for the RTC for any larger period.

[thalesfragoso]

Added the method proposed, still can't think of a good name for it, I'm open to suggestions. Also added the methods to SYST timer.

Yes, It's interesting, thanks for the heads-up.

I thought however that cnt register would be incremented by the Hertz value that you specified when instantiating the Timer. This would make it obvious for the programmer to know that it would be in ms if the timer was created with 1_000.hz(). I have to admit that I haven't tried this...

I was thinking about it and I guess you could do that with RTC, since it has a counter that behaves similar to that. People usually do that to count seconds, but you can even get a 16kHz clock with it.

Right now the HAL only uses it with 1Hz, maybe I can add a method to change the frequency this weekend and add to this PR.

[TeXitoi]

By memory, there is access to thé internal rtc counter value, allowing to count sub seconds using rtc.

[thalesfragoso]

Added a method to change the RTC counter frequency, which gives a better way to count time without interrupts.
CC @mjepronk
CC @TheZoq2

But now the naming of set_seconds and seconds methods is a bit weird, but it makes sense somehow, since even the register for interrupts is called Second Interrupt even though it may not be counting seconds, but other unit of time.

[mjepronk]

This looks fine to me! Thank you.

However, note that I'm not that familiar with either the hardware or this crate...

[TheZoq2]

But now the naming of set_seconds and seconds methods is a bit weird, but it makes sense somehow, since even the register for interrupts is called Second Interrupt even though it may not be counting seconds, but other unit of time.

That does seem weird. Perhaps we should change it to set_time and current_time or something. The interrupt being called seconds is also kind of strange,, but I guess there isn't much we can do about that

[thalesfragoso]

That does seem weird. Perhaps we should change it to set_time and current_time or something. The interrupt being called seconds is also kind of strange,, but I guess there isn't much we can do about that

I thought about that, but I wasn't completely sure if it was worth the breaking. Also, what do you think about the micros_since name ?

[TheZoq2]

I thought about that, but I wasn't completely sure if it was worth the breaking

Since the functionality is changing, I almost think we should break things here.

Also, what do you think about the micros_since name?

Sounds good to me, at least until we get a good crate for typed times :)

[TheZoq2]

This looks good to me now! @therealprof Do you have any concerns with this?

[therealprof]

I only have a few nits: There's a second Changed section added to the CHANGELOG.md and the changes itself are not tagged as breaking despite changing public API.

[thalesfragoso]

I only have a few nits: There's a second Changed section added to the CHANGELOG.md and the changes itself are not tagged as breaking despite changing public API.

Oopsie, it must have slipped during rebasing. Also, some of the changes are non-breaking.
Another thing that caught my attention during rebasing was that now we call unlisten on timer stop for TIMX but not for SYST. See

stm32f1xx-hal/src/timer.rs

Line 160 in 9c9c6e1

pub fn stop(mut self) -> Timer<$TIMX> {

and

stm32f1xx-hal/src/timer.rs

Line 64 in 9c9c6e1

pub fn stop(&mut self) {

.

Unfortunately, I missed PR #95 and only saw it now.
To be honest I don't think that we should disable interrupts during stop, doesn't seem completely expected, but maybe I'm not getting the reasoning behind this.
CC @burrbull

[burrbull]

I've already answered you in matrix that you can delete string with interrupt disabling.

[thalesfragoso]

Yeah, sorry, riot didn't notify me for some reason...

[TheZoq2]

To be honest I don't think that we should disable interrupts during stop, doesn't seem completely expected, but maybe I'm not getting the reasoning behind this.

I agree, I would expect interrupts to stay enabled/disabled when calling start and stop

[thalesfragoso]

Fixed the Changelog and made some changes on the timer code to match the Timer<SYST> and Timer<TIMx> APIs.
CC
@burrbull
@therealprof
@TheZoq2

[burrbull]

LGTM

[TheZoq2]

Looks good apart from some documentation nitpicks

[TheZoq2]

Is this comment out of date, if I'm reading the function correctly, it does not generate an update event

[thalesfragoso]

Yeah, sorry, probably missed that

[TheZoq2]

To me, this comment is a bit hard to understand without knowlege of the RTC because it's not clear what counter is. Perhaps change it to Selects the frequency of the timer, though that doesn't feel quite right to me either

[TheZoq2]

This looks good now, thanks!