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

Revise I2C Timing Calculation #230

Closed
bjoernQ opened this issue Oct 26, 2022 · 0 comments · Fixed by #263
Closed

Revise I2C Timing Calculation #230

bjoernQ opened this issue Oct 26, 2022 · 0 comments · Fixed by #263
Assignees
Labels
bug Something isn't working

Comments

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 26, 2022

The code in

fn set_frequency(
&mut self,
source_clk: HertzU32,
bus_freq: HertzU32,
) -> Result<(), SetupError> {
cfg_if::cfg_if! {
if #[cfg(any(esp32c2, esp32c3, esp32s3))] {
// C2, C3, and S3 have a clock divider mechanism, which we want to configure
// as high as possible.
let sclk_div = source_clk.raw() / (bus_freq.raw() * 1024) + 1;
let half_cycle = source_clk.raw() / sclk_div as u32 / bus_freq.raw() / 2;
} else {
// For EPS32 and the S2 variant no clock divider mechanism exists.
let half_cycle = source_clk.raw() / bus_freq.raw() / 2;
}
}
// The different chips have highly very different timing configurations, so
// we're setting these up separately (this might introduce some overhead,
// but improves readability)
cfg_if::cfg_if! {
if #[cfg(any(esp32c2, esp32c3))] {
let scl_wait_high = if bus_freq.raw() <= 50000 { 0 } else { half_cycle / 8 };
let scl_high = half_cycle - scl_wait_high;
let sda_hold = half_cycle / 4;
let sda_sample = scl_high / 2;
} else if #[cfg(esp32s3)] {
let scl_high = if bus_freq.raw() <= 50000 { half_cycle } else { half_cycle / 5 * 4 + 4 };
let scl_wait_high = half_cycle - scl_high;
let sda_hold = half_cycle / 2;
let sda_sample = half_cycle / 2;
} else if #[cfg(esp32s2)] {
let scl_high = half_cycle / 2 + 2;
let scl_wait_high = half_cycle - scl_high;
let sda_hold = half_cycle / 2;
let sda_sample = half_cycle / 2 - 1;
} else {
// ESP32 is default (as it is the simplest case and does not even have
// the wait_high field)
let scl_high = half_cycle;
let sda_hold = half_cycle / 2;
let sda_sample = scl_high / 2;
let tout = half_cycle * 20;
}
}
let scl_low = half_cycle;
let setup = half_cycle;
let hold = half_cycle;
#[cfg(not(esp32))]
let scl_low = scl_low as u16 - 1;
#[cfg(esp32)]
let scl_low = scl_low as u16;
let scl_high = scl_high as u16;
#[cfg(any(esp32c2, esp32c3, esp32s3))]
let scl_wait_high = scl_wait_high as u8;
#[cfg(any(esp32s2))]
let scl_wait_high = scl_wait_high as u16;
let sda_hold = sda_hold
.try_into()
.map_err(|_| SetupError::InvalidClkConfig)?;
let setup = setup.try_into().map_err(|_| SetupError::InvalidClkConfig)?;
let sda_sample = sda_sample
.try_into()
.map_err(|_| SetupError::InvalidClkConfig)?;
let hold = hold.try_into().map_err(|_| SetupError::InvalidClkConfig)?;
#[cfg(any(esp32c2, esp32c3, esp32s3))]
let sclk_div = sclk_div as u8;
unsafe {
// divider
#[cfg(any(esp32c2, esp32c3, esp32s3))]
self.register_block()
.clk_conf
.modify(|_, w| w.sclk_sel().clear_bit().sclk_div_num().bits(sclk_div - 1));
// scl period
self.register_block()
.scl_low_period
.write(|w| w.scl_low_period().bits(scl_low));
// for high/wait_high we have to differentiate between the chips
// as the EPS32 does not have a wait_high field
cfg_if::cfg_if! {
if #[cfg(not(esp32))] {
self.register_block().scl_high_period.write(|w| {
w.scl_high_period()
.bits(scl_high)
.scl_wait_high_period()
.bits(scl_wait_high)
});
}
else {
self.register_block().scl_high_period.write(|w| {
w.scl_high_period()
.bits(scl_high)
});
}
}
// we already did that above but on S2 we need this to make it work
#[cfg(esp32s2)]
self.register_block().scl_high_period.write(|w| {
w.scl_wait_high_period()
.bits(scl_wait_high)
.scl_high_period()
.bits(scl_high)
});
// sda sample
self.register_block()
.sda_hold
.write(|w| w.time().bits(sda_hold));
self.register_block()
.sda_sample
.write(|w| w.time().bits(sda_sample));
// setup
self.register_block()
.scl_rstart_setup
.write(|w| w.time().bits(setup));
self.register_block()
.scl_stop_setup
.write(|w| w.time().bits(setup));
// hold
self.register_block()
.scl_start_hold
.write(|w| w.time().bits(hold));
self.register_block()
.scl_stop_hold
.write(|w| w.time().bits(hold));
// The ESP32 variant does not have an enable flag for the
// timeout mechanism
cfg_if::cfg_if! {
if #[cfg(esp32)] {
// timeout
self.register_block()
.to
.write(|w| w.time_out().bits(tout.into()));
}
else {
// timeout
// FIXME: Enable timout for other chips!
self.register_block()
.to
.write(|w| w.time_out_en().clear_bit());
}
}
}
Ok(())
}
is hard to read and hard to maintain - i.e. we should be able to easily compare that with e.g. ESP-IDF (it differs from that apparently already)

Since those calculations are very different for each chip it would make more sense to have implementations per chip (some chips share the same - e.g. ESP32-C3 and ESP32-C2)

Apparently also some subtle bugs here cause problems like we have seen with MPU6050 and ESP32-C3

@bjoernQ bjoernQ added bug Something isn't working enhancement labels Oct 26, 2022
@bjoernQ bjoernQ self-assigned this Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant