maarten-pennings / CCS811

Arduino library for the CCS811 gas sensor for monitoring indoor air quality.
MIT License
165 stars 46 forks source link

ccs811: begin: HW_ID read failed #9

Closed Creepwood closed 5 years ago

Creepwood commented 5 years ago

Something went wrong... but i don't know why. If I'm using the Ardafruit CCS811 lib, the sensor is working.

My Code mods CCS811 ccs811(-1, 0x5A); // nWAKE on D3 ccs811.set_i2cdelay(0); // Needed for ESP8266 because it doesn't handle I2C clock stretch correctly

My Hardware

Serial ouput: Starting CCS811 flasher ccs811: begin: HW_ID read failed init: CCS811 begin FAILED init: hardware version: FFFFFFFF init: bootloader version: FFFFFFFF init: application version: FFFFFFFF init: comment-out this code line if you want to flash loop: ended ...

maarten-pennings commented 5 years ago

Hi, I don't see any obvious mistakes.

Did you connect nWAKE to GND (since you pass -1 in the constructor)?

Did you (can you) run the ccs811basic example? What does it say?

Creepwood commented 5 years ago

Hi. Yes, I connected nWAKE to GND.

When I run the ccs811basic sketch I get following output: setup: Starting CCS811 basic demo setup: library version: 8 ccs811: begin: HW_ID read failed setup: CCS811 begin FAILED setup: hardware version: FFFFFFFF setup: bootloader version: FFFFFFFF setup: application version: FFFFFFFF CCS811: I2C error

Now I'm going to try with a LOLIN D32... I will be back

Creepwood commented 5 years ago

Same problem with LOLIN D32 :(

Creepwood commented 5 years ago

Ok...now it works. And I can flash the new firmware :) Very happy!

I changed your function CCS811::i2cread(int reg, int num, uint8_t * buf) an copied in the code to I2Cwrite out of the Ardafruit CCS811 lib :D

Now the function looks like:

bool CCS811::i2cread(int reg, int num, uint8_t * buf) { uint8_t value; uint8_t pos = 0;

//on arduino we need to read in 32 byte chunks
while(pos < num){

    uint8_t read_now = min((uint8_t)32, (uint8_t)(num - pos));
    Wire.beginTransmission((uint8_t)_slaveaddr);
    Wire.write((uint8_t)reg + pos);
    Wire.endTransmission();
    Wire.requestFrom((uint8_t)_slaveaddr, read_now);

    for(int i=0; i<read_now; i++){
        buf[pos] = Wire.read();
        pos++;
    }
}
return true;

}




sry for the bad markdown styling... I do not got it, how I can insert a code block

maarten-pennings commented 5 years ago

I don't see why your change solves your problem. The code that you added supports reads of more than 32 bytes, but the driver never does that. Could you simplify your read function by getting rid of the while loop (it will only be executed once since num is always smaller than 32). I want to know the root cause...

Creepwood commented 5 years ago

For sure. I'm also interested :)

I tested this code for you (removed while loop): And it works bool CCS811::i2cread(int reg, int num, uint8_t * buf) { uint8_t value; uint8_t pos = 0;

//on arduino we need to read in 32 byte chunks

uint8_t read_now = min((uint8_t)32, (uint8_t)(num - pos));
Wire.beginTransmission((uint8_t)_slaveaddr);
Wire.write((uint8_t)reg + pos);
Wire.endTransmission();
Wire.requestFrom((uint8_t)_slaveaddr, read_now);

for(int i=0; i<read_now; i++){
    buf[pos] = Wire.read();
    pos++;
}
return true;

} The Serial output: setup: Starting CCS811 basic demo setup: library version: 8 setup: hardware version: 12 setup: bootloader version: 1000 setup: application version: 2000 CCS811: waiting for (new) data CCS811: waiting for (new) data CCS811: waiting for (new) data CCS811: waiting for (new) data CCS811: eco2=400 ppm etvoc=0 ppb raw6=3 uA raw10=497 ADC R=267054 ohm CCS811: eco2=400 ppm etvoc=0 ppb raw6=3 uA raw10=502 ADC R=269741 ohm CCS811: eco2=400 ppm etvoc=0 ppb raw6=3 uA raw10=507 ADC R=272428 ohm




After that, I tested your function again, because maybe now with new frimware on ccs811 it will work: But it's the same error... // Reads 'countbytes from register at addressregaddr, and stores them inbuf`. Returns false on I2C problems. bool CCS811::i2cread(int regaddr, int count, uint8_t * buf) { Wire.beginTransmission(_slaveaddr); // START, SLAVEADDR Wire.write(regaddr); // Register address int wres= Wire.endTransmission(false); // Repeated START delayMicroseconds(_i2cdelay_us); // Wait int rres=Wire.requestFrom(_slaveaddr,count); // From CCS811, read bytes, STOP for( int i=0; i<count; i++ ) buf[i]=Wire.read(); return (wres==0) && (rres==count); }

The Serial output: setup: Starting CCS811 basic demo setup: library version: 8 ccs811: begin: HW_ID read failed setup: CCS811 begin FAILED setup: hardware version: FFFFFFFF setup: bootloader version: FFFFFFFF setup: application version: FFFFFFFF CCS811: I2C error

maarten-pennings commented 5 years ago

Your working code looks like this at the moment , right?

bool CCS811::i2cread(int reg, int num, uint8_t * buf)
{
  uint8_t value; 
  uint8_t pos = 0;

  //on arduino we need to read in 32 byte chunks
  uint8_t read_now = min((uint8_t)32, (uint8_t)(num - pos));
  Wire.beginTransmission((uint8_t)_slaveaddr);
  Wire.write((uint8_t)reg + pos);
  Wire.endTransmission();
  Wire.requestFrom((uint8_t)_slaveaddr, read_now);

  for(int i=0; i<read_now; i++){
    buf[pos] = Wire.read();
    pos++;
  }
  return true;
}

Notice that pos is always 0 in the first part of the code. And that in the second part pos equals i. Furthermore, num is smaller than 32, and value is not used. Let's use all this to simplify your code further. We get this:

bool CCS811::i2cread(int reg, int num, uint8_t * buf)
{
  Wire.beginTransmission((uint8_t)_slaveaddr);
  Wire.write((uint8_t)reg);
  Wire.endTransmission();
  Wire.requestFrom((uint8_t)_slaveaddr, (uint8_t)num);

  for(int i=0; i<read_now; i++){
    buf[i] = Wire.read();
  }
  return true;
}

Could you check if this simplification still works - I guess so (let's hope I made no typos). It would be great if you could rename reg to regaddr and num to count, then the code is even more equal.

If you compare the above code to my driver code, there is not much of a difference left anymore:

The last bullet is tricky. You have no parameter in Wire.endTransmission(). Then the parameter defaults to true. I have an explicit false. This means I have a single start-write-read-stop I2C transaction (as is normal for a read from register), you have two I2C transactions start-write-stop then start-read-stop (which is tricky because the chip could loose the register address).

Could you try my code and replace the false in Wire.endTransmission(false) to a true.

You could also run as separate check the third bullet, and change my return (wres==0) && (rres==count); also to a return true;.

Happy debugging :-)

Creepwood commented 5 years ago

1. I try this code, like your simplification: And it works bool CCS811::i2cread(int regaddr, int count, uint8_t * buf) { Wire.beginTransmission((uint8_t)_slaveaddr); Wire.write((uint8_t)regaddr); Wire.endTransmission(); Wire.requestFrom((uint8_t)_slaveaddr, (uint8_t)count);

for(int i=0; i<count; i++){ buf[i] = Wire.read(); } return true; }

I only changed for(int i=0; i<read_now; i++) to for(int i=0; i<count; i++) because read_now is not available any more.

2. I try your code with Wire.endTransmission(true) again: And now it works! bool CCS811::i2cread(int regaddr, int count, uint8_t * buf) { Wire.beginTransmission(_slaveaddr); // START, SLAVEADDR Wire.write(regaddr); // Register address int wres= Wire.endTransmission(true); // Repeated START delayMicroseconds(0); // Wait int rres=Wire.requestFrom(_slaveaddr,count); // From CCS811, read bytes, STOP for( int i=0; i<count; i++ ) buf[i]=Wire.read(); return (wres==0) && (rres==count); }

3. Let's try your code with Wire.endTransmission(false) : Boom! Not working! Now the code looks like: bool CCS811::i2cread(int regaddr, int count, uint8_t * buf) { Wire.beginTransmission(_slaveaddr); // START, SLAVEADDR Wire.write(regaddr); // Register address int wres= Wire.endTransmission(false); // Repeated START delayMicroseconds(0); // Wait int rres=Wire.requestFrom(_slaveaddr,count); // From CCS811, read bytes, STOP for( int i=0; i<count; i++ ) buf[i]=Wire.read(); return (wres==0) && (rres==count); }

And the Serial output is again: setup: Starting CCS811 basic demo setup: library version: 8 ccs811: begin: HW_ID read failed setup: CCS811 begin FAILED setup: hardware version: FFFFFFFF setup: bootloader version: FFFFFFFF setup: application version: FFFFFFFF CCS811: I2C error


So the problem is the Wire.endTransmission(false)... with Wire.endTransmission(true) it's working :) Why is that? :D Do you want to update your repro like that? I can also try to flash another CCS811 firmware with this code for you.


And I'm really excited if the CCS811 is now better working with the new firmware. Currently I log the values in an MySQL database and compare it with an iAQ-Core P sensor from AMS. I run two iAQ-Core P sensors since 3 years... and I really like it. They are installed in two different rooms, that are connected with a opened door. And both values are pretty good!

iaq_corep

But compared with the CCS811.... the CCS811 values ​​are totally different :( airquality

maarten-pennings commented 5 years ago

Thanks for all the testing! I already suspected the STOP condition.

We could change the codem however, from a transaction perspective my code (without the STOP) is better than the new code (with the STOP).

You are running this on an ESP32, right? Could you try if the ESP8266 workaround works for ESP32 as well?

int wres= Wire.endTransmission(false); // Repeated START
delayMicroseconds(50); // <== extra wait here

Do you happen to have a logic analyser/scope? I'm really interested to see what happens when this executes.

Creepwood commented 5 years ago

No problem ;) Yes I am using a ESP32. It seem, that the ESP8266 workaround is not working.

1. try with Wire.endTransmission(false): not working bool CCS811::i2cread(int regaddr, int count, uint8_t * buf) { Wire.beginTransmission(_slaveaddr); // START, SLAVEADDR Wire.write(regaddr); // Register address int wres= Wire.endTransmission(false); // Repeated START delayMicroseconds(50); // Wait int rres=Wire.requestFrom(_slaveaddr,count); // From CCS811, read bytes, STOP for( int i=0; i<count; i++ ) buf[i]=Wire.read(); return (wres==0) && (rres==count); } 20190113_170140

2. try with Wire.endTransmission(true): working bool CCS811::i2cread(int regaddr, int count, uint8_t * buf) { Wire.beginTransmission(_slaveaddr); // START, SLAVEADDR Wire.write(regaddr); // Register address int wres= Wire.endTransmission(true); // Repeated START delayMicroseconds(50); // Wait int rres=Wire.requestFrom(_slaveaddr,count); // From CCS811, read bytes, STOP for( int i=0; i<count; i++ ) buf[i]=Wire.read(); return (wres==0) && (rres==count); } 20190113_170042
I checked the whole scope buffer... but don't see any differents

maarten-pennings commented 5 years ago

Sorry was a busy week - did not yet have time to look at this ...

maarten-pennings commented 5 years ago

I think I solved your problem. See the new section I added for ESP32. Basically, there is a bug in the ESP32 I2C lib (for repeated start conditions). So, do not use ESP32 Arduino core lib version 1.0.0, but use 1.0.1.

I hooked an ESP32 to a CCS811, and this library change did it for me. (Using library 1.0.0, I do see the correct I2C transaction on the logic analyser, but the bytes reported by the CCS811 are not picked up by the ESP32 lib).

maarten-pennings commented 5 years ago

Can I close the Issue?

Creepwood commented 5 years ago

Sorry for waiting... this time it was a busy week for me :D

I think we can close the issue.

I'm going play around with your library, but I think the next few weeks will be to busy for me. And when I come across a problem again, I will contact you by a new issue ;)

Thanks bro.