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.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (4)

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

• finalize_pec_byte uses try_into() which fails if the hasher's finish() value doesn't fit into u8. Earlier docs state that the PEC is the truncated low byte of finish(), so this should explicitly truncate/mask instead of erroring on larger u64 values.

    /// 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:629

• block_write is implemented as a transaction with multiple Operation::Write steps (register, length, payload, PEC). This will generally insert repeated STARTs/address bytes between chunks, which is not equivalent to an SMBus block write frame and will also break PEC calculation. Build a single contiguous write buffer for the whole write phase (register + len + data [+ pec]) and send it as one write op.

            self.i2c
                .transaction(
                    address,
                    &mut [
                        Operation::Write(&[register]),
                        Operation::Write(&[data.len() as u8]),
                        Operation::Write(data),
                        Operation::Write(&[pec]),
                    ],
                )

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

• block_write_block_read_process_call is executed as many separate write/read operations inside a single transaction. This will generally introduce extra repeated STARTs/address bytes between operations, changing the SMBus wire sequence and PEC inputs (similar to process_call and block_read). Restructure to a single contiguous write op for the write phase and a single contiguous read op for the read phase (then parse length/data/pec).

            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),
                    ],
                )

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

• The ErrorType trait is documented as "I2C error type trait" even though it is defining the error type for the SMBus traits in this module. Updating the wording will avoid confusion for HAL/driver authors implementing this API.

/// 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>;
}

[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]

computed_pec is u64 but received_pec is u8. We are converting received_pec into u64 to compare it with computed_pec. Is the hasher guaranteed to return zeros in for bit 63 to bit 8?

[felipebalbi]

PEC is CRC8, no? Why u64 at all?

[tullom]

Its because I define that the CRC calculator must implement core::hash::Hasher, which outputs its final hash in the form of u64. Most hashing crates implement this trait. The hasher should be guaranteed to return zeros for bit 63 to bit 8, and i check for this in the implementation by promoting the u8 to a u64.

[jerrysxie]

This import seems to be not needed now.

[tullom]

It actually is needed for this line:
https://github.com/OpenDevicePartnership/embedded-mcu/pull/7/changes#diff-e28912d00e735f1ec57e7c90fc926b9887eefc013a240d8383eb414416054866R77

[jerrysxie]

For SMBus varliable data length responses, is the assumption here that the buffer passed in is always big enough for the largest data payload? The client has to fulfill this guanrantee?

[tullom]

For block reads, the size of the response is actually encoded in the response itself as byte 1 (zero indexed). The spec also defines an upper size limit of 64 bytes. So yes, the client needs to fulfill this guarantee. I2C::ErrorKind, which is encoded in an SMBus error, has an Overrun error kind that the client can use to indicate this error.

[jerrysxie]

We should consider cases where a reference to an array of size 0 is passed in all of our API where it takes a reference to an array.

[tullom]

Specifically for this method, write_buf(), it is not a public api method, but rather a convenience method, and every use case of it makes it impossible to pass in an array of size zero. As for the other methods that take in a reference to an array, the logic is correct according to the spec. A size 0 block write is valid in SMBus v3 and above

[felipebalbi]

Ugh, my bad, I should have suggested before that the _with_pec() variants can easily get a default implementation built on top of the non-pec variants.

[tullom]

Ah, should i revert to the way it was previously? im already using a convenience method to make the code repeatable and i dont think its needed to revert.