Fix NXP LPCxxxx i2c_start() handling of repeated start

[adamgreen]

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 begins 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.

My Repro Case
I wrote two samples to reproduce this bug and verify that it was fixed by this change. The master sample is expected to be connected to a device which is running the slave sample.

Master test code which is a modified version of what I found in https://github.com/mbedmicro/mbed/blob/master/libraries/tests/mbed/i2c_master/main.cpp I modified it to read registers using repeated starts like I was doing with the MPU-6050 sensor. I then schedule an ISR to trigger at a random time in the range of when I expect the repeated start to occur:

#include "mbed.h"
//#include "test_env.h"
void notify_completion(bool wasTestSuccessful) {
    printf("Test completed - %s\n", wasTestSuccessful ? "PASSED" : "FAILED");
}

#define ADDR (0x90)

#if defined(TARGET_KL25Z)
I2C i2c(PTE0, PTE1);
#elif defined(TARGET_nRF51822)
I2C i2c(p22,p20);
#elif defined(TARGET_FF_ARDUINO) || defined(TARGET_MAXWSNENV)
I2C i2c(I2C_SDA, I2C_SCL);
#elif defined(TARGET_EFM32LG_STK3600) || defined(TARGET_EFM32GG_STK3700) || defined(TARGET_EFM32WG_STK3800)
#define TEST_SDA_PIN PD6
#define TEST_SCL_PIN PD7
I2C i2c(TEST_SDA_PIN, TEST_SCL_PIN);
#elif defined(TARGET_EFM32ZG_STK3200)
#define TEST_SDA_PIN PE12
#define TEST_SCL_PIN PE13
I2C i2c(TEST_SDA_PIN, TEST_SCL_PIN);
#elif defined(TARGET_EFM32HG_STK3400)
#define TEST_SDA_PIN PD6
#define TEST_SCL_PIN PD7
I2C i2c(TEST_SDA_PIN, TEST_SCL_PIN);
#elif defined(TARGET_SAMR21G18A)
#define TEST_SDA_PIN PA16
#define TEST_SCL_PIN PA17
I2C i2c(TEST_SDA_PIN, TEST_SCL_PIN);
#elif defined(TARGET_SAMD21J18A)
#define TEST_SDA_PIN PA08
#define TEST_SCL_PIN PA09
I2C i2c(TEST_SDA_PIN, TEST_SCL_PIN);
#else
I2C i2c(p28, p27);
#endif

#define I2C_MICROSECONDS_PER_BIT (1000000 / 100000)

void isrHandler(void);

int main() {
    Timeout timeout;
    bool success = true;
    char counter = 0;
    char reg = 0xA5;
    char expectedCounter = 0x5A;
    char writeCommand[] = { reg, expectedCounter };
    char readCommand[] = { reg };

    // Initialize counter.
    i2c.write(ADDR, writeCommand, sizeof(writeCommand), false);

    for (int i = 0 ; i < 1000000 ; i++) {
        // Schedule a Timeout object to fire
        // ~(1 start bit + 8 address bits + 1 ack bit + 8 reg bits + 1 ack bit) = 2*8 + 3*1 = 16+3 = 19 I2C bit periods.
        // Attempting to fire an interrupt between ack of register byte and repeated start. This used to cause
        // problems since the I2C engine would send the register byte again rather than the repeated start due to a bug
        // in the I2C driver which would clear the SI (interrupt) bit too early in i2c_start().
        int triggerOffset = rand() % (8 * I2C_MICROSECONDS_PER_BIT);
        int triggerTime = (15 * I2C_MICROSECONDS_PER_BIT) + triggerOffset;
        timeout.attach_us(isrHandler, triggerTime);

        // Read out counter where a repeated start is used for the read after the write.
        i2c.write(ADDR, readCommand, sizeof(readCommand), true);
        i2c.read(ADDR, &counter, sizeof(counter));
        if (counter != expectedCounter) {
            success = false;
            break;
        }
        expectedCounter++;
    }

    notify_completion(success);
}

void isrHandler(void)
{
    // Waste 2 I2C bytes worth of time in this ISR handler.
    wait_us(16 * I2C_MICROSECONDS_PER_BIT);
}

Slave test code which is a modified version of what I found in https://github.com/mbedmicro/mbed/blob/master/libraries/tests/mbed/i2c_slave/main.cpp I modified it to support a register read/write protocol similar to MPU-6059 sensor. A write to the register sets an 8-bit counter and a read returns an auto-incremented version of the counter.

#include "mbed.h"

#define ADDR (0x90)

#if defined(TARGET_KL25Z)
I2CSlave slave(PTE0, PTE1);
#elif defined(TARGET_LPC4088)
I2CSlave slave(p9, p10);
#elif defined(TARGET_SAMR21G18A)
I2CSlave slave(PA16, PA17);
#elif defined(TARGET_SAMD21J18A)
I2CSlave slave(PA08, PA09);
#else
I2CSlave slave(p28, p27);
#endif

int main() {
    char command[2];
    char counter = 0;

    slave.address(ADDR);

    while (1) {
        // Read register address and its new value if a write register request.
        bool wasWriteRequest = false;
        int i2cState = slave.receive();
        switch (i2cState) {
        case I2CSlave::WriteAddressed:
            memset(command, 0xFF, sizeof(command));
            wasWriteRequest = !slave.read(command, sizeof(command));
            break;
        default:
            continue;
        }

        // Handle the register write now if both register address and register value were written at once.
        if (wasWriteRequest) {
            counter = command[1];
            continue;
        }

        // Now waiting for repeated start to begin register read.
        bool registerReadDone = false;
        while (!registerReadDone) {
            i2cState = slave.receive();
            switch (i2cState) {
            case I2CSlave::ReadAddressed:
                slave.write(&counter, sizeof(counter));
                counter++;
                registerReadDone = true;
                break;
            case I2CSlave::WriteAddressed:
                registerReadDone = true;
                break;
            case I2CSlave::WriteGeneral:
                registerReadDone = true;
                break;
            }
        }
    }
}

I couldn't get the slave code to work without a small modification to the i2c_slave_read() function as well. I am not that confident in this change so I just list the diff here for anyone trying to run this repro case.

diff --git a/external/mbed/libraries/mbed/targets/hal/TARGET_NXP/TARGET_LPC176X/i2c_api.c b/external/mbed/libraries/mbed
index c7eea7c..68617d8 100644
--- a/external/mbed/libraries/mbed/targets/hal/TARGET_NXP/TARGET_LPC176X/i2c_api.c
+++ b/external/mbed/libraries/mbed/targets/hal/TARGET_NXP/TARGET_LPC176X/i2c_api.c
@@ -346,8 +346,8 @@ int i2c_slave_read(i2c_t *obj, char *data, int length) {
         status = i2c_status(obj);
         if((status == 0x80) || (status == 0x90)) {
             data[count] = I2C_DAT(obj) & 0xFF;
+            count++;
         }
-        count++;
     } while (((status == 0x80) || (status == 0x90) ||
             (status == 0x060) || (status == 0x70)) && (count < length));

[0xc0170]

cc @toyowata @neilt6 (you guys did some i2c for NXP), please have a look

[neilt6]

@0xc0170 I don't recall contributing to the NXP I2C implementations so I can't help you there, but I believe I've encountered the bug @adamgreen is referring to.

[toyowata]

@0xc0170 I will have a look.

[toyowata]

I only tested this with LPC1768 target and the following test cases are failed.

• Test case failed:

[ 19] MBED_A19: I2C EEPROM read/write test

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C EEPROM read write test}}
{{test_id;MBED_A19}}
{{start}}
I2C: I2C Frequency: 400000 Hz
I2C: Write 0x66 at address 0x0000 test ...
I2C: Read data from address 0x0000 test ...
Test 0 failed at read, i2c_stat is 0x01
Test 1 failed at read, i2c_stat is 0x01
Test 2 failed at read, i2c_stat is 0x01
Test 3 failed at read, i2c_stat is 0x01
Test 4 failed at read, i2c_stat is 0x01
Test 5 failed at read, i2c_stat is 0x01
Test 6 failed at read, i2c_stat is 0x01
Test 7 failed at read, i2c_stat is 0x01
(snip)

[ 25] MBED_A25: I2C EEPROM line read/write test

{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C EEPROM line read write test}}
{{test_id;MBED_A25}}
{{start}}
I2C: I2C Frequency: 400000 Hz
I2C: Lines pattern write test ... [OK]
I2C: Write errors: 0 ... [OK]
I2C: Lines pattern read test ... [FAIL]
I2C: Read errors: 1000/1000 ... [FAIL]
EEPROM: Pattern match errors: 1000/1000 ... [FAIL]
{{failure}}
{{end}}

• Test case not failed, but I didn't get expected value (x, y, z axis data tied to zero)

[ 13] MBED_A13: I2C MMA7660 accelerometer

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C MMA7660 accelerometer}}
{{test_id;MBED_A13}}
{{start}}
x: 0.000000, y: 0.000000, z: 0.000000
x: 0.000000, y: 0.000000, z: 0.000000
x: 0.000000, y: 0.000000, z: 0.000000
x: 0.000000, y: 0.000000, z: 0.000000
x: 0.000000, y: 0.000000, z: 0.000000
{{success}}
{{end}}

Does this PR require to change whole existing application/test code?

[adamgreen]

Thanks for looking at it. It should have required no changes to existing code.

It looks like the repeated start is failing for those test cases...the thing I was actually trying to fix by removing a timing hole that would sometimes cause it to fail. It must be a bug introduced by my change since the only thing I could think of that would cause that problem if not my code is that the wrong I2C device was connected to the p28/p27 pins. If there were missing pull-ups or no I2C device there at all, different errors would be generated.

I guess I will have to order an I2C EEPROM to repro locally so that I can see what is going on under the debugger. It's strange that I would have broken repeated start for the EEPROM and accelerometer as I did test the change with the MPU-6050 IMU and it worked on that device.

[toyowata]

@adamgreen

I didn't fully apply your patch in my test last time. I picked some of them, but not all. Sorry my bad.
I am going to test again next week.

[adamgreen]

@toyowata As long as you pulled the whole change for hal/targets/hal/TARGET_NXP/TARGET_LPC176X/i2c_api.c from this PR then that should have been enough to test on the LPC1768. Just pulling part of the changes to that file would likely cause problems.

I have ordered the EEPROM so that I can reproduce the problem here locally. I suspect it will arrive by the end of the week.

[toyowata]

@adamgreen

Last time, I only picked a part of code by manually and it was a problem. Sorry my bad.
I applied whole patch of this PR into my local repo and tested again.

• LPC1768

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C MMA7660 accelerometer}}
{{test_id;MBED_A13}}
{{start}}
x: 0.046882, y: 0.000000, z: 1.031411
x: 0.093765, y: 0.000000, z: 0.937647
x: 0.093765, y: 0.046882, z: 0.984529
x: 0.046882, y: 0.000000, z: 0.984529
x: 0.046882, y: 0.046882, z: 0.984529
{{success}}
{{end}}

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C EEPROM read write test}}
{{test_id;MBED_A19}}
{{start}}
I2C: I2C Frequency: 400000 Hz
I2C: Write 0x66 at address 0x0000 test ...
I2C: Read data from address 0x0000 test ...
EEPROM: Test result ... [OK]
{{success}}
{{end}}

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C EEPROM line read write test}}
{{test_id;MBED_A25}}
{{start}}
I2C: I2C Frequency: 400000 Hz
I2C: Lines pattern write test ... [OK]
I2C: Write errors: 0 ... [OK]
I2C: Lines pattern read test ... [OK]
I2C: Read errors: 0/1000 ... [OK]
EEPROM: Pattern match errors: 0/1000 ... [OK]
{{success}}
{{end}}

• LPC11U68

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C MMA7660 accelerometer}}
{{test_id;MBED_A13}}
{{start}}
x: 0.046882, y: -0.703235, z: 0.750117
x: 0.093765, y: -0.703235, z: 0.797000
x: 0.046882, y: -0.656353, z: 0.843882
x: 0.046882, y: -0.703235, z: 0.750117
x: 0.000000, y: -0.703235, z: 0.797000
{{success}}
{{end}}

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C EEPROM read write test}}
{{test_id;MBED_A19}}
{{start}}
I2C: I2C Frequency: 400000 Hz
I2C: Write 0x66 at address 0x0000 test ...
I2C: Read data from address 0x0000 test ...
EEPROM: Test result ... [OK]
{{success}}
{{end}}

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C EEPROM line read write test}}
{{test_id;MBED_A25}}
{{start}}
I2C: I2C Frequency: 400000 Hz
I2C: Lines pattern write test ... [OK]
I2C: Write errors: 0 ... [OK]
I2C: Lines pattern read test ... [OK]
I2C: Read errors: 0/1000 ... [OK]
EEPROM: Pattern match errors: 0/1000 ... [OK]
{{success}}
{{end}}

• LPC1114FN28

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C MMA7660 accelerometer}}
{{test_id;MBED_A13}}
{{start}}
x: 0.046882, y: 0.000000, z: 0.984529
x: -0.187529, y: -0.093765, z: 1.031411
x: -0.234412, y: 0.000000, z: 0.984529
x: -0.281294, y: 0.046882, z: 0.937647
x: -0.375059, y: -0.046882, z: 0.937647
{{success}}
{{end}}

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C EEPROM read write test}}
{{test_id;MBED_A19}}
{{start}}
I2C: I2C Frequency: 400000 Hz
I2C: Write 0x66 at address 0x0000 test ...
I2C: Read data from address 0x0000 test ...
EEPROM: Test result ... [OK]
{{success}}
{{end}}

{{timeout;15}}
{{host_test_name;default_auto}}
{{description;I2C EEPROM line read write test}}
{{test_id;MBED_A25}}
{{start}}
I2C: I2C Frequency: 400000 Hz
I2C: Lines pattern write test ... [OK]
I2C: Write errors: 0 ... [OK]
I2C: Lines pattern read test ... [OK]
I2C: Read errors: 0/1000 ... [OK]
EEPROM: Pattern match errors: 0/1000 ... [OK]
{{success}}
{{end}}

I could not find any problem of this PR.

BTW, there are some missing targets such as LPC812, LPC824 and LPC1549. Didn't find any issue on these platforms?

[adamgreen]

@toyowata Thanks for rerunning the tests. I appreciate it.

BTW, there are some missing targets such as LPC812, LPC824 and LPC1549. Didn't find any issue on these platforms?

I haven't read the user manuals for those parts but from what I see in the code, their I2C peripherals are different and their drivers don't share code or this particular bug.

[toyowata]

@adamgreen

I haven't read the user manuals for those parts but from what I see in the code, their I2C peripherals are different and their drivers don't share code or this particular bug.

OK. Thank you very much for your answer.

[adamgreen]

@toyowata Thanks for testing.
@0xc0170 Thanks for merging.