Skip to content

Conversation

mikeysklar
Copy link
Contributor

Add SGP30 / SGP40 background sampling. Requires frequently 1s polling or returns 0 values.

Untested and only a proof of concept for feedback. This adds fastTick() polling to keep the SGPxx sensors producing usable data. Thanks @Timeline for the key insight of 1s polling being needed.

Difference observed between 15min polling versus 1s with the SGP40.

Screenshot 2025-09-12 at 9 39 50 AM

Address issue 732

Forum issues:

SGP30 issue with WipperSnapper
SGP40 Air Quality

Add SGP30 / SGP40 background sampling. Requires frequently 1s polling or returns 0 values.
@mikeysklar mikeysklar force-pushed the sgpxx-faster branch 3 times, most recently from 0ee75ca to 662f61c Compare September 13, 2025 02:35
@ladyada ladyada requested review from brentru and tyeth September 13, 2025 17:04
@mikeysklar
Copy link
Contributor Author

mikeysklar commented Sep 14, 2025

I can confirm the SGP40 works with this PR. I tested using:

Adafruit Qt Py ESP32-S3 (2MB PSRAM)
build artifact: wippersnapper.qtpy_esp32s3_n4r2.1.0.0-adafruit-67159326.uf2

Trying different settings from 1 second --> 15 minute update intervals for prolonged periods (hours) was working with the keep alive code.

The SGP30 changes are untested as I don't have one on hand.

@ladyada
Copy link
Member

ladyada commented Sep 15, 2025

brent and tyeth are both out right now but they'll review when return!

@tyeth
Copy link
Member

tyeth commented Sep 15, 2025

Thanks @mikeysklar, this has been weighing on my mind for a while as there are a few we currently "support" that are out of spec in their current use. I thought there was an existing issue, but can't find one, must be thinking of forum discussions.

The only other previous related thoughts were that it might be worth having an internal poll period defined for each component (with a matching entry in the components repo JSON definition), and then a separate publish period (the current thing in definition file), both potentially displayed to the user (publish >= poll in interface) although we'd then need to specify max and min as well as default internal poll period (fastTick speed).
I think that would help users understand (and play out well in low power mode in the future) poll periods.
Also as you've seen, it wasn't worth reading a lot of sensors too frequently (like more than every second). I'm thinking some other air sensors which are 5seconds minimum poll for example. Longer term it's my desire (and been raised) to have the reference values (temp/RH) be passable to the gas sensors too as currently it (20-25c/50%/101.325) throws all the data off. That would either be input feeds for the sensor/component, or some form of custom sensor properties or RPC thing, probably through Actions initially.

On the averaging front, it might be best to leave it to the current techniques, as I think it's probably expected that the moment it publishes data the value represents the current value and not a time accumulated average. The user can see averaged data instead using the dashboard and feed pages and APIs (charts and API get access to aggregated data at various time intervals), and possibly Actions.
Longer term it makes sense to offer the sample averaging as a configurable option (similarly for Analog averaging and/or hysteresis).
There's also a minor issue with the getEvent situation, as each method resets the data, and usually they are called one after another, before a single publish, so first getEvent(VOC for sake of argument) takes the averaged value from _n samples, but then getEventRAW doesn't match that dataset as the accumulators are reset by getEventVOC and so instead getEventRaw asks for a fresh data sample from the sensor.

These are all minor things, and more an attempt to uncork my stored up thoughts on the issue. I'll bring it up with Brent as to how he'd like to proceed, which will be next week, but I can't see any problem with the current idea at all.

@mikeysklar
Copy link
Contributor Author

mikeysklar commented Sep 15, 2025

@tyeth

Thank you for the larger explanation. Using a polling frequency value in the JSON config makes a lot of sense.

Another issue @Timeline pointed out is deep sleep mode which I believe is still in the works. I bring it up here only that continuous background polling could cancel out the ability to use sleep mode with these sensors.

A few more options to consider as JSON config values:

  • ramp up time (eg. 30sec - 1min)
  • number of initial values to throw away
  • deep sleep duration (realistic range)

If you are doing hourly air quality samples it doesn't make a lot of sense to keep the controller awake and polling every second.

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

Overall, this makes sense to me as an implementation for a few drivers in WipperSnapper beta.

We're currently implementing the new WipperSnapper API (v2) and have time to consider alternatives for specific things, like background sampling, into the new API design.

@tyeth If you also approve of this design, I wouldn't be opposed to merging it in. We can talk about it more tomorrow or in this PR.

@mikeysklar I've requested a few design/syntax changes. Please hold off on these until Tyeth also approves.

Comment on lines 1351 to 1355
for (auto *drv : drivers) {
if (drv)
drv->fastTick();
}

Copy link
Member

Choose a reason for hiding this comment

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

Do not use auto here or create a loop. It is redundant and adds a performance overhead for what should be a tight loop.

Instead, try calling drv->fastTick() when we are looping through the vectors below. Since fastTick is virtual, and the vect. of drivers exists, you dont need to check for nullptr either.

while (sensorsReturningFalse && retries > 0) {
sensorsReturningFalse = false;
retries--;
curTime = millis();
Copy link
Member

Choose a reason for hiding this comment

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

Why would curTime be modified here?

return;

uint32_t now = millis();
if (now - _lastFastMs >= 1000) {
Copy link
Member

Choose a reason for hiding this comment

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

1000 is a magic number, please #define what it is at the top of this header.

uint32_t _lastFastMs = 0;

/** Number of samples accumulated since last publish. */
uint32_t _n = 0;
Copy link
Member

Choose a reason for hiding this comment

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

_num_samples

Comment on lines 74 to 76
_n = 0;
_vocSum = 0.0f;
_rawSum = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Move these to the constructor

uint32_t _lastFastMs = 0;

/** Number of samples accumulated since last publish. */
uint32_t _n = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Please change to _sample_num or something more descriptive

if (!iaqEnabled())
return; // nothing enabled, save cycles
uint32_t now = millis();
if (now - _lastFastMs >= 1000) { // ~1 Hz cadence
Copy link
Member

Choose a reason for hiding this comment

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

1000 is a magic number, please #define it.

@mikeysklar
Copy link
Contributor Author

@brentru -

Welcome back. Thank you for the thorough review.

I'll hold off on the improved variable names and getting magic number defined updates until you and @tyeth discuss this.

@tyeth
Copy link
Member

tyeth commented Sep 25, 2025 via email

/*******************************************************************************/
/*!
@brief Instanciates an I2C sensor.
@brief Instanctiates an I2C sensor.
Copy link
Member

Choose a reason for hiding this comment

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

no middle c

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

@mikeysklar Tyeth and I discussed this fix this morning. You are on the right track, but we'd like to remove the averaging code.

For the SGP30,

  • Remove all the code related to averaging values within the driver.
  • fastTick is OK but should only just read the driver. It should set a boolean flag if IAQmeasure succeeded.
  • getEventX() methods should first check the validity of the previous read (return value of IAQmeasure()) from fastTick, and fill the Event if the read was OK.

For the SGP40,

  • Remove all the code related to averaging values within the driver.
  • Add uint16_t _rawValue and int32_t _vocIdx as protected: members
  • fastTick should just poll for raw value and voc inx, and save those into _rawValue and _vocIdx.
  • Within getEventX(), pack _rawValue or _vocIdx into an Event.

I added two comments below to reflect these changes. lmk if you need more info/have more questions, and thanks for taking a look at this. If you'd like Tyeth to take this over as well, let him know.

@mikeysklar
Copy link
Contributor Author

mikeysklar commented Sep 25, 2025

@brentru - Thank you for taking the time to make the above suggested changes. I'll start updating the PR with what you have requested and confirm functionality before asking for another review.

@tyeth - The mixed cohorts of the various sensors do complicate things. My feeling is that if a sensors needs 30 minutes to get its bearings then it should be blocked from sleep. I'm looking at your BME68x. Maybe it makes sense to build on the per sensor JSON configs that WS v2 introduces and have fairly elaborate JSON config knobs.

eg.

{
  "sensor_id": "sgp30-1",
  "class": "A",
  "ramp_up_s": 20,
  "min_poll_s": 1,
  "throwaway_n": 3,
  "commission_time_s": 43200,        // e.g., 12h SGP30 baseline build
  "persist_baseline": true,
  "max_sleep_s": 600,                // 10 min guard for algo continuity
  "keep_powered_in_sleep": true,     // overrides if sample period > max_sleep_s
  "publish_during_ramp": false,      // advanced users can set true
  "quality_threshold": {
    "variance_ppm": 50,              // stop throwing away once stabilized
    "min_samples": 3
  }
}

then rather than fighting for quality data publish the state of the data:

{
  "sensor_id": "sgp30-1",
  "state": "stable|commissioning|warming_up|resume_after_sleep|rebaseline",
  "ramp_elapsed_s": 28,
  "baseline_age_h": 3.2,
  "samples_discarded": 3
}

@mikeysklar
Copy link
Contributor Author

mikeysklar commented Sep 26, 2025

Current state of PR. Testing after averaging / corrections made went fine overnight for 1s, 30s, 1m, 10m intervals checks. I messed with air quality levels and the sensor was very responsive. The only concern I have in terms of functionality is I've pulled the 1Hz timer out of fastTick(). I should probably be put back so the sensor is not getting queried at main loop speed.

Screenshot 2025-09-26 at 6 59 58 AM

@brentru
Copy link
Member

brentru commented Sep 26, 2025

@mikeysklar Sorry, what 1Hz timer?

@mikeysklar
Copy link
Contributor Author

mikeysklar commented Sep 26, 2025

I had a little millis counter with the 1000 magic number in the SGP30 / SGP40 code to poll the sensors at 1Hz. I ripped it up yesterday, but should probably put that back in to prevent constant polling.

    if (now - _lastFastMs >= 1000) { // ~1 Hz cadence

@brentru
Copy link
Member

brentru commented Sep 26, 2025

Oh, yeah, that should go back in

@mikeysklar
Copy link
Contributor Author

mikeysklar commented Sep 28, 2025

I've been testing the SGP40 and it has been running smoothly with different update periods 1s - 10min. I believe I got all the suggested changes in. I ordered a SGP30 for testing which I can confirm functionality this week.

image

@brentru brentru requested review from brentru and tyeth September 29, 2025 13:55
Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

@mikeysklar I only have one change to request. Looks great.

@tyeth I've tagged you for secondary review since this touches the WipperSnapper_I2C class.

@mikeysklar
Copy link
Contributor Author

thanks @tyeth -

I like simple. I'm going to change up the SGP30 to look more like the SGP40 and just use the 1 Hz fastTick for sensor polling. No more consumed cache flag which is confusing.

@mikeysklar
Copy link
Contributor Author

SGP30 arrived today so I'll try running both SGP30 and SGP40 with the latest artifacts and report back.

@mikeysklar
Copy link
Contributor Author

I ran the SGP30 / SGP40 / SCD41 overnight at 5 minute intervals. Data was reliably coming and correlates to some degree. Keep in mind the SGP30 is measuring VOC in (PPB) while the SGP40 has its own index value of 0-500.

sgpxx-voc

Keep in mind that the SGP30 is a new sensor fresh out of the bag so has not been through a 48 hour burn-in period yet.

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

@mikeysklar LGTM! Thank you for adding this PR

@tyeth Please pull this in when you are ready.

@tyeth tyeth merged commit a94544f into adafruit:main Oct 1, 2025
46 checks passed
@tyeth
Copy link
Member

tyeth commented Oct 1, 2025

@mikeysklar released as v1.0.0-beta.117

@mikeysklar
Copy link
Contributor Author

Cool! Thank you @tyeth and @brentru for your patience. Glad to see fastTicks() is already being extended to additional devices.

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