From 7a4db1da2641c785f5fd9d2365df2a213f3aaade Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 20 Mar 2023 16:34:30 +0200 Subject: [PATCH 1/4] fix(rp): spi transfer Signed-off-by: Lachezar Lechev --- embassy-rp/src/dma.rs | 1 + embassy-rp/src/spi.rs | 46 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/embassy-rp/src/dma.rs b/embassy-rp/src/dma.rs index 05adcecd..ba07a88d 100644 --- a/embassy-rp/src/dma.rs +++ b/embassy-rp/src/dma.rs @@ -1,3 +1,4 @@ +//! Direct Memory Access (DMA) use core::future::Future; use core::pin::Pin; use core::sync::atomic::{compiler_fence, Ordering}; diff --git a/embassy-rp/src/spi.rs b/embassy-rp/src/spi.rs index 584370d5..c48e33fc 100644 --- a/embassy-rp/src/spi.rs +++ b/embassy-rp/src/spi.rs @@ -1,3 +1,4 @@ +//! Serial Peripheral Interface use core::marker::PhantomData; use embassy_embedded_hal::SetConfig; @@ -385,19 +386,36 @@ impl<'d, T: Instance> Spi<'d, T, Async> { async fn transfer_inner(&mut self, rx_ptr: *mut [u8], tx_ptr: *const [u8]) -> Result<(), Error> { let (_, from_len) = crate::dma::slice_ptr_parts(tx_ptr); let (_, to_len) = crate::dma::slice_ptr_parts_mut(rx_ptr); - assert_eq!(from_len, to_len); + unsafe { self.inner.regs().dmacr().write(|reg| { reg.set_rxdmae(true); reg.set_txdmae(true); }) }; - let tx_ch = self.tx_dma.as_mut().unwrap(); - let tx_transfer = unsafe { - // If we don't assign future to a variable, the data register pointer - // is held across an await and makes the future non-Send. - crate::dma::write(tx_ch, tx_ptr, self.inner.regs().dr().ptr() as *mut _, T::TX_DREQ) + + let mut tx_ch = self.tx_dma.as_mut().unwrap(); + // If we don't assign future to a variable, the data register pointer + // is held across an await and makes the future non-Send. + let tx_transfer = async { + let p = self.inner.regs(); + unsafe { + crate::dma::write(&mut tx_ch, tx_ptr, p.dr().ptr() as *mut _, T::TX_DREQ).await; + + if from_len > to_len { + let write_bytes_len = from_len - to_len; + // disable incrementation of buffer + tx_ch.regs().ctrl_trig().modify(|ctrl_trig| { + ctrl_trig.set_incr_write(false); + ctrl_trig.set_incr_read(false); + }); + + // write dummy data + crate::dma::write_repeated(tx_ch, p.dr().ptr() as *mut u8, write_bytes_len, T::TX_DREQ).await + } + } }; + let rx_ch = self.rx_dma.as_mut().unwrap(); let rx_transfer = unsafe { // If we don't assign future to a variable, the data register pointer @@ -405,6 +423,22 @@ impl<'d, T: Instance> Spi<'d, T, Async> { crate::dma::read(rx_ch, self.inner.regs().dr().ptr() as *const _, rx_ptr, T::RX_DREQ) }; join(tx_transfer, rx_transfer).await; + + // if tx > rx we should clear any overflow of the FIFO SPI buffer + if from_len > to_len { + let p = self.inner.regs(); + unsafe { + while p.sr().read().bsy() {} + + // clear RX FIFO contents to prevent stale reads + while p.sr().read().rne() { + let _: u16 = p.dr().read().data(); + } + // clear RX overrun interrupt + p.icr().write(|w| w.set_roric(true)); + } + } + Ok(()) } } From 9939d438007f33bc57697be97c9a73ee001fe737 Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Fri, 24 Mar 2023 12:14:23 +0200 Subject: [PATCH 2/4] fix: PR comment Signed-off-by: Lachezar Lechev --- embassy-rp/src/spi.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/embassy-rp/src/spi.rs b/embassy-rp/src/spi.rs index c48e33fc..e6682ad6 100644 --- a/embassy-rp/src/spi.rs +++ b/embassy-rp/src/spi.rs @@ -404,13 +404,8 @@ impl<'d, T: Instance> Spi<'d, T, Async> { if from_len > to_len { let write_bytes_len = from_len - to_len; - // disable incrementation of buffer - tx_ch.regs().ctrl_trig().modify(|ctrl_trig| { - ctrl_trig.set_incr_write(false); - ctrl_trig.set_incr_read(false); - }); - // write dummy data + // this will disable incrementation of the buffers crate::dma::write_repeated(tx_ch, p.dr().ptr() as *mut u8, write_bytes_len, T::TX_DREQ).await } } From cd2f28d2abb5b66981b7fdbb32566e6b942c7a54 Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Fri, 24 Mar 2023 12:14:38 +0200 Subject: [PATCH 3/4] chore: add spi_async tests for uneven buffers Signed-off-by: Lachezar Lechev --- tests/rp/src/bin/spi_async.rs | 44 +++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/tests/rp/src/bin/spi_async.rs b/tests/rp/src/bin/spi_async.rs index 6c85ef60..e3fe6e84 100644 --- a/tests/rp/src/bin/spi_async.rs +++ b/tests/rp/src/bin/spi_async.rs @@ -1,3 +1,6 @@ +//! Make sure to connect GPIO pins 3 (`PIN_3`) and 4 (`PIN_4`) together +//! to run this test. +//! #![no_std] #![no_main] #![feature(type_alias_impl_trait)] @@ -18,10 +21,43 @@ async fn main(_spawner: Spawner) { let mut spi = Spi::new(p.SPI0, clk, mosi, miso, p.DMA_CH0, p.DMA_CH1, Config::default()); - let tx_buf = [1_u8, 2, 3, 4, 5, 6]; - let mut rx_buf = [0_u8; 6]; - spi.transfer(&mut rx_buf, &tx_buf).await.unwrap(); - assert_eq!(rx_buf, tx_buf); + // equal rx & tx buffers + { + let tx_buf = [1_u8, 2, 3, 4, 5, 6]; + let mut rx_buf = [0_u8; 6]; + spi.transfer(&mut rx_buf, &tx_buf).await.unwrap(); + assert_eq!(rx_buf, tx_buf); + } + + // tx > rx buffer + { + let tx_buf = [7_u8, 8, 9, 10, 11, 12]; + + let mut rx_buf = [0_u8, 3]; + spi.transfer(&mut rx_buf, &tx_buf).await.unwrap(); + assert_eq!(rx_buf, tx_buf[..3]); + } + + // we make sure to that clearing FIFO works after the uneven buffers + + // equal rx & tx buffers + { + let tx_buf = [13_u8, 14, 15, 16, 17, 18]; + let mut rx_buf = [0_u8; 6]; + spi.transfer(&mut rx_buf, &tx_buf).await.unwrap(); + + assert_eq!(rx_buf, tx_buf); + } + + // rx > tx buffer + { + let tx_buf = [19_u8, 20, 21]; + let mut rx_buf = [0_u8; 6]; + spi.transfer(&mut rx_buf, &tx_buf).await.unwrap(); + + assert_eq!(rx_buf[..3], tx_buf, "only the first 3 TX bytes should have been received in the RX buffer"); + assert_eq!(rx_buf[3..], [0, 0, 0], "the rest of the RX bytes should be empty"); + } info!("Test OK"); cortex_m::asm::bkpt(); From 7be63b3468f72fc684267c90093a00e77cff1bdc Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Sun, 26 Mar 2023 18:14:17 +0300 Subject: [PATCH 4/4] fix: spi transfer bug and additions to test Signed-off-by: Lachezar Lechev --- embassy-rp/src/spi.rs | 10 +++++----- tests/rp/src/bin/spi_async.rs | 26 +++++++++++++++++++++++--- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/embassy-rp/src/spi.rs b/embassy-rp/src/spi.rs index e6682ad6..ebd621ec 100644 --- a/embassy-rp/src/spi.rs +++ b/embassy-rp/src/spi.rs @@ -384,8 +384,8 @@ impl<'d, T: Instance> Spi<'d, T, Async> { } async fn transfer_inner(&mut self, rx_ptr: *mut [u8], tx_ptr: *const [u8]) -> Result<(), Error> { - let (_, from_len) = crate::dma::slice_ptr_parts(tx_ptr); - let (_, to_len) = crate::dma::slice_ptr_parts_mut(rx_ptr); + let (_, tx_len) = crate::dma::slice_ptr_parts(tx_ptr); + let (_, rx_len) = crate::dma::slice_ptr_parts_mut(rx_ptr); unsafe { self.inner.regs().dmacr().write(|reg| { @@ -402,8 +402,8 @@ impl<'d, T: Instance> Spi<'d, T, Async> { unsafe { crate::dma::write(&mut tx_ch, tx_ptr, p.dr().ptr() as *mut _, T::TX_DREQ).await; - if from_len > to_len { - let write_bytes_len = from_len - to_len; + if rx_len > tx_len { + let write_bytes_len = rx_len - tx_len; // write dummy data // this will disable incrementation of the buffers crate::dma::write_repeated(tx_ch, p.dr().ptr() as *mut u8, write_bytes_len, T::TX_DREQ).await @@ -420,7 +420,7 @@ impl<'d, T: Instance> Spi<'d, T, Async> { join(tx_transfer, rx_transfer).await; // if tx > rx we should clear any overflow of the FIFO SPI buffer - if from_len > to_len { + if tx_len > rx_len { let p = self.inner.regs(); unsafe { while p.sr().read().bsy() {} diff --git a/tests/rp/src/bin/spi_async.rs b/tests/rp/src/bin/spi_async.rs index e3fe6e84..2e22c9de 100644 --- a/tests/rp/src/bin/spi_async.rs +++ b/tests/rp/src/bin/spi_async.rs @@ -33,9 +33,11 @@ async fn main(_spawner: Spawner) { { let tx_buf = [7_u8, 8, 9, 10, 11, 12]; - let mut rx_buf = [0_u8, 3]; + let mut rx_buf = [0_u8; 3]; spi.transfer(&mut rx_buf, &tx_buf).await.unwrap(); assert_eq!(rx_buf, tx_buf[..3]); + + defmt::info!("tx > rx buffer - OK"); } // we make sure to that clearing FIFO works after the uneven buffers @@ -45,18 +47,36 @@ async fn main(_spawner: Spawner) { let tx_buf = [13_u8, 14, 15, 16, 17, 18]; let mut rx_buf = [0_u8; 6]; spi.transfer(&mut rx_buf, &tx_buf).await.unwrap(); - assert_eq!(rx_buf, tx_buf); + + defmt::info!("buffer rx length == tx length - OK"); } // rx > tx buffer { let tx_buf = [19_u8, 20, 21]; let mut rx_buf = [0_u8; 6]; + + // we should have written dummy data to tx buffer to sync clock. spi.transfer(&mut rx_buf, &tx_buf).await.unwrap(); - assert_eq!(rx_buf[..3], tx_buf, "only the first 3 TX bytes should have been received in the RX buffer"); + assert_eq!( + rx_buf[..3], + tx_buf, + "only the first 3 TX bytes should have been received in the RX buffer" + ); assert_eq!(rx_buf[3..], [0, 0, 0], "the rest of the RX bytes should be empty"); + defmt::info!("buffer rx length > tx length - OK"); + } + + // equal rx & tx buffers + { + let tx_buf = [22_u8, 23, 24, 25, 26, 27]; + let mut rx_buf = [0_u8; 6]; + spi.transfer(&mut rx_buf, &tx_buf).await.unwrap(); + + assert_eq!(rx_buf, tx_buf); + defmt::info!("buffer rx length = tx length - OK"); } info!("Test OK");