nrf-rs / nrf51-hal

A Rust embedded-hal impl crate for the Nordic nrf51 series of microcontrollers
BSD Zero Clause License
19 stars 13 forks source link

Timers #12

Open droogmic opened 6 years ago

droogmic commented 6 years ago

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:

Consider / TODO:

droogmic commented 6 years ago

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 commented 6 years ago

Fixes following discussion with @therealprof.

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

droogmic commented 6 years ago

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:

  1. 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
  2. (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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

droogmic commented 6 years ago

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

using a type state

therealprof commented 6 years ago

@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 commented 6 years ago

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

droogmic commented 6 years ago

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

droogmic commented 6 years ago

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

droogmic commented 5 years ago

@therealprof See https://github.com/therealprof/microbit/pull/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 commented 5 years ago

@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.