Implement transactional I2C interface and add example

[eldruin]

I implemented the transactional I2C interface from rust-embedded/embedded-hal#223 and added an example with a driver which does a combined Write/Read transaction.
This is based on previous work from #33 by @ryankurte, rust-embedded/rust-i2cdev#51 and is similar to #35.

[rust-highfive]

r? @nastevens

(rust_highfive has picked a reviewer for you, use r? to override)

[nastevens]

Sorry, I haven't really been involved in these changes. Adding @ryankurte and @posborne instead as I believe they have.

[eldruin]

@ryankurte Do you know why embedded_hal::blocking::i2c::transactional::Default ends up being imported here? That seems to be the cause for the conflicting implementation errors:

error[E0119]: conflicting implementations of trait `hal::prelude::_embedded_hal_blocking_i2c_Write` for type `I2cdev`:
   --> src/lib.rs:187:1
    |
187 | impl hal::blocking::i2c::Write for I2cdev {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `embedded_hal`:
            - impl<E, S> hal::prelude::_embedded_hal_blocking_i2c_Write for S
              where <S as hal::prelude::_embedded_hal_blocking_i2c_Transactional>::Error == E, S: hal::blocking::i2c::transactional::Default<E>, S: hal::prelude::_embedded_hal_blocking_i2c_Transactional;
    = note: downstream crates may implement trait `hal::blocking::i2c::transactional::Default<_>` for type `I2cdev`

[ryankurte]

ahh, yeah whoops i should have caught that over on e-h. i believe it's because the trait bounds are different for Write (over address and errror) and Default (only over error), which gives the compiler nightmares.

you should be able to fix this in e-h by also binding the AddressMode across the default impl (as is done in spi::transactional::Default for exactly the same reason), so it becomes pub trait Default<AddressMode, Error> {} and is used in all the impls and where clauses.

lmk how that goes / if you get stuck lmk and i can provide more guidance.

[eldruin]

Ah, thank you! I added address mode support to the transactional traits over at rust-embedded/embedded-hal#260.
However, the conflicting implementation error is still there. It disappears if I remove the explicit implementations of i2c::Read, i2c::Write and i2c::WriteRead, of course, but the point of the default implementation inside the i2c::transactional module was to make it optional for HALs to use it or not.
I still do not understand how the default implementation is being pulled in for I2cdev.

error[E0119]: conflicting implementations of trait `hal::prelude::_embedded_hal_blocking_i2c_Write` for type `I2cdev`:
   --> src/lib.rs:187:1
    |
187 | impl hal::blocking::i2c::Write for I2cdev {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `embedded_hal`:
            - impl<A, E, S> hal::prelude::_embedded_hal_blocking_i2c_Write<A> for S
              where <S as hal::prelude::_embedded_hal_blocking_i2c_Transactional<A>>::Error == E, A: hal::blocking::i2c::AddressMode, S: hal::blocking::i2c::transactional::Default<A, E>, S: hal::prelude::_embedded_hal_blocking_i2c_Transactional<A>;
    = note: downstream crates may implement trait `hal::blocking::i2c::transactional::Default<u8, _>` for type `I2cdev`

[ryankurte]

ohh right, went back and looked and you've also gotta drop the error bound from the default type to that the transactional::Default trait, i believe this is required so there is no possibility of it existing over a foreign type.

sorry i missed that from the last advice, the following patch to e-h works for me.

diff --git a/src/blocking/i2c.rs b/src/blocking/i2c.rs
index 087d208..27b2092 100644
--- a/src/blocking/i2c.rs
+++ b/src/blocking/i2c.rs
@@ -334,12 +334,12 @@ pub mod transactional {
 
     /// Default implementation of `blocking::i2c::Write`, `blocking::i2c::Read` and
     /// `blocking::i2c::WriteRead` traits for `blocking::i2c::Transactional` implementers.
-    pub trait Default<A: AddressMode, E>: Transactional<A, Error = E> {}
+    pub trait Default<A: AddressMode>: Transactional<A> {}
 
-    impl<A, E, S> Write<A> for S
+    impl<A: 'static , E, S> Write<A> for S
     where
         A: AddressMode,
-        S: self::Default<A, E> + Transactional<A, Error = E>,
+        S: self::Default<A> + Transactional<A, Error = E>,
     {
         type Error = E;
 
@@ -351,7 +351,7 @@ pub mod transactional {
     impl<A, E, S> Read<A> for S
     where
         A: AddressMode,
-        S: self::Default<A, E> + Transactional<A, Error = E>,
+        S: self::Default<A> + Transactional<A, Error = E>,
     {
         type Error = E;
 
@@ -363,7 +363,7 @@ pub mod transactional {
     impl<A, E, S> WriteRead<A> for S
     where
         A: AddressMode,
-        S: self::Default<A, E> + Transactional<A, Error = E>,
+        S: self::Default<A> + Transactional<A, Error = E>,
     {
         type Error = E;

[eldruin]

@ryankurte Interesting. That works, thank you!
I still do not understand why it is a problem if nobody imported transactional::Default, though.

i believe this is required so there is no possibility of it existing over a foreign type.

Could you elaborate or refer me to some docs about this?

[ryankurte]

Could you elaborate or refer me to some docs about this?

my thinking was that Address is sealed but Error is not, so you could plausibly create a new Error type somewhere else then try to impl Default over this, but, in digging into this more i have only confused myself further 😢 might have to chalk it up to experience painfully earned.

are we happy with / ready to land this then?

[eldruin]

Yes, I am happy with the code itself as-is. However, to merge this we would need to make a new alpha release of embedded-hal and remove the patch to my branch.

[eldruin]

I have updated this to use embedded-hal 1.0.0-alpha.4 and is now ready to merge.

[ryankurte]

lgtm!

bors r+

[bors]

Timed out.

[eldruin]

Bors timed out again.

[ryankurte]

yeeeah, we really need to do #51 like all the other e-h repos

[eldruin]

Hmm, but in this case Travis is not the problem. It is bors the one that timed out. Or am I missing something else?

[ryankurte]

i might be misunderstanding but i thought the bors timeouts were usually because travis doesn't schedule / execute the job quickly enough (because the ewg burns through hours)

bors retry

[bors]

Build succeeded:

• continuous-integration/travis-ci/push

[eldruin]

Ah, I see. That sounds possible. Thanks!