Add SMBus HAL, dependent on embedded-hal-async::I2c

[tullom]

Add SMBus support in the form of a helper trait built on top of an embedded-hal-async I2C implementation.

Based on the SMBus v3,3 spec.

[github-actions]

Cargo Vet Audit Passed

cargo vet has passed in this PR. No new unvetted dependencies were found.

[kurtjd]

I think Robert's comments are a good idea but otherwise looks good.

[jerrysxie]

@tullom This looks fine to me as long as some of Robert's comments are addressed. I would like to see a demonstration of this on one of the charger/fuel gage driver. Put it in action is good way to iron out the kinks.

Non-blocking: Another consideration would be to use embedded-hal i2c mock to unit test some of the default implementation. Please create an issue for this.

[RobertZ2011]

Nothing to add on top of the existing comments.

[kurtjd]

Looks good (though Copilot suggestions seem like they might be valid).

[Copilot]

Pull request overview

Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (7)

embedded-mcu-hal/src/smbus/bus/asynch.rs:22

• SMBusError and I2cError are imported but not referenced anywhere in this module (search only finds them in the use lines). Please remove these unused imports to avoid warnings and keep the module tidy.

use core::hash::Hasher;
use core::marker::PhantomData;

use crate::smbus::bus::Error as SMBusError;
use embedded_hal_async::i2c::{Error as I2cError, I2c, Operation};

embedded-mcu-hal/src/smbus/bus/asynch.rs:78

• check_pec claims to compare only the low byte of computed_pec, but the current implementation compares the full u64 against received_pec. This will reject valid PECs whenever the hasher's finish() contains non-zero high bytes; compare against (computed_pec as u8) (or computed_pec & 0xFF) instead.

    /// Check PEC (Packet Error Code) validity.
    ///
    /// Compares a received PEC byte against a computed PEC value. Only the
    /// low byte of `computed_pec` is used.
    fn check_pec(received_pec: u8, computed_pec: u64) -> Result<(), <Self as crate::smbus::bus::ErrorType>::Error> {
        computed_pec
            .eq(&received_pec.into())
            .then_some(())
            .ok_or_else(|| <Self as crate::smbus::bus::ErrorType>::Error::to_kind(crate::smbus::bus::ErrorKind::Pec))
    }

embedded-mcu-hal/src/smbus/bus/asynch.rs:354

• finalize_pec_byte currently uses try_into() from u64 to u8, which errors if the hasher’s finish() isn’t already <= 255. The module/docs describe PEC as the truncated low byte of finish(), so this should mask/cast to the low byte rather than failing.

    /// Truncate a finished PEC value to its low byte.
    fn finalize_pec_byte(pec: u64) -> Result<u8, crate::smbus::bus::ErrorKind> {
        pec.try_into().map_err(|_| crate::smbus::bus::ErrorKind::Pec)
    }

embedded-mcu-hal/src/smbus/bus/asynch.rs:404

• When use_pec is true, read_buf seeds the PEC with the write address byte (addr<<1). For pure-read SMBus operations (e.g., Receive Byte), the on-the-wire address byte has the read bit set, so PEC should be seeded with read_address_byte(address) (and, if applicable, include the write address only when there was an actual write phase). As written, this will compute a different PEC than targets expect.

    /// Read a buffer of data with optional PEC verification.
    ///
    /// When `use_pec` is true, the caller must size `read` to include one
    /// extra trailing byte for the PEC byte; it is verified after the read.
    async fn read_buf(
        &mut self,
        address: u8,
        use_pec: bool,
        read: &mut [u8],
    ) -> Result<(), crate::smbus::bus::ErrorKind> {
        if use_pec {
            let mut pec = Self::pec_calc_with_write_addr(address)?;
            self.i2c
                .read(address, read)
                .await
                .map_err(|e| crate::smbus::bus::ErrorKind::from(e.kind()))?;
            let (pec_byte, rest) = read.split_last().ok_or(crate::smbus::bus::ErrorKind::Pec)?;
            pec.write(rest);
            <Self as Smbus>::check_pec(*pec_byte, pec.finish())?;

embedded-mcu-hal/src/smbus/bus/asynch.rs:692

• block_read reads the reported msg_size byte but then unconditionally reads data.len() bytes and computes PEC over the entire data buffer. This ignores the SMBus length field and can over-read/under-read the bus (and validate PEC over the wrong byte sequence). Please validate msg_size[0] against the provided buffer and only read/hash the indicated number of bytes (or return the actual length).

        let mut msg_size = [0u8];
        if use_pec {
            let mut pec_buf = [0u8];
            let mut pec = Self::pec_calc_with_write_addr(address)?;
            pec.write_u8(register);
            pec.write_u8(crate::smbus::bus::read_address_byte(address));
            self.i2c
                .transaction(
                    address,
                    &mut [
                        Operation::Write(&[register]),
                        Operation::Read(&mut msg_size),
                        Operation::Read(data),
                        Operation::Read(&mut pec_buf),
                    ],
                )
                .await
                .map_err(|e| crate::smbus::bus::ErrorKind::from(e.kind()))?;
            pec.write(&msg_size);
            pec.write(data);
            Self::check_pec(pec_buf[0], pec.finish())?;
        } else {
            self.i2c
                .transaction(
                    address,
                    &mut [
                        Operation::Write(&[register]),
                        Operation::Read(&mut msg_size),
                        Operation::Read(data),
                    ],
                )
                .await
                .map_err(|e| crate::smbus::bus::ErrorKind::from(e.kind()))?;
        }
        Ok(())

embedded-mcu-hal/src/smbus/bus/asynch.rs:745

• block_write_block_read_process_call reads the target-provided read_msg_size but then always reads read_data.len() bytes and includes the full buffer in PEC. This can desynchronize the transaction if the device reports a different length, and it makes PEC validation incorrect. Please validate and only read/hash the reported number of bytes (or change the API to return the actual length).

        if write_data.len() + read_data.len() > crate::smbus::bus::MAX_BLOCK_SIZE {
            return Err(crate::smbus::bus::ErrorKind::TooLargeBlockTransaction);
        }
        let mut read_msg_size = [0u8];
        if use_pec {
            let mut pec_buf = [0u8];
            let mut pec = Self::pec_calc_with_write_addr(address)?;
            pec.write_u8(register);
            pec.write_u8(write_data.len() as u8);
            pec.write(write_data);
            pec.write_u8(crate::smbus::bus::read_address_byte(address));
            self.i2c
                .transaction(
                    address,
                    &mut [
                        Operation::Write(&[register]),
                        Operation::Write(&[write_data.len() as u8]),
                        Operation::Write(write_data),
                        Operation::Read(&mut read_msg_size),
                        Operation::Read(read_data),
                        Operation::Read(&mut pec_buf),
                    ],
                )
                .await
                .map_err(|e| crate::smbus::bus::ErrorKind::from(e.kind()))?;
            pec.write(&read_msg_size);
            pec.write(read_data);
            Self::check_pec(pec_buf[0], pec.finish())?;
        } else {
            self.i2c
                .transaction(
                    address,
                    &mut [
                        Operation::Write(&[register]),
                        Operation::Write(&[write_data.len() as u8]),
                        Operation::Write(write_data),
                        Operation::Read(&mut read_msg_size),
                        Operation::Read(read_data),
                    ],
                )
                .await
                .map_err(|e| crate::smbus::bus::ErrorKind::from(e.kind()))?;
        }

embedded-mcu-hal/src/smbus/bus/mod.rs:160

• This section is labeled “I2C error type trait”, but it defines the associated error type for SMBus traits (and even depends on crate::smbus::bus::ErrorKind). Please rename/reword the doc comments to SMBus here to match the module and avoid confusion.

/// I2C error type trait.
///
/// This just defines the error type, to be used by the other traits.
pub trait ErrorType {
    /// Error type
    type Error: Error + From<embedded_hal_async::i2c::ErrorKind>;
}

[Copilot]

Pull request overview

Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

embedded-mcu-hal/src/smbus/bus/asynch.rs:22

• SMBusError and I2cError are imported but not used anywhere in this module (only appear in the use lines). Please remove these unused imports to avoid warnings and keep the module tidy.

use core::hash::Hasher;
use core::marker::PhantomData;

use crate::smbus::bus::Error as SMBusError;
use embedded_hal_async::i2c::{Error as I2cError, I2c, Operation};

embedded-mcu-hal/src/smbus/bus/asynch.rs:78

• check_pec's doc comment says only the low byte of computed_pec is used, but the implementation currently compares the full u64 against received_pec (after widening). Either update the docs to describe the strict comparison semantics, or change the implementation to explicitly truncate/mask to the low byte so the behavior matches the docs.

    /// Check PEC (Packet Error Code) validity.
    ///
    /// Compares a received PEC byte against a computed PEC value. Only the
    /// low byte of `computed_pec` is used.
    fn check_pec(received_pec: u8, computed_pec: u64) -> Result<(), <Self as crate::smbus::bus::ErrorType>::Error> {
        computed_pec
            .eq(&received_pec.into())
            .then_some(())
            .ok_or_else(|| <Self as crate::smbus::bus::ErrorType>::Error::from_kind(crate::smbus::bus::ErrorKind::Pec))
    }

embedded-mcu-hal/src/smbus/bus/mod.rs:97

• unreachable!() is used here to implement from_kind for Infallible. With workspace clippy lints set to deny(panic) and deny(unreachable), this is likely to fail CI (since unreachable!() is a form of panic). Consider using a non-panicking diverging implementation (e.g., an infinite loop with core::hint::unreachable_unchecked() only if acceptable) or explicitly allow the clippy::panic lint in this exact scope.

    fn from_kind(_kind: ErrorKind) -> Self {
        // `Infallible` is uninhabited, so this function can never actually
        // be called
        #[allow(clippy::unreachable)]
        {
            unreachable!()
        }

[jerrysxie]

Sorry, it has been a while since I last reviewed this. So this is basically a new review.

[jerrysxie]

Doh, cargo-vet for this repo is currently broken. But that should not block this PR, issue: #28