Usage of Wire.requestFrom()

[Koepel]

The files

• ESPEasy/src/_P045_MPU6050.ino
• ESPEasy/lib/HT16K33/HT16K33.cpp
• ESPEasy/src/_P030_BMP280.ino
• ESPEasy/src/_P032_MS5611.ino
• ESPEasy/src/_P006_BMP085.ino
• ESPEasy/src/_P047_i2c-soil-moisture-sensor.ino
• ESPEasy/src/_P028_BME280.ino
• ESPEasy/lib/AS_BH1750/AS_BH1750.cpp
• ESPEasy/lib/AS_BH1750/AS_BH1750A.cpp

have sometimes a extra Wire.beginTransmission() before the Wire.requestFrom() and many times a Wire.endTransmission() after the Wire.requestFrom(). Those can be removed. Explanation: Common-mistakes, number 2 and 3.

[TD-er]

This is great, since I was going to collect all I2C related code spread of all plugins into a single I2C file. And add some new functions like fetching data in a burst. I will look into these files tomorrow, I hope.

[Koepel]

When you use a single file or library then you have to document that well, so everyone knows that it should be added. When you add the same file to every project, you might see me again with a list of the projects where the same mistake in that file is used.

[TD-er]

With ESPeasy people tend to copy existing plugin files and start editing. So if the existing plugin files are well written (and kept organized) it will help people to start using the right functions.

And I totally agree on documentation, although I'm happy to see you again keeping an eye on the code ;)

[TD-er]

I just added a pull request. If someone could have a look at it (and perhaps test it ;) ) then we can merge it tomorrow.

[Koepel]

I have read the changes and I think it is okay. The code is close to the previous code, that is why I have a few notes:

The signed functions have a 'S' in the name, but the unsigned data don't get a 'U' ?

There is no function to write 16 bits ?

You could change the "is_ok" parameter from a pointer to by reference. That makes the code easier to read. Only the "I2C_read8_reg()" has that parameter, the others have not. You could add that parameter to all the "read" functions. Perhaps you can make a separate "does it exist ?" function.

The "I2C_read8_reg()" checks the number of received bytes, but not the error of the Wire.endTransmission(). When the sensor is not at that I2C address, it will be detected, therefor testing the error of Wire.endTransmission() will not improve it. When the I2C bus is very noise, that extra test could help, but a noisy I2C bus will not work anyway.

You have functions to read unsigned data and the functions for signed data are based upon the unsigned functions. The exception is "I2C_read24_reg()". It does not have a 'S' in its name like the others and there is no unsigned version. It does not really return a signed value, because the sign is not extended to the highest byte. In the datasheet of the Bosch BME280, they say that the unsigned 20-bits value can best be stored in a signed 32 bits integer. But that is specific for the BME280. That specific situation should be in the file for the BME280. In the file "P032_MS5611.ino" it is not used as a signed value, but converted into a unsigned long. In my opinion, a common function to read three bytes (unsigned) should return a unsigned variable. That means that return value should be cast to a signed integer in the BME280 file.

[TD-er]

I agree with all your points and will change it (and more). Only reason I pushed it already was to do it in little steps.

[clumsy-stefan]

I tried to adopt my plugins to the new I2C functions, but already onthe first one I ran into an issue (P130_VEML6075 from the playground). Basically I exchanged the followin gpart:

uint16_t Plugin_130_getVEML6075ID(int a)
{
    uint8_t rawData[2] = {0, 0};
    Wire.beginTransmission(a);
    Wire.write(0x0C);        // Command code for reading VEML6075 ID
    Wire.endTransmission(false);  // Send the Tx buffer, but send a restart to keep connection alive

    Wire.requestFrom(a, 2);  // Read two bytes from slave register address 
    uint8_t i = 0;
    while (Wire.available()) 
    {
        rawData[i++] = Wire.read();       // Put read results in the Rx buffer
    }     
    Wire.endTransmission();
    return ((uint16_t) rawData[1] << 8) | rawData[0];
}

with

I2C_read16_LE_reg(a, 0x0c);

however the funciotn always returns 0x00 (instead of the ID of the chip). Also tried to take the I2C_read16_reg() and I2C_read8_reg() functions, but the same happens...

Am I missing something here?

any help appreciated..

[clumsy-stefan]

Ok, I think I jsut found the answer myself (after trying half a day now). It seems that (at least for certain sensors) the Wire.endTransmission() before the Wire.requestFrom() need to have a "false" as argument to keep the connection to the sensor open. Only after having read everything, the Wire.endTransmission() should be called. At least it works like this for the VEML.... eg the read16 should probably look like:

uint16_t I2C_read16_reg(uint8_t i2caddr, byte reg) {
  uint16_t value(0);

  Wire.beginTransmission(i2caddr);
  Wire.write((uint8_t)reg);
  Wire.endTransmission(false);
  Wire.requestFrom(i2caddr, (byte)2);
  value = (Wire.read() << 8) | Wire.read();
  Wire.endTransmission();

  return value;
}

(PS: can make a pull request out of it, if required)

[Koepel]

According to the datasheet, the VEML6075 must use a repeated start. They even write in the datasheet: "A stop condition should not be sent here". Today it is common to set the default to a repeated start. I have recently changed my own library to use the repeated start as default. The function "I2C_read16_reg()" in "I2C.ino" does not use a repeated start.

@TD-er You could change those functions to a repeated start (add parameter 'false' to Wire.endTransmission() when setting the register address which is followed by reading the register data). I assume that every sensor will still work.

@clumsy-stefan Thanks for spotting this problem. Could you remove that second Wire.endTransmission() ? Explanation: Common-mistakes#2

[clumsy-stefan]

@Koepel sure, it also works without it... thanks for the link, interesting reading ;)

BTW: also allother sensors I have on the same node still work with the "false" parameter, so I guess it's not a problem adding it.

[clumsy-stefan]

I have to correct my last comment!! @Koepel : it seems your assumption is correct from a host point of view but not for the sensor... the VEML spec. says that the device ID is 16 Bit. When only reading 8 bit (I2C_read8_reg) the read address is correct, but the sensor won't react to the subsequent config write (always returns false). only if I read the full 16 bit (I2C_read16_reg) or add a wire.endTransmission() to the I2C_read8_reg function at the end, the subsequent I2C_write8_reg works!! It somehow seems that the sensor is waiting to get rid of it's second address byte and only discards it after an endTransmission() or if it's effectively read. which would mean, that you assumption the that beginTransmission "clears" everything would be wrong or that Common-mistake#2 is not true for all sensors... Not sure what's the "clean" solution, but I think adding a wire.endTransmission() to all read functions at the end would be the safe side... what do you think?

[Koepel]

I agree that the sensor seems to wait until that second byte is read or until the I2C communication is reset with a I2C Start and Stop. However, using a bug to fix an other bug can't be right.

A Wire.endTransmission() may not be used after a Wire.requestFrom() according to the official documentation, according to the source code of the Wire library and according to a logic analyzer connected to the I2C bus.

I don't have a VEML6075, but seems very itchy. I suggest to read 16 bits and never read 8 bits. In the datasheet are only examples of reading and writing 16 bits data. So I assume that is how the VEML6075 is internally organized.

If you ever get in the situation that you need to restart the I2C communication, you could do:

Wire.beginTransmission( address);
Wire.endTransmission();

That is not needed in this situation, because you better not read 8 bits of data.

I can turn this around: can you prove with the datasheet that it is allowed to read 8 bits of data ?

[clumsy-stefan]

according to datasheets a number of sensors have 16bit registers and, especially for the address, always only 8bits are read...

but I agree, using the chip according to the spec would be the better solution!

but then we would need to add 16 and 32bit I2Cread and I2Cwrite functions as well to the core, because some sensors (CCS811) use 4byte registers...

I try to add at least the write16 and read32 for test purposes. @TD-er can still decide if he want's to include them in the core!

[Koepel]

Of course, but I was talking now about the itchy VEML6075. It seems to go haywire when reading a single byte.

[clumsy-stefan]

what I'm also wonderign is how we find out if a read was successful or not.. as the I2C_read* functions don't return a status if a result has been received or not... (eg. while reading a LML75A)

[clumsy-stefan]

added a pull request (#1501) for review!

[s0170071]

Nice. However, ... I currently redo my sht25 plugin so that I can run multiple sensors (all with the same address) using different pins. Works fine as the ESP uses bit-banging, not hardware I2C. What I would need is a method for void setI2CClock( speed ); // -1 for default void setPins (SDA, SCL); // -1 for ESPEasy global setting

[TD-er]

@s0170071 If you look closely, to the I2C.ino file I added, you'll see there is some attempt to move to exactly that. The functions to read multiple addresses in one call (when allowed by the buffer size), which I need for the BMx280 sensors, is already using a new lib. This new lib allows multiple instances, although there currently is no way of selecting another instance if you want to.

My idea was to add a map of these, based on the pins used. A bit like I did with the ultrasonic plugin and the BME280 to allow multiple instances of the same plugin use their own data.

[Koepel]

Looking at the current code of the "mega" branch:

This Wire.endTransmission() https://github.com/letscontrolit/ESPEasy/blob/mega/lib/HT16K33/HT16K33.cpp#L136 should be removed.

This Wire.beginTransmission() https://github.com/letscontrolit/ESPEasy/blob/mega/src/_P034_DHT12.ino#L63 should be removed.

This https://github.com/letscontrolit/ESPEasy/blob/mega/lib/I2Cdevlib/I2Cdev.cpp has these issues https://github.com/jrowberg/i2cdevlib/issues/265

This https://github.com/letscontrolit/ESPEasy/blob/mega/lib/AS_BH1750/AS_BH1750.cpp and this https://github.com/letscontrolit/ESPEasy/blob/mega/lib/AS_BH1750/AS_BH1750A.cpp have these issues https://github.com/hexenmeister/AS_BH1750/issues/4

[TD-er]

@Koepel Can you make a new issue for that (and reference to this one) Old (closed) issues are very low on the list of issues to be looked at and this sounds like an easy fix to do.

[Koepel]

Can you re-open this one ? It is the same issue. The original I2Cdev lib will probably never be fixed, but the other ones are easy to fix.

[TD-er]

Changed it for P034. The others are part of external libraries. Will have to upgrade those first and then see if they still have these issues with the Wire calls.

[tonhuisman]

The I2C plugins and relevant code have seen a lot of changes, so this doesn't seem relevant anymore. Issue can be closed.