Sgp40 fix #2462

natelust · 2021-10-07T22:10:51Z

What does this implement/fix?

The spg40 air quality sensor was incorrectly using the manufacturers algorithm. The documentation here indicates that to get a correct VOC index, this sensor must be sampled and supplied to their algorithm at 1Hz (With discussion here suggesting tolerances should allow slight differences that would make software timing loops sufficient).

Previously this sensor (and related value) were only sampled at the update interval, which would lead to an incorrect VOC index of unknown magnitude. This change introduces a separate timer for reading the sensor and updating the VOC index at the correct cadence. The update interval is then used only to publish this state to other components or the front end, saving power with the wifi module, or limiting the number of samples supplied to the front end. To allow the old behavior a new config parameter is introduced, optimal_sampling, which can be set to false.

In a separate commit, a fix is made to the dump_config method, adding missing config entries.

This is a breaking change not in the sense that a user needs to make a change to benefit from the fix. However, since the sensor will be read out at the correct interval, it may cause more wake-ups or power consumption on a device when a user is not expecting it.

Quick description and explanation of changes

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#1515

Test Environment

ESP32
[x ] ESP8266

Example entry for `config.yaml`:

New config parameter defaults to new correct behavior. To restore the old
incorrect behavior the config would look like:

sensor:
  - platform: sgp40
    name: "Air Quality"
    optimal_sampling: false

# Example config.yaml

Checklist:

[x ] 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:

[x ] Documentation added/updated in esphome-docs.

The spg40 sensor must be sampled at 1Hz for the VOC index algorithm to work correctly. This commit introduces a on device timer to sample correctly seperately from updating the public state of the component.

The SGP40 component was not printing all of it's configuration in dump_config, add in the missing store_baseline value.

probot-esphome · 2021-10-07T22:10:58Z

Hey there @SenexCrenshaw, mind taking a look at this pull request as it has been labeled with an integration (sgp40) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

OttoWinter · 2021-10-09T08:31:04Z

esphome/components/sgp40/sgp40.cpp

+  */
+  if (this->optimal_sampling_) {
+    ESP_LOGD(TAG, "Using optimal sampling, setting up background sampler");
+    App.scheduler.set_interval(this, "", 1000, [this](){this->update_voc_index();});


Suggested change

App.scheduler.set_interval(this, "", 1000, [this](){this->update_voc_index();});

this->set_interval(1000, [this](){this->update_voc_index();});

Component already has this method

Oh right, sorry, I'm still trying to load this code base into my mental cache

OttoWinter · 2021-10-09T08:36:04Z

esphome/components/sgp40/sgp40.cpp

+
+  At configuration the component can be configured to turn off optimal sampling in which
+  case the sensor will be read out with the same cadence as the update method, leading to
+  likely inacurate measurements, but possibly accurate enough for certain use cases. 


Do we really need this? I mean if you have to update close to 1Hz anyway, why should we even expose this option?

I'd say we remove that option, and if someone does come up with a real use-case we can always add it back

I didn't really know what your projects stance on backwards compatibility was, and I wanted to provide a way for people who may be running this on a battery or have somehow coded a flow around the incorrect behavior to retain the existing behavior. If that is not a priority I am happy to change it and the accompanying documentation.

Ah sure no problem

We try to not have breaking changes, though I think in this case the previous behavior would have been broken anyway unless it's polled at 1Hz, no? In that case it wouldn't really be a breaking change and ok.

Yeah, how it currently is the VOC measurement is just not an accurate reflection of reality, it may happen to be close enough for some, but will not behave correctly overtime.

Thanks for the heads up on the way the project approaches things. I am pretty new to ESPHome, and it's pretty amazing. I have a few other things I hope to contribute, so I hope to make things easier on you all reviewing things in future PRs.

OttoWinter

Thanks!

Just some smaller things re config option, and also see the lint checks on the PR - a bit of code needs to be reformatted

natelust · 2021-10-09T13:59:43Z

Sorry about the formatting, I'll get that fixed up, and shoot something back to you. I'm not sure about the eradicate on commits of your project. Would you like the changes re-based away, or on a separate commit to maintain the github history?

OttoWinter · 2021-10-09T14:01:58Z

Sorry about the formatting, I'll get that fixed up, and shoot something back to you. I'm not sure about the eradicate on commits of your project. Would you like the changes re-based away, or on a separate commit to maintain the github history?

Only necessary when there are merge conflicts. We'll squash merge the commit anyways, so just adding more commits to the branch is fine

natelust · 2021-10-09T16:49:31Z

I think I have addressed your concerns and cleaned up the formatting, whenever you want to take another look. I am happy to do any additional work you may request. Thank you.

OttoWinter

LGTM, thanks!

natelust added 2 commits October 7, 2021 17:26

Sample from SGP40 sensor at the appropriate interval

8c70db6

The spg40 sensor must be sampled at 1Hz for the VOC index algorithm to work correctly. This commit introduces a on device timer to sample correctly seperately from updating the public state of the component.

Add missing configuration values for SGP40

859eb32

The SGP40 component was not printing all of it's configuration in dump_config, add in the missing store_baseline value.

project-bot bot added this to Needs Review in ESPHome Dev Oct 7, 2021

probot-esphome bot added the integration: sgp40 label Oct 7, 2021

natelust mentioned this pull request Oct 7, 2021

Add new documentation to SGP40 component esphome/esphome-docs#1515

Merged

2 tasks

OttoWinter reviewed Oct 9, 2021

View reviewed changes

natelust added 3 commits October 9, 2021 12:21

Address review comments

a397206

Format according to clang-tidy

eef477e

Attempt 2 at clang tidy

f8bb7f0

OttoWinter approved these changes Oct 10, 2021

View reviewed changes

ESPHome Dev automation moved this from Needs Review to Reviewer Approved Oct 10, 2021

OttoWinter merged commit 92b85f9 into esphome:dev Oct 10, 2021

ESPHome Dev automation moved this from Reviewer Approved to Done Oct 10, 2021

github-actions bot locked and limited conversation to collaborators Oct 12, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sgp40 fix #2462

Sgp40 fix #2462

natelust commented Oct 7, 2021 •

edited

probot-esphome bot commented Oct 7, 2021

OttoWinter Oct 9, 2021

natelust Oct 9, 2021

OttoWinter Oct 9, 2021

natelust Oct 9, 2021

OttoWinter Oct 9, 2021

natelust Oct 9, 2021

OttoWinter left a comment

natelust commented Oct 9, 2021

OttoWinter commented Oct 9, 2021

natelust commented Oct 9, 2021

OttoWinter left a comment

	App.scheduler.set_interval(this, "", 1000, [this](){this->update_voc_index();});
	this->set_interval(1000, [this](){this->update_voc_index();});

Sgp40 fix #2462

Sgp40 fix #2462

Conversation

natelust commented Oct 7, 2021 • edited

What does this implement/fix?

Types of changes

Test Environment

Example entry for config.yaml:

Checklist:

probot-esphome bot commented Oct 7, 2021

OttoWinter Oct 9, 2021

Choose a reason for hiding this comment

natelust Oct 9, 2021

Choose a reason for hiding this comment

OttoWinter Oct 9, 2021

Choose a reason for hiding this comment

natelust Oct 9, 2021

Choose a reason for hiding this comment

OttoWinter Oct 9, 2021

Choose a reason for hiding this comment

natelust Oct 9, 2021

Choose a reason for hiding this comment

OttoWinter left a comment

Choose a reason for hiding this comment

natelust commented Oct 9, 2021

OttoWinter commented Oct 9, 2021

natelust commented Oct 9, 2021

OttoWinter left a comment

Choose a reason for hiding this comment

natelust commented Oct 7, 2021 •

edited

Example entry for `config.yaml`: