add timer api for timer bindings in zephyr #41

jpeeters · 2024-12-10T16:58:06Z

No description provided.

d3zd3z

Thank you for this contribution. Overall, I think this is a good implementation of sys timers. I think it will fit well with the channels I have, where crossbeam supports a timer that gets either a periodic message, or a single message. I'll have to pub some though though into how to do that. But for that, I'll definitely want dynamically allocate timers (Timer::new.()).

The future stuff is interesting, and I look forward to seeing how we can make use of Async. It is interesting to me, but I don't think I'd be able to get to it for a while.

d3zd3z · 2024-12-11T22:21:19Z

zephyr/src/sys/timer.rs

+};
+use crate::time::{Duration, Timeout};
+
+/// Zephyr `k_timer` usable from safe Rust code.


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.

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.

Thank you for the comment. This is very useful.

d3zd3z · 2024-12-11T22:25:24Z

zephyr/src/sys/timer.rs

+
+unsafe impl Sync for StaticKernelObject<k_timer> {}
+
+unsafe impl Send for Timer {}


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.

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

d3zd3z · 2024-12-11T22:29:59Z

zephyr/src/sys/timer.rs

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


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.

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

d3zd3z · 2024-12-11T22:32:33Z

zephyr/src/sys/timer.rs

+
+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);


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.

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.

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.

d3zd3z · 2024-12-11T22:33:55Z

zephyr/src/sys/timer.rs

+    inner: Fixed<k_timer>,
+
+    /// The expired callback if any.
+    expired: Option<(fn(&mut Self, *mut ()), *mut ())>,


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.

d3zd3z · 2024-12-11T22:39:35Z

zephyr/src/sys/timer.rs

+    }
+}
+
+// impl Drop for Timer {


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.

d3zd3z · 2024-12-11T22:41:04Z

zephyr/src/sys/timer.rs

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


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

d3zd3z · 2024-12-11T22:43:02Z

zephyr/src/sys/timer.rs

+//     }
+// }
+
+mod futures {


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.

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.

d3zd3z · 2024-12-11T22:45:01Z

zephyr/src/sys/timer.rs

+            expired: None,
+            _marker: PhantomPinned,
+        }
+    }


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.

d3zd3z · 2024-12-11T22:54:54Z

Oh, another thing, we'll want to come up with some kind of test for this, I'm probably going to be working on enhancing channels next, and it might make sense to have timer tests there, which might count as testing this.

d3zd3z · 2024-12-11T22:55:05Z

Also, are you on the #rust discord channel?

d3zd3z · 2024-12-12T19:24:04Z

@jpeeters Can you have a look at #42. I think I might want to split this into a SimpleTimer type that doesn't have the callback, and a Timer type like what I have here. The SimpleTimer could be static, or the Timer could just have a 'new' method that constructs a dynamic one.

But, I think I've made the types on the callback mechanism sound.

jpeeters · 2024-12-20T13:55:31Z

Also, are you on the #rust discord channel?

I am now ;-)

jpeeters · 2024-12-20T13:57:15Z

@jpeeters Can you have a look at #42. I think I might want to split this into a SimpleTimer type that doesn't have the callback, and a Timer type like what I have here. The SimpleTimer could be static, or the Timer could just have a 'new' method that constructs a dynamic one.

But, I think I've made the types on the callback mechanism sound.

Sorry for the late response. I looked at it and I like it. I need to be more at ease with Zephyr and the Rust integration so as to provide a better feedback right now.

jpeeters · 2024-12-20T13:58:41Z

@d3zd3z I don't know what you think of this : may be I should close this PR and start again from your current implementation of timers. My contribution could be then more focused on futures for instance.

What do you think?

zephyr: add timer api from zephyr bindings

3f99f27

jpeeters marked this pull request as draft December 10, 2024 16:58

d3zd3z requested changes Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add timer api for timer bindings in zephyr #41

add timer api for timer bindings in zephyr #41

jpeeters commented Dec 10, 2024

d3zd3z left a comment

d3zd3z Dec 11, 2024

d3zd3z Dec 12, 2024

jpeeters Dec 20, 2024

d3zd3z Dec 11, 2024

jpeeters Dec 20, 2024

d3zd3z Dec 11, 2024

jpeeters Dec 20, 2024

d3zd3z Dec 11, 2024

d3zd3z Dec 11, 2024

jpeeters Dec 20, 2024

d3zd3z Dec 11, 2024

d3zd3z Dec 11, 2024

d3zd3z Dec 11, 2024

d3zd3z Dec 11, 2024

jpeeters Dec 20, 2024

d3zd3z Dec 11, 2024

d3zd3z commented Dec 11, 2024

d3zd3z commented Dec 11, 2024

d3zd3z commented Dec 12, 2024

jpeeters commented Dec 20, 2024

jpeeters commented Dec 20, 2024

jpeeters commented Dec 20, 2024 •

edited

Loading


		unsafe impl Sync for StaticKernelObject<k_timer> {}

		unsafe impl Send for Timer {}

add timer api for timer bindings in zephyr #41

Are you sure you want to change the base?

add timer api for timer bindings in zephyr #41

Conversation

jpeeters commented Dec 10, 2024

d3zd3z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3zd3z commented Dec 11, 2024

d3zd3z commented Dec 11, 2024

d3zd3z commented Dec 12, 2024

jpeeters commented Dec 20, 2024

jpeeters commented Dec 20, 2024

jpeeters commented Dec 20, 2024 • edited Loading

jpeeters commented Dec 20, 2024 •

edited

Loading