stm32/i2c: Rename FrameOptions enum to OperationFraming

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<impl IntoIterator<Item = (&'a mut embedded_hal_1::i2c::Operation<'b>, FrameOptions)>, Error> {
+) -> Result<impl IntoIterator<Item = (&'a mut embedded_hal_1::i2c::Operation<'b>, 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 (&current_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?,