jrowberg / i2cdevlib

I2C device library collection for AVR/Arduino or other C++-based MCUs
http://www.i2cdevlib.com
3.92k stars 7.52k forks source link

I2Cdev library reports wrong data from AD7746 chip. Using same library on ESP32 #770

Open dzalf opened 1 month ago

dzalf commented 1 month ago

Hi. Thank you for the great effort put into developing this set of libraries.

I have encountered a serious issue with the I2Cdev.h library where the values reported from the registers of an AD7746 chip are wrong.

I have implemented a read_register method, similar to the already existing write_register method from the AD7746 library.

uint8_t AD7746::read_register(uint8_t addr)
{
     uint8_t data;

     I2Cdev::readByte(_devAddr, addr, &data);

    return data;
}

where data holds the 8-bit value from the requested register.

The issue is that the retrieved data is wrong. If I change the implementation to a slightly more barebones approach, I get the right values:


uint8_t AD7746::read_register(uint8_t addr)
{
    _i2c->beginTransmission(_devAddr);

    _i2c->write(addr);

    _i2c->endTransmission(false);

    _i2c->requestFrom(_devAddr, (uint8_t)1);

    return _i2c->read();
}

Please note that I made some additional modifications to the AD7746 library, where I pass a pointer to my Wire port (since the ESP32 board has multiple I2Cs).

Here is a result of the mistaken data and the correct one. Here I merged both methods into one to retrieve the Chip ID from my AD7746, however, the implementation using I2Cdev fails every time reporting either 0x02 or 0x07 every time.

image

Pay attention to the correct Chip ID 0x51 retrieved by my implementation (confirmed through the AD7746 Eval Board GUI App) while the one using I2Clib.h reports 0x07. In this case, I made a code that reads all registers from 0x00 to 0x13 hence why there are so many lines.

Similarly, retrieving the capacitance reports completely wrong values. Since the original method getCapacitance uses the I2Clib.h header, I decided to make some modifications and work on my implementation that avoids using it.

uint32_t AD7746::getRawCapacitance()
{
    uint32_t capacitance = 0;

    uint8_t _cap_buffer[3] = {0, 0, 0};

    // I2Cdev::readBytes(_devAddr, AD7746_RA_CAP_DATA_H, 3, _cap_buffer); // --> this fails every time

    // Using my implementation
    _cap_buffer[0] = this->read_register(AD7746_RA_CAP_DATA_H);
    _cap_buffer[1] = this->read_register(AD7746_RA_CAP_DATA_H + 1);
    _cap_buffer[2] = this->read_register(AD7746_RA_CAP_DATA_H + 2);

    capacitance = ((uint32_t)_cap_buffer[0] << 16) | ((uint32_t)_cap_buffer[1] << 8) | (uint32_t)_cap_buffer[2];

    if (_debug)
    {
        sprintf(_printBuffer, "C [hex] = 0x%X\n", capacitance);
        _debugPort->print(_printBuffer);
    }

    return capacitance;
}

Once again, I get the right values by NOT using the library but rather my proposed solution

image
jrowberg commented 1 month ago

The AD7746 library was contributed by another user, but after looking into this, I believe I know at least part of the problem. In your test code, you are reading three bytes starting from the 2nd register address (0x01). However, in the I2Cdev library's readBytes method, it does not use the repeated start flag after ending the transmission that sends the register address. So this call:

I2Cdev::readBytes(_devAddr, AD7746_RA_CAP_DATA_H, 3, _cap_buffer);

Actually reads registers 0, 1, and 2 instead of reading registers 1, 2, and 3. This is a quirk of the AD7764 specifically, which says on page 12 about write operations:

A stop condition is defined by a low-to-high transition on SDA while SCL remains high. If a stop condition is ever encountered by the AD7745/AD7746, it returns to its idle condition and the address pointer is reset to Address 0x00.

...and about read operations:

When a read is selected in the start byte, the register that is currently addressed by the address pointer is transmitted on to the SDA line by the AD7745/AD7746. This is then clocked out by the master device and the AD7745/AD7746 awaits an acknowledge from the master.

If an acknowledge is received from the master, the address autoincrementer automatically increments the address pointer register and outputs the next addressed register content on to the SDA line for transmission to the master. If no acknowledge is received, the AD7745/AD7746 return to the idle state and the address pointer is not incremented.

So if you changed the line to this:

I2Cdev::readBytes(_devAddr, AD7746_RA_STATUS, 4, _cap_buffer);

...you might get good data. This problem will occur using the existing AD7764 library code for any reads that do not begin at register 0, because the I2Cdev doesn't actually use the repeated start condition as the device requires. This could be fixed by exposing a mechanism to use repeated start via a new argument to the readBytes method.

dzalf commented 3 weeks ago

Thanks for your prompt answer.

Indeed, the repeated start is missing.

This issue has been reported before.

I believe I will stick to a more barebones approach with regards to the Wire library (for the time being at least)