I2C transport: Unit tests, fix surfaced bugs, small improvements and doc

Signed-off-by: Marvin Gudel <[email protected]>

Author: embediver

mctp-estack/src/i2c.rs

@@ -21,13 +21,31 @@ const MCTP_I2C_HEADER: usize = 4;
 // bytecount is limited to u8, includes MCTP payload + 1 byte i2c source
 pub const MCTP_I2C_MAXMTU: usize = u8::MAX as usize - 1;
 
+/// MCTP I2C encapsulation header
 pub struct MctpI2cHeader {
+    /// Destination address
+    ///
+    /// The 7-bit destination address of the encapsulated packet.
+    /// (Not including the SMBus R/W# bit)
     pub dest: u8,
+    /// Source address
+    ///
+    /// The 7-bit source address of the encapsulated packet.
+    /// (Not including fixed bit `[0]`)
     pub source: u8,
+    /// Byte count
+    ///
+    /// The count of bytes that follow the _Byte Count_ field up to,
+    /// but not including, the PEC byte.
     pub byte_count: usize,
 }
 
 impl MctpI2cHeader {
+    /// Encode this header
+    ///
+    /// Creates a 4-byte header with destination, command code, byte count, and source.
+    /// Returns a [BadArgument](Error::BadArgument) error when the addresses are not 7-bit,
+    /// or when the byte count is larger than [u8::MAX].
     fn encode(&self) -> Result<[u8; 4]> {
         if self.dest > 0x7f || self.source > 0x7f {
             return Err(Error::BadArgument);
@@ -43,9 +61,13 @@ impl MctpI2cHeader {
         ])
     }
 
-    fn decode(pkt: &[u8]) -> Result<Self> {
+    /// Decode a 4-byte I2C header
+    ///
+    /// Checks and decodes destination and source address,
+    /// command code, and byte count.
+    fn decode(header: &[u8]) -> Result<Self> {
         let [dest, cmd, byte_count, source] =
-            pkt.try_into().map_err(|_| Error::BadArgument)?;
+            header.try_into().map_err(|_| Error::BadArgument)?;
         if dest & 1 != 0 {
             trace!("Bad i2c dest write bit");
             return Err(Error::InvalidInput);
@@ -59,8 +81,8 @@ impl MctpI2cHeader {
             return Err(Error::InvalidInput);
         }
         Ok(Self {
-            dest,
-            source,
+            dest: dest >> 1,
+            source: source >> 1,
             byte_count: byte_count as usize,
         })
     }
@@ -81,16 +103,27 @@ impl MctpI2cEncap {
         self.own_addr
     }
 
+    /// Decode a I2C encapsulated packet
+    ///
+    /// Decodes and verifies the I2C transport header.
+    /// Optionally an included _PEC_ will be verified.
+    ///
+    /// The inner MCTP packet and the decoded header are returned on success.
+    ///
+    /// ### Errors when
+    /// - The _PEC_ is incorrect
+    /// - Header decoding fails
+    /// - The decoded byte count field does not match the packet size
     pub fn decode<'f>(
         &self,
         mut packet: &'f [u8],
         pec: bool,
-    ) -> Result<(&'f [u8], u8)> {
+    ) -> Result<(&'f [u8], MctpI2cHeader)> {
+        if packet.is_empty() {
+            return Err(Error::InvalidInput);
+        }
         if pec {
             // Remove the pec byte, check it.
-            if packet.is_empty() {
-                return Err(Error::InvalidInput);
-            }
             let packet_pec;
             (packet_pec, packet) = packet.split_last().unwrap();
             let calc_pec = smbus_pec::pec(packet);
@@ -100,13 +133,20 @@ impl MctpI2cEncap {
             }
         }
 
-        let header = MctpI2cHeader::decode(packet)?;
-        // +1 for i2c source address field
-        if header.byte_count != packet.len() + 1 {
+        let header = MctpI2cHeader::decode(
+            &packet.get(..4).ok_or(Error::InvalidInput)?,
+        )?;
+        // total packet len == byte_count + 3 (destination, command code, byte count)
+        // pec is not included
+        if header.byte_count + 3 != packet.len() {
+            trace!("Packet byte count mismatch");
             return Err(Error::InvalidInput);
         }
+        if header.dest != self.own_addr {
+            trace!("I2C destination address mismatch");
+        }
 
-        Ok((&packet[MCTP_I2C_HEADER..], header.source))
+        Ok((&packet[MCTP_I2C_HEADER..], header))
     }
 
     /// Handles a MCTP fragment with the PEC already validated
@@ -117,7 +157,7 @@ impl MctpI2cEncap {
         &self,
         packet: &[u8],
         mctp: &'f mut Stack,
-    ) -> Result<Option<(MctpMessage<'f>, u8)>> {
+    ) -> Result<Option<(MctpMessage<'f>, MctpI2cHeader)>> {
         let (mctp_packet, i2c_src) = self.decode(packet, false)?;
 
         // Pass to MCTP stack
@@ -252,7 +292,7 @@ impl MctpI2cHandler {
         &mut self,
         packet: &[u8],
         mctp: &'f mut Stack,
-    ) -> Result<Option<(MctpMessage<'f>, u8)>> {
+    ) -> Result<Option<(MctpMessage<'f>, MctpI2cHeader)>> {
         self.encap.receive_done_pec(packet, mctp)
     }
 
@@ -369,3 +409,51 @@ enum HandlerSendState {
         i2c_dest: u8,
     },
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn i2c_codec_roundtrip() {
+        let codec = MctpI2cEncap::new(0x0A);
+        const PACKET: &[u8] =
+            &[0x01, 0x00, 0x09, 0xc8, 0x00, 0x8a, 0x01, 0x00, 0x0a];
+
+        let mut buf = [0; 128];
+        let i2c_packet = codec.encode(0x0B, &PACKET, &mut buf, false).unwrap();
+
+        assert_eq!(&i2c_packet[..4], [0x16, 0x0f, 0x0a, 0x15]);
+
+        let codec = MctpI2cEncap::new(0x0B);
+        let (decoded_packet, header) = codec.decode(i2c_packet, false).unwrap();
+        assert_eq!(decoded_packet, PACKET);
+        assert_eq!(header.source, 0x0a);
+        assert_eq!(header.dest, 0x0b);
+    }
+
+    #[test]
+    fn test_partial_packet_decode() {
+        let codec = MctpI2cEncap::new(0x0A);
+
+        // Test that empty packets are handled correctly
+        let res = codec.decode(&[], false);
+        assert!(res.is_err());
+        let res = codec.decode(&[], true);
+        assert!(res.is_err());
+        // Test that packets with only partial header are handled correctly
+        let res = codec.decode(&[0x16, 0x0f], false);
+        assert!(res.is_err());
+        let res = codec.decode(&[0x16, 0x0f], true);
+        assert!(res.is_err());
+    }
+
+    #[test]
+    fn test_decode_byte_count_mismatch() {
+        let codec = MctpI2cEncap::new(0x0A);
+
+        // Try to decode a packet with a `byte count` of 0x0a followed by only 3 bytes
+        let res = codec.decode(&[0x16, 0x0f, 0x0a, 0x15, 0x01, 0x02], false);
+        assert!(res.is_err_and(|e| matches!(e, Error::InvalidInput)));
+    }
+}