From 8f717c49bb16baa028ad8d99bffdd2985e336088 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 24 Apr 2025 16:58:17 +0200 Subject: [PATCH] Move some code to non-generic impl (#3417) --- esp-hal/CHANGELOG.md | 1 + esp-hal/MIGRATING-1.0.0-beta.0.md | 11 ++ esp-hal/src/i2c/master/mod.rs | 274 ++++++++++++++++-------------- esp-hal/src/spi/master.rs | 35 ++-- esp-hal/src/uart.rs | 206 +++++++++++----------- 5 files changed, 285 insertions(+), 242 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 9f292839d..d3638b010 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -49,6 +49,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Renamed `Flex::enable_input` to `set_input_enable` (#3387) - Make `esp_hal::interrupt::current_runlevel` public under the unstable feature (#3403) - Update `defmt` to 1.0 (#3416) +- `spi::master::Spi::transfer` no longer returns the received data as a slice (#?) ### Fixed diff --git a/esp-hal/MIGRATING-1.0.0-beta.0.md b/esp-hal/MIGRATING-1.0.0-beta.0.md index e60cb2450..cc92c580e 100644 --- a/esp-hal/MIGRATING-1.0.0-beta.0.md +++ b/esp-hal/MIGRATING-1.0.0-beta.0.md @@ -292,6 +292,17 @@ The data and ctrl pins of the camera have been split out into individual `with_* + camera.with_data0(peripherals.GPIO11).with_data1(peripherals.GPIO9).with_dataX(); ``` +## SPI changes + +`Spi::transfer` no longer returns the input buffer. + +```diff +-let received = spi.transfer(&mut data[..]).unwrap(); +-work_with(received); ++spi.transfer(&mut data[..]).unwrap(); ++work_with(&data[..]); +``` + ## Configuration changes Some configuration options are now unstable and they require the `unstable` feature to be diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index a71963a0f..f0111ca35 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -555,11 +555,12 @@ impl embedded_hal::i2c::I2c for I2c<'_, Dm> { address: u8, operations: &mut [embedded_hal::i2c::Operation<'_>], ) -> Result<(), Self::Error> { - self.transaction_impl( - I2cAddress::SevenBit(address), - operations.iter_mut().map(Operation::from), - ) - .inspect_err(|_| self.internal_recover()) + self.driver() + .transaction_impl( + I2cAddress::SevenBit(address), + operations.iter_mut().map(Operation::from), + ) + .inspect_err(|_| self.internal_recover()) } } @@ -842,19 +843,10 @@ impl<'d> I2c<'d, Async> { write_buffer: &[u8], read_buffer: &mut [u8], ) -> Result<(), Error> { - let address = address.into(); - self.driver() - .write(address, write_buffer, true, read_buffer.is_empty()) + .write_read(address.into(), write_buffer, read_buffer) .await - .inspect_err(|_| self.internal_recover())?; - - self.driver() - .read(address, read_buffer, true, true, false) - .await - .inspect_err(|_| self.internal_recover())?; - - Ok(()) + .inspect_err(|_| self.internal_recover()) } /// Execute the provided operations on the I2C bus as a single @@ -889,63 +881,11 @@ impl<'d> I2c<'d, Async> { address: A, operations: impl IntoIterator>, ) -> Result<(), Error> { - self.transaction_impl_async(address.into(), operations.into_iter().map(Operation::from)) + self.driver() + .transaction_impl_async(address.into(), operations.into_iter().map(Operation::from)) .await .inspect_err(|_| self.internal_recover()) } - - async fn transaction_impl_async<'a>( - &mut self, - address: I2cAddress, - operations: impl Iterator>, - ) -> Result<(), Error> { - address.validate()?; - - let mut last_op: Option = None; - // filter out 0 length read operations - let mut op_iter = operations - .filter(|op| op.is_write() || !op.is_empty()) - .peekable(); - - while let Some(op) = op_iter.next() { - let next_op = op_iter.peek().map(|v| v.kind()); - let kind = op.kind(); - match op { - Operation::Write(buffer) => { - // execute a write operation: - // - issue START/RSTART if op is different from previous - // - issue STOP if op is the last one - self.driver() - .write( - address, - buffer, - !matches!(last_op, Some(OpKind::Write)), - next_op.is_none(), - ) - .await?; - } - Operation::Read(buffer) => { - // execute a read operation: - // - issue START/RSTART if op is different from previous - // - issue STOP if op is the last one - // - will_continue is true if there is another read operation next - self.driver() - .read( - address, - buffer, - !matches!(last_op, Some(OpKind::Read)), - next_op.is_none(), - matches!(next_op, Some(OpKind::Read)), - ) - .await?; - } - } - - last_op = Some(kind); - } - - Ok(()) - } } impl<'d, Dm> I2c<'d, Dm> @@ -968,55 +908,6 @@ where _ = self.driver().setup(&self.config); } - fn transaction_impl<'a>( - &mut self, - address: I2cAddress, - operations: impl Iterator>, - ) -> Result<(), Error> { - address.validate()?; - - let mut last_op: Option = None; - // filter out 0 length read operations - let mut op_iter = operations - .filter(|op| op.is_write() || !op.is_empty()) - .peekable(); - - while let Some(op) = op_iter.next() { - let next_op = op_iter.peek().map(|v| v.kind()); - let kind = op.kind(); - match op { - Operation::Write(buffer) => { - // execute a write operation: - // - issue START/RSTART if op is different from previous - // - issue STOP if op is the last one - self.driver().write_blocking( - address, - buffer, - !matches!(last_op, Some(OpKind::Write)), - next_op.is_none(), - )?; - } - Operation::Read(buffer) => { - // execute a read operation: - // - issue START/RSTART if op is different from previous - // - issue STOP if op is the last one - // - will_continue is true if there is another read operation next - self.driver().read_blocking( - address, - buffer, - !matches!(last_op, Some(OpKind::Read)), - next_op.is_none(), - matches!(next_op, Some(OpKind::Read)), - )?; - } - } - - last_op = Some(kind); - } - - Ok(()) - } - /// Connect a pin to the I2C SDA signal. /// /// This will replace previous pin assignments for this signal. @@ -1132,17 +1023,9 @@ where write_buffer: &[u8], read_buffer: &mut [u8], ) -> Result<(), Error> { - let address = address.into(); - self.driver() - .write_blocking(address, write_buffer, true, read_buffer.is_empty()) - .inspect_err(|_| self.internal_recover())?; - - self.driver() - .read_blocking(address, read_buffer, true, true, false) - .inspect_err(|_| self.internal_recover())?; - - Ok(()) + .write_read_blocking(address.into(), write_buffer, read_buffer) + .inspect_err(|_| self.internal_recover()) } /// Execute the provided operations on the I2C bus. @@ -1194,7 +1077,8 @@ where address: A, operations: impl IntoIterator>, ) -> Result<(), Error> { - self.transaction_impl(address.into(), operations.into_iter().map(Operation::from)) + self.driver() + .transaction_impl(address.into(), operations.into_iter().map(Operation::from)) .inspect_err(|_| self.internal_recover()) } @@ -1217,7 +1101,8 @@ impl embedded_hal_async::i2c::I2c for I2c<'_, Async> { address: u8, operations: &mut [EhalOperation<'_>], ) -> Result<(), Self::Error> { - self.transaction_impl_async(address.into(), operations.iter_mut().map(Operation::from)) + self.driver() + .transaction_impl_async(address.into(), operations.iter_mut().map(Operation::from)) .await .inspect_err(|_| self.internal_recover()) } @@ -2613,6 +2498,133 @@ impl Driver<'_> { Ok(()) } + + async fn write_read( + &self, + address: I2cAddress, + write_buffer: &[u8], + read_buffer: &mut [u8], + ) -> Result<(), Error> { + self.write(address, write_buffer, true, read_buffer.is_empty()) + .await?; + + self.read(address, read_buffer, true, true, false).await?; + + Ok(()) + } + + fn write_read_blocking( + &self, + address: I2cAddress, + write_buffer: &[u8], + read_buffer: &mut [u8], + ) -> Result<(), Error> { + self.write_blocking(address, write_buffer, true, read_buffer.is_empty())?; + + self.read_blocking(address, read_buffer, true, true, false)?; + + Ok(()) + } + + fn transaction_impl<'a>( + &self, + address: I2cAddress, + operations: impl Iterator>, + ) -> Result<(), Error> { + address.validate()?; + + let mut last_op: Option = None; + // filter out 0 length read operations + let mut op_iter = operations + .filter(|op| op.is_write() || !op.is_empty()) + .peekable(); + + while let Some(op) = op_iter.next() { + let next_op = op_iter.peek().map(|v| v.kind()); + let kind = op.kind(); + match op { + Operation::Write(buffer) => { + // execute a write operation: + // - issue START/RSTART if op is different from previous + // - issue STOP if op is the last one + self.write_blocking( + address, + buffer, + !matches!(last_op, Some(OpKind::Write)), + next_op.is_none(), + )?; + } + Operation::Read(buffer) => { + // execute a read operation: + // - issue START/RSTART if op is different from previous + // - issue STOP if op is the last one + // - will_continue is true if there is another read operation next + self.read_blocking( + address, + buffer, + !matches!(last_op, Some(OpKind::Read)), + next_op.is_none(), + matches!(next_op, Some(OpKind::Read)), + )?; + } + } + + last_op = Some(kind); + } + + Ok(()) + } + + async fn transaction_impl_async<'a>( + &self, + address: I2cAddress, + operations: impl Iterator>, + ) -> Result<(), Error> { + address.validate()?; + + let mut last_op: Option = None; + // filter out 0 length read operations + let mut op_iter = operations + .filter(|op| op.is_write() || !op.is_empty()) + .peekable(); + + while let Some(op) = op_iter.next() { + let next_op = op_iter.peek().map(|v| v.kind()); + let kind = op.kind(); + match op { + Operation::Write(buffer) => { + // execute a write operation: + // - issue START/RSTART if op is different from previous + // - issue STOP if op is the last one + self.write( + address, + buffer, + !matches!(last_op, Some(OpKind::Write)), + next_op.is_none(), + ) + .await?; + } + Operation::Read(buffer) => { + // execute a read operation: + // - issue START/RSTART if op is different from previous + // - issue STOP if op is the last one + // - will_continue is true if there is another read operation next + self.read( + address, + buffer, + !matches!(last_op, Some(OpKind::Read)), + next_op.is_none(), + matches!(next_op, Some(OpKind::Read)), + ) + .await?; + } + } + + last_op = Some(kind); + } + + Ok(()) + } } fn check_timeout(v: u32, max: u32) -> Result { diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 2251c2417..cb9f8de37 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -829,16 +829,7 @@ impl<'d> Spi<'d, Async> { /// Waits for the completion of previous operations. pub async fn flush_async(&mut self) -> Result<(), Error> { - let driver = self.driver(); - - if !driver.busy() { - return Ok(()); - } - - let future = SpiFuture::setup(&driver).await; - future.await; - - Ok(()) + self.driver().flush_async().await } /// Sends `words` to the slave. Returns the `words` received from the slave. @@ -850,7 +841,7 @@ impl<'d> Spi<'d, Async> { pub async fn transfer_in_place_async(&mut self, words: &mut [u8]) -> Result<(), Error> { // We need to flush because the blocking transfer functions may return while a // transfer is still in progress. - self.flush_async().await?; + self.driver().flush_async().await?; self.driver().setup_full_duplex()?; self.driver().transfer_in_place_async(words).await } @@ -1056,8 +1047,9 @@ where self.driver().read(words) } - /// Sends `words` to the slave. Returns the `words` received from the slave. - pub fn transfer<'w>(&mut self, words: &'w mut [u8]) -> Result<&'w [u8], Error> { + /// Sends `words` to the slave. The received data will be written to + /// `words`, overwriting its contents. + pub fn transfer(&mut self, words: &mut [u8]) -> Result<(), Error> { self.driver().setup_full_duplex()?; self.driver().transfer(words) } @@ -2594,7 +2586,7 @@ mod ehal1 { } fn transfer_in_place(&mut self, words: &mut [u8]) -> Result<(), Self::Error> { - self.driver().transfer(words).map(|_| ()) + self.driver().transfer(words) } fn flush(&mut self) -> Result<(), Self::Error> { @@ -3457,6 +3449,17 @@ impl Driver { self.regs().cmd().read().usr().bit_is_set() } + // Check if the bus is busy and if it is wait for it to be idle + #[cfg_attr(place_spi_master_driver_in_ram, ram)] + async fn flush_async(&self) -> Result<(), Error> { + if self.busy() { + let future = SpiFuture::setup(self).await; + future.await; + } + + Ok(()) + } + // Check if the bus is busy and if it is wait for it to be idle #[cfg_attr(place_spi_master_driver_in_ram, ram)] fn flush(&self) -> Result<(), Error> { @@ -3467,14 +3470,14 @@ impl Driver { } #[cfg_attr(place_spi_master_driver_in_ram, ram)] - fn transfer<'w>(&self, words: &'w mut [u8]) -> Result<&'w [u8], Error> { + fn transfer(&self, words: &mut [u8]) -> Result<(), Error> { for chunk in words.chunks_mut(FIFO_SIZE) { self.write(chunk)?; self.flush()?; self.read_from_fifo(chunk)?; } - Ok(words) + Ok(()) } #[cfg_attr(place_spi_master_driver_in_ram, ram)] diff --git a/esp-hal/src/uart.rs b/esp-hal/src/uart.rs index d86df530d..08d6a629c 100644 --- a/esp-hal/src/uart.rs +++ b/esp-hal/src/uart.rs @@ -623,7 +623,8 @@ impl<'d> UartTx<'d, Async> { // We need to loop in case the TX empty interrupt was fired but not cleared // before, but the FIFO itself was filled up by a previous write. let space = loop { - let space = Info::UART_FIFO_SIZE - self.tx_fifo_count(); + let tx_fifo_count = self.uart.info().tx_fifo_count(); + let space = Info::UART_FIFO_SIZE - tx_fifo_count; if space != 0 { break space; } @@ -633,7 +634,9 @@ impl<'d> UartTx<'d, Async> { let free = (space as usize).min(bytes.len()); for &byte in &bytes[..free] { - self.regs() + self.uart + .info() + .regs() .fifo() .write(|w| unsafe { w.rxfifo_rd_byte().bits(byte) }); } @@ -654,7 +657,7 @@ impl<'d> UartTx<'d, Async> { // Nothing is guaranteed to clear the Done status, so let's loop here in case Tx // was Done before the last write operation that pushed data into the // FIFO. - while self.tx_fifo_count() > 0 { + while self.uart.info().tx_fifo_count() > 0 { UartTxFuture::new(self.uart.reborrow(), TxEvent::Done).await; } @@ -725,33 +728,7 @@ where /// write operation. #[instability::unstable] pub fn write(&mut self, data: &[u8]) -> Result { - if data.is_empty() { - return Ok(0); - } - - while self.tx_fifo_count() >= Info::UART_FIFO_SIZE {} - - let space = ((Info::UART_FIFO_SIZE - self.tx_fifo_count()) as usize).min(data.len()); - for &byte in &data[..space] { - self.write_byte(byte)?; - } - - Ok(space) - } - - fn write_byte(&mut self, word: u8) -> Result<(), TxError> { - self.regs() - .fifo() - .write(|w| unsafe { w.rxfifo_rd_byte().bits(word) }); - - Ok(()) - } - - #[allow(clippy::useless_conversion)] - /// Returns the number of bytes currently in the TX FIFO for this UART - /// instance. - fn tx_fifo_count(&self) -> u16 { - self.regs().status().read().txfifo_cnt().bits().into() + self.uart.info().write(data) } /// Flush the transmit buffer. @@ -760,7 +737,7 @@ where /// transmitted. #[instability::unstable] pub fn flush(&mut self) -> Result<(), TxError> { - while self.tx_fifo_count() > 0 {} + while self.uart.info().tx_fifo_count() > 0 {} self.flush_last_byte(); Ok(()) } @@ -902,7 +879,7 @@ impl<'d> UartRx<'d, Async> { preferred: usize, listen_for_timeout: bool, ) -> Result<(), RxError> { - while self.rx_fifo_count() < (minimum as u16).min(Info::RX_FIFO_MAX_THRHD) { + while self.uart.info().rx_fifo_count() < (minimum as u16).min(Info::RX_FIFO_MAX_THRHD) { let amount = u16::try_from(preferred) .unwrap_or(Info::RX_FIFO_MAX_THRHD) .min(Info::RX_FIFO_MAX_THRHD); @@ -1007,7 +984,7 @@ impl<'d> UartRx<'d, Async> { self.wait_for_buffered_data(buf.len(), buf.len(), false) .await?; - let read = self.read_buffered(buf)?; + let read = self.uart.info().read_buffered(buf)?; buf = &mut buf[read..]; } @@ -1076,20 +1053,7 @@ where /// If a FIFO overflow is detected, the RX FIFO is reset. #[instability::unstable] pub fn check_for_errors(&mut self) -> Result<(), RxError> { - let errors = RxEvent::FifoOvf - | RxEvent::FifoTout - | RxEvent::GlitchDetected - | RxEvent::FrameError - | RxEvent::ParityError; - let events = self.uart.info().rx_events().intersection(errors); - let result = rx_event_check_for_error(events); - if result.is_err() { - self.uart.info().clear_rx_events(errors); - if events.contains(RxEvent::FifoOvf) { - self.uart.info().rxfifo_reset(); - } - } - result + self.uart.info().check_for_errors() } /// Read bytes. @@ -1112,16 +1076,7 @@ where /// the FIFO are not modified. #[instability::unstable] pub fn read(&mut self, buf: &mut [u8]) -> Result { - if buf.is_empty() { - return Ok(0); - } - - while self.rx_fifo_count() == 0 { - // Block until we received at least one byte - self.check_for_errors()?; - } - - self.read_buffered(buf) + self.uart.info().read(buf) } /// Read already received bytes. @@ -1143,43 +1098,7 @@ where /// the FIFO are not modified. #[instability::unstable] pub fn read_buffered(&mut self, buf: &mut [u8]) -> Result { - // Get the count first, to avoid accidentally reading a corrupted byte received - // after the error check. - let to_read = (self.rx_fifo_count() as usize).min(buf.len()); - self.check_for_errors()?; - - for byte_into in buf[..to_read].iter_mut() { - *byte_into = self.uart.info().read_next_from_fifo(); - } - - Ok(to_read) - } - - #[cfg(not(esp32))] - #[allow(clippy::unnecessary_cast)] - fn rx_fifo_count(&self) -> u16 { - self.regs().status().read().rxfifo_cnt().bits() as u16 - } - - #[cfg(esp32)] - fn rx_fifo_count(&self) -> u16 { - let fifo_cnt = self.regs().status().read().rxfifo_cnt().bits(); - - // Calculate the real count based on the FIFO read and write offset address: - // https://docs.espressif.com/projects/esp-chip-errata/en/latest/esp32/03-errata-description/esp32/uart-fifo-cnt-indicates-data-length-incorrectly.html - let status = self.regs().mem_rx_status().read(); - let rd_addr: u16 = status.mem_rx_rd_addr().bits(); - let wr_addr: u16 = status.mem_rx_wr_addr().bits(); - - if wr_addr > rd_addr { - wr_addr - rd_addr - } else if wr_addr < rd_addr { - (wr_addr + Info::UART_FIFO_SIZE) - rd_addr - } else if fifo_cnt > 0 { - Info::UART_FIFO_SIZE - } else { - 0 - } + self.uart.info().read_buffered(buf) } /// Disables all RX-related interrupts for this UART instance. @@ -1895,7 +1814,7 @@ where Dm: DriverMode, { fn read_ready(&mut self) -> Result { - Ok(self.rx_fifo_count() > 0) + Ok(self.uart.info().rx_fifo_count() > 0) } } @@ -2970,6 +2889,103 @@ impl Info { access_fifo_register(|| fifo_reg.read().rxfifo_rd_byte().bits()) } + + #[allow(clippy::useless_conversion)] + fn tx_fifo_count(&self) -> u16 { + u16::from(self.regs().status().read().txfifo_cnt().bits()) + } + + fn write_byte(&self, byte: u8) { + self.regs() + .fifo() + .write(|w| unsafe { w.rxfifo_rd_byte().bits(byte) }); + } + + fn check_for_errors(&self) -> Result<(), RxError> { + let errors = RxEvent::FifoOvf + | RxEvent::FifoTout + | RxEvent::GlitchDetected + | RxEvent::FrameError + | RxEvent::ParityError; + let events = self.rx_events().intersection(errors); + let result = rx_event_check_for_error(events); + if result.is_err() { + self.clear_rx_events(errors); + if events.contains(RxEvent::FifoOvf) { + self.rxfifo_reset(); + } + } + result + } + + #[cfg(not(esp32))] + #[allow(clippy::unnecessary_cast)] + fn rx_fifo_count(&self) -> u16 { + self.regs().status().read().rxfifo_cnt().bits() as u16 + } + + #[cfg(esp32)] + fn rx_fifo_count(&self) -> u16 { + let fifo_cnt = self.regs().status().read().rxfifo_cnt().bits(); + + // Calculate the real count based on the FIFO read and write offset address: + // https://docs.espressif.com/projects/esp-chip-errata/en/latest/esp32/03-errata-description/esp32/uart-fifo-cnt-indicates-data-length-incorrectly.html + let status = self.regs().mem_rx_status().read(); + let rd_addr: u16 = status.mem_rx_rd_addr().bits(); + let wr_addr: u16 = status.mem_rx_wr_addr().bits(); + + if wr_addr > rd_addr { + wr_addr - rd_addr + } else if wr_addr < rd_addr { + (wr_addr + Info::UART_FIFO_SIZE) - rd_addr + } else if fifo_cnt > 0 { + Info::UART_FIFO_SIZE + } else { + 0 + } + } + + fn write(&self, data: &[u8]) -> Result { + if data.is_empty() { + return Ok(0); + } + + while self.tx_fifo_count() >= Info::UART_FIFO_SIZE {} + + let space = (Info::UART_FIFO_SIZE - self.tx_fifo_count()) as usize; + let to_write = space.min(data.len()); + for &byte in &data[..to_write] { + self.write_byte(byte); + } + + Ok(to_write) + } + + fn read(&self, buf: &mut [u8]) -> Result { + if buf.is_empty() { + return Ok(0); + } + + while self.rx_fifo_count() == 0 { + // Block until we received at least one byte + self.check_for_errors()?; + } + + self.read_buffered(buf) + } + + fn read_buffered(&self, buf: &mut [u8]) -> Result { + // Get the count first, to avoid accidentally reading a corrupted byte received + // after the error check. + let to_read = (self.rx_fifo_count() as usize).min(buf.len()); + self.check_for_errors()?; + + for byte_into in buf[..to_read].iter_mut() { + *byte_into = self.read_next_from_fifo(); + } + + Ok(to_read) + } } impl PartialEq for Info {