-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
…measurement for the SCD41
Ah, yes! I knew this was incoming but did not see it had been submitted. I'll test it this week! |
@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. |
I have not found out how to distinguish between the two. I have an idea, but it's dependent on Basically if it doesn't return '0' for the co2 ppm on the I'm just at the stage where I'm understanding how the two sensors respond to the same call. |
@KeithTheEE Noted! I'll let you know what I get once I get set up to test it. |
@KeithTheEE Here are the results I get with your example. Please let me know if I should be trying something else.
|
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 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? |
Oh yeah that would work! I didn't know I could keep it as a draft. |
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. |
I recently got the scd41, so I should be able to start testing this more rapidly this week. |
@KeithTheEE just checking in on this one, is this still something you are going to be looking into? |
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. |
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. |
# Conflicts: # adafruit_scd4x.py
There was a problem hiding this 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!
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
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