adafruit / Adafruit_BME280_Library

Arduino Library for BME280 sensors
Other
328 stars 301 forks source link

begin() Method Corrupts Sensor ID If Not Valid #69

Closed gt500sc closed 4 years ago

gt500sc commented 4 years ago

Hello,

First post so please forgive posting guideline deviations :)

I had unknowingly received a BMP280 board instead of the BME280 board I ordered and tried to use it with this library. When I called the begin() method using I2C and passing the correct address, it returned false and a subsequent call to the sensorID() method returned 0xFF. After much troubleshooting, I wrote code to read the chip's ID register directly and found that it was 0x58, indicating that it was the wrong board (BMP280). Based on this, the begin() method correctly returned false, but it corrupted the _sensorID value.

The way that the begin() methods are implemented, if the return from init() is false for whatever reason, it will try to use the other I2C address. If the original problem was just that the chip ID was not valid, using the other I2C address will cause the chip ID to be set to 0xFF, indicating a possible address problem.

I would recommend changing the begin() methods so that if the I2C address is passed, only that address is read. If it's not passed, both addresses are attempted. This can be accomplished I believe without affecting existing users by making the following changes to two of the begin() methods:

bool Adafruit_BME280::begin(uint8_t addr, TwoWire *theWire) {
  _i2caddr = addr;
  _wire = theWire;
  return init();
}

bool Adafruit_BME280::begin(TwoWire *theWire) {
  bool status = false;
  status = begin(BME280_ADDRESS, theWire);
  if (!status) {
    status = begin(BME280_ADDRESS_ALTERNATE, theWire);
  }
  return status;
}

Please let me know if there is a better/correct way to submit this type of report, or if you need any additional info regarding this.

For completeness, here are my specifics:

Thank you, Greg Thielen

caternuson commented 4 years ago

When I called the begin() method using I2C and passing the correct address, it returned false and a subsequent call to the sensorID() method returned 0xFF

Hmmm. A false return from a call to begin() means something happened and you can't really expect any subsequent calls, like to sensorID(), to actually work. Basically you just have to stop and figure out what's wrong.

Looks like the example even has some commentary about this? https://github.com/adafruit/Adafruit_BME280_Library/blob/616c7290e4a1bf863021506fe8367e924881cc80/examples/bme280test/bme280test.ino#L47-L53

gt500sc commented 4 years ago

Thanks for your reply, but I don't really agree with this reasoning, especially because the library is doing something I didn't tell it to do.  I provided the I2C address I wanted the library to use and if it did not try the other address, the sensorID value would be intact.  With the change that I suggested, the library would only try multiple I2C addresses if one wasn't explicitly passed.  In either case, if there was some other kind of error, like the device was just not found, I would expect that nothing beyond begin() would work and that the sensorID() method would also return an error.

On 12/4/19 4:37 PM, Carter Nelson wrote:

When I called the begin() method using I2C and passing the correct
address, it returned false and a subsequent call to the sensorID()
method returned 0xFF

Hmmm. A false return from a call to |begin()| means something happened and you can't really expect any subsequent calls, like to |sensorID()|, to actually work. Basically you just have to stop and figure out what's wrong.

Looks like the example even has some commentary about this? https://github.com/adafruit/Adafruit_BME280_Library/blob/616c7290e4a1bf863021506fe8367e924881cc80/examples/bme280test/bme280test.ino#L47-L53

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_BME280_Library/issues/69?email_source=notifications&email_token=AGFP2HMBZP37RUMLOMSV3ADQXBEK5A5CNFSM4JR4GPD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF7BQSA#issuecomment-561911880, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGFP2HIH2XN77YBTKDCX3D3QXBEK5ANCNFSM4JR4GPDQ.

caternuson commented 4 years ago

In general, it's up to you to check the return from begin(). If it's false, then your code should act accordingly, i.e. stop and not call anything else. But if you do make additional calls, you should expect unknown results. So for example, here:

When I called the begin() method using I2C and passing the correct address, it returned false and a subsequent call to the sensorID() method returned 0xFF.

You should have investigated why begin() returned false and generally ignored the return from sensorID().

Your original issue was from using this library with a different sensor. If you use it with the correct sensor, do you still have issues? If the auto-switching to alternate address does not work as expected with that arrangement, then that's something that could be investigated.

gt500sc commented 4 years ago

Carter,

I understand what you're saying - I agree that the initial issue was due to the wrong sensor being used, but let's take a look at another situation.

Let's say I have two legit BME280 sensors, one on 0x77 and one on 0x76, and I want to use both of them concurrently.  In this case, I would do something like:

Adafruit_BME280 bme1; Adafruit_BME280 bme2;

bool rc1 = bme1.begin(0x77); bool rc2 = bme2.begin(0x76);

bme1 is found and is a valid BME280 so rc1 = true. bme2 for whatever reason fails.  rc2 should be false, but the way begin() currently works, it will then try 0x77 and find the existing bme1 sensor and return true.  Now my program thinks I have two good sensors, but in reality they're both pointing to the same address.  If the changes I proposed were implemented, 0x77 would not be tried when 0x76 failed because I explicitly passed in the address I wanted to use.

Would this situation warrant consideration?

Thanks, Greg

On 12/5/19 8:43 AM, Carter Nelson wrote:

In general, it's up to you to check the return from |begin()|. If it's |false|, then your code should act accordingly, i.e. stop and not call anything else. But if you do make additional calls, you should expect unknown results. So for example, here:

When I called the begin() method using I2C and passing the correct
address, it returned false and a subsequent call to the sensorID()
method returned 0xFF.

You should have investigated why |begin()| returned false and generally ignored the return from |sensorID()|.

Your original issue was from using this library with a different sensor. If you use it with the correct sensor, do you still have issues? If the auto-switching to alternate address does not work as expected with that arrangement, then that's something that could be investigated.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_BME280_Library/issues/69?email_source=notifications&email_token=AGFP2HLG4NORF6AIKU6ECR3QXEVSXA5CNFSM4JR4GPD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGBKXRQ#issuecomment-562211782, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGFP2HLAQ6BWB63HKOC6J33QXEVSXANCNFSM4JR4GPDQ.

ladyada commented 4 years ago

greg, with two sensors you would check both returns. we have no plans to add a check per function. if you submit a lightweight PR we'll look at merging it!

gt500sc commented 4 years ago

@ladyada my example above is checking both returns and I'm not really sure what you're saying about not adding a check per function, but I submitted a PR with my changes. My first PR so I hope I did it right. Thanks!

caternuson commented 4 years ago

The two BME280 sensor case, one good + one bad, does seem more interesting. The good one will mask the bad one and the code will run, just not as expected. I could see how that would be confusing.

@ladyada Do you know the history for this "try the alternate address" logic? Why not just keep it simple and only try the address provided?

ladyada commented 4 years ago

im p. sure it was a 'well meaning' PR, because folks use non-adafruit sensors and get confused when they 'dont work' - we should revert it, tho

caternuson commented 4 years ago

So revert to simple? No fancy "try the alternate address" logic?

ladyada commented 4 years ago

yeah!

caternuson commented 4 years ago

@gt500sc Do you want to try changing your PR to make that happen? This is essentially much simpler. There's no logic needed at all. Just try to init with the supplied address and return status. If no address is specified, just use a default (that would be handled via the overloaded c-tors).