Skip to content

Make sure we turn ambient pressure to an int. #7

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

Merged
merged 2 commits into from
Jan 29, 2021

Conversation

rpavlik
Copy link
Contributor

@rpavlik rpavlik commented Jan 24, 2021

I noticed that there was this feature to set the pressure for better results, and since I was using a CLUE, I figured I'd set it on startup. Of course, the BMP280 returns a float, not an int, so I got an error about trying to shift a float. This fixes the error, by turning whatever we're given in the setter of ambient_pressure into an int, so you can just:

# Tell it our ambient pressure on startup for best data
scd.ambient_pressure = clue.pressure

@ladyada ladyada requested a review from caternuson January 24, 2021 04:46
@tannewt
Copy link
Member

tannewt commented Jan 27, 2021

ping @caternuson

@caternuson
Copy link
Contributor

@tannewt yep. on it. just got one yesterday :)

@caternuson
Copy link
Contributor

@rpavlik What about doing this in user code instead?

scd.ambient_pressure = int(clue.pressure)

This would help reinforce the fact that the setting really is only integer resolution:
image

It would also make things more symetric in terms of getting the same value back when you read scd.ambient_pressure.

@caternuson
Copy link
Contributor

Actually, I'm going to be like a politician and flip-flip on this. I'm cool with it. I think the trade off of allowing simpler user code vs. the potential issues of the automagic behavior is minimal.

@rpavlik Want to do something similar for altitude while you're at it?

@rpavlik
Copy link
Contributor Author

rpavlik commented Jan 28, 2021

Ah, I hadn't thought of altitude since my thought process was literally "hey, I can have it compensate for pressure, this clue has a bmp280, I shall take the value from bmp280 and apply to scd30". Sure, I can get to that tonight probably.

@rpavlik
Copy link
Contributor Author

rpavlik commented Jan 29, 2021

There you go!

@rpavlik
Copy link
Contributor Author

rpavlik commented Jan 29, 2021

BTW, we probably want to bump major version soon on this because of the eCO2 -> CO2 rename?

@caternuson
Copy link
Contributor

Looks good. Tested real quick on a MatrixPortal. The parameter is still eCO2 since this PR is based on branch forked without that merged in.

Adafruit CircuitPython 6.1.0 on 2021-01-21; Adafruit Matrix Portal M4 with samd51j19
>>> import board
>>> import adafruit_scd30
>>> scd30 = adafruit_scd30.SCD30(board.I2C())
>>> scd30.eCO2
755.951
>>> scd30.ambient_pressure
0
>>> scd30.ambient_pressure = 1010.5
>>> scd30.ambient_pressure
1010
>>> scd30.altitude
0
>>> scd30.altitude = 300.5
>>> scd30.altitude
300
>>> scd30.eCO2
779.802
>>> 

@caternuson caternuson merged commit 66b756d into adafruit:main Jan 29, 2021
@caternuson
Copy link
Contributor

BTW, we probably want to bump major version soon on this because of the eCO2 -> CO2 rename?

Yep. Looks like that was done a couple of days ago with the 2.0.0 release:
https://github.com/adafruit/Adafruit_CircuitPython_SCD30/releases/tag/2.0.0

@rpavlik
Copy link
Contributor Author

rpavlik commented Jan 29, 2021

awesome thanks!

@rpavlik rpavlik deleted the pressure-type branch January 29, 2021 18:33
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Feb 3, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_SCD30 to 2.0.1 from 2.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_SCD30#7 from rpavlik/pressure-type

Updating https://github.com/adafruit/Adafruit_CircuitPython_Seesaw to 1.7.0 from 1.6.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_seesaw#58 from adafruit/dherrada-patch-1
  > Merge pull request adafruit/Adafruit_CircuitPython_seesaw#59 from rsbohn/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Text to 2.12.1 from 2.11.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#112 from jposada202020/updating-docs
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#110 from FoamyGuy/bitmap_label_ascent_descent

Updating https://github.com/adafruit/Adafruit_CircuitPython_Slideshow to 1.5.5 from 1.5.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_Slideshow#35 from adafruit/dherrada-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Added the following libraries: Adafruit_CircuitPython_SimpleMath
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.

3 participants