Skip to content

Add pull-up detection and timeout mechanisms to prevent I2C hangs #113

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.

Sign up for GitHub

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

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from
Draft

Add pull-up detection and timeout mechanisms to prevent I2C hangs #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
569fce6
Initial plan
Copilot Jul 9, 2025
ebf5ce4
Implement pull-up detection and timeout mechanisms for single port I2…
Copilot Jul 9, 2025
65d57a2
Implement pull-up detection and timeout mechanisms for two-port I2C m…
Copilot Jul 9, 2025
9a4f8fa
Implement pull-up detection and timeout mechanisms for async I2C master
Copilot Jul 9, 2025
f7a37fa
Implement startup-only pull-up detection following @xross feedback
Copilot Jul 9, 2025
0e14baf
Extract pull-up detection functions to shared file as requested
Copilot Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 100 additions & 8 deletions lib_i2c/api/i2c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)
Expand Down Expand Up @@ -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__
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
}
Expand Down Expand Up @@ -222,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 {
Expand All @@ -255,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;
}
Expand Down Expand Up @@ -294,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 {
Expand Down Expand Up @@ -332,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;
}
Expand Down Expand Up @@ -370,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 {
Expand Down Expand Up @@ -406,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;
}
Expand Down
21 changes: 20 additions & 1 deletion lib_i2c/src/i2c_master.xc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <timer.h>

#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.
Expand Down Expand Up @@ -54,9 +55,9 @@ static void release_clock_and_wait(
unsigned delay,
static const unsigned kbits_per_second)
{
timer tmr;
p_scl when pinseq(1) :> void;

timer tmr;
unsigned time;
tmr when timerafter(fall_time + delay) :> time;

Expand Down Expand Up @@ -203,6 +204,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 {

Expand All @@ -211,6 +216,12 @@ 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);
Expand Down Expand Up @@ -258,6 +269,14 @@ 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);
Expand Down
33 changes: 33 additions & 0 deletions lib_i2c/src/i2c_master_async.xc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,21 @@ void i2c_master_async_comb(
int stopped = 1;
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;
{
// 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;
}
}

/* 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
progress the transaction or on an event from the SCL line when waiting
Expand Down Expand Up @@ -472,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;
Expand All @@ -492,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;
Expand Down
19 changes: 19 additions & 0 deletions lib_i2c/src/i2c_master_single_port.xc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <timer.h>

#include "xassert.h"
#include "i2c_pullup_common.h"

#define SDA_LOW 0
#define SCL_LOW 0
Expand Down Expand Up @@ -103,6 +104,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);
Copy link
Contributor

@xross xross Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont need this comment since we're checking pull ups are present before we call this function

fall_time = fall_time + bit_time;
tmr when timerafter(fall_time) :> void;
Expand All @@ -126,6 +128,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
Copy link
Contributor

@xross xross Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont need this comment since we're checking pull ups are present before we call this function

wait_for_clock_high(p_i2c, scl_bit_position, fall_time, (bit_time * 3) / 4, kbits_per_second);

int sample_value = peek(p_i2c);
Expand Down Expand Up @@ -225,13 +228,22 @@ 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_single_port(p_i2c, scl_bit_position, sda_bit_position, other_bits_mask);
Copy link
Contributor

@xross xross Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check both pull ups at start up


while (1) {
select {
case (size_t i = 0; i < n; i++)
(n == 1 || (locked_client == -1 || i == locked_client)) =>
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);
Expand Down Expand Up @@ -282,6 +294,13 @@ 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);
Expand Down
Loading