Improve error handling: impl TryFrom for I2cBuffer to ensure buffer size

.circleci/config.yml

@@ -3,7 +3,7 @@ version: 2
 jobs:
   build:
     docker:
-      - image: rust:1.32
+      - image: rust:1.51
     steps:
       - checkout
       - run: rustup component add rustfmt

src/crc8.rs

@@ -1,10 +1,25 @@
 //! Helper functions for CRC8 checksum validation
 
 /// Errors which can happen in the crc8 module
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub enum Error {
+#[derive(Debug, PartialEq)]
+pub enum CrcError {
     /// CRC validation failed
     CrcError,
+    /// Invalid length (not a multiple of 3)
+    InvalidBufferSize,
+}
+
+impl<W, R> From<CrcError> for crate::i2c::Error<W, R>
+where
+    W: embedded_hal::blocking::i2c::Write,
+    R: embedded_hal::blocking::i2c::Read,
+{
+    fn from(e: CrcError) -> Self {
+        match e {
+            CrcError::CrcError => crate::i2c::Error::CrcError,
+            CrcError::InvalidBufferSize => crate::i2c::Error::InvalidBufferSize,
+        }
+    }
 }
 
 /// Calculate the CRC8 checksum.
@@ -29,16 +44,13 @@ pub fn calculate(data: &[u8]) -> u8 {
 /// The buffer must be in the form of `[d0, d1, crc01, d2, d3, crc23, ...]` where every third byte
 /// is the checksum byte of the previous two bytes
 /// If the checksum is wrong, return `Err`.
-///
-/// # Panics
-///
-/// This method will consider every third byte a checksum byte. If the buffer size is not a
-/// multiple of 3, then it will panic.
-pub fn validate(buf: &[u8]) -> Result<(), Error> {
-    assert!(buf.len() % 3 == 0, "Buffer must be a multiple of 3");
-    for chunk in buf.chunks(3) {
+pub fn validate(buf: &[u8]) -> Result<(), CrcError> {
+    if buf.len() % 3 != 0 {
+        return Err(CrcError::InvalidBufferSize);
+    }
+    for chunk in buf.chunks_exact(3) {
         if calculate(&[chunk[0], chunk[1]]) != chunk[2] {
-            return Err(Error::CrcError);
+            return Err(CrcError::CrcError);
         }
     }
     Ok(())
@@ -48,6 +60,19 @@ pub fn validate(buf: &[u8]) -> Result<(), Error> {
 mod tests {
     use crate::crc8;
 
+    #[test]
+    fn crc8_validate_empty_valid() {
+        let buffer: [u8; 0] = [];
+        crc8::validate(&buffer).unwrap()
+    }
+
+    #[test]
+    #[should_panic]
+    fn crc8_validate_one_invalid() {
+        let buffer: [u8; 1] = [3];
+        crc8::validate(&buffer).unwrap()
+    }
+
     /// Test the crc function against the test value provided in the SHTC3 datasheet (section
     /// 5.10).
     #[test]
@@ -57,14 +82,15 @@ mod tests {
     }
 
     #[test]
-    fn crc8_validate_empty() {
-        crc8::validate(&[]).unwrap();
+    fn crc8_validate_valid() {
+        let data = [0xbeu8, 0xef, 0x92];
+        assert!(crc8::validate(&data).is_ok());
     }
 
     #[test]
-    #[should_panic]
-    fn crc8_validate_not_enough_data() {
-        crc8::validate(&[0xbe]).unwrap();
+    fn crc8_validate_invalid() {
+        let buffer: [u8; 3] = [0xbe, 0xef, 0x91];
+        assert_eq!(crc8::validate(&buffer), Err(crc8::CrcError::CrcError));
     }
 
     #[test]
@@ -75,7 +101,7 @@ mod tests {
         // Invalid CRC
         assert_eq!(
             crc8::validate(&[0xbe, 0xef, 0x91]),
-            Err(crc8::Error::CrcError)
+            Err(crc8::CrcError::CrcError)
         );
     }
 }

src/i2c.rs

@@ -1,22 +1,22 @@
-//! Helper functions for the u16 word based I²C communication.
+//! Helper functions for the u16-word based I²C communication.
 
 use crate::crc8;
+
 use embedded_hal::blocking::i2c;
 
 /// All possible errors in this crate
-#[derive(Debug, PartialEq, Copy, Clone)]
+#[derive(Debug, PartialEq)]
 pub enum Error<I2cWrite: i2c::Write, I2cRead: i2c::Read> {
+    /// I2C write error
     I2cWrite(I2cWrite::Error),
+    /// I2C read error
     I2cRead(I2cRead::Error),
-    Crc,
-}
-
-impl<W: i2c::Write, R: i2c::Read> From<crc8::Error> for Error<W, R> {
-    fn from(err: crc8::Error) -> Error<W, R> {
-        match err {
-            crc8::Error::CrcError => Error::Crc,
-        }
-    }
+    /// Cyclic Redundancy Checksum error
+    CrcError,
+    /// Invalid buffer size (not a multiple of 3)
+    InvalidBufferSize,
+    /// Buffer too small
+    BufferTooSmall,
 }
 
 /// Write an u16 command to the I²C bus.
@@ -29,24 +29,14 @@ pub fn write_command<I2cWrite: i2c::Write>(
 }
 
 /// Read data into the provided buffer and validate the CRC8 checksum.
-///
 /// If the checksum is wrong, return `Error::Crc`.
-///
-/// # Panics
-///
-/// This method will consider every third byte a checksum byte. If the buffer size is not a
-/// multiple of 3, then it will panic.
 pub fn read_words_with_crc<I2c: i2c::Read + i2c::Write>(
     i2c: &mut I2c,
     addr: u8,
     data: &mut [u8],
 ) -> Result<(), Error<I2c, I2c>> {
-    assert!(
-        data.len() % 3 == 0,
-        "Buffer must hold a multiple of 3 bytes"
-    );
     i2c.read(addr, data).map_err(Error::I2cRead)?;
-    crc8::validate(data)?;
+    crc8::validate(&data)?;
     Ok(())
 }
 
@@ -62,20 +52,20 @@ mod tests {
         let mut buf = [0; 3];
 
         // Valid CRC
-        let expectations = [Transaction::read(0x58, vec![0xBE, 0xEF, 0x92])];
+        let expectations = [Transaction::read(0x58, vec![0xbe, 0xef, 0x92])];
         let mut mock = I2cMock::new(&expectations);
         i2c::read_words_with_crc(&mut mock, 0x58, &mut buf).unwrap();
         assert_eq!(buf, [0xbe, 0xef, 0x92]);
 
         // Invalid CRC
-        let expectations = [Transaction::read(0x58, vec![0xBE, 0xEF, 0x00])];
+        let expectations = [Transaction::read(0x58, vec![0xbe, 0xef, 0x00])];
         let mut mock = I2cMock::new(&expectations);
         match i2c::read_words_with_crc(&mut mock, 0x58, &mut buf) {
-            Err(i2c::Error::Crc) => {}
+            Err(i2c::Error::CrcError) => {}
             Err(_) => panic!("Invalid error: Must be Crc"),
             Ok(_) => panic!("CRC check did not fail"),
         }
-        assert_eq!(buf, [0xbe, 0xef, 0x00]); // Buf was changed
+        assert_eq!(buf, [0xbe, 0xef, 0x00]); // Buf is unchanged
     }
 
     #[test]