From 07ed22df17f28f1b1890aa47c3ae599182397712 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 10 Nov 2023 15:04:05 +0100 Subject: [PATCH] Clean up (#920) --- esp-hal-common/src/aes/mod.rs | 23 +++++- esp-hal-common/src/i2s.rs | 19 +++-- esp-hal-common/src/parl_io.rs | 11 ++- esp-hal-common/src/spi/master.rs | 70 +++++++++---------- esp-hal-common/src/spi/slave.rs | 16 +++-- .../examples/spi_eh1_device_loopback.rs | 2 +- esp32s2-hal/examples/spi_eh1_loopback.rs | 5 +- .../spi_halfduplex_read_manufacturer_id.rs | 2 +- esp32s2-hal/examples/spi_loopback.rs | 5 +- .../examples/spi_eh1_device_loopback.rs | 2 +- esp32s3-hal/examples/spi_eh1_loopback.rs | 5 +- .../spi_halfduplex_read_manufacturer_id.rs | 2 +- esp32s3-hal/examples/spi_loopback.rs | 5 +- 13 files changed, 87 insertions(+), 80 deletions(-) diff --git a/esp-hal-common/src/aes/mod.rs b/esp-hal-common/src/aes/mod.rs index 681fb91bd6..150da0031e 100644 --- a/esp-hal-common/src/aes/mod.rs +++ b/esp-hal-common/src/aes/mod.rs @@ -1,19 +1,25 @@ //! # Advanced Encryption Standard (AES) support. //! //! ## Overview +//! //! The AES module provides an interface to interact with the AES peripheral, //! provides encryption and decryption capabilities for ESP chips using the AES //! algorithm. We currently support the following AES encryption modes: +//! //! * AES-128 //! * AES-192 //! * AES-256 //! //! ## Example +//! //! ### Initialization +//! //! ```no_run //! let mut aes = Aes::new(peripherals.AES); //! ``` +//! //! ### Creating key and block Buffer +//! //! ```no_run //! let keytext = "SUp4SeCp@sSw0rd".as_bytes(); //! let plaintext = "message".as_bytes(); @@ -28,6 +34,7 @@ //! ``` //! //! ### Encrypting and Decrypting (using hardware) +//! //! ```no_run //! let mut block = block_buf.clone(); //! aes.process(&mut block, Mode::Encryption128, &keybuf); @@ -38,6 +45,7 @@ //! ``` //! //! ### Encrypting and Decrypting (using software) +//! //! ```no_run //! let key = GenericArray::from(keybuf); //! @@ -52,13 +60,19 @@ //! ``` //! //! ### Implementation State +//! //! * DMA mode is currently not supported on ESP32 and ESP32S2 ⚠️ +//! //! ## DMA-AES Mode +//! //! Supports 6 block cipher modes including `ECB/CBC/OFB/CTR/CFB8/CFB128`. +//! //! * Initialization vector (IV) is currently not supported ⚠️ //! //! ## Example -//! ### Initializaton +//! +//! ### Initialization +//! //! ```no_run //! let dma = Gdma::new(peripherals.DMA); //! let dma_channel = dma.channel0; @@ -75,6 +89,7 @@ //! ``` //! //! ### Operation +//! //! ```no_run //! let transfer = aes //! .process( @@ -312,6 +327,8 @@ pub mod dma { self, ) -> Result<(RXBUF, TXBUF, AesDma<'d, C>), (DmaError, RXBUF, TXBUF, AesDma<'d, C>)> { + // Waiting for the DMA transfer is not enough. We need to wait for the + // peripheral to finish flushing its buffers, too. while self.aes_dma.aes.aes.state.read().state().bits() != 2 // DMA status DONE == 2 && !self.aes_dma.channel.tx.is_done() { @@ -320,6 +337,8 @@ pub mod dma { self.aes_dma.finish_transform(); + let err = self.aes_dma.channel.rx.has_error() || self.aes_dma.channel.tx.has_error(); + // `DmaTransferRxTx` needs to have a `Drop` implementation, because we accept // managed buffers that can free their memory on drop. Because of that // we can't move out of the `DmaTransferRxTx`'s fields, so we use `ptr::read` @@ -331,8 +350,6 @@ pub mod dma { let rbuffer = core::ptr::read(&self.rbuffer); let tbuffer = core::ptr::read(&self.tbuffer); let payload = core::ptr::read(&self.aes_dma); - let err = (&self).aes_dma.channel.rx.has_error() - || (&self).aes_dma.channel.tx.has_error(); mem::forget(self); if err { Err((DmaError::DescriptorError, rbuffer, tbuffer, payload)) diff --git a/esp-hal-common/src/i2s.rs b/esp-hal-common/src/i2s.rs index 746a81e53d..3142ed0bd3 100644 --- a/esp-hal-common/src/i2s.rs +++ b/esp-hal-common/src/i2s.rs @@ -393,7 +393,10 @@ where fn wait( self, ) -> Result<(BUFFER, I2sTx<'d, T, P, CH>), (DmaError, BUFFER, I2sTx<'d, T, P, CH>)> { - self.i2s_tx.wait_tx_dma_done().ok(); // waiting for the DMA transfer is not enough + // Waiting for the DMA transfer is not enough. We need to wait for the + // peripheral to finish flushing its buffers, too. + self.i2s_tx.wait_tx_dma_done().ok(); + let err = self.i2s_tx.tx_channel.has_error(); // `DmaTransfer` needs to have a `Drop` implementation, because we accept // managed buffers that can free their memory on drop. Because of that @@ -405,7 +408,6 @@ where unsafe { let buffer = core::ptr::read(&self.buffer); let payload = core::ptr::read(&self.i2s_tx); - let err = (&self).i2s_tx.tx_channel.has_error(); core::mem::forget(self); if err { Err((DmaError::DescriptorError, buffer, payload)) @@ -496,9 +498,11 @@ where dst: &mut [u8], ) -> Result<(BUFFER, I2sRx<'d, T, P, CH>, usize), (DmaError, BUFFER, I2sRx<'d, T, P, CH>, usize)> { - self.i2s_rx.wait_rx_dma_done().ok(); // waiting for the DMA transfer is not enough - + // Waiting for the DMA transfer is not enough. We need to wait for the + // peripheral to finish flushing its buffers, too. + self.i2s_rx.wait_rx_dma_done().ok(); let len = self.i2s_rx.rx_channel.drain_buffer(dst).unwrap(); + let err = self.i2s_rx.rx_channel.has_error(); // `DmaTransfer` needs to have a `Drop` implementation, because we accept // managed buffers that can free their memory on drop. Because of that @@ -510,7 +514,6 @@ where unsafe { let buffer = core::ptr::read(&self.buffer); let payload = core::ptr::read(&self.i2s_rx); - let err = (&self).i2s_rx.rx_channel.has_error(); core::mem::forget(self); if err { Err((DmaError::DescriptorError, buffer, payload, len)) @@ -533,7 +536,10 @@ where fn wait( self, ) -> Result<(BUFFER, I2sRx<'d, T, P, CH>), (DmaError, BUFFER, I2sRx<'d, T, P, CH>)> { - self.i2s_rx.wait_rx_dma_done().ok(); // waiting for the DMA transfer is not enough + // Waiting for the DMA transfer is not enough. We need to wait for the + // peripheral to finish flushing its buffers, too. + self.i2s_rx.wait_rx_dma_done().ok(); + let err = self.i2s_rx.rx_channel.has_error(); // `DmaTransfer` needs to have a `Drop` implementation, because we accept // managed buffers that can free their memory on drop. Because of that @@ -545,7 +551,6 @@ where unsafe { let buffer = core::ptr::read(&self.buffer); let payload = core::ptr::read(&self.i2s_rx); - let err = (&self).i2s_rx.rx_channel.has_error(); core::mem::forget(self); if err { Err((DmaError::DescriptorError, buffer, payload)) diff --git a/esp-hal-common/src/parl_io.rs b/esp-hal-common/src/parl_io.rs index c8da196acd..6fb6904284 100644 --- a/esp-hal-common/src/parl_io.rs +++ b/esp-hal-common/src/parl_io.rs @@ -1247,14 +1247,14 @@ where pub fn wait( self, ) -> Result<(BUFFER, ParlIoTx<'d, C, P, CP>), (DmaError, BUFFER, ParlIoTx<'d, C, P, CP>)> { - loop { - if Instance::is_tx_eof() { - break; - } - } + // Waiting for the DMA transfer is not enough. We need to wait for the + // peripheral to finish flushing its buffers, too. + while !Instance::is_tx_eof() {} Instance::set_tx_start(false); + let err = self.instance.tx_channel.has_error(); + // `DmaTransfer` needs to have a `Drop` implementation, because we accept // managed buffers that can free their memory on drop. Because of that // we can't move out of the `DmaTransfer`'s fields, so we use `ptr::read` @@ -1265,7 +1265,6 @@ where unsafe { let buffer = core::ptr::read(&self.buffer); let payload = core::ptr::read(&self.instance); - let err = (&self).instance.tx_channel.has_error(); mem::forget(self); if err { Err((DmaError::DescriptorError, buffer, payload)) diff --git a/esp-hal-common/src/spi/master.rs b/esp-hal-common/src/spi/master.rs index 690c0076f1..669aea7ddd 100644 --- a/esp-hal-common/src/spi/master.rs +++ b/esp-hal-common/src/spi/master.rs @@ -876,7 +876,10 @@ pub mod dma { (RXBUF, TXBUF, SpiDma<'d, T, C, M>), (DmaError, RXBUF, TXBUF, SpiDma<'d, T, C, M>), > { - self.spi_dma.spi.flush().ok(); // waiting for the DMA transfer is not enough + // Waiting for the DMA transfer is not enough. We need to wait for the + // peripheral to finish flushing its buffers, too. + self.spi_dma.spi.flush().ok(); + let err = self.spi_dma.channel.rx.has_error() || self.spi_dma.channel.tx.has_error(); // `DmaTransfer` needs to have a `Drop` implementation, because we accept // managed buffers that can free their memory on drop. Because of that @@ -889,8 +892,6 @@ pub mod dma { let rbuffer = core::ptr::read(&self.rbuffer); let tbuffer = core::ptr::read(&self.tbuffer); let payload = core::ptr::read(&self.spi_dma); - let err = (&self).spi_dma.channel.rx.has_error() - || (&self).spi_dma.channel.tx.has_error(); mem::forget(self); if err { Err((DmaError::DescriptorError, rbuffer, tbuffer, payload)) @@ -945,7 +946,10 @@ pub mod dma { mut self, ) -> Result<(BUFFER, SpiDma<'d, T, C, M>), (DmaError, BUFFER, SpiDma<'d, T, C, M>)> { - self.spi_dma.spi.flush().ok(); // waiting for the DMA transfer is not enough + // Waiting for the DMA transfer is not enough. We need to wait for the + // peripheral to finish flushing its buffers, too. + self.spi_dma.spi.flush().ok(); + let err = self.spi_dma.channel.rx.has_error() || self.spi_dma.channel.tx.has_error(); // `DmaTransfer` needs to have a `Drop` implementation, because we accept // managed buffers that can free their memory on drop. Because of that @@ -957,8 +961,6 @@ pub mod dma { unsafe { let buffer = core::ptr::read(&self.buffer); let payload = core::ptr::read(&self.spi_dma); - let err = (&self).spi_dma.channel.rx.has_error() - || (&self).spi_dma.channel.tx.has_error(); mem::forget(self); if err { Err((DmaError::DescriptorError, buffer, payload)) @@ -1515,19 +1517,19 @@ pub mod dma { #[cfg(feature = "eh1")] mod ehal1 { - use embedded_hal_1::spi::SpiBus; + use embedded_hal_1::spi::{ErrorType, SpiBus}; use super::{super::InstanceDma, *}; use crate::{dma::ChannelTypes, FlashSafeDma}; - impl<'d, T, C, M> embedded_hal_1::spi::ErrorType for SpiDma<'d, T, C, M> + impl<'d, T, C, M> ErrorType for SpiDma<'d, T, C, M> where T: InstanceDma, C::Rx<'d>>, C: ChannelTypes, C::P: SpiPeripheral, M: IsFullDuplex, { - type Error = super::super::Error; + type Error = Error; } impl<'d, T, C, M> SpiBus for SpiDma<'d, T, C, M> @@ -1574,15 +1576,11 @@ pub mod dma { } } - impl embedded_hal_1::spi::ErrorType - for FlashSafeDma - { + impl ErrorType for FlashSafeDma { type Error = T::Error; } - impl embedded_hal_1::spi::SpiBus - for FlashSafeDma - { + impl SpiBus for FlashSafeDma { fn read(&mut self, words: &mut [u8]) -> Result<(), Self::Error> { self.inner.read(words) } @@ -1851,7 +1849,7 @@ where self.flush().unwrap(); } - return Ok(words); + Ok(words) } fn transfer_dma<'w>( @@ -1888,7 +1886,7 @@ where } } - return Ok(read_buffer); + Ok(read_buffer) } fn start_transfer_dma<'w>( @@ -1946,7 +1944,7 @@ where self.flush().unwrap(); // seems "is_done" doesn't work as intended? } - return Ok(words); + Ok(words) } fn start_write_bytes_dma<'w>( @@ -1976,7 +1974,7 @@ where } reg_block.cmd.modify(|_, w| w.usr().set_bit()); - return Ok(()); + Ok(()) } fn start_read_bytes_dma<'w>( @@ -2006,7 +2004,7 @@ where } reg_block.cmd.modify(|_, w| w.usr().set_bit()); - return Ok(()); + Ok(()) } fn dma_peripheral(&self) -> DmaPeripheral { @@ -2596,24 +2594,22 @@ pub trait Instance { } fn read_byte(&mut self) -> nb::Result { - let reg_block = self.register_block(); - - if reg_block.cmd.read().usr().bit_is_set() { + if self.busy() { return Err(nb::Error::WouldBlock); } + let reg_block = self.register_block(); Ok(u32::try_into(reg_block.w0.read().bits()).unwrap_or_default()) } fn write_byte(&mut self, word: u8) -> nb::Result<(), Error> { - let reg_block = self.register_block(); - - if reg_block.cmd.read().usr().bit_is_set() { + if self.busy() { return Err(nb::Error::WouldBlock); } self.configure_datalen(8); + let reg_block = self.register_block(); reg_block.w0.write(|w| unsafe { w.bits(word.into()) }); self.update(); @@ -2633,7 +2629,6 @@ pub trait Instance { /// [`Self::flush`]. // FIXME: See below. fn write_bytes(&mut self, words: &[u8]) -> Result<(), Error> { - let reg_block = self.register_block(); let num_chunks = words.len() / FIFO_SIZE; // The fifo has a limited fixed size, so the data must be chunked and then @@ -2641,7 +2636,7 @@ pub trait Instance { for (i, chunk) in words.chunks(FIFO_SIZE).enumerate() { self.configure_datalen(chunk.len() as u32 * 8); - let fifo_ptr = reg_block.w0.as_ptr(); + let fifo_ptr = self.register_block().w0.as_ptr(); for i in (0..chunk.len()).step_by(4) { let state = if chunk.len() - i < 4 { chunk.len() % 4 @@ -2673,7 +2668,7 @@ pub trait Instance { self.update(); - reg_block.cmd.modify(|_, w| w.usr().set_bit()); + self.register_block().cmd.modify(|_, w| w.usr().set_bit()); // Wait for all chunks to complete except the last one. // The function is allowed to return before the bus is idle. @@ -2682,9 +2677,7 @@ pub trait Instance { // THIS IS NOT TRUE FOR EH 0.2.X! MAKE SURE TO FLUSH IN EH 0.2.X TRAIT // IMPLEMENTATIONS! if i < num_chunks { - while reg_block.cmd.read().usr().bit_is_set() { - // wait - } + self.flush()?; } } Ok(()) @@ -2738,11 +2731,14 @@ pub trait Instance { Ok(()) } - // Check if the bus is busy and if it is wait for it to be idle - fn flush(&mut self) -> Result<(), Error> { + fn busy(&self) -> bool { let reg_block = self.register_block(); + reg_block.cmd.read().usr().bit_is_set() + } - while reg_block.cmd.read().usr().bit_is_set() { + // Check if the bus is busy and if it is wait for it to be idle + fn flush(&mut self) -> Result<(), Error> { + while self.busy() { // wait for bus to be clear } Ok(()) @@ -2942,9 +2938,7 @@ pub trait Instance { self.configure_datalen(buffer.len() as u32 * 8); self.update(); reg_block.cmd.modify(|_, w| w.usr().set_bit()); - while reg_block.cmd.read().usr().bit_is_set() { - // wait for completion - } + self.flush()?; self.read_bytes_from_fifo(buffer) } diff --git a/esp-hal-common/src/spi/slave.rs b/esp-hal-common/src/spi/slave.rs index a9ee52c98b..1f83980fdf 100644 --- a/esp-hal-common/src/spi/slave.rs +++ b/esp-hal-common/src/spi/slave.rs @@ -247,8 +247,12 @@ pub mod dma { mut self, ) -> Result<(RXBUF, TXBUF, SpiDma<'d, T, C>), (DmaError, RXBUF, TXBUF, SpiDma<'d, T, C>)> { + // Waiting for the DMA transfer is not enough. We need to wait for the + // peripheral to finish flushing its buffers, too. while !self.is_done() {} - self.spi_dma.spi.flush().ok(); // waiting for the DMA transfer is not enough + self.spi_dma.spi.flush().ok(); + + let err = self.spi_dma.channel.rx.has_error() || self.spi_dma.channel.tx.has_error(); // `DmaTransfer` needs to have a `Drop` implementation, because we accept // managed buffers that can free their memory on drop. Because of that @@ -261,8 +265,6 @@ pub mod dma { let rbuffer = core::ptr::read(&self.rbuffer); let tbuffer = core::ptr::read(&self.tbuffer); let payload = core::ptr::read(&self.spi_dma); - let err = (&self).spi_dma.channel.rx.has_error() - || (&self).spi_dma.channel.tx.has_error(); mem::forget(self); if err { Err((DmaError::DescriptorError, rbuffer, tbuffer, payload)) @@ -313,8 +315,12 @@ pub mod dma { fn wait( mut self, ) -> Result<(BUFFER, SpiDma<'d, T, C>), (DmaError, BUFFER, SpiDma<'d, T, C>)> { + // Waiting for the DMA transfer is not enough. We need to wait for the + // peripheral to finish flushing its buffers, too. while !self.is_done() {} - self.spi_dma.spi.flush().ok(); // waiting for the DMA transfer is not enough + self.spi_dma.spi.flush().ok(); + + let err = self.spi_dma.channel.rx.has_error() || self.spi_dma.channel.tx.has_error(); // `DmaTransfer` needs to have a `Drop` implementation, because we accept // managed buffers that can free their memory on drop. Because of that @@ -326,8 +332,6 @@ pub mod dma { unsafe { let buffer = core::ptr::read(&self.buffer); let payload = core::ptr::read(&self.spi_dma); - let err = (&self).spi_dma.channel.rx.has_error() - || (&self).spi_dma.channel.tx.has_error(); mem::forget(self); if err { Err((DmaError::DescriptorError, buffer, payload)) diff --git a/esp32s2-hal/examples/spi_eh1_device_loopback.rs b/esp32s2-hal/examples/spi_eh1_device_loopback.rs index 4feb4d6ba7..0ea37d4049 100644 --- a/esp32s2-hal/examples/spi_eh1_device_loopback.rs +++ b/esp32s2-hal/examples/spi_eh1_device_loopback.rs @@ -25,7 +25,7 @@ use esp32s2_hal::{ peripherals::Peripherals, prelude::*, spi::{ - master::{prelude::*, Spi, SpiBusController}, + master::{Spi, SpiBusController}, SpiMode, }, Delay, diff --git a/esp32s2-hal/examples/spi_eh1_loopback.rs b/esp32s2-hal/examples/spi_eh1_loopback.rs index a086f520e4..d35a4c36a4 100644 --- a/esp32s2-hal/examples/spi_eh1_loopback.rs +++ b/esp32s2-hal/examples/spi_eh1_loopback.rs @@ -22,10 +22,7 @@ use esp32s2_hal::{ gpio::IO, peripherals::Peripherals, prelude::*, - spi::{ - master::{prelude::*, Spi}, - SpiMode, - }, + spi::{master::Spi, SpiMode}, Delay, }; use esp_backtrace as _; diff --git a/esp32s2-hal/examples/spi_halfduplex_read_manufacturer_id.rs b/esp32s2-hal/examples/spi_halfduplex_read_manufacturer_id.rs index 21841924e1..d6db4ee5f7 100644 --- a/esp32s2-hal/examples/spi_halfduplex_read_manufacturer_id.rs +++ b/esp32s2-hal/examples/spi_halfduplex_read_manufacturer_id.rs @@ -23,7 +23,7 @@ use esp32s2_hal::{ peripherals::Peripherals, prelude::*, spi::{ - master::{prelude::*, Address, Command, HalfDuplexReadWrite, Spi}, + master::{Address, Command, HalfDuplexReadWrite, Spi}, SpiDataMode, SpiMode, }, diff --git a/esp32s2-hal/examples/spi_loopback.rs b/esp32s2-hal/examples/spi_loopback.rs index 465af6f9be..bcfdb7531d 100644 --- a/esp32s2-hal/examples/spi_loopback.rs +++ b/esp32s2-hal/examples/spi_loopback.rs @@ -21,10 +21,7 @@ use esp32s2_hal::{ gpio::IO, peripherals::Peripherals, prelude::*, - spi::{ - master::{prelude::*, Spi}, - SpiMode, - }, + spi::{master::Spi, SpiMode}, Delay, }; use esp_backtrace as _; diff --git a/esp32s3-hal/examples/spi_eh1_device_loopback.rs b/esp32s3-hal/examples/spi_eh1_device_loopback.rs index a73080bf61..63e6bbde83 100644 --- a/esp32s3-hal/examples/spi_eh1_device_loopback.rs +++ b/esp32s3-hal/examples/spi_eh1_device_loopback.rs @@ -25,7 +25,7 @@ use esp32s3_hal::{ peripherals::Peripherals, prelude::*, spi::{ - master::{prelude::*, Spi, SpiBusController}, + master::{Spi, SpiBusController}, SpiMode, }, Delay, diff --git a/esp32s3-hal/examples/spi_eh1_loopback.rs b/esp32s3-hal/examples/spi_eh1_loopback.rs index 00a8b3a3a7..6dd026738c 100644 --- a/esp32s3-hal/examples/spi_eh1_loopback.rs +++ b/esp32s3-hal/examples/spi_eh1_loopback.rs @@ -22,10 +22,7 @@ use esp32s3_hal::{ gpio::IO, peripherals::Peripherals, prelude::*, - spi::{ - master::{prelude::*, Spi}, - SpiMode, - }, + spi::{master::Spi, SpiMode}, Delay, }; use esp_backtrace as _; diff --git a/esp32s3-hal/examples/spi_halfduplex_read_manufacturer_id.rs b/esp32s3-hal/examples/spi_halfduplex_read_manufacturer_id.rs index a2545dd293..1b3eefb009 100644 --- a/esp32s3-hal/examples/spi_halfduplex_read_manufacturer_id.rs +++ b/esp32s3-hal/examples/spi_halfduplex_read_manufacturer_id.rs @@ -23,7 +23,7 @@ use esp32s3_hal::{ peripherals::Peripherals, prelude::*, spi::{ - master::{prelude::*, Address, Command, HalfDuplexReadWrite, Spi}, + master::{Address, Command, HalfDuplexReadWrite, Spi}, SpiDataMode, SpiMode, }, diff --git a/esp32s3-hal/examples/spi_loopback.rs b/esp32s3-hal/examples/spi_loopback.rs index 4212c96b8b..8a7bf6b9ba 100644 --- a/esp32s3-hal/examples/spi_loopback.rs +++ b/esp32s3-hal/examples/spi_loopback.rs @@ -21,10 +21,7 @@ use esp32s3_hal::{ gpio::IO, peripherals::Peripherals, prelude::*, - spi::{ - master::{prelude::*, Spi}, - SpiMode, - }, + spi::{master::Spi, SpiMode}, Delay, }; use esp_backtrace as _;