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 queue example #460

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Vollbrecht
Copy link
Collaborator

No description provided.

@okhsunrog
Copy link
Contributor

Looks very useful, why not merged yet?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Aug 5, 2024

Because I'm not sure it is ready and I was not called as a reviewer. @Vollbrecht - is this ready, and should I review?

@Vollbrecht
Copy link
Collaborator Author

yeah it was not high prio for me just wanted to put it out there before i forgot it.

The current queue API is not optimal but working, and i was thinking about ways to make it more ergonomically and require less unsafe. So till we have something better i just put it out here.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Aug 5, 2024

yeah it was not high prio for me just wanted to put it out there before i forgot it.

The current queue API is not optimal but working, and i was thinking about ways to make it more ergonomically and require less unsafe. So till we have something better i just put it out here.

new_borrowed is ... weird and probably not necessary given that you can always borrow with just & and then unsafely move a copy of the borrowed reference into the ISR - either as a regular reference (with a possibly transmuted lifetime) or as a raw pointer.

So this API we should probably remove as I don't see the point of it at all.

Other than that, you can Arc the Queue and as long as the last reference to the queue is not in the ISR (which would still work but would introduce latencies) you should be OK. (Update: fortunately not true; the Arc would be dropped when you call unsubscribe actually, not from within the ISR).

But the easiest and most safe (if there is anything safe w.r.t. ISRs) method to work with a Queue instance is to just put it in a 'static context with static_cell.

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

I have a few comments that might improve the example - particularly removing new_borrowed which IMO is completely unnecessary. But if you don't have time/will to fix, merge as-is. We might rework later, or I can just do a follow-up push.

let queue_recv = Queue::<u8>::new(100);

// SAFETY: as long as queue_send/recv live we can use isr_receive & isr_send.
let isr_recv: Queue<u8> = unsafe { Queue::new_borrowed(queue_send.as_raw()) };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit - as per my comment in the main PR thread, this is just not necessary. Just borrowing the queues with e.g. &queue_send and &queue_recv is good enough.

//
// Safe since we never end main and thouse never unsubscribe nor drop queue_send/queue_recv
unsafe {
timer.subscribe(move || {
Copy link
Collaborator

@ivmarkov ivmarkov Aug 5, 2024

Choose a reason for hiding this comment

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

Since & is copy (unlike &mut) you can just push the references into subscribe.

  • If indeed subscribe is called, then & need to be transmuted to &'static OR cast to *const Queue<u8> (same outcome)
  • If you use subscribe_nonstatic here, none of the above ^^^ would be necessary, as *_nonstatic can use nonstatic borrows (but yes, don't core::mem::forget it then, but your example is even now un-"forgettable" because new_borrowed is just a hyper-complex way to type & and then unsafely transmute the & lifetime to whatever you want)
  • Finally, if you just Arc the queues (or even better, put then in a 'static context with static_cell) also none of the above would be necessary and you can keep the usage of subscribe as opposed to switching to subscribe_nonstatic.

}

// we can now check if there are still more items in the queue and read the rest.
let unread_item_count = unsafe { uxQueueMessagesWaiting(queue_recv.as_raw()) };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we expose this as a safe method?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Aug 5, 2024

Also possibly wrong (but not urgent to fix) issues in queue.rs I've missed during the original review:

    pub struct Queue<T> {
        ptr: sys::QueueHandle_t,
        is_owned: bool,
        _marker: PhantomData<T>, // Should be `PhantomData<fn() -> T>` instead
    }
unsafe impl<T> Send for Queue<T> where T: Send + Sync {} // T: Send + Sync probably not necessary, because T is Copy already; and in general T: Send should be enough if T was NOT copy
unsafe impl<T> Sync for Queue<T> where T: Send + Sync {} // Ditto

@Vollbrecht
Copy link
Collaborator Author

yeah i will clean it up a bit later, we should get rid of the new_borrowd stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants