I2C DMA on MAX32666 does not work

[sullivanmj]

I am trying to get I2C working with DMA on a MAX32666 and finding that it does not work. I set up my code based on the example at https://github.com/analogdevicesinc/msdk/tree/main/Examples/MAX32665/I2C

I need to be able to do the following types of transactions - none of which currently work:

1. Write a register address, followed by reading one or more bytes
2. Write a register address, followed by writing one or more bytes
3. Read one or more bytes

I made some changes in a i2c_reva.c that have 1 and 2 working, but I am unable to complete a transaction in the style of item 3 above.
https://github.com/sullivanmj/msdk/tree/fix(I2C-DMA)

I also was not able to get any of the above working with high speed I2C at ~3.2 MHz. This isn't a big pain point, but it would be nice to have working too.

[EricB-ADI]

Hi @sullivanmj, Can you attach your code which you are trying to do 1-3? I am gonna tag @Jake-Carter and @sihyung-maxim. They may be able to help if I cannot.

[sullivanmj]

Hi @EricB-ADI, this gist has an example that shows what I am trying to do. Everything works fine until we get to line 154. After making that call, the ISR/callback is never executed, and executeDmaTransaction spins forever, waiting for i2cResult to be set.

[Jake-Carter]

Hi @sullivanmj, I think the main problems are related to making sure the correct IRQ is enabled.

I noticed this loop starts at DMA8, but you are using DMA1. So I suspect the DMA1 IRQ is not enabled, and the vector is not assigned.

MXC_DMA_Init(MXC_DMA1);
for (IRQn_Type i = DMA8_IRQn; i <= DMA15_IRQn; i++)
{
    MXC_NVIC_SetVector(i, DmaPoolIrqHandler);
    NVIC_SetPriority(i, 6);
    NVIC_EnableIRQ(i);
}

Our async and DMA I2C callback functions are reliant on MXC_I2C_AsyncHandler/MXC_DMA_Handler being called from the correct ISR, so if the NVIC routing is not enabled properly then the callbacks will not work.

Instead I would suggest explicitly enabling and routing the exact IRQ for the instance you're using.

NVIC_EnableIRQ(MXC_DMA_CH_GET_IRQ(MXC_DMA_GET_IDX(MXC_DMA1)));
MXC_NVIC_SetVector(MXC_DMA_CH_GET_IRQ(MXC_DMA_GET_IDX(MXC_DMA1)), DmaPoolIrqHandler);
// ...

[sullivanmj]

Hi @Jake-Carter,

Thanks for taking a look at this. I am able to hit the DMA ISR and see the callbacks executing. My example code gets successfully through everything, until it gets to the i2cRead call, where it is stuck inside waiting for i2cResult to be set by the callback. The ISR & callback all appear to be executing correctly for all of the transactions until we get to an I2C transaction that has mxc_i2c_req_t.rx_len of 0, at the very end of my sample program.

The DMA setup is a little confusing, but the intent is to initialize the DMA1 peripheral, which contains DMA channels 8-15, and then enable interrupts on each channel (8-15) in DMA1. I have used this approach in another codebase where we are using DMA channels 8-15 as a pool, with other peripherals, and they all need to call MXC_DMA_Handler in their respective ISRs, so this bit of code works well for that arrangement. In any case, I updated the code in the gist to setup the interrupts on an as-needed basis, depending on whatever channel is returned from MXC_I2C_DMA_GetTXChannel and MXC_I2C_DMA_GetRXChannel, I still don't see any change in the cases that work vs do not work.

I also updated my branch that fixes case 1, to only include the requisite change for that fix.

So, to summarize/clarify, here's where I am at:

1. I am able to do transactions of this format, but only with the change in my branch.
   • Write slave address
   • Write n bytes (mxc_i2c_req_t.tx_len > 0)
2. I am able to do transactions of this format, even without the change in my branch. It is possibly noteworthy that this is the only flavor of DMA-based I2C transaction in the I2C example for MAX32666, where both a read and a write occurs in the same transaction.
   • Write slave address
   • Write 1 byte (the example writes 255, I have not tried writing any more than 1, though)
   • Read n bytes (mxc_i2c_req_t.rx_len > 0)
3. I am not able to do transactions of this format. I have tried a few things in the SDK code (such as adjusting the TX threshold, and doing a similar change as what i have in my branch for the line directly above it), but couldn't get it to work. What I see is that the slave address is shifted out, and then the MAX32666 holds SCL low, indefinitely. No DMA ISR ever triggers.
   • Write slave address
   • Write 0 bytes, this is the distinction between 2 above and this case
   • Read n bytes (mxc_i2c_req_t.rx_len > 0)

[Jake-Carter]

Thanks @sullivanmj, this helps clarify. We'll definitely merge in your fix for case 1.

I'll take a deeper look at case 3. I went down a similar path as you and it's not immediately obvious what is holding up the TX side just looking through the software. I'll run some tests and take a look at what's going on in the hardware

[sullivanmj]

Great, thanks. I'll be happy to help test this one as well, if/when you have a fix.

[Jake-Carter]

@sullivanmj just want to shoot you a quick update on this - it's still the queue. We've been hit with some other really critical issues on a few other parts that we have to look into first, so there has been some delay

[sullivanmj]

Just checking in - is there any ETA on a fix for this?

[Jake-Carter]

@sullivanmj Last week I brought in some additional resources to help and we're simultaneously looking into some other I2C timing issues that seem to be related on the MAX78002. Your fix was relevant to fix some callback setups on that micro as well.

It's being more actively worked on now - no ETA yet just because we're still not exactly sure what the issue is. We should make some progress this week and I'll keep you updated

[Jake-Carter]

@sullivanmj are you communicating with an I2C peripheral, or using a loopback configuration?

[sullivanmj]

It's an I2C peripheral.

[Jake-Carter]

What's the part number for the peripheral?

[sullivanmj]

It's an MS8607-02BA01. The particular transaction sequence that is problematic is illustrated in can be found in Figure 19 of the Datasheet, which illustrates the "I2C Measure RH No Hold Master communication sequence".

[sullivanmj]

@Jake-Carter I hate to be the squeaky wheel, but is there any timeframe on this getting fixed?

[ttmut]

@sullivanmj Not sure if you have already tried but I think in the 3rd case you may have to set .restart to 1 in i2cWriteAddressed.

[Jake-Carter]

@sullivanmj We're starting to take some chunks out of our backlog again. Yours is top of the queue. I've got an Adafruit module on the way which should get here tomorrow. I hope to have a fix for this implemented by the end of the week.

[sullivanmj]

@ttmut thanks for the tip, I will give this a shot.

@Jake-Carter thank you for the update.

[Jake-Carter]

@sullivanmj I've fixed the issue.

The main problem is that our drivers did not deal with NACKs from the slave on the RX side of the DMA I2C transactions. I'm glad I got ahold of an MS8607 to test with - we had been struggling to replicate this artificially. See the PR above for the implementation/test code and some logic analyzer traces.

Here's a version of the test code that will build for the MAX32666. I need to re-order another MAX32666 kit to actually test it, mine seems to have disappeared recently...

Let me know if you can run this/how it works. I think there may still be some updates to make to handle the MAX32666's multiple DMA instances. Note I am also assigning the default DMA handler in the drivers now - may need to implement some mechanism similar to #763 to support custom handlers

Build against my fix/gh-987 branch.

git remote add jake-carter [email protected]:Jake-Carter/msdk.git
git fetch jake-carter
git checkout remotes/jake-carter/fix/gh-987 -b fix/gh-987

/******************************************************************************
 *
 * 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.
 *
 ******************************************************************************/

/***** 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_BUS0

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, MXC_DMA);
    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);
    }
}

[sullivanmj]

@Jake-Carter That's great news - thanks for looking at this. I'll try to test it out early next week.

[sullivanmj]

@Jake-Carter to quickly test this on a MAX32666, I modified the MXC_DMA_Handler always call MXC_DMA_RevA_Handler passing in MXC_DMA0 as the argument.

I was able to run your example app on my custom board, and it works.

To test continuous RH conversions, I added a while loop around the calls to reset & measure RH, with a 1 second delay at the end of the loop, and found that after the first RH conversion, the g_dma_status and err are always both E_COMM_ERR. But moving the reset to happen only once, before the while loop fixes this problem. So I don't really know if it is a problem with the peripheral or with the I2C/DMA driver code.

Works:

int err = MS8607_Reset();
if (err) {
	printf("Failed to reset MS8607, error %i\n", err);
}
	
while (1) {
	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);
	}
	MXC_Delay(MXC_DELAY_SEC(1));
}

Does not work:

while (1) {
	int err = MS8607_Reset();
	if (err) {
		printf("Failed to reset MS8607, error %i\n", 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);
	}
	MXC_Delay(MXC_DELAY_SEC(1));
}

[sullivanmj]

Prodded this a bit more, it was actually a problem with reusing the g_req during MS8607_Reset, after it had been used for the RH conversion. Once I fixed that up, it can run either way in a loop.

[Jake-Carter]

@sullivanmj great, thanks for testing! Good catch on the reset function - I did not reset the RX-related fields.

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;
    g_req.rx_buf = NULL; // <--- this should fix it in the loop
    g_req.rx_len = 0; //    <---
    int err = launch_DMA_Transaction();
    MXC_Delay(MXC_DELAY_MSEC(15));
    return err;
}

Working on a way to implement the handlers then will get this reviewed and merged. Hopefully we've unblocked you in the meantime. Let me know if there's any other issues

[Jake-Carter]

@sullivanmj FYI just went with a basic API for routing things to "hard" DMA handlers for DMA0/DMA1. See 7e8df44

Should have this reviewed and merged into main in the next couple days.