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(PeriphDrivers): Fix I2C DMA Issues When Slave Does Not ACK Address, Fix 1-length I2C DMA Transactions #1109

Merged
merged 3 commits into from
Sep 6, 2024

Conversation

Jake-Carter
Copy link
Contributor

@Jake-Carter Jake-Carter commented Aug 9, 2024

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 Jake-Carter marked this pull request as ready for review August 28, 2024 23:34
@github-actions github-actions bot added the MAX32665 Related to the MAX32665 (ME14) label Aug 28, 2024
Copy link
Contributor

@sihyung-maxim sihyung-maxim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed the review notification last week.

Comment on lines +456 to +468
/**
* @brief Interrupt handler function for DMA0
* @details Used by some internal drivers that automatically assign DMA handlers.
* This is equivalent to "MXC_DMA_Handler(MXC_DMA0);"
*/
void MXC_DMA_DMA0_Handler(void);

/**
* @brief Interrupt handler function for DMA0
* @details Used by some internal drivers that automatically assign DMA handlers.
* This is equivalent to "MXC_DMA_Handler(MXC_DMA1);"
*/
void MXC_DMA_DMA1_Handler(void);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside of the precedent set when the DMA drivers were first set up... :(

@Jake-Carter
Copy link
Contributor Author

Sorry, I missed the review notification last week.

@sihyung-maxim No worries, thanks for all your reviews. I'm spamming you recently :)

@Jake-Carter Jake-Carter merged commit 4e41038 into analogdevicesinc:main Sep 6, 2024
11 checks passed
@Jake-Carter Jake-Carter deleted the fix/gh-987 branch September 6, 2024 02:18
sihyung-maxim pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Sep 6, 2024
ozersa pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Sep 11, 2024
ozersa pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MAX32665 Related to the MAX32665 (ME14)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I2C DMA on MAX32666 does not work
2 participants