Ensure that i2c pins have PullUp activated #708
Conversation
jannic
added
the
breaking change
rp2040-hal/src/i2c.rs
Outdated
| $crate::paste::paste! { | ||
| /// Configures the I2C peripheral to work in master mode | ||
| /// This function can be called without the pull-ups on the I2C pins. | ||
| pub fn [<$i2cX _unchecked>]<F, SystemF>( |
There was a problem hiding this comment.
I'm not convinced by the _unchecked suffix, as it's otherwise often used to skip some runtime check. Any better ideas?
There was a problem hiding this comment.
How about a new_with_external_pull_up ?
There was a problem hiding this comment.
That wouldn't fit. The difference is just that this function doesn't check the pull type. It could still use internal pull-ups.
Perhaps new_unchecked_pull_mode?
IMHO for this method it doesn't matter if the name is long and ugly. I don't expect it to be used very often. It's just for completeness, in case someone wants to do something strange.
There was a problem hiding this comment.
I like new_with_external_pull_up a lot better. The meaning of "unchecked" in this context is not very clear. Having external pull-ups is quite common in general and might be desirable to tailor your design to your EMI and speed constraints.
There was a problem hiding this comment.
Ok, changed the function name. I still omitted the new part, because it would be inconsistent. Now the two functions available are:
pub fn i2c0<F, SystemF>(
i2c: I2C0,
sda_pin: Sda,
scl_pin: Scl,
freq: F,
resets: &mut RESETS,
system_clock: SystemF,
) -> Self
where
F: Into<HertzU32>,
Sda: ValidPinSda<I2C0> + AnyPin<Pull = PullUp>,
Scl: ValidPinScl<I2C0> + AnyPin<Pull = PullUp>,
SystemF: Into<HertzU32>,
pub fn i2c0_with_external_pull_up<F, SystemF>(
i2c: I2C0,
sda_pin: Sda,
scl_pin: Scl,
freq: F,
resets: &mut RESETS,
system_clock: SystemF,
) -> Self
where
F: Into<HertzU32>,
Sda: ValidPinSda<I2C0>,
Scl: ValidPinScl<I2C0>,
SystemF: Into<HertzU32>,
(And of course the same for i2c1)
|
The added check immediately uncovered an issue in a doc comment, where the I2C pins were not configured with pull-ups: https://github.com/rp-rs/rp-hal/actions/runs/6644179376/job/18052788902 |
It is possible to skip this check by using `hal::I2C::i2c0_unchecked(...)` or `hal::I2C::i2c1_unchecked(...)`, but the default constructor functions now only accept pins with activated PullUp. Fixes rp-rs#690.
jannic
force-pushed
the
checked-i2c-construction
branch
from
bee0191 to
bb8a9fd
Compare
It is possible to skip this check by using
hal::I2C::i2c0_unchecked(...)orhal::I2C::i2c1_unchecked(...), but the default constructor functions now only accept pins with activated PullUp.Fixes #690.