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 SCD40/41 support #13139

Merged
merged 11 commits into from
Sep 23, 2021
Merged

Add SCD40/41 support #13139

merged 11 commits into from
Sep 23, 2021

Conversation

Arnold-n
Copy link

@Arnold-n Arnold-n commented Sep 15, 2021

Description:

Adds Sensirion SCD40/SCD41 CO2 sensor support.
Related discussion: #11329

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works with Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works with Tasmota core ESP32 V.1.0.7.3
  • I accept the CLA.

Code change is tested and works on ESP8266 with Tasmota 9.5.0.8, not sure which core version.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@s-hadinger
Copy link
Collaborator

Thanks for your PR. You may need to wait for a week or two before Theo can merge

@Arnold-n
Copy link
Author

Thanks s-hadinger!
According to sergios100 (#11329), it works on ESP32 too.

@Jason2866
Copy link
Collaborator

Imho enabling both SCD30 AND SCD40 as default is to much. For example the SPS30 is not enabled at all. For backwards compatibility of the firmware, please disable the SCD40 driver.

@Arnold-n
Copy link
Author

What's the reason not to enable it by default, code size vs expected interest? And do you want it excluded for the ESP32 image too? As you likely know the SCD30 and SCD40 (or SPS30) are unrelated devices: the SCD30 is NDIR while the SCD40 is a miniature sensor based on the photoacoustic sensing principle.

As another (newby) question, currently the SCD40xxx read operations return -1 in case of failure (and a data value otherwise), while write operations return -1 in case of failure or 0 otherwise - should I change the latter also to "or a data value otherwise" ?

@Jason2866
Copy link
Collaborator

Simple reason not all sensors can be enabled. It is Theos decision what will enabled by default.
The decision is based on what sensors are most used.

@Arnold-n
Copy link
Author

Arnold-n commented Sep 17, 2021

Sorry for a mistaken "merge branch" - undone by a force push.

@Arnold-n Arnold-n marked this pull request as ready for review September 17, 2021 07:58
@@ -107,6 +107,7 @@
//#define USE_MGC3130 // [I2cDriver27] Enable MGC3130 Electric Field Effect Sensor (I2C address 0x42) (+2k7 code, 0k3 mem)
//#define USE_MAX44009 // [I2cDriver28] Enable MAX44009 Ambient Light sensor (I2C addresses 0x4A and 0x4B) (+0k8 code)
#define USE_SCD30 // [I2cDriver29] Enable Sensiron SCd30 CO2 sensor (I2C address 0x61) (+3k3 code)
#define USE_SCD40 // [I2cDriver62] Enable Sensiron SCd40 CO2 sensor (I2C address 0x62) (+3k5 code)
Copy link
Owner

Choose a reason for hiding this comment

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

Pls disable as the image will become too big to support easy installation

Copy link
Author

Choose a reason for hiding this comment

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

Disabled in sensors, not in ESP32, let me know if you intended to have that changed too.

@arendst arendst merged commit 608ab64 into arendst:development Sep 23, 2021
@arendst
Copy link
Owner

arendst commented Sep 23, 2021

Thx.

arendst added a commit that referenced this pull request Sep 23, 2021
Add support for Sensirion SCD40/SCD41 CO2 sensor (#13139)
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.

4 participants