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

Add one shot co2 measurement #7

Merged
merged 6 commits into from
Jul 30, 2023
Merged

Conversation

KeithTheEE
Copy link
Contributor

This pull request probably isn't ready to merge because I haven't tested it on the SCD41.

There's an added example that should make it easy to setup and test on the SCD41. It should grab three samples of one shot co2 measurements, and then three samples of one shot relative humidity and temperature. I grab three samples because the datasheet suggests if the single shot is made right after powerup, it's better to grab three and drop the first two due to noise at start up.

I expected to get nonsense values on the SCD40, but it just returned 'kind of' accurate measurements. (I say kind of because there was a moderate amount of noise, but they were close to the expected values).

The measure_single_shot_rht_only should have a co2 value of 0 ppm, and if we don't get that on the SCD41, then there's a bug in the code I need to fix. I guessed the SCD40 would at least return a 0 there as well, but it did not, so I don't know how much of that is because of a bug vs because the 40 doesn't respond to that command.

In the event this is ready to be merged soon, it should address #5

@tannewt tannewt requested a review from kattni August 30, 2021 19:40
@kattni
Copy link
Contributor

kattni commented Aug 30, 2021

Ah, yes! I knew this was incoming but did not see it had been submitted. I'll test it this week!

@kattni
Copy link
Contributor

kattni commented Aug 30, 2021

@KeithTheEE I haven't looked deeply at this PR yet, but did you ever really figure out how to differentiate between the chips? Or do we simply have to document the caveat that it isn't supported on the 40? I want to add the feature, but I'm hesitant to add something that will "work" on both, even though it's only supported on one.

@KeithTheEE
Copy link
Contributor Author

I have not found out how to distinguish between the two. I have an idea, but it's dependent on measure_single_shot_rht_only for the moment.

Basically if it doesn't return '0' for the co2 ppm on the measure_single_shot_rht_only, it's an scd40, if it does it's an scd41. But that's assuming I even implemented that correctly. So if you don't get a 0 co2 ppm then when you call it on an SCD41, this code has a bug and shouldn't be merged.

I'm just at the stage where I'm understanding how the two sensors respond to the same call.

@kattni
Copy link
Contributor

kattni commented Aug 30, 2021

@KeithTheEE Noted! I'll let you know what I get once I get set up to test it.

@kattni
Copy link
Contributor

kattni commented Aug 30, 2021

@KeithTheEE Here are the results I get with your example. Please let me know if I should be trying something else.

code.py output:
Serial number: ['0x41', '0x6f', '0x5f', '0x7', '0x3b', '0x4a']
Waiting for single shot CO2 measurement from SCD41....
CO2: 660 ppm
Temperature: 18.5 *C
Humidity: 56.1 %

CO2: 718 ppm
Temperature: 18.7 *C
Humidity: 55.9 %

CO2: 691 ppm
Temperature: 19.0 *C
Humidity: 55.4 %

Waiting for single shot Humidity and Temperature measurement from SCD41....
CO2: 656 ppm
Temperature: 19.3 *C
Humidity: 54.7 %

CO2: 656 ppm
Temperature: 19.3 *C
Humidity: 54.7 %

CO2: 656 ppm
Temperature: 19.3 *C
Humidity: 54.6 %


Code done running.

@KeithTheEE
Copy link
Contributor Author

Whelp that means I messed something up. Thank you for giving it a test, I'll have to dig in and see what's the cause of the bug.

@KeithTheEE KeithTheEE closed this Aug 30, 2021
@kattni
Copy link
Contributor

kattni commented Aug 30, 2021

@KeithTheEE We can keep this open and put it in draft status if you intend to continue working on it. That would keep someone from merging it, but still keep it open for collaboration and so on. Would that work better for you?

@KeithTheEE
Copy link
Contributor Author

Oh yeah that would work! I didn't know I could keep it as a draft.

@KeithTheEE KeithTheEE reopened this Aug 30, 2021
@kattni kattni marked this pull request as draft August 30, 2021 21:26
@kattni
Copy link
Contributor

kattni commented Aug 30, 2021

Yeah, it's a subtle suggestion in the menu on the right side of the PR. Not super obvious if you don't know to look for it. Also, I'm uncertain whether you, not having write access, can even see the option. Either way, bumped into draft status.

@KeithTheEE
Copy link
Contributor Author

I recently got the scd41, so I should be able to start testing this more rapidly this week.

@FoamyGuy
Copy link
Contributor

@KeithTheEE just checking in on this one, is this still something you are going to be looking into?

@hopkapi
Copy link

hopkapi commented Jul 2, 2022

Hey, don't know if this is helpful/news to anyone, but it looks like Sensirion recently (May 2022) updated the datasheet to add info on power down and wake up commands specifically for the single-shot mode. I also now have a sensor to actually test with, though don't have anything that can measure µAs of power etc.

https://sensirion.github.io/python-i2c-scd/api.html?highlight=wake#sensirion_i2c_scd.scd4x.device.Scd4xI2cDevice.power_down too

@litui
Copy link

litui commented Jul 14, 2022

Just chiming in that this patch is confirmed working for me with @KeithTheEE's changes on the SCD41. It could probably be merged as-is.

@FoamyGuy FoamyGuy marked this pull request as ready for review July 30, 2023 19:22
Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've merged main, and added typing information for the new functions so they match the functions that came from main.

I'm thinking this looks good for now. I do not have the proper hardware for testing. We had a report from litui that it was confirmed working, it looks to completely isolated to only new functionality and shouldn't pose any issues with backwards compatibility.

If we do end up having trouble with it or decide upon further changes later we can always make a new PR for it.

Thank you @KeithTheEE and @litui for leaving a note about testing it!

@FoamyGuy FoamyGuy merged commit 22aba82 into adafruit:main Jul 30, 2023
1 check passed
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 31, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_MS8607 to 1.0.19 from 1.0.18:
  > Merge pull request adafruit/Adafruit_CircuitPython_MS8607#13 from sdomoszlai13/type_annotations

Updating https://github.com/adafruit/Adafruit_CircuitPython_SCD4X to 1.4.0 from 1.3.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_SCD4X#7 from KeithTheEE/add_one_shot_co2

Updating https://github.com/adafruit/Adafruit_CircuitPython_ServoKit to 1.3.15 from 1.3.14:
  > Merge pull request adafruit/Adafruit_CircuitPython_ServoKit#49 from spovlot/fix-i2c-doc

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.

5 participants