adafruit / Adafruit_CircuitPython_BME280

CircuitPython driver for the BME280
MIT License
63 stars 42 forks source link

fix #25 + fix #19 + refactor `pressure` property #26

Closed barbudor closed 5 years ago

barbudor commented 5 years ago

Tested on CPX with CircuitPython 4.0.1 using bme280_simpletest and bme280_normal_mode

barbudor commented 5 years ago

Recap: in the pressure property there is a test if var1 == 0: in order to avoid a division-by-zero exception. Current code return a pressure of 0 in that case. It was suggested that my PR makes this a little more pythonic by usage of exception raise.

Analysis of the root cause: Last calculation before the test is var1 = (1.0 + var1 / 32768.0) * self._pressure_calib[0] The only reason for var1 to become 0 is self._pressure_calib[0] to be 0. Calibration values are read at init time from registers in the chip. So the root cause probably relates to a register read problem.

Analysis of other similar codes Bosch bme280.c (Github): if var1 == 0, return minimum valid pressure (300hPa). Adafruit CiPy BMP280 driver: => initially, no tests were performed => In Feb, jraber implemeted return 0 in a commit named "Adopt changes from the BME280 library" => In Mar, jraber implemeted return minimum value in a commit named "Remove unnecessary 'if' and 'else' in the pressure property" Adafruit CiPy BMP680 driver: not tested => ZeroDivisionError

I feel that returning silently the minimun valid pressure is not correct to draw attention on a possible hardware reliability problem. Returning 0 could make sense only if we were sure that the user was testing the return value against 0. Alternatively, letting the ZeroDivisionError occur will not provide proper clue to the user as to the root cause of the problem and will probably lead to this issue to be raised again as a bug in GitHub.

Proposed solution: Test against 0 and raise a ArithmeticError with message "Invalid calculation possibly related to error while reading the calibration registers"

ladyada commented 5 years ago

@kattni plz also test since you have a BME280

jerryneedell commented 5 years ago

FWIW -- I download and ran both examples from the repo on this PR on a Raspberry Pi. No problems.

ladyada commented 5 years ago

@jerryneedell thanks! merged. anyone here can do a bump/releaes :)

caternuson commented 5 years ago

Done. Bump/release to 2.3.1.