Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reading correct values #26

Merged
merged 1 commit into from
Mar 5, 2023
Merged

reading correct values #26

merged 1 commit into from
Mar 5, 2023

Conversation

jposada202020
Copy link
Contributor

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

while (
self._read_u8(_MPL3115A2_REGISTER_STATUS) & _MPL3115A2_REGISTER_STATUS_PDR
== 0

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

self._write_u8(
_MPL3115A2_PT_DATA_CFG,
_MPL3115A2_PT_DATA_CFG_TDEFE
| _MPL3115A2_PT_DATA_CFG_PDEFE
| _MPL3115A2_PT_DATA_CFG_DREM,
)

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

@caternuson caternuson merged commit df0c9fc into adafruit:main Mar 5, 2023
@jposada202020 jposada202020 deleted the correcting_readings branch March 5, 2023 16:44
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Mar 6, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_DotStar to 2.2.7 from 2.2.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_DotStar#63 from Neradoc/no-bus-device

Updating https://github.com/adafruit/Adafruit_CircuitPython_MPL3115A2 to 2.0.5 from 2.0.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_MPL3115A2#26 from jposada202020/correcting_readings
  > Add upload url to release action
  > Add .venv to .gitignore

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect pressure readings
2 participants