Timers #12

droogmic · 2018-10-20T21:37:29Z

Based on #8, appears to work as required. It is however, very messy, I am just happy I got it working.
I am not totally up to speed with recent embedded rust developments and roadmap, nor am I very experienced with rust, so a skim read and advice if there is a better standard to follow would be appreciated.

Changes:

Refactored so all logic sits in timers.rs
Added TIMER1, TIMER2, RTC0 and RTC1
Examples to test this can be found in https://github.com/droogmic/microbit/tree/examples/tests

Consider / TODO:

Duplicate start method
Commented out features
Move things to a time.rs file
hal::timer::Periodic marker trait flag, doesn't apply as for the nrf51 this can be changed at runtime, how is this usually handled?
A robust testing strategy is probably needed; some tests are provided in microbit which are a good start for manual verification, but ideally these would be run as tests not examples; also briefly discussed in fix typo'd target name #10
Compare with other examples, e.g. stm32f* hal crates
What should be the API for shortcuts
How should the similar feature set between TIMER and RTC be combined to a unified API

TIMER1 and TIMER2 can now be used for this functionality instead of just TIMER0.

droogmic · 2018-10-21T09:02:45Z

src/timer.rs

+                {
+
+                    // Reset comparison interrupt
+                    self.reset_compare(0);


Remove duplication from start

droogmic · 2018-10-22T21:14:04Z

I think this is ready for an initial review, I have qualified the functionality of all 5 peripherals on my device.
I can rebase on request
I can run rustfmt on request

droogmic · 2018-10-23T21:21:50Z

src/timer.rs

+
+            impl Timer<$RTC> {
+                /// Construct RTC based timer with prescaler
+                /// *WARNING* The LFCLK needs to be activated first, e.g.


technically, this will breach the embedded-hal Countdown trait contract

droogmic · 2018-10-23T22:32:32Z

Fixes following discussion with @therealprof.

Still some open issues, but a lot is down to shortcomings in embedded-hal

therealprof · 2018-10-24T21:11:58Z

src/lib.rs

 #![feature(const_fn)]
+#![feature(try_from)]
+#![feature(duration_as_u128)]


We should try to get rid of all those features. This should eventually be Rust 2018 stable-able.

Ah, I had forgotten about that:

Remove all 3 2018-incompatible features

I used core::time::Duration to get it working, but it is expensive in an MCU to use u128s to manipulate time. It seems there are many ongoing discussions about how to handle real unite both in rust and rust-embedded, none nearing conclusion. So for now I will just make 3 types: tick (An nrf51 specific tick, 1/16th of a μs, a μs and a ms.

therealprof · 2018-10-24T21:22:58Z

src/delay.rs

+macro_rules! delay {
+    ( $($TIM:ty),+ ) => {
+        $(
+            impl DelayMs<u32> for Timer<$TIM> {


So you've renamed Delay to Timer? That'll break all some stuff, including your own examples in microbit.

I'll create an alias to Delay as a structural fix, as that what clients will expect. Behind the scenes though it makes no sense to have a distinction.

droogmic · 2018-10-25T16:39:25Z

src/timer.rs

+
+/// Error to respresent a stopped countdown
+#[derive(Debug)]
+pub struct CountdownStoppedError;


therealprof · 2018-10-27T20:26:45Z

src/timer.rs

+
+impl From<Millis> for Lfticks {
+    fn from(millis: Millis) -> Self {
+        Lfticks(u32(f64::from(millis.0) * 32.768).unwrap())


f64 is a very expensive type on this platform. Is this really necessary?

Oh my, that is an unacceptably bad bit of code..., it isn't even mathematically correct, apologies:

Lfticks(millis.0 * LFCLK_FREQ)

On second thought, it was mathematically correct, ( 32.768 =/= 32_768)
I need to find a table which lists how expensive various operations are...

Well, this is a Cortex-M0, so no hardware division. And no FPU, so floating point is all emulated.

droogmic · 2018-10-28T08:13:49Z

Don't merge this, I think there is a major flaw with this as is.
If DelayMs and Timer are implemented on the same struct, then they can currently both be used at the same time. Obviously this currently breaks everything...
e.g. start an asynchronous countdown, use a delay on the same struct, then waiting for the countdown (hint: the countdown will block forever as the timer is now stopped)
2 options:

Macro everything to have a full duplicate implementation of everything in Delay and Timer, with the exception of the embedded-hal traits, which will be only in one or the other
(Possibly, I need to think further if it is possible) take a performance penalty (might need to do more computations and store a u32 in the struct) but enable more functionality: use the second comparison registers and events on the nrf51's timers and rtcs to enable compliant functionality

Thoughts?

therealprof · 2018-10-28T12:24:14Z

@droogmic Just from reading the code I'm not sure how conflicting use would be possible. Is this due to Delay being a type alias for a Timer? If so this could be solved by using a type state, i.e. have some general timer initialisation code and then two different types implementing either HAL functionality.

droogmic · 2018-10-28T17:18:56Z

@droogmic Just from reading the code I'm not sure how conflicting use would be possible. Is this due to Delay being a type alias for a Timer? If so this could be solved by using a type state, i.e. have some general timer initialisation code and then two different types implementing either HAL functionality.

Ah yeah, that would be option 1, don't actually need macros.
That is the simplest and probably the best, as testing for interactions will be painful

droogmic · 2018-10-28T17:20:41Z

@therealprof And yes, because of the alias you can make a Delay item with both DelayMs and Timer traits implemented

@therealprof

After @therealprof review + Move time to a seperate file + Move generic timer/counter implementation to file + Create newtype Delay and Timer from TimCo

droogmic · 2018-11-03T16:35:31Z

@therealprof I am not sure if this is what you meant

using a type state

therealprof · 2018-11-03T18:02:02Z

@droogmic I don't have the time right now to look into your update but typestate programming is something like this: https://github.com/therealprof/stm32f042-hal/blob/master/src/gpio.rs

Basically what you had previously but with empty structs for the different possibilities (e.g. Countdown or Delay), functions to move the generic timer into those states and then only impl the traits on the timers which are in that specific state.

droogmic · 2018-11-03T18:41:15Z

@therealprof Thanks, I'll try that and get back to you.

Used to seperate Countdown and Delay trait implementation for same Timer struct

droogmic · 2018-11-04T23:15:15Z

@therealprof I have made an attempt, but I am not really happy with the result...

droogmic · 2018-11-11T15:56:25Z

@therealprof bump, a quick look to see if this is what you meant would be appreciated.

therealprof

It looks good structure-wise (except maybe for the excess blank lines) but I'm a bit lost on how to actually use this, i.e. how do I turn a RTCx or TIMERx peripheral into a timer I can actually call into_delay() or into_countdown() on? As a nice litmus test it would be great if you could also come up with a PR that changes the microbit crate to make use of the changed interface.

droogmic · 2018-11-22T19:59:17Z

@therealprof
See nrf-rs/microbit#11 for a demo.

Not sure type state is the way to go anymore, as the moment the struct specializes most of the methods should probably be removed. I think having 3 different structs would be simpler, where Delay and Countdown structs container a single Timer struct.

therealprof · 2018-11-28T10:21:37Z

@droogmic Actually I was hoping that instead of Timer<Generic, TIMERx> we would have something like Timer<TIMERx<Generic>>, potentially even without the additional Timer though I'm not sure whether that would work register-wise.

cgm616 and others added 4 commits July 15, 2018 20:04

Allow multiple timers in Delay and Timer

2554f1d

TIMER1 and TIMER2 can now be used for this functionality instead of just TIMER0.

Merge branch 'master' of https://github.com/cgm616/nrf51-hal into timers

86fd1ed

Configure delay and timer for TIMER{0-3}

1af2775

Major TIMER update, moving everything to timer.rs

d43f331

droogmic commented Oct 21, 2018

View reviewed changes

droogmic added 2 commits October 22, 2018 01:31

Add support for RTC and refactor

be7ebde

fixes, get RTC working

28a0573

droogmic changed the title ~~[WIP] Timers~~ Timers Oct 22, 2018

droogmic commented Oct 23, 2018

View reviewed changes

add TIMER periodic, comment for cancel

3347294

therealprof reviewed Oct 24, 2018

View reviewed changes

droogmic commented Oct 25, 2018

View reviewed changes

droogmic added 3 commits October 27, 2018 12:01

Rewrite time and fixes after review by @therealprof

35ad9ae

Merge branch 'master' into timers

2f48eb8

Merge branch 'master' into timers

0a6911b

therealprof reviewed Oct 27, 2018

View reviewed changes

fix Millis->Lfticks using f64

1095368

droogmic force-pushed the timers branch from 36b9d60 to 1095368 Compare October 27, 2018 21:10

therealprof added the DNM Do not merge! label Oct 28, 2018

droogmic added 2 commits November 3, 2018 17:31

Changes to timers and counters

42c91c6

After @therealprof review + Move time to a seperate file + Move generic timer/counter implementation to file + Create newtype Delay and Timer from TimCo

Merge branch 'master' into timers

d457597

Attempt to implement type states

a6c97ab

Used to seperate Countdown and Delay trait implementation for same Timer struct

therealprof suggested changes Nov 11, 2018

View reviewed changes

droogmic added 3 commits November 18, 2018 17:02

add Duration support to time

a0d1b89

rearrange imports in delay

0890315

further improvements

551feb1

droogmic mentioned this pull request Nov 22, 2018

Demo use of PR#12 in nrf51-hal nrf-rs/microbit#11

Closed

mattheww mentioned this pull request Apr 6, 2019

Drivers for the TIMER and RTC peripherals #19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timers #12

Timers #12

droogmic commented Oct 20, 2018 •

edited

Loading

droogmic Oct 21, 2018 •

edited

Loading

droogmic commented Oct 22, 2018 •

edited

Loading

droogmic Oct 23, 2018

droogmic commented Oct 23, 2018

therealprof Oct 24, 2018

droogmic Oct 25, 2018

therealprof Oct 24, 2018

droogmic Oct 25, 2018

droogmic Oct 25, 2018

therealprof Oct 27, 2018

droogmic Oct 27, 2018

droogmic Oct 27, 2018

therealprof Oct 27, 2018 •

edited

Loading

droogmic commented Oct 28, 2018 •

edited

Loading

therealprof commented Oct 28, 2018

droogmic commented Oct 28, 2018

droogmic commented Oct 28, 2018 •

edited

Loading

droogmic commented Nov 3, 2018

therealprof commented Nov 3, 2018

droogmic commented Nov 3, 2018

droogmic commented Nov 4, 2018

droogmic commented Nov 11, 2018

therealprof left a comment

droogmic commented Nov 22, 2018

therealprof commented Nov 28, 2018

Timers #12

Are you sure you want to change the base?

Timers #12

Conversation

droogmic commented Oct 20, 2018 • edited Loading

droogmic Oct 21, 2018 • edited Loading

Choose a reason for hiding this comment

droogmic commented Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

droogmic commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therealprof Oct 27, 2018 • edited Loading

Choose a reason for hiding this comment

droogmic commented Oct 28, 2018 • edited Loading

therealprof commented Oct 28, 2018

droogmic commented Oct 28, 2018

droogmic commented Oct 28, 2018 • edited Loading

droogmic commented Nov 3, 2018

therealprof commented Nov 3, 2018

droogmic commented Nov 3, 2018

droogmic commented Nov 4, 2018

droogmic commented Nov 11, 2018

therealprof left a comment

Choose a reason for hiding this comment

droogmic commented Nov 22, 2018

therealprof commented Nov 28, 2018

droogmic commented Oct 20, 2018 •

edited

Loading

droogmic Oct 21, 2018 •

edited

Loading

droogmic commented Oct 22, 2018 •

edited

Loading

therealprof Oct 27, 2018 •

edited

Loading

droogmic commented Oct 28, 2018 •

edited

Loading

droogmic commented Oct 28, 2018 •

edited

Loading