Enable cancelable countdowns via a new `Cancel` trait #67

jonas-schievink · 2018-03-26T21:02:53Z

Fixes #65

~~This is the simplest solution; it breaks every current CountDown implementor. Not sure what the policy around breaking changes is since there haven't been any yet.~~

This now adds a Cancel trait that can be implemented by timers to make it possible to cancel a running countdown.

(this is no longer a breaking change)

hannobraun · 2018-03-27T07:00:09Z

Looks good to me. Unless others have objections, I'd like to merge this.

Not sure what the policy around breaking changes is since there haven't been any yet.

Not sure myself what the policy is for breaking changes to proven traits. I don't think we should have too many qualms at this point.

therealprof · 2018-03-27T07:56:32Z

I also agree with this change. The only alternatives I see are:

Provide a default implementation
Turn it into a new trait CancellableCountDown

hannobraun · 2018-03-27T08:04:18Z

@therealprof

I also agree with this change. The only alternatives I see are:

Provide a default implementation

We could provide default implementations for all new methods added to proven traits, then later remove those default implementations in one go, when we're ready to cut a breaking release. I wouldn't be opposed to doing it like this, but personally, I don't think it's worth the hassle at this point.

Turn it into a new trait CancellableCountDown

I think we should only create a separate trait, if there are real timers that can't be cancelled. I'm opposed to adding this complexity solely for backwards compatibility.

therealprof · 2018-03-27T08:12:46Z

I think we should only create a separate trait, if there are real timers that can't be cancelled.

While I don't know of any right now from the top of my head, I'd not exclude the possibility that they exist.

I'm opposed to adding this complexity solely for backwards compatibility.

There're quite a few issues related to this which I wouldn't want to dismiss. Like having to do a major version jump due to semantic versioning rules; we can't/shouldn't do these too often.

Coming to think of it we really should consider adding a default implementation.

jonas-schievink · 2018-03-27T11:46:42Z

I don't disagree with being cautious around breaking changes, but adding a default impl that unconditionally panics doesn't seem right to me (which is probably the sanest behavior it could have).

Maybe we can collect breaking changes and do them all at once? This PR isn't really urgent after all. Of course this would only work if other breaking changes are actually proposed :)

therealprof · 2018-03-27T11:54:00Z

I don't disagree with being cautious around breaking changes, but adding a default impl that unconditionally panics doesn't seem right to me (which is probably the sanest behavior it could have).

Panicking doesn't sound sane to me. If anything it should return a Result (instead of nothing) specifying whether the action was successful. I'd certainly be interested in knowing whether a cancellation was successful or not (e.g. because the CountDown already expired or wasn't started in the first place). A Result could also raise a NotImplemented error.

jonas-schievink · 2018-03-27T12:27:00Z

Ah yes, with that signature change it does make sense

hannobraun · 2018-03-27T19:59:28Z

@therealprof

While I don't know of any right now from the top of my head, I'd not exclude the possibility that they exist.

Same for me. I think we should keep things simple until we have actual evidence of their existence. :-)

There're quite a few issues related to this which I wouldn't want to dismiss. Like having to do a major version jump due to semantic versioning rules; we can't/shouldn't do these too often.

For 0.x versions, semantic versioning only requires a minor version jump (so from 0.1.y to 0.2.0). I don't think this should be a big deal at this early stage.

@jonas-schievink

Maybe we can collect breaking changes and do them all at once? This PR isn't really urgent after all. Of course this would only work if other breaking changes are actually proposed :)

Hence my idea of adding a default implementation, and removing the default implementations all at once, to bundle the breaking changes.

@therealprof

Panicking doesn't sound sane to me. If anything it should return a Result (instead of nothing) specifying whether the action was successful.

I disagree. How exactly would you handle that missing capability at runtime? I think panicking is the sane thing to do, to notify the developer that this operation is not supported, and they need to find another way to achieve whatever they intended to do.

To clarify: In my mind, this default implementation wouldn't be permanent, just a stopgap until we are ready to cut a breaking release which removes all those default implementations.

Please note that I'm only pointing out that we can use default implementations in this way to manage the release of breaking changes. I'm not against doing this, but I also think that it's probably too much hassle, and that breaking changes are not a big deal at this point. It wouldn't be unreasonable to disagree with this, of course.

therealprof · 2018-03-27T20:45:09Z

@hannobraun

I disagree. How exactly would you handle that missing capability at runtime? I think panicking is the sane thing to do, to notify the developer that this operation is not supported, and they need to find another way to achieve whatever they intended to do.

I absolutely hate panics on embedded. Why? Because per default there's no way to figure out that and why a firmware has panicked. For me that's a last resort in case there's something critically wrong on the system which can absolutely not be handled in a sane way and a failure to cancel a countdown is absolutely not in this category.

If something critical is not implemented on an architecture it MUST fail at compile time and MUST always fail (irregardless of compiler options, debug_assert!is the worst atrocity for such a thing).

hannobraun · 2018-03-28T07:58:02Z

@therealprof
I mostly agree with you. I definitely think that failure at compile-time is the way to go, hence my preference for not having the default implementation and just make it a breaking change (i.e. merge the pull request as is).

You convinced me that a panic is not a viable option. I figured that a method that always panics would make it obvious that something is wrong, while the developer can still do something about it. But the cancel call could be buried deep within some nested if blocks, which could hide the panic completely and just lead to weird failures in production.

I'm still not convinced that returning a Result is a good idea. I would absolutely support that, if we knew about some piece of hardware where cancelling the timer can actually fail (we'd probably want to add an associated error type in this case). I don't think it's worth it, just to prevent breaking changes. I think those shouldn't be considered a big deal at this point in time.

therealprof · 2018-03-28T10:56:25Z

I would absolutely support that, if we knew about some piece of hardware where cancelling the timer can actually fail (we'd probably want to add an associated error type in this case).

It's not just hardware that determines whether this operation can fail. There're also usage errors that one might want to flag, like trying to cancel a CountDown which has already expired or never been started.

jonas-schievink · 2018-03-30T16:36:25Z

For consistency, we'd also have to flag those errors when calling wait or start, then (ie. return a Result<(), TimerError>). Is that the final decision here? If so, I can implement the changes.

therealprof · 2018-03-30T17:43:34Z

For me that's the way to go. Anyone else agrees?

hannobraun · 2018-04-11T10:52:18Z

Sorry for not replying earlier. At this point I'm thoroughly confused about what should happen here, thanks to @therealprof (which is good, being confused is better than being wrong).

Some thoughts/questions:

Will the proposed error types be pre-defined (i.e. an enum with variants for "already started", "not yet started", etc.)? Or will they be associated types?
If there will be associated types for errors, will we have one error type, or one error type per function? start can fail in different ways than cancel ("already started" vs. "not yet started").
Knowing at runtime that you did something wrong (e.g. cancel without start) is probably better than not knowing at all. Knowing at compile-time would be even better, but would it be worth the added complexity in this case.

My last point mentions the possibility about knowing about one's mistakes at compile-time. Here's a rough sketch showing what this could look like, in this case:

extern crate nb;


// The timer traits

trait Start {
    type Started;
    type Error;

    fn start(self) -> Result<Self::Started, Self::Error>;
}

trait Wait {
    type Error;

    fn wait(&mut self) -> nb::Result<(), Self::Error>;
}

trait Cancel {
    type Cancelled;
    type Error;

    fn cancel(self) -> Result<Self::Cancelled, Self::Error>;
}


// A simple implementation

pub struct SimpleTimer;

impl<'r> Start for &'r mut SimpleTimer {
    type Started = ();
    type Error   = !;

    // Note: The trait is implemented for `&mut SimpleTimer`, so this is
    // basically like `&mut self`.
    fn start(self) -> Result<(), !> {
        // TODO: Start timer
        Ok(())
    }
}

impl Wait for SimpleTimer {
    type Error = !;

    fn wait(&mut self) -> nb::Result<(), !> {
        // TODO: Wait
        Ok(())
    }
}

impl<'r> Cancel for &'r mut SimpleTimer {
    type Cancelled = ();
    type Error     = !;

    // Note: The trait is implemented for `&mut SimpleTimer`, so this is
    // basically like `&mut self`.
    fn cancel(self) -> Result<(), !> {
        // TODO: Cancel timer
        Ok(())
    }
}


// A robust implementation

pub struct RobustTimer<State>(State);

impl Start for RobustTimer<Unused> {
    type Started = RobustTimer<Started>;
    type Error   = !;

    fn start(self) -> Result<Self::Started, !> {
        // TODO: Start timer
        Ok(RobustTimer(Started))
    }
}

impl Wait for RobustTimer<Started> {
    type Error = !;

    fn wait(&mut self) -> nb::Result<(), !> {
        // TODO: Wait
        Ok(())
    }
}

impl Cancel for RobustTimer<Started> {
    type Cancelled = RobustTimer<Unused>;
    type Error     = !;

    fn cancel(self) -> Result<Self::Cancelled, !> {
        // TODO: Cancel timer
        Ok(RobustTimer(Unused))
    }
}

pub struct Unused;
pub struct Started;

Note: There's an experimental feature for associated type defaults, so Started and Cancelled could default to () one day. This doesn't seem to work well right now, so I've omitted it from the example.

I don't know if this is a good idea or not (although it certainly appeals to me). I'm going to present both sides of the argument:

Pro: Maximum flexibility for implementers. Covers all use cases. And the simple implementation isn't even that much more complicated than it would otherwise have been.
Contra: Additional complexity that might not be worth it. Why do I even need to know that the timer I'm cancelling wasn't started? It's going to end up not running in either case. Plus, this might not even be usable, for unknown reasons that are not apparent from this toy example.

What do you think?

(Disclaimer: Some of the insights required for this comment came from a short conversation with @japaric. I certainly don't speak for him, but I'd like to give credit. And avoid the mistaken impression that I can come up with an independent thought by myself.)

jonas-schievink · 2018-04-11T13:28:42Z

Generally, I'm all for type-safe interfaces that are checked at compile time, but I think we might be stepping into overengineering territory here.

In particular, I'm not yet convinced of the utility of a few aspects discussed here: Is it really useful that cancel returns an error when the timer hasn't been started? As you noted above, if the outcome would be that the timer is stopped in any case, why complain?

Does it ever make sense to report an error when the timer has expired when I try to cancel it? This can happen non-deterministically depending on the timing of things, and I'm doubtful that the proper reaction is to flag an error. If the timer happens to be Periodic, this error would never even happen. I think we should just ignore this and ensure that the timer isn't running after cancel returns.

I do agree with wait returning an error. Waiting on a timer that doesn't run can only do a few things:

Just don't check and enter an endless sleep, which is probably never desirable here
panic!(), which can be problematic when a cancel call can race the wait (imagine your app panicking very rarely, possibly depending on things like data being broadcasted over the air, which slightly affects the timing of everything)
returning an error that can be ignored by the caller, or can be unwrapped if it makes sense (eg. if cancel is never used and the timer just blinks a status LED or something)

therealprof · 2018-04-11T14:01:35Z

@jonas-schievink Well, the user of the timer is always free to just ignore an Error if it doesn't matter in a particular application. In some (hard realtime) applications cancelling an already expired timer may mean that a crucial deadline has been missed which may need to be treated as a critical problem.

I don't think it is over engineering trying to design an interface that is capable of handling all real world applications, especially if the interface doesn't add any major usage obstacles.

hannobraun · 2018-06-19T12:00:00Z

@jonas-schievink This PR has languished for a while now, which is very unfortunate. I'm sorry for not handling this better!

@jonas-schievink @therealprof I'd like to wrap this up. What do you think of this suggestion:

We merge the following: Add the cancel method in a new trait. cancel returns a Result. The error type is defined as an associated type on the new Cancel trait. This is the most conservative choice regarding breaking changes and error handling. It's also a good path towards my proposal, should we choose to implement that later.
We open a follow-up issue to track adding an error to start and wait. I think we should definitely do this, but it would be counterproductive to weigh down this pull request with more decisions.
We open a follow-up issue for implementing the more robust API that I suggested in a previous comment. This needs a prototype (which I intend to build at some later point) and further discussion. Definitely out of scope for this issue.

Can we all agree on this plan?

therealprof · 2018-06-19T12:05:44Z

@hannobraun Sounds good to me.

jonas-schievink · 2018-06-19T18:40:57Z

Okay, sounds good. I'll implement the changes.

jonas-schievink · 2018-06-19T18:52:03Z

Done. I took the liberty to also make CountDown a supertrait of Cancel. IMO the same thing should be done for Periodic.

therealprof

LGTM

hannobraun

Thank you @jonas-schievink! I have some nitpicks. Let me know what you think.

hannobraun · 2018-06-19T19:08:34Z

src/timer.rs

+    /// # Errors
+    ///
+    /// An error will be returned if the countdown has already been canceled or was never started.
+    /// An error is also returned if the countdown is not `Periodic` and has already expired.


I'm wondering if the documentation should be that specific. Maybe setting Error to ! is a valid thing for an implementation to decide.

I'm unsure. Unless others have stronger opinions, I'd feel more comfortable if the documentation were less definitive ("Possible error conditions include ...", something like that).

I think what you say is the right approach for most traits embedded-hal defines, but not this one: It's important for applications to be able to rely on the possible reasons this could fail, because applications will potentially choose to ignore cancellation errors deliberately if the timer has done its job and is no longer needed (ie. the application doesn't care if the timer expired in the meantime).

If the error conditions were unclear, applications would have to .unwrap() every time to be sure, which can cause sporadic panics if the timer expires shortly before that.

Maybe setting Error to ! is a valid thing for an implementation to decide.

This comes with another problem: If this is done, applications can no longer use cancel to determine if the timer has expired before the call. @therealprof has mentioned in #67 (comment) that this might be used by real-time systems to determine if a deadline was missed, which seems like a very valid use case to me (and in fact the only use case that convinced me that returning a Result is useful at all).

So yeah, I think defining the error conditions to some extent is required for this to be useful at all.

Thank you, that makes a lot of sense. In that case we should consider whether pre-defining an error enum (instead of having the associated type) would be a good idea. In the interest of moving things along, I consider that a question for later though. I'll open follow-up issues tomorrow and will make sure to mention that.

hannobraun · 2018-06-19T19:08:46Z

src/timer.rs

@@ -77,3 +77,17 @@ pub trait CountDown {

 /// Marker trait that indicates that a timer is periodic
 pub trait Periodic {}
+
+/// Trait for cancelable countdowns.
+pub trait Cancel : CountDown {


hannobraun · 2018-06-19T19:20:25Z

src/timer.rs

+
+/// Trait for cancelable countdowns.
+pub trait Cancel : CountDown {
+    /// Error returned when a countdown can't be canceled.


Typo. Should be "cancelled" (one "l" missing).

Oops, just found out that this is a British vs. American English thing. Feel free to disregard.

Yeah I had to look this up, too. Decided to go with American English since most internationally used words in computer science and engineering use their AE spelling in favor of BE.

hannobraun · 2018-06-19T20:48:41Z

@japaric I don't think we ever hammered out a policy about who is allowed to merge what under which circumstances. This pull request has seen extensive discussion and has been approved by @therealprof and me. Do you want final veto rights, or is it okay if we merge this?

hannobraun · 2018-06-27T12:21:33Z

Okay, I'm going to merge this. @japaric can revert the change if he doesn't like it :)

67: Prepare 0.4.0-alpha.1 release r=ryankurte a=eldruin Closes rust-embedded#62 Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>

Add a Cancel trait for cancelable countdowns

3804ea8

therealprof approved these changes Jun 19, 2018

View reviewed changes

hannobraun reviewed Jun 19, 2018

View reviewed changes

hannobraun approved these changes Jun 19, 2018

View reviewed changes

jonas-schievink changed the title ~~Add CountDown::cancel~~ Enable cancelable countdowns via a new Cancel trait Jun 19, 2018

This was referenced Jun 20, 2018

Error handling for timer traits #86

Closed

Should timer::CountDown be a supertrait for timer::Periodic? #87

Closed

hannobraun merged commit 99a0f4d into rust-embedded:master Jun 27, 2018

jonas-schievink deleted the countdown-cancel branch June 27, 2018 12:31

peckpeck pushed a commit to peckpeck/embedded-hal that referenced this pull request Nov 10, 2022

Merge rust-embedded#67

ee0e6af

67: Prepare 0.4.0-alpha.1 release r=ryankurte a=eldruin Closes rust-embedded#62 Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable cancelable countdowns via a new `Cancel` trait #67

Enable cancelable countdowns via a new `Cancel` trait #67

jonas-schievink commented Mar 26, 2018 •

edited

Loading

hannobraun commented Mar 27, 2018

therealprof commented Mar 27, 2018

hannobraun commented Mar 27, 2018

therealprof commented Mar 27, 2018

jonas-schievink commented Mar 27, 2018

therealprof commented Mar 27, 2018

jonas-schievink commented Mar 27, 2018

hannobraun commented Mar 27, 2018

therealprof commented Mar 27, 2018

hannobraun commented Mar 28, 2018

therealprof commented Mar 28, 2018

jonas-schievink commented Mar 30, 2018

therealprof commented Mar 30, 2018

hannobraun commented Apr 11, 2018

jonas-schievink commented Apr 11, 2018

therealprof commented Apr 11, 2018

hannobraun commented Jun 19, 2018 •

edited

Loading

therealprof commented Jun 19, 2018

jonas-schievink commented Jun 19, 2018

jonas-schievink commented Jun 19, 2018

therealprof left a comment

hannobraun left a comment

hannobraun Jun 19, 2018

jonas-schievink Jun 19, 2018

hannobraun Jun 19, 2018

hannobraun Jun 19, 2018

hannobraun Jun 19, 2018

hannobraun Jun 19, 2018

jonas-schievink Jun 19, 2018

hannobraun commented Jun 19, 2018

hannobraun commented Jun 27, 2018

Enable cancelable countdowns via a new Cancel trait #67

Enable cancelable countdowns via a new Cancel trait #67

Conversation

jonas-schievink commented Mar 26, 2018 • edited Loading

hannobraun commented Mar 27, 2018

therealprof commented Mar 27, 2018

hannobraun commented Mar 27, 2018

therealprof commented Mar 27, 2018

jonas-schievink commented Mar 27, 2018

therealprof commented Mar 27, 2018

jonas-schievink commented Mar 27, 2018

hannobraun commented Mar 27, 2018

therealprof commented Mar 27, 2018

hannobraun commented Mar 28, 2018

therealprof commented Mar 28, 2018

jonas-schievink commented Mar 30, 2018

therealprof commented Mar 30, 2018

hannobraun commented Apr 11, 2018

jonas-schievink commented Apr 11, 2018

therealprof commented Apr 11, 2018

hannobraun commented Jun 19, 2018 • edited Loading

therealprof commented Jun 19, 2018

jonas-schievink commented Jun 19, 2018

jonas-schievink commented Jun 19, 2018

therealprof left a comment

Choose a reason for hiding this comment

hannobraun left a comment

Choose a reason for hiding this comment

hannobraun Jun 19, 2018

Choose a reason for hiding this comment

jonas-schievink Jun 19, 2018

Choose a reason for hiding this comment

hannobraun Jun 19, 2018

Choose a reason for hiding this comment

hannobraun Jun 19, 2018

Choose a reason for hiding this comment

hannobraun Jun 19, 2018

Choose a reason for hiding this comment

hannobraun Jun 19, 2018

Choose a reason for hiding this comment

jonas-schievink Jun 19, 2018

Choose a reason for hiding this comment

hannobraun commented Jun 19, 2018

hannobraun commented Jun 27, 2018

Enable cancelable countdowns via a new `Cancel` trait #67

Enable cancelable countdowns via a new `Cancel` trait #67

jonas-schievink commented Mar 26, 2018 •

edited

Loading

hannobraun commented Jun 19, 2018 •

edited

Loading