adafruit / Adafruit_BME680

117 stars 76 forks source link

Strange behavior and non responsiveness (VERY long delay) when using NTP #29

Closed PurpleAir closed 5 years ago

PurpleAir commented 5 years ago

After implementing this sensor, we experienced a strange, seemingly random behavior. Sometimes, the sensor would initialize and provide readings like expected, other times it would take 60 seconds to provide the first reading, then would be ok after that, other times, it would initiate a delay that was in the hundreds of thousands of seconds so it would just grind everything to a halt for an indefinite period.

After some debugging and running the sample that worked perfectly every time, it turns out that the NTP time sync we use causes this problem. It is something to do with the way you calculate the delay. It could also be a problem with (or if) the millis() rollover were to happen between beginning and ending the reading (see https://arduino.stackexchange.com/questions/12587/how-can-i-handle-the-millis-rollover).

My "quick fix" was to only permit small delays of less than 1 second as follows in three places:

unsigned long Adafruit_BME680::beginReading(void) {
//
//Added following check to make sure we do not delay for too long
if (_meas_end - millis() > 1000) {
_meas_end = 0;
}
.
.
.
bool Adafruit_BME680::endReading(void) {
  unsigned long meas_end = beginReading();
  if (meas_end == 0) {
    return false;
  }

  unsigned long now = millis();
  if (meas_end > now) {
    unsigned long meas_period = meas_end - now;
#ifdef BME680_DEBUG
    Serial.print("Waiting (ms) "); Serial.println(meas_period);
#endif
//Added following check to make sure we do not delay for too long
    if (meas_period < 1000) {
        delay(meas_period * 2); /* Delay till the measurement is ready */
    }
.
.
.

static void delay_msec(uint32_t ms){
//Added if check to make sure we do not delay for too long
  if(ms < 1000) delay(ms);
}

This may work, but it is not a good solution. Would be better to fix the timing properly so these high delays do not happen. It is also not really "async" as implemented since you cannot safely call any of the functions from a ticker or other type of interrupt based function since you use delay().

PurpleAir commented 5 years ago

This may be a fix? https://github.com/adafruit/Adafruit_BME680/pull/22

PurpleAir commented 5 years ago

Something else that is happening, the library when installed using Arduino is not the latest code. I had to manually copy and paste the raw files into it to get the updates that are in here with a fix that seems to solve my problem, ie the implementation of remainingReadingMillis().

PurpleAir commented 5 years ago

In the function int Adafruit_BME680::remainingReadingMillis(void)

Instead of int remaing_time = (millis() - _meas_start) - (int)_meas_period;

It should be int remaing_time = (_meas_start + (int)_meas_period) - millis();

hoffmannjan commented 5 years ago

tested it and looks ok for me, will be merged soon. thanks! 👍

PurpleAir commented 5 years ago

I am not sure if this will survive a millis() rollover. Are you @hoffmannjan ?