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

[krwq]

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

[krwq]

Seems this is working around the problem with pressure:

bme280.SetPowerMode(Bmx280PowerMode.Forced);

The pressure/altitude after this shows us:

Pressure: 101477.66015625 Pa
Altitude: -12.7022561440805 meters

I'm definitely not under water right now so presumably sea level pressure or some other calculation is wrong as well

[RobinTTY]

Yeah you will always need to perform a measurement by calling SetPowerMode (this is by design), I just tested this with a reference python library and basically got the same value for pressure:

My phones sensor is currently giving me a value of 981.0 so not far off. I'll take a look at the Altitude formula since I didn't tocuh it in my last PR, maybe something is wrong with that.

[krwq]

@RobinTTY python's bug doesn't mean we have to repeat the same bug in here.

If pressure cannot be read for whatever reason it should throw an exception or return false. Constructing BME280 should also provide sane defaults which means it should be usable.

We cannot return bogus results unless hardware has gone bad.

[RobinTTY]

Yeah currently we are providing the default configuration at construction (Things like sampling modes, etc.). The problem is that currently TryReadX has no check whether a measurement was already made since it only reads the register with the values of the last measurement (after device startup default register value). So we could just perform a single measurement on construction to avoid that.

[RobinTTY]

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.

[krwq]

Cool, thank you @RobinTTY! Yes we should either return false+NaN or give correct answer.

Also, I've actually tried sleeping for 2s before reading the value and it read the wrong value as well, do you also need to call something to initiate reading process?

[RobinTTY]

@krwq: So I tried this piece of code:

class Program
    {
        static void Main(string[] args)
        {
            var settings = new I2cConnectionSettings(1, Bme280.SecondaryI2cAddress);
            var i2c = I2cDevice.Create(settings);

            using (var sensor = new Bme280(i2c))
            {
                var duration = sensor.GetMeasurementDuration();

                for (var i = 0; i < 3; i++)
                {
                    sensor.SetPowerMode(Bmx280PowerMode.Forced);
                    Task.Delay(duration).Wait();
                    sensor.TryReadTemperature(out var temperature);
                    sensor.TryReadHumidity(out var humidity);
                    sensor.TryReadPressure(out var pressure);
                    sensor.TryReadAltitude(1000, out var altitude);

                    Console.WriteLine($"Temperature: {temperature.Celsius}");
                    Console.WriteLine($"Humidity: {humidity}");
                    Console.WriteLine($"Pressure: {pressure}");
                    Console.WriteLine($"Altitude: {altitude}");
                }
            }
        }
    }

And I get correct values on the first read. What value was wrong for you?

[krwq]

@RobinTTY I'm expecting everything to work without sensor.SetPowerMode(Bmx280PowerMode.Forced);

[RobinTTY]

@krwq That would require a change in the current behavior of the driver. The way the sensor works is that you specify settings like sampling rate and filter (you can for instance skip the pressure meassurement and only measure temperature/humidity/gas). Then you trigger the actual measurement by setting the PowerMode.

The Bmx280 has besides the forced power mode a normal power mode, where measurements are continously being made after setting the sensor to that mode. That is why you always have to set the power mode currently, before reading the results. We could instead change the behavior to something like this:

// Single Measurement (power mode forced)
PerformMeasurement(out Temperature temperature, out double pressure, out double humidity)
{
    _bmx280.SetPowerMode(PowerMode.Forced);
    Task.Delay(measurementTime).Wait();
    temperature = CompensateTemperature(temp >> 4);
    ....
}

// Continuous measurements (power mode normal)
PerformContinousMeasurements
{
    _bmx280.SetPowerMode(PowerMode.Normal);
}

That would mean that you would still need the methods to read the registers for the case of the continous measurements but wouldn't need them for a single one. Is that what you would want?

[krwq]

@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.

[RobinTTY]

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

[krwq]

Thank you @RobinTTY! To avoid extra work/refactoring in the PR it might be worth to write something resembling API proposal (no implementation) - just what you think everything should look like (i.e. just sample code from user perspective)

[RobinTTY]

Sure, I'll post the proposal here.

[Ellerbach]

The sensor default mode once created is PowerMode.Sleep. So, correct, you just need to set up a mode before reading a value. It's the case for most sensor having a sleep mode. Btw, the readme and the example shows the right order.

[krwq]

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)

[RobinTTY]

Ok so what I think the API surface should look like is very similar to what your proposed @krwq:

// I would create a struct like this for each sensor type
// since they all differ in what they are able to measure
struct Bme280ReadResult
{
    Temperature? Temperature { get; }
    double? Pressure { get; }
    double? Humidity { get; }
}

// Bme680 would have a seperate enum since only manual mode and sleep are available
enum Bmx280OperationMode
{
    Continous,
    Manual,
    Sleep
}

// Bme680 would only need ReadAsync() since no continous mode is available
class Bmx280
{
    // for continous mode
    public Bme280ReadResult Read();
    // for manual mode
    public Task<Bme280ReadResult> ReadAsync();
    // change the mode (For Bmx280 would start in continous mode by default)
    public SetOperationMode(Bmx280OperationMode mode)
}

[krwq]

Thanks @RobinTTY, couple of comments:

• Bme280ReadResult - would the struct work for other BMxx80? If so we should re-use it
• Bmx280OperationMode: what's the expected default? Manual/Continuous/(don't change)?
• public SetOperationMode(Bmx280OperationMode mode) - can we make this a property instead? (assuming we don't have to wait for the operation mode to be changed) Is the most likely way of using it be setting this value once or are we expecting users have to call this function frequently?
• How is Manual different than Sleep? In both cases users have to do something special to start reading which IMO makes Manual and Sleep equivalent (the moment you need to read the implementation would wake up the device, do the reading and put it back to sleep)

[RobinTTY]

@krwq

• Bme280ReadResult would only work for the Bme280, I would have a separate struct for Bmp280 (no humidity measurement) and Bme680 (additional gas measurement). An alternative would be to create one struct that has all the properties and set the ones not able to be measured by the sensor to null.

On second thought I would actually drop SetOperationMode and just do:

class Bmx280
{
    // continous mode
    public Bme280ReadResult Read();
    // manual mode (perform one measurement)
    public Task<Bme280ReadResult> ReadAsync();
    // sleep mode (no measurements)
    public void Sleep()
}

The reasons to have a method to put the sensor to sleep are:

• It's more intuitive to call Sleep() if you want to switch from continous mode to sleep instead of calling ReadAsync() which doesn't really indicate that the sensor will switch to sleep after performing one measurement
• You have a way to switch the sensor from continous mode to sleep without performing another measurement, this might be an edge case where you don't want another measurement to happen before going to sleep but it's probably good to give the option

[krwq]

@RobinTTY the reason I added nullable double results is to make it clear that value is not available and had exactly what you mentioned in mind - if we are making it per sensor then I'd prefer values to be non-nullable. I'm fine with either choice.

I think the manual/continuous makes sense:

• in continous mode Read returns immediately whatever sensor returns (except perhaps first measurement)
• in manual mode Read waits until measurement is made

Would explicit sleep mean we are always in manual mode? When would continuous be used in such case?

I'm fine with adding extra Sleep method on top of manual/continous if there is any scenario where it makes sense to have it (i.e. less battery consumption and calling sleep would auto wake it up on read and potentially make read last longer)

[RobinTTY]

• Yeah non nullable sounds good to me and a struct per sensor
• Explicit sleep just means that the sensor is neither in manual or continous mode. Setting it to manual mode would be done by calling ReadAsync() after which the sensor performs a single measurement and goes back to sleep. Going back to continous mode would be done by calling Read().
• Sleep is nice for ultra low power consumption as you mentioned.

[krwq]

@RobinTTY Read and ReadAsync should not have such differences in behavior

I think we should go with the ReadAsync behavior you mentioned above so for both Read/ReadAsync following should happen:

• Sleep: Reading changes device to Manual mode, initiates measurements, waits until it is done and puts device back to Sleep mode
• Manual: Reading initiates measurement and waits until it is done
• Continuous: Device does measurements on its own in the background, reading immediately returns value stored internally by the device without any waiting (first measurement is ok to have extra wait)

Other thing is I'm not sure I like name Sleep, perhaps we should call it LowConsumption or something along these lines and make it clear in the docs that you should use continuous when you measure very frequently, manual when measuring once every few seconds and LowConsumption when measuring every 30s or more.

(Numbers above are example, we should do better estimate based on spec and add it in the xml doc comments)

[RobinTTY]

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.

[krwq]

@RobinTTY let's do only manual/continuous in such case and have a property/method to change between the modes (if it's quick to change then let's make it property, if requires couple of ms then method)

[RobinTTY]

@krwq The reason why I would do a Sleep() method instead of a property to change to the different modes is:

• You would expect Read() to change to continous mode if not already in and give you a result
• You would expect ReadAsync() to change the mode to manual and give you a result
• The only thing left is switching to sleep mode

Switching additionally through a property doesn't make much sense to me since this is what we already have with SetPowerMode() which we wanted to eliminate to make the API more intuitive. I would never expect someone to switch to manual mode trough the property since you would then need to call ReadAsync() again to get the result which does another measurement.

[krwq]

@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

[RobinTTY]

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.

[MarkCiliaVincenti]

The altitude calculation can be improved. I'm working on a WeatherHelper (#783) and I'll be adding an altitude calculation there. However I'm trying to gather information on how to improve this calculation due to the fact that there is a missing input in the current formula: temperature. The current calculation is assuming 15C, but you can see that temperature affects the altitude calculation. Take a look at https://www.mide.com/pages/air-pressure-at-altitude-calculator for example.

[MarkCiliaVincenti]

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

[Ellerbach]

I feel this has been fixed over time with the various PR. So closing it. Feel free to reopen if it's still not the case.

[krwq]

still an issue in 1.4 (it keeps reading same value if you don't set it). I also need to wait around 100ms after setting power mode, otherwise I'm getting random errors ocasionally

[pgrawehr]

Removing vNext, since not a blocker (there's an existing and well-known workaround)