Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable cancelable countdowns via a new Cancel trait #67

Merged
merged 1 commit into from
Jun 27, 2018
Merged

Enable cancelable countdowns via a new Cancel trait #67

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/timer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

/// Error returned when a countdown can't be canceled.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type Error;

/// Tries to cancel this countdown.
///
/// # 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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