From 0ad45eba3e9ee7ff3a772baebf4a647b6166b6bc Mon Sep 17 00:00:00 2001 From: Alex Moon Date: Wed, 14 Aug 2024 17:07:57 -0400 Subject: [PATCH 1/3] Add support for transactions to Twim in embassy-nrf --- embassy-nrf/src/twim.rs | 621 ++++++++++++++++++++++++---------------- 1 file changed, 377 insertions(+), 244 deletions(-) diff --git a/embassy-nrf/src/twim.rs b/embassy-nrf/src/twim.rs index c64743ecc3..d0518807c1 100644 --- a/embassy-nrf/src/twim.rs +++ b/embassy-nrf/src/twim.rs @@ -4,6 +4,7 @@ use core::future::{poll_fn, Future}; use core::marker::PhantomData; +use core::mem::MaybeUninit; use core::sync::atomic::compiler_fence; use core::sync::atomic::Ordering::SeqCst; use core::task::Poll; @@ -13,11 +14,12 @@ use embassy_hal_internal::{into_ref, PeripheralRef}; use embassy_sync::waitqueue::AtomicWaker; #[cfg(feature = "time")] use embassy_time::{Duration, Instant}; +use embedded_hal_1::i2c::Operation; use crate::chip::{EASY_DMA_SIZE, FORCE_COPY_BUFFER_SIZE}; use crate::gpio::Pin as GpioPin; use crate::interrupt::typelevel::Interrupt; -use crate::util::{slice_in_ram, slice_in_ram_or}; +use crate::util::slice_in_ram; use crate::{gpio, interrupt, pac, Peripheral}; /// TWI frequency @@ -103,6 +105,18 @@ impl interrupt::typelevel::Handler for InterruptHandl let r = T::regs(); let s = T::state(); + // Workaround for lack of LASTRX_SUSPEND short in some nRF chips + // Do this first to minimize latency + #[cfg(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120"))] + if r.events_lastrx.read().bits() != 0 { + r.tasks_suspend.write(|w| unsafe { w.bits(1) }); + r.events_lastrx.reset(); + } + + if r.events_suspended.read().bits() != 0 { + s.end_waker.wake(); + r.intenclr.write(|w| w.suspended().clear()); + } if r.events_stopped.read().bits() != 0 { s.end_waker.wake(); r.intenclr.write(|w| w.stopped().clear()); @@ -182,8 +196,22 @@ impl<'d, T: Instance> Twim<'d, T> { } /// Set TX buffer, checking that it is in RAM and has suitable length. - unsafe fn set_tx_buffer(&mut self, buffer: &[u8]) -> Result<(), Error> { - slice_in_ram_or(buffer, Error::BufferNotInRAM)?; + unsafe fn set_tx_buffer( + &mut self, + buffer: &[u8], + ram_buffer: Option<&mut [MaybeUninit; FORCE_COPY_BUFFER_SIZE]>, + ) -> Result<(), Error> { + let buffer = if slice_in_ram(buffer) { + buffer + } else { + let ram_buffer = ram_buffer.ok_or(Error::BufferNotInRAM)?; + trace!("Copying TWIM tx buffer into RAM for DMA"); + let ram_buffer = &mut ram_buffer[..buffer.len()]; + // Inline implementation of the nightly API MaybeUninit::copy_from_slice(ram_buffer, buffer) + let uninit_src: &[MaybeUninit] = unsafe { core::mem::transmute(buffer) }; + ram_buffer.copy_from_slice(uninit_src); + unsafe { &*(ram_buffer as *const [MaybeUninit] as *const [u8]) } + }; if buffer.len() > EASY_DMA_SIZE { return Err(Error::TxBufferTooLong); @@ -290,7 +318,7 @@ impl<'d, T: Instance> Twim<'d, T> { fn blocking_wait(&mut self) { let r = T::regs(); loop { - if r.events_stopped.read().bits() != 0 { + if r.events_suspended.read().bits() != 0 || r.events_stopped.read().bits() != 0 { r.events_stopped.reset(); break; } @@ -307,7 +335,7 @@ impl<'d, T: Instance> Twim<'d, T> { let r = T::regs(); let deadline = Instant::now() + timeout; loop { - if r.events_stopped.read().bits() != 0 { + if r.events_suspended.read().bits() != 0 || r.events_stopped.read().bits() != 0 { r.events_stopped.reset(); break; } @@ -331,7 +359,7 @@ impl<'d, T: Instance> Twim<'d, T> { let s = T::state(); s.end_waker.register(cx.waker()); - if r.events_stopped.read().bits() != 0 { + if r.events_suspended.read().bits() != 0 || r.events_stopped.read().bits() != 0 { r.events_stopped.reset(); return Poll::Ready(()); @@ -347,168 +375,357 @@ impl<'d, T: Instance> Twim<'d, T> { }) } - fn setup_write_from_ram(&mut self, address: u8, buffer: &[u8], inten: bool) -> Result<(), Error> { + fn setup_operations( + &mut self, + address: u8, + operations: &mut [Operation<'_>], + tx_ram_buffer: Option<&mut [MaybeUninit; FORCE_COPY_BUFFER_SIZE]>, + inten: bool, + stop: bool, + ) -> Result<(), Error> { let r = T::regs(); compiler_fence(SeqCst); r.address.write(|w| unsafe { w.address().bits(address) }); - // Set up the DMA write. - unsafe { self.set_tx_buffer(buffer)? }; - - // Clear events + let was_suspended = r.events_suspended.read().bits() != 0; + r.events_suspended.reset(); r.events_stopped.reset(); r.events_error.reset(); - r.events_lasttx.reset(); self.clear_errorsrc(); if inten { - r.intenset.write(|w| w.stopped().set().error().set()); + r.intenset.write(|w| w.suspended().set().stopped().set().error().set()); } else { - r.intenclr.write(|w| w.stopped().clear().error().clear()); + r.intenclr + .write(|w| w.suspended().clear().stopped().clear().error().clear()); } + #[cfg(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120"))] + r.intenclr.write(|w| w.lastrx().clear()); + + match operations { + [Operation::Read(rd_buffer), Operation::Write(wr_buffer), rest @ ..] + if !rd_buffer.is_empty() && !wr_buffer.is_empty() => + { + let stop = stop && rest.is_empty(); + + // Set up DMA buffers. + unsafe { + self.set_tx_buffer(wr_buffer, tx_ram_buffer)?; + self.set_rx_buffer(rd_buffer)?; + } - // Start write operation. - r.shorts.write(|w| w.lasttx_stop().enabled()); - r.tasks_starttx.write(|w| unsafe { w.bits(1) }); - if buffer.is_empty() { - // With a zero-length buffer, LASTTX doesn't fire (because there's no last byte!), so do the STOP ourselves. - r.tasks_stop.write(|w| unsafe { w.bits(1) }); - } - Ok(()) - } + r.shorts.write(|w| { + w.lastrx_starttx().enabled(); + if stop { + w.lasttx_stop().enabled(); + } else { + w.lasttx_suspend().enabled(); + } + w + }); + + // Start read+write operation. + r.tasks_startrx.write(|w| unsafe { w.bits(1) }); + + if was_suspended { + r.tasks_resume.write(|w| unsafe { w.bits(1) }); + } + } + [Operation::Write(wr_buffer), Operation::Read(rd_buffer), rest @ ..] + if !wr_buffer.is_empty() && !rd_buffer.is_empty() => + { + let stop = stop && rest.is_empty(); + + // Set up DMA buffers. + unsafe { + self.set_tx_buffer(wr_buffer, tx_ram_buffer)?; + self.set_rx_buffer(rd_buffer)?; + } - fn setup_read(&mut self, address: u8, buffer: &mut [u8], inten: bool) -> Result<(), Error> { - let r = T::regs(); + // Start write+read operation. + r.shorts.write(|w| { + w.lasttx_startrx().enabled(); + if stop { + w.lastrx_stop().enabled(); + } else { + #[cfg(not(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120")))] + w.lastrx_suspend().enabled(); + } + w + }); + #[cfg(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120"))] + if !stop { + r.intenset.write(|w| w.lastrx().set()); + } - compiler_fence(SeqCst); + r.tasks_starttx.write(|w| unsafe { w.bits(1) }); - r.address.write(|w| unsafe { w.address().bits(address) }); + if was_suspended { + r.tasks_resume.write(|w| unsafe { w.bits(1) }); + } + } + [Operation::Read(buffer), rest @ ..] => { + let stop = stop && rest.is_empty(); - // Set up the DMA read. - unsafe { self.set_rx_buffer(buffer)? }; + // Set up DMA buffers. + unsafe { + self.set_rx_buffer(buffer)?; + } - // Clear events - r.events_stopped.reset(); - r.events_error.reset(); - self.clear_errorsrc(); + // Start read operation. + r.shorts.write(|w| { + if stop { + w.lastrx_stop().enabled(); + } else { + #[cfg(not(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120")))] + w.lastrx_suspend().enabled(); + } + w + }); + #[cfg(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120"))] + if !stop { + r.intenset.write(|w| w.lastrx().set()); + } - if inten { - r.intenset.write(|w| w.stopped().set().error().set()); - } else { - r.intenclr.write(|w| w.stopped().clear().error().clear()); - } + r.tasks_startrx.write(|w| unsafe { w.bits(1) }); - // Start read operation. - r.shorts.write(|w| w.lastrx_stop().enabled()); - r.tasks_startrx.write(|w| unsafe { w.bits(1) }); - if buffer.is_empty() { - // With a zero-length buffer, LASTRX doesn't fire (because there's no last byte!), so do the STOP ourselves. - r.tasks_stop.write(|w| unsafe { w.bits(1) }); + if was_suspended { + r.tasks_resume.write(|w| unsafe { w.bits(1) }); + } + + if buffer.is_empty() { + // With a zero-length buffer, LASTRX doesn't fire (because there's no last byte!), so do the STOP/SUSPEND ourselves. + if stop { + r.tasks_stop.write(|w| unsafe { w.bits(1) }); + } else { + r.tasks_suspend.write(|w| unsafe { w.bits(1) }); + } + } + } + [Operation::Write(buffer), rest @ ..] => { + let stop = stop && rest.is_empty(); + + // Set up DMA buffers. + unsafe { + self.set_tx_buffer(buffer, tx_ram_buffer)?; + } + + // Start write operation. + r.shorts.write(|w| { + if stop { + w.lasttx_stop().enabled(); + } else { + w.lasttx_suspend().enabled(); + } + w + }); + + r.tasks_starttx.write(|w| unsafe { w.bits(1) }); + + if was_suspended { + r.tasks_resume.write(|w| unsafe { w.bits(1) }); + } + + if buffer.is_empty() { + // With a zero-length buffer, LASTTX doesn't fire (because there's no last byte!), so do the STOP/SUSPEND ourselves. + if stop { + r.tasks_stop.write(|w| unsafe { w.bits(1) }); + } else { + r.tasks_suspend.write(|w| unsafe { w.bits(1) }); + } + } + } + [] => { + if stop { + if was_suspended { + r.tasks_resume.write(|w| unsafe { w.bits(1) }); + } + + r.tasks_stop.write(|w| unsafe { w.bits(1) }); + } + } } + Ok(()) } - fn setup_write_read_from_ram( - &mut self, - address: u8, - wr_buffer: &[u8], - rd_buffer: &mut [u8], - inten: bool, - ) -> Result<(), Error> { - let r = T::regs(); - + fn check_operations(&mut self, operations: &mut [Operation<'_>]) -> Result { compiler_fence(SeqCst); + self.check_errorsrc()?; - r.address.write(|w| unsafe { w.address().bits(address) }); - - // Set up DMA buffers. - unsafe { - self.set_tx_buffer(wr_buffer)?; - self.set_rx_buffer(rd_buffer)?; + match operations { + [Operation::Read(rd_buffer), Operation::Write(wr_buffer), ..] + | [Operation::Write(wr_buffer), Operation::Read(rd_buffer), ..] + if !rd_buffer.is_empty() && !wr_buffer.is_empty() => + { + self.check_tx(wr_buffer.len())?; + self.check_rx(rd_buffer.len())?; + Ok(2) + } + [Operation::Read(buffer), ..] => { + self.check_rx(buffer.len())?; + Ok(1) + } + [Operation::Write(buffer), ..] => { + self.check_tx(buffer.len())?; + Ok(1) + } + [] => Ok(0), } + } - // Clear events - r.events_stopped.reset(); - r.events_error.reset(); - self.clear_errorsrc(); + // =========================================== - if inten { - r.intenset.write(|w| w.stopped().set().error().set()); - } else { - r.intenclr.write(|w| w.stopped().clear().error().clear()); + /// Execute the provided operations on the I2C bus. + /// + /// Each buffer must have a length of at most 255 bytes on the nRF52832 + /// and at most 65535 bytes on the nRF52840. + /// + /// If `stop` is set, the transaction will be terminated with a STOP + /// condition and the Twim will be stopped. Otherwise, the bus will be + /// left busy via clock stretching and Twim will be suspended. + /// + /// The nrf52832, nrf5340, and nrf9120 do not have hardware support for + /// suspending following a read operation therefore it is emulated by the + /// interrupt handler. If the latency of servicing that interrupt is + /// longer than a byte worth of clocks on the bus, the SCL clock will + /// continue to run for one or more additional bytes. This applies to + /// consecutive read operations, certain write-read-write sequences, or + /// any sequence of operations ending in a read when `stop == false`. + pub fn blocking_transaction( + &mut self, + address: u8, + mut operations: &mut [Operation<'_>], + stop: bool, + ) -> Result<(), Error> { + let mut tx_ram_buffer = [MaybeUninit::uninit(); FORCE_COPY_BUFFER_SIZE]; + while !operations.is_empty() { + self.setup_operations(address, operations, Some(&mut tx_ram_buffer), false, stop)?; + self.blocking_wait(); + let consumed = self.check_operations(operations)?; + operations = &mut operations[consumed..]; + } + Ok(()) + } + + /// Same as [`blocking_transaction`](Twim::blocking_transaction) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. + pub fn blocking_transaction_from_ram( + &mut self, + address: u8, + mut operations: &mut [Operation<'_>], + stop: bool, + ) -> Result<(), Error> { + while !operations.is_empty() { + self.setup_operations(address, operations, None, false, stop)?; + self.blocking_wait(); + let consumed = self.check_operations(operations)?; + operations = &mut operations[consumed..]; } + Ok(()) + } - // Start write+read operation. - r.shorts.write(|w| { - w.lasttx_startrx().enabled(); - w.lastrx_stop().enabled(); - w - }); - r.tasks_starttx.write(|w| unsafe { w.bits(1) }); - if wr_buffer.is_empty() && rd_buffer.is_empty() { - // With a zero-length buffer, LASTRX/LASTTX doesn't fire (because there's no last byte!), so do the STOP ourselves. - // TODO handle when only one of the buffers is zero length - r.tasks_stop.write(|w| unsafe { w.bits(1) }); + /// Execute the provided operations on the I2C bus with timeout. + /// + /// See [`blocking_transaction`]. + #[cfg(feature = "time")] + pub fn blocking_transaction_timeout( + &mut self, + address: u8, + mut operations: &mut [Operation<'_>], + stop: bool, + timeout: Duration, + ) -> Result<(), Error> { + let mut tx_ram_buffer = [MaybeUninit::uninit(); FORCE_COPY_BUFFER_SIZE]; + while !operations.is_empty() { + self.setup_operations(address, operations, Some(&mut tx_ram_buffer), false, stop)?; + self.blocking_wait_timeout(timeout)?; + let consumed = self.check_operations(operations)?; + operations = &mut operations[consumed..]; } + Ok(()) + } + /// Same as [`blocking_transaction_timeout`](Twim::blocking_transaction_timeout) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. + #[cfg(feature = "time")] + pub fn blocking_transaction_from_ram_timeout( + &mut self, + address: u8, + mut operations: &mut [Operation<'_>], + stop: bool, + timeout: Duration, + ) -> Result<(), Error> { + while !operations.is_empty() { + self.setup_operations(address, operations, None, false, stop)?; + self.blocking_wait_timeout(timeout)?; + let consumed = self.check_operations(operations)?; + operations = &mut operations[consumed..]; + } Ok(()) } - fn setup_write_read( + /// Execute the provided operations on the I2C bus. + /// + /// Each buffer must have a length of at most 255 bytes on the nRF52832 + /// and at most 65535 bytes on the nRF52840. + /// + /// If `stop` is set, the transaction will be terminated with a STOP + /// condition and the Twim will be stopped. Otherwise, the bus will be + /// left busy via clock stretching and Twim will be suspended. + /// + /// The nrf52832, nrf5340, and nrf9120 do not have hardware support for + /// suspending following a read operation therefore it is emulated by the + /// interrupt handler. If the latency of servicing that interrupt is + /// longer than a byte worth of clocks on the bus, the SCL clock will + /// continue to run for one or more additional bytes. This applies to + /// consecutive read operations, certain write-read-write sequences, or + /// any sequence of operations ending in a read when `stop == false`. + pub async fn transaction( &mut self, address: u8, - wr_buffer: &[u8], - rd_buffer: &mut [u8], - inten: bool, + mut operations: &mut [Operation<'_>], + stop: bool, ) -> Result<(), Error> { - match self.setup_write_read_from_ram(address, wr_buffer, rd_buffer, inten) { - Ok(_) => Ok(()), - Err(Error::BufferNotInRAM) => { - trace!("Copying TWIM tx buffer into RAM for DMA"); - let tx_ram_buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..wr_buffer.len()]; - tx_ram_buf.copy_from_slice(wr_buffer); - self.setup_write_read_from_ram(address, tx_ram_buf, rd_buffer, inten) - } - Err(error) => Err(error), + let mut tx_ram_buffer = [MaybeUninit::uninit(); FORCE_COPY_BUFFER_SIZE]; + while !operations.is_empty() { + self.setup_operations(address, operations, Some(&mut tx_ram_buffer), true, stop)?; + self.async_wait().await; + let consumed = self.check_operations(operations)?; + operations = &mut operations[consumed..]; } + Ok(()) } - fn setup_write(&mut self, address: u8, wr_buffer: &[u8], inten: bool) -> Result<(), Error> { - match self.setup_write_from_ram(address, wr_buffer, inten) { - Ok(_) => Ok(()), - Err(Error::BufferNotInRAM) => { - trace!("Copying TWIM tx buffer into RAM for DMA"); - let tx_ram_buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..wr_buffer.len()]; - tx_ram_buf.copy_from_slice(wr_buffer); - self.setup_write_from_ram(address, tx_ram_buf, inten) - } - Err(error) => Err(error), + /// Same as [`transaction`](Twim::transaction) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. + pub async fn transaction_from_ram( + &mut self, + address: u8, + mut operations: &mut [Operation<'_>], + stop: bool, + ) -> Result<(), Error> { + while !operations.is_empty() { + self.setup_operations(address, operations, None, true, stop)?; + self.async_wait().await; + let consumed = self.check_operations(operations)?; + operations = &mut operations[consumed..]; } + Ok(()) } + // =========================================== + /// Write to an I2C slave. /// /// The buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub fn blocking_write(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { - self.setup_write(address, buffer, false)?; - self.blocking_wait(); - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(buffer.len())?; - Ok(()) + self.blocking_transaction(address, &mut [Operation::Write(buffer)], true) } /// Same as [`blocking_write`](Twim::blocking_write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub fn blocking_write_from_ram(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { - self.setup_write_from_ram(address, buffer, false)?; - self.blocking_wait(); - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(buffer.len())?; - Ok(()) + self.blocking_transaction_from_ram(address, &mut [Operation::Write(buffer)], true) } /// Read from an I2C slave. @@ -516,12 +733,7 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub fn blocking_read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error> { - self.setup_read(address, buffer, false)?; - self.blocking_wait(); - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_rx(buffer.len())?; - Ok(()) + self.blocking_transaction(address, &mut [Operation::Read(buffer)], true) } /// Write data to an I2C slave, then read data from the slave without @@ -530,13 +742,11 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffers must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub fn blocking_write_read(&mut self, address: u8, wr_buffer: &[u8], rd_buffer: &mut [u8]) -> Result<(), Error> { - self.setup_write_read(address, wr_buffer, rd_buffer, false)?; - self.blocking_wait(); - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(wr_buffer.len())?; - self.check_rx(rd_buffer.len())?; - Ok(()) + self.blocking_transaction( + address, + &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], + true, + ) } /// Same as [`blocking_write_read`](Twim::blocking_write_read) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. @@ -546,13 +756,11 @@ impl<'d, T: Instance> Twim<'d, T> { wr_buffer: &[u8], rd_buffer: &mut [u8], ) -> Result<(), Error> { - self.setup_write_read_from_ram(address, wr_buffer, rd_buffer, false)?; - self.blocking_wait(); - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(wr_buffer.len())?; - self.check_rx(rd_buffer.len())?; - Ok(()) + self.blocking_transaction_from_ram( + address, + &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], + true, + ) } // =========================================== @@ -562,12 +770,7 @@ impl<'d, T: Instance> Twim<'d, T> { /// See [`blocking_write`]. #[cfg(feature = "time")] pub fn blocking_write_timeout(&mut self, address: u8, buffer: &[u8], timeout: Duration) -> Result<(), Error> { - self.setup_write(address, buffer, false)?; - self.blocking_wait_timeout(timeout)?; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(buffer.len())?; - Ok(()) + self.blocking_transaction_timeout(address, &mut [Operation::Write(buffer)], true, timeout) } /// Same as [`blocking_write`](Twim::blocking_write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. @@ -578,12 +781,7 @@ impl<'d, T: Instance> Twim<'d, T> { buffer: &[u8], timeout: Duration, ) -> Result<(), Error> { - self.setup_write_from_ram(address, buffer, false)?; - self.blocking_wait_timeout(timeout)?; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(buffer.len())?; - Ok(()) + self.blocking_transaction_from_ram_timeout(address, &mut [Operation::Write(buffer)], true, timeout) } /// Read from an I2C slave. @@ -592,12 +790,7 @@ impl<'d, T: Instance> Twim<'d, T> { /// and at most 65535 bytes on the nRF52840. #[cfg(feature = "time")] pub fn blocking_read_timeout(&mut self, address: u8, buffer: &mut [u8], timeout: Duration) -> Result<(), Error> { - self.setup_read(address, buffer, false)?; - self.blocking_wait_timeout(timeout)?; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_rx(buffer.len())?; - Ok(()) + self.blocking_transaction_timeout(address, &mut [Operation::Read(buffer)], true, timeout) } /// Write data to an I2C slave, then read data from the slave without @@ -613,13 +806,12 @@ impl<'d, T: Instance> Twim<'d, T> { rd_buffer: &mut [u8], timeout: Duration, ) -> Result<(), Error> { - self.setup_write_read(address, wr_buffer, rd_buffer, false)?; - self.blocking_wait_timeout(timeout)?; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(wr_buffer.len())?; - self.check_rx(rd_buffer.len())?; - Ok(()) + self.blocking_transaction_timeout( + address, + &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], + true, + timeout, + ) } /// Same as [`blocking_write_read`](Twim::blocking_write_read) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. @@ -631,13 +823,12 @@ impl<'d, T: Instance> Twim<'d, T> { rd_buffer: &mut [u8], timeout: Duration, ) -> Result<(), Error> { - self.setup_write_read_from_ram(address, wr_buffer, rd_buffer, false)?; - self.blocking_wait_timeout(timeout)?; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(wr_buffer.len())?; - self.check_rx(rd_buffer.len())?; - Ok(()) + self.blocking_transaction_from_ram_timeout( + address, + &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], + true, + timeout, + ) } // =========================================== @@ -647,12 +838,7 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub async fn read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error> { - self.setup_read(address, buffer, true)?; - self.async_wait().await; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_rx(buffer.len())?; - Ok(()) + self.transaction(address, &mut [Operation::Read(buffer)], true).await } /// Write to an I2C slave. @@ -660,22 +846,13 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub async fn write(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { - self.setup_write(address, buffer, true)?; - self.async_wait().await; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(buffer.len())?; - Ok(()) + self.transaction(address, &mut [Operation::Write(buffer)], true).await } /// Same as [`write`](Twim::write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub async fn write_from_ram(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { - self.setup_write_from_ram(address, buffer, true)?; - self.async_wait().await; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(buffer.len())?; - Ok(()) + self.transaction_from_ram(address, &mut [Operation::Write(buffer)], true) + .await } /// Write data to an I2C slave, then read data from the slave without @@ -684,13 +861,12 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffers must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub async fn write_read(&mut self, address: u8, wr_buffer: &[u8], rd_buffer: &mut [u8]) -> Result<(), Error> { - self.setup_write_read(address, wr_buffer, rd_buffer, true)?; - self.async_wait().await; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(wr_buffer.len())?; - self.check_rx(rd_buffer.len())?; - Ok(()) + self.transaction( + address, + &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], + true, + ) + .await } /// Same as [`write_read`](Twim::write_read) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. @@ -700,13 +876,12 @@ impl<'d, T: Instance> Twim<'d, T> { wr_buffer: &[u8], rd_buffer: &mut [u8], ) -> Result<(), Error> { - self.setup_write_read_from_ram(address, wr_buffer, rd_buffer, true)?; - self.async_wait().await; - compiler_fence(SeqCst); - self.check_errorsrc()?; - self.check_tx(wr_buffer.len())?; - self.check_rx(rd_buffer.len())?; - Ok(()) + self.transaction_from_ram( + address, + &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], + true, + ) + .await } } @@ -777,16 +952,7 @@ mod eh02 { type Error = Error; fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Error> { - if slice_in_ram(bytes) { - self.blocking_write(addr, bytes) - } else { - let buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..]; - for chunk in bytes.chunks(FORCE_COPY_BUFFER_SIZE) { - buf[..chunk.len()].copy_from_slice(chunk); - self.blocking_write(addr, &buf[..chunk.len()])?; - } - Ok(()) - } + self.blocking_write(addr, bytes) } } @@ -832,47 +998,14 @@ impl<'d, T: Instance> embedded_hal_1::i2c::ErrorType for Twim<'d, T> { } impl<'d, T: Instance> embedded_hal_1::i2c::I2c for Twim<'d, T> { - fn read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Self::Error> { - self.blocking_read(address, buffer) - } - - fn write(&mut self, address: u8, buffer: &[u8]) -> Result<(), Self::Error> { - self.blocking_write(address, buffer) - } - - fn write_read(&mut self, address: u8, wr_buffer: &[u8], rd_buffer: &mut [u8]) -> Result<(), Self::Error> { - self.blocking_write_read(address, wr_buffer, rd_buffer) - } - - fn transaction( - &mut self, - _address: u8, - _operations: &mut [embedded_hal_1::i2c::Operation<'_>], - ) -> Result<(), Self::Error> { - todo!(); + fn transaction(&mut self, address: u8, operations: &mut [Operation<'_>]) -> Result<(), Self::Error> { + self.blocking_transaction(address, operations, true) } } impl<'d, T: Instance> embedded_hal_async::i2c::I2c for Twim<'d, T> { - async fn read(&mut self, address: u8, read: &mut [u8]) -> Result<(), Self::Error> { - self.read(address, read).await - } - - async fn write(&mut self, address: u8, write: &[u8]) -> Result<(), Self::Error> { - self.write(address, write).await - } - async fn write_read(&mut self, address: u8, write: &[u8], read: &mut [u8]) -> Result<(), Self::Error> { - self.write_read(address, write, read).await - } - - async fn transaction( - &mut self, - address: u8, - operations: &mut [embedded_hal_1::i2c::Operation<'_>], - ) -> Result<(), Self::Error> { - let _ = address; - let _ = operations; - todo!() + async fn transaction(&mut self, address: u8, operations: &mut [Operation<'_>]) -> Result<(), Self::Error> { + self.transaction(address, operations, true).await } } From f83bf016ef2f0010e66c3b2819fc927aae5c356e Mon Sep 17 00:00:00 2001 From: Alex Moon Date: Mon, 19 Aug 2024 16:35:13 -0400 Subject: [PATCH 2/3] Remove support for consecutive Read operations Due to Twim hardware limitations --- embassy-nrf/src/twim.rs | 189 ++++++++++++---------------------------- 1 file changed, 57 insertions(+), 132 deletions(-) diff --git a/embassy-nrf/src/twim.rs b/embassy-nrf/src/twim.rs index d0518807c1..4a239e8b51 100644 --- a/embassy-nrf/src/twim.rs +++ b/embassy-nrf/src/twim.rs @@ -105,14 +105,6 @@ impl interrupt::typelevel::Handler for InterruptHandl let r = T::regs(); let s = T::state(); - // Workaround for lack of LASTRX_SUSPEND short in some nRF chips - // Do this first to minimize latency - #[cfg(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120"))] - if r.events_lastrx.read().bits() != 0 { - r.tasks_suspend.write(|w| unsafe { w.bits(1) }); - r.events_lastrx.reset(); - } - if r.events_suspended.read().bits() != 0 { s.end_waker.wake(); r.intenclr.write(|w| w.suspended().clear()); @@ -381,7 +373,6 @@ impl<'d, T: Instance> Twim<'d, T> { operations: &mut [Operation<'_>], tx_ram_buffer: Option<&mut [MaybeUninit; FORCE_COPY_BUFFER_SIZE]>, inten: bool, - stop: bool, ) -> Result<(), Error> { let r = T::regs(); @@ -401,14 +392,12 @@ impl<'d, T: Instance> Twim<'d, T> { r.intenclr .write(|w| w.suspended().clear().stopped().clear().error().clear()); } - #[cfg(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120"))] - r.intenclr.write(|w| w.lastrx().clear()); match operations { [Operation::Read(rd_buffer), Operation::Write(wr_buffer), rest @ ..] if !rd_buffer.is_empty() && !wr_buffer.is_empty() => { - let stop = stop && rest.is_empty(); + let stop = rest.is_empty(); // Set up DMA buffers. unsafe { @@ -433,11 +422,9 @@ impl<'d, T: Instance> Twim<'d, T> { r.tasks_resume.write(|w| unsafe { w.bits(1) }); } } - [Operation::Write(wr_buffer), Operation::Read(rd_buffer), rest @ ..] + [Operation::Write(wr_buffer), Operation::Read(rd_buffer)] if !wr_buffer.is_empty() && !rd_buffer.is_empty() => { - let stop = stop && rest.is_empty(); - // Set up DMA buffers. unsafe { self.set_tx_buffer(wr_buffer, tx_ram_buffer)?; @@ -447,18 +434,9 @@ impl<'d, T: Instance> Twim<'d, T> { // Start write+read operation. r.shorts.write(|w| { w.lasttx_startrx().enabled(); - if stop { - w.lastrx_stop().enabled(); - } else { - #[cfg(not(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120")))] - w.lastrx_suspend().enabled(); - } + w.lastrx_stop().enabled(); w }); - #[cfg(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120"))] - if !stop { - r.intenset.write(|w| w.lastrx().set()); - } r.tasks_starttx.write(|w| unsafe { w.bits(1) }); @@ -466,9 +444,7 @@ impl<'d, T: Instance> Twim<'d, T> { r.tasks_resume.write(|w| unsafe { w.bits(1) }); } } - [Operation::Read(buffer), rest @ ..] => { - let stop = stop && rest.is_empty(); - + [Operation::Read(buffer)] => { // Set up DMA buffers. unsafe { self.set_rx_buffer(buffer)?; @@ -476,18 +452,9 @@ impl<'d, T: Instance> Twim<'d, T> { // Start read operation. r.shorts.write(|w| { - if stop { - w.lastrx_stop().enabled(); - } else { - #[cfg(not(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120")))] - w.lastrx_suspend().enabled(); - } + w.lastrx_stop().enabled(); w }); - #[cfg(any(feature = "nrf52832", feature = "_nrf5340", feature = "_nrf9120"))] - if !stop { - r.intenset.write(|w| w.lastrx().set()); - } r.tasks_startrx.write(|w| unsafe { w.bits(1) }); @@ -496,16 +463,15 @@ impl<'d, T: Instance> Twim<'d, T> { } if buffer.is_empty() { - // With a zero-length buffer, LASTRX doesn't fire (because there's no last byte!), so do the STOP/SUSPEND ourselves. - if stop { - r.tasks_stop.write(|w| unsafe { w.bits(1) }); - } else { - r.tasks_suspend.write(|w| unsafe { w.bits(1) }); - } + // With a zero-length buffer, LASTRX doesn't fire (because there's no last byte!), so do the STOP ourselves. + r.tasks_stop.write(|w| unsafe { w.bits(1) }); } } + [Operation::Read(_), ..] => { + panic!("Suspending after a read is not supported!"); + } [Operation::Write(buffer), rest @ ..] => { - let stop = stop && rest.is_empty(); + let stop = rest.is_empty(); // Set up DMA buffers. unsafe { @@ -538,13 +504,11 @@ impl<'d, T: Instance> Twim<'d, T> { } } [] => { - if stop { - if was_suspended { - r.tasks_resume.write(|w| unsafe { w.bits(1) }); - } - - r.tasks_stop.write(|w| unsafe { w.bits(1) }); + if was_suspended { + r.tasks_resume.write(|w| unsafe { w.bits(1) }); } + + r.tasks_stop.write(|w| unsafe { w.bits(1) }); } } @@ -557,17 +521,18 @@ impl<'d, T: Instance> Twim<'d, T> { match operations { [Operation::Read(rd_buffer), Operation::Write(wr_buffer), ..] - | [Operation::Write(wr_buffer), Operation::Read(rd_buffer), ..] + | [Operation::Write(wr_buffer), Operation::Read(rd_buffer)] if !rd_buffer.is_empty() && !wr_buffer.is_empty() => { self.check_tx(wr_buffer.len())?; self.check_rx(rd_buffer.len())?; Ok(2) } - [Operation::Read(buffer), ..] => { + [Operation::Read(buffer)] => { self.check_rx(buffer.len())?; Ok(1) } + [Operation::Read(_), ..] => unreachable!(), [Operation::Write(buffer), ..] => { self.check_tx(buffer.len())?; Ok(1) @@ -583,26 +548,17 @@ impl<'d, T: Instance> Twim<'d, T> { /// Each buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. /// - /// If `stop` is set, the transaction will be terminated with a STOP - /// condition and the Twim will be stopped. Otherwise, the bus will be - /// left busy via clock stretching and Twim will be suspended. - /// - /// The nrf52832, nrf5340, and nrf9120 do not have hardware support for - /// suspending following a read operation therefore it is emulated by the - /// interrupt handler. If the latency of servicing that interrupt is - /// longer than a byte worth of clocks on the bus, the SCL clock will - /// continue to run for one or more additional bytes. This applies to - /// consecutive read operations, certain write-read-write sequences, or - /// any sequence of operations ending in a read when `stop == false`. - pub fn blocking_transaction( - &mut self, - address: u8, - mut operations: &mut [Operation<'_>], - stop: bool, - ) -> Result<(), Error> { + /// Consecutive `Read` operations are not supported because the Twim + /// hardware does not support suspending after a read operation. (Setting + /// the SUSPEND task in response to a LASTRX event causes the final byte of + /// the operation to be ACKed instead of NAKed. When the TWIM is resumed, + /// one more byte will be read before the new operation is started, leading + /// to an Overrun error if the RXD has not been updated, or an extraneous + /// byte read into the new buffer if the RXD has been updated.) + pub fn blocking_transaction(&mut self, address: u8, mut operations: &mut [Operation<'_>]) -> Result<(), Error> { let mut tx_ram_buffer = [MaybeUninit::uninit(); FORCE_COPY_BUFFER_SIZE]; while !operations.is_empty() { - self.setup_operations(address, operations, Some(&mut tx_ram_buffer), false, stop)?; + self.setup_operations(address, operations, Some(&mut tx_ram_buffer), false)?; self.blocking_wait(); let consumed = self.check_operations(operations)?; operations = &mut operations[consumed..]; @@ -615,10 +571,9 @@ impl<'d, T: Instance> Twim<'d, T> { &mut self, address: u8, mut operations: &mut [Operation<'_>], - stop: bool, ) -> Result<(), Error> { while !operations.is_empty() { - self.setup_operations(address, operations, None, false, stop)?; + self.setup_operations(address, operations, None, false)?; self.blocking_wait(); let consumed = self.check_operations(operations)?; operations = &mut operations[consumed..]; @@ -634,12 +589,11 @@ impl<'d, T: Instance> Twim<'d, T> { &mut self, address: u8, mut operations: &mut [Operation<'_>], - stop: bool, timeout: Duration, ) -> Result<(), Error> { let mut tx_ram_buffer = [MaybeUninit::uninit(); FORCE_COPY_BUFFER_SIZE]; while !operations.is_empty() { - self.setup_operations(address, operations, Some(&mut tx_ram_buffer), false, stop)?; + self.setup_operations(address, operations, Some(&mut tx_ram_buffer), false)?; self.blocking_wait_timeout(timeout)?; let consumed = self.check_operations(operations)?; operations = &mut operations[consumed..]; @@ -653,11 +607,10 @@ impl<'d, T: Instance> Twim<'d, T> { &mut self, address: u8, mut operations: &mut [Operation<'_>], - stop: bool, timeout: Duration, ) -> Result<(), Error> { while !operations.is_empty() { - self.setup_operations(address, operations, None, false, stop)?; + self.setup_operations(address, operations, None, false)?; self.blocking_wait_timeout(timeout)?; let consumed = self.check_operations(operations)?; operations = &mut operations[consumed..]; @@ -670,26 +623,17 @@ impl<'d, T: Instance> Twim<'d, T> { /// Each buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. /// - /// If `stop` is set, the transaction will be terminated with a STOP - /// condition and the Twim will be stopped. Otherwise, the bus will be - /// left busy via clock stretching and Twim will be suspended. - /// - /// The nrf52832, nrf5340, and nrf9120 do not have hardware support for - /// suspending following a read operation therefore it is emulated by the - /// interrupt handler. If the latency of servicing that interrupt is - /// longer than a byte worth of clocks on the bus, the SCL clock will - /// continue to run for one or more additional bytes. This applies to - /// consecutive read operations, certain write-read-write sequences, or - /// any sequence of operations ending in a read when `stop == false`. - pub async fn transaction( - &mut self, - address: u8, - mut operations: &mut [Operation<'_>], - stop: bool, - ) -> Result<(), Error> { + /// Consecutive `Read` operations are not supported because the Twim + /// hardware does not support suspending after a read operation. (Setting + /// the SUSPEND task in response to a LASTRX event causes the final byte of + /// the operation to be ACKed instead of NAKed. When the TWIM is resumed, + /// one more byte will be read before the new operation is started, leading + /// to an Overrun error if the RXD has not been updated, or an extraneous + /// byte read into the new buffer if the RXD has been updated.) + pub async fn transaction(&mut self, address: u8, mut operations: &mut [Operation<'_>]) -> Result<(), Error> { let mut tx_ram_buffer = [MaybeUninit::uninit(); FORCE_COPY_BUFFER_SIZE]; while !operations.is_empty() { - self.setup_operations(address, operations, Some(&mut tx_ram_buffer), true, stop)?; + self.setup_operations(address, operations, Some(&mut tx_ram_buffer), true)?; self.async_wait().await; let consumed = self.check_operations(operations)?; operations = &mut operations[consumed..]; @@ -702,10 +646,9 @@ impl<'d, T: Instance> Twim<'d, T> { &mut self, address: u8, mut operations: &mut [Operation<'_>], - stop: bool, ) -> Result<(), Error> { while !operations.is_empty() { - self.setup_operations(address, operations, None, true, stop)?; + self.setup_operations(address, operations, None, true)?; self.async_wait().await; let consumed = self.check_operations(operations)?; operations = &mut operations[consumed..]; @@ -720,12 +663,12 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub fn blocking_write(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { - self.blocking_transaction(address, &mut [Operation::Write(buffer)], true) + self.blocking_transaction(address, &mut [Operation::Write(buffer)]) } /// Same as [`blocking_write`](Twim::blocking_write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub fn blocking_write_from_ram(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { - self.blocking_transaction_from_ram(address, &mut [Operation::Write(buffer)], true) + self.blocking_transaction_from_ram(address, &mut [Operation::Write(buffer)]) } /// Read from an I2C slave. @@ -733,7 +676,7 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub fn blocking_read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error> { - self.blocking_transaction(address, &mut [Operation::Read(buffer)], true) + self.blocking_transaction(address, &mut [Operation::Read(buffer)]) } /// Write data to an I2C slave, then read data from the slave without @@ -742,11 +685,7 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffers must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub fn blocking_write_read(&mut self, address: u8, wr_buffer: &[u8], rd_buffer: &mut [u8]) -> Result<(), Error> { - self.blocking_transaction( - address, - &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], - true, - ) + self.blocking_transaction(address, &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)]) } /// Same as [`blocking_write_read`](Twim::blocking_write_read) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. @@ -756,11 +695,7 @@ impl<'d, T: Instance> Twim<'d, T> { wr_buffer: &[u8], rd_buffer: &mut [u8], ) -> Result<(), Error> { - self.blocking_transaction_from_ram( - address, - &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], - true, - ) + self.blocking_transaction_from_ram(address, &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)]) } // =========================================== @@ -770,7 +705,7 @@ impl<'d, T: Instance> Twim<'d, T> { /// See [`blocking_write`]. #[cfg(feature = "time")] pub fn blocking_write_timeout(&mut self, address: u8, buffer: &[u8], timeout: Duration) -> Result<(), Error> { - self.blocking_transaction_timeout(address, &mut [Operation::Write(buffer)], true, timeout) + self.blocking_transaction_timeout(address, &mut [Operation::Write(buffer)], timeout) } /// Same as [`blocking_write`](Twim::blocking_write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. @@ -781,7 +716,7 @@ impl<'d, T: Instance> Twim<'d, T> { buffer: &[u8], timeout: Duration, ) -> Result<(), Error> { - self.blocking_transaction_from_ram_timeout(address, &mut [Operation::Write(buffer)], true, timeout) + self.blocking_transaction_from_ram_timeout(address, &mut [Operation::Write(buffer)], timeout) } /// Read from an I2C slave. @@ -790,7 +725,7 @@ impl<'d, T: Instance> Twim<'d, T> { /// and at most 65535 bytes on the nRF52840. #[cfg(feature = "time")] pub fn blocking_read_timeout(&mut self, address: u8, buffer: &mut [u8], timeout: Duration) -> Result<(), Error> { - self.blocking_transaction_timeout(address, &mut [Operation::Read(buffer)], true, timeout) + self.blocking_transaction_timeout(address, &mut [Operation::Read(buffer)], timeout) } /// Write data to an I2C slave, then read data from the slave without @@ -809,7 +744,6 @@ impl<'d, T: Instance> Twim<'d, T> { self.blocking_transaction_timeout( address, &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], - true, timeout, ) } @@ -826,7 +760,6 @@ impl<'d, T: Instance> Twim<'d, T> { self.blocking_transaction_from_ram_timeout( address, &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], - true, timeout, ) } @@ -838,7 +771,7 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub async fn read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error> { - self.transaction(address, &mut [Operation::Read(buffer)], true).await + self.transaction(address, &mut [Operation::Read(buffer)]).await } /// Write to an I2C slave. @@ -846,12 +779,12 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub async fn write(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { - self.transaction(address, &mut [Operation::Write(buffer)], true).await + self.transaction(address, &mut [Operation::Write(buffer)]).await } /// Same as [`write`](Twim::write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub async fn write_from_ram(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { - self.transaction_from_ram(address, &mut [Operation::Write(buffer)], true) + self.transaction_from_ram(address, &mut [Operation::Write(buffer)]) .await } @@ -861,12 +794,8 @@ impl<'d, T: Instance> Twim<'d, T> { /// The buffers must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub async fn write_read(&mut self, address: u8, wr_buffer: &[u8], rd_buffer: &mut [u8]) -> Result<(), Error> { - self.transaction( - address, - &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], - true, - ) - .await + self.transaction(address, &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)]) + .await } /// Same as [`write_read`](Twim::write_read) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. @@ -876,12 +805,8 @@ impl<'d, T: Instance> Twim<'d, T> { wr_buffer: &[u8], rd_buffer: &mut [u8], ) -> Result<(), Error> { - self.transaction_from_ram( - address, - &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)], - true, - ) - .await + self.transaction_from_ram(address, &mut [Operation::Write(wr_buffer), Operation::Read(rd_buffer)]) + .await } } @@ -999,13 +924,13 @@ impl<'d, T: Instance> embedded_hal_1::i2c::ErrorType for Twim<'d, T> { impl<'d, T: Instance> embedded_hal_1::i2c::I2c for Twim<'d, T> { fn transaction(&mut self, address: u8, operations: &mut [Operation<'_>]) -> Result<(), Self::Error> { - self.blocking_transaction(address, operations, true) + self.blocking_transaction(address, operations) } } impl<'d, T: Instance> embedded_hal_async::i2c::I2c for Twim<'d, T> { async fn transaction(&mut self, address: u8, operations: &mut [Operation<'_>]) -> Result<(), Self::Error> { - self.transaction(address, operations, true).await + self.transaction(address, operations).await } } From fcf87a640d1425cfa563ef2900205d1493a885bf Mon Sep 17 00:00:00 2001 From: Alex Moon Date: Wed, 21 Aug 2024 10:57:14 -0400 Subject: [PATCH 3/3] Refactoring and cleanup --- embassy-nrf/src/twim.rs | 181 +++++++++++++++++++++------------------- 1 file changed, 95 insertions(+), 86 deletions(-) diff --git a/embassy-nrf/src/twim.rs b/embassy-nrf/src/twim.rs index 4a239e8b51..187fce0214 100644 --- a/embassy-nrf/src/twim.rs +++ b/embassy-nrf/src/twim.rs @@ -311,6 +311,7 @@ impl<'d, T: Instance> Twim<'d, T> { let r = T::regs(); loop { if r.events_suspended.read().bits() != 0 || r.events_stopped.read().bits() != 0 { + r.events_suspended.reset(); r.events_stopped.reset(); break; } @@ -372,15 +373,15 @@ impl<'d, T: Instance> Twim<'d, T> { address: u8, operations: &mut [Operation<'_>], tx_ram_buffer: Option<&mut [MaybeUninit; FORCE_COPY_BUFFER_SIZE]>, + last_op: Option<&Operation<'_>>, inten: bool, - ) -> Result<(), Error> { + ) -> Result { let r = T::regs(); compiler_fence(SeqCst); r.address.write(|w| unsafe { w.address().bits(address) }); - let was_suspended = r.events_suspended.read().bits() != 0; r.events_suspended.reset(); r.events_stopped.reset(); r.events_error.reset(); @@ -393,10 +394,12 @@ impl<'d, T: Instance> Twim<'d, T> { .write(|w| w.suspended().clear().stopped().clear().error().clear()); } + assert!(!operations.is_empty()); match operations { - [Operation::Read(rd_buffer), Operation::Write(wr_buffer), rest @ ..] - if !rd_buffer.is_empty() && !wr_buffer.is_empty() => - { + [Operation::Read(_), Operation::Read(_), ..] => { + panic!("Consecutive read operations are not supported!") + } + [Operation::Read(rd_buffer), Operation::Write(wr_buffer), rest @ ..] => { let stop = rest.is_empty(); // Set up DMA buffers. @@ -417,10 +420,38 @@ impl<'d, T: Instance> Twim<'d, T> { // Start read+write operation. r.tasks_startrx.write(|w| unsafe { w.bits(1) }); + if last_op.is_some() { + r.tasks_resume.write(|w| unsafe { w.bits(1) }); + } - if was_suspended { + // TODO: Handle empty write buffer + if rd_buffer.is_empty() { + // With a zero-length buffer, LASTRX doesn't fire (because there's no last byte!), so do the STARTTX ourselves. + r.tasks_starttx.write(|w| unsafe { w.bits(1) }); + } + + Ok(2) + } + [Operation::Read(buffer)] => { + // Set up DMA buffers. + unsafe { + self.set_rx_buffer(buffer)?; + } + + r.shorts.write(|w| w.lastrx_stop().enabled()); + + // Start read operation. + r.tasks_startrx.write(|w| unsafe { w.bits(1) }); + if last_op.is_some() { r.tasks_resume.write(|w| unsafe { w.bits(1) }); } + + if buffer.is_empty() { + // With a zero-length buffer, LASTRX doesn't fire (because there's no last byte!), so do the STOP ourselves. + r.tasks_stop.write(|w| unsafe { w.bits(1) }); + } + + Ok(1) } [Operation::Write(wr_buffer), Operation::Read(rd_buffer)] if !wr_buffer.is_empty() && !rd_buffer.is_empty() => @@ -439,36 +470,11 @@ impl<'d, T: Instance> Twim<'d, T> { }); r.tasks_starttx.write(|w| unsafe { w.bits(1) }); - - if was_suspended { - r.tasks_resume.write(|w| unsafe { w.bits(1) }); - } - } - [Operation::Read(buffer)] => { - // Set up DMA buffers. - unsafe { - self.set_rx_buffer(buffer)?; - } - - // Start read operation. - r.shorts.write(|w| { - w.lastrx_stop().enabled(); - w - }); - - r.tasks_startrx.write(|w| unsafe { w.bits(1) }); - - if was_suspended { + if last_op.is_some() { r.tasks_resume.write(|w| unsafe { w.bits(1) }); } - if buffer.is_empty() { - // With a zero-length buffer, LASTRX doesn't fire (because there's no last byte!), so do the STOP ourselves. - r.tasks_stop.write(|w| unsafe { w.bits(1) }); - } - } - [Operation::Read(_), ..] => { - panic!("Suspending after a read is not supported!"); + Ok(2) } [Operation::Write(buffer), rest @ ..] => { let stop = rest.is_empty(); @@ -489,8 +495,7 @@ impl<'d, T: Instance> Twim<'d, T> { }); r.tasks_starttx.write(|w| unsafe { w.bits(1) }); - - if was_suspended { + if last_op.is_some() { r.tasks_resume.write(|w| unsafe { w.bits(1) }); } @@ -502,43 +507,33 @@ impl<'d, T: Instance> Twim<'d, T> { r.tasks_suspend.write(|w| unsafe { w.bits(1) }); } } - } - [] => { - if was_suspended { - r.tasks_resume.write(|w| unsafe { w.bits(1) }); - } - r.tasks_stop.write(|w| unsafe { w.bits(1) }); + Ok(1) } + [] => unreachable!(), } - - Ok(()) } - fn check_operations(&mut self, operations: &mut [Operation<'_>]) -> Result { + fn check_operations(&mut self, operations: &[Operation<'_>]) -> Result<(), Error> { compiler_fence(SeqCst); self.check_errorsrc()?; + assert!(operations.len() == 1 || operations.len() == 2); match operations { - [Operation::Read(rd_buffer), Operation::Write(wr_buffer), ..] - | [Operation::Write(wr_buffer), Operation::Read(rd_buffer)] - if !rd_buffer.is_empty() && !wr_buffer.is_empty() => - { - self.check_tx(wr_buffer.len())?; + [Operation::Read(rd_buffer), Operation::Write(wr_buffer)] + | [Operation::Write(wr_buffer), Operation::Read(rd_buffer)] => { self.check_rx(rd_buffer.len())?; - Ok(2) + self.check_tx(wr_buffer.len())?; } [Operation::Read(buffer)] => { self.check_rx(buffer.len())?; - Ok(1) } - [Operation::Read(_), ..] => unreachable!(), [Operation::Write(buffer), ..] => { self.check_tx(buffer.len())?; - Ok(1) } - [] => Ok(0), + _ => unreachable!(), } + Ok(()) } // =========================================== @@ -548,20 +543,21 @@ impl<'d, T: Instance> Twim<'d, T> { /// Each buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. /// - /// Consecutive `Read` operations are not supported because the Twim - /// hardware does not support suspending after a read operation. (Setting - /// the SUSPEND task in response to a LASTRX event causes the final byte of - /// the operation to be ACKed instead of NAKed. When the TWIM is resumed, - /// one more byte will be read before the new operation is started, leading - /// to an Overrun error if the RXD has not been updated, or an extraneous - /// byte read into the new buffer if the RXD has been updated.) + /// Consecutive `Operation::Read`s are not supported due to hardware + /// limitations. + /// + /// An `Operation::Write` following an `Operation::Read` must have a + /// non-empty buffer. pub fn blocking_transaction(&mut self, address: u8, mut operations: &mut [Operation<'_>]) -> Result<(), Error> { let mut tx_ram_buffer = [MaybeUninit::uninit(); FORCE_COPY_BUFFER_SIZE]; + let mut last_op = None; while !operations.is_empty() { - self.setup_operations(address, operations, Some(&mut tx_ram_buffer), false)?; + let ops = self.setup_operations(address, operations, Some(&mut tx_ram_buffer), last_op, false)?; + let (in_progress, rest) = operations.split_at_mut(ops); self.blocking_wait(); - let consumed = self.check_operations(operations)?; - operations = &mut operations[consumed..]; + self.check_operations(in_progress)?; + last_op = in_progress.last(); + operations = rest; } Ok(()) } @@ -572,11 +568,14 @@ impl<'d, T: Instance> Twim<'d, T> { address: u8, mut operations: &mut [Operation<'_>], ) -> Result<(), Error> { + let mut last_op = None; while !operations.is_empty() { - self.setup_operations(address, operations, None, false)?; + let ops = self.setup_operations(address, operations, None, last_op, false)?; + let (in_progress, rest) = operations.split_at_mut(ops); self.blocking_wait(); - let consumed = self.check_operations(operations)?; - operations = &mut operations[consumed..]; + self.check_operations(in_progress)?; + last_op = in_progress.last(); + operations = rest; } Ok(()) } @@ -592,11 +591,14 @@ impl<'d, T: Instance> Twim<'d, T> { timeout: Duration, ) -> Result<(), Error> { let mut tx_ram_buffer = [MaybeUninit::uninit(); FORCE_COPY_BUFFER_SIZE]; + let mut last_op = None; while !operations.is_empty() { - self.setup_operations(address, operations, Some(&mut tx_ram_buffer), false)?; + let ops = self.setup_operations(address, operations, Some(&mut tx_ram_buffer), last_op, false)?; + let (in_progress, rest) = operations.split_at_mut(ops); self.blocking_wait_timeout(timeout)?; - let consumed = self.check_operations(operations)?; - operations = &mut operations[consumed..]; + self.check_operations(in_progress)?; + last_op = in_progress.last(); + operations = rest; } Ok(()) } @@ -609,11 +611,14 @@ impl<'d, T: Instance> Twim<'d, T> { mut operations: &mut [Operation<'_>], timeout: Duration, ) -> Result<(), Error> { + let mut last_op = None; while !operations.is_empty() { - self.setup_operations(address, operations, None, false)?; + let ops = self.setup_operations(address, operations, None, last_op, false)?; + let (in_progress, rest) = operations.split_at_mut(ops); self.blocking_wait_timeout(timeout)?; - let consumed = self.check_operations(operations)?; - operations = &mut operations[consumed..]; + self.check_operations(in_progress)?; + last_op = in_progress.last(); + operations = rest; } Ok(()) } @@ -623,20 +628,21 @@ impl<'d, T: Instance> Twim<'d, T> { /// Each buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. /// - /// Consecutive `Read` operations are not supported because the Twim - /// hardware does not support suspending after a read operation. (Setting - /// the SUSPEND task in response to a LASTRX event causes the final byte of - /// the operation to be ACKed instead of NAKed. When the TWIM is resumed, - /// one more byte will be read before the new operation is started, leading - /// to an Overrun error if the RXD has not been updated, or an extraneous - /// byte read into the new buffer if the RXD has been updated.) + /// Consecutive `Operation::Read`s are not supported due to hardware + /// limitations. + /// + /// An `Operation::Write` following an `Operation::Read` must have a + /// non-empty buffer. pub async fn transaction(&mut self, address: u8, mut operations: &mut [Operation<'_>]) -> Result<(), Error> { let mut tx_ram_buffer = [MaybeUninit::uninit(); FORCE_COPY_BUFFER_SIZE]; + let mut last_op = None; while !operations.is_empty() { - self.setup_operations(address, operations, Some(&mut tx_ram_buffer), true)?; + let ops = self.setup_operations(address, operations, Some(&mut tx_ram_buffer), last_op, true)?; + let (in_progress, rest) = operations.split_at_mut(ops); self.async_wait().await; - let consumed = self.check_operations(operations)?; - operations = &mut operations[consumed..]; + self.check_operations(in_progress)?; + last_op = in_progress.last(); + operations = rest; } Ok(()) } @@ -647,11 +653,14 @@ impl<'d, T: Instance> Twim<'d, T> { address: u8, mut operations: &mut [Operation<'_>], ) -> Result<(), Error> { + let mut last_op = None; while !operations.is_empty() { - self.setup_operations(address, operations, None, true)?; + let ops = self.setup_operations(address, operations, None, last_op, true)?; + let (in_progress, rest) = operations.split_at_mut(ops); self.async_wait().await; - let consumed = self.check_operations(operations)?; - operations = &mut operations[consumed..]; + self.check_operations(in_progress)?; + last_op = in_progress.last(); + operations = rest; } Ok(()) }