adafruit / Adafruit_CircuitPython_MPL3115A2

CircuitPython module for the MPL3115A2 barometric pressure & temperature sensor.
MIT License
5 stars 8 forks source link

reading correct values #26

Closed jposada202020 closed 1 year ago

jposada202020 commented 1 year ago

closes #24

After further investigation, I found that because of the logic of the library, the flags of the DR_STATUS Register were not being cleared. Causing, the while conditionals waiting for the measure flag were not taken into consideration, as the condition was already different from 0

https://github.com/adafruit/Adafruit_CircuitPython_MPL3115A2/blob/b32af7d72e016762632f656a891e847058ae41ae/adafruit_mpl3115a2.py#L236-L238

Depending, on the reading order, a quick survey on the register even showed data overwrite on these bits (See table 12 on the datasheet)

According to the datasheet, you need to read OUT_P_MSB or OUT_T_MSB. However, when we read the temperature, the flag for pressure/altitude was not cleared because we were only reading two bytes in OUT_T_MSB, so the flag in DR_STATUS Register for pressure/altitude measurement was not cleared, causing all sort of problems downwards.

Just a heads-up, the flag behaviour configuration is set up here https://github.com/adafruit/Adafruit_CircuitPython_MPL3115A2/blob/b32af7d72e016762632f656a891e847058ae41ae/adafruit_mpl3115a2.py#L152-L157

Finally, I do think that it would be good to re-do the library, as at the moment we will be reading the whole altitude/pressure and temperature anyway. But is not the intention of this PR.

@caternuson could you take a look? :) Thanks.

caternuson commented 1 year ago

Cool. I think you've found the underlying issue. It also explains why the Arduino library works, since it reads both the pressure and temperature data buffers with every data fetch (i.e., all 5 bytes): https://github.com/adafruit/Adafruit_MPL3115A2_Library/blob/1890a3c891d1577a8895ecf89cca4166e6af1616/Adafruit_MPL3115A2.cpp#L193

Is there a chance the temperature reading could have a similar issue? Since the pressure and altitude fetches are only reading 3 bytes? This would be less obvious, since back to back temperature readings will generally be similar anyway. It was obvious for pressure and altitude since they share the same data registers, but are drastically different in actual value.

jposada202020 commented 1 year ago

Yes, same thing. It does create a flag in the register that needs to be cleared. So yes, if we do not clear the flag, and that is why we read the whole 5 bytes for the pressure/altitude. Presently, if we read the temperature first, it would create the flag "pressure/altitude measure ready", but we do not do anything about it as we do not read OUT_P_MSB, so next time that we read altitude/pressure it will fail. But I did consider that in this PR, and change the logic, to read the whole 5 bytes, but only using the temperature ones.

caternuson commented 1 year ago

oh! sry, scanned the code too fast. now i see - reading all 5 bytes for everything now.

tested:

Adafruit CircuitPython 7.3.3 on 2022-08-29; Adafruit QT Py RP2040 with rp2040
>>> import board, adafruit_mpl3115a2
>>> mpl = adafruit_mpl3115a2.MPL3115A2(board.I2C())
>>> print(mpl.pressure, mpl.altitude, mpl.temperature)
993.03 168.878 18.6875
>>> print(mpl.pressure, mpl.altitude, mpl.temperature)
993.052 169.003 18.75
>>> print(mpl.pressure, mpl.altitude, mpl.temperature)
993.055 168.878 18.75
>>> print(mpl.pressure, mpl.altitude, mpl.temperature)
993.047 168.628 18.75
>>>  

thanks! this lgtm. agree a future refactor/cleanup would be nice. but like this PR as is - it's the simplest fix for now.