Remove proprietary Bosch BSEC blob; open in-tree IAQ estimator for BME680 - #11381
Conversation
…E680 BSEC2 cost ~37-39 KB flash and ~4-5 KB static RAM on ~190 of ~240 build targets, linked whether or not a BME680 was attached, and was a no-source proprietary archive inside GPLv3 release binaries. The firmware consumed exactly one BSEC-exclusive output: the IAQ value. - New BME680IaqEstimator: clean-room log-domain baseline tracker (humidity-compensated gas resistance vs a rise-fast/decay-slow ceiling, 0-500 scale matching the existing UI bands), pure math, unit-tested on native (test_bme680_iaq, 15 tests incl. a deep-sleep reboot simulation). Warm-up/burn-in progress persists to /prefs/bme680.dat via SafeFile so one-sample-per-wake SENSOR nodes converge across reboots; stale /prefs/bsec.dat is removed once. - BME680Sensor: single-path rewrite on Adafruit_BME680 with async once-per-minute sampling (~20x lower heater duty than BSEC LP mode), a hard 2-minute publish-freshness bound (a dead sensor stops reporting instead of freezing its last reading on the wire), and suppression of bogus gas_resistance=0 points from heater-unstable cycles. - platformio.ini: environmental_extra_common/_extra/_no_bsec collapsed into one section; Bosch BSEC2 + BME68x deps deleted; per-variant BSEC link-path hacks and the TEMPORARY promicro lib_ignore removed. nrf52_promicro_diy_tcxo regains BME680 support at 36 KB clear of the warm-store cap; rak4631 lands at 75 KB clear. - EnvironmentTelemetry: iaq rendering gates on has_iaq (a genuine IAQ of 0 now displays); stale BSEC comments rewritten. - rak4631 size budgets tightened (113000->108000 RAM, 786000->746000 flash) to lock in the reclaimed headroom. - bin/bme680_iaq_replay.cpp: host-side replay harness for tuning the estimator against captured BSEC traces (mean abs error + band agreement), no reflashing needed. Measured (develop -> this branch): rak4631 -38.8 KB flash / -4.9 KB RAM; heltec-v3 -36.4 KB / -4.0 KB; tlora-v2-1-1_6 +1.3 KB (its IAQ approximation had been dead code since #9663 due to an inverted isfinite check and now actually runs). Note: gas_resistance stays kOhm on the wire for fleet compatibility; the proto comment claiming MOhm gets a separate meshtastic/protobufs docs PR.
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR replaces BSEC2-based BME680 IAQ processing with an in-tree estimator. It adds asynchronous sampling, persisted estimator state, telemetry integration, build configuration updates, automated tests, and a host-side CSV replay harness. ChangesBME680 IAQ replacement
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The current change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant EnvironmentTelemetry
participant BME680Sensor
participant AdafruitBME680
participant BME680IaqEstimator
participant SafeFile
EnvironmentTelemetry->>BME680Sensor: request telemetry metrics
BME680Sensor->>AdafruitBME680: start and complete asynchronous reading
AdafruitBME680-->>BME680Sensor: temperature, humidity, pressure, gas resistance
BME680Sensor->>BME680IaqEstimator: update gas resistance and humidity
BME680IaqEstimator-->>BME680Sensor: IAQ value and readiness
BME680Sensor-->>EnvironmentTelemetry: cached metrics and IAQ presence
BME680Sensor->>SafeFile: serialize and atomically save estimator state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/modules/Telemetry/Sensor/BME680Sensor.h (1)
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the file-name members static.
stateFileNameandlegacyBsecStateFileNameare instance members, so each holds a pointer in RAM and is initialized at construction. Declare themstatic constexpr const char *to move them out of the object. This PR tightens therak4631RAM budget, so the saving is aligned with the goal.♻️ Proposed change
- const char *stateFileName = "/prefs/bme680.dat"; - const char *legacyBsecStateFileName = "/prefs/bsec.dat"; // left behind by pre-open-IAQ firmware + static constexpr const char *stateFileName = "/prefs/bme680.dat"; + // left behind by pre-open-IAQ firmware + static constexpr const char *legacyBsecStateFileName = "/prefs/bsec.dat";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/Telemetry/Sensor/BME680Sensor.h` around lines 33 - 34, Update the BME680Sensor file-name members stateFileName and legacyBsecStateFileName to static constexpr const char * declarations, preserving their existing string values and removing per-instance storage.src/modules/Telemetry/Sensor/BME680IaqEstimator.cpp (1)
6-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
meshUtils.h’sclamphelper instead of duplicatingclampf.
src/meshUtils.halready provides a C++17clamptemplate, and other native-tested sources use it directly. Replaceclampfwithclampfrom the existing repository helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/Telemetry/Sensor/BME680IaqEstimator.cpp` around lines 6 - 9, Remove the local clampf helper and update its call sites in the BME680 IAQ estimator to use the existing clamp template from meshUtils.h. Include the appropriate helper header and preserve the current lower and upper bounds.Source: Coding guidelines
bin/bme680_iaq_replay.cpp (1)
1-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the replay workflow out of the source file.
This 23-line block contains a multi-paragraph build, capture, input, and output guide. Keep a one- or two-line usage summary here and move the full workflow to a README or tool document.
As per coding guidelines, code comments must be minimal, normally one or two lines, and must not contain multi-paragraph explanatory blocks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/bme680_iaq_replay.cpp` around lines 1 - 23, Reduce the file-level comment above the replay harness to a one- or two-line usage summary, retaining only its purpose and basic invocation. Move the detailed build, input format, trace-capture procedure, and output description to an appropriate README or tool document, without changing the replay implementation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/bme680_iaq_replay.cpp`:
- Around line 68-75: Update the replay input loop around fgets and sscanf to
distinguish comment/header lines from malformed data, report each skipped
malformed row (including its row number or content), and track the input stream
with ferror(in) after the loop. Return a non-zero status when a read error
occurs while preserving normal EOF and valid-row processing.
- Around line 27-29: The printf formatting for iaq in the replay output is
incompatible with its uint16_t type. Update the relevant format string and
argument in the replay code to use a matching representation, either casting iaq
to uintmax_t with %ju or using %hu, and include <cstdint> as needed.
In `@src/modules/Telemetry/Sensor/BME680Sensor.cpp`:
- Around line 56-57: Replace the raw elapsed-time rate-limit predicates in
BME680Sensor with Throttle::isWithinTimespanMs, covering the sampling check near
haveSample, IAQ carry check, sample-freshness checks, and state-save check. Keep
the existing subtraction in the sampling branch to calculate the returned
remaining delay, and leave the absolute beginReading deadline calculations
unchanged.
---
Nitpick comments:
In `@bin/bme680_iaq_replay.cpp`:
- Around line 1-23: Reduce the file-level comment above the replay harness to a
one- or two-line usage summary, retaining only its purpose and basic invocation.
Move the detailed build, input format, trace-capture procedure, and output
description to an appropriate README or tool document, without changing the
replay implementation.
In `@src/modules/Telemetry/Sensor/BME680IaqEstimator.cpp`:
- Around line 6-9: Remove the local clampf helper and update its call sites in
the BME680 IAQ estimator to use the existing clamp template from meshUtils.h.
Include the appropriate helper header and preserve the current lower and upper
bounds.
In `@src/modules/Telemetry/Sensor/BME680Sensor.h`:
- Around line 33-34: Update the BME680Sensor file-name members stateFileName and
legacyBsecStateFileName to static constexpr const char * declarations,
preserving their existing string values and removing per-instance storage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a56c69db-fdac-4298-a11a-cf23924a2628
📒 Files selected for processing (15)
bin/bme680_iaq_replay.cppbin/ram_budgets.jsonplatformio.inisrc/modules/Telemetry/EnvironmentTelemetry.cppsrc/modules/Telemetry/Sensor/BME680IaqEstimator.cppsrc/modules/Telemetry/Sensor/BME680IaqEstimator.hsrc/modules/Telemetry/Sensor/BME680Sensor.cppsrc/modules/Telemetry/Sensor/BME680Sensor.htest/native-suite-counttest/test_bme680_iaq/test_main.cppvariants/esp32/esp32.inivariants/esp32p4/esp32p4.inivariants/nrf52840/ELECROW-ThinkNode-M3/platformio.inivariants/nrf52840/diy/nrf52_promicro_diy_tcxo/platformio.inivariants/nrf52840/muzi_base/platformio.ini
💤 Files with no reviewable changes (4)
- variants/nrf52840/diy/nrf52_promicro_diy_tcxo/platformio.ini
- variants/nrf52840/ELECROW-ThinkNode-M3/platformio.ini
- variants/nrf52840/muzi_base/platformio.ini
- variants/esp32p4/esp32p4.ini
- Use Throttle::isWithinTimespanMs for all elapsed-time predicates in BME680Sensor per coding guidelines (deadline math for the async reading completion stays raw, as it targets an absolute timestamp) - Make the state file name members static constexpr - Replay tool: cast uint16_t before %u (default argument promotion), report malformed input lines instead of silently skipping, and fail non-zero on stream read errors
- Replace the local clampf helper with std::clamp (meshUtils.h's clamp drags in Arduino.h, which would break the estimator's standalone host build that the replay harness depends on) - Trim the replay tool's file header to a two-line summary; the full build, capture, and tuning workflow moves to docs/bme680_iaq_replay.md
…emove-bsec # Conflicts: # platformio.ini # src/modules/Telemetry/Sensor/BME680Sensor.cpp # test/native-suite-count
# Conflicts: # src/modules/Telemetry/Sensor/BME680Sensor.cpp # src/modules/Telemetry/Sensor/BME680Sensor.h
Hardware A/B: open estimator vs. BSEC on two RAK4631 + BME68014-hour side-by-side soak, MethodTwo RAK4631s with BME680 at 0x76:
The number that matters is the replay, not the live A/B: device A's own logged readings are fed offline through Two resampling details that decide whether the replay is valid at all:
Size claim: independently confirmedSame board, same toolchain, from the linker:
Matches the ~37–39 KB / ~4–5 KB in the PR description. (A carries the trace patch, so the true BSEC-only flash delta is a hair under 39,888 B. Note a UF2 size delta is 2× the flash delta — UF2 carries 256 payload bytes per 512-byte block — so don't quote UF2 bytes.) What passedResponsiveness — r = 0.92 across 93 scored rows. A dry-VOC event (IPA gauze) took the estimator 13 → 430 while BSEC went 107 → 500: No false alarms in clean air — 718 overnight rows, max IAQ 7, zero excursions above the 150 banner or 200 buzzer thresholds. Baseline resists sustained pollution — unplanned, but I left the source in place for 45 minutes: the estimator held at 242 → 255 rather than normalizing the pollution away. Burn-in and persistence work on real hardware — first output at 33 samples as specified, state written to What did not pass, and why I don't think the numbers mean what they look likeMean absolute error 123.1 (target < 50). Band agreement 23.7% (target ≥ 80%). Two reasons to discount those specific figures:
The real finding is that the disagreement is a shape mismatch, not a scale error:
The curves converge under heavy pollution and diverge at the clean end — which is where users live and where the 150/200 thresholds sit. No single Whether that's worth chasing is a judgement call I'd rather surface than silently optimize toward: the estimator is relative-to-baseline by design, and it demonstrably responds to real pollution, resists sustained pollution, and never false-alarmed in 13 hours of clean air. If the goal is "usable IAQ without the blob," matching BSEC's absolute banding may be the wrong target. One real bug found, with a fixA humid transient could ratchet the clean-air baseline upward.
Measured, from this run — a breath event at 07:20 (RH 41.9 → 56.5, raw gas −26%): Compensated gas went 24% above the pre-event baseline during a pollution event. Humidity compensation normalizes a reading for comparison; it should not be able to mint a new "cleanest air seen" record. Fix — gate upward ceiling movement on the direction of the raw change: const bool rawImproved = !haveLastRaw || xRaw >= lastXRaw;
const float alpha = (x > lnCeiling && rawImproved) ? ALPHA_UP : ALPHA_DOWN;
lnCeiling = std::clamp(lnCeiling + alpha * (x - lnCeiling), LN_FLOOR, LN_CEIL_MAX);Gating on direction rather than clamping the ceiling to the raw value matters: clamping mixes compensated and uncompensated space and would under-report by ~65 IAQ points at any steadily elevated RH. The guard state is deliberately not persisted — Replayed against the same captured trace, post-event false elevation drops 13 → 6 and the IPA response is preserved (436 → 430). It halves rather than eliminates the effect: raw gas is also recovering during the tail of a humid event, so some legitimate-by-this-rule rise remains. Given the residual is ~6 points against a 150 threshold, I stopped there rather than adding more state. Two regression tests added to Incidental
Reproducing
Caveats worth stating plainly: single sensor pair, one room, one night, 93 scored rows, and the strongest event saturated the reference. The pass/fail items above I'd consider settled; the absolute-agreement numbers I would not, until there's a sweep that holds BSEC in the 200–350 range without pegging. |
3 participants
Why
The Bosch BSEC2 blob is one of our largest dependencies: measured ~37-39 KB flash and ~4-5 KB static RAM on every image that links it (~190 of ~240 targets), paid whether or not a BME680 is attached. It's also a no-source proprietary archive statically linked into GPLv3 release binaries, and it's been the recurring culprit in our nRF52 flash-pressure incidents (#11363 left a "TEMPORARY"
lib_ignorehack onnrf52_promicro_diy_tcxowith a TODO to make the roster opt-in).The firmware only ever consumed one BSEC-exclusive output: the
iaqvalue. Temperature, humidity, pressure, and gas resistance all come from the plain sensor.What
environmental_extra_common/environmental_extra/environmental_extra_no_bseccollapse into a singleenvironmental_extra(common sensors + Adafruit_BME680 2.0.6, which vendors Bosch's open BSD-3bme68xdriver). The per-variant BSEC link-path hacks (ThinkNode-M3, muzi_base) and the promicrolib_ignoreblock are gone.BME680IaqEstimator): humidity-compensated log-resistance tracked against a rise-fast/decay-slow rolling ceiling, scored onto the same 0-500 scale and bands the UI already uses. Pure math, no platform dependencies, fully unit-tested on native (15 tests, including a deep-sleep reboot-cycle simulation). Warm-up/burn-in progress persists to/prefs/bme680.dat(atomic SafeFile), so one-sample-per-wake SENSOR nodes converge across reboots; the orphaned/prefs/bsec.datis deleted once.gas_resistance=0points from heater-unstable cycles.iaqrendering now gates onhas_iaq, so a legitimate IAQ of 0 displays instead of vanishing. Alert thresholds (banner >150, beep >200) unchanged.bin/bme680_iaq_replay.cppreplays a CSV captured from a BSEC build through the estimator on a dev machine and reports mean absolute error + UI-band agreement, so the constants can be tuned against real BSEC traces without reflashing (capture instructions in the file header).Measured (develop → this branch, same commit)
rak4631heltec-v3nrf52_promicro_diy_tcxotlora-v2-1-1_6Behavior changes (release-notes material)
isfinitecheck made it dead code). Those targets now report IAQ for the first time.gas_resistancestays kOhm on the wire for fleet continuity; the proto comment claiming MOhm gets a separate docs fix in meshtastic/protobufs.Testing
test_bme680_iaq(new, 15 cases) passes; full native suite shows no regressions vs develop (the only failure,test_packet_signingB11/B12, fails identically on clean develop).Hardware soak on a RAK4631+RAK1906 against a BSEC-build reference node is the remaining validation; the estimator constants are centralized and documented as tunables for exactly that.
Summary by CodeRabbit
New Features
Documentation
Chores