[adc] Fix autorange negative coefficient bug causing incorrect voltage readings - #10549
Merged
Conversation
Contributor
|
To use the changes from this PR as an external component, add the following to your ESPHome configuration YAML file: external_components:
- source: github://pr#10549
components: [adc]
refresh: 1h(Added by the PR bot) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #10549 +/- ##
=======================================
Coverage 61.00% 61.00%
=======================================
Files 52 52
Lines 10504 10504
Branches 1400 1400
=======================================
Hits 6408 6408
Misses 3738 3738
Partials 358 358 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…e readings ## What does this implement/fix? Fixes a critical bug in the ESP32 ADC autorange implementation where negative coefficient calculations wrap around when assigned to unsigned integers, causing extremely incorrect voltage readings (millions of volts instead of expected 1-3V range). ## The Problem The autorange algorithm calculates weighting coefficients to combine readings from different attenuation levels: ```cpp uint32_t c6 = adc_half - std::abs(raw6 - adc_half); ``` When the raw ADC reading exceeds `adc_half + adc_half` (i.e., > 4096), this calculation becomes negative. Since the coefficient is stored as `uint32_t`, the negative value wraps around to ~4.3 billion, completely breaking the weighted average calculation. **Example with user's data:** - `raw6 = 6157`, `adc_half = 2048` - `2048 - abs(6157 - 2048) = 2048 - 4109 = -2061` - As `uint32_t`: `-2061` → `4,294,965,235` (negative converted to huge positive) - Final coefficient sum becomes ~4.3 billion instead of reasonable values ## The Solution Use signed arithmetic for the calculation and clamp negative results to zero: ```cpp int32_t c6_signed = adc_half - std::abs(raw6 - adc_half); uint32_t c6 = std::max(0, c6_signed); ``` This ensures that unreliable readings (far from the optimal range) get zero weight instead of maximum weight. ## Root Cause This bug was introduced during the ESP-IDF v5 migration (PR esphome#9021) when the ADC implementation was rewritten to use the new oneshot API. The previous legacy implementation didn't have this issue. ## Testing - ✅ Fixes the reported case where autorange returned 30+ million volts - ✅ Fixed attenuation (6db) works correctly: ~1.036V - ✅ Raw ADC readings are normal: 3252/4095 - ✅ After fix, autorange should return values close to fixed attenuation readings **Related issue:** esphome#10522
edwardtfn
marked this pull request as ready for review
September 8, 2025 15:19
jesserockz
approved these changes
Sep 9, 2025
Merged
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
What does this implement/fix?
Fixes a critical bug in the ESP32 ADC autorange implementation where negative coefficient calculations wrap around when assigned to unsigned integers, causing extremely incorrect voltage readings (millions of volts instead of expected 1-3V range).
The Problem
The autorange algorithm calculates weighting coefficients to combine readings from different attenuation levels:
uint32_t c6 = adc_half - std::abs(raw6 - adc_half);When the raw ADC reading exceeds
adc_half + adc_half(i.e., > 4096), this calculation becomes negative. Since the coefficient is stored asuint32_t, the negative value wraps around to ~4.3 billion, completely breaking the weighted average calculation.Example with user's data:
raw6 = 6157,adc_half = 20482048 - abs(6157 - 2048) = 2048 - 4109 = -2061uint32_t:-2061→4,294,965,235(negative converted to huge positive)The Solution
Use signed arithmetic for the calculation and clamp negative results to zero:
This ensures that unreliable readings (far from the optimal range) get zero weight instead of maximum weight.
Root Cause
This bug was introduced during the ESP-IDF v5 migration (PR #9021) when the ADC implementation was rewritten to use the new oneshot API. The previous legacy implementation didn't have this issue.
Types of changes
Related issue or feature (if applicable):
Pull request in esphome-docs with documentation (if applicable): N/A
Test Environment
Example entry for
config.yaml: N/AChecklist:
tests/folder).If user exposed functionality or configuration variables are added/changed: