Clean up the I2C impls.

[jonathanpallant]

Also we have inherent methods, so you can ignore the Embedded HAL traits if you want (makes the examples simpler too).

Whilst doing this I found out we only support 7-bit I2C addresses. I'll come back and fix that later.

[jannic]

There are a lot of i2c related changes in #747. That's why I didn't touch i2c in #753. Please coordinate with @ithinuel.

[ithinuel]

True, I’m mostly done with the tests of the blocking APIs.
I’m currently testing & fixing the async implementation. I’d like to reduce the "colored" part of the implementation before turning the PR back from Draft to Open state.

[jonathanpallant]

I'd like to not block an embedded-hal 1.0 compatible release on async changes, but if that's going to be quick, then sure. Or I can drop this and go and extract the non-async bits from 753.

[jannic]

@jonathanpallant: Is there anything left in this PR that's not covered by the already merged #747?

[jonathanpallant]

Wearing my @thejpster hat, I'll try and look this weekend.

[thejpster]

OK, so I spent some time cleaning this up ... and I'm not sure I like it.

Well, I do like the idea of the types having inherent methods so that functionality just works, without having to import a trait to make it work. Also it's easier to read about the functionality in the docs because it's not hidden behind a trait.

But, if you have an inherent method which has the same name as an async trait method, you get the inherent one in preference, which does not have the same return type.

[jannic]

Well, I do like the idea of the types having inherent methods so that functionality just works, without having to import a trait to make it work. Also it's easier to read about the functionality in the docs because it's not hidden behind a trait.

But, if you have an inherent method which has the same name as an async trait method, you get the inherent one in preference, which does not have the same return type.

Interesting problem. The root cause is that the type can be used in two different contexts, and it depends on the context whether the caller wants the async or the blocking methods. This can't be disambiguated on the type itself.

What are the options?

• disambiguation at each call site (as you did)
• use separate types for sync and async (one can be a wrapper type of the other)
• forget about the inherent methods, always use the traits
• use different names for the sync and the async methods (but if both sync and async traits already use the same method name, we can't change that in rp2040-hal)
• ...?

[thejpster]

Sounds about right. Can't think of anything else.

[ithinuel]

Always using the trait doesn't really solve the problem either if you have both sync and async traits in scope. You still need to disambiguate.

For inherent methods, I'd probably affix (pre- or su-) the method colour to its name.
It's not pretty but does the job and makes it obvious what colour method they are.

I don't like call-site disambiguation as, to some extent, it defeats the point of traits and type inference.

I really don't like how it breaks the UX of async code :(

[jannic]

Always using the trait doesn't really solve the problem either if you have both sync and async traits in scope. You still need to disambiguate.

Is this really an issue? Why would you import both the sync and the async traits?

If it's a rare situation, then manual disambiguation is fine IMHO. If most users of the async traits would need to import the sync ones as well, I'd say that would be a limitation of the async traits and they should be improved so it's no longer needed.

For inherent methods, I'd probably affix (pre- or su-) the method colour to its name.
It's not pretty but does the job and makes it obvious what colour method they are.

That's an option, but I wonder if it provides better usability compared to just using the traits. I guess it provides better discoverability?

[ithinuel]

Is this really an issue? Why would you import both the sync and the async traits?

If it's a rare situation, then manual disambiguation is fine IMHO. If most users of the async traits would need to import the sync ones as well, I'd say that would be a limitation of the async traits and they should be improved so it's no longer needed.

Indeed, The only case where I’d expect to see both traits in scope is in the HiL tests and even there I managed to work around that problem.

That's an option, but I wonder if it provides better usability compared to just using the traits. I guess it provides better discoverability?

Yeah, my affix suggestion is for if we really want to have inherent methods.
I don’t mind too much having a dependency on the hal to import the trait. It seems to me like a reasonable ask. The hal traits are pretty lean and don’t cost too much compilation time.

[thejpster]

I was more worried about disoverability when reading the docs.

I could go for inherent blocking_read and async_read and then if you pull in a trait you can just call read. This also helps sort the function names together in the docs.