Implement timer::Cancel trait for CountDownTimer

[mariusknaust]

Analog to the stm32f4xx-hal

[TheZoq2]

Thanks for the PR! Looks good overall, I just have one minor comment:

Would it make more sense to call the Disabled error Canceled instead. Disabled feels like a slightly broader term to me, but that might be some personal bias.

[mariusknaust]

I totally agree, should be changed now.

[mariusknaust]

On second thought, embedded-hal says that an error will also be returned if the timer was never started and in the non-periodic case when it is already expired (not applicable here). So the more generic Disabled might not be that bad, do you have another suggestion?

[TheZoq2]

Hmm good question, perhaps NotRunning or NotCounting?

[mariusknaust]

What do you think of Inactive?

[TheZoq2]

I like that option :) Maybe add a doc comment clarifying what it means in case there is any confusion

[mariusknaust]

I was just going to change it but then I noticed that in this crate the CountDownTimer is kind of a typestate and when it is not started or stopped it will be a normal Timer type, which doesn’t implement the Cancel trait. This means that the case when cancel was called before should be the only possibility to get this error (in contrast to my concern before), which should leave Canceled the most precise name for it. Do you agree?

Sorry for the back and forth 😂 I was actually wondering before if there is any reason for the CountDownTimer typestate in this create or if it would be better to simplify it, so Timer itself implements the CountDown trait, like the other stm32 hal crates do it?

[TheZoq2]

Yea, that's a good point. While I am generally a fan of type states, I think it might be a bit strange for this particular use case. I don't quite remember why we changed it from Timer implementing CountDown, I think it might have been to avoid having Timer take a timeout in its constructor. #95 the corresponding PR does not give much clarity as to why we made that choice.

Either way, I think a better approach, might be to get rid of the type state, make timers start stopped, and have errors returned in functions that are only valid during count down.

If we wanted to, we could also do 2 timer implementations, one type state based version, and one at runtime.

cc: @burrbull (since you authored #95 and may remember some details), @therealprof

[mariusknaust]

Thanks for the link to the PR, now the design makes a lot more sense. It's not a typestate representing if the countdown is running or not, it's more a typestate in which mode the timer is. As you can go from Timer to Pwm, PwmInput, or CountDownTimer mode (and back). I think the design is superior to the simpler once in the other hal crates. It was only lacking a way to stop the countdown without leaving the mode, but this is where the Cancel trait steps in 😉

[TheZoq2]

Oh right, yea. I missremembered what the type states did too, I thought there were separate constructors for the pwm etc. traits :)

[mariusknaust]

I've just rebased the branch to the master, from my side it should be good to be merged. Do you squash the commits on merge?

[mariusknaust]

Never mind, I've squashed them.

[TheZoq2]

Lovely, thanks! And yea, I tend to either squash or rebase depending on how well formated the messages are