esp-rs / esp-idf-hal

embedded-hal implementation for Rust on ESP32 and ESP-IDF
https://docs.esp-rs.org/esp-idf-hal/
Apache License 2.0
472 stars 171 forks source link

add queue example #460

Open Vollbrecht opened 4 months ago

okhsunrog commented 4 months ago

Looks very useful, why not merged yet?

ivmarkov commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

ivmarkov commented 3 months ago

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 commented 3 months ago

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