From 746ded94b13d037c5be16071d2d9d195b979c0c7 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 20 Mar 2024 19:52:36 +0100 Subject: [PATCH 01/13] Fix minor typos --- embassy-stm32/src/i2c/v1.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 9f29ed5e0..563bbfdaf 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -58,10 +58,10 @@ pub unsafe fn on_interrupt() { /// - `ST` = start condition /// - `SR` = repeated start condition /// - `SP` = stop condition +/// - `ACK`/`NACK` = last byte in read operation #[derive(Copy, Clone)] enum FrameOptions { - /// `[ST/SR]+[NACK]+[SP]` First frame (of this type) in operation and last frame overall in this - /// transaction. + /// `[ST/SR]+[NACK]+[SP]` First frame (of this type) in transaction and also last frame overall. FirstAndLastFrame, /// `[ST/SR]+[NACK]` First frame of this type in transaction, last frame in a read operation but /// not the last frame overall. From 0885c102d3c332591b2d913113cde08fca8fe41b Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 20 Mar 2024 20:11:59 +0100 Subject: [PATCH 02/13] Refactor async I2C transfers to use frame options --- embassy-stm32/src/i2c/v1.rs | 341 +++++++++++++++++++----------------- 1 file changed, 183 insertions(+), 158 deletions(-) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 563bbfdaf..2751d443f 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -459,7 +459,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { }); } - async fn write_with_stop(&mut self, address: u8, write: &[u8], send_stop: bool) -> Result<(), Error> + async fn write_frame(&mut self, address: u8, write: &[u8], frame: FrameOptions) -> Result<(), Error> where TXDMA: crate::i2c::TxDma, { @@ -487,74 +487,76 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { }) }); - Self::enable_interrupts(); - - // Send a START condition - T::regs().cr1().modify(|reg| { - reg.set_start(true); - }); - let state = T::state(); - // Wait until START condition was generated - poll_fn(|cx| { - state.waker.register(cx.waker()); + if frame.send_start() { + // Send a START condition + Self::enable_interrupts(); + T::regs().cr1().modify(|reg| { + reg.set_start(true); + }); - match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err(e)), - Ok(sr1) => { - if sr1.start() { - Poll::Ready(Ok(())) - } else { - Poll::Pending + // Wait until START condition was generated + poll_fn(|cx| { + state.waker.register(cx.waker()); + + match Self::check_and_clear_error_flags() { + Err(e) => Poll::Ready(Err(e)), + Ok(sr1) => { + if sr1.start() { + Poll::Ready(Ok(())) + } else { + Poll::Pending + } } } - } - }) - .await?; + }) + .await?; - // Also wait until signalled we're master and everything is waiting for us - Self::enable_interrupts(); - poll_fn(|cx| { - state.waker.register(cx.waker()); + // Also wait until signalled we're master and everything is waiting for us + Self::enable_interrupts(); + poll_fn(|cx| { + state.waker.register(cx.waker()); - match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err(e)), - Ok(_) => { - let sr2 = T::regs().sr2().read(); - if !sr2.msl() && !sr2.busy() { - Poll::Pending - } else { - Poll::Ready(Ok(())) + match Self::check_and_clear_error_flags() { + Err(e) => Poll::Ready(Err(e)), + Ok(_) => { + let sr2 = T::regs().sr2().read(); + if !sr2.msl() && !sr2.busy() { + Poll::Pending + } else { + Poll::Ready(Ok(())) + } } } - } - }) - .await?; + }) + .await?; - // Set up current address, we're trying to talk to - Self::enable_interrupts(); - T::regs().dr().write(|reg| reg.set_dr(address << 1)); + // Set up current address, we're trying to talk to + Self::enable_interrupts(); + T::regs().dr().write(|reg| reg.set_dr(address << 1)); - poll_fn(|cx| { - state.waker.register(cx.waker()); - match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err(e)), - Ok(sr1) => { - if sr1.addr() { - // Clear the ADDR condition by reading SR2. - T::regs().sr2().read(); - Poll::Ready(Ok(())) - } else { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. - Self::enable_interrupts(); - Poll::Pending + poll_fn(|cx| { + state.waker.register(cx.waker()); + match Self::check_and_clear_error_flags() { + Err(e) => Poll::Ready(Err(e)), + Ok(sr1) => { + if sr1.addr() { + // Clear the ADDR condition by reading SR2. + T::regs().sr2().read(); + Poll::Ready(Ok(())) + } else { + // If we need to go around, then re-enable the interrupts, otherwise nothing + // can wake us up and we'll hang. + Self::enable_interrupts(); + Poll::Pending + } } } - } - }) - .await?; + }) + .await?; + } + Self::enable_interrupts(); let poll_error = poll_fn(|cx| { state.waker.register(cx.waker()); @@ -591,7 +593,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { Err(e) => Poll::Ready(Err(e)), Ok(sr1) => { if sr1.btf() { - if send_stop { + if frame.send_stop() { T::regs().cr1().modify(|w| { w.set_stop(true); }); @@ -606,6 +608,21 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { }) .await?; + if frame.send_stop() { + // Wait for STOP condition to transmit. + Self::enable_interrupts(); + poll_fn(|cx| { + T::state().waker.register(cx.waker()); + // TODO: error interrupts are enabled here, should we additional check for and return errors? + if T::regs().cr1().read().stop() { + Poll::Pending + } else { + Poll::Ready(Ok(())) + } + }) + .await?; + } + drop(on_drop); // Fallthrough is success @@ -617,26 +634,24 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { where TXDMA: crate::i2c::TxDma, { - self.write_with_stop(address, write, true).await?; - - // Wait for STOP condition to transmit. - Self::enable_interrupts(); - poll_fn(|cx| { - T::state().waker.register(cx.waker()); - // TODO: error interrupts are enabled here, should we additional check for and return errors? - if T::regs().cr1().read().stop() { - Poll::Pending - } else { - Poll::Ready(Ok(())) - } - }) - .await?; + self.write_frame(address, write, FrameOptions::FirstAndLastFrame) + .await?; Ok(()) } /// Read. pub async fn read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error> + where + RXDMA: crate::i2c::RxDma, + { + self.read_frame(address, buffer, FrameOptions::FirstAndLastFrame) + .await?; + + Ok(()) + } + + async fn read_frame(&mut self, address: u8, buffer: &mut [u8], frame: FrameOptions) -> Result<(), Error> where RXDMA: crate::i2c::RxDma, { @@ -667,98 +682,99 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { }) }); - Self::enable_interrupts(); + if frame.send_start() { + // Send a START condition and set ACK bit + Self::enable_interrupts(); + T::regs().cr1().modify(|reg| { + reg.set_start(true); + reg.set_ack(true); + }); - // Send a START condition and set ACK bit - T::regs().cr1().modify(|reg| { - reg.set_start(true); - reg.set_ack(true); - }); + // Wait until START condition was generated + poll_fn(|cx| { + state.waker.register(cx.waker()); - // Wait until START condition was generated - poll_fn(|cx| { - state.waker.register(cx.waker()); - - match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err(e)), - Ok(sr1) => { - if sr1.start() { - Poll::Ready(Ok(())) - } else { - Poll::Pending - } - } - } - }) - .await?; - - // Also wait until signalled we're master and everything is waiting for us - Self::enable_interrupts(); - poll_fn(|cx| { - state.waker.register(cx.waker()); - - // blocking read didn’t have a check_and_clear call here, but blocking write did so - // I’m adding it here in case that was an oversight. - match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err(e)), - Ok(_) => { - let sr2 = T::regs().sr2().read(); - if !sr2.msl() && !sr2.busy() { - Poll::Pending - } else { - Poll::Ready(Ok(())) - } - } - } - }) - .await?; - - // Set up current address, we're trying to talk to - T::regs().dr().write(|reg| reg.set_dr((address << 1) + 1)); - - // Wait for the address to be acknowledged - - Self::enable_interrupts(); - poll_fn(|cx| { - state.waker.register(cx.waker()); - - match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err(e)), - Ok(sr1) => { - if sr1.addr() { - // 18.3.8: When a single byte must be received: the NACK must be programmed during EV6 - // event, i.e. program ACK=0 when ADDR=1, before clearing ADDR flag. - if buffer_len == 1 { - T::regs().cr1().modify(|w| { - w.set_ack(false); - }); + match Self::check_and_clear_error_flags() { + Err(e) => Poll::Ready(Err(e)), + Ok(sr1) => { + if sr1.start() { + Poll::Ready(Ok(())) + } else { + Poll::Pending } - Poll::Ready(Ok(())) - } else { - Poll::Pending } } - } - }) - .await?; + }) + .await?; - // Clear ADDR condition by reading SR2 - T::regs().sr2().read(); + // Also wait until signalled we're master and everything is waiting for us + Self::enable_interrupts(); + poll_fn(|cx| { + state.waker.register(cx.waker()); + + // blocking read didn’t have a check_and_clear call here, but blocking write did so + // I’m adding it here in case that was an oversight. + match Self::check_and_clear_error_flags() { + Err(e) => Poll::Ready(Err(e)), + Ok(_) => { + let sr2 = T::regs().sr2().read(); + if !sr2.msl() && !sr2.busy() { + Poll::Pending + } else { + Poll::Ready(Ok(())) + } + } + } + }) + .await?; + + // Set up current address, we're trying to talk to + T::regs().dr().write(|reg| reg.set_dr((address << 1) + 1)); + + // Wait for the address to be acknowledged + + Self::enable_interrupts(); + poll_fn(|cx| { + state.waker.register(cx.waker()); + + match Self::check_and_clear_error_flags() { + Err(e) => Poll::Ready(Err(e)), + Ok(sr1) => { + if sr1.addr() { + // 18.3.8: When a single byte must be received: the NACK must be programmed during EV6 + // event, i.e. program ACK=0 when ADDR=1, before clearing ADDR flag. + if buffer_len == 1 && frame.send_nack() { + T::regs().cr1().modify(|w| { + w.set_ack(false); + }); + } + Poll::Ready(Ok(())) + } else { + Poll::Pending + } + } + } + }) + .await?; + + // Clear ADDR condition by reading SR2 + T::regs().sr2().read(); + } // 18.3.8: When a single byte must be received: [snip] Then the // user can program the STOP condition either after clearing ADDR flag, or in the // DMA Transfer Complete interrupt routine. - if buffer_len == 1 { + if buffer_len == 1 && frame.send_stop() { T::regs().cr1().modify(|w| { w.set_stop(true); }); - } else { + } else if buffer_len != 1 && frame.send_nack() { // If, in the I2C_CR2 register, the LAST bit is set, I2C // automatically sends a NACK after the next byte following EOT_1. The user can // generate a Stop condition in the DMA Transfer Complete interrupt routine if enabled. T::regs().cr2().modify(|w| { w.set_last(true); - }) + }); } // Wait for bytes to be received, or an error to occur. @@ -777,18 +793,27 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { _ => Ok(()), }?; - // Wait for the STOP to be sent (STOP bit cleared). - Self::enable_interrupts(); - poll_fn(|cx| { - state.waker.register(cx.waker()); - // TODO: error interrupts are enabled here, should we additional check for and return errors? - if T::regs().cr1().read().stop() { - Poll::Pending - } else { - Poll::Ready(Ok(())) + if frame.send_stop() { + if buffer_len != 1 { + T::regs().cr1().modify(|w| { + w.set_stop(true); + }); } - }) - .await?; + + // Wait for the STOP to be sent (STOP bit cleared). + Self::enable_interrupts(); + poll_fn(|cx| { + state.waker.register(cx.waker()); + // TODO: error interrupts are enabled here, should we additional check for and return errors? + if T::regs().cr1().read().stop() { + Poll::Pending + } else { + Poll::Ready(Ok(())) + } + }) + .await?; + } + drop(on_drop); // Fallthrough is success @@ -801,8 +826,8 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { RXDMA: crate::i2c::RxDma, TXDMA: crate::i2c::TxDma, { - self.write_with_stop(address, write, false).await?; - self.read(address, read).await + self.write_frame(address, write, FrameOptions::FirstFrame).await?; + self.read_frame(address, read, FrameOptions::FirstAndLastFrame).await } } From 9c00a40e73f49aa0d46a47259fe8adc8c3f248b8 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Thu, 21 Mar 2024 00:25:39 +0100 Subject: [PATCH 03/13] Extract frame options generation into iterator to reuse in async --- embassy-stm32/src/i2c/v1.rs | 102 ++++++++++++++++++++---------------- 1 file changed, 58 insertions(+), 44 deletions(-) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 2751d443f..dd2cea6b8 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -5,7 +5,7 @@ //! All other devices (as of 2023-12-28) use [`v2`](super::v2) instead. use core::future::poll_fn; -use core::task::Poll; +use core::{iter, task::Poll}; use embassy_embedded_hal::SetConfig; use embassy_futures::select::{select, Either}; @@ -103,6 +103,62 @@ impl FrameOptions { } } +/// Iterates over operations in transaction. +/// +/// Returns necessary frame options for each operation to uphold the [transaction contract] and have +/// the right start/stop/(N)ACK conditions on the wire. +/// +/// [transaction contract]: embedded_hal_1::i2c::I2c::transaction +fn operation_frames<'a, 'b: 'a>( + operations: &'a mut [Operation<'b>], +) -> impl IntoIterator, FrameOptions)> { + let mut operations = operations.iter_mut().peekable(); + + let mut next_first_frame = true; + + iter::from_fn(move || { + let Some(op) = operations.next() else { + return None; + }; + + // Is `op` first frame of its type? + let first_frame = next_first_frame; + let next_op = operations.peek(); + + // Get appropriate frame options as combination of the following properties: + // + // - For each first operation of its type, generate a (repeated) start condition. + // - For the last operation overall in the entire transaction, generate a stop condition. + // - For read operations, check the next operation: if it is also a read operation, we merge + // these and send ACK for all bytes in the current operation; send NACK only for the final + // read operation's last byte (before write or end of entire transaction) to indicate last + // byte read and release the bus for transmission of the bus master's next byte (or stop). + // + // We check the third property unconditionally, i.e. even for write opeartions. This is okay + // because the resulting frame options are identical for write operations. + let frame = match (first_frame, next_op) { + (true, None) => FrameOptions::FirstAndLastFrame, + (true, Some(Operation::Read(_))) => FrameOptions::FirstAndNextFrame, + (true, Some(Operation::Write(_))) => FrameOptions::FirstFrame, + // + (false, None) => FrameOptions::LastFrame, + (false, Some(Operation::Read(_))) => FrameOptions::NextFrame, + (false, Some(Operation::Write(_))) => FrameOptions::LastFrameNoStop, + }; + + // Pre-calculate if `next_op` is the first operation of its type. We do this here and not at + // the beginning of the loop because we hand out `op` as iterator value and cannot access it + // anymore in the next iteration. + next_first_frame = match (&op, next_op) { + (_, None) => false, + (Operation::Read(_), Some(Operation::Write(_))) | (Operation::Write(_), Some(Operation::Read(_))) => true, + (Operation::Read(_), Some(Operation::Read(_))) | (Operation::Write(_), Some(Operation::Write(_))) => false, + }; + + Some((op, frame)) + }) +} + impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { pub(crate) fn init(&mut self, freq: Hertz, _config: Config) { T::regs().cr1().modify(|reg| { @@ -397,53 +453,11 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { let timeout = self.timeout(); - let mut operations = operations.iter_mut(); - - let mut prev_op: Option<&mut Operation<'_>> = None; - let mut next_op = operations.next(); - - while let Some(op) = next_op { - next_op = operations.next(); - - // Check if this is the first frame of this type. This is the case for the first overall - // frame in the transaction and whenever the type of operation changes. - let first_frame = - match (prev_op.as_ref(), &op) { - (None, _) => true, - (Some(Operation::Read(_)), Operation::Write(_)) - | (Some(Operation::Write(_)), Operation::Read(_)) => true, - (Some(Operation::Read(_)), Operation::Read(_)) - | (Some(Operation::Write(_)), Operation::Write(_)) => false, - }; - - let frame = match (first_frame, next_op.as_ref()) { - // If this is the first frame of this type, we generate a (repeated) start condition - // but have to consider the next operation: if it is the last, we generate the final - // stop condition. Otherwise, we branch on the operation: with read operations, only - // the last byte overall (before a write operation or the end of the transaction) is - // to be NACK'd, i.e. if another read operation follows, we must ACK this last byte. - (true, None) => FrameOptions::FirstAndLastFrame, - // Make sure to keep sending ACK for last byte in read operation when it is followed - // by another consecutive read operation. If the current operation is write, this is - // identical to `FirstFrame`. - (true, Some(Operation::Read(_))) => FrameOptions::FirstAndNextFrame, - // Otherwise, send NACK for last byte (in read operation). (For write, this does not - // matter and could also be `FirstAndNextFrame`.) - (true, Some(Operation::Write(_))) => FrameOptions::FirstFrame, - - // If this is not the first frame of its type, we do not generate a (repeated) start - // condition. Otherwise, we branch the same way as above. - (false, None) => FrameOptions::LastFrame, - (false, Some(Operation::Read(_))) => FrameOptions::NextFrame, - (false, Some(Operation::Write(_))) => FrameOptions::LastFrameNoStop, - }; - + for (op, frame) in operation_frames(operations) { match op { Operation::Read(read) => self.blocking_read_timeout(addr, read, timeout, frame)?, Operation::Write(write) => self.write_bytes(addr, write, timeout, frame)?, } - - prev_op = Some(op); } Ok(()) From accec7a84076ff26f9bdad388809b96537cf31da Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Thu, 21 Mar 2024 00:30:53 +0100 Subject: [PATCH 04/13] Implement asynchronous transaction for I2C v1 --- embassy-stm32/src/i2c/mod.rs | 4 +- embassy-stm32/src/i2c/v1.rs | 214 ++++++++++++++++++++--------------- 2 files changed, 123 insertions(+), 95 deletions(-) diff --git a/embassy-stm32/src/i2c/mod.rs b/embassy-stm32/src/i2c/mod.rs index f1b11cc44..6700f0f7d 100644 --- a/embassy-stm32/src/i2c/mod.rs +++ b/embassy-stm32/src/i2c/mod.rs @@ -332,8 +332,6 @@ impl<'d, T: Instance, TXDMA: TxDma, RXDMA: RxDma> embedded_hal_async::i2c: address: u8, operations: &mut [embedded_hal_1::i2c::Operation<'_>], ) -> Result<(), Self::Error> { - let _ = address; - let _ = operations; - todo!() + self.transaction(address, operations).await } } diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index dd2cea6b8..a740ab834 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -111,12 +111,21 @@ impl FrameOptions { /// [transaction contract]: embedded_hal_1::i2c::I2c::transaction fn operation_frames<'a, 'b: 'a>( operations: &'a mut [Operation<'b>], -) -> impl IntoIterator, FrameOptions)> { +) -> Result, FrameOptions)>, Error> { + // Check empty read buffer before starting transaction. Otherwise, we would risk halting with an + // error in the middle of the transaction. + if operations.iter().any(|op| match op { + Operation::Read(read) => read.is_empty(), + Operation::Write(_) => false, + }) { + return Err(Error::Overrun); + } + let mut operations = operations.iter_mut().peekable(); let mut next_first_frame = true; - iter::from_fn(move || { + Ok(iter::from_fn(move || { let Some(op) = operations.next() else { return None; }; @@ -156,7 +165,7 @@ fn operation_frames<'a, 'b: 'a>( }; Some((op, frame)) - }) + })) } impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { @@ -442,18 +451,9 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { /// /// [transaction contract]: embedded_hal_1::i2c::I2c::transaction pub fn blocking_transaction(&mut self, addr: u8, operations: &mut [Operation<'_>]) -> Result<(), Error> { - // Check empty read buffer before starting transaction. Otherwise, we would not generate the - // stop condition below. - if operations.iter().any(|op| match op { - Operation::Read(read) => read.is_empty(), - Operation::Write(_) => false, - }) { - return Err(Error::Overrun); - } - let timeout = self.timeout(); - for (op, frame) in operation_frames(operations) { + for (op, frame) in operation_frames(operations)? { match op { Operation::Read(read) => self.blocking_read_timeout(addr, read, timeout, frame)?, Operation::Write(write) => self.write_bytes(addr, write, timeout, frame)?, @@ -480,9 +480,12 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { let dma_transfer = unsafe { let regs = T::regs(); regs.cr2().modify(|w| { + // Note: Do not enable the ITBUFEN bit in the I2C_CR2 register if DMA is used for reception. + w.set_itbufen(false); // DMA mode can be enabled for transmission by setting the DMAEN bit in the I2C_CR2 register. w.set_dmaen(true); - w.set_itbufen(false); + // Sending NACK is not necessary (nor possible) for write transfer. + w.set_last(false); }); // Set the I2C_DR register address in the DMA_SxPAR register. The data will be moved to this address from the memory after each TxE event. let dst = regs.dr().as_ptr() as *mut u8; @@ -520,6 +523,9 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if sr1.start() { Poll::Ready(Ok(())) } else { + // If we need to go around, then re-enable the interrupts, otherwise nothing + // can wake us up and we'll hang. + Self::enable_interrupts(); Poll::Pending } } @@ -537,6 +543,9 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { Ok(_) => { let sr2 = T::regs().sr2().read(); if !sr2.msl() && !sr2.busy() { + // If we need to go around, then re-enable the interrupts, otherwise nothing + // can wake us up and we'll hang. + Self::enable_interrupts(); Poll::Pending } else { Poll::Ready(Ok(())) @@ -550,14 +559,14 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { Self::enable_interrupts(); T::regs().dr().write(|reg| reg.set_dr(address << 1)); + // Wait for the address to be acknowledged poll_fn(|cx| { state.waker.register(cx.waker()); + match Self::check_and_clear_error_flags() { Err(e) => Poll::Ready(Err(e)), Ok(sr1) => { if sr1.addr() { - // Clear the ADDR condition by reading SR2. - T::regs().sr2().read(); Poll::Ready(Ok(())) } else { // If we need to go around, then re-enable the interrupts, otherwise nothing @@ -569,8 +578,12 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { } }) .await?; + + // Clear condition by reading SR2 + T::regs().sr2().read(); } + // Wait for bytes to be sent, or an error to occur. Self::enable_interrupts(); let poll_error = poll_fn(|cx| { state.waker.register(cx.waker()); @@ -579,7 +592,12 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { // Unclear why the Err turbofish is necessary here? The compiler didn’t require it in the other // identical poll_fn check_and_clear matches. Err(e) => Poll::Ready(Err::(e)), - Ok(_) => Poll::Pending, + Ok(_) => { + // If we need to go around, then re-enable the interrupts, otherwise nothing + // can wake us up and we'll hang. + Self::enable_interrupts(); + Poll::Pending + } } }); @@ -589,52 +607,38 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { _ => Ok(()), }?; - // The I2C transfer itself will take longer than the DMA transfer, so wait for that to finish too. - - // 18.3.8 “Master transmitter: In the interrupt routine after the EOT interrupt, disable DMA - // requests then wait for a BTF event before programming the Stop condition.” - - // TODO: If this has to be done “in the interrupt routine after the EOT interrupt”, where to put it? T::regs().cr2().modify(|w| { w.set_dmaen(false); }); - Self::enable_interrupts(); - poll_fn(|cx| { - state.waker.register(cx.waker()); - - match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err(e)), - Ok(sr1) => { - if sr1.btf() { - if frame.send_stop() { - T::regs().cr1().modify(|w| { - w.set_stop(true); - }); - } - - Poll::Ready(Ok(())) - } else { - Poll::Pending - } - } - } - }) - .await?; - if frame.send_stop() { - // Wait for STOP condition to transmit. + // The I2C transfer itself will take longer than the DMA transfer, so wait for that to finish too. + + // 18.3.8 “Master transmitter: In the interrupt routine after the EOT interrupt, disable DMA + // requests then wait for a BTF event before programming the Stop condition.” Self::enable_interrupts(); poll_fn(|cx| { - T::state().waker.register(cx.waker()); - // TODO: error interrupts are enabled here, should we additional check for and return errors? - if T::regs().cr1().read().stop() { - Poll::Pending - } else { - Poll::Ready(Ok(())) + state.waker.register(cx.waker()); + + match Self::check_and_clear_error_flags() { + Err(e) => Poll::Ready(Err(e)), + Ok(sr1) => { + if sr1.btf() { + Poll::Ready(Ok(())) + } else { + // If we need to go around, then re-enable the interrupts, otherwise nothing + // can wake us up and we'll hang. + Self::enable_interrupts(); + Poll::Pending + } + } } }) .await?; + + T::regs().cr1().modify(|w| { + w.set_stop(true); + }); } drop(on_drop); @@ -669,15 +673,19 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { where RXDMA: crate::i2c::RxDma, { - let state = T::state(); let buffer_len = buffer.len(); let dma_transfer = unsafe { let regs = T::regs(); regs.cr2().modify(|w| { - // DMA mode can be enabled for transmission by setting the DMAEN bit in the I2C_CR2 register. + // Note: Do not enable the ITBUFEN bit in the I2C_CR2 register if DMA is used for reception. w.set_itbufen(false); + // DMA mode can be enabled for transmission by setting the DMAEN bit in the I2C_CR2 register. w.set_dmaen(true); + // If, in the I2C_CR2 register, the LAST bit is set, I2C + // automatically sends a NACK after the next byte following EOT_1. The user can + // generate a Stop condition in the DMA Transfer Complete interrupt routine if enabled. + w.set_last(frame.send_nack() && buffer_len != 1); }); // Set the I2C_DR register address in the DMA_SxPAR register. The data will be moved to this address from the memory after each TxE event. let src = regs.dr().as_ptr() as *mut u8; @@ -696,6 +704,8 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { }) }); + let state = T::state(); + if frame.send_start() { // Send a START condition and set ACK bit Self::enable_interrupts(); @@ -714,6 +724,9 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if sr1.start() { Poll::Ready(Ok(())) } else { + // If we need to go around, then re-enable the interrupts, otherwise nothing + // can wake us up and we'll hang. + Self::enable_interrupts(); Poll::Pending } } @@ -733,6 +746,9 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { Ok(_) => { let sr2 = T::regs().sr2().read(); if !sr2.msl() && !sr2.busy() { + // If we need to go around, then re-enable the interrupts, otherwise nothing + // can wake us up and we'll hang. + Self::enable_interrupts(); Poll::Pending } else { Poll::Ready(Ok(())) @@ -743,11 +759,10 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { .await?; // Set up current address, we're trying to talk to + Self::enable_interrupts(); T::regs().dr().write(|reg| reg.set_dr((address << 1) + 1)); // Wait for the address to be acknowledged - - Self::enable_interrupts(); poll_fn(|cx| { state.waker.register(cx.waker()); @@ -755,15 +770,11 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { Err(e) => Poll::Ready(Err(e)), Ok(sr1) => { if sr1.addr() { - // 18.3.8: When a single byte must be received: the NACK must be programmed during EV6 - // event, i.e. program ACK=0 when ADDR=1, before clearing ADDR flag. - if buffer_len == 1 && frame.send_nack() { - T::regs().cr1().modify(|w| { - w.set_ack(false); - }); - } Poll::Ready(Ok(())) } else { + // If we need to go around, then re-enable the interrupts, otherwise nothing + // can wake us up and we'll hang. + Self::enable_interrupts(); Poll::Pending } } @@ -771,24 +782,29 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { }) .await?; - // Clear ADDR condition by reading SR2 + // 18.3.8: When a single byte must be received: the NACK must be programmed during EV6 + // event, i.e. program ACK=0 when ADDR=1, before clearing ADDR flag. + if frame.send_nack() && buffer_len == 1 { + T::regs().cr1().modify(|w| { + w.set_ack(false); + }); + } + + // Clear condition by reading SR2 T::regs().sr2().read(); + } else if frame.send_nack() && buffer_len == 1 { + T::regs().cr1().modify(|w| { + w.set_ack(false); + }); } // 18.3.8: When a single byte must be received: [snip] Then the // user can program the STOP condition either after clearing ADDR flag, or in the // DMA Transfer Complete interrupt routine. - if buffer_len == 1 && frame.send_stop() { + if frame.send_stop() && buffer_len == 1 { T::regs().cr1().modify(|w| { w.set_stop(true); }); - } else if buffer_len != 1 && frame.send_nack() { - // If, in the I2C_CR2 register, the LAST bit is set, I2C - // automatically sends a NACK after the next byte following EOT_1. The user can - // generate a Stop condition in the DMA Transfer Complete interrupt routine if enabled. - T::regs().cr2().modify(|w| { - w.set_last(true); - }); } // Wait for bytes to be received, or an error to occur. @@ -798,7 +814,12 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { match Self::check_and_clear_error_flags() { Err(e) => Poll::Ready(Err::(e)), - _ => Poll::Pending, + _ => { + // If we need to go around, then re-enable the interrupts, otherwise nothing + // can wake us up and we'll hang. + Self::enable_interrupts(); + Poll::Pending + } } }); @@ -807,25 +828,14 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { _ => Ok(()), }?; - if frame.send_stop() { - if buffer_len != 1 { - T::regs().cr1().modify(|w| { - w.set_stop(true); - }); - } + T::regs().cr2().modify(|w| { + w.set_dmaen(false); + }); - // Wait for the STOP to be sent (STOP bit cleared). - Self::enable_interrupts(); - poll_fn(|cx| { - state.waker.register(cx.waker()); - // TODO: error interrupts are enabled here, should we additional check for and return errors? - if T::regs().cr1().read().stop() { - Poll::Pending - } else { - Poll::Ready(Ok(())) - } - }) - .await?; + if frame.send_stop() && buffer_len != 1 { + T::regs().cr1().modify(|w| { + w.set_stop(true); + }); } drop(on_drop); @@ -843,6 +853,26 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { self.write_frame(address, write, FrameOptions::FirstFrame).await?; self.read_frame(address, read, FrameOptions::FirstAndLastFrame).await } + + /// Transaction with operations. + /// + /// Consecutive operations of same type are merged. See [transaction contract] for details. + /// + /// [transaction contract]: embedded_hal_1::i2c::I2c::transaction + pub async fn transaction(&mut self, addr: u8, operations: &mut [Operation<'_>]) -> Result<(), Error> + where + RXDMA: crate::i2c::RxDma, + TXDMA: crate::i2c::TxDma, + { + for (op, frame) in operation_frames(operations)? { + match op { + Operation::Read(read) => self.read_frame(addr, read, frame).await?, + Operation::Write(write) => self.write_frame(addr, write, frame).await?, + } + } + + Ok(()) + } } impl<'d, T: Instance, TXDMA, RXDMA> Drop for I2c<'d, T, TXDMA, RXDMA> { From c1175bf7d850d6e9091853e6e9980b11407b5a21 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 27 Mar 2024 00:04:35 +0100 Subject: [PATCH 05/13] It is not necessary to wait for STOP to be fully generated --- embassy-stm32/src/i2c/v1.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index a740ab834..1e2205389 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -296,10 +296,6 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if frame.send_stop() { // Send a STOP condition T::regs().cr1().modify(|reg| reg.set_stop(true)); - // Wait for STOP condition to transmit. - while T::regs().cr1().read().stop() { - timeout.check()?; - } } // Fallthrough is success @@ -405,13 +401,6 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { // Receive last byte *last = self.recv_byte(timeout)?; - if frame.send_stop() { - // Wait for the STOP to be sent. - while T::regs().cr1().read().stop() { - timeout.check()?; - } - } - // Fallthrough is success Ok(()) } From 2e2986c67b3a7a6a87695a359d630864c9eb6194 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 27 Mar 2024 00:17:12 +0100 Subject: [PATCH 06/13] It is not necessary to wait for SB and MSL sequentially --- embassy-stm32/src/i2c/v1.rs | 72 ++++++++----------------------------- 1 file changed, 14 insertions(+), 58 deletions(-) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 1e2205389..1e0eea33c 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -264,14 +264,9 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { timeout.check()?; } - // Also wait until signalled we're master and everything is waiting for us - while { - Self::check_and_clear_error_flags()?; - - let sr2 = T::regs().sr2().read(); - !sr2.msl() && !sr2.busy() - } { - timeout.check()?; + // Check if we were the ones to generate START + if T::regs().cr1().read().start() || !T::regs().sr2().read().msl() { + return Err(Error::Arbitration); } // Set up current address, we're trying to talk to @@ -362,12 +357,9 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { timeout.check()?; } - // Also wait until signalled we're master and everything is waiting for us - while { - let sr2 = T::regs().sr2().read(); - !sr2.msl() && !sr2.busy() - } { - timeout.check()?; + // Check if we were the ones to generate START + if T::regs().cr1().read().start() || !T::regs().sr2().read().msl() { + return Err(Error::Arbitration); } // Set up current address, we're trying to talk to @@ -522,27 +514,10 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { }) .await?; - // Also wait until signalled we're master and everything is waiting for us - Self::enable_interrupts(); - poll_fn(|cx| { - state.waker.register(cx.waker()); - - match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err(e)), - Ok(_) => { - let sr2 = T::regs().sr2().read(); - if !sr2.msl() && !sr2.busy() { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. - Self::enable_interrupts(); - Poll::Pending - } else { - Poll::Ready(Ok(())) - } - } - } - }) - .await?; + // Check if we were the ones to generate START + if T::regs().cr1().read().start() || !T::regs().sr2().read().msl() { + return Err(Error::Arbitration); + } // Set up current address, we're trying to talk to Self::enable_interrupts(); @@ -723,29 +698,10 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { }) .await?; - // Also wait until signalled we're master and everything is waiting for us - Self::enable_interrupts(); - poll_fn(|cx| { - state.waker.register(cx.waker()); - - // blocking read didn’t have a check_and_clear call here, but blocking write did so - // I’m adding it here in case that was an oversight. - match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err(e)), - Ok(_) => { - let sr2 = T::regs().sr2().read(); - if !sr2.msl() && !sr2.busy() { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. - Self::enable_interrupts(); - Poll::Pending - } else { - Poll::Ready(Ok(())) - } - } - } - }) - .await?; + // Check if we were the ones to generate START + if T::regs().cr1().read().start() || !T::regs().sr2().read().msl() { + return Err(Error::Arbitration); + } // Set up current address, we're trying to talk to Self::enable_interrupts(); From b299266cd240d73bf6ec36d6a0710523ce5eb139 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 27 Mar 2024 00:20:29 +0100 Subject: [PATCH 07/13] It is not necessary to enable interrupts before registering waker --- embassy-stm32/src/i2c/v1.rs | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 1e0eea33c..7c0bb31d6 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -489,7 +489,6 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if frame.send_start() { // Send a START condition - Self::enable_interrupts(); T::regs().cr1().modify(|reg| { reg.set_start(true); }); @@ -504,8 +503,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if sr1.start() { Poll::Ready(Ok(())) } else { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. + // When pending, (re-)enable interrupts to wake us up. Self::enable_interrupts(); Poll::Pending } @@ -520,7 +518,6 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { } // Set up current address, we're trying to talk to - Self::enable_interrupts(); T::regs().dr().write(|reg| reg.set_dr(address << 1)); // Wait for the address to be acknowledged @@ -533,8 +530,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if sr1.addr() { Poll::Ready(Ok(())) } else { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. + // When pending, (re-)enable interrupts to wake us up. Self::enable_interrupts(); Poll::Pending } @@ -548,7 +544,6 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { } // Wait for bytes to be sent, or an error to occur. - Self::enable_interrupts(); let poll_error = poll_fn(|cx| { state.waker.register(cx.waker()); @@ -557,8 +552,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { // identical poll_fn check_and_clear matches. Err(e) => Poll::Ready(Err::(e)), Ok(_) => { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. + // When pending, (re-)enable interrupts to wake us up. Self::enable_interrupts(); Poll::Pending } @@ -580,7 +574,6 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { // 18.3.8 “Master transmitter: In the interrupt routine after the EOT interrupt, disable DMA // requests then wait for a BTF event before programming the Stop condition.” - Self::enable_interrupts(); poll_fn(|cx| { state.waker.register(cx.waker()); @@ -590,8 +583,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if sr1.btf() { Poll::Ready(Ok(())) } else { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. + // When pending, (re-)enable interrupts to wake us up. Self::enable_interrupts(); Poll::Pending } @@ -672,7 +664,6 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if frame.send_start() { // Send a START condition and set ACK bit - Self::enable_interrupts(); T::regs().cr1().modify(|reg| { reg.set_start(true); reg.set_ack(true); @@ -688,8 +679,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if sr1.start() { Poll::Ready(Ok(())) } else { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. + // When pending, (re-)enable interrupts to wake us up. Self::enable_interrupts(); Poll::Pending } @@ -704,7 +694,6 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { } // Set up current address, we're trying to talk to - Self::enable_interrupts(); T::regs().dr().write(|reg| reg.set_dr((address << 1) + 1)); // Wait for the address to be acknowledged @@ -717,8 +706,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { if sr1.addr() { Poll::Ready(Ok(())) } else { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. + // When pending, (re-)enable interrupts to wake us up. Self::enable_interrupts(); Poll::Pending } @@ -753,15 +741,13 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { } // Wait for bytes to be received, or an error to occur. - Self::enable_interrupts(); let poll_error = poll_fn(|cx| { state.waker.register(cx.waker()); match Self::check_and_clear_error_flags() { Err(e) => Poll::Ready(Err::(e)), _ => { - // If we need to go around, then re-enable the interrupts, otherwise nothing - // can wake us up and we'll hang. + // When pending, (re-)enable interrupts to wake us up. Self::enable_interrupts(); Poll::Pending } From 7e44db099ca6a475af9f22fd0ebb85845553a570 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 27 Mar 2024 00:33:04 +0100 Subject: [PATCH 08/13] Move FrameOptions and related function to module itself --- embassy-stm32/src/i2c/mod.rs | 130 +++++++++++++++++++++++++++++++++++ embassy-stm32/src/i2c/v1.rs | 129 +--------------------------------- 2 files changed, 131 insertions(+), 128 deletions(-) diff --git a/embassy-stm32/src/i2c/mod.rs b/embassy-stm32/src/i2c/mod.rs index 6700f0f7d..85381047d 100644 --- a/embassy-stm32/src/i2c/mod.rs +++ b/embassy-stm32/src/i2c/mod.rs @@ -6,6 +6,7 @@ mod _version; use core::future::Future; +use core::iter; use core::marker::PhantomData; use embassy_hal_internal::{into_ref, Peripheral, PeripheralRef}; @@ -335,3 +336,132 @@ impl<'d, T: Instance, TXDMA: TxDma, RXDMA: RxDma> embedded_hal_async::i2c: self.transaction(address, operations).await } } + +/// Frame type in I2C transaction. +/// +/// This tells each method what kind of framing to use, to generate a (repeated) start condition (ST +/// or SR), and/or a stop condition (SP). For read operations, this also controls whether to send an +/// ACK or NACK after the last byte received. +/// +/// For write operations, the following options are identical because they differ only in the (N)ACK +/// treatment relevant for read operations: +/// +/// - `FirstFrame` and `FirstAndNextFrame` +/// - `NextFrame` and `LastFrameNoStop` +/// +/// Abbreviations used below: +/// +/// - `ST` = start condition +/// - `SR` = repeated start condition +/// - `SP` = stop condition +/// - `ACK`/`NACK` = last byte in read operation +#[derive(Copy, Clone)] +enum FrameOptions { + /// `[ST/SR]+[NACK]+[SP]` First frame (of this type) in transaction and also last frame overall. + FirstAndLastFrame, + /// `[ST/SR]+[NACK]` First frame of this type in transaction, last frame in a read operation but + /// not the last frame overall. + FirstFrame, + /// `[ST/SR]+[ACK]` First frame of this type in transaction, neither last frame overall nor last + /// frame in a read operation. + FirstAndNextFrame, + /// `[ACK]` Middle frame in a read operation (neither first nor last). + NextFrame, + /// `[NACK]+[SP]` Last frame overall in this transaction but not the first frame. + LastFrame, + /// `[NACK]` Last frame in a read operation but not last frame overall in this transaction. + LastFrameNoStop, +} + +impl FrameOptions { + /// Sends start or repeated start condition before transfer. + fn send_start(self) -> bool { + match self { + Self::FirstAndLastFrame | Self::FirstFrame | Self::FirstAndNextFrame => true, + Self::NextFrame | Self::LastFrame | Self::LastFrameNoStop => false, + } + } + + /// Sends stop condition after transfer. + fn send_stop(self) -> bool { + match self { + Self::FirstAndLastFrame | Self::LastFrame => true, + Self::FirstFrame | Self::FirstAndNextFrame | Self::NextFrame | Self::LastFrameNoStop => false, + } + } + + /// Sends NACK after last byte received, indicating end of read operation. + fn send_nack(self) -> bool { + match self { + Self::FirstAndLastFrame | Self::FirstFrame | Self::LastFrame | Self::LastFrameNoStop => true, + Self::FirstAndNextFrame | Self::NextFrame => false, + } + } +} + +/// Iterates over operations in transaction. +/// +/// Returns necessary frame options for each operation to uphold the [transaction contract] and have +/// the right start/stop/(N)ACK conditions on the wire. +/// +/// [transaction contract]: embedded_hal_1::i2c::I2c::transaction +fn operation_frames<'a, 'b: 'a>( + operations: &'a mut [embedded_hal_1::i2c::Operation<'b>], +) -> Result, FrameOptions)>, Error> { + use embedded_hal_1::i2c::Operation; + + // Check empty read buffer before starting transaction. Otherwise, we would risk halting with an + // error in the middle of the transaction. + if operations.iter().any(|op| match op { + Operation::Read(read) => read.is_empty(), + Operation::Write(_) => false, + }) { + return Err(Error::Overrun); + } + + let mut operations = operations.iter_mut().peekable(); + + let mut next_first_frame = true; + + Ok(iter::from_fn(move || { + let Some(op) = operations.next() else { + return None; + }; + + // Is `op` first frame of its type? + let first_frame = next_first_frame; + let next_op = operations.peek(); + + // Get appropriate frame options as combination of the following properties: + // + // - For each first operation of its type, generate a (repeated) start condition. + // - For the last operation overall in the entire transaction, generate a stop condition. + // - For read operations, check the next operation: if it is also a read operation, we merge + // these and send ACK for all bytes in the current operation; send NACK only for the final + // read operation's last byte (before write or end of entire transaction) to indicate last + // byte read and release the bus for transmission of the bus master's next byte (or stop). + // + // We check the third property unconditionally, i.e. even for write opeartions. This is okay + // because the resulting frame options are identical for write operations. + let frame = match (first_frame, next_op) { + (true, None) => FrameOptions::FirstAndLastFrame, + (true, Some(Operation::Read(_))) => FrameOptions::FirstAndNextFrame, + (true, Some(Operation::Write(_))) => FrameOptions::FirstFrame, + // + (false, None) => FrameOptions::LastFrame, + (false, Some(Operation::Read(_))) => FrameOptions::NextFrame, + (false, Some(Operation::Write(_))) => FrameOptions::LastFrameNoStop, + }; + + // Pre-calculate if `next_op` is the first operation of its type. We do this here and not at + // the beginning of the loop because we hand out `op` as iterator value and cannot access it + // anymore in the next iteration. + next_first_frame = match (&op, next_op) { + (_, None) => false, + (Operation::Read(_), Some(Operation::Write(_))) | (Operation::Write(_), Some(Operation::Read(_))) => true, + (Operation::Read(_), Some(Operation::Read(_))) | (Operation::Write(_), Some(Operation::Write(_))) => false, + }; + + Some((op, frame)) + })) +} diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 7c0bb31d6..be7f91a9a 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -5,7 +5,7 @@ //! All other devices (as of 2023-12-28) use [`v2`](super::v2) instead. use core::future::poll_fn; -use core::{iter, task::Poll}; +use core::task::Poll; use embassy_embedded_hal::SetConfig; use embassy_futures::select::{select, Either}; @@ -41,133 +41,6 @@ pub unsafe fn on_interrupt() { }); } -/// Frame type in I2C transaction. -/// -/// This tells each method what kind of framing to use, to generate a (repeated) start condition (ST -/// or SR), and/or a stop condition (SP). For read operations, this also controls whether to send an -/// ACK or NACK after the last byte received. -/// -/// For write operations, the following options are identical because they differ only in the (N)ACK -/// treatment relevant for read operations: -/// -/// - `FirstFrame` and `FirstAndNextFrame` -/// - `NextFrame` and `LastFrameNoStop` -/// -/// Abbreviations used below: -/// -/// - `ST` = start condition -/// - `SR` = repeated start condition -/// - `SP` = stop condition -/// - `ACK`/`NACK` = last byte in read operation -#[derive(Copy, Clone)] -enum FrameOptions { - /// `[ST/SR]+[NACK]+[SP]` First frame (of this type) in transaction and also last frame overall. - FirstAndLastFrame, - /// `[ST/SR]+[NACK]` First frame of this type in transaction, last frame in a read operation but - /// not the last frame overall. - FirstFrame, - /// `[ST/SR]+[ACK]` First frame of this type in transaction, neither last frame overall nor last - /// frame in a read operation. - FirstAndNextFrame, - /// `[ACK]` Middle frame in a read operation (neither first nor last). - NextFrame, - /// `[NACK]+[SP]` Last frame overall in this transaction but not the first frame. - LastFrame, - /// `[NACK]` Last frame in a read operation but not last frame overall in this transaction. - LastFrameNoStop, -} - -impl FrameOptions { - /// Sends start or repeated start condition before transfer. - fn send_start(self) -> bool { - match self { - Self::FirstAndLastFrame | Self::FirstFrame | Self::FirstAndNextFrame => true, - Self::NextFrame | Self::LastFrame | Self::LastFrameNoStop => false, - } - } - - /// Sends stop condition after transfer. - fn send_stop(self) -> bool { - match self { - Self::FirstAndLastFrame | Self::LastFrame => true, - Self::FirstFrame | Self::FirstAndNextFrame | Self::NextFrame | Self::LastFrameNoStop => false, - } - } - - /// Sends NACK after last byte received, indicating end of read operation. - fn send_nack(self) -> bool { - match self { - Self::FirstAndLastFrame | Self::FirstFrame | Self::LastFrame | Self::LastFrameNoStop => true, - Self::FirstAndNextFrame | Self::NextFrame => false, - } - } -} - -/// Iterates over operations in transaction. -/// -/// Returns necessary frame options for each operation to uphold the [transaction contract] and have -/// the right start/stop/(N)ACK conditions on the wire. -/// -/// [transaction contract]: embedded_hal_1::i2c::I2c::transaction -fn operation_frames<'a, 'b: 'a>( - operations: &'a mut [Operation<'b>], -) -> Result, FrameOptions)>, Error> { - // Check empty read buffer before starting transaction. Otherwise, we would risk halting with an - // error in the middle of the transaction. - if operations.iter().any(|op| match op { - Operation::Read(read) => read.is_empty(), - Operation::Write(_) => false, - }) { - return Err(Error::Overrun); - } - - let mut operations = operations.iter_mut().peekable(); - - let mut next_first_frame = true; - - Ok(iter::from_fn(move || { - let Some(op) = operations.next() else { - return None; - }; - - // Is `op` first frame of its type? - let first_frame = next_first_frame; - let next_op = operations.peek(); - - // Get appropriate frame options as combination of the following properties: - // - // - For each first operation of its type, generate a (repeated) start condition. - // - For the last operation overall in the entire transaction, generate a stop condition. - // - For read operations, check the next operation: if it is also a read operation, we merge - // these and send ACK for all bytes in the current operation; send NACK only for the final - // read operation's last byte (before write or end of entire transaction) to indicate last - // byte read and release the bus for transmission of the bus master's next byte (or stop). - // - // We check the third property unconditionally, i.e. even for write opeartions. This is okay - // because the resulting frame options are identical for write operations. - let frame = match (first_frame, next_op) { - (true, None) => FrameOptions::FirstAndLastFrame, - (true, Some(Operation::Read(_))) => FrameOptions::FirstAndNextFrame, - (true, Some(Operation::Write(_))) => FrameOptions::FirstFrame, - // - (false, None) => FrameOptions::LastFrame, - (false, Some(Operation::Read(_))) => FrameOptions::NextFrame, - (false, Some(Operation::Write(_))) => FrameOptions::LastFrameNoStop, - }; - - // Pre-calculate if `next_op` is the first operation of its type. We do this here and not at - // the beginning of the loop because we hand out `op` as iterator value and cannot access it - // anymore in the next iteration. - next_first_frame = match (&op, next_op) { - (_, None) => false, - (Operation::Read(_), Some(Operation::Write(_))) | (Operation::Write(_), Some(Operation::Read(_))) => true, - (Operation::Read(_), Some(Operation::Read(_))) | (Operation::Write(_), Some(Operation::Write(_))) => false, - }; - - Some((op, frame)) - })) -} - impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { pub(crate) fn init(&mut self, freq: Hertz, _config: Config) { T::regs().cr1().modify(|reg| { From 54d7d495135a2d4a17457d7fdb5d9306f598acc2 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 27 Mar 2024 01:07:42 +0100 Subject: [PATCH 09/13] Refactor DMA implementation of I2C v1, clarify flow of code --- embassy-stm32/src/i2c/v1.rs | 131 ++++++++++++++++++++---------------- 1 file changed, 73 insertions(+), 58 deletions(-) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index be7f91a9a..5c57a9ccc 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -142,7 +142,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { return Err(Error::Arbitration); } - // Set up current address, we're trying to talk to + // Set up current address we're trying to talk to T::regs().dr().write(|reg| reg.set_dr(addr << 1)); // Wait until address was sent @@ -235,7 +235,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { return Err(Error::Arbitration); } - // Set up current address, we're trying to talk to + // Set up current address we're trying to talk to T::regs().dr().write(|reg| reg.set_dr((addr << 1) + 1)); // Wait until address was sent @@ -331,27 +331,21 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { where TXDMA: crate::i2c::TxDma, { - let dma_transfer = unsafe { - let regs = T::regs(); - regs.cr2().modify(|w| { - // Note: Do not enable the ITBUFEN bit in the I2C_CR2 register if DMA is used for reception. - w.set_itbufen(false); - // DMA mode can be enabled for transmission by setting the DMAEN bit in the I2C_CR2 register. - w.set_dmaen(true); - // Sending NACK is not necessary (nor possible) for write transfer. - w.set_last(false); - }); - // Set the I2C_DR register address in the DMA_SxPAR register. The data will be moved to this address from the memory after each TxE event. - let dst = regs.dr().as_ptr() as *mut u8; - - let ch = &mut self.tx_dma; - let request = ch.request(); - Transfer::new_write(ch, request, write, dst, Default::default()) - }; + T::regs().cr2().modify(|w| { + // Note: Do not enable the ITBUFEN bit in the I2C_CR2 register if DMA is used for + // reception. + w.set_itbufen(false); + // DMA mode can be enabled for transmission by setting the DMAEN bit in the I2C_CR2 + // register. + w.set_dmaen(true); + // Sending NACK is not necessary (nor possible) for write transfer. + w.set_last(false); + }); + // Sentinel to disable transfer when an error occurs or future is canceled. + // TODO: Generate STOP condition on cancel? let on_drop = OnDrop::new(|| { - let regs = T::regs(); - regs.cr2().modify(|w| { + T::regs().cr2().modify(|w| { w.set_dmaen(false); w.set_iterren(false); w.set_itevten(false); @@ -390,7 +384,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { return Err(Error::Arbitration); } - // Set up current address, we're trying to talk to + // Set up current address we're trying to talk to T::regs().dr().write(|reg| reg.set_dr(address << 1)); // Wait for the address to be acknowledged @@ -416,14 +410,22 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { T::regs().sr2().read(); } + let dma_transfer = unsafe { + // Set the I2C_DR register address in the DMA_SxPAR register. The data will be moved to + // this address from the memory after each TxE event. + let dst = T::regs().dr().as_ptr() as *mut u8; + + let ch = &mut self.tx_dma; + let request = ch.request(); + Transfer::new_write(ch, request, write, dst, Default::default()) + }; + // Wait for bytes to be sent, or an error to occur. let poll_error = poll_fn(|cx| { state.waker.register(cx.waker()); match Self::check_and_clear_error_flags() { - // Unclear why the Err turbofish is necessary here? The compiler didn’t require it in the other - // identical poll_fn check_and_clear matches. - Err(e) => Poll::Ready(Err::(e)), + Err(e) => Poll::Ready(Err::<(), Error>(e)), Ok(_) => { // When pending, (re-)enable interrupts to wake us up. Self::enable_interrupts(); @@ -502,31 +504,30 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { where RXDMA: crate::i2c::RxDma, { - let buffer_len = buffer.len(); + if buffer.is_empty() { + return Err(Error::Overrun); + } - let dma_transfer = unsafe { - let regs = T::regs(); - regs.cr2().modify(|w| { - // Note: Do not enable the ITBUFEN bit in the I2C_CR2 register if DMA is used for reception. - w.set_itbufen(false); - // DMA mode can be enabled for transmission by setting the DMAEN bit in the I2C_CR2 register. - w.set_dmaen(true); - // If, in the I2C_CR2 register, the LAST bit is set, I2C - // automatically sends a NACK after the next byte following EOT_1. The user can - // generate a Stop condition in the DMA Transfer Complete interrupt routine if enabled. - w.set_last(frame.send_nack() && buffer_len != 1); - }); - // Set the I2C_DR register address in the DMA_SxPAR register. The data will be moved to this address from the memory after each TxE event. - let src = regs.dr().as_ptr() as *mut u8; + // Some branches below depend on whether the buffer contains only a single byte. + let single_byte = buffer.len() == 1; - let ch = &mut self.rx_dma; - let request = ch.request(); - Transfer::new_read(ch, request, src, buffer, Default::default()) - }; + T::regs().cr2().modify(|w| { + // Note: Do not enable the ITBUFEN bit in the I2C_CR2 register if DMA is used for + // reception. + w.set_itbufen(false); + // DMA mode can be enabled for transmission by setting the DMAEN bit in the I2C_CR2 + // register. + w.set_dmaen(true); + // If, in the I2C_CR2 register, the LAST bit is set, I2C automatically sends a NACK + // after the next byte following EOT_1. The user can generate a Stop condition in + // the DMA Transfer Complete interrupt routine if enabled. + w.set_last(frame.send_nack() && !single_byte); + }); + // Sentinel to disable transfer when an error occurs or future is canceled. + // TODO: Generate STOP condition on cancel? let on_drop = OnDrop::new(|| { - let regs = T::regs(); - regs.cr2().modify(|w| { + T::regs().cr2().modify(|w| { w.set_dmaen(false); w.set_iterren(false); w.set_itevten(false); @@ -566,7 +567,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { return Err(Error::Arbitration); } - // Set up current address, we're trying to talk to + // Set up current address we're trying to talk to T::regs().dr().write(|reg| reg.set_dr((address << 1) + 1)); // Wait for the address to be acknowledged @@ -590,7 +591,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { // 18.3.8: When a single byte must be received: the NACK must be programmed during EV6 // event, i.e. program ACK=0 when ADDR=1, before clearing ADDR flag. - if frame.send_nack() && buffer_len == 1 { + if frame.send_nack() && single_byte { T::regs().cr1().modify(|w| { w.set_ack(false); }); @@ -598,27 +599,41 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { // Clear condition by reading SR2 T::regs().sr2().read(); - } else if frame.send_nack() && buffer_len == 1 { - T::regs().cr1().modify(|w| { - w.set_ack(false); - }); + } else { + // Before starting reception of single byte (but without START condition, i.e. in case + // of continued frame), program NACK to emit at end of this byte. + if frame.send_nack() && single_byte { + T::regs().cr1().modify(|w| { + w.set_ack(false); + }); + } } - // 18.3.8: When a single byte must be received: [snip] Then the - // user can program the STOP condition either after clearing ADDR flag, or in the - // DMA Transfer Complete interrupt routine. - if frame.send_stop() && buffer_len == 1 { + // 18.3.8: When a single byte must be received: [snip] Then the user can program the STOP + // condition either after clearing ADDR flag, or in the DMA Transfer Complete interrupt + // routine. + if frame.send_stop() && single_byte { T::regs().cr1().modify(|w| { w.set_stop(true); }); } + let dma_transfer = unsafe { + // Set the I2C_DR register address in the DMA_SxPAR register. The data will be moved + // from this address from the memory after each RxE event. + let src = T::regs().dr().as_ptr() as *mut u8; + + let ch = &mut self.rx_dma; + let request = ch.request(); + Transfer::new_read(ch, request, src, buffer, Default::default()) + }; + // Wait for bytes to be received, or an error to occur. let poll_error = poll_fn(|cx| { state.waker.register(cx.waker()); match Self::check_and_clear_error_flags() { - Err(e) => Poll::Ready(Err::(e)), + Err(e) => Poll::Ready(Err::<(), Error>(e)), _ => { // When pending, (re-)enable interrupts to wake us up. Self::enable_interrupts(); @@ -636,7 +651,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { w.set_dmaen(false); }); - if frame.send_stop() && buffer_len != 1 { + if frame.send_stop() && !single_byte { T::regs().cr1().modify(|w| { w.set_stop(true); }); From 0cfb65abc225cafa89562eb80d8e61389465d7ef Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 27 Mar 2024 01:30:27 +0100 Subject: [PATCH 10/13] Add transaction stub to I2C v2 --- embassy-stm32/src/i2c/v2.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/embassy-stm32/src/i2c/v2.rs b/embassy-stm32/src/i2c/v2.rs index 8baf2849d..da3b0ee30 100644 --- a/embassy-stm32/src/i2c/v2.rs +++ b/embassy-stm32/src/i2c/v2.rs @@ -557,6 +557,21 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { Ok(()) } + /// Transaction with operations. + /// + /// Consecutive operations of same type are merged. See [transaction contract] for details. + /// + /// [transaction contract]: embedded_hal_1::i2c::I2c::transaction + pub async fn transaction(&mut self, addr: u8, operations: &mut [Operation<'_>]) -> Result<(), Error> + where + RXDMA: crate::i2c::RxDma, + TXDMA: crate::i2c::TxDma, + { + let _ = addr; + let _ = operations; + todo!() + } + // ========================= // Blocking public API From 13636556d969314843a04907d22e2921b1be388d Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 27 Mar 2024 01:41:13 +0100 Subject: [PATCH 11/13] Mark shared data structure as dead_code for I2C v2 branch --- embassy-stm32/src/i2c/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/embassy-stm32/src/i2c/mod.rs b/embassy-stm32/src/i2c/mod.rs index 85381047d..9a91e2f25 100644 --- a/embassy-stm32/src/i2c/mod.rs +++ b/embassy-stm32/src/i2c/mod.rs @@ -356,6 +356,7 @@ impl<'d, T: Instance, TXDMA: TxDma, RXDMA: RxDma> embedded_hal_async::i2c: /// - `SP` = stop condition /// - `ACK`/`NACK` = last byte in read operation #[derive(Copy, Clone)] +#[allow(dead_code)] enum FrameOptions { /// `[ST/SR]+[NACK]+[SP]` First frame (of this type) in transaction and also last frame overall. FirstAndLastFrame, @@ -373,6 +374,7 @@ enum FrameOptions { LastFrameNoStop, } +#[allow(dead_code)] impl FrameOptions { /// Sends start or repeated start condition before transfer. fn send_start(self) -> bool { @@ -405,6 +407,7 @@ impl FrameOptions { /// the right start/stop/(N)ACK conditions on the wire. /// /// [transaction contract]: embedded_hal_1::i2c::I2c::transaction +#[allow(dead_code)] fn operation_frames<'a, 'b: 'a>( operations: &'a mut [embedded_hal_1::i2c::Operation<'b>], ) -> Result, FrameOptions)>, Error> { From b52e9a60eb929facd21eab2d524781e127f3e04b Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 27 Mar 2024 10:39:33 +0100 Subject: [PATCH 12/13] Add missing check for empty buffer in asynchronous read_write() --- embassy-stm32/src/i2c/mod.rs | 4 ++++ embassy-stm32/src/i2c/v1.rs | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/embassy-stm32/src/i2c/mod.rs b/embassy-stm32/src/i2c/mod.rs index 9a91e2f25..d4d4aec5d 100644 --- a/embassy-stm32/src/i2c/mod.rs +++ b/embassy-stm32/src/i2c/mod.rs @@ -415,6 +415,10 @@ fn operation_frames<'a, 'b: 'a>( // Check empty read buffer before starting transaction. Otherwise, we would risk halting with an // error in the middle of the transaction. + // + // In principle, we could allow empty read frames within consecutive read operations, as long as + // at least one byte remains in the final (merged) read operation, but that makes the logic more + // complicated and error-prone. if operations.iter().any(|op| match op { Operation::Read(read) => read.is_empty(), Operation::Write(_) => false, diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 5c57a9ccc..d45c48b24 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -669,6 +669,12 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { RXDMA: crate::i2c::RxDma, TXDMA: crate::i2c::TxDma, { + // Check empty read buffer before starting transaction. Otherwise, we would not generate the + // stop condition below. + if read.is_empty() { + return Err(Error::Overrun); + } + self.write_frame(address, write, FrameOptions::FirstFrame).await?; self.read_frame(address, read, FrameOptions::FirstAndLastFrame).await } From bb5fcce0a095431de063aedd6eb12161d94805db Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 27 Mar 2024 10:42:38 +0100 Subject: [PATCH 13/13] Use named imports within function to make code easier to read --- embassy-stm32/src/i2c/mod.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/embassy-stm32/src/i2c/mod.rs b/embassy-stm32/src/i2c/mod.rs index d4d4aec5d..a46061d54 100644 --- a/embassy-stm32/src/i2c/mod.rs +++ b/embassy-stm32/src/i2c/mod.rs @@ -411,7 +411,7 @@ impl FrameOptions { fn operation_frames<'a, 'b: 'a>( operations: &'a mut [embedded_hal_1::i2c::Operation<'b>], ) -> Result, FrameOptions)>, Error> { - use embedded_hal_1::i2c::Operation; + use embedded_hal_1::i2c::Operation::{Read, Write}; // Check empty read buffer before starting transaction. Otherwise, we would risk halting with an // error in the middle of the transaction. @@ -420,8 +420,8 @@ fn operation_frames<'a, 'b: 'a>( // at least one byte remains in the final (merged) read operation, but that makes the logic more // complicated and error-prone. if operations.iter().any(|op| match op { - Operation::Read(read) => read.is_empty(), - Operation::Write(_) => false, + Read(read) => read.is_empty(), + Write(_) => false, }) { return Err(Error::Overrun); } @@ -452,12 +452,12 @@ fn operation_frames<'a, 'b: 'a>( // because the resulting frame options are identical for write operations. let frame = match (first_frame, next_op) { (true, None) => FrameOptions::FirstAndLastFrame, - (true, Some(Operation::Read(_))) => FrameOptions::FirstAndNextFrame, - (true, Some(Operation::Write(_))) => FrameOptions::FirstFrame, + (true, Some(Read(_))) => FrameOptions::FirstAndNextFrame, + (true, Some(Write(_))) => FrameOptions::FirstFrame, // (false, None) => FrameOptions::LastFrame, - (false, Some(Operation::Read(_))) => FrameOptions::NextFrame, - (false, Some(Operation::Write(_))) => FrameOptions::LastFrameNoStop, + (false, Some(Read(_))) => FrameOptions::NextFrame, + (false, Some(Write(_))) => FrameOptions::LastFrameNoStop, }; // Pre-calculate if `next_op` is the first operation of its type. We do this here and not at @@ -465,8 +465,8 @@ fn operation_frames<'a, 'b: 'a>( // anymore in the next iteration. next_first_frame = match (&op, next_op) { (_, None) => false, - (Operation::Read(_), Some(Operation::Write(_))) | (Operation::Write(_), Some(Operation::Read(_))) => true, - (Operation::Read(_), Some(Operation::Read(_))) | (Operation::Write(_), Some(Operation::Write(_))) => false, + (Read(_), Some(Write(_))) | (Write(_), Some(Read(_))) => true, + (Read(_), Some(Read(_))) | (Write(_), Some(Write(_))) => false, }; Some((op, frame))