esp-idf-hal icon indicating copy to clipboard operation
esp-idf-hal copied to clipboard

rmt::Transmit::start unsound?

Open karlri opened this issue 3 years ago • 5 comments
trafficstars

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?

karlri avatar Jul 16 '22 19:07 karlri

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 ?

pyaillet avatar Aug 28 '22 07:08 pyaillet

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?

karlri avatar Aug 29 '22 00:08 karlri

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.

faern avatar Jul 23 '23 18:07 faern

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

Vollbrecht avatar Jun 21 '24 09:06 Vollbrecht

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.

ivmarkov avatar Jun 21 '24 10:06 ivmarkov