Bmxx80 API modification

[RobinTTY]

Addresses #753

Still need to rebase (as soon as #783 is merged) and make some other changes.
This will be a breaking change.

[MarkCiliaVincenti]

You should use the new Pressure unit throughout (refer to #768) which was merged.

[MarkCiliaVincenti]

@RobinTTY, #783 was merged at last. How can we incorporate this now?
Also, should we be providing async methods? @krwq @joperezr

[RobinTTY]

@MarkCiliaVincenti ah great, I will take a look at it and try to finish this pull request finally in the next few days. I will have to take a look again but I think we agreed in #753 to implement a new read method (synchronous and async implementation) while keeping the old implementation where you still have to set the powerMode manually. If the plan to keep the current methods hasn't changed I would try to incorporate that.

Edit: @krwq @joperezr Since last working on this the project states now "This repository is still in experimental stage and all APIs are subject to changes.". Does this mean you would be fine with removing the old public facing methods e.g.:

i2CBmpe80.SetPowerMode(Bmx280PowerMode.Forced);
i2CBmpe80.TryReadTemperature(out tempValue);
i2CBmpe80.TryReadPressure(out preValue);
i2CBmpe80.TryReadHumidity(out humValue);

for one read method as mentioned in #753? It would certainly make the implementation more clean.

[MarkCiliaVincenti]

@MarkCiliaVincenti ah great, I will take a look at it and try to finish this pull request finally in the next few days. I will have to take a look again but I think we agreed in #753 to implement a new read method (synchronous and async implementation) while keeping the old implementation where you still have to set the powerMode manually. If the plan to keep the current methods hasn't changed I would try to incorporate that.

Edit: @krwq @joperezr Since last working on this the project states now "This repository is still in experimental stage and all APIs are subject to changes.". Does this mean you would be fine with removing the old public facing methods e.g.:

i2CBmpe80.SetPowerMode(Bmx280PowerMode.Forced);
i2CBmpe80.TryReadTemperature(out tempValue);
i2CBmpe80.TryReadPressure(out preValue);
i2CBmpe80.TryReadHumidity(out humValue);

for one read method as mentioned in #753? It would certainly make the implementation more clean.

My opinion would be to leave them there, call the read method and mark them as obsolete.

[RobinTTY]

I implemented the Read/ReadAsync methods but discovered a bug that was introduced when the switch to UnitsNet happened.
Since the Bmxx80 devices have an option to disable a certain reading like this:

bme680.PressureSampling = Sampling.Skipped;

and UnitsNet does not handle NaN values, all sensors will currently throw an exception when a sampling is skipped since the current implementation will try this:

if (PressureSampling == Sampling.Skipped)
{
    // UnitsNet will throw an exception here
    pressure = Pressure.FromPascals(double.NaN);
}

Is there already some guidance on how to handle this? All solutions I could think of are not that clean. The easiest and "cleanest" probably being setting those values to the maximum possible value if they are not measured and letting the user handle it. Nullable values complicate using the driver a lot:

public static async Task Main()
        {
            Console.WriteLine("Hello BME680!");

            // The I2C bus ID on the Raspberry Pi 3.
            const int busId = 1;
            // set this to the current sea level pressure in the area for correct altitude readings
            var defaultSeaLevelPressure = WeatherHelper.MeanSeaLevel;

            var i2cSettings = new I2cConnectionSettings(busId, Bme680.DefaultI2cAddress);
            var i2cDevice = I2cDevice.Create(i2cSettings);

            using (var bme680 = new Bme680(i2cDevice))
            {
                while (true)
                {
                    // 10 consecutive measurement with default settings
                    for (var i = 0; i < 10; i++)
                    {
                        // Perform the measurement
                        var result = bme680.Read();

                        // Print out the measured data
                        double altValue = 0;
                        if (result.Pressure != null && result.Temperature != null)
                        {
                            altValue = WeatherHelper.CalculateAltitude((Pressure)result.Pressure, defaultSeaLevelPressure, (Temperature)result.Temperature);
                        }

                        Console.WriteLine($"Gas resistance: {result.GasResistance:0.##}Ohm");
                        Console.WriteLine($"Temperature: {result.Temperature?.DegreesCelsius:0.#}\u00B0C");
                        Console.WriteLine($"Pressure: {result.Pressure?.Hectopascals:0.##}hPa");
                        Console.WriteLine($"Altitude: {altValue:0.##}m");
                        Console.WriteLine($"Relative humidity: {result.Humidity:0.#}%");

                        // WeatherHelper supports more calculations, such as saturated vapor pressure, actual vapor pressure and absolute humidity.
                        if (result.Temperature != null && result.Humidity != null)
                        {
                            Console.WriteLine($"Heat index: {WeatherHelper.CalculateHeatIndex((Temperature)result.Temperature, (double)result.Humidity).DegreesCelsius:0.#}\u00B0C");
                            Console.WriteLine($"Dew point: {WeatherHelper.CalculateDewPoint((Temperature)result.Temperature, (double)result.Humidity).DegreesCelsius:0.#}\u00B0C");
                        }

                        // when measuring the gas resistance on each cycle it is important to wait a certain interval
                        // because a heating plate is activated which will heat up the sensor without sleep, this can
                        // falsify all readings coming from the sensor
                        Thread.Sleep(1000);
                    }
                }
            }
        }

@MarkCiliaVincenti @krwq @joperezr

[RobinTTY]

Also as a note: Users that choose to skip samplings are pobably experienced enough to remember that those values are not "usable". So setting it to the maximum value is probably safe.

[MarkCiliaVincenti]

In reality, if you choose to skip pressure, will it actually do something less with the new methods? If it involves the same communication with the sensor, we may want to consider not allowing the developer to skip sampling.

[RobinTTY]

Yeah, the measurement will be faster if samplings are skipped, with the new methods even more so because we don't always have to do an additional temperature measurement to update the "tfine" value.

[MarkCiliaVincenti]

Man, I'm really looking forward to this being merged. A bit of a bummer regarding the NaN. I know it's not ideal, but I'd personally prefer using nullable rather than a maxvalue. I understand it makes it a bit more tedious to use, but it's definitely neater. @krwq @joperezr

[RobinTTY]

Will finish this as soon as we get a response from @krwq or @joperezr. 😄

[MarkCiliaVincenti]

If one would still want to keep on using NaN, you'd need code like:

var temp = readings.Temperature?.Value.DegreesCelsius ?? double.NaN;

[krwq]

should you return false if any of these fails?

[RobinTTY]

The new Read() method always succeeds even if one of the measurements is skipped otherwise it would be pretty useless. But the measurement that is skipped would have been NaN in the past. But since UnitsNet doesn't handle NaN we need another solution (null or max value).
See comment #797 (comment).

[krwq]

If you use default(Temperature) as result then you can check i.e. temp.Unit == TemperatureUnit.Undefined

[krwq]

the old methods returning double can check for that and return NaN instead

[RobinTTY]

Using default would result in:

(Temperature + Humidity + Pressure measurement skipped). This could make problems if you use these for some calculation where you don't immediately notice.
Max value looks like this:

So probably both not optimal.

[krwq]

I'm using

            Temperature temp = default;
            Console.WriteLine(temp);
            Console.WriteLine(temp.Unit);
            Console.WriteLine(temp.DegreesCelsius);

and it prints:

0 K
Kelvin
-273.15

unfortunately it's not throwing any errors or return bogus units but you can tell immediately the value is wrong.

For Try* methods user is responsible for checking the output (similarly as if you used Dictionary with Value being int you'd get 0). For existing methods if possible we should hack it so they still work or just remove them and make the breaking change.

When returning structs where one of the fields can be null use Temperature?

[krwq]

Another option is to use: Temperature temp = new Temperature(0, (UnitsNet.Units.TemperatureUnit)(-1)); - that is actually causing failure on any conversion or ToString:

Temperature temp = new Temperature(0, (TemperatureUnit)(-1));
Console.WriteLine(temp); // Throws NotImplementedException
Console.WriteLine(temp.Unit); // Prints -1
Console.WriteLine(temp.DegreesCelsius); // Throws NotImplementedException

another option is to create a single Invalid struct with reflection and then copy it in all places which need it

[krwq]

did you try using default(Unit) instead of NaN? I think that will give you invalid value. Looking at the code the enum they have inside is Nullable which mean default will produce null:
https://github.com/angularsen/UnitsNet/blob/master/UnitsNet/GeneratedCode/Quantities/Temperature.g.cs#L47 that should cause all operations on that unit fail in some way

[MarkCiliaVincenti]

There's another way you can do it too. So you'll have private nullable variables like:

private Temperature? _temperature;

If the temperature reading is skipped, you would set the value of its corresponding value to null.
Then you would have public properties reading those variables. If the value is null, it throws an exception. Eg:

public Temperature Temperature
{
   get
   {
      if (_temperature.HasValue) return _temperature.Value;
      throw new DataException("Temperature was not read.");
   }
}

And here we would be able to decide on what type of exception would be the best to use. I went for DataException, but we could decide on NotSupportedException or TypeAccessException.

@krwq @RobinTTY

[RobinTTY]

There's another way you can do it too. So you'll have private nullable variables like:

private Temperature? _temperature;

If the temperature reading is skipped, you would set the value of its corresponding value to null.
Then you would have public properties reading those variables. If the value is null, it throws an exception. Eg:

public Temperature Temperature
{
   get
   {
      if (_temperature.HasValue) return _temperature.Value;
      throw new DataException("Temperature was not read.");
   }
}

And here we would be able to decide on what type of exception would be the best to use. I went for DataException, but we could decide on NotSupportedException or TypeAccessException.

@krwq @RobinTTY

I like this approach we could do this for the double values as well, so instead of returning double.NaN we throw an exception. The exception message is also a lot more helpful than when you see that something went wrong in the UnitsNet conversion.

Edit: So this approach would be ok for the new Read method, the tryRead methods can just return false and return the default value

I also noticed that UnitsNet has units for Distance and Resistance which could have been used for the bmxx devices. (altitude -> meters, GasResistance -> resistance). Was there a reason this was not done or should we make that change as well?

[krwq]

When throwing exception from properties we should give user a way to check if it will throw, perhaps we should just expose nullable properties and perhaps make Read return some kind of status like: Success, PartialRead, Error.

[RobinTTY]

When throwing exception from properties we should give user a way to check if it will throw, perhaps we should just expose nullable properties and perhaps make Read return some kind of status like: Success, PartialRead, Error.

@krwq So would you be in favor of nullable properties for both old methods and new? Or leave it at non nullable and add a return code to the new Read method while leaving the old ones as is (where you can already check the returned bool value to check status). Both approaches are fine by me.

Edit: for the status an enum with the Flags attribute would probably be best so we can indicate which readings are not usable

[MarkCiliaVincenti]

Yes sure, we should have TryRead. I don't get what PartialRead means though. Is it because you specified something to be skipped or is it because it failed to get, say, the humidity (is this even possible that it gets some but not all values?)

[RobinTTY]

Yes sure, we should have TryRead. I don't get what PartialRead means though. Is it because you specified something to be skipped or is it because it failed to get, say, the humidity (is this even possible that it gets some but not all values?)

The sensor will only not return a certain measurement if the user specifies it should be skipped. There is no "failing" to get a measurement otherwise.

[MarkCiliaVincenti]

Well, that's what we need to handle. Let's say your sensor is unplugged. If you try to read measurements, then we should ideally have a TryRead that would return false in such a case (sensor is unplugged).

[RobinTTY]

Well if you try to create an instance of the bmxx class it will check if the sensor is present (checking i2c address). I don't know if it is helpful to check this every time you make a measurement, I don't think we do this for other devices?! I think it's ok to get a communication related exception in this case if thats the behavior for other devices as well.

Edit: If i disconnect the sensor while doing a measurement I recieve an System.IO.Exception with the I2C error code which gives me a very good description of what happened. I don't think we want to add another layer of error codes over this.

[krwq]

I thought about partial read because I thought it is possible for i.e. humidity read to fail while temperature works. If it's 100% user choice that they don't want a reading then only TryRead method returning bool is enough. With properties throwing user must have a way of telling if that will fail or not (if someone unplugs the sensor while you're reading that is exceptional and we can throw but we should then have a way of detecting that as well separately with something like Status so that user can add more diagnostics in case of exception). I like the solution proposed somewhere here:

bool TryRead(); // I think it would be ok to call it just Read too, no preference on my side
Temperature Temperature { get; } // throws if users select an option that they don't want it or no read has been performed (or it failed), otherwise it must give correct reading
... // whatever other properties same as temperature

We can extend error functionality by adding property like Status LastReadStatus or something in the future

[RobinTTY]

@krwq since we return the read results inside a class with the read method like this:

public Bme680ReadResult Read();

public class Bme680ReadResult
{
    public Temperature Temperature { get; }
    public Pressure Pressure { get; }
    ....
}

we could either just add a Status property to that class which would look something like this:

class Bme680ReadStatus
{
    bool TemperatureSampled { get; }
    bool PressureSampled { get; }
    ....
}

So this way you can individually check if the required sampling was performed. Or we could return the status and have the result as out parameter:

public Bme680ReadStatus Read(out Bme680ReadResult result);

This way it would be similar to the old methods. Would either of those options be fine with you?

[krwq]

@RobinTTY would it make sense to just make fields nullable? that would make it much simpler to use

[RobinTTY]

Sorry for the delay, will get to this in the next few days 😄

[joperezr]

[Triage] @RobinTTY still interested in doing this? Do note that given this PR is a bit old a lot of things have changed underneath, including the fact that the bindings code is now marked as nullable so that will require some rebasing. Let us know if you are not interested in doing this so that we can close this in our next triage meeting.

[joperezr]

[Triage] Friendly Ping 😄

[RobinTTY]

Apologies, I will get to this today.

[RobinTTY]

@joperezr Alright this is ready for review.

So what this PR achieves is, instead of doing:

bme680.SetPowerMode(Bme680PowerMode.Forced);

Thread.Sleep(measurementDuration.ToTimeSpan());

bme680.TryReadTemperature(out var tempValue);
bme680.TryReadPressure(out var preValue);
bme680.TryReadHumidity(out var humValue);
bme680.TryReadGasResistance(out var gasResistance);

you can now simply use:

var readResult = bme680.Read();
// or
var readResult = await bme680.ReadAsync();

What you get is an object which looks something like this (depending on the sensor):

    public class Bme680ReadResult
    {
        public Temperature? Temperature { get; }
        public Pressure? Pressure { get; }
        public Ratio? Humidity { get; }
        public ElectricResistance? GasResistance { get; }
    }

Values are null if the user decides he wants to skip a particular sampling.

[joperezr]

Awesome Thanks for getting to it @RobinTTY, we will take a look at this soon.

[krwq]

What do you gain from having *Core method? wouldn't it be better to directly call TryReadGasResistance?

[krwq]

LGTM, thanks, sorry for this being so stretched out in time

[krwq]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).