msdk icon indicating copy to clipboard operation
msdk copied to clipboard

fix(PeriphDrivers): Fix I2C DMA Issues When Slave Does Not ACK Address, Fix 1-length I2C DMA Transactions

Open Jake-Carter opened this issue 6 months ago • 0 comments

Description

This PR fixes various I2C DMA issues that are caused by slaves that intentionally NACK their I2C address, such as the MS8607.

Fixes #987

Ex:

image

Previously, MXC_I2C_MasterTransactionDMA would send the read request and then immediately start reading from the I2C RX FIFO with DMA. If the slave NACKs that address, no further bytes will be sent. Therefore the DMA request will never complete and DMA callbacks will never be hit, all the while the master locks up and holds the SCL line low.

Now, we wait to see if we got an ACK or a NACK immediately after we send the read request. If we get a NACK, we abort the transaction, issue a STOP signal, and pass the communication error along to the user's callback and the return code. This solves the master "lockup" and allows the user application to decide what to do from here, such as retry the transaction.

It also applies a logical fix that corrects issues with TX/RX transactions with a length of 1.

Tested with an MS8607 Adafruit module and MAX78002EVKIT

Test Code
/******************************************************************************
 *
 * Copyright (C) 2024 Analog Devices, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 *
 ******************************************************************************/

/**
 * @file    main.c
 * @brief   Hello World!
 * @details This example uses the UART to print to a terminal and flashes an LED.
 */

/***** Includes *****/
#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>
#include <math.h>
#include "mxc_device.h"
#include "led.h"
#include "board.h"
#include "mxc_delay.h"
#include "i2c.h"

/***** Definitions *****/
#define ADDR_PT 0x76
#define ADDR_RH 0x40
#define MS8607_CMD_RESET_RH 0b11111110
#define MS8607_CMD_MEASURE_RH_HOLD 0b11100101
#define MS8607_CMD_MEASURE_RH_NOHOLD 0b11110101

static volatile bool g_dma_complete = false;
static volatile int g_dma_status = E_NO_ERROR;

#define I2C_INSTANCE MXC_I2C0

void DMA_Callback(mxc_i2c_req_t *req, int result)
{
    g_dma_complete = true;
    g_dma_status = result;
}

/***** Globals *****/
static mxc_i2c_req_t g_req = {
        .i2c = I2C_INSTANCE,
        .addr = ADDR_RH,
        .rx_buf = NULL,
        .rx_len = 0,
        .tx_buf = NULL,
        .tx_len = 0,
        .callback = DMA_Callback
    };

/***** Functions *****/
// Utility function for resetting flags and launching the I2C DMA transaction
static inline int launch_DMA_Transaction(void)
{
    g_dma_complete = false;
    g_dma_status = E_NO_ERROR;
    int err = MXC_I2C_MasterTransactionDMA(&g_req);
    return err;
}

int MS8607_Reset(void)
{
    uint8_t tx_buf = MS8607_CMD_RESET_RH;
    g_req.addr = ADDR_RH;
    g_req.tx_buf = &tx_buf;
    g_req.tx_len = 1;
    int err = launch_DMA_Transaction();
    MXC_Delay(MXC_DELAY_MSEC(15));
    return err;
}

int MS8607_MeasureRH_NoHold(double *out_RH)
{
    uint8_t tx_buf = MS8607_CMD_MEASURE_RH_NOHOLD;
    uint8_t rx_buf[3] = { 0, 0, 0 };
    
    // Send command
    g_req.addr = ADDR_RH;
    g_req.tx_buf = &tx_buf;
    g_req.rx_buf = NULL;
    g_req.tx_len = 1;
    g_req.rx_len = 0;
    int err = launch_DMA_Transaction();
    while(!g_dma_complete) {} // Wait for command to be sent

    do {
        MXC_Delay(MXC_DELAY_MSEC(2));
        // ^ RH measurement seems to take about 15ms.
        // If we constantly spam the MS8607 with polling reads then
        // the measurement will never finish.  It needs some space to
        // perform the calculations, so we delay ~2ms between each poll.
        g_req.tx_len = 0;
        g_req.rx_len = 3;
        g_req.rx_buf = rx_buf;
        err = launch_DMA_Transaction();
        while(!g_dma_complete) {} // Wait for read to complete.
    } while (g_dma_status != E_NO_ERROR && err != E_NO_ERROR);
    // ^ The MS8607 will NACK our read request if the measurement is not ready.
    // In that case, the callback function will receive E_COMM_ERR and MXC_I2C_MasterTransactionDMA 
    // will return E_COMM_ERR.  We want to keep re-trying until the sensors ACKs our read request.
    
    uint16_t D3 = rx_buf[0] << 8 | (rx_buf[1] & 0b11111100);
    int16_t RH = -600 + 12500 * (D3 / (pow(2, 16)));
    *out_RH = RH / 100.0f;
    return err;
}

// *****************************************************************************
int main(void)
{
    MXC_I2C_Init(I2C_INSTANCE, true, ADDR_RH);
    MXC_I2C_SetFrequency(I2C_INSTANCE, 400000);
    MXC_I2C_DMA_Init(I2C_INSTANCE, MXC_DMA, true, true);

    int err = MS8607_Reset();
    if (err) {
        printf("Failed to reset MS8607, error %i\n", err);
        return err;
    }

    double RH = 0;
    err = MS8607_MeasureRH_NoHold(&RH);
    if (err) {
        printf("RH Measurement error %i\n", err);
    } else {
        printf("RH Success, result: %.2f%%\n", RH);
    }
}

Complete analyzer trace: gh-987-fixed.zip

image

Master issues the TX Command...

image

... Polls the device with an RX command that is NACK'd...

image

... Continues polling...

image

... Until an ACK is received. DMA unloads the data bytes successfully...

image

Checklist Before Requesting Review

  • [ ] PR Title follows correct guidelines.
  • [ ] Description of changes and all other relevant information.
  • [ ] (Optional) Link any related GitHub issues using a keyword
  • [ ] (Optional) Provide info on any relevant functional testing/validation. For API changes or significant features, this is not optional.

Jake-Carter avatar Aug 09 '24 22:08 Jake-Carter