I2c addr #2864
I2c addr #2864
Conversation
| @@ -521,33 +619,38 @@ where | |||
| } | |||
|
|
|||
| /// Writes bytes to slave with address `address` | |||
| pub fn write(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { | |||
| pub fn write<A: AddressMode>(&mut self, address: A, buffer: &[u8]) -> Result<(), Error> { | |||
There was a problem hiding this comment.
So with this way of doing things, when we add 10bit support, won't this break code like this:
i2c.write(0xaa /* <- ambiguous type here, could be a u8 or a u16 */, &[])?;There was a problem hiding this comment.
We would be able to implement for i321 and decide what to do with that. But it would probably be unclear what i32 would represent. Instead, I would recommend implementing AddressMode for u8 and TenBit(u16) instead, not for other numeric types.
Footnotes
There was a problem hiding this comment.
oh yeah - we can't have defaults for type parameters on functions :( which would have been an easy solution
I would recommend implementing AddressMode for u8 and TenBit(u16) instead, not for other numeric types
I like this solution a lot - 10-bit is uncommon enough to have a new-type for it
| pub trait AddressMode: _private::AddressModeInternal + Copy + 'static {} | ||
|
|
||
| mod _private { | ||
| pub trait AddressModeInternal { |
There was a problem hiding this comment.
I don't understand the need for this. I2cAddress can impl From from the possible address types.
There was a problem hiding this comment.
idea is that implementing From extends the public API surface and we would need to expose the internal enum
There was a problem hiding this comment.
Why is that a problem, since that enum is already marked with a bunch of derives that only make sense if it's public?
There was a problem hiding this comment.
we can ofc remove the unnecessary derives - we just shouldn't make anything public which doesn't need to
esp-hal/src/i2c/master/mod.rs
Outdated
| } | ||
|
|
||
| impl I2cAddress { | ||
| pub fn as_u8(&self) -> u8 { |
There was a problem hiding this comment.
Why is this public, and what will happen when we introduce 10-bit addresses?
There was a problem hiding this comment.
can be pub(super) but since it's in a private module doesn't make much difference
10-bit address support will introduce some more changes - we'll probably have to match on the enum and do a few things differently (and will remove this function)
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
Fixes #2786
Testing
CI