-
Notifications
You must be signed in to change notification settings - Fork 171
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
base: master
Are you sure you want to change the base?
add queue example #460
Conversation
Looks very useful, why not merged yet? |
Because I'm not sure it is ready and I was not called as a reviewer. @Vollbrecht - is this ready, and should I review? |
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. |
So this API we should probably remove as I don't see the point of it at all. Other than that, you can But the easiest and most safe (if there is anything safe w.r.t. ISRs) method to work with a |
There was a problem hiding this 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()) }; |
There was a problem hiding this comment.
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 || { |
There was a problem hiding this comment.
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'tcore::mem::forget
it then, but your example is even now un-"forgettable" becausenew_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 withstatic_cell
) also none of the above would be necessary and you can keep the usage ofsubscribe
as opposed to switching tosubscribe_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()) }; |
There was a problem hiding this comment.
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?
Also possibly wrong (but not urgent to fix) issues in 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 |
yeah i will clean it up a bit later, we should get rid of the new_borrowd stuff. |
No description provided.