Error handling for timer traits

[hannobraun]

Pull request #67 adds a cancel method that returns a Result. We ended up discussing timer error handling in general there, but decided to move further discussion to a follow-up issue (this one).

I think we need to find answers to the following questions:

1. Can starting a timer fail? Not in the cases I've seen so far, but there might be hardware out there were it could.
2. Can waiting on a timer fail? This might fail due to user error (timer was never started).
3. Are we willing to complicate the traits, if it allows us to catch user errors at compile time?

cc @therealprof @jonas-schievink

[hannobraun]

Regarding question 3: In the other thread, I proposed a set of traits that allow us to catch errors at compile-time. Those won't work in practice, as it's impossible to use the traits without knowing about the implementation, making them useless. I plan to experiment with this idea some more (no timeline yet).

[therealprof]

Re 1) Yes I would say so, e.g. because it already has been started.
Re 2) Potentially yes, but I don't think it is of big importance, also because it might end up being racy because at one point it might be necessary to decide what constitutes as an error (e.g. at which point is an expired timer an error and when is it just expected behaviour; what happens if a timer expired and you wait() for it a second time? Also how would you separate the cases "was never started" and "has expired"?)

[hannobraun]

Another question:

4. Should error types be implementation-defined as associated types, or should we pre-define error enums? @jonas-schievink makes an argument that errors for cancel need to be well-defined. A combination of both approaches would be possible too. An error enum could pre-define some errors, but also have a generic variant for implementation-defined errors.

[austinbes]

Re 4: I'm in favor of the combined approach

[austinbes]

Re 1 and 2: I agree with @therealprof, but I think my assessment of importance is flopped. Either way, it's an argument for a combined approach for those error enums as well -- applications need to be able to distinguish the core "logical" errors from implementation-specific ones.

Overall, it seems like this is a step towards forcing applications to use more explicit error handling. While this is a good thing, it could be good to add some additional trait functions with generic implementations that return the "inner" error type (ignoring "generic" errors). Something like:

pub trait CountDown {
    type Time;
    type Error;

    fn start<T>(&mut self, count: T) -> std::Result<(), CountDownError<Self::Error>>
    where
        T: Into<Self::Time>;

    fn force_start<T>(&mut self, count: T) -> std::Result<(), Self::Error>
    where
        T: Into<Self::Time>;
    {
        match self.start(count) {
            Ok(()) => Ok(()),
            Err(CountDownError::AlreadyStarted) => Ok(()),
            Err(CountDownError::Other(e)) => Err(e),
        }
    }
}

pub enum CountDownError<T> {
    AlreadyStarted,
    Other(T),
}

(disclaimer: haven't tried compiling the above, I'm just providing it as a brief example. I also haven't thought through the function name to any deep degree)

[dsxmachina]

Regarding the error handling discussion I would make an argument for error types instead of generic traits:

You cannot compose errors if you define them as traits.

To make this clear - here is an example a colleague of mine was facing, while trying to write an abstraction for a hardware display (it's not the original code, but a stripped down version):

// We have a display and some items like a SPI, that is defined by the traits from the embedded_hal library
impl<SPI, CS> Display<SPI, CS>
where
    SPI: Transfer<u8> + Write<u8>,
    CS: OutputPin,
{
    pub(crate) fn write(spi: &mut SPI, cs: &mut CS, command: Command) -> Result<(), DisplayError> {
        if cs.set_low().is_err() {
            /* error handling here */
            return DisplayError::CSErr;
        }
        if spi.write(&command.as_byte_array()).is_err() {
            /* error handling here */
            return DisplayError::SPIErr;
        }
        Ok(())
    }

}

So we have a function, that does two things - set some pin and do something with the spi. Both operations can fail and return an Error. What you usually do in this case (FYI: I am not an embedded programmer, maybe I am missing something why you use these traits, I just want to share my perspective), is to implement the From trait for your custom Error type, so that the "?"-operator can do it's magic, and you pipe the error outwards, so the caller can decide what to do:

    pub(crate) fn write(spi: &mut SPI, cs: &mut CS, command: Command) -> Result<(), DisplayError> {
        cs.set_low()?;
        spi.write(&command.as_byte_array())?;
        Ok(())
    }

Looks way cleaner. However, I cannot do this, if SPI::Error and CS::Error are defined as traits, because we don't know the types of these errors yet.
What I would expect from a library is to give me a set of defined error types, that I can then use to wrap in my own error types.
Think of the std::io library from rust - there is a std::io::Error in there, that is well defined. If there are some io::Errors I want to raise, that the designers of the std library did not think of,
I can just use the constructor of io::Error do define something more custom:

        std::io::Error::new(std::io::ErrorKind::Other, "Something unexpected happened");

So I am not bound to any limitations of the pre-defined error type, while still having one well defined type to work with

[eldruin]

@dsxmachina If you are not interested in the contents of the errors, however they look should not matter much in your case.
IIUC, you should be able to do something like:

pub(crate) fn write(spi: &mut SPI, cs: &mut CS, command: Command) -> Result<(), DisplayError> {
    cs.set_low().map_err(|_| DisplayError::Cs)?;
    spi.write(&command.as_byte_array()).map_err(|_| DisplayError::Spi)?;
    Ok(())
}

Anyway this issue is specifically about errors returned by timers, not error handling in general. That was discussed at length over at #229.

PS: remember to set cs high again after the transfer.

[dsxmachina]

Okay, thank you for the info - haven't found the other Issue (might have just searched for open ones). My point was the exact same as therealprof was making in the mentioned issue - I will take a look at the 1.0.0-alpha-0.8 where the changes are merged.

P.S: Thanks :) - but as I said, it's a very stripped down version of the original code. Just wanted to show you what I mean.

[Dirbaio]

I'm going to close this, now that the old timer traits are removed #324.

Discussion for design for the new traits can go in #359 . Linking it so this issue appears there.