iot: BME280 does not measure pressure/altitude correctly when SetPowerMode is not called

Test:

private static Bme280 CreateBme280()
{
    var settings = new I2cConnectionSettings(1, Bme280.DefaultI2cAddress);
    return new Bme280(I2cDevice.Create(settings));
}

const double SeaLevelPressure = 1013.25;
using (Bme280 bme280 = CreateBme280())
{
    Assert.True(bme280.TryReadTemperature(out Temperature temperature));
    Console.WriteLine($"Temperature: {temperature.Celsius} C");
    Assert.True(bme280.TryReadPressure(out double pressure));
    Console.WriteLine($"Pressure: {pressure} Pa");
    Assert.True(bme280.TryReadAltitude(SeaLevelPressure, out double altitudeMeters));
    Console.WriteLine($"Altitude: {altitudeMeters} meters");
    Assert.True(bme280.TryReadHumidity(out var relativeHumidity));
    Console.WriteLine($"Humidity: {relativeHumidity} %");
}

Output:

Temperature: 23.0049688216532 C
Pressure: 74116.26953125 Pa
Altitude: 2560.9681612177 meters
Humidity: 62.4485427633319 %

definitely Pressure and Altitude don’t seem reasonable. I’m currently at around 15-100m above the sea level.

cc: @RobinTTY @georgemathieson

About this issue

Original URL
State: open
Created 5 years ago
Comments: 31 (31 by maintainers)

Most upvoted comments

@RobinTTY please take the changes in #783 into consideration when making these changes, because they will conflict otherwise.

MarkCiliaVincenti on Oct 11, 2019

Oh I get your point. I would go with a property then for operational mode since you can always change the mode, disposing and constructing an object just to change it would be unnecessary work.

I’ll make these changes and submit a PR soon.

RobinTTY on Oct 3, 2019

@RobinTTY Read vs ReadAsync behavior should be the same - this is not personal preference but that’s what all our corefx APIs do.

The difference between current version and the new one would be that with new APIs most of the time you’d set the power mode once (presumably in the constructor) vs right now you should be setting forced mode and waiting before each read

krwq on Oct 3, 2019

Oooh I understand now what you meant. In the case you explained there is no difference between Sleep and Manual because the way the sensor works is that in manual mode (forced mode per data sheet) the sensor automatically goes back to sleep after the measurement, you can’t keep it in manual mode, since manual mode is only one single measurement.

The reasoning I had for having a Sleep() method is to switch from continous mode to sleep mode with the benefits as explained before.

RobinTTY on Oct 2, 2019

Don’t disagree - just thinking that the abstraction can be simplified as proposed above (Read would auto set the mode to forced unless you’re in continuous mode)

krwq on Oct 2, 2019

Sure, I’ll post the proposal here.

RobinTTY on Oct 1, 2019

@krwq Sounds good to me, I’ll submit a PR along with the other changes in the next few days.

RobinTTY on Oct 1, 2019

@RobinTTY changing the behavior is fine if it is well justified and in this case I think it is.

I think by default users will expect constructing a class and reading temperature will just work without any extra knowledge and I think this should be the default option (if I understand correctly: PowerMode.Normal). For any advanced settings we should have a way of doing it but it should not be required to read the spec or all docs.

Does the power mode need to be set before every measurement and cannot be set once globally? The current API shape suggests this is a one-time operation. If this is something you need to do every time the general rule of thumb suggest we should automate it.

Perhaps something along the lines of (pseudo C#):

struct ReadResult
{
  double? Temperature { get; }
  double? Pressure { get; }
  double? Humidity { get; }
  DateTimeOffset Timestamp { get; } // optional: when the measurement was made
}

struct ReadMode
{
  Continuous,
  Manual,
}

class Bme280
{
  public Bme280(..., ReadMode readMode = ReadMode.Continuous);
  public ReadResult Read();
  public Task<ReadResult> ReadAsync();
}

basically for continuous mode Read() will always return immediately and is suggested method of use. For manual mode ReadAsync is suggested which will start measurement and wait for measurement to be completed.

krwq on Oct 1, 2019

Or probably a check whether a read has been performed is better and just return NaN if non was performed. Since if there is a long timespan between construction and first call of TryRead, the values would be out of date.

Edit: The altitude formula seems to be correct but it’s not taking temperature into account. But that shouldn’t make a big difference. Also keep in mind you need to reference the current atmospheric pressure at sea level, a difference of 10hPa can make a difference of 100m.

I will open a pull request to take temperature into account and to return NaN if no measurement was taken before reading the value.

RobinTTY on Sep 30, 2019