From 569fce68223089f8d6e5f16669a21826c7d68ded Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 9 Jul 2025 17:11:30 +0000 Subject: [PATCH 1/6] Initial plan From ebf5ce4e3e31d5b547c80aa87c285c59d151cab5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 9 Jul 2025 17:22:14 +0000 Subject: [PATCH 2/6] Implement pull-up detection and timeout mechanisms for single port I2C master Co-authored-by: xross <642910+xross@users.noreply.github.com> --- lib_i2c/api/i2c.h | 30 +++++- lib_i2c/src/i2c_master_single_port.xc | 144 ++++++++++++++++++++++++-- 2 files changed, 162 insertions(+), 12 deletions(-) diff --git a/lib_i2c/api/i2c.h b/lib_i2c/api/i2c.h index 0bf6ff3..fab26ad 100644 --- a/lib_i2c/api/i2c.h +++ b/lib_i2c/api/i2c.h @@ -26,6 +26,8 @@ typedef enum { I2C_NACK, ///< the slave has NACKed the last byte I2C_ACK, ///< the slave has ACKed the last byte + I2C_SCL_PULLUP_MISSING, ///< SCL pull-up resistor not detected + I2C_SDA_PULLUP_MISSING, ///< SDA pull-up resistor not detected } i2c_res_t; #if(defined __XC__ || defined __DOXYGEN__) @@ -114,7 +116,9 @@ typedef interface i2c_master_if { typedef enum { I2C_REGOP_SUCCESS, ///< the operation was successful I2C_REGOP_DEVICE_NACK, ///< the operation was NACKed when sending the device address, so either the device is missing or busy - I2C_REGOP_INCOMPLETE ///< the operation was NACKed halfway through by the slave + I2C_REGOP_INCOMPLETE, ///< the operation was NACKed halfway through by the slave + I2C_REGOP_SCL_PULLUP_MISSING, ///< SCL pull-up resistor not detected + I2C_REGOP_SDA_PULLUP_MISSING, ///< SDA pull-up resistor not detected } i2c_regop_res_t; #ifndef __DOXYGEN__ @@ -152,12 +156,28 @@ extends client interface i2c_master_if : { size_t n; i2c_res_t res; res = i.write(device_addr, a_reg, 1, n, 0); + if (res == I2C_SCL_PULLUP_MISSING) { + result = I2C_REGOP_SCL_PULLUP_MISSING; + return 0; + } + if (res == I2C_SDA_PULLUP_MISSING) { + result = I2C_REGOP_SDA_PULLUP_MISSING; + return 0; + } if (n != 1) { result = I2C_REGOP_DEVICE_NACK; i.send_stop_bit(); return 0; } res = i.read(device_addr, data, 1, 1); + if (res == I2C_SCL_PULLUP_MISSING) { + result = I2C_REGOP_SCL_PULLUP_MISSING; + return 0; + } + if (res == I2C_SDA_PULLUP_MISSING) { + result = I2C_REGOP_SDA_PULLUP_MISSING; + return 0; + } if (res == I2C_ACK) { result = I2C_REGOP_SUCCESS; } else { @@ -183,7 +203,13 @@ extends client interface i2c_master_if : { { uint8_t a_data[2] = {reg, data}; size_t n; - i.write(device_addr, a_data, 2, n, 1); + i2c_res_t res = i.write(device_addr, a_data, 2, n, 1); + if (res == I2C_SCL_PULLUP_MISSING) { + return I2C_REGOP_SCL_PULLUP_MISSING; + } + if (res == I2C_SDA_PULLUP_MISSING) { + return I2C_REGOP_SDA_PULLUP_MISSING; + } if (n == 0) { return I2C_REGOP_DEVICE_NACK; } diff --git a/lib_i2c/src/i2c_master_single_port.xc b/lib_i2c/src/i2c_master_single_port.xc index 3a1aefa..3720f05 100644 --- a/lib_i2c/src/i2c_master_single_port.xc +++ b/lib_i2c/src/i2c_master_single_port.xc @@ -49,13 +49,75 @@ static unsigned inline compute_bus_off_ticks(static const unsigned kbits_per_sec return bit_time/2 + bit_time/16; } +/** Check if pull-up resistors are present on SCL and SDA lines. + * This function drives the lines low briefly, then releases them + * and checks if they return to high state within a reasonable time. + * + * \returns I2C_ACK if both pull-ups are present, + * I2C_SCL_PULLUP_MISSING if SCL pull-up is missing, + * I2C_SDA_PULLUP_MISSING if SDA pull-up is missing + */ +static i2c_res_t check_pullups( + port p_i2c, + static const unsigned scl_bit_position, + static const unsigned sda_bit_position, + static const unsigned other_bits_mask) +{ + const unsigned SCL_HIGH = BIT_MASK(scl_bit_position); + const unsigned SDA_HIGH = BIT_MASK(sda_bit_position); + const unsigned SCL_LOW = 0; + const unsigned SDA_LOW = 0; + + timer tmr; + unsigned start_time, timeout_time; + const unsigned PULLUP_TIMEOUT_TICKS = 1000; // 10us timeout for pull-up detection + + // Test SCL pull-up + p_i2c <: SCL_LOW | SDA_HIGH | other_bits_mask; // Drive SCL low, SDA high + delay_ticks(100); // Brief delay to ensure line is driven low + p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; // Release SCL + + tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + unsigned val = peek(p_i2c); + while (!(val & SCL_HIGH)) { + tmr :> start_time; + if (start_time > timeout_time) { + return I2C_SCL_PULLUP_MISSING; + } + val = peek(p_i2c); + } + + // Test SDA pull-up + p_i2c <: SCL_HIGH | SDA_LOW | other_bits_mask; // Drive SDA low, SCL high + delay_ticks(100); // Brief delay to ensure line is driven low + p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; // Release SDA + + tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + val = peek(p_i2c); + while (!(val & SDA_HIGH)) { + tmr :> start_time; + if (start_time > timeout_time) { + return I2C_SDA_PULLUP_MISSING; + } + val = peek(p_i2c); + } + + return I2C_ACK; +} + /** Reads back the SCL line, waiting until it goes high (in * case the slave is clock stretching). It is assumed that the clock * line has been release (driven high) before calling this function. * Since the line going high may be delayed, the fall_time * value may need to be adjusted + * + * \returns I2C_ACK on success, I2C_SCL_PULLUP_MISSING on timeout */ -static void wait_for_clock_high( +static i2c_res_t wait_for_clock_high( port p_i2c, static const unsigned scl_bit_position, unsigned &fall_time, @@ -63,13 +125,23 @@ static void wait_for_clock_high( static const unsigned kbits_per_second) { const unsigned SCL_HIGH = BIT_MASK(scl_bit_position); + const unsigned TIMEOUT_MULTIPLIER = 1000; // Allow 1000x normal bit time for clock stretching + const unsigned timeout_ticks = BIT_TIME(kbits_per_second) * TIMEOUT_MULTIPLIER; + + timer tmr; + unsigned start_time, timeout_time; + tmr :> start_time; + timeout_time = start_time + timeout_ticks; unsigned val = peek(p_i2c); while (!(val & SCL_HIGH)) { + tmr :> start_time; + if (start_time > timeout_time) { + return I2C_SCL_PULLUP_MISSING; + } val = peek(p_i2c); } - timer tmr; unsigned time; tmr when timerafter(fall_time + delay) :> time; @@ -83,6 +155,8 @@ static void wait_for_clock_high( fall_time = time - compute_low_period_ticks(kbits_per_second) - wake_up_ticks; tmr when timerafter(fall_time + delay) :> void; } + + return I2C_ACK; } static void high_pulse_drive( @@ -103,6 +177,7 @@ static void high_pulse_drive( p_i2c <: SCL_LOW | sdaValue | other_bits_mask; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; p_i2c <: SCL_HIGH | sdaValue | other_bits_mask; + // TODO: This call may hang if no pull-up present - to be addressed in future update wait_for_clock_high(p_i2c, scl_bit_position, fall_time, (bit_time * 3) / 4, kbits_per_second); fall_time = fall_time + bit_time; tmr when timerafter(fall_time) :> void; @@ -126,6 +201,7 @@ static int high_pulse_sample( p_i2c <: SCL_LOW | SDA_HIGH | other_bits_mask; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; + // TODO: This call may hang if no pull-up present - to be addressed in future update wait_for_clock_high(p_i2c, scl_bit_position, fall_time, (bit_time * 3) / 4, kbits_per_second); int sample_value = peek(p_i2c); @@ -141,7 +217,7 @@ static int high_pulse_sample( return sample_value; } -static void start_bit( +static i2c_res_t start_bit( port p_i2c, static const unsigned kbits_per_second, static const unsigned scl_bit_position, @@ -159,16 +235,21 @@ static void start_bit( if (!stopped) { tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; - wait_for_clock_high(p_i2c, scl_bit_position, fall_time, bit_time, kbits_per_second); + i2c_res_t res = wait_for_clock_high(p_i2c, scl_bit_position, fall_time, bit_time, kbits_per_second); + if (res != I2C_ACK) { + return res; + } } p_i2c <: SCL_HIGH | SDA_LOW | other_bits_mask; delay_ticks(bit_time / 2); p_i2c <: SCL_LOW | SDA_LOW | other_bits_mask; tmr :> fall_time; + + return I2C_ACK; } -static void stop_bit( +static i2c_res_t stop_bit( port p_i2c, static const unsigned kbits_per_second, static const unsigned scl_bit_position, @@ -185,9 +266,14 @@ static void stop_bit( p_i2c <: SCL_LOW | SDA_LOW | other_bits_mask; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; p_i2c <: SCL_HIGH | SDA_LOW | other_bits_mask; - wait_for_clock_high(p_i2c, scl_bit_position, fall_time, bit_time, kbits_per_second); + i2c_res_t res = wait_for_clock_high(p_i2c, scl_bit_position, fall_time, bit_time, kbits_per_second); + if (res != I2C_ACK) { + return res; + } p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; delay_ticks(compute_bus_off_ticks(kbits_per_second)); + + return I2C_ACK; } static int tx8( @@ -225,6 +311,9 @@ void i2c_master_single_port( set_port_drive_low(p_i2c); p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; + // Check for pull-up resistors at startup + i2c_res_t bus_error = check_pullups(p_i2c, scl_bit_position, sda_bit_position, other_bits_mask); + while (1) { select { case (size_t i = 0; i < n; i++) @@ -232,9 +321,19 @@ void i2c_master_single_port( c[i].read(uint8_t device, uint8_t buf[m], size_t m, int send_stop_bit) -> i2c_res_t result: + // Return error immediately if bus has problems + if (bus_error != I2C_ACK) { + result = bus_error; + break; + } + const int stopped = locked_client == -1; unsigned fall_time = last_fall_time; - start_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time, stopped); + i2c_res_t start_res = start_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time, stopped); + if (start_res != I2C_ACK) { + result = start_res; + break; + } int ack = tx8(p_i2c, (device << 1) | 1, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); if (ack == 0) { for (size_t j = 0; j < m; j++){ @@ -264,7 +363,12 @@ void i2c_master_single_port( } } if (send_stop_bit) { - stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); + i2c_res_t stop_res = stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); + if (stop_res != I2C_ACK) { + result = stop_res; + last_fall_time = fall_time; + break; + } locked_client = -1; } else { locked_client = i; @@ -282,9 +386,21 @@ void i2c_master_single_port( size_t &num_bytes_sent, int send_stop_bit) -> i2c_res_t result: + // Return error immediately if bus has problems + if (bus_error != I2C_ACK) { + result = bus_error; + num_bytes_sent = 0; + break; + } + const int stopped = locked_client == -1; unsigned fall_time = last_fall_time; - start_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time, stopped); + i2c_res_t start_res = start_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time, stopped); + if (start_res != I2C_ACK) { + result = start_res; + num_bytes_sent = 0; + break; + } int ack = tx8(p_i2c, device<<1, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); size_t j = 0; for (; j < n; j++) { @@ -294,7 +410,13 @@ void i2c_master_single_port( ack = tx8(p_i2c, buf[j], kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); } if (send_stop_bit) { - stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); + i2c_res_t stop_res = stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); + if (stop_res != I2C_ACK) { + result = stop_res; + num_bytes_sent = j; + last_fall_time = fall_time; + break; + } locked_client = -1; } else { locked_client = i; @@ -310,6 +432,8 @@ void i2c_master_single_port( timer tmr; unsigned fall_time; tmr :> fall_time; + // Note: For send_stop_bit, we don't return an error since it's a void function + // But we still need to handle potential hang scenario stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); locked_client = -1; break; From 65d57a2ff93c197c7d3d832c6075119c107d542d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 9 Jul 2025 17:27:02 +0000 Subject: [PATCH 3/6] Implement pull-up detection and timeout mechanisms for two-port I2C master Co-authored-by: xross <642910+xross@users.noreply.github.com> --- lib_i2c/src/i2c_master.xc | 145 +++++++++++++++++++++++++++++++++++--- 1 file changed, 134 insertions(+), 11 deletions(-) diff --git a/lib_i2c/src/i2c_master.xc b/lib_i2c/src/i2c_master.xc index 89481e8..3f4a2fa 100644 --- a/lib_i2c/src/i2c_master.xc +++ b/lib_i2c/src/i2c_master.xc @@ -43,20 +43,89 @@ static unsigned inline compute_bus_off_ticks(static const unsigned kbits_per_sec return bit_time/2 + bit_time/16; } +/** Check if pull-up resistors are present on SCL and SDA lines. + * This function drives the lines low briefly, then releases them + * and checks if they return to high state within a reasonable time. + * + * \returns I2C_ACK if both pull-ups are present, + * I2C_SCL_PULLUP_MISSING if SCL pull-up is missing, + * I2C_SDA_PULLUP_MISSING if SDA pull-up is missing + */ +static i2c_res_t check_pullups_two_port( + port p_scl, + port p_sda) +{ + timer tmr; + unsigned start_time, timeout_time; + const unsigned PULLUP_TIMEOUT_TICKS = 1000; // 10us timeout for pull-up detection + + // Test SCL pull-up + p_scl <: 0; // Drive SCL low + delay_ticks(100); // Brief delay to ensure line is driven low + p_scl :> void; // Release SCL + + tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + // Use select with timeout to avoid hanging + select { + case p_scl when pinseq(1) :> void: + // SCL pull-up is working + break; + case tmr when timerafter(timeout_time) :> void: + return I2C_SCL_PULLUP_MISSING; + } + + // Test SDA pull-up + p_sda <: 0; // Drive SDA low + delay_ticks(100); // Brief delay to ensure line is driven low + p_sda :> void; // Release SDA + + tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + // Use select with timeout to avoid hanging + select { + case p_sda when pinseq(1) :> void: + // SDA pull-up is working + break; + case tmr when timerafter(timeout_time) :> void: + return I2C_SDA_PULLUP_MISSING; + } + + return I2C_ACK; +} + /** Releases the SCL line, reads it back and waits until it goes high (in * case the slave is clock stretching). * Since the line going high may be delayed, the fall_time value may * need to be adjusted + * + * \returns I2C_ACK on success, I2C_SCL_PULLUP_MISSING on timeout */ -static void release_clock_and_wait( +static i2c_res_t release_clock_and_wait( port p_scl, unsigned &fall_time, unsigned delay, static const unsigned kbits_per_second) { - p_scl when pinseq(1) :> void; - + const unsigned TIMEOUT_MULTIPLIER = 1000; // Allow 1000x normal bit time for clock stretching + const unsigned timeout_ticks = BIT_TIME(kbits_per_second) * TIMEOUT_MULTIPLIER; + timer tmr; + unsigned start_time, timeout_time; + tmr :> start_time; + timeout_time = start_time + timeout_ticks; + + // Use select with timeout to avoid hanging + select { + case p_scl when pinseq(1) :> void: + // SCL line went high successfully + break; + case tmr when timerafter(timeout_time) :> void: + return I2C_SCL_PULLUP_MISSING; + } + unsigned time; tmr when timerafter(fall_time + delay) :> time; @@ -70,6 +139,8 @@ static void release_clock_and_wait( fall_time = time - compute_low_period_ticks(kbits_per_second) - wake_up_ticks; tmr when timerafter(fall_time + delay) :> void; } + + return I2C_ACK; } /** 'Pulse' the clock line high and in the middle of the high period @@ -89,6 +160,7 @@ static int inline high_pulse_sample( timer tmr; p_sda :> int _; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; + // TODO: This call may hang if no pull-up present - to be addressed in future update release_clock_and_wait(p_scl, fall_time, (bit_time * 3) / 4, kbits_per_second); p_sda :> sample_value; fall_time = fall_time + bit_time; @@ -111,6 +183,7 @@ static void inline high_pulse( timer tmr; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; + // TODO: This call may hang if no pull-up present - to be addressed in future update release_clock_and_wait(p_scl, fall_time, (bit_time * 3) / 4, kbits_per_second); fall_time = fall_time + bit_time; tmr when timerafter(fall_time) :> void; @@ -120,7 +193,7 @@ static void inline high_pulse( /** Output a start bit. The function updates the 'fall_time', i.e. the * reference clock time when the SCL line transitions to low. */ -static void start_bit( +static i2c_res_t start_bit( port p_scl, port p_sda, static const unsigned kbits_per_second, @@ -133,7 +206,10 @@ static void start_bit( if (!stopped) { tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; - release_clock_and_wait(p_scl, fall_time, bit_time, kbits_per_second); + i2c_res_t res = release_clock_and_wait(p_scl, fall_time, bit_time, kbits_per_second); + if (res != I2C_ACK) { + return res; + } } // Drive SDA low @@ -144,11 +220,13 @@ static void start_bit( // Record tmr :> fall_time; + + return I2C_ACK; } /** Output a stop bit. */ -static void stop_bit( +static i2c_res_t stop_bit( port p_scl, port p_sda, static const unsigned kbits_per_second, @@ -159,9 +237,14 @@ static void stop_bit( timer tmr; p_sda <: 0; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; - release_clock_and_wait(p_scl, fall_time, bit_time, kbits_per_second); + i2c_res_t res = release_clock_and_wait(p_scl, fall_time, bit_time, kbits_per_second); + if (res != I2C_ACK) { + return res; + } p_sda :> void; delay_ticks(compute_bus_off_ticks(kbits_per_second)); + + return I2C_ACK; } /** Transmit 8 bits of data, then read the ack back from the slave and return @@ -203,6 +286,10 @@ void i2c_master( int locked_client = -1; p_scl :> void; p_sda :> void; + + // Check for pull-up resistors at startup + i2c_res_t bus_error = check_pullups_two_port(p_scl, p_sda); + while (1) { select { @@ -211,9 +298,19 @@ void i2c_master( c[i].read(uint8_t device, uint8_t buf[m], size_t m, int send_stop_bit) -> i2c_res_t result: + // Return error immediately if bus has problems + if (bus_error != I2C_ACK) { + result = bus_error; + break; + } + const int stopped = (locked_client == -1); unsigned fall_time = last_fall_time; - start_bit(p_scl, p_sda, kbits_per_second, fall_time, stopped); + i2c_res_t start_res = start_bit(p_scl, p_sda, kbits_per_second, fall_time, stopped); + if (start_res != I2C_ACK) { + result = start_res; + break; + } int ack = tx8(p_scl, p_sda, (device << 1) | 1, kbits_per_second, fall_time); if (ack == 0) { @@ -240,7 +337,12 @@ void i2c_master( } } if (send_stop_bit) { - stop_bit(p_scl, p_sda, kbits_per_second, fall_time); + i2c_res_t stop_res = stop_bit(p_scl, p_sda, kbits_per_second, fall_time); + if (stop_res != I2C_ACK) { + result = stop_res; + last_fall_time = fall_time; + break; + } locked_client = -1; } else { @@ -258,9 +360,22 @@ void i2c_master( c[i].write(uint8_t device, uint8_t buf[n], size_t n, size_t &num_bytes_sent, int send_stop_bit) -> i2c_res_t result: + + // Return error immediately if bus has problems + if (bus_error != I2C_ACK) { + result = bus_error; + num_bytes_sent = 0; + break; + } + unsigned fall_time = last_fall_time; const int stopped = locked_client == -1; - start_bit(p_scl, p_sda, kbits_per_second, fall_time, stopped); + i2c_res_t start_res = start_bit(p_scl, p_sda, kbits_per_second, fall_time, stopped); + if (start_res != I2C_ACK) { + result = start_res; + num_bytes_sent = 0; + break; + } int ack = tx8(p_scl, p_sda, (device << 1), kbits_per_second, fall_time); size_t j = 0; for (; j < n; j++) { @@ -270,7 +385,13 @@ void i2c_master( ack = tx8(p_scl, p_sda, buf[j], kbits_per_second, fall_time); } if (send_stop_bit) { - stop_bit(p_scl, p_sda, kbits_per_second, fall_time); + i2c_res_t stop_res = stop_bit(p_scl, p_sda, kbits_per_second, fall_time); + if (stop_res != I2C_ACK) { + result = stop_res; + num_bytes_sent = j; + last_fall_time = fall_time; + break; + } locked_client = -1; } else { locked_client = i; @@ -286,6 +407,8 @@ void i2c_master( timer tmr; unsigned fall_time; tmr :> fall_time; + // Note: For send_stop_bit, we don't return an error since it's a void function + // But we still need to handle potential hang scenario stop_bit(p_scl, p_sda, kbits_per_second, fall_time); locked_client = -1; break; From 9a4f8faf3e7fc953a1b91c5a5c64fca54d2bad49 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 9 Jul 2025 17:31:12 +0000 Subject: [PATCH 4/6] Implement pull-up detection and timeout mechanisms for async I2C master Co-authored-by: xross <642910+xross@users.noreply.github.com> --- lib_i2c/src/i2c_master_async.xc | 82 +++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/lib_i2c/src/i2c_master_async.xc b/lib_i2c/src/i2c_master_async.xc index fabcb28..20c26a8 100644 --- a/lib_i2c/src/i2c_master_async.xc +++ b/lib_i2c/src/i2c_master_async.xc @@ -199,7 +199,55 @@ void i2c_master_async_comb( int cur_client = -1; int send_stop_bit = 0; int stopped = 1; + + // Check for pull-up resistors at startup i2c_res_t res = I2C_ACK; + { + timer start_tmr; + unsigned start_time, timeout_time; + const unsigned PULLUP_TIMEOUT_TICKS = 1000; // 10us timeout for pull-up detection + + // Test SCL pull-up + p_scl <: 0; // Drive SCL low + delay_ticks(100); // Brief delay + p_scl :> void; // Release SCL + + start_tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + select { + case p_scl when pinseq(1) :> void: + // SCL pull-up is working + break; + case start_tmr when timerafter(timeout_time) :> void: + res = I2C_SCL_PULLUP_MISSING; + break; + } + + if (res == I2C_ACK) { + // Test SDA pull-up + p_sda <: 0; // Drive SDA low + delay_ticks(100); // Brief delay + p_sda :> void; // Release SDA + + start_tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + select { + case p_sda when pinseq(1) :> void: + // SDA pull-up is working + break; + case start_tmr when timerafter(timeout_time) :> void: + res = I2C_SDA_PULLUP_MISSING; + break; + } + } + } + + // Add timeout mechanism for clock stretching/pull-up detection + int clock_timeout_enabled = 0; + int clock_timeout_time = 0; + const int CLOCK_TIMEOUT_TICKS = BIT_TIME(kbits_per_second) * 1000; // 1000x bit time /* These select cases represent the main state machine for the I2C master component. The state machine will change state based on a timer event to @@ -211,6 +259,7 @@ void i2c_master_async_comb( case waiting_for_clock_release => p_scl when pinseq(1) :> void: int now; tmr :> now; + clock_timeout_enabled = 0; // Disable timeout since clock was released successfully switch (state) { case WRITE_0: case WRITE_ACK_0: @@ -256,6 +305,9 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; + // Enable timeout for clock release detection + clock_timeout_enabled = 1; + clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; state = REPEATED_START_HOLD_CLOCK_HIGH; break; case REPEATED_START_HOLD_CLOCK_HIGH: @@ -290,6 +342,9 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; + // Enable timeout for clock release detection + clock_timeout_enabled = 1; + clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; if (bitnum == 8) { state = WRITE_ACK_0; } else { @@ -308,6 +363,9 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; + // Enable timeout for clock release detection + clock_timeout_enabled = 1; + clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; break; case WRITE_ACK_2: int ack; @@ -383,6 +441,9 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; + // Enable timeout for clock release detection + clock_timeout_enabled = 1; + clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; break; case READ_2: int bit; @@ -415,6 +476,9 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; + // Enable timeout for clock release detection + clock_timeout_enabled = 1; + clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; if (bytes_sent == num_bytes) { if (send_stop_bit) { state = STOP_BIT_0; @@ -444,6 +508,9 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; + // Enable timeout for clock release detection + clock_timeout_enabled = 1; + clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; break; case STOP_BIT_3: p_sda :> void; @@ -523,6 +590,21 @@ void i2c_master_async_comb( result = res; break; + case clock_timeout_enabled => tmr when timerafter(clock_timeout_time) :> int now: + // Clock timeout occurred - likely missing pull-up resistor + res = I2C_SCL_PULLUP_MISSING; + waiting_for_clock_release = 0; + clock_timeout_enabled = 0; + timer_enabled = 0; + + // Complete the current operation with error + if (cur_client != -1) { + i[cur_client].operation_complete(); + cur_client = -1; + } + state = IDLE; + break; + case i[int j].shutdown(): return; } From f7a37fa6513f5c076fbfec5db6f833648b9f1cc2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 9 Jul 2025 17:56:08 +0000 Subject: [PATCH 5/6] Implement startup-only pull-up detection following @xross feedback Co-authored-by: xross <642910+xross@users.noreply.github.com> --- lib_i2c/api/i2c.h | 78 ++++++++++++++++-- lib_i2c/src/i2c_master.xc | 72 +++-------------- lib_i2c/src/i2c_master_async.xc | 109 +++++++------------------- lib_i2c/src/i2c_master_single_port.xc | 66 +++------------- 4 files changed, 122 insertions(+), 203 deletions(-) diff --git a/lib_i2c/api/i2c.h b/lib_i2c/api/i2c.h index fab26ad..a9d8707 100644 --- a/lib_i2c/api/i2c.h +++ b/lib_i2c/api/i2c.h @@ -248,13 +248,29 @@ extends client interface i2c_master_if : { uint8_t data[1]; size_t n; i2c_res_t res; - i.write(device_addr, a_reg, 2, n, 0); + res = i.write(device_addr, a_reg, 2, n, 0); + if (res == I2C_SCL_PULLUP_MISSING) { + result = I2C_REGOP_SCL_PULLUP_MISSING; + return 0; + } + if (res == I2C_SDA_PULLUP_MISSING) { + result = I2C_REGOP_SDA_PULLUP_MISSING; + return 0; + } if (n != 2) { result = I2C_REGOP_DEVICE_NACK; i.send_stop_bit(); return 0; } res = i.read(device_addr, data, 1, 1); + if (res == I2C_SCL_PULLUP_MISSING) { + result = I2C_REGOP_SCL_PULLUP_MISSING; + return 0; + } + if (res == I2C_SDA_PULLUP_MISSING) { + result = I2C_REGOP_SDA_PULLUP_MISSING; + return 0; + } if (res == I2C_NACK) { result = I2C_REGOP_DEVICE_NACK; } else { @@ -281,7 +297,13 @@ extends client interface i2c_master_if : { uint8_t data) { uint8_t a_data[3] = {reg >> 8, reg, data}; size_t n; - i.write(device_addr, a_data, 3, n, 1); + i2c_res_t res = i.write(device_addr, a_data, 3, n, 1); + if (res == I2C_SCL_PULLUP_MISSING) { + return I2C_REGOP_SCL_PULLUP_MISSING; + } + if (res == I2C_SDA_PULLUP_MISSING) { + return I2C_REGOP_SDA_PULLUP_MISSING; + } if (n == 0) { return I2C_REGOP_DEVICE_NACK; } @@ -320,13 +342,29 @@ extends client interface i2c_master_if : { uint8_t data[2]; size_t n; i2c_res_t res; - i.write(device_addr, a_reg, 2, n, 0); + res = i.write(device_addr, a_reg, 2, n, 0); + if (res == I2C_SCL_PULLUP_MISSING) { + result = I2C_REGOP_SCL_PULLUP_MISSING; + return 0; + } + if (res == I2C_SDA_PULLUP_MISSING) { + result = I2C_REGOP_SDA_PULLUP_MISSING; + return 0; + } if (n != 2) { result = I2C_REGOP_DEVICE_NACK; i.send_stop_bit(); return 0; } res = i.read(device_addr, data, 2, 1); + if (res == I2C_SCL_PULLUP_MISSING) { + result = I2C_REGOP_SCL_PULLUP_MISSING; + return 0; + } + if (res == I2C_SDA_PULLUP_MISSING) { + result = I2C_REGOP_SDA_PULLUP_MISSING; + return 0; + } if (res == I2C_NACK) { result = I2C_REGOP_DEVICE_NACK; } else { @@ -358,7 +396,13 @@ extends client interface i2c_master_if : { uint16_t data) { uint8_t a_data[4] = {reg >> 8, reg, data >> 8, data}; size_t n; - i.write(device_addr, a_data, 4, n, 1); + i2c_res_t res = i.write(device_addr, a_data, 4, n, 1); + if (res == I2C_SCL_PULLUP_MISSING) { + return I2C_REGOP_SCL_PULLUP_MISSING; + } + if (res == I2C_SDA_PULLUP_MISSING) { + return I2C_REGOP_SDA_PULLUP_MISSING; + } if (n == 0) { return I2C_REGOP_DEVICE_NACK; } @@ -396,13 +440,29 @@ extends client interface i2c_master_if : { uint8_t data[2]; size_t n; i2c_res_t res; - i.write(device_addr, a_reg, 1, n, 0); + res = i.write(device_addr, a_reg, 1, n, 0); + if (res == I2C_SCL_PULLUP_MISSING) { + result = I2C_REGOP_SCL_PULLUP_MISSING; + return 0; + } + if (res == I2C_SDA_PULLUP_MISSING) { + result = I2C_REGOP_SDA_PULLUP_MISSING; + return 0; + } if (n != 1) { result = I2C_REGOP_DEVICE_NACK; i.send_stop_bit(); return 0; } res = i.read(device_addr, data, 2, 1); + if (res == I2C_SCL_PULLUP_MISSING) { + result = I2C_REGOP_SCL_PULLUP_MISSING; + return 0; + } + if (res == I2C_SDA_PULLUP_MISSING) { + result = I2C_REGOP_SDA_PULLUP_MISSING; + return 0; + } if (res == I2C_NACK) { result = I2C_REGOP_DEVICE_NACK; } else { @@ -432,7 +492,13 @@ extends client interface i2c_master_if : { uint16_t data) { uint8_t a_data[3] = {reg, data >> 8, data}; size_t n; - i.write(device_addr, a_data, 3, n, 1); + i2c_res_t res = i.write(device_addr, a_data, 3, n, 1); + if (res == I2C_SCL_PULLUP_MISSING) { + return I2C_REGOP_SCL_PULLUP_MISSING; + } + if (res == I2C_SDA_PULLUP_MISSING) { + return I2C_REGOP_SDA_PULLUP_MISSING; + } if (n == 0) { return I2C_REGOP_DEVICE_NACK; } diff --git a/lib_i2c/src/i2c_master.xc b/lib_i2c/src/i2c_master.xc index 3f4a2fa..752a6a0 100644 --- a/lib_i2c/src/i2c_master.xc +++ b/lib_i2c/src/i2c_master.xc @@ -100,31 +100,15 @@ static i2c_res_t check_pullups_two_port( * case the slave is clock stretching). * Since the line going high may be delayed, the fall_time value may * need to be adjusted - * - * \returns I2C_ACK on success, I2C_SCL_PULLUP_MISSING on timeout */ -static i2c_res_t release_clock_and_wait( +static void release_clock_and_wait( port p_scl, unsigned &fall_time, unsigned delay, static const unsigned kbits_per_second) { - const unsigned TIMEOUT_MULTIPLIER = 1000; // Allow 1000x normal bit time for clock stretching - const unsigned timeout_ticks = BIT_TIME(kbits_per_second) * TIMEOUT_MULTIPLIER; - timer tmr; - unsigned start_time, timeout_time; - tmr :> start_time; - timeout_time = start_time + timeout_ticks; - - // Use select with timeout to avoid hanging - select { - case p_scl when pinseq(1) :> void: - // SCL line went high successfully - break; - case tmr when timerafter(timeout_time) :> void: - return I2C_SCL_PULLUP_MISSING; - } + p_scl when pinseq(1) :> void; unsigned time; tmr when timerafter(fall_time + delay) :> time; @@ -139,8 +123,6 @@ static i2c_res_t release_clock_and_wait( fall_time = time - compute_low_period_ticks(kbits_per_second) - wake_up_ticks; tmr when timerafter(fall_time + delay) :> void; } - - return I2C_ACK; } /** 'Pulse' the clock line high and in the middle of the high period @@ -160,7 +142,6 @@ static int inline high_pulse_sample( timer tmr; p_sda :> int _; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; - // TODO: This call may hang if no pull-up present - to be addressed in future update release_clock_and_wait(p_scl, fall_time, (bit_time * 3) / 4, kbits_per_second); p_sda :> sample_value; fall_time = fall_time + bit_time; @@ -183,7 +164,6 @@ static void inline high_pulse( timer tmr; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; - // TODO: This call may hang if no pull-up present - to be addressed in future update release_clock_and_wait(p_scl, fall_time, (bit_time * 3) / 4, kbits_per_second); fall_time = fall_time + bit_time; tmr when timerafter(fall_time) :> void; @@ -193,7 +173,7 @@ static void inline high_pulse( /** Output a start bit. The function updates the 'fall_time', i.e. the * reference clock time when the SCL line transitions to low. */ -static i2c_res_t start_bit( +static void start_bit( port p_scl, port p_sda, static const unsigned kbits_per_second, @@ -206,10 +186,7 @@ static i2c_res_t start_bit( if (!stopped) { tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; - i2c_res_t res = release_clock_and_wait(p_scl, fall_time, bit_time, kbits_per_second); - if (res != I2C_ACK) { - return res; - } + release_clock_and_wait(p_scl, fall_time, bit_time, kbits_per_second); } // Drive SDA low @@ -220,13 +197,11 @@ static i2c_res_t start_bit( // Record tmr :> fall_time; - - return I2C_ACK; } /** Output a stop bit. */ -static i2c_res_t stop_bit( +static void stop_bit( port p_scl, port p_sda, static const unsigned kbits_per_second, @@ -237,14 +212,9 @@ static i2c_res_t stop_bit( timer tmr; p_sda <: 0; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; - i2c_res_t res = release_clock_and_wait(p_scl, fall_time, bit_time, kbits_per_second); - if (res != I2C_ACK) { - return res; - } + release_clock_and_wait(p_scl, fall_time, bit_time, kbits_per_second); p_sda :> void; delay_ticks(compute_bus_off_ticks(kbits_per_second)); - - return I2C_ACK; } /** Transmit 8 bits of data, then read the ack back from the slave and return @@ -306,11 +276,7 @@ void i2c_master( const int stopped = (locked_client == -1); unsigned fall_time = last_fall_time; - i2c_res_t start_res = start_bit(p_scl, p_sda, kbits_per_second, fall_time, stopped); - if (start_res != I2C_ACK) { - result = start_res; - break; - } + start_bit(p_scl, p_sda, kbits_per_second, fall_time, stopped); int ack = tx8(p_scl, p_sda, (device << 1) | 1, kbits_per_second, fall_time); if (ack == 0) { @@ -337,12 +303,7 @@ void i2c_master( } } if (send_stop_bit) { - i2c_res_t stop_res = stop_bit(p_scl, p_sda, kbits_per_second, fall_time); - if (stop_res != I2C_ACK) { - result = stop_res; - last_fall_time = fall_time; - break; - } + stop_bit(p_scl, p_sda, kbits_per_second, fall_time); locked_client = -1; } else { @@ -370,12 +331,7 @@ void i2c_master( unsigned fall_time = last_fall_time; const int stopped = locked_client == -1; - i2c_res_t start_res = start_bit(p_scl, p_sda, kbits_per_second, fall_time, stopped); - if (start_res != I2C_ACK) { - result = start_res; - num_bytes_sent = 0; - break; - } + start_bit(p_scl, p_sda, kbits_per_second, fall_time, stopped); int ack = tx8(p_scl, p_sda, (device << 1), kbits_per_second, fall_time); size_t j = 0; for (; j < n; j++) { @@ -385,13 +341,7 @@ void i2c_master( ack = tx8(p_scl, p_sda, buf[j], kbits_per_second, fall_time); } if (send_stop_bit) { - i2c_res_t stop_res = stop_bit(p_scl, p_sda, kbits_per_second, fall_time); - if (stop_res != I2C_ACK) { - result = stop_res; - num_bytes_sent = j; - last_fall_time = fall_time; - break; - } + stop_bit(p_scl, p_sda, kbits_per_second, fall_time); locked_client = -1; } else { locked_client = i; @@ -407,8 +357,6 @@ void i2c_master( timer tmr; unsigned fall_time; tmr :> fall_time; - // Note: For send_stop_bit, we don't return an error since it's a void function - // But we still need to handle potential hang scenario stop_bit(p_scl, p_sda, kbits_per_second, fall_time); locked_client = -1; break; diff --git a/lib_i2c/src/i2c_master_async.xc b/lib_i2c/src/i2c_master_async.xc index 20c26a8..73cf39b 100644 --- a/lib_i2c/src/i2c_master_async.xc +++ b/lib_i2c/src/i2c_master_async.xc @@ -199,55 +199,22 @@ void i2c_master_async_comb( int cur_client = -1; int send_stop_bit = 0; int stopped = 1; - - // Check for pull-up resistors at startup i2c_res_t res = I2C_ACK; + + // Check for pull-up resistors at startup (simplified version) + // This checks if the lines are naturally high (indicating pull-ups are present) + i2c_res_t bus_error = I2C_ACK; { - timer start_tmr; - unsigned start_time, timeout_time; - const unsigned PULLUP_TIMEOUT_TICKS = 1000; // 10us timeout for pull-up detection - - // Test SCL pull-up - p_scl <: 0; // Drive SCL low - delay_ticks(100); // Brief delay - p_scl :> void; // Release SCL - - start_tmr :> start_time; - timeout_time = start_time + PULLUP_TIMEOUT_TICKS; - - select { - case p_scl when pinseq(1) :> void: - // SCL pull-up is working - break; - case start_tmr when timerafter(timeout_time) :> void: - res = I2C_SCL_PULLUP_MISSING; - break; - } - - if (res == I2C_ACK) { - // Test SDA pull-up - p_sda <: 0; // Drive SDA low - delay_ticks(100); // Brief delay - p_sda :> void; // Release SDA - - start_tmr :> start_time; - timeout_time = start_time + PULLUP_TIMEOUT_TICKS; - - select { - case p_sda when pinseq(1) :> void: - // SDA pull-up is working - break; - case start_tmr when timerafter(timeout_time) :> void: - res = I2C_SDA_PULLUP_MISSING; - break; - } + // If either line is stuck low at startup, assume missing pull-up + unsigned scl_val, sda_val; + p_scl :> scl_val; + p_sda :> sda_val; + if (!scl_val) { + bus_error = I2C_SCL_PULLUP_MISSING; + } else if (!sda_val) { + bus_error = I2C_SDA_PULLUP_MISSING; } } - - // Add timeout mechanism for clock stretching/pull-up detection - int clock_timeout_enabled = 0; - int clock_timeout_time = 0; - const int CLOCK_TIMEOUT_TICKS = BIT_TIME(kbits_per_second) * 1000; // 1000x bit time /* These select cases represent the main state machine for the I2C master component. The state machine will change state based on a timer event to @@ -259,7 +226,6 @@ void i2c_master_async_comb( case waiting_for_clock_release => p_scl when pinseq(1) :> void: int now; tmr :> now; - clock_timeout_enabled = 0; // Disable timeout since clock was released successfully switch (state) { case WRITE_0: case WRITE_ACK_0: @@ -305,9 +271,6 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; - // Enable timeout for clock release detection - clock_timeout_enabled = 1; - clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; state = REPEATED_START_HOLD_CLOCK_HIGH; break; case REPEATED_START_HOLD_CLOCK_HIGH: @@ -342,9 +305,6 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; - // Enable timeout for clock release detection - clock_timeout_enabled = 1; - clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; if (bitnum == 8) { state = WRITE_ACK_0; } else { @@ -363,9 +323,6 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; - // Enable timeout for clock release detection - clock_timeout_enabled = 1; - clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; break; case WRITE_ACK_2: int ack; @@ -441,9 +398,6 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; - // Enable timeout for clock release detection - clock_timeout_enabled = 1; - clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; break; case READ_2: int bit; @@ -476,9 +430,6 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; - // Enable timeout for clock release detection - clock_timeout_enabled = 1; - clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; if (bytes_sent == num_bytes) { if (send_stop_bit) { state = STOP_BIT_0; @@ -508,9 +459,6 @@ void i2c_master_async_comb( p_scl :> void; timer_enabled = 0; waiting_for_clock_release = 1; - // Enable timeout for clock release detection - clock_timeout_enabled = 1; - clock_timeout_time = now + CLOCK_TIMEOUT_TICKS; break; case STOP_BIT_3: p_sda :> void; @@ -539,6 +487,15 @@ void i2c_master_async_comb( case i[int j].write(uint8_t device_addr, uint8_t buf0[n], size_t n, int _send_stop_bit): + // Return error immediately if bus has problems + if (bus_error != I2C_ACK) { + res = bus_error; + bytes_sent = 0; + cur_client = j; + i[cur_client].operation_complete(); + break; + } + data = (device_addr << 1) | 0; bitnum = 0; optype = WRITE; @@ -559,6 +516,15 @@ void i2c_master_async_comb( break; case i[int j].read(uint8_t device_addr, size_t n, int _send_stop_bit): + // Return error immediately if bus has problems + if (bus_error != I2C_ACK) { + res = bus_error; + bytes_sent = 0; + cur_client = j; + i[cur_client].operation_complete(); + break; + } + data = (device_addr << 1) | 1; bitnum = 0; optype = READ; @@ -590,21 +556,6 @@ void i2c_master_async_comb( result = res; break; - case clock_timeout_enabled => tmr when timerafter(clock_timeout_time) :> int now: - // Clock timeout occurred - likely missing pull-up resistor - res = I2C_SCL_PULLUP_MISSING; - waiting_for_clock_release = 0; - clock_timeout_enabled = 0; - timer_enabled = 0; - - // Complete the current operation with error - if (cur_client != -1) { - i[cur_client].operation_complete(); - cur_client = -1; - } - state = IDLE; - break; - case i[int j].shutdown(): return; } diff --git a/lib_i2c/src/i2c_master_single_port.xc b/lib_i2c/src/i2c_master_single_port.xc index 3720f05..484965f 100644 --- a/lib_i2c/src/i2c_master_single_port.xc +++ b/lib_i2c/src/i2c_master_single_port.xc @@ -114,10 +114,8 @@ static i2c_res_t check_pullups( * line has been release (driven high) before calling this function. * Since the line going high may be delayed, the fall_time * value may need to be adjusted - * - * \returns I2C_ACK on success, I2C_SCL_PULLUP_MISSING on timeout */ -static i2c_res_t wait_for_clock_high( +static void wait_for_clock_high( port p_i2c, static const unsigned scl_bit_position, unsigned &fall_time, @@ -125,23 +123,13 @@ static i2c_res_t wait_for_clock_high( static const unsigned kbits_per_second) { const unsigned SCL_HIGH = BIT_MASK(scl_bit_position); - const unsigned TIMEOUT_MULTIPLIER = 1000; // Allow 1000x normal bit time for clock stretching - const unsigned timeout_ticks = BIT_TIME(kbits_per_second) * TIMEOUT_MULTIPLIER; - - timer tmr; - unsigned start_time, timeout_time; - tmr :> start_time; - timeout_time = start_time + timeout_ticks; unsigned val = peek(p_i2c); while (!(val & SCL_HIGH)) { - tmr :> start_time; - if (start_time > timeout_time) { - return I2C_SCL_PULLUP_MISSING; - } val = peek(p_i2c); } + timer tmr; unsigned time; tmr when timerafter(fall_time + delay) :> time; @@ -155,8 +143,6 @@ static i2c_res_t wait_for_clock_high( fall_time = time - compute_low_period_ticks(kbits_per_second) - wake_up_ticks; tmr when timerafter(fall_time + delay) :> void; } - - return I2C_ACK; } static void high_pulse_drive( @@ -217,7 +203,7 @@ static int high_pulse_sample( return sample_value; } -static i2c_res_t start_bit( +static void start_bit( port p_i2c, static const unsigned kbits_per_second, static const unsigned scl_bit_position, @@ -235,21 +221,16 @@ static i2c_res_t start_bit( if (!stopped) { tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; - i2c_res_t res = wait_for_clock_high(p_i2c, scl_bit_position, fall_time, bit_time, kbits_per_second); - if (res != I2C_ACK) { - return res; - } + wait_for_clock_high(p_i2c, scl_bit_position, fall_time, bit_time, kbits_per_second); } p_i2c <: SCL_HIGH | SDA_LOW | other_bits_mask; delay_ticks(bit_time / 2); p_i2c <: SCL_LOW | SDA_LOW | other_bits_mask; tmr :> fall_time; - - return I2C_ACK; } -static i2c_res_t stop_bit( +static void stop_bit( port p_i2c, static const unsigned kbits_per_second, static const unsigned scl_bit_position, @@ -266,14 +247,9 @@ static i2c_res_t stop_bit( p_i2c <: SCL_LOW | SDA_LOW | other_bits_mask; tmr when timerafter(fall_time + compute_low_period_ticks(kbits_per_second)) :> void; p_i2c <: SCL_HIGH | SDA_LOW | other_bits_mask; - i2c_res_t res = wait_for_clock_high(p_i2c, scl_bit_position, fall_time, bit_time, kbits_per_second); - if (res != I2C_ACK) { - return res; - } + wait_for_clock_high(p_i2c, scl_bit_position, fall_time, bit_time, kbits_per_second); p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; delay_ticks(compute_bus_off_ticks(kbits_per_second)); - - return I2C_ACK; } static int tx8( @@ -329,11 +305,7 @@ void i2c_master_single_port( const int stopped = locked_client == -1; unsigned fall_time = last_fall_time; - i2c_res_t start_res = start_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time, stopped); - if (start_res != I2C_ACK) { - result = start_res; - break; - } + start_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time, stopped); int ack = tx8(p_i2c, (device << 1) | 1, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); if (ack == 0) { for (size_t j = 0; j < m; j++){ @@ -363,12 +335,7 @@ void i2c_master_single_port( } } if (send_stop_bit) { - i2c_res_t stop_res = stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); - if (stop_res != I2C_ACK) { - result = stop_res; - last_fall_time = fall_time; - break; - } + stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); locked_client = -1; } else { locked_client = i; @@ -395,12 +362,7 @@ void i2c_master_single_port( const int stopped = locked_client == -1; unsigned fall_time = last_fall_time; - i2c_res_t start_res = start_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time, stopped); - if (start_res != I2C_ACK) { - result = start_res; - num_bytes_sent = 0; - break; - } + start_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time, stopped); int ack = tx8(p_i2c, device<<1, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); size_t j = 0; for (; j < n; j++) { @@ -410,13 +372,7 @@ void i2c_master_single_port( ack = tx8(p_i2c, buf[j], kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); } if (send_stop_bit) { - i2c_res_t stop_res = stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); - if (stop_res != I2C_ACK) { - result = stop_res; - num_bytes_sent = j; - last_fall_time = fall_time; - break; - } + stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); locked_client = -1; } else { locked_client = i; @@ -432,8 +388,6 @@ void i2c_master_single_port( timer tmr; unsigned fall_time; tmr :> fall_time; - // Note: For send_stop_bit, we don't return an error since it's a void function - // But we still need to handle potential hang scenario stop_bit(p_i2c, kbits_per_second, scl_bit_position, sda_bit_position, other_bits_mask, fall_time); locked_client = -1; break; From 0e14baf357c8514ce5c9a293758faab5f1060b72 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 9 Jul 2025 18:07:00 +0000 Subject: [PATCH 6/6] Extract pull-up detection functions to shared file as requested Co-authored-by: xross <642910+xross@users.noreply.github.com> --- lib_i2c/src/i2c_master.xc | 54 +---------- lib_i2c/src/i2c_master_single_port.xc | 63 +------------ lib_i2c/src/i2c_pullup_common.h | 44 +++++++++ lib_i2c/src/i2c_pullup_common.xc | 128 ++++++++++++++++++++++++++ 4 files changed, 175 insertions(+), 114 deletions(-) create mode 100644 lib_i2c/src/i2c_pullup_common.h create mode 100644 lib_i2c/src/i2c_pullup_common.xc diff --git a/lib_i2c/src/i2c_master.xc b/lib_i2c/src/i2c_master.xc index 752a6a0..1d9f95a 100644 --- a/lib_i2c/src/i2c_master.xc +++ b/lib_i2c/src/i2c_master.xc @@ -6,6 +6,7 @@ #include #include "xassert.h" +#include "i2c_pullup_common.h" /* NOTE: the kbits_per_second needs to be passed around due to the fact that the * compiler won't compute a new static const from a static const. @@ -43,59 +44,6 @@ static unsigned inline compute_bus_off_ticks(static const unsigned kbits_per_sec return bit_time/2 + bit_time/16; } -/** Check if pull-up resistors are present on SCL and SDA lines. - * This function drives the lines low briefly, then releases them - * and checks if they return to high state within a reasonable time. - * - * \returns I2C_ACK if both pull-ups are present, - * I2C_SCL_PULLUP_MISSING if SCL pull-up is missing, - * I2C_SDA_PULLUP_MISSING if SDA pull-up is missing - */ -static i2c_res_t check_pullups_two_port( - port p_scl, - port p_sda) -{ - timer tmr; - unsigned start_time, timeout_time; - const unsigned PULLUP_TIMEOUT_TICKS = 1000; // 10us timeout for pull-up detection - - // Test SCL pull-up - p_scl <: 0; // Drive SCL low - delay_ticks(100); // Brief delay to ensure line is driven low - p_scl :> void; // Release SCL - - tmr :> start_time; - timeout_time = start_time + PULLUP_TIMEOUT_TICKS; - - // Use select with timeout to avoid hanging - select { - case p_scl when pinseq(1) :> void: - // SCL pull-up is working - break; - case tmr when timerafter(timeout_time) :> void: - return I2C_SCL_PULLUP_MISSING; - } - - // Test SDA pull-up - p_sda <: 0; // Drive SDA low - delay_ticks(100); // Brief delay to ensure line is driven low - p_sda :> void; // Release SDA - - tmr :> start_time; - timeout_time = start_time + PULLUP_TIMEOUT_TICKS; - - // Use select with timeout to avoid hanging - select { - case p_sda when pinseq(1) :> void: - // SDA pull-up is working - break; - case tmr when timerafter(timeout_time) :> void: - return I2C_SDA_PULLUP_MISSING; - } - - return I2C_ACK; -} - /** Releases the SCL line, reads it back and waits until it goes high (in * case the slave is clock stretching). * Since the line going high may be delayed, the fall_time value may diff --git a/lib_i2c/src/i2c_master_single_port.xc b/lib_i2c/src/i2c_master_single_port.xc index 484965f..e6220d6 100644 --- a/lib_i2c/src/i2c_master_single_port.xc +++ b/lib_i2c/src/i2c_master_single_port.xc @@ -9,6 +9,7 @@ #include #include "xassert.h" +#include "i2c_pullup_common.h" #define SDA_LOW 0 #define SCL_LOW 0 @@ -49,66 +50,6 @@ static unsigned inline compute_bus_off_ticks(static const unsigned kbits_per_sec return bit_time/2 + bit_time/16; } -/** Check if pull-up resistors are present on SCL and SDA lines. - * This function drives the lines low briefly, then releases them - * and checks if they return to high state within a reasonable time. - * - * \returns I2C_ACK if both pull-ups are present, - * I2C_SCL_PULLUP_MISSING if SCL pull-up is missing, - * I2C_SDA_PULLUP_MISSING if SDA pull-up is missing - */ -static i2c_res_t check_pullups( - port p_i2c, - static const unsigned scl_bit_position, - static const unsigned sda_bit_position, - static const unsigned other_bits_mask) -{ - const unsigned SCL_HIGH = BIT_MASK(scl_bit_position); - const unsigned SDA_HIGH = BIT_MASK(sda_bit_position); - const unsigned SCL_LOW = 0; - const unsigned SDA_LOW = 0; - - timer tmr; - unsigned start_time, timeout_time; - const unsigned PULLUP_TIMEOUT_TICKS = 1000; // 10us timeout for pull-up detection - - // Test SCL pull-up - p_i2c <: SCL_LOW | SDA_HIGH | other_bits_mask; // Drive SCL low, SDA high - delay_ticks(100); // Brief delay to ensure line is driven low - p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; // Release SCL - - tmr :> start_time; - timeout_time = start_time + PULLUP_TIMEOUT_TICKS; - - unsigned val = peek(p_i2c); - while (!(val & SCL_HIGH)) { - tmr :> start_time; - if (start_time > timeout_time) { - return I2C_SCL_PULLUP_MISSING; - } - val = peek(p_i2c); - } - - // Test SDA pull-up - p_i2c <: SCL_HIGH | SDA_LOW | other_bits_mask; // Drive SDA low, SCL high - delay_ticks(100); // Brief delay to ensure line is driven low - p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; // Release SDA - - tmr :> start_time; - timeout_time = start_time + PULLUP_TIMEOUT_TICKS; - - val = peek(p_i2c); - while (!(val & SDA_HIGH)) { - tmr :> start_time; - if (start_time > timeout_time) { - return I2C_SDA_PULLUP_MISSING; - } - val = peek(p_i2c); - } - - return I2C_ACK; -} - /** Reads back the SCL line, waiting until it goes high (in * case the slave is clock stretching). It is assumed that the clock * line has been release (driven high) before calling this function. @@ -288,7 +229,7 @@ void i2c_master_single_port( p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; // Check for pull-up resistors at startup - i2c_res_t bus_error = check_pullups(p_i2c, scl_bit_position, sda_bit_position, other_bits_mask); + i2c_res_t bus_error = check_pullups_single_port(p_i2c, scl_bit_position, sda_bit_position, other_bits_mask); while (1) { select { diff --git a/lib_i2c/src/i2c_pullup_common.h b/lib_i2c/src/i2c_pullup_common.h new file mode 100644 index 0000000..03e0ca8 --- /dev/null +++ b/lib_i2c/src/i2c_pullup_common.h @@ -0,0 +1,44 @@ +// Copyright 2025 XMOS LIMITED. +// This Software is subject to the terms of the XMOS Public Licence: Version 1. + +#ifndef _i2c_pullup_common_h_ +#define _i2c_pullup_common_h_ + +#include +#include + +/** Check if pull-up resistors are present on SCL and SDA lines for two-port I2C. + * This function drives the lines low briefly, then releases them + * and checks if they return to high state within a reasonable time. + * + * \param p_scl SCL port + * \param p_sda SDA port + * + * \returns I2C_ACK if both pull-ups are present, + * I2C_SCL_PULLUP_MISSING if SCL pull-up is missing, + * I2C_SDA_PULLUP_MISSING if SDA pull-up is missing + */ +i2c_res_t check_pullups_two_port( + port p_scl, + port p_sda); + +/** Check if pull-up resistors are present on SCL and SDA lines for single-port I2C. + * This function drives the lines low briefly, then releases them + * and checks if they return to high state within a reasonable time. + * + * \param p_i2c single port containing both SCL and SDA + * \param scl_bit_position bit position of SCL on the port + * \param sda_bit_position bit position of SDA on the port + * \param other_bits_mask mask for other bits that should be preserved + * + * \returns I2C_ACK if both pull-ups are present, + * I2C_SCL_PULLUP_MISSING if SCL pull-up is missing, + * I2C_SDA_PULLUP_MISSING if SDA pull-up is missing + */ +i2c_res_t check_pullups_single_port( + port p_i2c, + static const unsigned scl_bit_position, + static const unsigned sda_bit_position, + static const unsigned other_bits_mask); + +#endif // _i2c_pullup_common_h_ \ No newline at end of file diff --git a/lib_i2c/src/i2c_pullup_common.xc b/lib_i2c/src/i2c_pullup_common.xc new file mode 100644 index 0000000..ccf511f --- /dev/null +++ b/lib_i2c/src/i2c_pullup_common.xc @@ -0,0 +1,128 @@ +// Copyright 2025 XMOS LIMITED. +// This Software is subject to the terms of the XMOS Public Licence: Version 1. + +#include +#include +#include +#include + +/** Check if pull-up resistors are present on SCL and SDA lines for two-port I2C. + * This function drives the lines low briefly, then releases them + * and checks if they return to high state within a reasonable time. + * + * \param p_scl SCL port + * \param p_sda SDA port + * + * \returns I2C_ACK if both pull-ups are present, + * I2C_SCL_PULLUP_MISSING if SCL pull-up is missing, + * I2C_SDA_PULLUP_MISSING if SDA pull-up is missing + */ +i2c_res_t check_pullups_two_port( + port p_scl, + port p_sda) +{ + timer tmr; + unsigned start_time, timeout_time; + const unsigned PULLUP_TIMEOUT_TICKS = 1000; // 10us timeout for pull-up detection + + // Test SCL pull-up + p_scl <: 0; // Drive SCL low + delay_ticks(100); // Brief delay to ensure line is driven low + p_scl :> void; // Release SCL + + tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + // Use select with timeout to avoid hanging + select { + case p_scl when pinseq(1) :> void: + // SCL pull-up is working + break; + case tmr when timerafter(timeout_time) :> void: + return I2C_SCL_PULLUP_MISSING; + } + + // Test SDA pull-up + p_sda <: 0; // Drive SDA low + delay_ticks(100); // Brief delay to ensure line is driven low + p_sda :> void; // Release SDA + + tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + // Use select with timeout to avoid hanging + select { + case p_sda when pinseq(1) :> void: + // SDA pull-up is working + break; + case tmr when timerafter(timeout_time) :> void: + return I2C_SDA_PULLUP_MISSING; + } + + return I2C_ACK; +} + +/** Check if pull-up resistors are present on SCL and SDA lines for single-port I2C. + * This function drives the lines low briefly, then releases them + * and checks if they return to high state within a reasonable time. + * + * \param p_i2c single port containing both SCL and SDA + * \param scl_bit_position bit position of SCL on the port + * \param sda_bit_position bit position of SDA on the port + * \param other_bits_mask mask for other bits that should be preserved + * + * \returns I2C_ACK if both pull-ups are present, + * I2C_SCL_PULLUP_MISSING if SCL pull-up is missing, + * I2C_SDA_PULLUP_MISSING if SDA pull-up is missing + */ +i2c_res_t check_pullups_single_port( + port p_i2c, + static const unsigned scl_bit_position, + static const unsigned sda_bit_position, + static const unsigned other_bits_mask) +{ + const unsigned SCL_HIGH = BIT_MASK(scl_bit_position); + const unsigned SDA_HIGH = BIT_MASK(sda_bit_position); + const unsigned SCL_LOW = 0; + const unsigned SDA_LOW = 0; + + timer tmr; + unsigned start_time, timeout_time; + const unsigned PULLUP_TIMEOUT_TICKS = 1000; // 10us timeout for pull-up detection + + // Test SCL pull-up + p_i2c <: SCL_LOW | SDA_HIGH | other_bits_mask; // Drive SCL low, SDA high + delay_ticks(100); // Brief delay to ensure line is driven low + p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; // Release SCL + + tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + unsigned val = peek(p_i2c); + while (!(val & SCL_HIGH)) { + tmr :> start_time; + if (start_time > timeout_time) { + return I2C_SCL_PULLUP_MISSING; + } + val = peek(p_i2c); + } + + // Test SDA pull-up + p_i2c <: SCL_HIGH | SDA_LOW | other_bits_mask; // Drive SDA low, SCL high + delay_ticks(100); // Brief delay to ensure line is driven low + p_i2c <: SCL_HIGH | SDA_HIGH | other_bits_mask; // Release SDA + + tmr :> start_time; + timeout_time = start_time + PULLUP_TIMEOUT_TICKS; + + val = peek(p_i2c); + while (!(val & SDA_HIGH)) { + tmr :> start_time; + if (start_time > timeout_time) { + return I2C_SDA_PULLUP_MISSING; + } + val = peek(p_i2c); + } + + return I2C_ACK; +} \ No newline at end of file