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 support for SCD4X #2217

Merged
merged 8 commits into from Sep 28, 2021
Merged

Add support for SCD4X #2217

merged 8 commits into from Sep 28, 2021

Conversation

sjtrny
Copy link
Contributor

@sjtrny sjtrny commented Aug 29, 2021

What does this implement/fix?

Adds support for the SCD4X sensor, which is a CO2 sensor (+temp and RH).

There is already a PR for this sensor. However it modifies the SCD30 sensor.

This is a simpler version that provides the same feature set as SCD30 but with the updated SCD40 sensor.

In future better support for temperature/pressure compensation and forced recalibration could be provided.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#1429

Test Environment

  • ESP32
  • ESP8266

Example entry for config.yaml:

  - platform: scd4x
    co2:
      name: "CO2"
    temperature:
      name: "Temperature"
    humidity:
      name: "Humidity"

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@probot-esphome
Copy link

Hey there @sjtrny,
Thanks for submitting this pull request! Can you add yourself as a codeowner for this integration? This way we can notify you if a bug report for this integration is reported.
In __init__.py of the integration, please add:

CODEOWNERS = ["@sjtrny"]

And run script/build_codeowners.py

(message by NeedsCodeownersLabel)

@sjtrny sjtrny marked this pull request as ready for review August 29, 2021 06:21
@oxan
Copy link
Member

oxan commented Aug 29, 2021

There is already a PR for this sensor.

That seems to be #1561.

However it modifies the SCD30 sensor.

This is a simpler version that provides the same feature set as SCD30 but with the updated SCD40 sensor.

Could you elaborate a bit on why this PR is needed or better than that other PR? Judging from the code they seem to be 90% similar.

@sjtrny
Copy link
Contributor Author

sjtrny commented Aug 29, 2021

There is already a PR for this sensor.

That seems to be #1561.

However it modifies the SCD30 sensor.
This is a simpler version that provides the same feature set as SCD30 but with the updated SCD40 sensor.

Could you elaborate a bit on why this PR is needed or better than that other PR? Judging from the code they seem to be 90% similar.

Because it modifies an existing sensor definition. I think it's a better idea (hygiene, bug safety) to just have the introduction of a new sensor be standalone. Only if there's enough justification should a merged/shared codebase be used.

I kind of expect uptake of SCD30 sensors to die anyway. Most people will move to the SCD4X series because of size.

ESPHome Dev automation moved this from Needs Review to Reviewer Approved Sep 22, 2021
@oxan
Copy link
Member

oxan commented Sep 27, 2021

@sjtrny Could you test the component with the changes Otto made? If it still works, this can be merged.

@sjtrny
Copy link
Contributor Author

sjtrny commented Sep 28, 2021

@sjtrny Could you test the component with the changes Otto made? If it still works, this can be merged.

Confirmed. It is working as expected 🎉.

[19:27:40][D][sensor:121]: 'CO2': Sending state 691.00000 ppm with 0 decimals of accuracy
[19:27:40][D][sensor:121]: 'Temperature': Sending state 22.85736 °C with 2 decimals of accuracy
[19:27:40][D][sensor:121]: 'Humidity': Sending state 51.92871 % with 2 decimals of accuracy

@oxan oxan merged commit af04f56 into esphome:dev Sep 28, 2021
ESPHome Dev automation moved this from Reviewer Approved to Done Sep 28, 2021
@oxan
Copy link
Member

oxan commented Sep 29, 2021

Thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Oct 1, 2021
ESPHome Dev automation moved this from Done to Reviewer Approved Sep 23, 2023
ESPHome Dev automation moved this from Reviewer Approved to Done Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants