Incorrect implementation of is_oneshot() in trait Timer in kernel::hil::time

[lebakassemmerl]

As I was trying to implement the Timer trait from kernel::hil::time, I noticed that the function is_oneshot() is probably not correctly implemented.
The current implementation is the following one:

/// Returns whether this is a oneshot (rather than repeating) timer.
fn is_oneshot(&self) -> bool {
    self.interval().is_none()
}

However, the function inverval() says that it can return None if the timer is configured as oneshot-timer OR if the timer is disabled:

/// Returns the interval for a repeating timer.
///
/// Returns `None` if the timer is disabled or in oneshot mode and `Some(interval)` if it is
/// repeating.
fn interval(&self) -> Option<W>;

So I think a correct implementation would be something like this:

fn is_oneshot(&self) -> bool {
    self.interval().is_none() && self.is_enabled()
}

Am I right on this?

[hudson-ayers]

You are right that the existing implementation is incorrect, and I think your correct implementation looks right as well.

[lebakassemmerl]

Ok!
Should I open a PR for this issue?

[hudson-ayers]

A PR would be welcome! There are ongoing efforts to revamp the alarm and timer traits, but they may not land in master for some time, so getting this fix in now would be great.

[lebakassemmerl]

I have opened an PR now, should I close this issue?

[hudson-ayers]

I linked the PR, when it is merged this issue will be closed!