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
Sgp40 fix #2462
Merged
Merged
Sgp40 fix #2462
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8c70db6
Sample from SGP40 sensor at the appropriate interval
natelust 859eb32
Add missing configuration values for SGP40
natelust a397206
Address review comments
natelust eef477e
Format according to clang-tidy
natelust f8bb7f0
Attempt 2 at clang tidy
natelust File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -77,6 +77,26 @@ void SGP40Component::setup() { | |||||
} | ||||||
|
||||||
this->self_test_(); | ||||||
|
||||||
/* The official spec for this sensor at https://docs.rs-online.com/1956/A700000007055193.pdf | ||||||
indicates this sensor should be driven at 1Hz. Comments from the developers at: | ||||||
https://github.com/Sensirion/embedded-sgp/issues/136 indicate the algorithm should be a bit | ||||||
resilient to slight timing variations so the software timer should be accurate enough for | ||||||
this. | ||||||
|
||||||
This block starts sampling from the sensor at 1Hz, and is done seperately from the call | ||||||
to the update method. This seperation is to support getting accurate measurements but | ||||||
limit the amount of communication done over wifi for power consumption or to keep the | ||||||
number of records reported from being overwhelming. | ||||||
|
||||||
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. | ||||||
*/ | ||||||
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();}); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Component already has this method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh right, sorry, I'm still trying to load this code base into my mental cache |
||||||
} | ||||||
} | ||||||
|
||||||
void SGP40Component::self_test_() { | ||||||
|
@@ -224,21 +244,32 @@ uint8_t SGP40Component::generate_crc_(const uint8_t *data, uint8_t datalen) { | |||||
return crc; | ||||||
} | ||||||
|
||||||
void SGP40Component::update() { | ||||||
this->seconds_since_last_store_ += this->update_interval_ / 1000; | ||||||
|
||||||
uint32_t voc_index = this->measure_voc_index_(); | ||||||
void SGP40Component::update_voc_index() { | ||||||
uint32_t update_interval = this->optimal_sampling_ ? 1 : this->update_interval_ / 1000; | ||||||
this->seconds_since_last_store_ += update_interval; | ||||||
|
||||||
this->voc_index_ = this->measure_voc_index_(); | ||||||
if (this->samples_read_ < this->samples_to_stabalize_) { | ||||||
this->samples_read_++; | ||||||
ESP_LOGD(TAG, "Sensor has not collected enough samples yet. (%d/%d) VOC index is: %u", this->samples_read_, | ||||||
this->samples_to_stabalize_, voc_index); | ||||||
this->samples_to_stabalize_, this->voc_index_); | ||||||
return; | ||||||
} | ||||||
|
||||||
} | ||||||
|
||||||
void SGP40Component::update() { | ||||||
if (!(this->optimal_sampling_)) { | ||||||
this->update_voc_index(); | ||||||
} | ||||||
|
||||||
if (this->samples_read_ < this->samples_to_stabalize_) { | ||||||
return; | ||||||
} | ||||||
|
||||||
if (voc_index != UINT16_MAX) { | ||||||
if (this->voc_index_ != UINT16_MAX) { | ||||||
this->status_clear_warning(); | ||||||
this->publish_state(voc_index); | ||||||
this->publish_state(this->voc_index_); | ||||||
} else { | ||||||
this->status_set_warning(); | ||||||
} | ||||||
|
@@ -247,6 +278,9 @@ void SGP40Component::update() { | |||||
void SGP40Component::dump_config() { | ||||||
ESP_LOGCONFIG(TAG, "SGP40:"); | ||||||
LOG_I2C_DEVICE(this); | ||||||
ESP_LOGCONFIG(TAG, " optimal_samping: %d", this->optimal_sampling_); | ||||||
ESP_LOGCONFIG(TAG, " store_baseline: %d", this->store_baseline_); | ||||||
|
||||||
if (this->is_failed()) { | ||||||
switch (this->error_code_) { | ||||||
case COMMUNICATION_FAILED: | ||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
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 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.
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.
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.
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.
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.