From a76eb70c23ec4196cd95d6fd977a887210dfb3b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 23 Sep 2024 20:51:33 +0200 Subject: [PATCH 01/19] Drop State from DMA --- esp-hal/src/spi/master.rs | 963 +++++++++++++++++++------------------- 1 file changed, 493 insertions(+), 470 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index c15155b53ad..90bb3001d11 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -977,6 +977,341 @@ mod dma { } } + #[doc(hidden)] + pub trait SpiBusRef<'d>: Sized { + type T: InstanceDma; + type CH: DmaChannel; + type DM: DuplexMode; + type M: Mode; + + fn spi(&self) -> &Self::T; + fn spi_mut(&mut self) -> &mut Self::T; + fn channel(&self) -> &Channel<'d, Self::CH, Self::M>; + fn channel_mut(&mut self) -> &mut Channel<'d, Self::CH, Self::M>; + + fn start_write_bytes_dma( + &mut self, + buffer: &mut TX, + full_duplex: bool, + ) -> Result<(), Error>; + + fn start_read_bytes_dma( + &mut self, + buffer: &mut RX, + full_duplex: bool, + ) -> Result<(), Error>; + + fn start_transfer_dma( + &mut self, + rx_buffer: &mut RX, + tx_buffer: &mut TX, + ) -> Result<(), Error>; + + #[allow(clippy::type_complexity)] + #[cfg_attr(place_spi_driver_in_ram, ram)] + fn do_dma_write( + mut self, + mut buffer: TX, + ) -> Result, (Error, Self, TX)> { + let bytes_to_write = buffer.length(); + if bytes_to_write > MAX_DMA_SIZE { + return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); + } + + if let Err(e) = self.start_write_bytes_dma(&mut buffer, true) { + return Err((e, self, buffer)); + } + + Ok(SpiDmaTransfer::new(self, buffer, false, true)) + } + + #[allow(clippy::type_complexity)] + #[cfg_attr(place_spi_driver_in_ram, ram)] + fn do_dma_read( + mut self, + mut buffer: RX, + ) -> Result, (Error, Self, RX)> { + let bytes_to_read = buffer.length(); + if bytes_to_read > MAX_DMA_SIZE { + return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); + } + + if let Err(e) = self.start_read_bytes_dma(&mut buffer, true) { + return Err((e, self, buffer)); + } + + Ok(SpiDmaTransfer::new(self, buffer, true, false)) + } + + #[allow(clippy::type_complexity)] + fn do_dma_transfer( + mut self, + mut rx_buffer: RX, + mut tx_buffer: TX, + ) -> Result, (Error, Self, RX, TX)> { + let bytes_to_read = rx_buffer.length(); + let bytes_to_write = tx_buffer.length(); + + if bytes_to_write > MAX_DMA_SIZE || bytes_to_read > MAX_DMA_SIZE { + return Err(( + Error::MaxDmaTransferSizeExceeded, + self, + rx_buffer, + tx_buffer, + )); + } + + if let Err(e) = self.start_transfer_dma(&mut rx_buffer, &mut tx_buffer) { + return Err((e, self, rx_buffer, tx_buffer)); + } + + Ok(SpiDmaTransfer::new( + self, + (rx_buffer, tx_buffer), + true, + true, + )) + } + + fn do_half_duplex_read( + mut self, + data_mode: SpiDataMode, + cmd: Command, + address: Address, + dummy: u8, + mut buffer: RX, + ) -> Result, (Error, Self, RX)> + where + Self: SpiBusRef<'d, DM = HalfDuplexMode>, + { + let bytes_to_read = buffer.length(); + if bytes_to_read > MAX_DMA_SIZE { + return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); + } + + self.spi_mut().setup_half_duplex( + false, + cmd, + address, + false, + dummy, + bytes_to_read == 0, + data_mode, + ); + + if let Err(e) = self.start_read_bytes_dma(&mut buffer, false) { + return Err((e, self, buffer)); + } + + Ok(SpiDmaTransfer::new(self, buffer, bytes_to_read > 0, false)) + } + + fn do_half_duplex_write( + mut self, + data_mode: SpiDataMode, + cmd: Command, + address: Address, + dummy: u8, + mut buffer: TX, + ) -> Result, (Error, Self, TX)> + where + Self: SpiBusRef<'d, DM = HalfDuplexMode>, + { + let bytes_to_write = buffer.length(); + if bytes_to_write > MAX_DMA_SIZE { + return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); + } + + #[cfg(all(esp32, spi_address_workaround))] + { + // On the ESP32, if we don't have data, the address is always sent + // on a single line, regardless of its data mode. + if bytes_to_write == 0 && address.mode() != SpiDataMode::Single { + if let Err(e) = self.set_up_address_workaround(cmd, address, dummy) { + return Err((e, self, buffer)); + } + + return Ok(SpiDmaTransfer::new(self, buffer, false, bytes_to_write > 0)); + } + } + + self.spi_mut().setup_half_duplex( + true, + cmd, + address, + false, + dummy, + bytes_to_write == 0, + data_mode, + ); + + if let Err(e) = self.start_write_bytes_dma(&mut buffer, false) { + return Err((e, self, buffer)); + } + + Ok(SpiDmaTransfer::new(self, buffer, false, bytes_to_write > 0)) + } + + #[cfg(all(esp32, spi_address_workaround))] + fn set_up_address_workaround( + &mut self, + cmd: Command, + address: Address, + dummy: u8, + ) -> Result<(), Error>; + } + + impl<'d, T: SpiBusRef<'d>> SpiBusRef<'d> for &mut T { + type T = T::T; + type CH = T::CH; + type DM = T::DM; + type M = T::M; + + fn spi(&self) -> &Self::T { + (**self).spi() + } + fn spi_mut(&mut self) -> &mut Self::T { + (**self).spi_mut() + } + fn channel(&self) -> &Channel<'d, Self::CH, Self::M> { + (**self).channel() + } + fn channel_mut(&mut self) -> &mut Channel<'d, Self::CH, Self::M> { + (**self).channel_mut() + } + + fn start_write_bytes_dma( + &mut self, + buffer: &mut TX, + full_duplex: bool, + ) -> Result<(), Error> { + (**self).start_write_bytes_dma(buffer, full_duplex) + } + + fn start_read_bytes_dma( + &mut self, + buffer: &mut RX, + full_duplex: bool, + ) -> Result<(), Error> { + (**self).start_read_bytes_dma(buffer, full_duplex) + } + + fn start_transfer_dma( + &mut self, + rx_buffer: &mut RX, + tx_buffer: &mut TX, + ) -> Result<(), Error> { + (**self).start_transfer_dma(rx_buffer, tx_buffer) + } + + #[cfg(all(esp32, spi_address_workaround))] + fn set_up_address_workaround( + &mut self, + cmd: Command, + address: Address, + dummy: u8, + ) -> Result<(), Error> { + (**self).set_up_address_workaround(cmd, address, dummy) + } + } + + impl<'d, T, C, D, M> SpiBusRef<'d> for SpiDma<'d, T, C, D, M> + where + C: DmaChannel, + C::P: SpiPeripheral, + D: DuplexMode, + M: Mode, + T: InstanceDma, + { + type T = T; + type CH = C; + type DM = D; + type M = M; + + fn spi(&self) -> &Self::T { + &self.spi + } + fn spi_mut(&mut self) -> &mut Self::T { + &mut self.spi + } + fn channel(&self) -> &Channel<'d, Self::CH, M> { + &self.channel + } + fn channel_mut(&mut self) -> &mut Channel<'d, Self::CH, M> { + &mut self.channel + } + + fn start_write_bytes_dma( + &mut self, + buffer: &mut TX, + full_duplex: bool, + ) -> Result<(), Error> { + unsafe { + self.spi + .start_write_bytes_dma(buffer, &mut self.channel.tx, full_duplex) + } + } + + fn start_read_bytes_dma( + &mut self, + buffer: &mut RX, + full_duplex: bool, + ) -> Result<(), Error> { + unsafe { + self.spi + .start_read_bytes_dma(buffer, &mut self.channel.rx, full_duplex) + } + } + + fn start_transfer_dma( + &mut self, + rx_buffer: &mut RX, + tx_buffer: &mut TX, + ) -> Result<(), Error> { + unsafe { + self.spi.start_transfer_dma( + rx_buffer, + tx_buffer, + &mut self.channel.rx, + &mut self.channel.tx, + ) + } + } + + #[cfg(all(esp32, spi_address_workaround))] + fn set_up_address_workaround( + &mut self, + cmd: Command, + address: Address, + dummy: u8, + ) -> Result<(), Error> { + let bytes_to_write = address.width().div_ceil(8); + // The address register is read in big-endian order, + // we have to prepare the emulated write in the same way. + let addr_bytes = address.value().to_be_bytes(); + let addr_bytes = &addr_bytes[4 - bytes_to_write..][..bytes_to_write]; + self.address_buffer.fill(addr_bytes); + + self.spi.setup_half_duplex( + true, + cmd, + Address::None, + false, + dummy, + bytes_to_write == 0, + address.mode(), + ); + + unsafe { + self.spi.start_write_bytes_dma( + &mut self.address_buffer, + &mut self.channel.tx, + false, + ) + } + } + } + /// A DMA capable SPI instance. /// /// Using `SpiDma` is not recommended unless you wish @@ -1158,31 +1493,26 @@ mod dma { /// /// This structure holds references to the SPI instance, DMA buffers, and /// transfer status. - pub struct SpiDmaTransfer<'d, T, C, D, M, Buf> + pub struct SpiDmaTransfer<'d, R, Buf> where - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, - M: Mode, + R: SpiBusRef<'d>, { - spi_dma: SpiDma<'d, T, C, D, M>, + spi_dma: R, dma_buf: Buf, is_rx: bool, is_tx: bool, rx_future_awaited: bool, tx_future_awaited: bool, + + _marker: PhantomData<&'d ()>, } - impl<'d, T, C, D, M, Buf> SpiDmaTransfer<'d, T, C, D, M, Buf> + impl<'d, R, Buf> SpiDmaTransfer<'d, R, Buf> where - T: Instance, - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, - M: Mode, + R: SpiBusRef<'d>, { - fn new(spi_dma: SpiDma<'d, T, C, D, M>, dma_buf: Buf, is_rx: bool, is_tx: bool) -> Self { + fn new(spi_dma: R, dma_buf: Buf, is_rx: bool, is_tx: bool) -> Self { Self { spi_dma, dma_buf, @@ -1190,6 +1520,7 @@ mod dma { is_tx, rx_future_awaited: false, tx_future_awaited: false, + _marker: PhantomData, } } @@ -1198,10 +1529,10 @@ mod dma { /// This method returns `true` if both RX and TX operations are done, /// and the SPI instance is no longer busy. pub fn is_done(&self) -> bool { - if self.is_tx && !self.tx_future_awaited && !self.spi_dma.channel.tx.is_done() { + if self.is_tx && !self.tx_future_awaited && !self.spi_dma.channel().tx.is_done() { return false; } - if self.spi_dma.spi.busy() { + if self.spi_dma.spi().busy() { return false; } if self.is_rx && !self.rx_future_awaited { @@ -1212,8 +1543,8 @@ mod dma { // of the received data as possible into the buffer and // discarded the rest. The user doesn't care about this discarded data. - if !self.spi_dma.channel.rx.is_done() - && !self.spi_dma.channel.rx.has_dscr_empty_error() + if !self.spi_dma.channel().rx.is_done() + && !self.spi_dma.channel().rx.has_dscr_empty_error() { return false; } @@ -1225,7 +1556,7 @@ mod dma { /// /// This method blocks until the transfer is finished and returns the /// `SpiDma` instance and the associated buffer. - pub fn wait(self) -> (SpiDma<'d, T, C, D, M>, Buf) { + pub fn wait(self) -> (R, Buf) { while !self.is_done() { // Wait for the transfer to complete } @@ -1234,26 +1565,23 @@ mod dma { } } - impl<'d, T, C, D, Buf> SpiDmaTransfer<'d, T, C, D, crate::Async, Buf> + impl<'d, R, Buf> SpiDmaTransfer<'d, R, Buf> where - T: Instance, - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, + R: SpiBusRef<'d, M = crate::Async>, { /// Waits for the DMA transfer to complete asynchronously. /// /// This method awaits the completion of both RX and TX operations. pub async fn wait_for_done(&mut self) { if self.is_tx && !self.tx_future_awaited { - let _ = DmaTxFuture::new(&mut self.spi_dma.channel.tx).await; + let _ = DmaTxFuture::new(&mut self.spi_dma.channel_mut().tx).await; self.tx_future_awaited = true; } // As a future enhancement, setup Spi Future in here as well. if self.is_rx && !self.rx_future_awaited { - let _ = DmaRxFuture::new(&mut self.spi_dma.channel.rx).await; + let _ = DmaRxFuture::new(&mut self.spi_dma.channel_mut().rx).await; self.rx_future_awaited = true; } } @@ -1274,23 +1602,10 @@ mod dma { #[allow(clippy::type_complexity)] #[cfg_attr(place_spi_driver_in_ram, ram)] pub fn dma_write( - mut self, - mut buffer: TX, - ) -> Result, (Error, Self, TX)> { - let bytes_to_write = buffer.length(); - if bytes_to_write > MAX_DMA_SIZE { - return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); - } - - let result = unsafe { - self.spi - .start_write_bytes_dma(&mut buffer, &mut self.channel.tx, true) - }; - if let Err(e) = result { - return Err((e, self, buffer)); - } - - Ok(SpiDmaTransfer::new(self, buffer, false, true)) + self, + buffer: TX, + ) -> Result, (Error, Self, TX)> { + self.do_dma_write(buffer) } /// Perform a DMA read. @@ -1301,23 +1616,10 @@ mod dma { #[allow(clippy::type_complexity)] #[cfg_attr(place_spi_driver_in_ram, ram)] pub fn dma_read( - mut self, - mut buffer: RX, - ) -> Result, (Error, Self, RX)> { - let bytes_to_read = buffer.length(); - if bytes_to_read > MAX_DMA_SIZE { - return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); - } - - let result = unsafe { - self.spi - .start_read_bytes_dma(&mut buffer, &mut self.channel.rx, true) - }; - if let Err(e) = result { - return Err((e, self, buffer)); - } - - Ok(SpiDmaTransfer::new(self, buffer, true, false)) + self, + buffer: RX, + ) -> Result, (Error, Self, RX)> { + self.do_dma_read(buffer) } /// Perform a DMA transfer @@ -1327,41 +1629,11 @@ mod dma { /// sent/received is 32736 bytes. #[allow(clippy::type_complexity)] pub fn dma_transfer( - mut self, - mut rx_buffer: RX, - mut tx_buffer: TX, - ) -> Result, (Error, Self, RX, TX)> - { - let bytes_to_read = rx_buffer.length(); - let bytes_to_write = tx_buffer.length(); - - if bytes_to_write > MAX_DMA_SIZE || bytes_to_read > MAX_DMA_SIZE { - return Err(( - Error::MaxDmaTransferSizeExceeded, - self, - rx_buffer, - tx_buffer, - )); - } - - let result = unsafe { - self.spi.start_transfer_dma( - &mut rx_buffer, - &mut tx_buffer, - &mut self.channel.rx, - &mut self.channel.tx, - ) - }; - if let Err(e) = result { - return Err((e, self, rx_buffer, tx_buffer)); - } - - Ok(SpiDmaTransfer::new( - self, - (rx_buffer, tx_buffer), - true, - true, - )) + self, + rx_buffer: RX, + tx_buffer: TX, + ) -> Result, (Error, Self, RX, TX)> { + self.do_dma_transfer(rx_buffer, tx_buffer) } } @@ -1376,131 +1648,31 @@ mod dma { #[allow(clippy::type_complexity)] #[cfg_attr(place_spi_driver_in_ram, ram)] pub fn read( - mut self, + self, data_mode: SpiDataMode, cmd: Command, address: Address, dummy: u8, - mut buffer: RX, - ) -> Result, (Error, Self, RX)> { - let bytes_to_read = buffer.length(); - if bytes_to_read > MAX_DMA_SIZE { - return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); - } - - self.spi.setup_half_duplex( - false, - cmd, - address, - false, - dummy, - bytes_to_read == 0, - data_mode, - ); - - let result = unsafe { - self.spi - .start_read_bytes_dma(&mut buffer, &mut self.channel.rx, false) - }; - if let Err(e) = result { - return Err((e, self, buffer)); - } - - Ok(SpiDmaTransfer::new(self, buffer, bytes_to_read > 0, false)) + buffer: RX, + ) -> Result, (Error, Self, RX)> { + self.do_half_duplex_read(data_mode, cmd, address, dummy, buffer) } /// Perform a half-duplex write operation using DMA. #[allow(clippy::type_complexity)] #[cfg_attr(place_spi_driver_in_ram, ram)] pub fn write( - mut self, + self, data_mode: SpiDataMode, cmd: Command, address: Address, dummy: u8, - mut buffer: TX, - ) -> Result, (Error, Self, TX)> { - let bytes_to_write = buffer.length(); - if bytes_to_write > MAX_DMA_SIZE { - return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); - } - - #[cfg(all(esp32, spi_address_workaround))] - { - // On the ESP32, if we don't have data, the address is always sent - // on a single line, regardless of its data mode. - if bytes_to_write == 0 && address.mode() != SpiDataMode::Single { - let bytes_to_write = address.width().div_ceil(8); - // The address register is read in big-endian order, - // we have to prepare the emulated write in the same way. - let addr_bytes = address.value().to_be_bytes(); - let addr_bytes = &addr_bytes[4 - bytes_to_write..][..bytes_to_write]; - self.address_buffer.fill(addr_bytes); - - self.spi.setup_half_duplex( - true, - cmd, - Address::None, - false, - dummy, - bytes_to_write == 0, - address.mode(), - ); - - let result = unsafe { - self.spi.start_write_bytes_dma( - &mut self.address_buffer, - &mut self.channel.tx, - false, - ) - }; - if let Err(e) = result { - return Err((e, self, buffer)); - } - - return Ok(SpiDmaTransfer::new(self, buffer, false, bytes_to_write > 0)); - } - } - - self.spi.setup_half_duplex( - true, - cmd, - address, - false, - dummy, - bytes_to_write == 0, - data_mode, - ); - - let result = unsafe { - self.spi - .start_write_bytes_dma(&mut buffer, &mut self.channel.tx, false) - }; - if let Err(e) = result { - return Err((e, self, buffer)); - } - - Ok(SpiDmaTransfer::new(self, buffer, false, bytes_to_write > 0)) + buffer: TX, + ) -> Result, (Error, Self, TX)> { + self.do_half_duplex_write(data_mode, cmd, address, dummy, buffer) } } - #[derive(Default)] - enum State<'d, T, C, D, M> - where - T: InstanceDma, - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, - M: Mode, - { - Idle(SpiDma<'d, T, C, D, M>, DmaRxBuf, DmaTxBuf), - Reading(SpiDmaTransfer<'d, T, C, D, M, DmaRxBuf>, DmaTxBuf), - Writing(SpiDmaTransfer<'d, T, C, D, M, DmaTxBuf>, DmaRxBuf), - Transferring(SpiDmaTransfer<'d, T, C, D, M, (DmaRxBuf, DmaTxBuf)>), - #[default] - TemporarilyRemoved, - } - /// A DMA-capable SPI bus. /// /// This structure is responsible for managing SPI transfers using DMA @@ -1513,7 +1685,9 @@ mod dma { D: DuplexMode, M: Mode, { - state: State<'d, T, C, D, M>, + spi_dma: SpiDma<'d, T, C, D, M>, + rx_buf: DmaRxBuf, + tx_buf: DmaTxBuf, } impl<'d, T, C, D, M> SpiDmaBus<'d, T, C, D, M> @@ -1526,13 +1700,11 @@ mod dma { { /// Creates a new `SpiDmaBus` with the specified SPI instance and DMA /// buffers. - pub fn new( - spi: SpiDma<'d, T, C, D, M>, - dma_rx_buf: DmaRxBuf, - dma_tx_buf: DmaTxBuf, - ) -> Self { + pub fn new(spi_dma: SpiDma<'d, T, C, D, M>, rx_buf: DmaRxBuf, tx_buf: DmaTxBuf) -> Self { Self { - state: State::Idle(spi, dma_rx_buf, dma_tx_buf), + spi_dma, + rx_buf, + tx_buf, } } @@ -1540,69 +1712,36 @@ mod dma { /// /// Interrupts are not enabled at the peripheral level here. pub fn set_interrupt_handler(&mut self, handler: InterruptHandler) { - let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - spi_dma.set_interrupt_handler(handler); - self.state = State::Idle(spi_dma, rx_buf, tx_buf); + self.spi_dma.set_interrupt_handler(handler); } /// Listen for the given interrupts #[cfg(gdma)] pub fn listen(&mut self, interrupts: EnumSet) { - let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - spi_dma.listen(interrupts); - self.state = State::Idle(spi_dma, rx_buf, tx_buf); + self.spi_dma.listen(interrupts); } /// Unlisten the given interrupts #[cfg(gdma)] pub fn unlisten(&mut self, interrupts: EnumSet) { - let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - spi_dma.unlisten(interrupts); - self.state = State::Idle(spi_dma, rx_buf, tx_buf); + self.spi_dma.unlisten(interrupts); } /// Gets asserted interrupts #[cfg(gdma)] pub fn interrupts(&mut self) -> EnumSet { - let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - let interrupts = spi_dma.interrupts(); - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - - interrupts + self.spi_dma.interrupts() } /// Resets asserted interrupts #[cfg(gdma)] pub fn clear_interrupts(&mut self, interrupts: EnumSet) { - let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - spi_dma.clear_interrupts(interrupts); - self.state = State::Idle(spi_dma, rx_buf, tx_buf); + self.spi_dma.clear_interrupts(interrupts); } /// Changes the SPI bus frequency for the DMA-enabled SPI instance. pub fn change_bus_frequency(&mut self, frequency: HertzU32) { - let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - spi_dma.change_bus_frequency(frequency); - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - } - - fn wait_for_idle(&mut self) -> (SpiDma<'d, T, C, D, M>, DmaRxBuf, DmaTxBuf) { - match core::mem::take(&mut self.state) { - State::Idle(spi, rx_buf, tx_buf) => (spi, rx_buf, tx_buf), - State::Reading(transfer, tx_buf) => { - let (spi, rx_buf) = transfer.wait(); - (spi, rx_buf, tx_buf) - } - State::Writing(transfer, rx_buf) => { - let (spi, tx_buf) = transfer.wait(); - (spi, rx_buf, tx_buf) - } - State::Transferring(transfer) => { - let (spi, (rx_buf, tx_buf)) = transfer.wait(); - (spi, rx_buf, tx_buf) - } - State::TemporarilyRemoved => unreachable!(), - } + self.spi_dma.change_bus_frequency(frequency); } } @@ -1616,9 +1755,7 @@ mod dma { { /// Configures the interrupt handler for the DMA-enabled SPI instance. fn set_interrupt_handler(&mut self, handler: crate::interrupt::InterruptHandler) { - let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - SpiDma::set_interrupt_handler(&mut spi_dma, handler); - self.state = State::Idle(spi_dma, rx_buf, tx_buf); + SpiDma::set_interrupt_handler(&mut self.spi_dma, handler); } } @@ -1641,56 +1778,40 @@ mod dma { { /// Reads data from the SPI bus using DMA. pub fn read(&mut self, words: &mut [u8]) -> Result<(), Error> { - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle(); + for chunk in words.chunks_mut(self.rx_buf.capacity()) { + self.rx_buf.set_length(chunk.len()); - for chunk in words.chunks_mut(rx_buf.capacity()) { - rx_buf.set_length(chunk.len()); + match (&mut self.spi_dma).do_dma_read(&mut self.rx_buf) { + Ok(transfer) => { + transfer.wait(); - match spi_dma.dma_read(rx_buf) { - Ok(transfer) => self.state = State::Reading(transfer, tx_buf), - Err((e, spi, rx)) => { - self.state = State::Idle(spi, rx, tx_buf); - return Err(e); + let bytes_read = self.rx_buf.read_received_data(chunk); + debug_assert_eq!(bytes_read, chunk.len()); } + Err((e, _, _)) => return Err(e), } - (spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - - let bytes_read = rx_buf.read_received_data(chunk); - debug_assert_eq!(bytes_read, chunk.len()); } - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - Ok(()) } /// Writes data to the SPI bus using DMA. pub fn write(&mut self, words: &[u8]) -> Result<(), Error> { - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle(); + for chunk in words.chunks(self.tx_buf.capacity()) { + self.tx_buf.fill(chunk); - for chunk in words.chunks(tx_buf.capacity()) { - tx_buf.fill(chunk); - - match spi_dma.dma_write(tx_buf) { - Ok(transfer) => self.state = State::Writing(transfer, rx_buf), - Err((e, spi, tx)) => { - self.state = State::Idle(spi, rx_buf, tx); - return Err(e); - } - } - (spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); + match (&mut self.spi_dma).do_dma_write(&mut self.tx_buf) { + Ok(transfer) => transfer.wait(), + Err((e, _, _)) => return Err(e), + }; } - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - Ok(()) } /// Transfers data to and from the SPI bus simultaneously using DMA. pub fn transfer(&mut self, read: &mut [u8], write: &[u8]) -> Result<(), Error> { - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle(); - - let chunk_size = min(tx_buf.capacity(), rx_buf.capacity()); + let chunk_size = min(self.tx_buf.capacity(), self.rx_buf.capacity()); let common_length = min(read.len(), write.len()); let (read_common, read_remainder) = read.split_at_mut(common_length); @@ -1700,24 +1821,20 @@ mod dma { .chunks_mut(chunk_size) .zip(write_common.chunks(chunk_size)) { - tx_buf.fill(write_chunk); - rx_buf.set_length(read_chunk.len()); - - match spi_dma.dma_transfer(rx_buf, tx_buf) { - Ok(transfer) => self.state = State::Transferring(transfer), - Err((e, spi, rx, tx)) => { - self.state = State::Idle(spi, rx, tx); - return Err(e); + self.tx_buf.fill(write_chunk); + self.rx_buf.set_length(read_chunk.len()); + + match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { + Ok(transfer) => { + transfer.wait(); + + let bytes_read = self.rx_buf.read_received_data(read_chunk); + debug_assert_eq!(bytes_read, read_chunk.len()); } + Err((e, _, _, _)) => return Err(e), } - (spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - - let bytes_read = rx_buf.read_received_data(read_chunk); - debug_assert_eq!(bytes_read, read_chunk.len()); } - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - if !read_remainder.is_empty() { self.read(read_remainder) } else if !write_remainder.is_empty() { @@ -1729,28 +1846,23 @@ mod dma { /// Transfers data in place on the SPI bus using DMA. pub fn transfer_in_place(&mut self, words: &mut [u8]) -> Result<(), Error> { - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle(); - - let chunk_size = min(tx_buf.capacity(), rx_buf.capacity()); + let chunk_size = min(self.tx_buf.capacity(), self.rx_buf.capacity()); for chunk in words.chunks_mut(chunk_size) { - tx_buf.fill(chunk); - rx_buf.set_length(chunk.len()); - - match spi_dma.dma_transfer(rx_buf, tx_buf) { - Ok(transfer) => self.state = State::Transferring(transfer), - Err((e, spi, rx, tx)) => { - self.state = State::Idle(spi, rx, tx); - return Err(e); + self.tx_buf.fill(chunk); + self.rx_buf.set_length(chunk.len()); + + match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { + Ok(transfer) => { + transfer.wait(); + + let bytes_read = self.rx_buf.read_received_data(chunk); + debug_assert_eq!(bytes_read, chunk.len()); } + Err((e, _, _, _)) => return Err(e), } - (spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - - let bytes_read = rx_buf.read_received_data(chunk); - debug_assert_eq!(bytes_read, chunk.len()); } - self.state = State::Idle(spi_dma, rx_buf, tx_buf); Ok(()) } } @@ -1773,28 +1885,27 @@ mod dma { dummy: u8, buffer: &mut [u8], ) -> Result<(), Self::Error> { - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle(); - if buffer.len() > rx_buf.capacity() { + if buffer.len() > self.rx_buf.capacity() { return Err(super::Error::DmaError(DmaError::Overflow)); } + self.rx_buf.set_length(buffer.len()); - rx_buf.set_length(buffer.len()); + match (&mut self.spi_dma).do_half_duplex_read( + data_mode, + cmd, + address, + dummy, + &mut self.rx_buf, + ) { + Ok(transfer) => { + transfer.wait(); - match spi_dma.read(data_mode, cmd, address, dummy, rx_buf) { - Ok(transfer) => self.state = State::Reading(transfer, tx_buf), - Err((e, spi, rx)) => { - self.state = State::Idle(spi, rx, tx_buf); - return Err(e); + let bytes_read = self.rx_buf.read_received_data(buffer); + debug_assert_eq!(bytes_read, buffer.len()); + Ok(()) } + Err((e, _, _)) => return Err(e), } - (spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - - let bytes_read = rx_buf.read_received_data(buffer); - debug_assert_eq!(bytes_read, buffer.len()); - - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - - Ok(()) } /// Half-duplex write. @@ -1806,25 +1917,25 @@ mod dma { dummy: u8, buffer: &[u8], ) -> Result<(), Self::Error> { - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle(); - if buffer.len() > tx_buf.capacity() { + if buffer.len() > self.tx_buf.capacity() { return Err(super::Error::DmaError(DmaError::Overflow)); } + self.tx_buf.fill(buffer); - tx_buf.fill(buffer); + match (&mut self.spi_dma).do_half_duplex_write( + data_mode, + cmd, + address, + dummy, + &mut self.tx_buf, + ) { + Ok(transfer) => { + transfer.wait(); - match spi_dma.write(data_mode, cmd, address, dummy, tx_buf) { - Ok(transfer) => self.state = State::Writing(transfer, rx_buf), - Err((e, spi, tx)) => { - self.state = State::Idle(spi, rx_buf, tx); - return Err(e); + Ok(()) } + Err((e, _, _)) => Err(e), } - (spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); - - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - - Ok(()) } } @@ -1860,7 +1971,7 @@ mod dma { /// Async functionality mod asynch { - use core::{cmp::min, mem::take}; + use core::cmp::min; use super::*; @@ -1870,90 +1981,40 @@ mod dma { C: DmaChannel, C::P: SpiPeripheral, { - async fn wait_for_idle_async( - &mut self, - ) -> ( - SpiDma<'d, T, C, FullDuplexMode, crate::Async>, - DmaRxBuf, - DmaTxBuf, - ) { - match &mut self.state { - State::Idle(_, _, _) => (), - State::Reading(transfer, _) => transfer.wait_for_done().await, - State::Writing(transfer, _) => transfer.wait_for_done().await, - State::Transferring(transfer) => transfer.wait_for_done().await, - State::TemporarilyRemoved => unreachable!(), - } - match take(&mut self.state) { - State::Idle(spi, rx_buf, tx_buf) => (spi, rx_buf, tx_buf), - State::Reading(transfer, tx_buf) => { - let (spi, rx_buf) = transfer.wait(); - (spi, rx_buf, tx_buf) - } - State::Writing(transfer, rx_buf) => { - let (spi, tx_buf) = transfer.wait(); - (spi, rx_buf, tx_buf) - } - State::Transferring(transfer) => { - let (spi, (rx_buf, tx_buf)) = transfer.wait(); - (spi, rx_buf, tx_buf) - } - State::TemporarilyRemoved => unreachable!(), - } - } - /// Fill the given buffer with data from the bus. pub async fn read_async(&mut self, words: &mut [u8]) -> Result<(), super::Error> { - // Get previous transfer. - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle_async().await; + let chunk_size = self.rx_buf.capacity(); - for chunk in words.chunks_mut(rx_buf.capacity()) { - rx_buf.set_length(chunk.len()); + for chunk in words.chunks_mut(chunk_size) { + self.rx_buf.set_length(chunk.len()); - match spi_dma.dma_read(rx_buf) { - Ok(transfer) => { - self.state = State::Reading(transfer, tx_buf); - } - Err((e, spi, rx)) => { - self.state = State::Idle(spi, rx, tx_buf); - return Err(e); + match (&mut self.spi_dma).do_dma_read(&mut self.rx_buf) { + Ok(mut transfer) => { + transfer.wait_for_done().await; + + let bytes_read = self.rx_buf.read_received_data(chunk); + debug_assert_eq!(bytes_read, chunk.len()); } + Err((e, _, _)) => return Err(e), }; - - (spi_dma, rx_buf, tx_buf) = self.wait_for_idle_async().await; - - let bytes_read = rx_buf.read_received_data(chunk); - debug_assert_eq!(bytes_read, chunk.len()); } - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - Ok(()) } /// Transmit the given buffer to the bus. pub async fn write_async(&mut self, words: &[u8]) -> Result<(), super::Error> { - // Get previous transfer. - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle_async().await; + let chunk_size = self.tx_buf.capacity(); - for chunk in words.chunks(tx_buf.capacity()) { - tx_buf.fill(chunk); + for chunk in words.chunks(chunk_size) { + self.tx_buf.fill(chunk); - match spi_dma.dma_write(tx_buf) { - Ok(transfer) => { - self.state = State::Writing(transfer, rx_buf); - } - Err((e, spi, tx)) => { - self.state = State::Idle(spi, rx_buf, tx); - return Err(e); - } - }; - - (spi_dma, rx_buf, tx_buf) = self.wait_for_idle_async().await; + match (&mut self.spi_dma).do_dma_write(&mut self.tx_buf) { + Ok(mut transfer) => transfer.wait_for_done().await, + Err((e, _, _)) => return Err(e), + } } - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - Ok(()) } @@ -1964,10 +2025,7 @@ mod dma { read: &mut [u8], write: &[u8], ) -> Result<(), super::Error> { - // Get previous transfer. - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle_async().await; - - let chunk_size = min(tx_buf.capacity(), rx_buf.capacity()); + let chunk_size = min(self.tx_buf.capacity(), self.rx_buf.capacity()); let common_length = min(read.len(), write.len()); let (read_common, read_remainder) = read.split_at_mut(common_length); @@ -1977,27 +2035,20 @@ mod dma { .chunks_mut(chunk_size) .zip(write_common.chunks(chunk_size)) { - tx_buf.fill(write_chunk); - rx_buf.set_length(read_chunk.len()); - - match spi_dma.dma_transfer(rx_buf, tx_buf) { - Ok(transfer) => { - self.state = State::Transferring(transfer); - } - Err((e, spi, rx, tx)) => { - self.state = State::Idle(spi, rx, tx); - return Err(e); - } - }; + self.tx_buf.fill(write_chunk); + self.rx_buf.set_length(read_chunk.len()); - (spi_dma, rx_buf, tx_buf) = self.wait_for_idle_async().await; + match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { + Ok(mut transfer) => { + transfer.wait_for_done().await; - let bytes_read = rx_buf.read_received_data(read_chunk); - assert_eq!(bytes_read, read_chunk.len()); + let bytes_read = self.rx_buf.read_received_data(read_chunk); + assert_eq!(bytes_read, read_chunk.len()); + } + Err((e, _, _, _)) => return Err(e), + } } - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - if !read_remainder.is_empty() { self.read_async(read_remainder).await } else if !write_remainder.is_empty() { @@ -2013,50 +2064,21 @@ mod dma { &mut self, words: &mut [u8], ) -> Result<(), super::Error> { - // Get previous transfer. - let (mut spi_dma, mut rx_buf, mut tx_buf) = self.wait_for_idle_async().await; + for chunk in words.chunks_mut(self.tx_buf.capacity()) { + self.tx_buf.fill(chunk); + self.rx_buf.set_length(chunk.len()); - for chunk in words.chunks_mut(tx_buf.capacity()) { - tx_buf.fill(chunk); - rx_buf.set_length(chunk.len()); + match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { + Ok(mut transfer) => { + transfer.wait_for_done().await; - match spi_dma.dma_transfer(rx_buf, tx_buf) { - Ok(transfer) => { - self.state = State::Transferring(transfer); - } - Err((e, spi, rx, tx)) => { - self.state = State::Idle(spi, rx, tx); - return Err(e); + let bytes_read = self.rx_buf.read_received_data(chunk); + assert_eq!(bytes_read, chunk.len()); } - }; - - match &mut self.state { - State::Transferring(transfer) => transfer.wait_for_done().await, - _ => unreachable!(), - }; - - (spi_dma, rx_buf, tx_buf) = match take(&mut self.state) { - State::Transferring(transfer) => { - let (spi, (rx_buf, tx_buf)) = transfer.wait(); - (spi, rx_buf, tx_buf) - } - _ => unreachable!(), - }; - - let bytes_read = rx_buf.read_received_data(chunk); - debug_assert_eq!(bytes_read, chunk.len()); + Err((e, _, _, _)) => return Err(e), + } } - self.state = State::Idle(spi_dma, rx_buf, tx_buf); - - Ok(()) - } - - /// Flush any pending data in the SPI peripheral. - pub async fn flush_async(&mut self) -> Result<(), super::Error> { - // Get previous transfer. - let (spi_dma, rx_buf, tx_buf) = self.wait_for_idle_async().await; - self.state = State::Idle(spi_dma, rx_buf, tx_buf); Ok(()) } } @@ -2084,7 +2106,8 @@ mod dma { } async fn flush(&mut self) -> Result<(), Self::Error> { - self.flush_async().await + // All operations currently flush so this is no-op. + Ok(()) } } } From 0fa2b0636f317652b4ec8d16abfd0a876b2cc09b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 23 Sep 2024 20:52:41 +0200 Subject: [PATCH 02/19] Simplify Error paths --- esp-hal/src/spi/master.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 90bb3001d11..6825281d605 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1874,7 +1874,7 @@ mod dma { C::P: SpiPeripheral, M: Mode, { - type Error = super::Error; + type Error = Error; /// Half-duplex read. fn read( @@ -1886,7 +1886,7 @@ mod dma { buffer: &mut [u8], ) -> Result<(), Self::Error> { if buffer.len() > self.rx_buf.capacity() { - return Err(super::Error::DmaError(DmaError::Overflow)); + return Err(Error::DmaError(DmaError::Overflow)); } self.rx_buf.set_length(buffer.len()); @@ -1918,7 +1918,7 @@ mod dma { buffer: &[u8], ) -> Result<(), Self::Error> { if buffer.len() > self.tx_buf.capacity() { - return Err(super::Error::DmaError(DmaError::Overflow)); + return Err(Error::DmaError(DmaError::Overflow)); } self.tx_buf.fill(buffer); @@ -1946,7 +1946,7 @@ mod dma { C: DmaChannel, C::P: SpiPeripheral, { - type Error = super::Error; + type Error = Error; fn transfer<'w>(&mut self, words: &'w mut [u8]) -> Result<&'w [u8], Self::Error> { self.transfer_in_place(words)?; @@ -1961,7 +1961,7 @@ mod dma { C: DmaChannel, C::P: SpiPeripheral, { - type Error = super::Error; + type Error = Error; fn write(&mut self, words: &[u8]) -> Result<(), Self::Error> { self.write(words)?; @@ -1982,7 +1982,7 @@ mod dma { C::P: SpiPeripheral, { /// Fill the given buffer with data from the bus. - pub async fn read_async(&mut self, words: &mut [u8]) -> Result<(), super::Error> { + pub async fn read_async(&mut self, words: &mut [u8]) -> Result<(), Error> { let chunk_size = self.rx_buf.capacity(); for chunk in words.chunks_mut(chunk_size) { @@ -2003,7 +2003,7 @@ mod dma { } /// Transmit the given buffer to the bus. - pub async fn write_async(&mut self, words: &[u8]) -> Result<(), super::Error> { + pub async fn write_async(&mut self, words: &[u8]) -> Result<(), Error> { let chunk_size = self.tx_buf.capacity(); for chunk in words.chunks(chunk_size) { @@ -2024,7 +2024,7 @@ mod dma { &mut self, read: &mut [u8], write: &[u8], - ) -> Result<(), super::Error> { + ) -> Result<(), Error> { let chunk_size = min(self.tx_buf.capacity(), self.rx_buf.capacity()); let common_length = min(read.len(), write.len()); @@ -2060,10 +2060,7 @@ mod dma { /// Transfer by writing out a buffer and reading the response from /// the bus into the same buffer. - pub async fn transfer_in_place_async( - &mut self, - words: &mut [u8], - ) -> Result<(), super::Error> { + pub async fn transfer_in_place_async(&mut self, words: &mut [u8]) -> Result<(), Error> { for chunk in words.chunks_mut(self.tx_buf.capacity()) { self.tx_buf.fill(chunk); self.rx_buf.set_length(chunk.len()); @@ -2165,7 +2162,7 @@ mod ehal1 { use super::*; impl embedded_hal::spi::ErrorType for Spi<'_, T, M> { - type Error = super::Error; + type Error = Error; } impl FullDuplex for Spi<'_, T, FullDuplexMode> From 4c7fd67d1e51dcfbbcd7bbe681aa6be3d17692d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 23 Sep 2024 22:00:23 +0200 Subject: [PATCH 03/19] Cancel dropped transfers, fix and test --- esp-hal/src/spi/master.rs | 140 +++++++++++++++++------------- hil-test/tests/spi_full_duplex.rs | 98 +++++++++++++++++---- 2 files changed, 164 insertions(+), 74 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 6825281d605..794c23d417c 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -908,6 +908,7 @@ where mod dma { use core::{ cmp::min, + mem::ManuallyDrop, sync::atomic::{fence, Ordering}, }; @@ -1234,10 +1235,10 @@ mod dma { fn spi_mut(&mut self) -> &mut Self::T { &mut self.spi } - fn channel(&self) -> &Channel<'d, Self::CH, M> { + fn channel(&self) -> &Channel<'d, C, M> { &self.channel } - fn channel_mut(&mut self) -> &mut Channel<'d, Self::CH, M> { + fn channel_mut(&mut self) -> &mut Channel<'d, C, M> { &mut self.channel } @@ -1497,8 +1498,8 @@ mod dma { where R: SpiBusRef<'d>, { - spi_dma: R, - dma_buf: Buf, + spi_dma: ManuallyDrop, + dma_buf: ManuallyDrop, is_rx: bool, is_tx: bool, @@ -1514,8 +1515,8 @@ mod dma { { fn new(spi_dma: R, dma_buf: Buf, is_rx: bool, is_tx: bool) -> Self { Self { - spi_dma, - dma_buf, + spi_dma: ManuallyDrop::new(spi_dma), + dma_buf: ManuallyDrop::new(dma_buf), is_rx, is_tx, rx_future_awaited: false, @@ -1552,16 +1553,50 @@ mod dma { true } + fn do_wait(&self) { + while !self.is_done() { + // Wait for the transfer to complete + } + fence(Ordering::Acquire); + } + /// Waits for the DMA transfer to complete. /// /// This method blocks until the transfer is finished and returns the /// `SpiDma` instance and the associated buffer. - pub fn wait(self) -> (R, Buf) { - while !self.is_done() { - // Wait for the transfer to complete + pub fn wait(mut self) -> (R, Buf) { + self.do_wait(); + unsafe { + ( + ManuallyDrop::take(&mut self.spi_dma), + ManuallyDrop::take(&mut self.dma_buf), + ) } - fence(Ordering::Acquire); - (self.spi_dma, self.dma_buf) + } + + /// Cancels the DMA transfer. + pub fn cancel(&mut self) { + if !self.is_done() { + self.spi_dma.spi_mut().configure_datalen(0, 0); + if self.is_tx { + self.spi_dma.channel_mut().tx.stop_transfer(); + self.is_tx = false; + } + if self.is_rx { + self.spi_dma.channel_mut().rx.stop_transfer(); + self.is_rx = false; + } + } + } + } + + impl<'d, R, Buf> Drop for SpiDmaTransfer<'d, R, Buf> + where + R: SpiBusRef<'d>, + { + fn drop(&mut self) { + self.cancel(); + self.do_wait(); } } @@ -1782,14 +1817,12 @@ mod dma { self.rx_buf.set_length(chunk.len()); match (&mut self.spi_dma).do_dma_read(&mut self.rx_buf) { - Ok(transfer) => { - transfer.wait(); - - let bytes_read = self.rx_buf.read_received_data(chunk); - debug_assert_eq!(bytes_read, chunk.len()); - } + Ok(transfer) => transfer.wait(), Err((e, _, _)) => return Err(e), - } + }; + + let bytes_read = self.rx_buf.read_received_data(chunk); + debug_assert_eq!(bytes_read, chunk.len()); } Ok(()) @@ -1825,14 +1858,12 @@ mod dma { self.rx_buf.set_length(read_chunk.len()); match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { - Ok(transfer) => { - transfer.wait(); - - let bytes_read = self.rx_buf.read_received_data(read_chunk); - debug_assert_eq!(bytes_read, read_chunk.len()); - } + Ok(transfer) => transfer.wait(), Err((e, _, _, _)) => return Err(e), - } + }; + + let bytes_read = self.rx_buf.read_received_data(read_chunk); + debug_assert_eq!(bytes_read, read_chunk.len()); } if !read_remainder.is_empty() { @@ -1853,14 +1884,12 @@ mod dma { self.rx_buf.set_length(chunk.len()); match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { - Ok(transfer) => { - transfer.wait(); - - let bytes_read = self.rx_buf.read_received_data(chunk); - debug_assert_eq!(bytes_read, chunk.len()); - } + Ok(transfer) => transfer.wait(), Err((e, _, _, _)) => return Err(e), - } + }; + + let bytes_read = self.rx_buf.read_received_data(chunk); + debug_assert_eq!(bytes_read, chunk.len()); } Ok(()) @@ -1897,15 +1926,14 @@ mod dma { dummy, &mut self.rx_buf, ) { - Ok(transfer) => { - transfer.wait(); - - let bytes_read = self.rx_buf.read_received_data(buffer); - debug_assert_eq!(bytes_read, buffer.len()); - Ok(()) - } + Ok(transfer) => transfer.wait(), Err((e, _, _)) => return Err(e), - } + }; + + let bytes_read = self.rx_buf.read_received_data(buffer); + debug_assert_eq!(bytes_read, buffer.len()); + + Ok(()) } /// Half-duplex write. @@ -1989,14 +2017,12 @@ mod dma { self.rx_buf.set_length(chunk.len()); match (&mut self.spi_dma).do_dma_read(&mut self.rx_buf) { - Ok(mut transfer) => { - transfer.wait_for_done().await; - - let bytes_read = self.rx_buf.read_received_data(chunk); - debug_assert_eq!(bytes_read, chunk.len()); - } + Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _)) => return Err(e), - }; + } + + let bytes_read = self.rx_buf.read_received_data(chunk); + debug_assert_eq!(bytes_read, chunk.len()); } Ok(()) @@ -2039,14 +2065,12 @@ mod dma { self.rx_buf.set_length(read_chunk.len()); match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { - Ok(mut transfer) => { - transfer.wait_for_done().await; - - let bytes_read = self.rx_buf.read_received_data(read_chunk); - assert_eq!(bytes_read, read_chunk.len()); - } + Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _, _)) => return Err(e), } + + let bytes_read = self.rx_buf.read_received_data(read_chunk); + assert_eq!(bytes_read, read_chunk.len()); } if !read_remainder.is_empty() { @@ -2066,14 +2090,12 @@ mod dma { self.rx_buf.set_length(chunk.len()); match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { - Ok(mut transfer) => { - transfer.wait_for_done().await; - - let bytes_read = self.rx_buf.read_received_data(chunk); - assert_eq!(bytes_read, chunk.len()); - } + Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _, _)) => return Err(e), } + + let bytes_read = self.rx_buf.read_received_data(chunk); + assert_eq!(bytes_read, chunk.len()); } Ok(()) diff --git a/hil-test/tests/spi_full_duplex.rs b/hil-test/tests/spi_full_duplex.rs index 76ee33a90a3..04bfd9c1335 100644 --- a/hil-test/tests/spi_full_duplex.rs +++ b/hil-test/tests/spi_full_duplex.rs @@ -12,7 +12,7 @@ use embedded_hal::spi::SpiBus; #[cfg(pcnt)] use embedded_hal_async::spi::SpiBus as SpiBusAsync; use esp_hal::{ - dma::{Dma, DmaPriority, DmaRxBuf, DmaTxBuf}, + dma::{Dma, DmaDescriptor, DmaPriority, DmaRxBuf, DmaTxBuf}, dma_buffers, gpio::{Io, Level, NoPin}, peripherals::SPI2, @@ -37,6 +37,11 @@ cfg_if::cfg_if! { struct Context { spi: Spi<'static, SPI2, FullDuplexMode>, dma_channel: DmaChannelCreator, + // Reuse the really large buffer so we don't run out of DRAM with many tests + rx_buffer: &'static mut [u8], + rx_descriptors: &'static mut [DmaDescriptor], + tx_buffer: &'static mut [u8], + tx_descriptors: &'static mut [DmaDescriptor], #[cfg(pcnt)] pcnt_source: InputSignal, #[cfg(pcnt)] @@ -74,17 +79,21 @@ mod tests { .with_mosi(mosi) .with_miso(mosi_loopback); - cfg_if::cfg_if! { - if #[cfg(pcnt)] { - let pcnt = Pcnt::new(peripherals.PCNT); - Context { - spi, dma_channel, - pcnt_source: mosi_loopback_pcnt, - pcnt_unit: pcnt.unit0, - } - } else { - Context { spi, dma_channel } - } + let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(32000); + + #[cfg(pcnt)] + let pcnt = Pcnt::new(peripherals.PCNT); + Context { + spi, + dma_channel, + rx_buffer, + rx_descriptors, + tx_buffer, + tx_descriptors, + #[cfg(pcnt)] + pcnt_source: mosi_loopback_pcnt, + #[cfg(pcnt)] + pcnt_unit: pcnt.unit0, } } @@ -251,9 +260,8 @@ mod tests { fn test_symmetric_dma_transfer(ctx: Context) { // This test case sends a large amount of data, multiple times to verify that // https://github.com/esp-rs/esp-hal/issues/2151 is and remains fixed. - let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(32000); - let mut dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap(); - let mut dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap(); + let mut dma_rx_buf = DmaRxBuf::new(ctx.rx_descriptors, ctx.rx_buffer).unwrap(); + let mut dma_tx_buf = DmaTxBuf::new(ctx.tx_descriptors, ctx.tx_buffer).unwrap(); for (i, v) in dma_tx_buf.as_mut_slice().iter_mut().enumerate() { *v = (i % 255) as u8; @@ -478,4 +486,64 @@ mod tests { assert_eq!(&[0xff, 0xff, 0xff, 0xff], dma_rx_buf.as_slice()); } + + #[test] + #[timeout(3)] + fn cancel_stops_transaction(mut ctx: Context) { + // Slow down + ctx.spi.change_bus_frequency(100.Hz()); + + // Set up a large buffer that would trigger a timeout + let dma_rx_buf = DmaRxBuf::new(ctx.rx_descriptors, ctx.rx_buffer).unwrap(); + let dma_tx_buf = DmaTxBuf::new(ctx.tx_descriptors, ctx.tx_buffer).unwrap(); + + let spi = ctx + .spi + .with_dma(ctx.dma_channel.configure(false, DmaPriority::Priority0)); + + let mut transfer = spi + .dma_transfer(dma_rx_buf, dma_tx_buf) + .map_err(|e| e.0) + .unwrap(); + + transfer.cancel(); + transfer.wait(); + } + + #[test] + #[timeout(3)] + fn can_transmit_after_cancel(mut ctx: Context) { + // Slow down + ctx.spi.change_bus_frequency(100.Hz()); + + // Set up a large buffer that would trigger a timeout + let mut dma_rx_buf = DmaRxBuf::new(ctx.rx_descriptors, ctx.rx_buffer).unwrap(); + let mut dma_tx_buf = DmaTxBuf::new(ctx.tx_descriptors, ctx.tx_buffer).unwrap(); + + let mut spi = ctx + .spi + .with_dma(ctx.dma_channel.configure(false, DmaPriority::Priority0)); + + let mut transfer = spi + .dma_transfer(dma_rx_buf, dma_tx_buf) + .map_err(|e| e.0) + .unwrap(); + + transfer.cancel(); + (spi, (dma_rx_buf, dma_tx_buf)) = transfer.wait(); + + spi.change_bus_frequency(10000.kHz()); + + let transfer = spi + .dma_transfer(dma_rx_buf, dma_tx_buf) + .map_err(|e| e.0) + .unwrap(); + + let (_, (dma_rx_buf, dma_tx_buf)) = transfer.wait(); + if dma_tx_buf.as_slice() != dma_rx_buf.as_slice() { + defmt::info!("dma_tx_buf: {:?}", dma_tx_buf.as_slice()[0..100]); + defmt::info!("dma_rx_buf: {:?}", dma_rx_buf.as_slice()[0..100]); + panic!("Failed to transmit after cancel"); + } + } } From 023b37e444d9aa1af9c36d741011c215d0f55d5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 23 Sep 2024 23:18:51 +0200 Subject: [PATCH 04/19] Fix C6 --- esp-hal/src/spi/master.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 794c23d417c..891ca6e4d60 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1578,6 +1578,7 @@ mod dma { pub fn cancel(&mut self) { if !self.is_done() { self.spi_dma.spi_mut().configure_datalen(0, 0); + self.spi_dma.spi_mut().update(); if self.is_tx { self.spi_dma.channel_mut().tx.stop_transfer(); self.is_tx = false; From 0e290872f13df285ade2cf5b16c72728fc4c34f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 24 Sep 2024 00:10:59 +0200 Subject: [PATCH 05/19] Avoid cancelling a completed transaction --- esp-hal/src/spi/master.rs | 49 +++++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 891ca6e4d60..f1b9600aa15 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1560,6 +1560,27 @@ mod dma { fence(Ordering::Acquire); } + fn do_cancel(&mut self) { + cfg_if::cfg_if! { + if #[cfg(esp32)] { + // TODO: examine what happens exactly. (0, 0) just doesn't take effect. + self.spi_dma.spi_mut().configure_datalen(1, 1); + } else { + self.spi_dma.spi_mut().configure_datalen(0, 0); + } + }; + self.spi_dma.spi_mut().update(); + + if self.is_tx { + self.spi_dma.channel_mut().tx.stop_transfer(); + self.is_tx = false; + } + if self.is_rx { + self.spi_dma.channel_mut().rx.stop_transfer(); + self.is_rx = false; + } + } + /// Waits for the DMA transfer to complete. /// /// This method blocks until the transfer is finished and returns the @@ -1577,16 +1598,7 @@ mod dma { /// Cancels the DMA transfer. pub fn cancel(&mut self) { if !self.is_done() { - self.spi_dma.spi_mut().configure_datalen(0, 0); - self.spi_dma.spi_mut().update(); - if self.is_tx { - self.spi_dma.channel_mut().tx.stop_transfer(); - self.is_tx = false; - } - if self.is_rx { - self.spi_dma.channel_mut().rx.stop_transfer(); - self.is_rx = false; - } + self.do_cancel(); } } } @@ -1596,8 +1608,10 @@ mod dma { R: SpiBusRef<'d>, { fn drop(&mut self) { - self.cancel(); - self.do_wait(); + if !self.is_done() { + self.do_cancel(); + self.do_wait(); + } } } @@ -1620,6 +1634,17 @@ mod dma { let _ = DmaRxFuture::new(&mut self.spi_dma.channel_mut().rx).await; self.rx_future_awaited = true; } + + core::future::poll_fn(|cx| { + use core::task::Poll; + if self.is_done() { + Poll::Ready(()) + } else { + cx.waker().wake_by_ref(); + Poll::Pending + } + }) + .await; } } From 87d61b6dc195aa9f4f2f27fa9519ba347ca23d84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 24 Sep 2024 13:23:07 +0200 Subject: [PATCH 06/19] Do not implement DmaTxRxBuf for references --- esp-hal/src/spi/master.rs | 51 ++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index f1b9600aa15..937d724b685 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -922,8 +922,10 @@ mod dma { DmaChannel, DmaRxBuf, DmaRxBuffer, + DmaRxBufferRef, DmaTxBuf, DmaTxBuffer, + DmaTxBufferRef, Rx, Spi2Peripheral, SpiPeripheral, @@ -1842,7 +1844,8 @@ mod dma { for chunk in words.chunks_mut(self.rx_buf.capacity()) { self.rx_buf.set_length(chunk.len()); - match (&mut self.spi_dma).do_dma_read(&mut self.rx_buf) { + let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + match (&mut self.spi_dma).do_dma_read(rx_buffer) { Ok(transfer) => transfer.wait(), Err((e, _, _)) => return Err(e), }; @@ -1859,7 +1862,8 @@ mod dma { for chunk in words.chunks(self.tx_buf.capacity()) { self.tx_buf.fill(chunk); - match (&mut self.spi_dma).do_dma_write(&mut self.tx_buf) { + let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + match (&mut self.spi_dma).do_dma_write(tx_buffer) { Ok(transfer) => transfer.wait(), Err((e, _, _)) => return Err(e), }; @@ -1883,7 +1887,9 @@ mod dma { self.tx_buf.fill(write_chunk); self.rx_buf.set_length(read_chunk.len()); - match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { + let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { Ok(transfer) => transfer.wait(), Err((e, _, _, _)) => return Err(e), }; @@ -1909,7 +1915,9 @@ mod dma { self.tx_buf.fill(chunk); self.rx_buf.set_length(chunk.len()); - match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { + let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { Ok(transfer) => transfer.wait(), Err((e, _, _, _)) => return Err(e), }; @@ -1945,13 +1953,9 @@ mod dma { } self.rx_buf.set_length(buffer.len()); - match (&mut self.spi_dma).do_half_duplex_read( - data_mode, - cmd, - address, - dummy, - &mut self.rx_buf, - ) { + let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + match (&mut self.spi_dma).do_half_duplex_read(data_mode, cmd, address, dummy, rx_buffer) + { Ok(transfer) => transfer.wait(), Err((e, _, _)) => return Err(e), }; @@ -1976,13 +1980,10 @@ mod dma { } self.tx_buf.fill(buffer); - match (&mut self.spi_dma).do_half_duplex_write( - data_mode, - cmd, - address, - dummy, - &mut self.tx_buf, - ) { + let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + match (&mut self.spi_dma) + .do_half_duplex_write(data_mode, cmd, address, dummy, tx_buffer) + { Ok(transfer) => { transfer.wait(); @@ -2042,7 +2043,8 @@ mod dma { for chunk in words.chunks_mut(chunk_size) { self.rx_buf.set_length(chunk.len()); - match (&mut self.spi_dma).do_dma_read(&mut self.rx_buf) { + let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + match (&mut self.spi_dma).do_dma_read(rx_buffer) { Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _)) => return Err(e), } @@ -2061,7 +2063,8 @@ mod dma { for chunk in words.chunks(chunk_size) { self.tx_buf.fill(chunk); - match (&mut self.spi_dma).do_dma_write(&mut self.tx_buf) { + let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + match (&mut self.spi_dma).do_dma_write(tx_buffer) { Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _)) => return Err(e), } @@ -2090,7 +2093,9 @@ mod dma { self.tx_buf.fill(write_chunk); self.rx_buf.set_length(read_chunk.len()); - match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { + let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _, _)) => return Err(e), } @@ -2115,7 +2120,9 @@ mod dma { self.tx_buf.fill(chunk); self.rx_buf.set_length(chunk.len()); - match (&mut self.spi_dma).do_dma_transfer(&mut self.rx_buf, &mut self.tx_buf) { + let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _, _)) => return Err(e), } From a7c46d9e21284bbcf63ee34f0074d1f72232cb37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 24 Sep 2024 13:29:01 +0200 Subject: [PATCH 07/19] Remove unnecessary import --- esp-hal/src/spi/master.rs | 50 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 937d724b685..157e52fcb50 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1404,32 +1404,45 @@ mod dma { /// /// Interrupts are not enabled at the peripheral level here. pub fn set_interrupt_handler(&mut self, handler: InterruptHandler) { + self.wait_for_idle(); self.spi.set_interrupt_handler(handler); } /// Listen for the given interrupts #[cfg(gdma)] pub fn listen(&mut self, interrupts: EnumSet) { + self.wait_for_idle(); self.spi.listen(interrupts); } /// Unlisten the given interrupts #[cfg(gdma)] pub fn unlisten(&mut self, interrupts: EnumSet) { + self.wait_for_idle(); self.spi.unlisten(interrupts); } /// Gets asserted interrupts #[cfg(gdma)] pub fn interrupts(&mut self) -> EnumSet { + self.wait_for_idle(); self.spi.interrupts() } /// Resets asserted interrupts #[cfg(gdma)] pub fn clear_interrupts(&mut self, interrupts: EnumSet) { + self.wait_for_idle(); self.spi.clear_interrupts(interrupts); } + + fn wait_for_idle(&self) { + // TODO: don't ignore DMA status. We can move the transfer's logic here. + while self.spi.busy() { + // Wait for the SPI to become idle + } + fence(Ordering::Acquire); + } } impl<'d, T, C, D, M> crate::private::Sealed for SpiDma<'d, T, C, D, M> @@ -1668,6 +1681,8 @@ mod dma { self, buffer: TX, ) -> Result, (Error, Self, TX)> { + self.wait_for_idle(); + self.do_dma_write(buffer) } @@ -1682,6 +1697,8 @@ mod dma { self, buffer: RX, ) -> Result, (Error, Self, RX)> { + self.wait_for_idle(); + self.do_dma_read(buffer) } @@ -1696,6 +1713,8 @@ mod dma { rx_buffer: RX, tx_buffer: TX, ) -> Result, (Error, Self, RX, TX)> { + self.wait_for_idle(); + self.do_dma_transfer(rx_buffer, tx_buffer) } } @@ -1718,6 +1737,8 @@ mod dma { dummy: u8, buffer: RX, ) -> Result, (Error, Self, RX)> { + self.wait_for_idle(); + self.do_half_duplex_read(data_mode, cmd, address, dummy, buffer) } @@ -1732,6 +1753,8 @@ mod dma { dummy: u8, buffer: TX, ) -> Result, (Error, Self, TX)> { + self.wait_for_idle(); + self.do_half_duplex_write(data_mode, cmd, address, dummy, buffer) } } @@ -1771,6 +1794,10 @@ mod dma { } } + fn wait_for_idle(&mut self) { + self.spi_dma.wait_for_idle(); + } + /// Sets the interrupt handler /// /// Interrupts are not enabled at the peripheral level here. @@ -1841,6 +1868,7 @@ mod dma { { /// Reads data from the SPI bus using DMA. pub fn read(&mut self, words: &mut [u8]) -> Result<(), Error> { + self.wait_for_idle(); for chunk in words.chunks_mut(self.rx_buf.capacity()) { self.rx_buf.set_length(chunk.len()); @@ -1859,6 +1887,7 @@ mod dma { /// Writes data to the SPI bus using DMA. pub fn write(&mut self, words: &[u8]) -> Result<(), Error> { + self.wait_for_idle(); for chunk in words.chunks(self.tx_buf.capacity()) { self.tx_buf.fill(chunk); @@ -1874,6 +1903,7 @@ mod dma { /// Transfers data to and from the SPI bus simultaneously using DMA. pub fn transfer(&mut self, read: &mut [u8], write: &[u8]) -> Result<(), Error> { + self.wait_for_idle(); let chunk_size = min(self.tx_buf.capacity(), self.rx_buf.capacity()); let common_length = min(read.len(), write.len()); @@ -1909,6 +1939,7 @@ mod dma { /// Transfers data in place on the SPI bus using DMA. pub fn transfer_in_place(&mut self, words: &mut [u8]) -> Result<(), Error> { + self.wait_for_idle(); let chunk_size = min(self.tx_buf.capacity(), self.rx_buf.capacity()); for chunk in words.chunks_mut(chunk_size) { @@ -1951,6 +1982,7 @@ mod dma { if buffer.len() > self.rx_buf.capacity() { return Err(Error::DmaError(DmaError::Overflow)); } + self.wait_for_idle(); self.rx_buf.set_length(buffer.len()); let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); @@ -1978,6 +2010,7 @@ mod dma { if buffer.len() > self.tx_buf.capacity() { return Err(Error::DmaError(DmaError::Overflow)); } + self.wait_for_idle(); self.tx_buf.fill(buffer); let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); @@ -2036,8 +2069,22 @@ mod dma { C: DmaChannel, C::P: SpiPeripheral, { + async fn wait_for_idle_async(&mut self) { + core::future::poll_fn(|cx| { + use core::task::Poll; + if !self.spi_dma.spi().busy() { + Poll::Ready(()) + } else { + cx.waker().wake_by_ref(); + Poll::Pending + } + }) + .await; + } + /// Fill the given buffer with data from the bus. pub async fn read_async(&mut self, words: &mut [u8]) -> Result<(), Error> { + self.wait_for_idle_async().await; let chunk_size = self.rx_buf.capacity(); for chunk in words.chunks_mut(chunk_size) { @@ -2058,6 +2105,7 @@ mod dma { /// Transmit the given buffer to the bus. pub async fn write_async(&mut self, words: &[u8]) -> Result<(), Error> { + self.wait_for_idle_async().await; let chunk_size = self.tx_buf.capacity(); for chunk in words.chunks(chunk_size) { @@ -2080,6 +2128,7 @@ mod dma { read: &mut [u8], write: &[u8], ) -> Result<(), Error> { + self.wait_for_idle_async().await; let chunk_size = min(self.tx_buf.capacity(), self.rx_buf.capacity()); let common_length = min(read.len(), write.len()); @@ -2116,6 +2165,7 @@ mod dma { /// Transfer by writing out a buffer and reading the response from /// the bus into the same buffer. pub async fn transfer_in_place_async(&mut self, words: &mut [u8]) -> Result<(), Error> { + self.wait_for_idle_async().await; for chunk in words.chunks_mut(self.tx_buf.capacity()) { self.tx_buf.fill(chunk); self.rx_buf.set_length(chunk.len()); From ebcdbd185871fc29f841f6dfa76727d5d741de17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 24 Sep 2024 15:48:44 +0200 Subject: [PATCH 08/19] Merge BufferRef structs --- esp-hal/src/spi/master.rs | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 157e52fcb50..c043c321c19 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -919,13 +919,12 @@ mod dma { dma::{ asynch::{DmaRxFuture, DmaTxFuture}, Channel, + DmaBufferRef, DmaChannel, DmaRxBuf, DmaRxBuffer, - DmaRxBufferRef, DmaTxBuf, DmaTxBuffer, - DmaTxBufferRef, Rx, Spi2Peripheral, SpiPeripheral, @@ -1872,7 +1871,7 @@ mod dma { for chunk in words.chunks_mut(self.rx_buf.capacity()) { self.rx_buf.set_length(chunk.len()); - let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); match (&mut self.spi_dma).do_dma_read(rx_buffer) { Ok(transfer) => transfer.wait(), Err((e, _, _)) => return Err(e), @@ -1891,7 +1890,7 @@ mod dma { for chunk in words.chunks(self.tx_buf.capacity()) { self.tx_buf.fill(chunk); - let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); match (&mut self.spi_dma).do_dma_write(tx_buffer) { Ok(transfer) => transfer.wait(), Err((e, _, _)) => return Err(e), @@ -1917,8 +1916,8 @@ mod dma { self.tx_buf.fill(write_chunk); self.rx_buf.set_length(read_chunk.len()); - let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); - let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); + let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { Ok(transfer) => transfer.wait(), Err((e, _, _, _)) => return Err(e), @@ -1946,8 +1945,8 @@ mod dma { self.tx_buf.fill(chunk); self.rx_buf.set_length(chunk.len()); - let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); - let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); + let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { Ok(transfer) => transfer.wait(), Err((e, _, _, _)) => return Err(e), @@ -1985,7 +1984,7 @@ mod dma { self.wait_for_idle(); self.rx_buf.set_length(buffer.len()); - let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); match (&mut self.spi_dma).do_half_duplex_read(data_mode, cmd, address, dummy, rx_buffer) { Ok(transfer) => transfer.wait(), @@ -2013,7 +2012,7 @@ mod dma { self.wait_for_idle(); self.tx_buf.fill(buffer); - let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); match (&mut self.spi_dma) .do_half_duplex_write(data_mode, cmd, address, dummy, tx_buffer) { @@ -2090,7 +2089,7 @@ mod dma { for chunk in words.chunks_mut(chunk_size) { self.rx_buf.set_length(chunk.len()); - let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); + let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); match (&mut self.spi_dma).do_dma_read(rx_buffer) { Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _)) => return Err(e), @@ -2111,7 +2110,7 @@ mod dma { for chunk in words.chunks(chunk_size) { self.tx_buf.fill(chunk); - let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); match (&mut self.spi_dma).do_dma_write(tx_buffer) { Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _)) => return Err(e), @@ -2142,8 +2141,8 @@ mod dma { self.tx_buf.fill(write_chunk); self.rx_buf.set_length(read_chunk.len()); - let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); - let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); + let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _, _)) => return Err(e), @@ -2170,8 +2169,8 @@ mod dma { self.tx_buf.fill(chunk); self.rx_buf.set_length(chunk.len()); - let rx_buffer = DmaRxBufferRef::new(&mut self.rx_buf); - let tx_buffer = DmaTxBufferRef::new(&mut self.tx_buf); + let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); + let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { Ok(mut transfer) => transfer.wait_for_done().await, Err((e, _, _, _)) => return Err(e), From df2efeb7cea83d7eef186bc405e9aebf073f6f09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 24 Sep 2024 21:26:00 +0200 Subject: [PATCH 09/19] Move wait impl to the peripheral --- esp-hal/src/spi/master.rs | 336 ++++++++++++++++-------------- hil-test/tests/spi_full_duplex.rs | 24 +++ 2 files changed, 206 insertions(+), 154 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index c043c321c19..b16737298be 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -991,6 +991,91 @@ mod dma { fn channel(&self) -> &Channel<'d, Self::CH, Self::M>; fn channel_mut(&mut self) -> &mut Channel<'d, Self::CH, Self::M>; + fn is_rx_in_progress(&self) -> bool; + fn is_tx_in_progress(&self) -> bool; + fn set_rx_in_progress(&mut self, in_progress: bool); + fn set_tx_in_progress(&mut self, in_progress: bool); + + fn is_done(&self) -> bool; + + fn wait_for_idle(&self) { + while !self.is_done() { + // Wait for the SPI to become idle + } + fence(Ordering::Acquire); + } + + async fn wait_for_idle_async(&mut self) { + // As a future enhancement, setup Spi Future in here as well. + + /// An optional future. + enum OptionalFuture { + Future(T), + Value(T::Output), + } + + impl OptionalFuture { + fn some(future: T) -> Self { + Self::Future(future) + } + + fn none(value: T::Output) -> Self { + Self::Value(value) + } + } + + use core::{ + future::Future, + pin::{pin, Pin}, + task::{Context, Poll}, + }; + impl Future for OptionalFuture + where + T: Future + Unpin, + T::Output: Copy + Unpin, + { + type Output = T::Output; + + fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + match self.get_mut() { + Self::Future(future) => pin!(future).poll(cx), + Self::Value(value) => Poll::Ready(*value), + } + } + } + + let is_rx = self.is_rx_in_progress(); + let is_tx = self.is_tx_in_progress(); + let channels = self.channel_mut(); + + let rx_future = if is_rx { + OptionalFuture::some(DmaRxFuture::new(&mut channels.rx)) + } else { + OptionalFuture::none(Ok(())) + }; + let tx_future = if is_tx { + OptionalFuture::some(DmaTxFuture::new(&mut channels.tx)) + } else { + OptionalFuture::none(Ok(())) + }; + + _ = embassy_futures::join::join(rx_future, tx_future).await; + + self.set_rx_in_progress(false); + self.set_tx_in_progress(false); + + core::future::poll_fn(|cx| { + use core::task::Poll; + if self.is_done() { + Poll::Ready(()) + } else { + cx.waker().wake_by_ref(); + Poll::Pending + } + }) + .await; + } + fn start_write_bytes_dma( &mut self, buffer: &mut TX, @@ -1169,51 +1254,43 @@ mod dma { type DM = T::DM; type M = T::M; - fn spi(&self) -> &Self::T { - (**self).spi() - } - fn spi_mut(&mut self) -> &mut Self::T { - (**self).spi_mut() - } - fn channel(&self) -> &Channel<'d, Self::CH, Self::M> { - (**self).channel() - } - fn channel_mut(&mut self) -> &mut Channel<'d, Self::CH, Self::M> { - (**self).channel_mut() - } - - fn start_write_bytes_dma( - &mut self, - buffer: &mut TX, - full_duplex: bool, - ) -> Result<(), Error> { - (**self).start_write_bytes_dma(buffer, full_duplex) - } - - fn start_read_bytes_dma( - &mut self, - buffer: &mut RX, - full_duplex: bool, - ) -> Result<(), Error> { - (**self).start_read_bytes_dma(buffer, full_duplex) - } - - fn start_transfer_dma( - &mut self, - rx_buffer: &mut RX, - tx_buffer: &mut TX, - ) -> Result<(), Error> { - (**self).start_transfer_dma(rx_buffer, tx_buffer) - } - - #[cfg(all(esp32, spi_address_workaround))] - fn set_up_address_workaround( - &mut self, - cmd: Command, - address: Address, - dummy: u8, - ) -> Result<(), Error> { - (**self).set_up_address_workaround(cmd, address, dummy) + delegate::delegate! { + to (**self) { + fn spi(&self) -> &Self::T; + fn spi_mut(&mut self) -> &mut Self::T; + fn channel(&self) -> &Channel<'d, Self::CH, Self::M>; + fn channel_mut(&mut self) -> &mut Channel<'d, Self::CH, Self::M>; + + fn is_rx_in_progress(&self) -> bool; + fn is_tx_in_progress(&self) -> bool; + fn set_rx_in_progress(&mut self, in_progress: bool); + fn set_tx_in_progress(&mut self, in_progress: bool); + + fn is_done(&self) -> bool; + + fn start_write_bytes_dma( + &mut self, + buffer: &mut TX, + full_duplex: bool, + ) -> Result<(), Error>; + fn start_read_bytes_dma( + &mut self, + buffer: &mut RX, + full_duplex: bool, + ) -> Result<(), Error>; + fn start_transfer_dma( + &mut self, + rx_buffer: &mut RX, + tx_buffer: &mut TX, + ) -> Result<(), Error>; + #[cfg(all(esp32, spi_address_workaround))] + fn set_up_address_workaround( + &mut self, + cmd: Command, + address: Address, + dummy: u8, + ) -> Result<(), Error>; + } } } @@ -1243,6 +1320,44 @@ mod dma { &mut self.channel } + fn is_rx_in_progress(&self) -> bool { + self.rx_transfer_in_progress + } + + fn is_tx_in_progress(&self) -> bool { + self.tx_transfer_in_progress + } + + fn set_rx_in_progress(&mut self, in_progress: bool) { + self.rx_transfer_in_progress = in_progress; + } + + fn set_tx_in_progress(&mut self, in_progress: bool) { + self.tx_transfer_in_progress = in_progress; + } + + fn is_done(&self) -> bool { + if self.tx_transfer_in_progress && !self.channel().tx.is_done() { + return false; + } + if self.spi().busy() { + return false; + } + if self.rx_transfer_in_progress { + // If this is an asymmetric transfer and the RX side is smaller, the RX channel + // will never be "done" as it won't have enough descriptors/buffer to receive + // the EOF bit from the SPI. So instead the RX channel will hit + // a "descriptor empty" which means the DMA is written as much + // of the received data as possible into the buffer and + // discarded the rest. The user doesn't care about this discarded data. + + if !self.channel().rx.is_done() && !self.channel().rx.has_dscr_empty_error() { + return false; + } + } + true + } + fn start_write_bytes_dma( &mut self, buffer: &mut TX, @@ -1330,6 +1445,8 @@ mod dma { { pub(crate) spi: PeripheralRef<'d, T>, pub(crate) channel: Channel<'d, C, M>, + tx_transfer_in_progress: bool, + rx_transfer_in_progress: bool, #[cfg(all(esp32, spi_address_workaround))] address_buffer: DmaTxBuf, _mode: PhantomData, @@ -1383,6 +1500,8 @@ mod dma { Self { spi, channel, + tx_transfer_in_progress: false, + rx_transfer_in_progress: false, address_buffer: unwrap!(DmaTxBuf::new( unsafe { &mut DESCRIPTORS[id] }, crate::as_mut_byte_array!(BUFFERS[id], 4) @@ -1393,6 +1512,8 @@ mod dma { Self { spi, channel, + tx_transfer_in_progress: false, + rx_transfer_in_progress: false, _mode: PhantomData, } } @@ -1434,14 +1555,6 @@ mod dma { self.wait_for_idle(); self.spi.clear_interrupts(interrupts); } - - fn wait_for_idle(&self) { - // TODO: don't ignore DMA status. We can move the transfer's logic here. - while self.spi.busy() { - // Wait for the SPI to become idle - } - fence(Ordering::Acquire); - } } impl<'d, T, C, D, M> crate::private::Sealed for SpiDma<'d, T, C, D, M> @@ -1480,16 +1593,7 @@ mod dma { pub fn change_bus_frequency(&mut self, frequency: HertzU32) { self.spi.ch_bus_freq(frequency); } - } - impl<'d, T, C, D, M> SpiDma<'d, T, C, D, M> - where - T: InstanceDma, - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, - M: Mode, - { /// Configures the DMA buffers for the SPI instance. /// /// This method sets up both RX and TX buffers for DMA transfers. @@ -1514,11 +1618,6 @@ mod dma { { spi_dma: ManuallyDrop, dma_buf: ManuallyDrop, - is_rx: bool, - is_tx: bool, - - rx_future_awaited: bool, - tx_future_awaited: bool, _marker: PhantomData<&'d ()>, } @@ -1527,53 +1626,16 @@ mod dma { where R: SpiBusRef<'d>, { - fn new(spi_dma: R, dma_buf: Buf, is_rx: bool, is_tx: bool) -> Self { + fn new(mut spi_dma: R, dma_buf: Buf, is_rx: bool, is_tx: bool) -> Self { + spi_dma.set_rx_in_progress(is_rx); + spi_dma.set_tx_in_progress(is_tx); Self { spi_dma: ManuallyDrop::new(spi_dma), dma_buf: ManuallyDrop::new(dma_buf), - is_rx, - is_tx, - rx_future_awaited: false, - tx_future_awaited: false, _marker: PhantomData, } } - /// Checks if the DMA transfer is complete. - /// - /// This method returns `true` if both RX and TX operations are done, - /// and the SPI instance is no longer busy. - pub fn is_done(&self) -> bool { - if self.is_tx && !self.tx_future_awaited && !self.spi_dma.channel().tx.is_done() { - return false; - } - if self.spi_dma.spi().busy() { - return false; - } - if self.is_rx && !self.rx_future_awaited { - // If this is an asymmetric transfer and the RX side is smaller, the RX channel - // will never be "done" as it won't have enough descriptors/buffer to receive - // the EOF bit from the SPI. So instead the RX channel will hit - // a "descriptor empty" which means the DMA is written as much - // of the received data as possible into the buffer and - // discarded the rest. The user doesn't care about this discarded data. - - if !self.spi_dma.channel().rx.is_done() - && !self.spi_dma.channel().rx.has_dscr_empty_error() - { - return false; - } - } - true - } - - fn do_wait(&self) { - while !self.is_done() { - // Wait for the transfer to complete - } - fence(Ordering::Acquire); - } - fn do_cancel(&mut self) { cfg_if::cfg_if! { if #[cfg(esp32)] { @@ -1585,13 +1647,13 @@ mod dma { }; self.spi_dma.spi_mut().update(); - if self.is_tx { + if self.spi_dma.is_tx_in_progress() { self.spi_dma.channel_mut().tx.stop_transfer(); - self.is_tx = false; + self.spi_dma.set_tx_in_progress(false); } - if self.is_rx { + if self.spi_dma.is_rx_in_progress() { self.spi_dma.channel_mut().rx.stop_transfer(); - self.is_rx = false; + self.spi_dma.set_rx_in_progress(false); } } @@ -1600,7 +1662,7 @@ mod dma { /// This method blocks until the transfer is finished and returns the /// `SpiDma` instance and the associated buffer. pub fn wait(mut self) -> (R, Buf) { - self.do_wait(); + self.spi_dma.wait_for_idle(); unsafe { ( ManuallyDrop::take(&mut self.spi_dma), @@ -1611,7 +1673,7 @@ mod dma { /// Cancels the DMA transfer. pub fn cancel(&mut self) { - if !self.is_done() { + if !self.spi_dma.is_done() { self.do_cancel(); } } @@ -1622,9 +1684,9 @@ mod dma { R: SpiBusRef<'d>, { fn drop(&mut self) { - if !self.is_done() { + if !self.spi_dma.is_done() { self.do_cancel(); - self.do_wait(); + self.spi_dma.wait_for_idle() } } } @@ -1637,28 +1699,7 @@ mod dma { /// /// This method awaits the completion of both RX and TX operations. pub async fn wait_for_done(&mut self) { - if self.is_tx && !self.tx_future_awaited { - let _ = DmaTxFuture::new(&mut self.spi_dma.channel_mut().tx).await; - self.tx_future_awaited = true; - } - - // As a future enhancement, setup Spi Future in here as well. - - if self.is_rx && !self.rx_future_awaited { - let _ = DmaRxFuture::new(&mut self.spi_dma.channel_mut().rx).await; - self.rx_future_awaited = true; - } - - core::future::poll_fn(|cx| { - use core::task::Poll; - if self.is_done() { - Poll::Ready(()) - } else { - cx.waker().wake_by_ref(); - Poll::Pending - } - }) - .await; + self.spi_dma.wait_for_idle_async().await; } } @@ -2068,22 +2109,9 @@ mod dma { C: DmaChannel, C::P: SpiPeripheral, { - async fn wait_for_idle_async(&mut self) { - core::future::poll_fn(|cx| { - use core::task::Poll; - if !self.spi_dma.spi().busy() { - Poll::Ready(()) - } else { - cx.waker().wake_by_ref(); - Poll::Pending - } - }) - .await; - } - /// Fill the given buffer with data from the bus. pub async fn read_async(&mut self, words: &mut [u8]) -> Result<(), Error> { - self.wait_for_idle_async().await; + self.spi_dma.wait_for_idle_async().await; let chunk_size = self.rx_buf.capacity(); for chunk in words.chunks_mut(chunk_size) { @@ -2104,7 +2132,7 @@ mod dma { /// Transmit the given buffer to the bus. pub async fn write_async(&mut self, words: &[u8]) -> Result<(), Error> { - self.wait_for_idle_async().await; + self.spi_dma.wait_for_idle_async().await; let chunk_size = self.tx_buf.capacity(); for chunk in words.chunks(chunk_size) { @@ -2127,7 +2155,7 @@ mod dma { read: &mut [u8], write: &[u8], ) -> Result<(), Error> { - self.wait_for_idle_async().await; + self.spi_dma.wait_for_idle_async().await; let chunk_size = min(self.tx_buf.capacity(), self.rx_buf.capacity()); let common_length = min(read.len(), write.len()); @@ -2164,7 +2192,7 @@ mod dma { /// Transfer by writing out a buffer and reading the response from /// the bus into the same buffer. pub async fn transfer_in_place_async(&mut self, words: &mut [u8]) -> Result<(), Error> { - self.wait_for_idle_async().await; + self.spi_dma.wait_for_idle_async().await; for chunk in words.chunks_mut(self.tx_buf.capacity()) { self.tx_buf.fill(chunk); self.rx_buf.set_length(chunk.len()); diff --git a/hil-test/tests/spi_full_duplex.rs b/hil-test/tests/spi_full_duplex.rs index 04bfd9c1335..9dd25690f93 100644 --- a/hil-test/tests/spi_full_duplex.rs +++ b/hil-test/tests/spi_full_duplex.rs @@ -546,4 +546,28 @@ mod tests { panic!("Failed to transmit after cancel"); } } + + #[test] + #[timeout(3)] + async fn cancelling_an_awaited_transfer_does_nothing(ctx: Context) { + // Set up a large buffer that would trigger a timeout + let dma_rx_buf = DmaRxBuf::new(ctx.rx_descriptors, ctx.rx_buffer).unwrap(); + let dma_tx_buf = DmaTxBuf::new(ctx.tx_descriptors, ctx.tx_buffer).unwrap(); + + let spi = ctx + .spi + .with_dma(ctx.dma_channel.configure_for_async(false, DmaPriority::Priority0)); + + let mut transfer = spi + .dma_transfer(dma_rx_buf, dma_tx_buf) + .map_err(|e| e.0) + .unwrap(); + + transfer.wait_for_done().await; + transfer.cancel(); + + transfer.wait_for_done().await; + transfer.cancel(); + _ = transfer.wait(); + } } From 525b592a1e172ae431b18525821b9160d6813c30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 24 Sep 2024 21:49:07 +0200 Subject: [PATCH 10/19] Allow the current byte to complete --- esp-hal/src/spi/master.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index b16737298be..fdeaa60999d 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1637,14 +1637,11 @@ mod dma { } fn do_cancel(&mut self) { - cfg_if::cfg_if! { - if #[cfg(esp32)] { - // TODO: examine what happens exactly. (0, 0) just doesn't take effect. - self.spi_dma.spi_mut().configure_datalen(1, 1); - } else { - self.spi_dma.spi_mut().configure_datalen(0, 0); - } - }; + // 0 doesn't take effect on ESP32 and cuts the currently transmitted byte + // immediately. + // 1 seems to stop after transmitting the current byte which is somewhat less + // impolite. + self.spi_dma.spi_mut().configure_datalen(1, 1); self.spi_dma.spi_mut().update(); if self.spi_dma.is_tx_in_progress() { From 612fb2a9e5ab65296a05e350ee1afc1576b4a577 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 24 Sep 2024 21:51:53 +0200 Subject: [PATCH 11/19] Restore SpiDmaTransfer::is_done --- esp-hal/src/spi/master.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index fdeaa60999d..304c871cb4b 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1654,6 +1654,14 @@ mod dma { } } + /// Checks if the transfer is complete. + /// + /// This method returns `true` if both RX and TX operations are done, + /// and the SPI instance is no longer busy. + pub fn is_done(&self) -> bool { + self.spi_dma.is_done() + } + /// Waits for the DMA transfer to complete. /// /// This method blocks until the transfer is finished and returns the From 97b723871be3eb287ff70ef87c297efa52f9ecfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 24 Sep 2024 22:01:06 +0200 Subject: [PATCH 12/19] Explain cancel code --- esp-hal/src/spi/master.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 304c871cb4b..98e3d32b6d9 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1637,6 +1637,8 @@ mod dma { } fn do_cancel(&mut self) { + // The SPI peripheral is controlling how much data we transfer, so let's + // update its counter. // 0 doesn't take effect on ESP32 and cuts the currently transmitted byte // immediately. // 1 seems to stop after transmitting the current byte which is somewhat less @@ -1644,6 +1646,7 @@ mod dma { self.spi_dma.spi_mut().configure_datalen(1, 1); self.spi_dma.spi_mut().update(); + // We need to stop the DMA transfer, too. if self.spi_dma.is_tx_in_progress() { self.spi_dma.channel_mut().tx.stop_transfer(); self.spi_dma.set_tx_in_progress(false); From f1da4578212811fbd4874f25b8a96667b7e09fe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 24 Sep 2024 23:55:17 +0200 Subject: [PATCH 13/19] Fix test formatting --- hil-test/tests/spi_full_duplex.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hil-test/tests/spi_full_duplex.rs b/hil-test/tests/spi_full_duplex.rs index 9dd25690f93..5df30fdc5b7 100644 --- a/hil-test/tests/spi_full_duplex.rs +++ b/hil-test/tests/spi_full_duplex.rs @@ -554,9 +554,10 @@ mod tests { let dma_rx_buf = DmaRxBuf::new(ctx.rx_descriptors, ctx.rx_buffer).unwrap(); let dma_tx_buf = DmaTxBuf::new(ctx.tx_descriptors, ctx.tx_buffer).unwrap(); - let spi = ctx - .spi - .with_dma(ctx.dma_channel.configure_for_async(false, DmaPriority::Priority0)); + let spi = ctx.spi.with_dma( + ctx.dma_channel + .configure_for_async(false, DmaPriority::Priority0), + ); let mut transfer = spi .dma_transfer(dma_rx_buf, dma_tx_buf) From 877946a99038d86e91a379f4dac98c7ebbee4db7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Wed, 25 Sep 2024 14:28:58 +0200 Subject: [PATCH 14/19] Changelog --- esp-hal/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 7ee64cd910b..9ca3a1f918f 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -62,6 +62,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `EspTwaiFrame` constructors now accept any type that converts into `esp_hal::twai::Id` (#2207) - Change `DmaTxBuf` to support PSRAM on `esp32s3` (#2161) - I2c `transaction` is now also available as a inherent function, lift size limit on `write`,`read` and `write_read` (#2262) +- SPI transactions are now cancelled if the transfer object (or async Future) is dropped. (#2216) ### Fixed From 01b966e80235a933677bae1ae0ae6cafaec3143a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 27 Sep 2024 10:49:11 +0200 Subject: [PATCH 15/19] Simplify implementation --- esp-hal/src/spi/master.rs | 1100 +++++++++++++++++-------------------- 1 file changed, 516 insertions(+), 584 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 98e3d32b6d9..46b05a148bd 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -919,7 +919,6 @@ mod dma { dma::{ asynch::{DmaRxFuture, DmaTxFuture}, Channel, - DmaBufferRef, DmaChannel, DmaRxBuf, DmaRxBuffer, @@ -979,29 +978,150 @@ mod dma { } } - #[doc(hidden)] - pub trait SpiBusRef<'d>: Sized { - type T: InstanceDma; - type CH: DmaChannel; - type DM: DuplexMode; - type M: Mode; + /// A DMA capable SPI instance. + /// + /// Using `SpiDma` is not recommended unless you wish + /// to manage buffers yourself. It's recommended to use + /// [`SpiDmaBus`] via `with_buffers` to get access + /// to a DMA capable SPI bus that implements the + /// embedded-hal traits. + pub struct SpiDma<'d, T, C, D, M> + where + C: DmaChannel, + C::P: SpiPeripheral, + D: DuplexMode, + M: Mode, + { + pub(crate) spi: PeripheralRef<'d, T>, + pub(crate) channel: Channel<'d, C, M>, + tx_transfer_in_progress: bool, + rx_transfer_in_progress: bool, + #[cfg(all(esp32, spi_address_workaround))] + address_buffer: DmaTxBuf, + _mode: PhantomData, + } + + #[cfg(all(esp32, spi_address_workaround))] + unsafe impl<'d, T, C, D, M> Send for SpiDma<'d, T, C, D, M> + where + C: DmaChannel, + C::P: SpiPeripheral, + D: DuplexMode, + M: Mode, + { + } + + impl<'d, T, C, D, M> core::fmt::Debug for SpiDma<'d, T, C, D, M> + where + C: DmaChannel, + C::P: SpiPeripheral, + D: DuplexMode, + M: Mode, + { + /// Formats the `SpiDma` instance for debugging purposes. + /// + /// This method returns a debug struct with the name "SpiDma" without + /// exposing internal details. + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.debug_struct("SpiDma").finish() + } + } - fn spi(&self) -> &Self::T; - fn spi_mut(&mut self) -> &mut Self::T; - fn channel(&self) -> &Channel<'d, Self::CH, Self::M>; - fn channel_mut(&mut self) -> &mut Channel<'d, Self::CH, Self::M>; + impl<'d, T, C, D, M> SpiDma<'d, T, C, D, M> + where + T: InstanceDma, + C: DmaChannel, + C::P: SpiPeripheral, + D: DuplexMode, + M: Mode, + { + fn new(spi: PeripheralRef<'d, T>, channel: Channel<'d, C, M>) -> Self { + #[cfg(all(esp32, spi_address_workaround))] + let address_buffer = { + use crate::dma::DmaDescriptor; + const SPI_NUM: usize = 2; + static mut DESCRIPTORS: [[DmaDescriptor; 1]; SPI_NUM] = + [[DmaDescriptor::EMPTY]; SPI_NUM]; + static mut BUFFERS: [[u32; 1]; SPI_NUM] = [[0; 1]; SPI_NUM]; + + let id = spi.spi_num() as usize - 2; + + unwrap!(DmaTxBuf::new( + unsafe { &mut DESCRIPTORS[id] }, + crate::as_mut_byte_array!(BUFFERS[id], 4) + )) + }; + + Self { + spi, + channel, + #[cfg(all(esp32, spi_address_workaround))] + address_buffer, + tx_transfer_in_progress: false, + rx_transfer_in_progress: false, + _mode: PhantomData, + } + } + + /// Sets the interrupt handler + /// + /// Interrupts are not enabled at the peripheral level here. + pub fn set_interrupt_handler(&mut self, handler: InterruptHandler) { + self.spi.set_interrupt_handler(handler); + } + + /// Listen for the given interrupts + #[cfg(gdma)] + pub fn listen(&mut self, interrupts: EnumSet) { + self.spi.listen(interrupts); + } + + /// Unlisten the given interrupts + #[cfg(gdma)] + pub fn unlisten(&mut self, interrupts: EnumSet) { + self.spi.unlisten(interrupts); + } - fn is_rx_in_progress(&self) -> bool; - fn is_tx_in_progress(&self) -> bool; - fn set_rx_in_progress(&mut self, in_progress: bool); - fn set_tx_in_progress(&mut self, in_progress: bool); + /// Gets asserted interrupts + #[cfg(gdma)] + pub fn interrupts(&mut self) -> EnumSet { + self.spi.interrupts() + } - fn is_done(&self) -> bool; + /// Resets asserted interrupts + #[cfg(gdma)] + pub fn clear_interrupts(&mut self, interrupts: EnumSet) { + self.spi.clear_interrupts(interrupts); + } - fn wait_for_idle(&self) { + fn is_done(&self) -> bool { + if self.tx_transfer_in_progress && !self.channel.tx.is_done() { + return false; + } + if self.spi.busy() { + return false; + } + if self.rx_transfer_in_progress { + // If this is an asymmetric transfer and the RX side is smaller, the RX channel + // will never be "done" as it won't have enough descriptors/buffer to receive + // the EOF bit from the SPI. So instead the RX channel will hit + // a "descriptor empty" which means the DMA is written as much + // of the received data as possible into the buffer and + // discarded the rest. The user doesn't care about this discarded data. + + if !self.channel.rx.is_done() && !self.channel.rx.has_dscr_empty_error() { + return false; + } + } + true + } + + fn wait_for_idle(&mut self) { while !self.is_done() { // Wait for the SPI to become idle } + self.rx_transfer_in_progress = false; + self.tx_transfer_in_progress = false; fence(Ordering::Acquire); } @@ -1044,25 +1164,21 @@ mod dma { } } - let is_rx = self.is_rx_in_progress(); - let is_tx = self.is_tx_in_progress(); - let channels = self.channel_mut(); - - let rx_future = if is_rx { - OptionalFuture::some(DmaRxFuture::new(&mut channels.rx)) + let rx_future = if self.rx_transfer_in_progress { + OptionalFuture::some(DmaRxFuture::new(&mut self.channel.rx)) } else { OptionalFuture::none(Ok(())) }; - let tx_future = if is_tx { - OptionalFuture::some(DmaTxFuture::new(&mut channels.tx)) + let tx_future = if self.tx_transfer_in_progress { + OptionalFuture::some(DmaTxFuture::new(&mut self.channel.tx)) } else { OptionalFuture::none(Ok(())) }; _ = embassy_futures::join::join(rx_future, tx_future).await; - self.set_rx_in_progress(false); - self.set_tx_in_progress(false); + self.rx_transfer_in_progress = false; + self.tx_transfer_in_progress = false; core::future::poll_fn(|cx| { use core::task::Poll; @@ -1076,315 +1192,52 @@ mod dma { .await; } - fn start_write_bytes_dma( - &mut self, - buffer: &mut TX, - full_duplex: bool, - ) -> Result<(), Error>; - - fn start_read_bytes_dma( - &mut self, - buffer: &mut RX, - full_duplex: bool, - ) -> Result<(), Error>; - - fn start_transfer_dma( - &mut self, - rx_buffer: &mut RX, - tx_buffer: &mut TX, - ) -> Result<(), Error>; - - #[allow(clippy::type_complexity)] - #[cfg_attr(place_spi_driver_in_ram, ram)] - fn do_dma_write( - mut self, - mut buffer: TX, - ) -> Result, (Error, Self, TX)> { - let bytes_to_write = buffer.length(); - if bytes_to_write > MAX_DMA_SIZE { - return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); - } - - if let Err(e) = self.start_write_bytes_dma(&mut buffer, true) { - return Err((e, self, buffer)); - } - - Ok(SpiDmaTransfer::new(self, buffer, false, true)) - } - - #[allow(clippy::type_complexity)] + /// # Safety: + /// + /// The caller must ensure to not access the buffer contents while the + /// transfer is in progress. Moving the buffer itself is allowed. #[cfg_attr(place_spi_driver_in_ram, ram)] - fn do_dma_read( - mut self, - mut buffer: RX, - ) -> Result, (Error, Self, RX)> { - let bytes_to_read = buffer.length(); - if bytes_to_read > MAX_DMA_SIZE { - return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); - } - - if let Err(e) = self.start_read_bytes_dma(&mut buffer, true) { - return Err((e, self, buffer)); - } - - Ok(SpiDmaTransfer::new(self, buffer, true, false)) - } - - #[allow(clippy::type_complexity)] - fn do_dma_transfer( - mut self, - mut rx_buffer: RX, - mut tx_buffer: TX, - ) -> Result, (Error, Self, RX, TX)> { - let bytes_to_read = rx_buffer.length(); - let bytes_to_write = tx_buffer.length(); - - if bytes_to_write > MAX_DMA_SIZE || bytes_to_read > MAX_DMA_SIZE { - return Err(( - Error::MaxDmaTransferSizeExceeded, - self, - rx_buffer, - tx_buffer, - )); - } - - if let Err(e) = self.start_transfer_dma(&mut rx_buffer, &mut tx_buffer) { - return Err((e, self, rx_buffer, tx_buffer)); - } - - Ok(SpiDmaTransfer::new( - self, - (rx_buffer, tx_buffer), - true, - true, - )) - } - - fn do_half_duplex_read( - mut self, - data_mode: SpiDataMode, - cmd: Command, - address: Address, - dummy: u8, - mut buffer: RX, - ) -> Result, (Error, Self, RX)> - where - Self: SpiBusRef<'d, DM = HalfDuplexMode>, - { - let bytes_to_read = buffer.length(); - if bytes_to_read > MAX_DMA_SIZE { - return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); - } - - self.spi_mut().setup_half_duplex( - false, - cmd, - address, - false, - dummy, - bytes_to_read == 0, - data_mode, - ); - - if let Err(e) = self.start_read_bytes_dma(&mut buffer, false) { - return Err((e, self, buffer)); - } - - Ok(SpiDmaTransfer::new(self, buffer, bytes_to_read > 0, false)) - } - - fn do_half_duplex_write( - mut self, - data_mode: SpiDataMode, - cmd: Command, - address: Address, - dummy: u8, - mut buffer: TX, - ) -> Result, (Error, Self, TX)> - where - Self: SpiBusRef<'d, DM = HalfDuplexMode>, - { - let bytes_to_write = buffer.length(); - if bytes_to_write > MAX_DMA_SIZE { - return Err((Error::MaxDmaTransferSizeExceeded, self, buffer)); - } - - #[cfg(all(esp32, spi_address_workaround))] - { - // On the ESP32, if we don't have data, the address is always sent - // on a single line, regardless of its data mode. - if bytes_to_write == 0 && address.mode() != SpiDataMode::Single { - if let Err(e) = self.set_up_address_workaround(cmd, address, dummy) { - return Err((e, self, buffer)); - } - - return Ok(SpiDmaTransfer::new(self, buffer, false, bytes_to_write > 0)); - } - } - - self.spi_mut().setup_half_duplex( - true, - cmd, - address, - false, - dummy, - bytes_to_write == 0, - data_mode, - ); - - if let Err(e) = self.start_write_bytes_dma(&mut buffer, false) { - return Err((e, self, buffer)); - } - - Ok(SpiDmaTransfer::new(self, buffer, false, bytes_to_write > 0)) - } - - #[cfg(all(esp32, spi_address_workaround))] - fn set_up_address_workaround( - &mut self, - cmd: Command, - address: Address, - dummy: u8, - ) -> Result<(), Error>; - } - - impl<'d, T: SpiBusRef<'d>> SpiBusRef<'d> for &mut T { - type T = T::T; - type CH = T::CH; - type DM = T::DM; - type M = T::M; - - delegate::delegate! { - to (**self) { - fn spi(&self) -> &Self::T; - fn spi_mut(&mut self) -> &mut Self::T; - fn channel(&self) -> &Channel<'d, Self::CH, Self::M>; - fn channel_mut(&mut self) -> &mut Channel<'d, Self::CH, Self::M>; - - fn is_rx_in_progress(&self) -> bool; - fn is_tx_in_progress(&self) -> bool; - fn set_rx_in_progress(&mut self, in_progress: bool); - fn set_tx_in_progress(&mut self, in_progress: bool); - - fn is_done(&self) -> bool; - - fn start_write_bytes_dma( - &mut self, - buffer: &mut TX, - full_duplex: bool, - ) -> Result<(), Error>; - fn start_read_bytes_dma( - &mut self, - buffer: &mut RX, - full_duplex: bool, - ) -> Result<(), Error>; - fn start_transfer_dma( - &mut self, - rx_buffer: &mut RX, - tx_buffer: &mut TX, - ) -> Result<(), Error>; - #[cfg(all(esp32, spi_address_workaround))] - fn set_up_address_workaround( - &mut self, - cmd: Command, - address: Address, - dummy: u8, - ) -> Result<(), Error>; - } - } - } - - impl<'d, T, C, D, M> SpiBusRef<'d> for SpiDma<'d, T, C, D, M> - where - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, - M: Mode, - T: InstanceDma, - { - type T = T; - type CH = C; - type DM = D; - type M = M; - - fn spi(&self) -> &Self::T { - &self.spi - } - fn spi_mut(&mut self) -> &mut Self::T { - &mut self.spi - } - fn channel(&self) -> &Channel<'d, C, M> { - &self.channel - } - fn channel_mut(&mut self) -> &mut Channel<'d, C, M> { - &mut self.channel - } - - fn is_rx_in_progress(&self) -> bool { - self.rx_transfer_in_progress - } - - fn is_tx_in_progress(&self) -> bool { - self.tx_transfer_in_progress - } - - fn set_rx_in_progress(&mut self, in_progress: bool) { - self.rx_transfer_in_progress = in_progress; - } - - fn set_tx_in_progress(&mut self, in_progress: bool) { - self.tx_transfer_in_progress = in_progress; - } - - fn is_done(&self) -> bool { - if self.tx_transfer_in_progress && !self.channel().tx.is_done() { - return false; - } - if self.spi().busy() { - return false; - } - if self.rx_transfer_in_progress { - // If this is an asymmetric transfer and the RX side is smaller, the RX channel - // will never be "done" as it won't have enough descriptors/buffer to receive - // the EOF bit from the SPI. So instead the RX channel will hit - // a "descriptor empty" which means the DMA is written as much - // of the received data as possible into the buffer and - // discarded the rest. The user doesn't care about this discarded data. - - if !self.channel().rx.is_done() && !self.channel().rx.has_dscr_empty_error() { - return false; - } - } - true - } - - fn start_write_bytes_dma( + unsafe fn start_write_bytes_dma( &mut self, buffer: &mut TX, full_duplex: bool, ) -> Result<(), Error> { + self.tx_transfer_in_progress = buffer.length() > 0; unsafe { self.spi .start_write_bytes_dma(buffer, &mut self.channel.tx, full_duplex) } } - fn start_read_bytes_dma( + /// # Safety: + /// + /// The caller must ensure to not access the buffer contents while the + /// transfer is in progress. Moving the buffer itself is allowed. + #[cfg_attr(place_spi_driver_in_ram, ram)] + unsafe fn start_read_bytes_dma( &mut self, buffer: &mut RX, full_duplex: bool, ) -> Result<(), Error> { + self.rx_transfer_in_progress = buffer.length() > 0; unsafe { self.spi .start_read_bytes_dma(buffer, &mut self.channel.rx, full_duplex) } } - fn start_transfer_dma( + /// # Safety: + /// + /// The caller must ensure to not access the buffer contents while the + /// transfer is in progress. Moving the buffer itself is allowed. + #[cfg_attr(place_spi_driver_in_ram, ram)] + unsafe fn start_transfer_dma( &mut self, rx_buffer: &mut RX, tx_buffer: &mut TX, ) -> Result<(), Error> { + self.rx_transfer_in_progress = rx_buffer.length() > 0; + self.tx_transfer_in_progress = tx_buffer.length() > 0; unsafe { self.spi.start_transfer_dma( rx_buffer, @@ -1395,165 +1248,61 @@ mod dma { } } - #[cfg(all(esp32, spi_address_workaround))] - fn set_up_address_workaround( - &mut self, - cmd: Command, - address: Address, - dummy: u8, - ) -> Result<(), Error> { - let bytes_to_write = address.width().div_ceil(8); - // The address register is read in big-endian order, - // we have to prepare the emulated write in the same way. - let addr_bytes = address.value().to_be_bytes(); - let addr_bytes = &addr_bytes[4 - bytes_to_write..][..bytes_to_write]; - self.address_buffer.fill(addr_bytes); - - self.spi.setup_half_duplex( - true, - cmd, - Address::None, - false, - dummy, - bytes_to_write == 0, - address.mode(), - ); - - unsafe { - self.spi.start_write_bytes_dma( - &mut self.address_buffer, - &mut self.channel.tx, - false, - ) - } - } - } - - /// A DMA capable SPI instance. - /// - /// Using `SpiDma` is not recommended unless you wish - /// to manage buffers yourself. It's recommended to use - /// [`SpiDmaBus`] via `with_buffers` to get access - /// to a DMA capable SPI bus that implements the - /// embedded-hal traits. - pub struct SpiDma<'d, T, C, D, M> - where - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, - M: Mode, - { - pub(crate) spi: PeripheralRef<'d, T>, - pub(crate) channel: Channel<'d, C, M>, - tx_transfer_in_progress: bool, - rx_transfer_in_progress: bool, - #[cfg(all(esp32, spi_address_workaround))] - address_buffer: DmaTxBuf, - _mode: PhantomData, - } - - #[cfg(all(esp32, spi_address_workaround))] - unsafe impl<'d, T, C, D, M> Send for SpiDma<'d, T, C, D, M> - where - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, - M: Mode, - { - } - - impl<'d, T, C, D, M> core::fmt::Debug for SpiDma<'d, T, C, D, M> - where - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, - M: Mode, - { - /// Formats the `SpiDma` instance for debugging purposes. - /// - /// This method returns a debug struct with the name "SpiDma" without - /// exposing internal details. - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - f.debug_struct("SpiDma").finish() - } - } - - impl<'d, T, C, D, M> SpiDma<'d, T, C, D, M> - where - T: InstanceDma, - C: DmaChannel, - C::P: SpiPeripheral, - D: DuplexMode, - M: Mode, - { - fn new(spi: PeripheralRef<'d, T>, channel: Channel<'d, C, M>) -> Self { - cfg_if::cfg_if! { - if #[cfg(all(esp32, spi_address_workaround))] { - use crate::dma::DmaDescriptor; - const SPI_NUM: usize = 2; - static mut DESCRIPTORS: [[DmaDescriptor; 1]; SPI_NUM] = - [[DmaDescriptor::EMPTY]; SPI_NUM]; - static mut BUFFERS: [[u32; 1]; SPI_NUM] = [[0; 1]; SPI_NUM]; - - let id = spi.spi_num() as usize - 2; - - Self { - spi, - channel, - tx_transfer_in_progress: false, - rx_transfer_in_progress: false, - address_buffer: unwrap!(DmaTxBuf::new( - unsafe { &mut DESCRIPTORS[id] }, - crate::as_mut_byte_array!(BUFFERS[id], 4) - )), - _mode: PhantomData, - } - } else { - Self { - spi, - channel, - tx_transfer_in_progress: false, - rx_transfer_in_progress: false, - _mode: PhantomData, - } - } - } - } - - /// Sets the interrupt handler - /// - /// Interrupts are not enabled at the peripheral level here. - pub fn set_interrupt_handler(&mut self, handler: InterruptHandler) { - self.wait_for_idle(); - self.spi.set_interrupt_handler(handler); - } - - /// Listen for the given interrupts - #[cfg(gdma)] - pub fn listen(&mut self, interrupts: EnumSet) { - self.wait_for_idle(); - self.spi.listen(interrupts); - } + #[cfg(all(esp32, spi_address_workaround))] + fn set_up_address_workaround( + &mut self, + cmd: Command, + address: Address, + dummy: u8, + ) -> Result<(), Error> { + let bytes_to_write = address.width().div_ceil(8); + // The address register is read in big-endian order, + // we have to prepare the emulated write in the same way. + let addr_bytes = address.value().to_be_bytes(); + let addr_bytes = &addr_bytes[4 - bytes_to_write..][..bytes_to_write]; + self.address_buffer.fill(addr_bytes); - /// Unlisten the given interrupts - #[cfg(gdma)] - pub fn unlisten(&mut self, interrupts: EnumSet) { - self.wait_for_idle(); - self.spi.unlisten(interrupts); - } + self.spi.setup_half_duplex( + true, + cmd, + Address::None, + false, + dummy, + bytes_to_write == 0, + address.mode(), + ); - /// Gets asserted interrupts - #[cfg(gdma)] - pub fn interrupts(&mut self) -> EnumSet { - self.wait_for_idle(); - self.spi.interrupts() + self.tx_transfer_in_progress = true; + unsafe { + self.spi.start_write_bytes_dma( + &mut self.address_buffer, + &mut self.channel.tx, + false, + ) + } } - /// Resets asserted interrupts - #[cfg(gdma)] - pub fn clear_interrupts(&mut self, interrupts: EnumSet) { - self.wait_for_idle(); - self.spi.clear_interrupts(interrupts); + fn cancel_transfer(&mut self) { + // The SPI peripheral is controlling how much data we transfer, so let's + // update its counter. + // 0 doesn't take effect on ESP32 and cuts the currently transmitted byte + // immediately. + // 1 seems to stop after transmitting the current byte which is somewhat less + // impolite. + if self.tx_transfer_in_progress || self.rx_transfer_in_progress { + self.spi.configure_datalen(1, 1); + self.spi.update(); + + // We need to stop the DMA transfer, too. + if self.tx_transfer_in_progress { + self.channel.tx.stop_transfer(); + self.tx_transfer_in_progress = false; + } + if self.rx_transfer_in_progress { + self.channel.rx.stop_transfer(); + self.rx_transfer_in_progress = false; + } + } } } @@ -1612,23 +1361,29 @@ mod dma { /// /// This structure holds references to the SPI instance, DMA buffers, and /// transfer status. - pub struct SpiDmaTransfer<'d, R, Buf> + pub struct SpiDmaTransfer<'d, T, C, D, M, Buf> where - R: SpiBusRef<'d>, + T: InstanceDma, + C: DmaChannel, + C::P: SpiPeripheral, + D: DuplexMode, + M: Mode, { - spi_dma: ManuallyDrop, + spi_dma: ManuallyDrop>, dma_buf: ManuallyDrop, _marker: PhantomData<&'d ()>, } - impl<'d, R, Buf> SpiDmaTransfer<'d, R, Buf> + impl<'d, T, C, D, M, Buf> SpiDmaTransfer<'d, T, C, D, M, Buf> where - R: SpiBusRef<'d>, + T: InstanceDma, + C: DmaChannel, + C::P: SpiPeripheral, + D: DuplexMode, + M: Mode, { - fn new(mut spi_dma: R, dma_buf: Buf, is_rx: bool, is_tx: bool) -> Self { - spi_dma.set_rx_in_progress(is_rx); - spi_dma.set_tx_in_progress(is_tx); + fn new(spi_dma: SpiDma<'d, T, C, D, M>, dma_buf: Buf) -> Self { Self { spi_dma: ManuallyDrop::new(spi_dma), dma_buf: ManuallyDrop::new(dma_buf), @@ -1636,27 +1391,6 @@ mod dma { } } - fn do_cancel(&mut self) { - // The SPI peripheral is controlling how much data we transfer, so let's - // update its counter. - // 0 doesn't take effect on ESP32 and cuts the currently transmitted byte - // immediately. - // 1 seems to stop after transmitting the current byte which is somewhat less - // impolite. - self.spi_dma.spi_mut().configure_datalen(1, 1); - self.spi_dma.spi_mut().update(); - - // We need to stop the DMA transfer, too. - if self.spi_dma.is_tx_in_progress() { - self.spi_dma.channel_mut().tx.stop_transfer(); - self.spi_dma.set_tx_in_progress(false); - } - if self.spi_dma.is_rx_in_progress() { - self.spi_dma.channel_mut().rx.stop_transfer(); - self.spi_dma.set_rx_in_progress(false); - } - } - /// Checks if the transfer is complete. /// /// This method returns `true` if both RX and TX operations are done, @@ -1669,39 +1403,48 @@ mod dma { /// /// This method blocks until the transfer is finished and returns the /// `SpiDma` instance and the associated buffer. - pub fn wait(mut self) -> (R, Buf) { + pub fn wait(mut self) -> (SpiDma<'d, T, C, D, M>, Buf) { self.spi_dma.wait_for_idle(); - unsafe { + let retval = unsafe { ( ManuallyDrop::take(&mut self.spi_dma), ManuallyDrop::take(&mut self.dma_buf), ) - } + }; + core::mem::forget(self); + retval } /// Cancels the DMA transfer. pub fn cancel(&mut self) { if !self.spi_dma.is_done() { - self.do_cancel(); + self.spi_dma.cancel_transfer(); } } } - impl<'d, R, Buf> Drop for SpiDmaTransfer<'d, R, Buf> + impl<'d, T, C, D, M, Buf> Drop for SpiDmaTransfer<'d, T, C, D, M, Buf> where - R: SpiBusRef<'d>, + T: InstanceDma, + C: DmaChannel, + C::P: SpiPeripheral, + D: DuplexMode, + M: Mode, { fn drop(&mut self) { - if !self.spi_dma.is_done() { - self.do_cancel(); + if !self.is_done() { + self.spi_dma.cancel_transfer(); self.spi_dma.wait_for_idle() } } } - impl<'d, R, Buf> SpiDmaTransfer<'d, R, Buf> + impl<'d, T, C, D, Buf> SpiDmaTransfer<'d, T, C, D, crate::Async, Buf> where - R: SpiBusRef<'d, M = crate::Async>, + T: InstanceDma, + C: DmaChannel, + C::P: SpiPeripheral, + D: DuplexMode, { /// Waits for the DMA transfer to complete asynchronously. /// @@ -1718,6 +1461,20 @@ mod dma { C::P: SpiPeripheral, M: Mode, { + /// # Safety: + /// + /// The caller must ensure that the buffers are not accessed while the + /// transfer is in progress. Moving the buffers is allowed. + #[cfg_attr(place_spi_driver_in_ram, ram)] + unsafe fn start_dma_write(&mut self, buffer: &mut impl DmaTxBuffer) -> Result<(), Error> { + let bytes_to_write = buffer.length(); + if bytes_to_write > MAX_DMA_SIZE { + return Err(Error::MaxDmaTransferSizeExceeded); + } + + self.start_write_bytes_dma(buffer, true) + } + /// Perform a DMA write. /// /// This will return a [SpiDmaTransfer] owning the buffer and the @@ -1726,12 +1483,29 @@ mod dma { #[allow(clippy::type_complexity)] #[cfg_attr(place_spi_driver_in_ram, ram)] pub fn dma_write( - self, - buffer: TX, - ) -> Result, (Error, Self, TX)> { + mut self, + mut buffer: TX, + ) -> Result, (Error, Self, TX)> { self.wait_for_idle(); - self.do_dma_write(buffer) + match unsafe { self.start_dma_write(&mut buffer) } { + Ok(_) => Ok(SpiDmaTransfer::new(self, buffer)), + Err(e) => Err((e, self, buffer)), + } + } + + /// # Safety: + /// + /// The caller must ensure that the buffers are not accessed while the + /// transfer is in progress. Moving the buffers is allowed. + #[cfg_attr(place_spi_driver_in_ram, ram)] + unsafe fn start_dma_read(&mut self, buffer: &mut impl DmaRxBuffer) -> Result<(), Error> { + let bytes_to_read = buffer.length(); + if bytes_to_read > MAX_DMA_SIZE { + return Err(Error::MaxDmaTransferSizeExceeded); + } + + self.start_read_bytes_dma(buffer, true) } /// Perform a DMA read. @@ -1742,12 +1516,34 @@ mod dma { #[allow(clippy::type_complexity)] #[cfg_attr(place_spi_driver_in_ram, ram)] pub fn dma_read( - self, - buffer: RX, - ) -> Result, (Error, Self, RX)> { + mut self, + mut buffer: RX, + ) -> Result, (Error, Self, RX)> { self.wait_for_idle(); + match unsafe { self.start_dma_read(&mut buffer) } { + Ok(_) => Ok(SpiDmaTransfer::new(self, buffer)), + Err(e) => Err((e, self, buffer)), + } + } + + /// # Safety: + /// + /// The caller must ensure that the buffers are not accessed while the + /// transfer is in progress. Moving the buffers is allowed. + #[cfg_attr(place_spi_driver_in_ram, ram)] + unsafe fn start_dma_transfer( + &mut self, + rx_buffer: &mut impl DmaRxBuffer, + tx_buffer: &mut impl DmaTxBuffer, + ) -> Result<(), Error> { + let bytes_to_read = rx_buffer.length(); + let bytes_to_write = tx_buffer.length(); + + if bytes_to_write > MAX_DMA_SIZE || bytes_to_read > MAX_DMA_SIZE { + return Err(Error::MaxDmaTransferSizeExceeded); + } - self.do_dma_read(buffer) + self.start_transfer_dma(rx_buffer, tx_buffer) } /// Perform a DMA transfer @@ -1756,14 +1552,18 @@ mod dma { /// the SPI instance. The maximum amount of data to be /// sent/received is 32736 bytes. #[allow(clippy::type_complexity)] + #[cfg_attr(place_spi_driver_in_ram, ram)] pub fn dma_transfer( - self, - rx_buffer: RX, - tx_buffer: TX, - ) -> Result, (Error, Self, RX, TX)> { + mut self, + mut rx_buffer: RX, + mut tx_buffer: TX, + ) -> Result, (Error, Self, RX, TX)> + { self.wait_for_idle(); - - self.do_dma_transfer(rx_buffer, tx_buffer) + match unsafe { self.start_dma_transfer(&mut rx_buffer, &mut tx_buffer) } { + Ok(_) => Ok(SpiDmaTransfer::new(self, (rx_buffer, tx_buffer))), + Err(e) => Err((e, self, rx_buffer, tx_buffer)), + } } } @@ -1774,36 +1574,117 @@ mod dma { C::P: SpiPeripheral, M: Mode, { + /// # Safety: + /// + /// The caller must ensure that the buffers are not accessed while the + /// transfer is in progress. Moving the buffers is allowed. + #[cfg_attr(place_spi_driver_in_ram, ram)] + unsafe fn start_half_duplex_read( + &mut self, + data_mode: SpiDataMode, + cmd: Command, + address: Address, + dummy: u8, + buffer: &mut impl DmaRxBuffer, + ) -> Result<(), Error> { + let bytes_to_read = buffer.length(); + if bytes_to_read > MAX_DMA_SIZE { + return Err(Error::MaxDmaTransferSizeExceeded); + } + + self.spi.setup_half_duplex( + false, + cmd, + address, + false, + dummy, + bytes_to_read == 0, + data_mode, + ); + + self.start_read_bytes_dma(buffer, false) + } + /// Perform a half-duplex read operation using DMA. #[allow(clippy::type_complexity)] #[cfg_attr(place_spi_driver_in_ram, ram)] pub fn read( - self, + mut self, data_mode: SpiDataMode, cmd: Command, address: Address, dummy: u8, - buffer: RX, - ) -> Result, (Error, Self, RX)> { + mut buffer: RX, + ) -> Result, (Error, Self, RX)> { self.wait_for_idle(); - self.do_half_duplex_read(data_mode, cmd, address, dummy, buffer) + match unsafe { + self.start_half_duplex_read(data_mode, cmd, address, dummy, &mut buffer) + } { + Ok(_) => Ok(SpiDmaTransfer::new(self, buffer)), + Err(e) => Err((e, self, buffer)), + } + } + + /// # Safety: + /// + /// The caller must ensure that the buffers are not accessed while the + /// transfer is in progress. Moving the buffers is allowed. + #[cfg_attr(place_spi_driver_in_ram, ram)] + unsafe fn start_half_duplex_write( + &mut self, + data_mode: SpiDataMode, + cmd: Command, + address: Address, + dummy: u8, + buffer: &mut impl DmaTxBuffer, + ) -> Result<(), Error> { + let bytes_to_write = buffer.length(); + if bytes_to_write > MAX_DMA_SIZE { + return Err(Error::MaxDmaTransferSizeExceeded); + } + + #[cfg(all(esp32, spi_address_workaround))] + { + // On the ESP32, if we don't have data, the address is always sent + // on a single line, regardless of its data mode. + if bytes_to_write == 0 && address.mode() != SpiDataMode::Single { + return self.set_up_address_workaround(cmd, address, dummy); + } + } + + self.spi.setup_half_duplex( + true, + cmd, + address, + false, + dummy, + bytes_to_write == 0, + data_mode, + ); + + self.start_write_bytes_dma(buffer, false) } /// Perform a half-duplex write operation using DMA. #[allow(clippy::type_complexity)] #[cfg_attr(place_spi_driver_in_ram, ram)] pub fn write( - self, + mut self, data_mode: SpiDataMode, cmd: Command, address: Address, dummy: u8, - buffer: TX, - ) -> Result, (Error, Self, TX)> { + mut buffer: TX, + ) -> Result, (Error, Self, TX)> { self.wait_for_idle(); - self.do_half_duplex_write(data_mode, cmd, address, dummy, buffer) + match unsafe { + self.start_half_duplex_write(data_mode, cmd, address, dummy, &mut buffer) + } { + Ok(_) => Ok(SpiDmaTransfer::new(self, buffer)), + Err(e) => Err((e, self, buffer)), + } } } @@ -1920,11 +1801,8 @@ mod dma { for chunk in words.chunks_mut(self.rx_buf.capacity()) { self.rx_buf.set_length(chunk.len()); - let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); - match (&mut self.spi_dma).do_dma_read(rx_buffer) { - Ok(transfer) => transfer.wait(), - Err((e, _, _)) => return Err(e), - }; + unsafe { self.spi_dma.start_dma_read(&mut self.rx_buf)? }; + self.wait_for_idle(); let bytes_read = self.rx_buf.read_received_data(chunk); debug_assert_eq!(bytes_read, chunk.len()); @@ -1939,11 +1817,10 @@ mod dma { for chunk in words.chunks(self.tx_buf.capacity()) { self.tx_buf.fill(chunk); - let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); - match (&mut self.spi_dma).do_dma_write(tx_buffer) { - Ok(transfer) => transfer.wait(), - Err((e, _, _)) => return Err(e), - }; + unsafe { + self.spi_dma.start_dma_write(&mut self.tx_buf)?; + } + self.wait_for_idle(); } Ok(()) @@ -1965,12 +1842,11 @@ mod dma { self.tx_buf.fill(write_chunk); self.rx_buf.set_length(read_chunk.len()); - let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); - let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); - match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { - Ok(transfer) => transfer.wait(), - Err((e, _, _, _)) => return Err(e), - }; + unsafe { + self.spi_dma + .start_dma_transfer(&mut self.rx_buf, &mut self.tx_buf)?; + } + self.wait_for_idle(); let bytes_read = self.rx_buf.read_received_data(read_chunk); debug_assert_eq!(bytes_read, read_chunk.len()); @@ -1994,12 +1870,11 @@ mod dma { self.tx_buf.fill(chunk); self.rx_buf.set_length(chunk.len()); - let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); - let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); - match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { - Ok(transfer) => transfer.wait(), - Err((e, _, _, _)) => return Err(e), - }; + unsafe { + self.spi_dma + .start_dma_transfer(&mut self.rx_buf, &mut self.tx_buf)?; + } + self.wait_for_idle(); let bytes_read = self.rx_buf.read_received_data(chunk); debug_assert_eq!(bytes_read, chunk.len()); @@ -2033,12 +1908,17 @@ mod dma { self.wait_for_idle(); self.rx_buf.set_length(buffer.len()); - let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); - match (&mut self.spi_dma).do_half_duplex_read(data_mode, cmd, address, dummy, rx_buffer) - { - Ok(transfer) => transfer.wait(), - Err((e, _, _)) => return Err(e), - }; + unsafe { + self.spi_dma.start_half_duplex_read( + data_mode, + cmd, + address, + dummy, + &mut self.rx_buf, + )?; + } + + self.wait_for_idle(); let bytes_read = self.rx_buf.read_received_data(buffer); debug_assert_eq!(bytes_read, buffer.len()); @@ -2061,17 +1941,19 @@ mod dma { self.wait_for_idle(); self.tx_buf.fill(buffer); - let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); - match (&mut self.spi_dma) - .do_half_duplex_write(data_mode, cmd, address, dummy, tx_buffer) - { - Ok(transfer) => { - transfer.wait(); - - Ok(()) - } - Err((e, _, _)) => Err(e), + unsafe { + self.spi_dma.start_half_duplex_write( + data_mode, + cmd, + address, + dummy, + &mut self.tx_buf, + )?; } + + self.wait_for_idle(); + + Ok(()) } } @@ -2107,10 +1989,51 @@ mod dma { /// Async functionality mod asynch { - use core::cmp::min; + use core::{ + cmp::min, + ops::{Deref, DerefMut}, + }; use super::*; + struct DropGuard { + inner: ManuallyDrop, + on_drop: ManuallyDrop, + } + + impl DropGuard { + fn new(inner: I, on_drop: F) -> Self { + Self { + inner: ManuallyDrop::new(inner), + on_drop: ManuallyDrop::new(on_drop), + } + } + + fn defuse(self) {} + } + + impl Drop for DropGuard { + fn drop(&mut self) { + let inner = unsafe { ManuallyDrop::take(&mut self.inner) }; + let on_drop = unsafe { ManuallyDrop::take(&mut self.on_drop) }; + (on_drop)(inner) + } + } + + impl Deref for DropGuard { + type Target = I; + + fn deref(&self) -> &I { + &self.inner + } + } + + impl DerefMut for DropGuard { + fn deref_mut(&mut self) -> &mut I { + &mut self.inner + } + } + impl<'d, T, C> SpiDmaBus<'d, T, C, FullDuplexMode, crate::Async> where T: InstanceDma, @@ -2125,14 +2048,17 @@ mod dma { for chunk in words.chunks_mut(chunk_size) { self.rx_buf.set_length(chunk.len()); - let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); - match (&mut self.spi_dma).do_dma_read(rx_buffer) { - Ok(mut transfer) => transfer.wait_for_done().await, - Err((e, _, _)) => return Err(e), + let mut spi = DropGuard::new(&mut self.spi_dma, |spi| spi.cancel_transfer()); + + unsafe { + spi.start_dma_read(&mut self.rx_buf)?; } + spi.wait_for_idle_async().await; let bytes_read = self.rx_buf.read_received_data(chunk); debug_assert_eq!(bytes_read, chunk.len()); + + spi.defuse(); } Ok(()) @@ -2141,17 +2067,19 @@ mod dma { /// Transmit the given buffer to the bus. pub async fn write_async(&mut self, words: &[u8]) -> Result<(), Error> { self.spi_dma.wait_for_idle_async().await; + + let mut spi = DropGuard::new(&mut self.spi_dma, |spi| spi.cancel_transfer()); let chunk_size = self.tx_buf.capacity(); for chunk in words.chunks(chunk_size) { self.tx_buf.fill(chunk); - let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); - match (&mut self.spi_dma).do_dma_write(tx_buffer) { - Ok(mut transfer) => transfer.wait_for_done().await, - Err((e, _, _)) => return Err(e), + unsafe { + spi.start_dma_write(&mut self.tx_buf)?; } + spi.wait_for_idle_async().await; } + spi.defuse(); Ok(()) } @@ -2164,6 +2092,8 @@ mod dma { write: &[u8], ) -> Result<(), Error> { self.spi_dma.wait_for_idle_async().await; + + let mut spi = DropGuard::new(&mut self.spi_dma, |spi| spi.cancel_transfer()); let chunk_size = min(self.tx_buf.capacity(), self.rx_buf.capacity()); let common_length = min(read.len(), write.len()); @@ -2177,17 +2107,17 @@ mod dma { self.tx_buf.fill(write_chunk); self.rx_buf.set_length(read_chunk.len()); - let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); - let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); - match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { - Ok(mut transfer) => transfer.wait_for_done().await, - Err((e, _, _, _)) => return Err(e), + unsafe { + spi.start_dma_transfer(&mut self.rx_buf, &mut self.tx_buf)?; } + spi.wait_for_idle_async().await; let bytes_read = self.rx_buf.read_received_data(read_chunk); assert_eq!(bytes_read, read_chunk.len()); } + spi.defuse(); + if !read_remainder.is_empty() { self.read_async(read_remainder).await } else if !write_remainder.is_empty() { @@ -2201,21 +2131,23 @@ mod dma { /// the bus into the same buffer. pub async fn transfer_in_place_async(&mut self, words: &mut [u8]) -> Result<(), Error> { self.spi_dma.wait_for_idle_async().await; + + let mut spi = DropGuard::new(&mut self.spi_dma, |spi| spi.cancel_transfer()); for chunk in words.chunks_mut(self.tx_buf.capacity()) { self.tx_buf.fill(chunk); self.rx_buf.set_length(chunk.len()); - let rx_buffer = DmaBufferRef::new(&mut self.rx_buf); - let tx_buffer = DmaBufferRef::new(&mut self.tx_buf); - match (&mut self.spi_dma).do_dma_transfer(rx_buffer, tx_buffer) { - Ok(mut transfer) => transfer.wait_for_done().await, - Err((e, _, _, _)) => return Err(e), + unsafe { + spi.start_dma_transfer(&mut self.rx_buf, &mut self.tx_buf)?; } + spi.wait_for_idle_async().await; let bytes_read = self.rx_buf.read_received_data(chunk); assert_eq!(bytes_read, chunk.len()); } + spi.defuse(); + Ok(()) } } From ae4b3fd89c0375bfac9d4ddbcb5adc8985b33315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 30 Sep 2024 15:50:14 +0200 Subject: [PATCH 16/19] Make sure everything gets dropped --- esp-hal/src/spi/master.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 46b05a148bd..28d98b4d26c 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1434,7 +1434,12 @@ mod dma { fn drop(&mut self) { if !self.is_done() { self.spi_dma.cancel_transfer(); - self.spi_dma.wait_for_idle() + self.spi_dma.wait_for_idle(); + + unsafe { + ManuallyDrop::drop(&mut self.spi_dma); + ManuallyDrop::drop(&mut self.dma_buf); + } } } } From 0a3a313980b7d327e82b19ac0d482557bdb24c17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 30 Sep 2024 15:50:40 +0200 Subject: [PATCH 17/19] Remove unnecessary PhantomData --- esp-hal/src/spi/master.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 28d98b4d26c..95ea8e68fa9 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1371,8 +1371,6 @@ mod dma { { spi_dma: ManuallyDrop>, dma_buf: ManuallyDrop, - - _marker: PhantomData<&'d ()>, } impl<'d, T, C, D, M, Buf> SpiDmaTransfer<'d, T, C, D, M, Buf> @@ -1387,7 +1385,6 @@ mod dma { Self { spi_dma: ManuallyDrop::new(spi_dma), dma_buf: ManuallyDrop::new(dma_buf), - _marker: PhantomData, } } From fad136412441cbafa19c4de65f62e520fcdbb1ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 3 Oct 2024 11:50:54 +0200 Subject: [PATCH 18/19] Remove OptionalFuture --- esp-hal/src/spi/master.rs | 55 +++++---------------------------------- 1 file changed, 6 insertions(+), 49 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 95ea8e68fa9..0ebbc086c1d 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1128,58 +1128,15 @@ mod dma { async fn wait_for_idle_async(&mut self) { // As a future enhancement, setup Spi Future in here as well. - /// An optional future. - enum OptionalFuture { - Future(T), - Value(T::Output), + if self.rx_transfer_in_progress { + _ = DmaRxFuture::new(&mut self.channel.rx).await; + self.rx_transfer_in_progress = false; } - - impl OptionalFuture { - fn some(future: T) -> Self { - Self::Future(future) - } - - fn none(value: T::Output) -> Self { - Self::Value(value) - } + if self.tx_transfer_in_progress { + _ = DmaTxFuture::new(&mut self.channel.tx).await; + self.tx_transfer_in_progress = false; } - use core::{ - future::Future, - pin::{pin, Pin}, - task::{Context, Poll}, - }; - impl Future for OptionalFuture - where - T: Future + Unpin, - T::Output: Copy + Unpin, - { - type Output = T::Output; - - fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - match self.get_mut() { - Self::Future(future) => pin!(future).poll(cx), - Self::Value(value) => Poll::Ready(*value), - } - } - } - - let rx_future = if self.rx_transfer_in_progress { - OptionalFuture::some(DmaRxFuture::new(&mut self.channel.rx)) - } else { - OptionalFuture::none(Ok(())) - }; - let tx_future = if self.tx_transfer_in_progress { - OptionalFuture::some(DmaTxFuture::new(&mut self.channel.tx)) - } else { - OptionalFuture::none(Ok(())) - }; - - _ = embassy_futures::join::join(rx_future, tx_future).await; - - self.rx_transfer_in_progress = false; - self.tx_transfer_in_progress = false; - core::future::poll_fn(|cx| { use core::task::Poll; if self.is_done() { From 3b40895431490e7befa111d3398bb887f059ffd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 4 Oct 2024 11:53:53 +0200 Subject: [PATCH 19/19] Adjust test to a more realistic clock frequency --- hil-test/tests/spi_full_duplex.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/hil-test/tests/spi_full_duplex.rs b/hil-test/tests/spi_full_duplex.rs index 5df30fdc5b7..f9410341fe0 100644 --- a/hil-test/tests/spi_full_duplex.rs +++ b/hil-test/tests/spi_full_duplex.rs @@ -488,10 +488,12 @@ mod tests { } #[test] - #[timeout(3)] + #[timeout(2)] fn cancel_stops_transaction(mut ctx: Context) { - // Slow down - ctx.spi.change_bus_frequency(100.Hz()); + // Slow down. At 80kHz, the transfer is supposed to take a bit over 3 seconds. + // This means that without working cancellation, the test case should + // fail. + ctx.spi.change_bus_frequency(80.kHz()); // Set up a large buffer that would trigger a timeout let dma_rx_buf = DmaRxBuf::new(ctx.rx_descriptors, ctx.rx_buffer).unwrap(); @@ -513,8 +515,8 @@ mod tests { #[test] #[timeout(3)] fn can_transmit_after_cancel(mut ctx: Context) { - // Slow down - ctx.spi.change_bus_frequency(100.Hz()); + // Slow down. At 80kHz, the transfer is supposed to take a bit over 3 seconds. + ctx.spi.change_bus_frequency(80.kHz()); // Set up a large buffer that would trigger a timeout let mut dma_rx_buf = DmaRxBuf::new(ctx.rx_descriptors, ctx.rx_buffer).unwrap();