Skip to content
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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gen2] hal: Fixes I2C bus missing stretch configuration #1829

Merged
merged 2 commits into from Aug 2, 2019

Conversation

@YutingYou
Copy link
Contributor

commented Jun 17, 2019

Problem

This PR is a proposed solution for issue #1809 , we found when Gen2 device acts as an I2C slave device, it will send an extra address byte before its data bytes. This is because the stretch configuration is disabled after a transmission.

    /* Check on EV3 */
    case I2C_EVENT_SLAVE_BYTE_TRANSMITTING:
    case I2C_EVENT_SLAVE_BYTE_TRANSMITTED:
        if (i2cMap[i2c]->txBufferIndex < i2cMap[i2c]->txBufferLength)
        {
            I2C_SendData(i2cMap[i2c]->I2C_Peripheral, i2cMap[i2c]->txBuffer[i2cMap[i2c]->txBufferIndex++]);
        }
        else
        {
            // If no data is loaded into DR register and clock stretching is enabled,
            // the device will continue pulling SCL low. To avoid that, disable clock stretching
            // when the tx buffer is exhausted to release SCL.
            I2C_StretchClockCmd(i2cMap[i2c]->I2C_Peripheral, DISABLE); 馃憟馃憟馃憟
        }
        break;

Even though the configuration is restored in I2C slave event handler when detecting the STOP signal. However, for some user scenario, they will continue to occupy the I2C bus without sending a STOP signal, e.g. our example app. In this case, the restore operation is missed.

    /* EV4 */
    if (sr1 & I2C_EVENT_SLAVE_STOP_DETECTED)
    {
        /* software sequence to clear STOPF */
        I2C_GetFlagStatus(i2cMap[i2c]->I2C_Peripheral, I2C_FLAG_STOPF);
        // Restore clock stretching settings 馃憟馃憟馃憟
        // This will also clear EV4
        I2C_StretchClockCmd(i2cMap[i2c]->I2C_Peripheral, i2cMap[i2c]->clkStretchingEnabled ? ENABLE : DISABLE);   
...

Can we restore stretch configuration once entering the slave event handler?
No, it's too late to enable the configuration in the event callback.

Solution

I don't know the background of this workaround, I propose to release the I2C bus for a very short time to wait for I2C master capturing it, and then restore stretch configuration.

// If no data is loaded into DR register and clock stretching is enabled,
// the device will continue pulling SCL low. To avoid that, disable clock stretching
// when the tx buffer is exhausted to release SCL.

Steps to Test

  1. Flash example app on Gen3 device, set MASTER to 1, use it as an I2C master.
  2. Flash example app on Gen2 device, set MASTER to 0 use it as an I2C slave.
  3. Type 1 to request 6 bytes from I2C slave via Serial on I2C master device.

Example App

#include "application.h"

#define MASTER 0
#define speed 100000
#define SLAVE_ADDRESS 0x18

SYSTEM_MODE(MANUAL);

#if MASTER
int sendData(String cmd) {
   for (int i = 0; i < 1; ++i) {
        uint8_t count = Wire.requestFrom(SLAVE_ADDRESS, 6);
        // delay(10); // doesn't change anything
        while(Wire.available()) {
            char c = Wire.read();
            Serial.print(c);
        }
        Serial.println();
    }
    return 0;
}

void setup() {
    Serial.begin(115200);
    // Particle.function("sendData", sendData);
    //Wire.setSpeed(speed); // optional but same result
    Wire.begin();
}

void loop() {
    if (Serial.available()) {
        uint8_t cmd = Serial.read();
        switch(cmd) {
            case '1': {
                Serial.print("send hello");
                sendData("hello");
                break;
            }
            default:
                break;
        }
    }
}

#else 
void requestEvent() {
    Wire.print("456789");
}

void setup() {  
    //Wire.setSpeed(speed); // optional but same result
    Wire.begin(SLAVE_ADDRESS);    
    Wire.onRequest(requestEvent);
}
#endif

References

[CH34361]


Completeness

  • [bugfix] [Gen 2] Fixes an issue with clock stretching in I2C slave mode with underrun reads with certain I2C masters (e.g. Gen 3 devices)

@YutingYou YutingYou requested a review from avtolstoy Jun 17, 2019

@YutingYou YutingYou force-pushed the fix/gen2-i2c-stretch-missing branch from cbd81f4 to 49ceadb Jun 17, 2019

@@ -1026,6 +1026,8 @@ static void HAL_I2C_EV_InterruptHandler(HAL_I2C_Interface i2c)
// the device will continue pulling SCL low. To avoid that, disable clock stretching
// when the tx buffer is exhausted to release SCL.
I2C_StretchClockCmd(i2cMap[i2c]->I2C_Peripheral, DISABLE);
HAL_Delay_Microseconds(1);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Jun 17, 2019

Member

We shouldn't delay in an ISR. Would a simple __DMB() or __DSB() + __ISB() work here?

This comment has been minimized.

Copy link
@YutingYou

YutingYou Jun 17, 2019

Author Contributor

Updated.

@YutingYou YutingYou force-pushed the fix/gen2-i2c-stretch-missing branch from 49ceadb to 1b3332e Jun 17, 2019

@avtolstoy avtolstoy added this to the 1.3.1-rc.1 milestone Jul 26, 2019

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Closes #1809

@avtolstoy avtolstoy force-pushed the fix/gen2-i2c-stretch-missing branch from 1b3332e to 005ec96 Aug 2, 2019

@avtolstoy avtolstoy merged commit 71b189e into develop Aug 2, 2019

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@avtolstoy avtolstoy deleted the fix/gen2-i2c-stretch-missing branch Aug 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.