Skip to content

Commit 61e2296

Browse files
adamgreenadamgreen
committed
Fix NXP LPCxxxx i2c_start() handling of repeated start
In repeating start scenarios, there was a bug in the I2C driver for various NXP LPCxxxx parts which could allow an extra I/O from the previous operation to leak through. The scenario I encountered which triggered this bug went like so: * The higher level application code would first do an I2C write that doesn't send a stop bit (use repeating start instead.) * The higher level application code would then issues an I2C read operation which begin with a call to i2c_start(). * i2c_start() would clear the SI bit (interrupt flag) at the top of its implementation. * i2C_start() would then get interrupted right after clearing the SI bit. * While the CPU is off running the ISR code, the I2C peripheral repeats the last byte written to I2CDAT and then sets the SI bit to indicate that the write has completed. * The ISR returns to allow the i2c_start() to continue execution. * i2c_start() would then set the STA bit but it is too late. * i2c_start() waits for the SI bit to be set but it is already set because of the completed byte write and not because of the repeated start as expected. For me this bug would cause strange interactions between my ISRs and the code using I2C to read from the MPU-6050 IMU. I would be getting valid orientation data and then all of a sudden I would start receiving what looked like random values but I think it was just reading from the incorrect offset in the device's FIFO. It appears that atleast one other person has seen this before but it was never root caused since it required specific timing to reproduce: https://developer.mbed.org/forum/bugs-suggestions/topic/4254/ This bug can be found in most of the NXP I2C drivers and this commit contains a fix for them all. I however only have the LPC1768 and LPC11U24 for testing. My fix does the following: * No longer clears the SI bit in the i2c_conclr() call near the beginning of the i2c_start() function. It was this clear that previously caused the problem as described above. * The second part of the fix was to instead clear the SI bit after the STA (start) bit has been set with the i2c_conset() call. * The clearing of the SI bit should be skipped if there isn't an active interrupt when first entering i2c_start(). If you clear the SI bit when there isn't an active interrupt then the code is likely to skip over the interrupt for the start bit which was just sent and then allow the I2C peripheral to start sending the slave address before it has even been loaded into the I2CDAT register.
1 parent b32f7a9 commit 61e2296

File tree

10 files changed

+117
-57
lines changed

10 files changed

+117
-57
lines changed

‎hal/targets/hal/TARGET_NXP/TARGET_LPC11U6X/i2c_api.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -111,20 +111,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
111111

112112
inline int i2c_start(i2c_t *obj) {
113113
int status = 0;
114+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
115+
114116
// 8.1 Before master mode can be entered, I2CON must be initialised to:
115117
// - I2EN STA STO SI AA - -
116-
// - 1 0 0 0 x - -
118+
// - 1 0 0 x x - -
117119
// if AA = 0, it can't enter slave mode
118-
i2c_conclr(obj, 1, 1, 1, 1);
119-
120+
i2c_conclr(obj, 1, 1, 0, 1);
121+
120122
// The master mode may now be entered by setting the STA bit
121123
// this will generate a start condition when the bus becomes free
122124
i2c_conset(obj, 1, 0, 0, 1);
123-
125+
// Clearing SI bit when it wasn't set on entry can jump past state
126+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
127+
if (isInterrupted)
128+
i2c_clear_SI(obj);
129+
124130
i2c_wait_SI(obj);
125131
status = i2c_status(obj);
126-
127-
// Clear start bit now transmitted, and interrupt bit
132+
133+
// Clear start bit now that it's transmitted
128134
i2c_conclr(obj, 1, 0, 0, 0);
129135
return status;
130136
}

‎hal/targets/hal/TARGET_NXP/TARGET_LPC11UXX/i2c_api.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -94,20 +94,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
9494

9595
inline int i2c_start(i2c_t *obj) {
9696
int status = 0;
97+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
98+
9799
// 8.1 Before master mode can be entered, I2CON must be initialised to:
98100
// - I2EN STA STO SI AA - -
99-
// - 1 0 0 0 x - -
101+
// - 1 0 0 x x - -
100102
// if AA = 0, it can't enter slave mode
101-
i2c_conclr(obj, 1, 1, 1, 1);
102-
103+
i2c_conclr(obj, 1, 1, 0, 1);
104+
103105
// The master mode may now be entered by setting the STA bit
104106
// this will generate a start condition when the bus becomes free
105107
i2c_conset(obj, 1, 0, 0, 1);
106-
108+
// Clearing SI bit when it wasn't set on entry can jump past state
109+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
110+
if (isInterrupted)
111+
i2c_clear_SI(obj);
112+
107113
i2c_wait_SI(obj);
108114
status = i2c_status(obj);
109-
110-
// Clear start bit now transmitted, and interrupt bit
115+
116+
// Clear start bit now that it's transmitted
111117
i2c_conclr(obj, 1, 0, 0, 0);
112118
return status;
113119
}

‎hal/targets/hal/TARGET_NXP/TARGET_LPC11XX_11CXX/i2c_api.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,20 +104,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
104104

105105
inline int i2c_start(i2c_t *obj) {
106106
int status = 0;
107+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
108+
107109
// 8.1 Before master mode can be entered, I2CON must be initialised to:
108110
// - I2EN STA STO SI AA - -
109-
// - 1 0 0 0 x - -
111+
// - 1 0 0 x x - -
110112
// if AA = 0, it can't enter slave mode
111-
i2c_conclr(obj, 1, 1, 1, 1);
112-
113+
i2c_conclr(obj, 1, 1, 0, 1);
114+
113115
// The master mode may now be entered by setting the STA bit
114116
// this will generate a start condition when the bus becomes free
115117
i2c_conset(obj, 1, 0, 0, 1);
116-
118+
// Clearing SI bit when it wasn't set on entry can jump past state
119+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
120+
if (isInterrupted)
121+
i2c_clear_SI(obj);
122+
117123
i2c_wait_SI(obj);
118124
status = i2c_status(obj);
119-
120-
// Clear start bit now transmitted, and interrupt bit
125+
126+
// Clear start bit now that it's transmitted
121127
i2c_conclr(obj, 1, 0, 0, 0);
122128
return status;
123129
}

‎hal/targets/hal/TARGET_NXP/TARGET_LPC13XX/i2c_api.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,20 +104,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
104104

105105
inline int i2c_start(i2c_t *obj) {
106106
int status = 0;
107+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
108+
107109
// 8.1 Before master mode can be entered, I2CON must be initialised to:
108110
// - I2EN STA STO SI AA - -
109-
// - 1 0 0 0 x - -
111+
// - 1 0 0 x x - -
110112
// if AA = 0, it can't enter slave mode
111-
i2c_conclr(obj, 1, 1, 1, 1);
112-
113+
i2c_conclr(obj, 1, 1, 0, 1);
114+
113115
// The master mode may now be entered by setting the STA bit
114116
// this will generate a start condition when the bus becomes free
115117
i2c_conset(obj, 1, 0, 0, 1);
116-
118+
// Clearing SI bit when it wasn't set on entry can jump past state
119+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
120+
if (isInterrupted)
121+
i2c_clear_SI(obj);
122+
117123
i2c_wait_SI(obj);
118124
status = i2c_status(obj);
119-
120-
// Clear start bit now transmitted, and interrupt bit
125+
126+
// Clear start bit now that it's transmitted
121127
i2c_conclr(obj, 1, 0, 0, 0);
122128
return status;
123129
}

‎hal/targets/hal/TARGET_NXP/TARGET_LPC176X/i2c_api.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,20 +112,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
112112

113113
inline int i2c_start(i2c_t *obj) {
114114
int status = 0;
115+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
116+
115117
// 8.1 Before master mode can be entered, I2CON must be initialised to:
116118
// - I2EN STA STO SI AA - -
117-
// - 1 0 0 0 x - -
119+
// - 1 0 0 x x - -
118120
// if AA = 0, it can't enter slave mode
119-
i2c_conclr(obj, 1, 1, 1, 1);
120-
121+
i2c_conclr(obj, 1, 1, 0, 1);
122+
121123
// The master mode may now be entered by setting the STA bit
122124
// this will generate a start condition when the bus becomes free
123125
i2c_conset(obj, 1, 0, 0, 1);
124-
126+
// Clearing SI bit when it wasn't set on entry can jump past state
127+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
128+
if (isInterrupted)
129+
i2c_clear_SI(obj);
130+
125131
i2c_wait_SI(obj);
126132
status = i2c_status(obj);
127-
128-
// Clear start bit now transmitted, and interrupt bit
133+
134+
// Clear start bit now that it's transmitted
129135
i2c_conclr(obj, 1, 0, 0, 0);
130136
return status;
131137
}

‎hal/targets/hal/TARGET_NXP/TARGET_LPC23XX/i2c_api.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -111,20 +111,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
111111

112112
inline int i2c_start(i2c_t *obj) {
113113
int status = 0;
114+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
115+
114116
// 8.1 Before master mode can be entered, I2CON must be initialised to:
115117
// - I2EN STA STO SI AA - -
116-
// - 1 0 0 0 x - -
118+
// - 1 0 0 x x - -
117119
// if AA = 0, it can't enter slave mode
118-
i2c_conclr(obj, 1, 1, 1, 1);
119-
120+
i2c_conclr(obj, 1, 1, 0, 1);
121+
120122
// The master mode may now be entered by setting the STA bit
121123
// this will generate a start condition when the bus becomes free
122124
i2c_conset(obj, 1, 0, 0, 1);
123-
125+
// Clearing SI bit when it wasn't set on entry can jump past state
126+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
127+
if (isInterrupted)
128+
i2c_clear_SI(obj);
129+
124130
i2c_wait_SI(obj);
125131
status = i2c_status(obj);
126-
127-
// Clear start bit now transmitted, and interrupt bit
132+
133+
// Clear start bit now that it's transmitted
128134
i2c_conclr(obj, 1, 0, 0, 0);
129135
return status;
130136
}

‎hal/targets/hal/TARGET_NXP/TARGET_LPC2460/i2c_api.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -117,20 +117,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
117117

118118
inline int i2c_start(i2c_t *obj) {
119119
int status = 0;
120+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
121+
120122
// 8.1 Before master mode can be entered, I2CON must be initialised to:
121123
// - I2EN STA STO SI AA - -
122-
// - 1 0 0 0 x - -
124+
// - 1 0 0 x x - -
123125
// if AA = 0, it can't enter slave mode
124-
i2c_conclr(obj, 1, 1, 1, 1);
125-
126+
i2c_conclr(obj, 1, 1, 0, 1);
127+
126128
// The master mode may now be entered by setting the STA bit
127129
// this will generate a start condition when the bus becomes free
128130
i2c_conset(obj, 1, 0, 0, 1);
129-
131+
// Clearing SI bit when it wasn't set on entry can jump past state
132+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
133+
if (isInterrupted)
134+
i2c_clear_SI(obj);
135+
130136
i2c_wait_SI(obj);
131137
status = i2c_status(obj);
132-
133-
// Clear start bit now transmitted, and interrupt bit
138+
139+
// Clear start bit now that it's transmitted
134140
i2c_conclr(obj, 1, 0, 0, 0);
135141
return status;
136142
}

‎hal/targets/hal/TARGET_NXP/TARGET_LPC408X/TARGET_LPC4088/i2c_api.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -133,20 +133,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
133133

134134
inline int i2c_start(i2c_t *obj) {
135135
int status = 0;
136+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
137+
136138
// 8.1 Before master mode can be entered, I2CON must be initialised to:
137139
// - I2EN STA STO SI AA - -
138-
// - 1 0 0 0 x - -
140+
// - 1 0 0 x x - -
139141
// if AA = 0, it can't enter slave mode
140-
i2c_conclr(obj, 1, 1, 1, 1);
141-
142+
i2c_conclr(obj, 1, 1, 0, 1);
143+
142144
// The master mode may now be entered by setting the STA bit
143145
// this will generate a start condition when the bus becomes free
144146
i2c_conset(obj, 1, 0, 0, 1);
145-
147+
// Clearing SI bit when it wasn't set on entry can jump past state
148+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
149+
if (isInterrupted)
150+
i2c_clear_SI(obj);
151+
146152
i2c_wait_SI(obj);
147153
status = i2c_status(obj);
148-
149-
// Clear start bit now transmitted, and interrupt bit
154+
155+
// Clear start bit now that it's transmitted
150156
i2c_conclr(obj, 1, 0, 0, 0);
151157
return status;
152158
}

‎hal/targets/hal/TARGET_NXP/TARGET_LPC408X/TARGET_LPC4088_DM/i2c_api.c

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,20 +119,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
119119

120120
inline int i2c_start(i2c_t *obj) {
121121
int status = 0;
122+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
123+
122124
// 8.1 Before master mode can be entered, I2CON must be initialised to:
123125
// - I2EN STA STO SI AA - -
124-
// - 1 0 0 0 x - -
126+
// - 1 0 0 x x - -
125127
// if AA = 0, it can't enter slave mode
126-
i2c_conclr(obj, 1, 1, 1, 1);
128+
i2c_conclr(obj, 1, 1, 0, 1);
127129

128130
// The master mode may now be entered by setting the STA bit
129131
// this will generate a start condition when the bus becomes free
130132
i2c_conset(obj, 1, 0, 0, 1);
133+
// Clearing SI bit when it wasn't set on entry can jump past state
134+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
135+
if (isInterrupted)
136+
i2c_clear_SI(obj);
131137

132138
i2c_wait_SI(obj);
133139
status = i2c_status(obj);
134140

135-
// Clear start bit now transmitted, and interrupt bit
141+
// Clear start bit now that it's transmitted
136142
i2c_conclr(obj, 1, 0, 0, 0);
137143
return status;
138144
}

‎hal/targets/hal/TARGET_NXP/TARGET_LPC43XX/i2c_api.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -113,20 +113,26 @@ void i2c_init(i2c_t *obj, PinName sda, PinName scl) {
113113

114114
inline int i2c_start(i2c_t *obj) {
115115
int status = 0;
116+
int isInterrupted = I2C_CONSET(obj) & (1 << 3);
117+
116118
// 8.1 Before master mode can be entered, I2CON must be initialised to:
117119
// - I2EN STA STO SI AA - -
118-
// - 1 0 0 0 x - -
120+
// - 1 0 0 x x - -
119121
// if AA = 0, it can't enter slave mode
120-
i2c_conclr(obj, 1, 1, 1, 1);
121-
122+
i2c_conclr(obj, 1, 1, 0, 1);
123+
122124
// The master mode may now be entered by setting the STA bit
123125
// this will generate a start condition when the bus becomes free
124126
i2c_conset(obj, 1, 0, 0, 1);
125-
127+
// Clearing SI bit when it wasn't set on entry can jump past state
128+
// 0x10 or 0x08 and erroneously send uninitialized slave address.
129+
if (isInterrupted)
130+
i2c_clear_SI(obj);
131+
126132
i2c_wait_SI(obj);
127133
status = i2c_status(obj);
128-
129-
// Clear start bit now transmitted, and interrupt bit
134+
135+
// Clear start bit now that it's transmitted
130136
i2c_conclr(obj, 1, 0, 0, 0);
131137
return status;
132138
}

0 commit comments

Comments
 (0)