From 7c08616c02d8e0f49daa7402fd000887d13d4fe5 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 20 Mar 2024 02:10:45 +0100 Subject: [PATCH 1/5] Introduce frame options to control start/stop conditions --- embassy-stm32/src/i2c/v1.rs | 197 +++++++++++++++++++++++++----------- 1 file changed, 138 insertions(+), 59 deletions(-) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index cbbc201de..0843f45cf 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -41,6 +41,68 @@ 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 +#[derive(Copy, Clone)] +enum FrameOptions { + /// `[ST/SR]+[NACK]+[SP]` First frame (of this type) in operation and last frame overall in this + /// transaction. + 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, + } + } +} + 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| { @@ -124,46 +186,57 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { Ok(sr1) } - fn write_bytes(&mut self, addr: u8, bytes: &[u8], timeout: Timeout) -> Result<(), Error> { - // Send a START condition + fn write_bytes(&mut self, addr: u8, bytes: &[u8], timeout: Timeout, frame: FrameOptions) -> Result<(), Error> { + if frame.send_start() { + // Send a START condition - T::regs().cr1().modify(|reg| { - reg.set_start(true); - }); + T::regs().cr1().modify(|reg| { + reg.set_start(true); + }); - // Wait until START condition was generated - while !Self::check_and_clear_error_flags()?.start() { - timeout.check()?; + // Wait until START condition was generated + while !Self::check_and_clear_error_flags()?.start() { + 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()?; + } + + // 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 + // Wait for the address to be acknowledged + // Check for any I2C errors. If a NACK occurs, the ADDR bit will never be set. + while !Self::check_and_clear_error_flags()?.addr() { + timeout.check()?; + } + + // Clear condition by reading SR2 + let _ = T::regs().sr2().read(); } - // 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()?; - } - - // 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 - // Wait for the address to be acknowledged - // Check for any I2C errors. If a NACK occurs, the ADDR bit will never be set. - while !Self::check_and_clear_error_flags()?.addr() { - timeout.check()?; - } - - // Clear condition by reading SR2 - let _ = T::regs().sr2().read(); - // Send bytes for c in bytes { self.send_byte(*c, timeout)?; } + 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 Ok(()) } @@ -205,8 +278,18 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { Ok(value) } - fn blocking_read_timeout(&mut self, addr: u8, buffer: &mut [u8], timeout: Timeout) -> Result<(), Error> { - if let Some((last, buffer)) = buffer.split_last_mut() { + fn blocking_read_timeout( + &mut self, + addr: u8, + buffer: &mut [u8], + timeout: Timeout, + frame: FrameOptions, + ) -> Result<(), Error> { + let Some((last, buffer)) = buffer.split_last_mut() else { + return Err(Error::Overrun); + }; + + if frame.send_start() { // Send a START condition and set ACK bit T::regs().cr1().modify(|reg| { reg.set_start(true); @@ -237,49 +320,45 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { // Clear condition by reading SR2 let _ = T::regs().sr2().read(); + } - // Receive bytes into buffer - for c in buffer { - *c = self.recv_byte(timeout)?; - } + // Receive bytes into buffer + for c in buffer { + *c = self.recv_byte(timeout)?; + } - // Prepare to send NACK then STOP after next byte - T::regs().cr1().modify(|reg| { + // Prepare to send NACK then STOP after next byte + T::regs().cr1().modify(|reg| { + if frame.send_nack() { reg.set_ack(false); + } + if frame.send_stop() { reg.set_stop(true); - }); + } + }); - // Receive last byte - *last = self.recv_byte(timeout)?; + // 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(()) - } else { - Err(Error::Overrun) } + + // Fallthrough is success + Ok(()) } /// Blocking read. pub fn blocking_read(&mut self, addr: u8, read: &mut [u8]) -> Result<(), Error> { - self.blocking_read_timeout(addr, read, self.timeout()) + self.blocking_read_timeout(addr, read, self.timeout(), FrameOptions::FirstAndLastFrame) } /// Blocking write. pub fn blocking_write(&mut self, addr: u8, write: &[u8]) -> Result<(), Error> { - let timeout = self.timeout(); - - self.write_bytes(addr, write, timeout)?; - // 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()?; - } + self.write_bytes(addr, write, self.timeout(), FrameOptions::FirstAndLastFrame)?; // Fallthrough is success Ok(()) @@ -289,8 +368,8 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { pub fn blocking_write_read(&mut self, addr: u8, write: &[u8], read: &mut [u8]) -> Result<(), Error> { let timeout = self.timeout(); - self.write_bytes(addr, write, timeout)?; - self.blocking_read_timeout(addr, read, timeout)?; + self.write_bytes(addr, write, timeout, FrameOptions::FirstFrame)?; + self.blocking_read_timeout(addr, read, timeout, FrameOptions::FirstAndLastFrame)?; Ok(()) } From c96062fbcdb32b5ecc9b9d3507097f09bbe4b7ca Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 20 Mar 2024 02:55:18 +0100 Subject: [PATCH 2/5] Implement blocking transaction handling for I2C v1 --- embassy-stm32/src/i2c/mod.rs | 6 ++-- embassy-stm32/src/i2c/v1.rs | 61 ++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/embassy-stm32/src/i2c/mod.rs b/embassy-stm32/src/i2c/mod.rs index 2416005b5..2c606c3c9 100644 --- a/embassy-stm32/src/i2c/mod.rs +++ b/embassy-stm32/src/i2c/mod.rs @@ -311,10 +311,10 @@ impl<'d, T: Instance> embedded_hal_1::i2c::I2c for I2c<'d, T, NoDma, NoDma> { fn transaction( &mut self, - _address: u8, - _operations: &mut [embedded_hal_1::i2c::Operation<'_>], + address: u8, + operations: &mut [embedded_hal_1::i2c::Operation<'_>], ) -> Result<(), Self::Error> { - todo!(); + self.blocking_transaction(address, operations) } } diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 0843f45cf..678568706 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -10,6 +10,7 @@ use core::task::Poll; use embassy_embedded_hal::SetConfig; use embassy_futures::select::{select, Either}; use embassy_hal_internal::drop::OnDrop; +use embedded_hal_1::i2c::Operation; use super::*; use crate::dma::Transfer; @@ -374,6 +375,66 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { Ok(()) } + /// Blocking transaction with operations. + /// + /// Consecutive operations of same type are merged. See [transaction contract] for details. + /// + /// [transaction contract]: embedded_hal_1::i2c::I2c::transaction + pub fn blocking_transaction(&mut self, addr: u8, operations: &mut [Operation<'_>]) -> Result<(), Error> { + 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(mut 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, + }; + + match &mut 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(()) + } + // Async #[inline] // pretty sure this should always be inlined From 8f19a2b537c90c2bf04f6e5ea9a7107f6e000067 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 20 Mar 2024 02:20:21 +0100 Subject: [PATCH 3/5] Avoid missing stop condition when write/read with empty read buffer --- embassy-stm32/src/i2c/v1.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index 678568706..c96935d8f 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -367,6 +367,12 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { /// Blocking write, restart, read. pub fn blocking_write_read(&mut self, addr: u8, write: &[u8], read: &mut [u8]) -> Result<(), Error> { + // Check empty read buffer before starting transaction. Otherwise, we would not generate the + // stop condition below. + if read.is_empty() { + return Err(Error::Overrun); + } + let timeout = self.timeout(); self.write_bytes(addr, write, timeout, FrameOptions::FirstFrame)?; @@ -381,6 +387,15 @@ 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(); let mut operations = operations.iter_mut(); From 4eb4108952ee75d75b2b575144a0db3a6380dd61 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 20 Mar 2024 03:30:38 +0100 Subject: [PATCH 4/5] Fix build for I2C v2 targets --- embassy-stm32/src/i2c/v2.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/embassy-stm32/src/i2c/v2.rs b/embassy-stm32/src/i2c/v2.rs index bd3abaac1..1ac2740df 100644 --- a/embassy-stm32/src/i2c/v2.rs +++ b/embassy-stm32/src/i2c/v2.rs @@ -4,6 +4,7 @@ use core::task::Poll; use embassy_embedded_hal::SetConfig; use embassy_hal_internal::drop::OnDrop; +use embedded_hal_1::i2c::Operation; use super::*; use crate::dma::Transfer; @@ -579,6 +580,17 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { // Automatic Stop } + /// Blocking transaction with operations. + /// + /// Consecutive operations of same type are merged. See [transaction contract] for details. + /// + /// [transaction contract]: embedded_hal_1::i2c::I2c::transaction + pub fn blocking_transaction(&mut self, addr: u8, operations: &mut [Operation<'_>]) -> Result<(), Error> { + let _ = addr; + let _ = operations; + todo!() + } + /// Blocking write multiple buffers. /// /// The buffers are concatenated in a single write transaction. From cff665f2ecf2c60358534cd4dfe06b1a469d45fa Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Wed, 20 Mar 2024 13:08:42 +0100 Subject: [PATCH 5/5] Avoid unnecessary double-reference --- 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 c96935d8f..f1ed7ca40 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -403,7 +403,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { let mut prev_op: Option<&mut Operation<'_>> = None; let mut next_op = operations.next(); - while let Some(mut op) = next_op { + 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 @@ -439,7 +439,7 @@ impl<'d, T: Instance, TXDMA, RXDMA> I2c<'d, T, TXDMA, RXDMA> { (false, Some(Operation::Write(_))) => FrameOptions::LastFrameNoStop, }; - match &mut op { + match op { Operation::Read(read) => self.blocking_read_timeout(addr, read, timeout, frame)?, Operation::Write(write) => self.write_bytes(addr, write, timeout, frame)?, }