itsscb/esp-hal
1
0
Fork 0
mirror of https://github.com/esp-rs/esp-hal.git synced 2025-09-28 21:00:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix known SPI issues (#2179)

Browse Source 
* Re-enable tests

* Clean up

* Pass two lengths to configure_datalen

* Add Dominic's test changes

* Ensure DMA buffers are properly aligned

* Impl Format on DmaDescriptor

* Fix waiting for transaction to complete

* Fix DMA transfers

* Changelog

* Avoid explicit panic in test case

* Remove redundant test case

* Reintroduce wait for ESP32
This commit is contained in:
Dániel Buga 2024-09-19 20:00:50 +02:00 committed by GitHub
parent ba63beb3b2
commit 5de267ab09
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 172 additions and 167 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
esp-hal/CHANGELOG.md
View File

@ -59,6 +59,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Fix `SpiBus::transfer` transferring data twice in some cases (#2159)
- Fixed UART freezing when using `RcFast` clock source on ESP32-C2/C3 (#2170)
- I2S: on ESP32 and ESP32-S2 data is now output to the right (WS=1) channel first. (#2194)
- SPI: Fixed an issue where unexpected data was written outside of the read buffer (#2179)
- SPI: Fixed an issue where `wait` has returned before the DMA has finished writing the memory (#2179)
- SPI: Fixed an issue where repeated calls to `dma_transfer` may end up looping indefinitely (#2179)
- SPI: Fixed an issue that prevented correctly reading the first byte in a transaction (#2179)
### Removed

41
esp-hal/src/dma/mod.rs
View File

@ -205,6 +205,7 @@ where
bitfield::bitfield! {
#[doc(hidden)]
#[derive(Clone, Copy)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub struct DmaDescriptorFlags(u32);
u16;
@ -227,6 +228,7 @@ impl Debug for DmaDescriptorFlags {
/// A DMA transfer descriptor.
#[derive(Clone, Copy, Debug)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub struct DmaDescriptor {
pub(crate) flags: DmaDescriptorFlags,
pub(crate) buffer: *mut u8,
@ -397,6 +399,29 @@ macro_rules! dma_circular_descriptors {
};
}
/// Declares a DMA buffer with a specific size, aligned to 4 bytes
#[doc(hidden)]
#[macro_export]
macro_rules! declare_aligned_dma_buffer {
($name:ident, $size:expr) => {
// ESP32 requires word alignment for DMA buffers.
// ESP32-S2 technically supports byte-aligned DMA buffers, but the
// transfer ends up writing out of bounds.
// if the buffer's length is 2 or 3 (mod 4).
static mut $name: [u32; ($size + 3) / 4] = [0; ($size + 3) / 4];
};
}
/// Turns the potentially oversized static `u32`` array reference into a
/// correctly sized `u8` one
#[doc(hidden)]
#[macro_export]
macro_rules! as_mut_byte_array {
($name:ident, $size:expr) => {
unsafe { &mut *($name.as_mut_ptr() as *mut [u8; $size]) }
};
}
/// Convenience macro to create DMA buffers and descriptors with specific chunk
/// size.
///
@ -414,15 +439,15 @@ macro_rules! dma_circular_descriptors {
#[macro_export]
macro_rules! dma_buffers_chunk_size {
($rx_size:expr, $tx_size:expr, $chunk_size:expr) => {{
static mut RX_BUFFER: [u8; $rx_size] = [0u8; $rx_size];
static mut TX_BUFFER: [u8; $tx_size] = [0u8; $tx_size];
$crate::declare_aligned_dma_buffer!(RX_BUFFER, $rx_size);
$crate::declare_aligned_dma_buffer!(TX_BUFFER, $tx_size);
let (mut rx_descriptors, mut tx_descriptors) =
$crate::dma_descriptors_chunk_size!($rx_size, $tx_size, $chunk_size);
unsafe {
(
&mut RX_BUFFER,
$crate::as_mut_byte_array!(RX_BUFFER, $rx_size),
rx_descriptors,
&mut TX_BUFFER,
$crate::as_mut_byte_array!(TX_BUFFER, $tx_size),
tx_descriptors,
)
}
@ -450,15 +475,15 @@ macro_rules! dma_buffers_chunk_size {
#[macro_export]
macro_rules! dma_circular_buffers_chunk_size {
($rx_size:expr, $tx_size:expr, $chunk_size:expr) => {{
static mut RX_BUFFER: [u8; $rx_size] = [0u8; $rx_size];
static mut TX_BUFFER: [u8; $tx_size] = [0u8; $tx_size];
$crate::declare_aligned_dma_buffer!(RX_BUFFER, $rx_size);
$crate::declare_aligned_dma_buffer!(TX_BUFFER, $tx_size);
let (mut rx_descriptors, mut tx_descriptors) =
$crate::dma_circular_descriptors_chunk_size!($rx_size, $tx_size, $chunk_size);
unsafe {
(
&mut RX_BUFFER,
$crate::as_mut_byte_array!(RX_BUFFER, $rx_size),
rx_descriptors,
&mut TX_BUFFER,
$crate::as_mut_byte_array!(TX_BUFFER, $tx_size),
tx_descriptors,
)
}

2
esp-hal/src/soc/esp32/gpio.rs
View File

@ -103,7 +103,7 @@ pub(crate) fn get_io_mux_reg(gpio_num: u8) -> &'static io_mux::GPIO0 {
37 => transmute::<&'static io_mux::GPIO37, &'static io_mux::GPIO0>(iomux.gpio37()),
38 => transmute::<&'static io_mux::GPIO38, &'static io_mux::GPIO0>(iomux.gpio38()),
39 => transmute::<&'static io_mux::GPIO39, &'static io_mux::GPIO0>(iomux.gpio39()),
_ => panic!(),
other => panic!("GPIO {} does not exist", other),
}
}
}

254
esp-hal/src/spi/master.rs
View File

@ -109,9 +109,8 @@ const FIFO_SIZE: usize = 64;
const FIFO_SIZE: usize = 72;
/// Padding byte for empty write transfers
const EMPTY_WRITE_PAD: u8 = 0x00u8;
const EMPTY_WRITE_PAD: u8 = 0x00;
#[allow(unused)]
const MAX_DMA_SIZE: usize = 32736;
/// SPI commands, each consisting of a 16-bit command value and a data mode.
@ -490,7 +489,7 @@ impl<'d, T> Spi<'d, T, FullDuplexMode>
where
T: Instance,
{
/// Read bytes from SPI.
/// Read a byte from SPI.
///
/// Sends out a stuffing byte for every byte to read. This function doesn't
/// perform flushing. If you want to read the response to something you
@ -1154,8 +1153,10 @@ mod dma {
///
/// This method blocks until the transfer is finished and returns the
/// `SpiDma` instance and the associated buffer.
pub fn wait(mut self) -> (SpiDma<'d, T, C, D, M>, Buf) {
self.spi_dma.spi.flush().ok();
pub fn wait(self) -> (SpiDma<'d, T, C, D, M>, Buf) {
while !self.is_done() {
// Wait for the transfer to complete
}
fence(Ordering::Acquire);
(self.spi_dma, self.dma_buf)
}
@ -2221,10 +2222,7 @@ pub trait InstanceDma: Instance {
reg_block.dma_in_link().write(|w| w.bits(0));
}
self.configure_datalen(usize::max(rx_buffer.length(), tx_buffer.length()) as u32 * 8);
rx.is_done();
tx.is_done();
self.configure_datalen(rx_buffer.length(), tx_buffer.length());
// re-enable the MISO and MOSI
reg_block
@ -2232,15 +2230,14 @@ pub trait InstanceDma: Instance {
.modify(|_, w| w.usr_miso().bit(true).usr_mosi().bit(true));
self.enable_dma();
self.clear_dma_interrupts();
reset_dma_before_load_dma_dscr(reg_block);
rx.prepare_transfer(self.dma_peripheral(), rx_buffer)
.and_then(|_| rx.start_transfer())?;
tx.prepare_transfer(self.dma_peripheral(), tx_buffer)
.and_then(|_| tx.start_transfer())?;
reset_dma_before_usr_cmd(reg_block);
#[cfg(gdma)]
self.reset_dma();
self.start_operation();
@ -2255,9 +2252,7 @@ pub trait InstanceDma: Instance {
full_duplex: bool,
) -> Result<(), Error> {
let reg_block = self.register_block();
self.configure_datalen(buffer.length() as u32 * 8);
tx.is_done();
self.configure_datalen(0, buffer.length());
// disable MISO and re-enable MOSI (DON'T do it for half-duplex)
if full_duplex {
@ -2281,14 +2276,12 @@ pub trait InstanceDma: Instance {
}
self.enable_dma();
self.clear_dma_interrupts();
reset_dma_before_load_dma_dscr(reg_block);
tx.prepare_transfer(self.dma_peripheral(), buffer)
.and_then(|_| tx.start_transfer())?;
tx.prepare_transfer(self.dma_peripheral(), buffer)?;
tx.start_transfer()?;
reset_dma_before_usr_cmd(reg_block);
#[cfg(gdma)]
self.reset_dma();
self.start_operation();
@ -2311,9 +2304,7 @@ pub trait InstanceDma: Instance {
reg_block.dma_in_link().write(|w| w.bits(0));
}
self.configure_datalen(buffer.length() as u32 * 8);
rx.is_done();
self.configure_datalen(buffer.length(), 0);
// re-enable MISO and disable MOSI (DON'T do it for half-duplex)
if full_duplex {
@ -2323,14 +2314,12 @@ pub trait InstanceDma: Instance {
}
self.enable_dma();
self.clear_dma_interrupts();
reset_dma_before_load_dma_dscr(reg_block);
rx.prepare_transfer(self.dma_peripheral(), buffer)
.and_then(|_| rx.start_transfer())?;
rx.prepare_transfer(self.dma_peripheral(), buffer)?;
rx.start_transfer()?;
reset_dma_before_usr_cmd(reg_block);
#[cfg(gdma)]
self.reset_dma();
self.start_operation();
@ -2346,32 +2335,63 @@ pub trait InstanceDma: Instance {
}
}
#[cfg(gdma)]
fn enable_dma(&self) {
let reg_block = self.register_block();
reg_block.dma_conf().modify(|_, w| w.dma_tx_ena().set_bit());
reg_block.dma_conf().modify(|_, w| w.dma_rx_ena().set_bit());
#[cfg(gdma)]
{
// for non GDMA this is done in `assign_tx_device` / `assign_rx_device`
let reg_block = self.register_block();
reg_block.dma_conf().modify(|_, w| {
w.dma_tx_ena().set_bit();
w.dma_rx_ena().set_bit()
});
}
#[cfg(pdma)]
self.reset_dma();
}
#[cfg(pdma)]
fn enable_dma(&self) {
// for non GDMA this is done in `assign_tx_device` / `assign_rx_device`
fn reset_dma(&self) {
#[cfg(pdma)]
{
let reg_block = self.register_block();
reg_block.dma_conf().modify(|_, w| {
w.out_rst().set_bit();
w.in_rst().set_bit();
w.ahbm_fifo_rst().set_bit();
w.ahbm_rst().set_bit()
});
reg_block.dma_conf().modify(|_, w| {
w.out_rst().clear_bit();
w.in_rst().clear_bit();
w.ahbm_fifo_rst().clear_bit();
w.ahbm_rst().clear_bit()
});
}
#[cfg(gdma)]
{
let reg_block = self.register_block();
reg_block.dma_conf().modify(|_, w| {
w.rx_afifo_rst().set_bit();
w.buf_afifo_rst().set_bit();
w.dma_afifo_rst().set_bit()
});
reg_block.dma_conf().modify(|_, w| {
w.rx_afifo_rst().clear_bit();
w.buf_afifo_rst().clear_bit();
w.dma_afifo_rst().clear_bit()
});
}
self.clear_dma_interrupts();
}
#[cfg(gdma)]
fn clear_dma_interrupts(&self) {
let reg_block = self.register_block();
reg_block.dma_int_clr().write(|w| {
w.dma_infifo_full_err()
.clear_bit_by_one()
.dma_outfifo_empty_err()
.clear_bit_by_one()
.trans_done()
.clear_bit_by_one()
.mst_rx_afifo_wfull_err()
.clear_bit_by_one()
.mst_tx_afifo_rempty_err()
.clear_bit_by_one()
w.dma_infifo_full_err().clear_bit_by_one();
w.dma_outfifo_empty_err().clear_bit_by_one();
w.trans_done().clear_bit_by_one();
w.mst_rx_afifo_wfull_err().clear_bit_by_one();
w.mst_tx_afifo_rempty_err().clear_bit_by_one()
});
}
@ -2379,68 +2399,19 @@ pub trait InstanceDma: Instance {
fn clear_dma_interrupts(&self) {
let reg_block = self.register_block();
reg_block.dma_int_clr().write(|w| {
w.inlink_dscr_empty()
.clear_bit_by_one()
.outlink_dscr_error()
.clear_bit_by_one()
.inlink_dscr_error()
.clear_bit_by_one()
.in_done()
.clear_bit_by_one()
.in_err_eof()
.clear_bit_by_one()
.in_suc_eof()
.clear_bit_by_one()
.out_done()
.clear_bit_by_one()
.out_eof()
.clear_bit_by_one()
.out_total_eof()
.clear_bit_by_one()
w.inlink_dscr_empty().clear_bit_by_one();
w.outlink_dscr_error().clear_bit_by_one();
w.inlink_dscr_error().clear_bit_by_one();
w.in_done().clear_bit_by_one();
w.in_err_eof().clear_bit_by_one();
w.in_suc_eof().clear_bit_by_one();
w.out_done().clear_bit_by_one();
w.out_eof().clear_bit_by_one();
w.out_total_eof().clear_bit_by_one()
});
}
}
fn reset_dma_before_usr_cmd(_reg_block: &RegisterBlock) {
#[cfg(gdma)]
_reg_block.dma_conf().modify(|_, w| {
w.rx_afifo_rst()
.set_bit()
.buf_afifo_rst()
.set_bit()
.dma_afifo_rst()
.set_bit()
});
}
#[cfg(gdma)]
fn reset_dma_before_load_dma_dscr(_reg_block: &RegisterBlock) {}
#[cfg(pdma)]
fn reset_dma_before_load_dma_dscr(reg_block: &RegisterBlock) {
reg_block.dma_conf().modify(|_, w| {
w.out_rst()
.set_bit()
.in_rst()
.set_bit()
.ahbm_fifo_rst()
.set_bit()
.ahbm_rst()
.set_bit()
});
reg_block.dma_conf().modify(|_, w| {
w.out_rst()
.clear_bit()
.in_rst()
.clear_bit()
.ahbm_fifo_rst()
.clear_bit()
.ahbm_rst()
.clear_bit()
});
}
impl InstanceDma for crate::peripherals::SPI2 {}
#[cfg(spi3)]
@ -3032,7 +3003,7 @@ pub trait Instance: private::Sealed {
return Err(nb::Error::WouldBlock);
}
self.configure_datalen(8);
self.configure_datalen(0, 1);
let reg_block = self.register_block();
reg_block.w(0).write(|w| w.buf().set(word.into()));
@ -3061,7 +3032,7 @@ pub trait Instance: private::Sealed {
// The fifo has a limited fixed size, so the data must be chunked and then
// transmitted
for (i, chunk) in words.chunks(FIFO_SIZE).enumerate() {
self.configure_datalen(chunk.len() as u32 * 8);
self.configure_datalen(0, chunk.len());
{
// TODO: replace with `array_chunks` and `from_le_bytes`
@ -3130,7 +3101,7 @@ pub trait Instance: private::Sealed {
let reg_block = self.register_block();
for chunk in words.chunks_mut(FIFO_SIZE) {
self.configure_datalen(chunk.len() as u32 * 8);
self.configure_datalen(chunk.len(), 0);
for (index, w_reg) in (0..chunk.len()).step_by(4).zip(reg_block.w_iter()) {
let reg_val = w_reg.read().bits();
@ -3354,46 +3325,63 @@ pub trait Instance: private::Sealed {
.modify(|_, w| unsafe { w.usr_dummy_cyclelen().bits(dummy - 1) });
}
self.configure_datalen(buffer.len() as u32 * 8);
self.configure_datalen(buffer.len(), 0);
self.start_operation();
self.flush()?;
self.read_bytes_from_fifo(buffer)
}
#[cfg(gdma)]
fn update(&self) {
let reg_block = self.register_block();
cfg_if::cfg_if! {
if #[cfg(gdma)] {
let reg_block = self.register_block();
reg_block.cmd().modify(|_, w| w.update().set_bit());
reg_block.cmd().modify(|_, w| w.update().set_bit());
while reg_block.cmd().read().update().bit_is_set() {
// wait
while reg_block.cmd().read().update().bit_is_set() {
// wait
}
} else if #[cfg(esp32)] {
xtensa_lx::timer::delay(1);
} else {
// Doesn't seem to be needed for ESP32-S2
}
}
}
#[cfg(pdma)]
fn update(&self) {
// not need/available on ESP32/ESP32S2
}
fn configure_datalen(&self, len: u32) {
fn configure_datalen(&self, rx_len_bytes: usize, tx_len_bytes: usize) {
let reg_block = self.register_block();
let len = if len > 0 { len - 1 } else { 0 };
#[cfg(any(esp32c2, esp32c3, esp32c6, esp32h2, esp32s3))]
reg_block
.ms_dlen()
.write(|w| unsafe { w.ms_data_bitlen().bits(len) });
let rx_len = rx_len_bytes as u32 * 8;
let tx_len = tx_len_bytes as u32 * 8;
#[cfg(not(any(esp32c2, esp32c3, esp32c6, esp32h2, esp32s3)))]
{
reg_block
.mosi_dlen()
.write(|w| unsafe { w.usr_mosi_dbitlen().bits(len) });
let rx_len = rx_len.saturating_sub(1);
let tx_len = tx_len.saturating_sub(1);
cfg_if::cfg_if! {
if #[cfg(esp32)] {
let len = rx_len.max(tx_len);
reg_block
.mosi_dlen()
.write(|w| unsafe { w.usr_mosi_dbitlen().bits(len) });
reg_block
.miso_dlen()
.write(|w| unsafe { w.usr_miso_dbitlen().bits(len) });
} else if #[cfg(esp32s2)] {
reg_block
.mosi_dlen()
.write(|w| unsafe { w.usr_mosi_dbitlen().bits(tx_len) });
reg_block
.miso_dlen()
.write(|w| unsafe { w.usr_miso_dbitlen().bits(rx_len) });
} else {
reg_block
.ms_dlen()
.write(|w| unsafe { w.ms_data_bitlen().bits(rx_len.max(tx_len)) });
}
reg_block
.miso_dlen()
.write(|w| unsafe { w.usr_miso_dbitlen().bits(len) });
}
}
}

38
hil-test/tests/spi_full_duplex.rs
View File

@ -27,10 +27,7 @@ use esp_hal::{
use hil_test as _;
cfg_if::cfg_if! {
if #[cfg(any(
feature = "esp32",
feature = "esp32s2",
))] {
if #[cfg(any(esp32, esp32s2))] {
type DmaChannelCreator = esp_hal::dma::Spi2DmaChannelCreator;
} else {
type DmaChannelCreator = esp_hal::dma::ChannelCreator<0>;
@ -64,7 +61,7 @@ mod tests {
let dma = Dma::new(peripherals.DMA);
cfg_if::cfg_if! {
if #[cfg(any(feature = "esp32", feature = "esp32s2"))] {
if #[cfg(any(esp32, esp32s2))] {
let dma_channel = dma.spi2channel;
} else {
let dma_channel = dma.channel0;
@ -253,9 +250,8 @@ mod tests {
}
#[test]
#[cfg(not(esp32))]
fn test_symmetric_dma_transfer(ctx: Context) {
// This test case sends a large amount of data, twice, to verify that
// 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();
@ -278,13 +274,16 @@ mod tests {
.unwrap();
(spi, (dma_rx_buf, dma_tx_buf)) = transfer.wait();
assert_eq!(dma_tx_buf.as_slice(), dma_rx_buf.as_slice());
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!("Mismatch at iteration {}", i);
}
}
}
#[test]
#[timeout(3)]
#[cfg(not(feature = "esp32s2"))]
fn test_asymmetric_dma_transfer(ctx: Context) {
let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(2, 4);
let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
@ -299,30 +298,19 @@ mod tests {
.dma_transfer(dma_rx_buf, dma_tx_buf)
.map_err(|e| e.0)
.unwrap();
let (_, (dma_rx_buf, dma_tx_buf)) = transfer.wait();
assert_eq!(dma_tx_buf.as_slice()[0..1], dma_rx_buf.as_slice()[0..1]);
}
let (spi, (dma_rx_buf, mut dma_tx_buf)) = transfer.wait();
assert_eq!(dma_tx_buf.as_slice()[0..2], dma_rx_buf.as_slice()[0..2]);
#[test]
#[timeout(3)]
fn test_symmetric_dma_transfer_huge_buffer(ctx: Context) {
let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(4096);
let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
let mut dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap();
// Try transfer again to make sure DMA isn't in a broken state.
for (i, d) in dma_tx_buf.as_mut_slice().iter_mut().enumerate() {
*d = i as _;
}
dma_tx_buf.fill(&[0xaa, 0xdd, 0xef, 0xbe]);
let spi = ctx
.spi
.with_dma(ctx.dma_channel.configure(false, DmaPriority::Priority0));
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();
assert_eq!(dma_tx_buf.as_slice(), dma_rx_buf.as_slice());
assert_eq!(dma_tx_buf.as_slice()[0..2], dma_rx_buf.as_slice()[0..2]);
}
#[test]