search

xmos / lib_i2c

I²C library
Other
3 stars 25 forks source link

make sure the expected delay to happen #83

Closed QuinnWang closed 1 week ago

QuinnWang commented 3 years ago

make sure the delay in the release_clock_and_wait in i2c_master.xc to happen, and same idea for the delay of the wait_for_clock_high in the i2c_master_single_port.xc.

QuinnWang commented 3 years ago

Sorry that deleted the branch incorrectly, reopened

xross commented 2 weeks ago

I believe this is already fixed in #79

QuinnWang commented 2 weeks ago

I believe this is already fixed in #79

Seems no, this problem is based on the #79 repeat start fix, and please check the description as below in detail:

If the clock stretching happening, for example, the scl rising edge delays for 1 clock period, the original "tmr when timerafter(fall_time + delay) :> time" will not happen actually. If the example above does happen, the timer value "time" in the original code is actually the scl rising edge time, then suggest to update the reference time point - fall_time - to a new value, set it to one low_period_ticks ago to be as an expected last scl fall edge time, and do the delay again to make sure the delay will indeed happen.

To verify, just take the last restart fix as an example, if the scl rising edge delays for 1 clock period, the delay code in the release_clock_and_wait doesn't do the delay actually, then the following drive SDA low will run right after the scl released by I2C slave to high, we can see the restart problem again, because the previous restart bug is just the compute_bus_off_ticks delay in the release_clock_and_wait is almost equal to the compute_low_period_ticks before calling release_clock_and_wait, then no sufficient time for between scl goes high and sda goes low. With the changes, if go through/review with the clock stretching happening cases, since the delay should happen as expected, then no problem for the clock stretching cases.

And during understanding above, I found if changing the fall_time to last_scl_fall_edge_time, the readability of the code will become friendly and logic straight forward, then just changed as well. Then found if following the original design idea that the fall_time is last_scl_fall_edge_time, the restart fix should need to be changed slightly, so updated in the propose changes too.

shuchitak commented 1 week ago

I have gone through these changes and they make sense to me. Without these changes, both check_clock_high_time() and check_setup_start_time() checks fail for the clock stretching test case (test_master_clock_stretch.py) and they pass with these changes (Tested in https://github.com/xmos/lib_i2c/pull/105).

Note that I had to reduce the expected_speed in the test_master_clock_stretch.py since doing the required delay between the slave releasing SCL, and the master driving data on SDA, means that the detected I2C speed would be slower than before.