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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/gen3 i2c timeout #1875

Merged
merged 3 commits into from Aug 5, 2019

Conversation

@YutingYou
Copy link
Contributor

commented Aug 3, 2019

Problem

The I2C driver on Gen3 device doesn't support timeout, once the external I2C slave device dies, the I2C bus will be held, Device OS gets stuck in the endless wait, the user application doesn't run anymore.

Solution

Add timeout to the driver.

This PR also fixes an assertion failure on rand() usage from an ISR in i2c_master_slave/i2c_slave test.

Steps to Test

Unit Test

  1. Make sure this fix can pass the unit test under user/tests/wiring/i2c_master_slave

Test App

  1. Download below application to Xenon-I2C-Master and Xenon-I2C-Slave devices
  2. Connect Xenon-I2C-Master's SCL pin to the GND
  3. Connect Xenon-I2C-Master's SCL pin back to the Xenon-I2C-Slave's SCL pin
  4. Observe the log, the user application of I2C master should not be blocked and the data should be correct.

Example App

#include "application.h"

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

SYSTEM_MODE(MANUAL);

Serial1LogHandler logHandler(115200, LOG_LEVEL_ALL, {{"app", LOG_LEVEL_ALL}});

#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);
    Wire.begin();
}

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

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

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

References

[CH36348]

Completeness

  • [enhancement] [Gen3] Adds timeouts to I2C HAL operations

@YutingYou YutingYou requested a review from avtolstoy Aug 3, 2019

@@ -214,7 +237,7 @@ static int twi_init(HAL_I2C_Interface i2c) {

void HAL_I2C_Init(HAL_I2C_Interface i2c, void* reserved) {
if (m_i2c_map[i2c].enabled) {
twi_uinit(i2c);
twi_uninit(i2c);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 5, 2019

Member

As discussed, let's not call uninit here to avoid issues for the I2C peripheral shared between system and user code (the one that has PMIC and FuelGauge).

Going further down let's change:

    os_thread_scheduling(false, NULL);
    if (m_i2c_map[i2c].mutex == NULL) {
        os_mutex_recursive_create(&m_i2c_map[i2c].mutex);
    } else {
        // Already initialized
        return;
    }
    HAL_I2C_Acquire(i2c, NULL);
    os_thread_scheduling(true, NULL);

This comment has been minimized.

Copy link
@YutingYou

YutingYou Aug 5, 2019

Author Contributor

Should we re-enable schedule here?

    os_thread_scheduling(false, NULL);
    if (m_i2c_map[i2c].mutex == NULL) {
        os_mutex_recursive_create(&m_i2c_map[i2c].mutex);
    } else {
        // Already initialized
        os_thread_scheduling(true, NULL);
        return;
    }
    HAL_I2C_Acquire(i2c, NULL);
    os_thread_scheduling(true, NULL);

This comment has been minimized.

Copy link
@YutingYou

YutingYou Aug 5, 2019

Author Contributor

Should we re-enable schedule here?

    os_thread_scheduling(false, NULL);
    if (m_i2c_map[i2c].mutex == NULL) {
        os_mutex_recursive_create(&m_i2c_map[i2c].mutex);
    } else {
        // Already initialized
        os_thread_scheduling(true, NULL);
        return;
    }
    HAL_I2C_Acquire(i2c, NULL);
    os_thread_scheduling(true, NULL);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 5, 2019

Member

Yes, we should 👍

@avtolstoy avtolstoy force-pushed the fix/gen3-i2c-timeout branch from 0b62277 to c6a7794 Aug 5, 2019

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@YutingYou Please rebase on develop as well.

@YutingYou YutingYou force-pushed the fix/gen3-i2c-timeout branch from 3b447c8 to 96ad7fc Aug 5, 2019

@avtolstoy avtolstoy merged commit 7309e24 into develop Aug 5, 2019

2 checks passed

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

@avtolstoy avtolstoy deleted the fix/gen3-i2c-timeout branch Aug 5, 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’t perform that action at this time.