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
439 stars 169 forks source link

rmt::Transmit::start unsound? #102

Open karlri opened 2 years ago

karlri commented 2 years ago

It seems to me that once rmt::Transmit::start has been called, since the signal ownership was passed and rmt_write_items is called without block, the signal actually might (or probably will?) be dropped before actually writing out the items. I tested a bit and was able to get everything from memory access violation errors to watchdog errors. I suppose that start would have to return a handle that holds a reference to the signal and can't be dropped until the signal has been fully written. Having drop for the "signal transmit handle" block is controversial i suppose?

pyaillet commented 2 years ago

Maybe a solution to this would be to keep only the blocking versions and make them the default until a true rust oriented async version is made available. WDYT ?

karlri commented 2 years ago

My only concern about keeping only the blocking API would be using multiple channels. It would be nice to be able to block on writing out n different signals on different channels simultaneously with a single thread.

The blocking API could be extended to make that possible, and then the nonblocking one could be dropped.

The nonblocking API could be kept if it can be ensured that signals can only be passed by value which is dropped on completion. Or by reference, but the data must live long enough, so that mandates a "block-on-drop-until-done handle" or marking the start function unsafe.

I dont know enough about idf to know if the above proposals can be implemented. What do you think?

faern commented 1 year ago

Or by reference, but the data must live long enough, so that mandates a "block-on-drop-until-done handle"

Yes! I came here for this. I find it very limiting that I need to give away ownership of my signal iterators to the write methods. It would be really nice if there was a non-blocking write method that could just borrow the data to be written. With lifetimes like the following I think we can do this:

fn start_write<'a, T>(&mut self, signal_iter: &'a mut T) -> Result<WriteOperation<'a>, EspError>
where
    T: Iterator<Item = rmt_item32_t> + Send + 'a
{ ... }

And the Drop impl for WriteOperation cancels the current write and stops using the iterator.

Vollbrecht commented 2 months ago

@ivmarkov only glancing here at the old issue is this still a problem since start_iter is 'static ?

ivmarkov commented 2 months ago

I think we should keep this open. RMT was contributed, so I haven't looked I to it too carefully, but I would not be surprised if we have soundness errors there, as the posters say.