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.

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

add timer api for timer bindings in zephyr #41

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 zephyr/src/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,20 @@ macro_rules! _kobj_rule {
unsafe { ::core::mem::zeroed() };
};

// static TIMER: staticTimer;
($v:vis, $name:ident, StaticTimer) => {
#[link_section = concat!("._k_timer.static.", stringify!($name), ".", file!(), line!())]
$v static $name: $crate::sys::timer::StaticTimer =
unsafe { ::core::mem::zeroed() };
};

// static TIMERS: [staticTimer; COUNT];
($v:vis, $name:ident, [StaticTimer; $size:expr]) => {
#[link_section = concat!("._k_timer.static.", stringify!($name), ".", file!(), line!())]
$v static $name: [$crate::sys::timer::StaticTimer; $size] =
unsafe { ::core::mem::zeroed() };
};

// Use indirection on stack initializers to handle some different cases in the Rust syntax.
($v:vis, $name:ident, ThreadStack<$size:literal>) => {
$crate::_kobj_stack!($v, $name, $size);
Expand Down
1 change: 1 addition & 0 deletions zephyr/src/sys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use zephyr_sys::k_timeout_t;
pub mod queue;
pub mod sync;
pub mod thread;
pub mod timer;

// These two constants are not able to be captured by bindgen. It is unlikely that these values
// would change in the Zephyr headers, but there will be an explicit test to make sure they are
Expand Down
208 changes: 208 additions & 0 deletions zephyr/src/sys/timer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
// Copyright (c) 2024 EOVE SAS
// SPDX-License-Identifier: Apache-2.0

//! Zephyr `k_timer` wrapper.
//!
//! This module implements a thing wrapper around a `k_timer` object from Zephyr. It works
//! with the [`object`] system from the kernel. This offers to statically allocate a timer.
//!
//! [`object`]: crate::object

use core::fmt;
use core::future::Future;
use core::marker::PhantomPinned;
use core::pin::Pin;

use crate::object::{Fixed, StaticKernelObject, Wrapped};
use crate::raw::{
k_timer, k_timer_init, k_timer_start, k_timer_stop, k_timer_user_data_get,
k_timer_user_data_set,
};
use crate::time::{Duration, Timeout};

/// Zephyr `k_timer` usable from safe Rust code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be a bit of a "stretch" to state that this is usable from safe Rust code. Some of the entries are done with *mut () and are technically safe, but not particularly useful without having to do conversions on the pointers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, thinking about this more, the *mut (), are never possible to use soundly in Rust code. Rust forbids creating a &mut _ when there are any other possible references to that object.

To make this work, the pointer needs to be *const (), and the item should be protected by something like a SpinMutex, which was specifically designed for IRQ handlers.

I'm going to take a stab at seeing what it takes to make this code sound.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the comment. This is very useful.

///
/// This merely wraps a pointer to the kernel object. It implements clone.
pub struct Timer {
/// The raw Zephyr timer.
inner: Fixed<k_timer>,

/// The expired callback if any.
expired: Option<(fn(&mut Self, *mut ()), *mut ())>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's call this expiry to match the similar field in k_timer in Zephyr. It also avoids some confusion, as expired suggests it is some kind of state or such.


/// Marker to prevent from implementing Unpin.
_marker: PhantomPinned,
}

unsafe impl Sync for StaticKernelObject<k_timer> {}

unsafe impl Send for Timer {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll have to think a bit (and have a SAFETY comment here explaining it) as to why Timer would implement Send. Unfortunately, kernel.h doesn't document this very well.

Copy link
Author

Choose a reason for hiding this comment

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

I definitely need to read more about the kernel.h internal to be able to say something here.


impl Timer {
/// Create a new timer from a static pointer.
pub const fn new_from_ptr(ptr: *mut k_timer) -> Self {
Timer {
inner: Fixed::Static(ptr),
expired: None,
_marker: PhantomPinned,
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do think we can provide a new here that uses an owned pointer. In fact, that is probably a bit of overkill with nested pinning, but that is needed to handle the case of wanting static timers.


/// Set the expired callback.
pub fn with_expiry_callback(
&mut self,
expired: fn(&mut Self, *mut ()),
data: *mut (),
) -> &mut Self {
self.expired = Some((expired, data));
self
}

/// Start the Zephyr Timer after a given `duration` and repeat every `period`.
pub fn start(self: Pin<&mut Self>, delay: impl Into<Timeout>, period: impl Into<Timeout>) {
let ptr = self.inner.get();
let user: *mut Self = unsafe { self.get_unchecked_mut() };

unsafe {
k_timer_user_data_set(ptr, user as *mut ::core::ffi::c_void);
k_timer_start(ptr, delay.into().0, period.into().0);
}
}

/// Stop a Zephyr Timer.
pub fn stop(&mut self) {
unsafe { k_timer_stop(self.inner.get()) };
}
}

/// A static Zephyr Timer.
///
/// This is intended to be used from within `kobj_define!` macro. It declares a statically
/// allocated `k_timer` that will be proprely registered with the Zephyr object system. Call
/// [`init_once`] to get the [`Timer`] that is represents.
///
/// [`init_once`]: StaticTimer::init_once
pub type StaticTimer = StaticKernelObject<k_timer>;

impl Wrapped for StaticKernelObject<k_timer> {
type T = Timer;
type I = ();

fn get_wrapped(&self, _: Self::I) -> Self::T {
let ptr = self.value.get();
unsafe { k_timer_init(ptr, Some(expired), None) };
Timer::new_from_ptr(ptr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically, there is a race here, as expired could theoretically be called before the timer is constructed. But, it shouldn't be getting called until after it is started.
If anything, a SAFETY comment on the k_timer_init line, especially explaining why it is safe to do that is in order.

Copy link
Author

Choose a reason for hiding this comment

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

You're right. I lack of safety comment on this. You pointed out what I had in mind.

}
}

unsafe extern "C" fn expired(ktimer: *mut k_timer) {
let user: *mut _ = k_timer_user_data_get(ktimer);
let timer: &mut Timer = &mut *(user as *mut Timer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't safe to do, even in unsafe Rust, as it violates the rule about there, at any time, only ever being a single mutable reference to something. As the timer can be called at any time (and, in fact is called from IRQ context), this reference is almost certainly invalid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But, I notice that your use case below doesn't use the self reference. I suggest, at least initially, we just leave it out, and only rely in the user data passed in.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the feedback on unsafe usage here. I looked at you implementation in #42 and it does more make sense to me now.


if let Some((expired, user)) = timer.expired {
(expired)(timer, user);
}
}

impl fmt::Debug for Timer {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "sys::Timer {:?}", self.inner.get())
}
}

// impl Drop for Timer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's think a bit about whether Drop makes sense here. Right now, these timers can only be declared statically. The real question is, with Zephyr, is it legal to reuse memory for a timer after the timer has been stopped. It'd actually be pretty nice if we could support dynamic timers. In this case, we'd want to be able to have either the static pointer like this, or actually hold the timer itself (inside something pinned).

I think this is safe, as Drop can't be called if there is some thread waiting on the timer, because that thread would still have a reference to the timer. I've asked on #kernel to see if anyone has any more insight.

Part of why this is a decent question is that the use of Pin requires allocation anyway, so it'd be nice to just allocate the timer in the first place, and not have to worry about statics at all.

// #[inline]
// fn drop(&mut self) {
// self.stop();
// }
// }

mod futures {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I'm experienced enough with Futures to know beyond basic safety checks and the likes. Do you have an executor you are using on Zephyr to make use of this.

Copy link
Author

Choose a reason for hiding this comment

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

Yes at the moment I made my tests with embassy-executor with work well. But we could definitely discuss about creating one in zephyr, using the zephyr primitives for certain features like priority queues on separated threads.

use core::cell::RefCell;
use core::future::Future;
use core::marker::PhantomData;
use core::pin::Pin;
use core::task::{Context, Poll, Waker};

use crate::sync::{Arc, Mutex};
use crate::time::{Duration, NoWait};

use super::Timer;

#[derive(Default)]
struct TimerFutureState {
expired: bool,
waker: Option<Waker>,
}

impl TimerFutureState {
fn new() -> Self {
TimerFutureState::default()
}
}

pub struct TimerFuture<'a> {
/// The state of the future, indicating that the time has expired.
state: Arc<Mutex<RefCell<TimerFutureState>>>,

/// Marker to prevent from being dropped before the timer has
_marker: PhantomData<&'a ()>,
}

impl<'a> TimerFuture<'a> {
pub fn new<'b: 'a>(mut timer: Pin<&'b mut Timer>, delay: Duration) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is where it'd be really nice if the k_timer itself could just be inside of the Timer.

let state = Arc::new(Mutex::new(RefCell::new(TimerFutureState::new())));
let user: *const Mutex<_> = &*state;

{
let ptr = unsafe { timer.as_mut().get_unchecked_mut() };
ptr.with_expiry_callback(expired, user as *mut ());
}

// Start a one-shot timer.
timer.as_mut().start(delay, NoWait);

TimerFuture {
state,
_marker: PhantomData,
}
}
}

fn expired(_: &mut Timer, user: *mut ()) {
let mutex: &Mutex<_> = unsafe { &*(user as *const Mutex<RefCell<TimerFutureState>>) };

let mut guard = mutex.lock().expect("mutex lock should never fail");
let state = guard.get_mut();

state.expired = true;

if let Some(waker) = state.waker.take() {
waker.wake();
}
}

impl Future for TimerFuture<'_> {
type Output = ();

fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let mut guard = self.state.lock().expect("mutex lock should never fail");
let state = guard.get_mut();

if state.expired {
return Poll::Ready(());
}

state.waker = Some(cx.waker().clone());
Poll::Pending
}
}
} // mod futures

/// Wait for a given seconds using a timer.
pub fn wait<'a, 'b: 'a>(
timer: Pin<&'b mut Timer>,
delay: Duration,
) -> impl Future<Output = ()> + 'a {
futures::TimerFuture::new(timer, delay)
}
Loading