From 399ec8d90f1c4f62fd20d21dabbb9ce64c6986eb Mon Sep 17 00:00:00 2001 From: HybridChild Date: Sun, 27 Jul 2025 10:35:52 +0200 Subject: stm32/i2c: Rename FrameOptions enum to OperationFraming --- embassy-stm32/src/i2c/mod.rs | 185 +++++++++++++++++++++++++------------------ embassy-stm32/src/i2c/v1.rs | 28 +++---- 2 files changed, 124 insertions(+), 89 deletions(-) diff --git a/embassy-stm32/src/i2c/mod.rs b/embassy-stm32/src/i2c/mod.rs index 5fb49f943..f51682900 100644 --- a/embassy-stm32/src/i2c/mod.rs +++ b/embassy-stm32/src/i2c/mod.rs @@ -435,88 +435,117 @@ impl<'d, IM: MasterMode> embedded_hal_async::i2c::I2c for I2c<'d, Async, IM> { } } -/// Frame type in I2C transaction. +/// Operation framing configuration for I2C transactions. /// -/// 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. +/// This determines the I2C frame boundaries for each operation within a transaction, +/// controlling the generation of start conditions (ST or SR), stop conditions (SP), +/// and ACK/NACK behavior for read operations. /// -/// For write operations, the following options are identical because they differ only in the (N)ACK -/// treatment relevant for read operations: +/// For write operations, some framing configurations are functionally identical +/// because they differ only in ACK/NACK treatment which is relevant only for reads: /// -/// - `FirstFrame` and `FirstAndNextFrame` -/// - `NextFrame` and `LastFrameNoStop` -/// -/// Abbreviations used below: +/// - `First` and `FirstAndNext` behave identically for writes +/// - `Next` and `LastNoStop` behave identically for writes /// +/// **Framing Legend:** /// - `ST` = start condition -/// - `SR` = repeated start condition +/// - `SR` = repeated start condition /// - `SP` = stop condition -/// - `ACK`/`NACK` = last byte in read operation +/// - `ACK/NACK` = acknowledgment behavior for the final byte of read operations #[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, - /// `[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, +enum OperationFraming { + /// `[ST/SR]+[NACK]+[SP]` - First operation of its type in the transaction and also the final operation overall. + FirstAndLast, + /// `[ST/SR]+[NACK]` - First operation of its type in the transaction, final operation in a read sequence, but not the final operation overall. + First, + /// `[ST/SR]+[ACK]` - First operation of its type in the transaction, but neither the final operation overall nor the final operation in a read sequence. + FirstAndNext, + /// `[ACK]` - Continuation operation in a read sequence (neither first nor last). + Next, + /// `[NACK]+[SP]` - Final operation overall in the transaction, but not the first operation of its type. + Last, + /// `[NACK]` - Final operation in a read sequence, but not the final operation overall in the transaction. + LastNoStop, } #[allow(dead_code)] -impl FrameOptions { - /// Sends start or repeated start condition before transfer. +impl OperationFraming { + /// Returns true if a start or repeated start condition should be generated before this operation. fn send_start(self) -> bool { match self { - Self::FirstAndLastFrame | Self::FirstFrame | Self::FirstAndNextFrame => true, - Self::NextFrame | Self::LastFrame | Self::LastFrameNoStop => false, + Self::FirstAndLast | Self::First | Self::FirstAndNext => true, + Self::Next | Self::Last | Self::LastNoStop => false, } } - /// Sends stop condition after transfer. + /// Returns true if a stop condition should be generated after this operation. fn send_stop(self) -> bool { match self { - Self::FirstAndLastFrame | Self::LastFrame => true, - Self::FirstFrame | Self::FirstAndNextFrame | Self::NextFrame | Self::LastFrameNoStop => false, + Self::FirstAndLast | Self::Last => true, + Self::First | Self::FirstAndNext | Self::Next | Self::LastNoStop => false, } } - /// Sends NACK after last byte received, indicating end of read operation. + /// Returns true if NACK should be sent after the last byte received in a read operation. + /// + /// This signals the end of a read sequence and releases the bus for the master's + /// next transmission (or stop condition). fn send_nack(self) -> bool { match self { - Self::FirstAndLastFrame | Self::FirstFrame | Self::LastFrame | Self::LastFrameNoStop => true, - Self::FirstAndNextFrame | Self::NextFrame => false, + Self::FirstAndLast | Self::First | Self::Last | Self::LastNoStop => true, + Self::FirstAndNext | Self::Next => false, } } } -/// Iterates over operations in transaction. +/// Analyzes I2C transaction operations and assigns appropriate framing to each. +/// +/// This function processes a sequence of I2C operations and determines the correct +/// framing configuration for each operation to ensure proper I2C protocol compliance. +/// It handles the complex logic of: +/// +/// - Generating start conditions for the first operation of each type (read/write) +/// - Generating stop conditions for the final operation in the entire transaction +/// - Managing ACK/NACK behavior for read operations, including merging consecutive reads +/// - Ensuring proper bus handoff between different operation types +/// +/// **Transaction Contract Compliance:** +/// The framing assignments ensure compliance with the embedded-hal I2C transaction contract, +/// where consecutive operations of the same type are logically merged while maintaining +/// proper protocol boundaries. /// -/// Returns necessary frame options for each operation to uphold the [transaction contract] and have -/// the right start/stop/(N)ACK conditions on the wire. +/// **Error Handling:** +/// Returns an error if any read operation has an empty buffer, as this would create +/// an invalid I2C transaction that could halt mid-execution. /// -/// [transaction contract]: embedded_hal_1::i2c::I2c::transaction +/// # Arguments +/// * `operations` - Mutable slice of I2C operations from embedded-hal +/// +/// # Returns +/// An iterator over (operation, framing) pairs, or an error if the transaction is invalid +/// +/// # Example +/// ```rust +/// for (op, framing) in assign_operation_framing(operations)? { +/// match op { +/// Operation::Read(buffer) => self.read_with_framing(addr, buffer, framing).await?, +/// Operation::Write(data) => self.write_with_framing(addr, data, framing).await?, +/// } +/// } +/// ``` #[allow(dead_code)] -fn operation_frames<'a, 'b: 'a>( +fn assign_operation_framing<'a, 'b: 'a>( operations: &'a mut [embedded_hal_1::i2c::Operation<'b>], -) -> Result, FrameOptions)>, Error> { +) -> Result, OperationFraming)>, Error> { 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. + // Validate that no read operations have empty buffers before starting the transaction. + // Empty read operations would risk halting with an error mid-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. + // Note: We could theoretically allow empty read operations within consecutive read + // sequences as long as the final merged read has at least one byte, but this would + // complicate the logic significantly and create error-prone edge cases. if operations.iter().any(|op| match op { Read(read) => read.is_empty(), Write(_) => false, @@ -525,46 +554,52 @@ fn operation_frames<'a, 'b: 'a>( } let mut operations = operations.iter_mut().peekable(); - - let mut next_first_frame = true; + let mut next_first_operation = true; Ok(iter::from_fn(move || { - let op = operations.next()?; + let current_op = operations.next()?; - // Is `op` first frame of its type? - let first_frame = next_first_frame; + // Determine if this is the first operation of its type (read or write) + let is_first_of_type = next_first_operation; let next_op = operations.peek(); - // Get appropriate frame options as combination of the following properties: + // Compute the appropriate framing based on three key 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). + // 1. **Start Condition**: Generate (repeated) start for first operation of each type + // 2. **Stop Condition**: Generate stop for the final operation in the entire transaction + // 3. **ACK/NACK for Reads**: For read operations, send ACK if more reads follow in the + // sequence, or NACK for the final read in a sequence (before write or transaction end) // - // 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(Read(_))) => FrameOptions::FirstAndNextFrame, - (true, Some(Write(_))) => FrameOptions::FirstFrame, - // - (false, None) => FrameOptions::LastFrame, - (false, Some(Read(_))) => FrameOptions::NextFrame, - (false, Some(Write(_))) => FrameOptions::LastFrameNoStop, + // The third property is checked for all operations since the resulting framing + // configurations are identical for write operations regardless of ACK/NACK treatment. + let framing = match (is_first_of_type, next_op) { + // First operation of type, and it's also the final operation overall + (true, None) => OperationFraming::FirstAndLast, + // First operation of type, next operation is also a read (continue read sequence) + (true, Some(Read(_))) => OperationFraming::FirstAndNext, + // First operation of type, next operation is write (end current sequence) + (true, Some(Write(_))) => OperationFraming::First, + + // Continuation operation, and it's the final operation overall + (false, None) => OperationFraming::Last, + // Continuation operation, next operation is also a read (continue read sequence) + (false, Some(Read(_))) => OperationFraming::Next, + // Continuation operation, next operation is write (end current sequence, no stop) + (false, Some(Write(_))) => OperationFraming::LastNoStop, }; - // 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) { + // Pre-calculate whether the next operation will be the first of its type. + // This is done here because we consume `current_op` as the iterator value + // and cannot access it in the next iteration. + next_first_operation = match (¤t_op, next_op) { + // No next operation (_, None) => false, + // Operation type changes: next will be first of its type (Read(_), Some(Write(_))) | (Write(_), Some(Read(_))) => true, + // Operation type continues: next will not be first of its type (Read(_), Some(Read(_))) | (Write(_), Some(Write(_))) => false, }; - Some((op, frame)) + Some((current_op, framing)) })) } diff --git a/embassy-stm32/src/i2c/v1.rs b/embassy-stm32/src/i2c/v1.rs index a1ad9caef..7d2b731d5 100644 --- a/embassy-stm32/src/i2c/v1.rs +++ b/embassy-stm32/src/i2c/v1.rs @@ -151,7 +151,7 @@ impl<'d, M: Mode, IM: MasterMode> I2c<'d, M, IM> { Ok(sr1) } - fn write_bytes(&mut self, addr: u8, bytes: &[u8], timeout: Timeout, frame: FrameOptions) -> Result<(), Error> { + fn write_bytes(&mut self, addr: u8, bytes: &[u8], timeout: Timeout, frame: OperationFraming) -> Result<(), Error> { if frame.send_start() { // Send a START condition @@ -239,7 +239,7 @@ impl<'d, M: Mode, IM: MasterMode> I2c<'d, M, IM> { addr: u8, buffer: &mut [u8], timeout: Timeout, - frame: FrameOptions, + frame: OperationFraming, ) -> Result<(), Error> { let Some((last, buffer)) = buffer.split_last_mut() else { return Err(Error::Overrun); @@ -299,12 +299,12 @@ impl<'d, M: Mode, IM: MasterMode> I2c<'d, M, IM> { /// Blocking read. pub fn blocking_read(&mut self, addr: u8, read: &mut [u8]) -> Result<(), Error> { - self.blocking_read_timeout(addr, read, self.timeout(), FrameOptions::FirstAndLastFrame) + self.blocking_read_timeout(addr, read, self.timeout(), OperationFraming::FirstAndLast) } /// Blocking write. pub fn blocking_write(&mut self, addr: u8, write: &[u8]) -> Result<(), Error> { - self.write_bytes(addr, write, self.timeout(), FrameOptions::FirstAndLastFrame)?; + self.write_bytes(addr, write, self.timeout(), OperationFraming::FirstAndLast)?; // Fallthrough is success Ok(()) @@ -320,8 +320,8 @@ impl<'d, M: Mode, IM: MasterMode> I2c<'d, M, IM> { let timeout = self.timeout(); - self.write_bytes(addr, write, timeout, FrameOptions::FirstFrame)?; - self.blocking_read_timeout(addr, read, timeout, FrameOptions::FirstAndLastFrame)?; + self.write_bytes(addr, write, timeout, OperationFraming::First)?; + self.blocking_read_timeout(addr, read, timeout, OperationFraming::FirstAndLast)?; Ok(()) } @@ -334,7 +334,7 @@ impl<'d, M: Mode, IM: MasterMode> I2c<'d, M, IM> { pub fn blocking_transaction(&mut self, addr: u8, operations: &mut [Operation<'_>]) -> Result<(), Error> { let timeout = self.timeout(); - for (op, frame) in operation_frames(operations)? { + for (op, frame) in assign_operation_framing(operations)? { match op { Operation::Read(read) => self.blocking_read_timeout(addr, read, timeout, frame)?, Operation::Write(write) => self.write_bytes(addr, write, timeout, frame)?, @@ -356,7 +356,7 @@ impl<'d, M: Mode, IM: MasterMode> I2c<'d, M, IM> { } impl<'d, IM: MasterMode> I2c<'d, Async, IM> { - async fn write_frame(&mut self, address: u8, write: &[u8], frame: FrameOptions) -> Result<(), Error> { + async fn write_frame(&mut self, address: u8, write: &[u8], frame: OperationFraming) -> Result<(), Error> { self.info.regs.cr2().modify(|w| { // Note: Do not enable the ITBUFEN bit in the I2C_CR2 register if DMA is used for // reception. @@ -502,7 +502,7 @@ impl<'d, IM: MasterMode> I2c<'d, Async, IM> { /// Write. pub async fn write(&mut self, address: u8, write: &[u8]) -> Result<(), Error> { - self.write_frame(address, write, FrameOptions::FirstAndLastFrame) + self.write_frame(address, write, OperationFraming::FirstAndLast) .await?; Ok(()) @@ -510,13 +510,13 @@ impl<'d, IM: MasterMode> I2c<'d, Async, IM> { /// Read. pub async fn read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error> { - self.read_frame(address, buffer, FrameOptions::FirstAndLastFrame) + self.read_frame(address, buffer, OperationFraming::FirstAndLast) .await?; Ok(()) } - async fn read_frame(&mut self, address: u8, buffer: &mut [u8], frame: FrameOptions) -> Result<(), Error> { + async fn read_frame(&mut self, address: u8, buffer: &mut [u8], frame: OperationFraming) -> Result<(), Error> { if buffer.is_empty() { return Err(Error::Overrun); } @@ -680,8 +680,8 @@ impl<'d, IM: MasterMode> I2c<'d, Async, IM> { return Err(Error::Overrun); } - self.write_frame(address, write, FrameOptions::FirstFrame).await?; - self.read_frame(address, read, FrameOptions::FirstAndLastFrame).await + self.write_frame(address, write, OperationFraming::First).await?; + self.read_frame(address, read, OperationFraming::FirstAndLast).await } /// Transaction with operations. @@ -690,7 +690,7 @@ impl<'d, IM: MasterMode> I2c<'d, Async, IM> { /// /// [transaction contract]: embedded_hal_1::i2c::I2c::transaction pub async fn transaction(&mut self, addr: u8, operations: &mut [Operation<'_>]) -> Result<(), Error> { - for (op, frame) in operation_frames(operations)? { + for (op, frame) in assign_operation_framing(operations)? { match op { Operation::Read(read) => self.read_frame(addr, read, frame).await?, Operation::Write(write) => self.write_frame(addr, write, frame).await?, -- cgit