pimoroni / bme280-python

Python library for the BME280 temperature, pressure and humidity sensor
https://shop.pimoroni.com/products/bme280-breakout
MIT License
65 stars 25 forks source link

Double call of update_sensor() when getting altitude #16

Open JimMadge opened 3 years ago

JimMadge commented 3 years ago

Hi,

I noticed that update_sensor() appears to be called twice in a row when calling get_altitude. First self.update_sensor() is called followed by self.get_pressure(),

https://github.com/pimoroni/bme280-python/blob/97c80ced20b02caa129e98c8801b31f1468ee4b0/library/bme280/__init__.py#L275-L279

However, the first line of get_pressure is also self.update_sensor(),

https://github.com/pimoroni/bme280-python/blob/97c80ced20b02caa129e98c8801b31f1468ee4b0/library/bme280/__init__.py#L267-L269

So, if we unrolled this we would get something like

self.update_sensor()
self.update_sensor()
pressure = self.pressure

I'm happy to open a PR changing this if it isn't intended.

Gadgetoid commented 3 years ago

It's a side-effect of the fact it makes sense for all the readings to be processed at once, coupled with the fact I prefer methods over attributes for getting values... and you kind of expect a method to do something and fetch the data.

There are ways this could be mitigated, but it's one of those cases where running update_sensor() twice probably hurts less than contriving some method to prevent it.

With the benefit of hindsight, I'd probably rewrite the whole library API :grimacing: but since we can't really do that, I'm open to suggestions!

JimMadge commented 3 years ago

It's a side-effect of the fact it makes sense for all the readings to be processed at once

On that point, it did occur to me that adding a get_readings method which returns a tuple (temperature, pressure, humidity) would be a nice addition for those who want to get all of the values, particularly in forced mode. I could do that if you'd like.

coupled with the fact I prefer methods over attributes for getting values...

This is precisely the kind of thing that makes me spend a day agonising over how I should do something rather than actually doing it 😆 . Python's flexibility can be a blessing and a curse.

I think it would be safe to remove self.update_sensor() from get_altitude as it will always be called anyway on the first line of get_pressure (and get_pressure requires that line to function correctly).