-
Notifications
You must be signed in to change notification settings - Fork 211
add queue example #460
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
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 |
ivmarkov
left a comment
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
subscribeis called, then&need to be transmuted to&'staticOR cast to*const Queue<u8>(same outcome) - If you use
subscribe_nonstatichere, none of the above ^^^ would be necessary, as*_nonstaticcan use nonstatic borrows (but yes, don'tcore::mem::forgetit then, but your example is even now un-"forgettable" becausenew_borrowedis just a hyper-complex way to type&and then unsafely transmute the&lifetime to whatever you want) - Finally, if you just
Arcthe queues (or even better, put then in a'staticcontext withstatic_cell) also none of the above would be necessary and you can keep the usage ofsubscribeas 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.