Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added async I2C implementation #119

Merged
merged 4 commits into from
May 26, 2024
Merged

Conversation

asasine
Copy link
Contributor

@asasine asasine commented May 22, 2024

I saw that async I2C wasn't implemented and I'd like to use it for a sensor driver I'm writing. I implemented this following how embedded_hal_mock::spi::Spi was implemented for async.

Closes #118

@asasine
Copy link
Contributor Author

asasine commented May 22, 2024

I don't love how the tests are implemented because there's a lot of repetition. I was trying to come up with a way to share the body between the sync and async tests, but I'm a bit too new to Rust to find a good way to do that. I was thinking something along these lines:

#[cfg(test)]
mod test {
    use super::*;

    async fn write<I /* : something? */>()
    {
        let expectations = [Transaction::write(0xaa, vec![10, 12])];
        let mut i2c = Mock::new(&expectations);
        I::write(&mut i2c, 0xaa, &vec![10, 12]).await.unwrap();
        I::done(&mut i2c,);
    }

    #[test]
    fn test_write() {
        use embedded_hal::i2c::I2c;
        write::<I2c>().await;
    }

    #[tokio::test]
    #[cfg(feature = "embedded-hal-async")]
    async fn test_write_async() {
        use embedded_hal_async::i2c::I2c;
        write::<I2c>().await;
    }

  // --snip--
}

@felipebalbi
Copy link

I just tested this PR on a crate of my own and can confirm it works as expected. Thanks for doing the work @asasine.

@asasine
Copy link
Contributor Author

asasine commented May 26, 2024

@dbrgn is there a chance you can review and release this? I believe there are a couple of unreleased fixes too that would be great to have available on crates.io.

Copy link
Owner

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this looks quite straightforward!

Regarding the tests: If the async impls are doing nothing else than calling the sync impls, the additional tests don't add that much value and are mostly repeated code doing the same thing under the hood. Maybe one single big test that calls every async method (read, write, write_read, transaction) sequentially (plus very simple expectations for that) would suffice?

README.md Show resolved Hide resolved
src/eh1/i2c.rs Outdated Show resolved Hide resolved
@asasine
Copy link
Contributor Author

asasine commented May 26, 2024

I left the tests as-is for now since the same question exists for all other async implementations that are async-over-sync (delay and SPI) and it would best (in my opinion) to address all of them in one pass.

@asasine asasine requested a review from dbrgn May 26, 2024 20:25
Copy link
Owner

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍🏻

@dbrgn
Copy link
Owner

dbrgn commented May 26, 2024

I left the tests as-is for now since the same question exists for all other async implementations that are async-over-sync (delay and SPI) and it would best (in my opinion) to address all of them in one pass.

Fair point.

@dbrgn dbrgn merged commit 2219ed2 into dbrgn:main May 26, 2024
3 checks passed
@asasine
Copy link
Contributor Author

asasine commented May 26, 2024

No problem, thanks for the quick turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants