Adding ADC manager in preparation for AR update to IDF V5 - #5773
Conversation
WalkthroughAdds an ESP32-only ChangesESP32 ADC management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant WLEDAdcManager
participant ESP32ADCDriver
Caller->>WLEDAdcManager: begin(pin, sampleRateHz, samplesPerFrame)
WLEDAdcManager->>ESP32ADCDriver: Configure and start ADC1
Caller->>WLEDAdcManager: readSamples(buffer, numSamples, timeoutMs)
WLEDAdcManager->>ESP32ADCDriver: Read sample frames
ESP32ADCDriver-->>WLEDAdcManager: Return ADC samples
WLEDAdcManager-->>Caller: Return cached and continuous samples
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 6
🧹 Nitpick comments (4)
lib/wled_ADCmanager/library.json (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider restricting the library to ESP32 platforms.
The manifest has no
platformsfield, so PlatformIO builds this library for ESP8266 environments too. The source compiles to nothing there because of theARDUINO_ARCH_ESP32guard, but declaring the platform makes the intent explicit and skips the compile step.♻️ Proposed manifest addition
{ "name": "wled-ADCmanager", + "platforms": "espressif32", "build": { "libArchive": false } }🤖 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 `@lib/wled_ADCmanager/library.json` around lines 1 - 4, Update the library manifest near the existing build configuration to declare an ESP32-only platform restriction using the manifest’s platforms field. Preserve the current library name and libArchive setting.lib/wled_ADCmanager/wled_ADCmanager.h (1)
40-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up the commented-out declaration and fix the indentation.
Line 40 keeps a disabled
checkADC()declaration. Line 43 has no indentation. The project requires 2-space indentation and removal of dead code.♻️ Proposed cleanup
int analogRead(uint8_t pin); int analogReadMilliVolts(uint8_t pin); - //void checkADC(); // check ADC status, reset if overflow happened (watchdog function, needs to be called frequently if used, i.e. put this in main loop) private: -WLEDAdcManager(); + WLEDAdcManager(); ~WLEDAdcManager();As per coding guidelines: "Use 2-space indentation and no tabs in C++ files" and "Remove dead or unused code".
🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.h` around lines 40 - 44, Remove the commented-out checkADC declaration and indent the private constructor and destructor declarations by two spaces within the WLEDAdcManager class.Source: Coding guidelines
lib/wled_ADCmanager/wled_ADCmanager.cpp (2)
297-320: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCheck the return value of
adc_oneshot_config_channel().Line 307 discards the return value. If the configuration fails,
adc_oneshot_read()runs on an unconfigured channel and the caller receives a value that looks valid. Return false on failure and delete the unit.♻️ Proposed change
- adc_oneshot_config_channel(h, ch, &ccfg); + if (adc_oneshot_config_channel(h, ch, &ccfg) != ESP_OK) { + adc_oneshot_del_unit(h); + return false; + }🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.cpp` around lines 297 - 320, Update _oneshotRead to check the result of adc_oneshot_config_channel() before calling adc_oneshot_read(); on configuration failure, delete the ADC unit with adc_oneshot_del_unit(h) and return false so no unconfigured read occurs.
153-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead code and the unused variables.
Three items are dead:
- Lines 153-172: the commented-out overflow callback and
checkADC()implementation. The header comment on line 20 states the workaround is no longer needed.- Lines 353-354: an empty
#if SOC_ADC_DIG_IIR_FILTER_SUPPORTED/#endifpair.- Lines 357-359:
result_mvis never read, andchis assigned but never used.analogRead(pin)performs its own pin validation, so only the return check is needed.
ContinuousCtx::pin(line 56) is also written inbegin()and never read.♻️ Proposed cleanup for `analogReadMilliVolts()`
int WLEDAdcManager::analogReadMilliVolts(uint8_t pin) { - int result_mv = 0; - adc_channel_t ch; - if (!_pinToChannel(pin, &ch)) return 0; + adc_channel_t ch; + if (!_pinToChannel(pin, &ch)) return 0; // reject non-ADC1 pins before reading int raw = analogRead(pin);As per coding guidelines: "Remove dead or unused code". As per path instructions: "CHECK for singleton data (defined but never used) and for dead/disabled code, and suggest to remove them."
Also applies to: 353-354, 356-360
🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.cpp` around lines 153 - 172, Remove the obsolete commented overflow callback and checkADC implementation, the empty SOC_ADC_DIG_IIR_FILTER_SUPPORTED conditional, and unused result_mv/ch declarations in analogReadMilliVolts(), retaining only the analogRead(pin) return check. Remove ContinuousCtx::pin and its assignment in begin() since it is never read.Sources: Coding guidelines, Path instructions
🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.cpp`:
- Around line 327-337: Update analogRead() to check the return value of
_initContinuousADC() after restarting continuous sampling and handle failure by
preventing subsequent use of the invalid continuous context. Add a defensive
null check for _ctx->handle in readSamples() before calling
adc_continuous_read(), returning through the existing safe failure path when the
handle is unavailable.
- Around line 220-233: Update WLEDAdcManager::_drainToCache() to preserve unread
samples already stored in _ctx->cache: remove the unconditional reset of
_ctx->cacheCount and append newly drained samples from the current count, while
retaining the existing cache-size bounds.
- Around line 48-49: Update the ESP32-C6 branch in the ADC pin validation logic
to use the ADC channel range, accepting channels 0 through 6 inclusive. Replace
the current pin-based limit in the relevant conditional while preserving the
existing handling of ch and other target branches.
- Line 242: Replace the unbounded _mutex waits in readSamples()
(lib/wled_ADCmanager/wled_ADCmanager.cpp:242-242), begin() (95-95), end()
(143-143), and analogRead() (327-327) with bounded waits, checking _mutex for
null before each take; return 0 from readSamples() and analogRead(), return
false from begin(), and return early from end() when acquisition fails.
- Around line 340-351: Update WLEDAdcManager::_initCali() to support
ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED by creating _cali with
adc_cali_create_scheme_curve_fitting(), while preserving the existing
line-fitting path as appropriate. In the WLEDAdcManager destructor, release
curve-fitting calibration handles with
adc_cali_delete_scheme_curve_fitting(_cali), ensuring the scheme-specific
cleanup matches the creation path.
- Around line 177-182: Update the adc_continuous_handle_cfg_t initialization in
the ADC manager to make max_store_buf_size consistent with the documented
two-frame capacity, using frameBytes * 2 and ensuring the result exceeds
ADCMANAGER_DMA_BLOCKSIZE for small frames. Correct the conv_frame_size comment
to remove the incorrect 256-byte claim and state the actual 128-byte block size.
---
Nitpick comments:
In `@lib/wled_ADCmanager/library.json`:
- Around line 1-4: Update the library manifest near the existing build
configuration to declare an ESP32-only platform restriction using the manifest’s
platforms field. Preserve the current library name and libArchive setting.
In `@lib/wled_ADCmanager/wled_ADCmanager.cpp`:
- Around line 297-320: Update _oneshotRead to check the result of
adc_oneshot_config_channel() before calling adc_oneshot_read(); on configuration
failure, delete the ADC unit with adc_oneshot_del_unit(h) and return false so no
unconfigured read occurs.
- Around line 153-172: Remove the obsolete commented overflow callback and
checkADC implementation, the empty SOC_ADC_DIG_IIR_FILTER_SUPPORTED conditional,
and unused result_mv/ch declarations in analogReadMilliVolts(), retaining only
the analogRead(pin) return check. Remove ContinuousCtx::pin and its assignment
in begin() since it is never read.
In `@lib/wled_ADCmanager/wled_ADCmanager.h`:
- Around line 40-44: Remove the commented-out checkADC declaration and indent
the private constructor and destructor declarations by two spaces within the
WLEDAdcManager class.
🪄 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: fb8a355f-c017-4a73-a330-95f8519821a4
📒 Files selected for processing (4)
lib/wled_ADCmanager/library.jsonlib/wled_ADCmanager/wled_ADCmanager.cpplib/wled_ADCmanager/wled_ADCmanager.hwled00/wled.h
| #elif defined(CONFIG_IDF_TARGET_ESP32C6) | ||
| (void)ch; return (pin <= 5); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the C6 analog channel table in the installed framework, if present
fd -i 'esp32c6' -t d | head
rg -n 'ADC1_CHANNEL|analog_channel' --iglob '*esp32c6*' -g '*.h' -C2 | head -50Repository: wled/WLED
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'lib/wled_ADCmanager/wled_ADCmanager\.(cpp|h)$|wled_ADCmanager' || true
echo "== file excerpt =="
if [ -f lib/wled_ADCmanager/wled_ADCmanager.cpp ]; then
wc -l lib/wled_ADCmanager/wled_ADCmanager.cpp
sed -n '1,140p' lib/wled_ADCmanager/wled_ADCmanager.cpp | cat -n
fi
echo "== related snippets =="
rg -n '`#elif` defined\(CONFIG_IDF_TARGET_ESP32C6\)|CONFIG_IDF_TARGET_ESP32C6|esp32c6|pin <=|pin <' . \
-g '*.cpp' -g '*.h' -g '*.ino' -C 3 | head -200Repository: wled/WLED
Length of output: 21118
🌐 Web query:
ESP32-C6 ADC1 channels GPIO 0 to 6 technical reference manual
💡 Result:
The ESP32-C6 features a single 12-bit SAR ADC peripheral [1]. GPIO pins 0 through 6 are explicitly mapped to ADC1 channels 0 through 6, respectively [2][3][4][1]. The mapping is as follows: - GPIO0: ADC1_CH0 [2][3][4][1] - GPIO1: ADC1_CH1 [2][3][4][1] - GPIO2: ADC1_CH2 [2][3][4][1] - GPIO3: ADC1_CH3 [2][3][4][1] - GPIO4: ADC1_CH4 [2][3][4][1] - GPIO5: ADC1_CH5 [2][3][4][1] - GPIO6: ADC1_CH6 [2][3][4][1] Note that GPIO4 and GPIO5 are also strapping pins, which may influence their behavior during boot [2]. For optimal ADC accuracy, it is recommended to add a 0.1 µF filter capacitor between these pins and ground [3][4]. While these pins are defined as ADC1 channels in the ESP-IDF software environment (e.g., in adc_channel.h) [5][6][7], the physical routing for these analog functions is detailed in the ESP32-C6 Datasheet and Hardware Design Guidelines [3][1].
Citations:
- 1: https://www.espressif.com/sites/default/files/documentation/esp32-c6_datasheet_en.pdf
- 2: https://docs.espressif.com/projects/esp-idf/en/stable/esp32c6/api-reference/peripherals/gpio.html
- 3: https://docs.espressif.com/projects/esp-hardware-design-guidelines/en/latest/esp32c6/esp-hardware-design-guidelines-en-master-esp32c6.pdf
- 4: https://docs.espressif.com/projects/esp-hardware-design-guidelines/en/latest/esp32c6/schematic-checklist.html
- 5: https://github.com/espressif/esp-idf/blob/master/components/soc/esp32c6/include/soc/adc_channel.h
- 6: https://github.com/espressif/esp-idf/blob/v6.0.2/components/soc/esp32c6/include/soc/adc_channel.h
- 7: https://docs.espressif.com/projects/esp-idf/en/stable/esp32c6/api-reference/peripherals/adc/index.html
Use the ESP32-C6 ADC channel range instead of raw pin numbers.
ESP32-C6 ADC1 includes GPIO0 to GPIO6 as channels 0 to 6, so pin <= 5 rejects GPIO6. Also, the ESP32-C3 branch uses the raw pin range rather than channels, so use the same channel-based check here as for ESP32-S2/S3/C3 to match platform behavior.
🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.cpp` around lines 48 - 49, Update the
ESP32-C6 branch in the ADC pin validation logic to use the ADC channel range,
accepting channels 0 through 6 inclusive. Replace the current pin-based limit in
the relevant conditional while preserving the existing handling of ch and other
target branches.
| size_t frameBytes = (size_t)_ctx->samplesPerFrame * sizeof(adc_digi_output_data_t); | ||
| adc_continuous_handle_cfg_t hcfg = { | ||
| .max_store_buf_size = frameBytes * 1, // hold two frames in buffer, caller needs to drain it fast enough to avoid data loss | ||
| .conv_frame_size = ADCMANAGER_DMA_BLOCKSIZE, // use fixed DMA buffer size of 256 bytes (ADC driver creates 5 DMA descriptors with one buffer each, at 20kHz this means an interrupt every 1.4ms | ||
| .flags = { .flush_pool = false }, // do not flush the store buffer on overrun but discard new samples (true means discard oldest, is much slower and can cause issues, do not set true) | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the ADC store pool match the documented frame count.
max_store_buf_size is set to one frame (frameBytes), but the comment says it holds two frames. Keep one frame unless callers are expected to always drain before the next frame arrives. If two frames are needed, set max_store_buf_size to frameBytes * 2 and ensure it is above ADCMANAGER_DMA_BLOCKSIZE for small frame sizes.
Also remove the incorrect "256 bytes" claim; ADCMANAGER_DMA_BLOCKSIZE is 128.
🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.cpp` around lines 177 - 182, Update the
adc_continuous_handle_cfg_t initialization in the ADC manager to make
max_store_buf_size consistent with the documented two-frame capacity, using
frameBytes * 2 and ensuring the result exceeds ADCMANAGER_DMA_BLOCKSIZE for
small frames. Correct the conv_frame_size comment to remove the incorrect
256-byte claim and state the actual 128-byte block size.
| void WLEDAdcManager::_drainToCache() { | ||
| if (!_ctx || !_ctx->handle || !_ctx->cache) return; // safety check | ||
| adc_digi_output_data_t temp[32]; // size of data packets to request, 32 samples at 22kHz is 1.5ms, leftover samples are lost | ||
| _ctx->cacheCount = 0; | ||
| while (_ctx->cacheCount < _ctx->cacheSize) { | ||
| uint32_t n = 0; | ||
| // read what is available in the buffer in chunks (no timeout means do not wait for any additional samples) | ||
| if (adc_continuous_read(_ctx->handle, (uint8_t*)temp, sizeof(temp), &n, 0) != ESP_OK || n == 0) break; | ||
| uint16_t cnt = n / sizeof(adc_digi_output_data_t); | ||
| for (uint16_t i = 0; i < cnt && _ctx->cacheCount < _ctx->cacheSize; i++) { | ||
| _ctx->cache[_ctx->cacheCount++] = (int16_t)(temp[i].WLED_ADC_OUT_TYPE.data); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
_drainToCache() discards samples that the caller has not read yet.
Line 223 resets _ctx->cacheCount to 0 before draining. If analogRead() runs twice before readSamples() runs, the first drain result is lost. readSamples() also leaves a partial remainder in the cache, and the next analogRead() destroys it. Append to the cache instead of clearing it.
🐛 Proposed fix
void WLEDAdcManager::_drainToCache() {
if (!_ctx || !_ctx->handle || !_ctx->cache) return; // safety check
adc_digi_output_data_t temp[32]; // size of data packets to request, 32 samples at 22kHz is 1.5ms, leftover samples are lost
- _ctx->cacheCount = 0;
while (_ctx->cacheCount < _ctx->cacheSize) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void WLEDAdcManager::_drainToCache() { | |
| if (!_ctx || !_ctx->handle || !_ctx->cache) return; // safety check | |
| adc_digi_output_data_t temp[32]; // size of data packets to request, 32 samples at 22kHz is 1.5ms, leftover samples are lost | |
| _ctx->cacheCount = 0; | |
| while (_ctx->cacheCount < _ctx->cacheSize) { | |
| uint32_t n = 0; | |
| // read what is available in the buffer in chunks (no timeout means do not wait for any additional samples) | |
| if (adc_continuous_read(_ctx->handle, (uint8_t*)temp, sizeof(temp), &n, 0) != ESP_OK || n == 0) break; | |
| uint16_t cnt = n / sizeof(adc_digi_output_data_t); | |
| for (uint16_t i = 0; i < cnt && _ctx->cacheCount < _ctx->cacheSize; i++) { | |
| _ctx->cache[_ctx->cacheCount++] = (int16_t)(temp[i].WLED_ADC_OUT_TYPE.data); | |
| } | |
| } | |
| } | |
| void WLEDAdcManager::_drainToCache() { | |
| if (!_ctx || !_ctx->handle || !_ctx->cache) return; // safety check | |
| adc_digi_output_data_t temp[32]; // size of data packets to request, 32 samples at 22kHz is 1.5ms, leftover samples are lost | |
| while (_ctx->cacheCount < _ctx->cacheSize) { | |
| uint32_t n = 0; | |
| // read what is available in the buffer in chunks (no timeout means do not wait for any additional samples) | |
| if (adc_continuous_read(_ctx->handle, (uint8_t*)temp, sizeof(temp), &n, 0) != ESP_OK || n == 0) break; | |
| uint16_t cnt = n / sizeof(adc_digi_output_data_t); | |
| for (uint16_t i = 0; i < cnt && _ctx->cacheCount < _ctx->cacheSize; i++) { | |
| _ctx->cache[_ctx->cacheCount++] = (int16_t)(temp[i].WLED_ADC_OUT_TYPE.data); | |
| } | |
| } | |
| } |
🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.cpp` around lines 220 - 233, Update
WLEDAdcManager::_drainToCache() to preserve unread samples already stored in
_ctx->cache: remove the unconditional reset of _ctx->cacheCount and append newly
drained samples from the current count, while retaining the existing cache-size
bounds.
| uint16_t WLEDAdcManager::readSamples(int16_t* buffer, uint16_t numSamples, uint32_t timeoutMs) { | ||
| if (!_ctx || !buffer || !numSamples) return 0; | ||
| // note: DMA uses SOC_ADC_DIGI_MAX_BITWIDTH which is 12bits on all checked units TODO: should make sure and handle this to future proof it | ||
| xSemaphoreTake(_mutex, portMAX_DELAY); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace the unbounded mutex waits with bounded waits.
All four lock acquisitions use portMAX_DELAY. The project guidelines forbid this pattern and require graceful timeout handling. analogRead() holds the mutex across a one-shot read plus a full continuous-mode teardown and restart, which the file header documents as 0.7 ms to 2.5 ms. A caller on the audio task or the web-server task therefore blocks for that whole period. The header comment on line 21 also records a crash in this area, which makes bounded waits and an explicit failure path more valuable.
lib/wled_ADCmanager/wled_ADCmanager.cpp#L242-L242: use a bounded wait inreadSamples()and return 0 when the take fails.lib/wled_ADCmanager/wled_ADCmanager.cpp#L95-L95: use a bounded wait inbegin()and return false when the take fails.lib/wled_ADCmanager/wled_ADCmanager.cpp#L143-L143: use a bounded wait inend()and return early when the take fails.lib/wled_ADCmanager/wled_ADCmanager.cpp#L327-L327: use a bounded wait inanalogRead()and return 0 when the take fails.
_mutex can also be nullptr if xSemaphoreCreateMutex() fails in the constructor. Check it before every take.
🔒️ Proposed pattern
+#define ADCMANAGER_LOCK_TIMEOUT_MS 50
+
+// returns true when the mutex was acquired
+static inline bool adcLock(SemaphoreHandle_t m) {
+ return m && xSemaphoreTake(m, pdMS_TO_TICKS(ADCMANAGER_LOCK_TIMEOUT_MS)) == pdTRUE;
+}Then at each call site, for example in readSamples():
- xSemaphoreTake(_mutex, portMAX_DELAY);
+ if (!adcLock(_mutex)) return 0;As per coding guidelines: "do not wait on mutexes with portMAX_DELAY; handle timeouts gracefully".
📍 Affects 1 file
lib/wled_ADCmanager/wled_ADCmanager.cpp#L242-L242(this comment)lib/wled_ADCmanager/wled_ADCmanager.cpp#L95-L95lib/wled_ADCmanager/wled_ADCmanager.cpp#L143-L143lib/wled_ADCmanager/wled_ADCmanager.cpp#L327-L327
🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.cpp` at line 242, Replace the unbounded
_mutex waits in readSamples() (lib/wled_ADCmanager/wled_ADCmanager.cpp:242-242),
begin() (95-95), end() (143-143), and analogRead() (327-327) with bounded waits,
checking _mutex for null before each take; return 0 from readSamples() and
analogRead(), return false from begin(), and return early from end() when
acquisition fails.
Source: Coding guidelines
| xSemaphoreTake(_mutex, portMAX_DELAY); | ||
| if (_ctx) { // continuous sampling is used | ||
| _drainToCache(); | ||
| _endContinuousADC(); // stop sampling and free the ADC hardware if in use | ||
| _oneshotRead(ch, &raw); | ||
| _initContinuousADC(); // re-init and start sampling again | ||
| } else { | ||
| _oneshotRead(ch, &raw); | ||
| } | ||
| xSemaphoreGive(_mutex); | ||
| return raw; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
analogRead() ignores a failed restart of continuous sampling.
Line 332 discards the return value of _initContinuousADC(). On failure, _ctx stays non-null while _ctx->handle is nullptr. readSamples() then passes that null handle to adc_continuous_read() at line 268, which aborts on the IDF handle assertion. Handle the failure here, and add a defensive handle check in readSamples().
🐛 Proposed fix
_drainToCache();
_endContinuousADC(); // stop sampling and free the ADC hardware if in use
_oneshotRead(ch, &raw);
- _initContinuousADC(); // re-init and start sampling again
+ if (!_initContinuousADC()) { // re-init and start sampling again
+ DEBUG_PRINTLN(F("ADC: failed to restart continuous sampling"));
+ }Guard in readSamples() (line 240):
- if (!_ctx || !buffer || !numSamples) return 0;
+ if (!buffer || !numSamples) return 0;and after taking the mutex:
+ if (!_ctx || !_ctx->handle) { xSemaphoreGive(_mutex); return 0; }🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.cpp` around lines 327 - 337, Update
analogRead() to check the return value of _initContinuousADC() after restarting
continuous sampling and handle failure by preventing subsequent use of the
invalid continuous context. Add a defensive null check for _ctx->handle in
readSamples() before calling adc_continuous_read(), returning through the
existing safe failure path when the handle is unavailable.
| bool WLEDAdcManager::_initCali() { | ||
| if (_cali) return true; | ||
| #if ADC_CALI_SCHEME_LINE_FITTING_SUPPORTED | ||
| adc_cali_line_fitting_config_t cfg = { | ||
| .unit_id = ADC_UNIT_1, | ||
| .atten = ADC_ATTEN_DB_12, | ||
| .bitwidth = ADC_BITWIDTH_12, | ||
| }; | ||
| if (adc_cali_create_scheme_line_fitting(&cfg, &_cali) == ESP_OK) return true; | ||
| #endif | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
ESP-IDF 5 adc_cali curve fitting scheme supported targets ESP32-S3 C3 C6 line fitting
💡 Result:
In ESP-IDF 5 and later, the Curve Fitting calibration scheme is the supported method for the ESP32-S3, ESP32-C3, and ESP32-C6 targets [1][2][3]. These chips are designed to use the Curve Fitting scheme (ADC_CALI_SCHEME_VER_CURVE_FITTING) for ADC calibration, rather than the Line Fitting scheme [1][2][3]. The Line Fitting scheme (ADC_CALI_SCHEME_VER_LINE_FITTING) is generally supported for the original ESP32 target [4][5][6]. Key details regarding these schemes in ESP-IDF: 1. Support: You can verify the supported calibration scheme for your specific target by calling adc_cali_check_scheme() in your code [7]. This function returns a mask indicating which schemes are available [1][7]. 2. Implementation: The Curve Fitting scheme is implemented using the adc_cali_curve_fitting_config_t configuration structure and the adc_cali_create_scheme_curve_fitting() function [1][3]. 3. eFuse Requirements: The Curve Fitting scheme relies on calibration parameters (eFuse bits) burned into the chip during production [3]. If these bits are not present, the creation function may return ESP_ERR_NOT_SUPPORTED [3]. You can consult the components/esp_adc/interface/adc_cali_interface.h file in the ESP-IDF repository for the full interface definitions [1][7].
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/stable/esp32s3/api-reference/peripherals/adc_calibration.html
- 2: https://docs.espressif.com/projects/esp-idf/en/stable/esp32c3/api-reference/peripherals/adc_calibration.html
- 3: https://docs.espressif.com/projects/esp-idf/en/v5.2/esp32c6/api-reference/peripherals/adc_calibration.html
- 4: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/adc/adc_calibration.html
- 5: https://docs.espressif.com/projects/esp-idf/en/v6.0.1/esp32/api-reference/peripherals/adc/adc_calibration.html
- 6: https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/peripherals/adc/adc_calibration.html
- 7: https://docs.espressif.com/projects/esp-idf/en/stable/esp32c6/api-reference/peripherals/adc/adc_calibration.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate ADC manager files =="
fd -a 'wled_ADCmanager\.(cpp|h)$' . | sed 's#^\./##'
echo "== outline =="
file="$(fd 'wled_ADCmanager\.cpp$' . | head -n1)"
if [ -n "${file:-}" ]; then
ast-grep outline "$file" --view compact || true
echo "== relevant sections =="
nl -ba "$file" | sed -n '1,130p;180,280p;320,380p'
fi
echo "== references to analogReadMilliVolts and ADC manager class =="
rg -n "analogReadMilliVolts|WLEDAdcManager|_initCali|adc_cali_.*_fitting" . -g '!build' -g '!node_modules' | head -n 200
echo "== Battery usermod references =="
fd -a 'Battery\.(cpp|h)$' . | sed 's#^\./##'
bfile="$(fd 'Battery\.cpp$' . | grep 'usermods/Battery' | head -n1)"
if [ -n "${bfile:-}" ]; then
nl -ba "$bfile" | sed -n '1,170p'
fi
echo "== platform definitions for ADC schemes =="
rg -n "ADC_CALI_SCHEME_(LINE|CURVE)\.H|ADC_CALI_SCHEME_(LINE|CURVE)_SUPPORTED|ADC_CALI_SCHEME_(LINE|CURVE).*esp32|esp_adc_cal_handle|esp_adc_cal_.*scheme" . -g '!build' -g '!node_modules' | head -n 200Repository: wled/WLED
Length of output: 524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant ADC manager sections =="
file="$(fd 'wled_ADCmanager\.cpp$' . | head -n1)"
sed -n '1,130p' "$file"
echo "--- lines 130-290 ---"
sed -n '130,290p' "$file"
echo "--- lines 290-390 ---"
sed -n '290,390p' "$file"
echo "== ADC manager header =="
h="$(fd 'wled_ADCmanager\.h$' . | head -n1)"
sed -n '1,220p' "$h"
echo "== references to analogReadMilliVolts and ADC manager class =="
rg -n "analogReadMilliVolts|WLEDAdcManager|_initCali|adc_cali_.*_fitting|adc_cali_create|adc_cali_delete" . -g '!build' -g '!node_modules' | head -n 250
echo "== Battery usermod files and relevant code =="
fd 'Battery\.(cpp|h)$' . | sed -n '1,50p'
while IFS= read -r bfile; do
echo "--- $bfile ---"
sed -n '1,190p' "$bfile"
done < <(fd 'Battery\.cpp$' . | grep 'usermods/Battery' || true)
echo "== platform definitions for ADC schemes =="
rg -n "ADC_CALI_SCHEME_(LINE|CURVE)\.H|ADC_CALI_SCHEME_(LINE|CURVE)_SUPPORTED|ADC_CALI_SCHEME_VERSION_(LINE|CURVE)|adc_cali_check_scheme|esp_adc_cal_handle_t|esp_adc_cal_.*scheme" . -g '!build' -g '!node_modules' | head -n 300
echo "== static verifier for calibration path facts =="
python3 - <<'PY'
from pathlib import Path
p = Path('lib/wled_ADCmanager/wled_ADCmanager.cpp')
s = p.read_text()
checks = {
"_initCali_only_line_fitting_": "ADC_CALI_SCHEME_LINE_FITTING_SUPPORTED" in s and "ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED" not in s,
"_initCali_returns_false_after_line_fitting": "if (adc_cali_create_scheme_line_fitting(&cfg, &_cali) == ESP_OK) return true;\n#endif\n return false;" in s,
"_readRaw_uses_calibration_or_fallback": "adc_cali_convert(_cali, raw, &calibrated)" in s and "calibrated = (raw * 3300) / 4095;" in s,
"_analogReadMilliVolts_uses_raw_milli_calibrated": "_initCali()" in s and "analogReadMilliVolts" in s,
"destructor_deletes_line_fitting_only": "adc_cali_delete_scheme_line_fitting(_cali);" in s and "adc_cali_delete_scheme_curve_fitting(_cali)" not in s,
}
for k, v in checks.items():
print(f"{k}: {v}")
PYRepository: wled/WLED
Length of output: 20907
🌐 Web query:
ESP-IDF ESP32 C6 ADC CALI supported ADC_CALI_SCHEME_LINE_FITTING_SUPPORTED ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED
💡 Result:
The ESP32-C6 officially supports the Curve Fitting calibration scheme (ADC_CALI_SCHEME_VER_CURVE_FITTING) in the ESP-IDF [1][2]. Regarding the specific schemes you mentioned: - ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED: The ESP32-C6 is designed to use the Curve Fitting scheme [1][3]. - ADC_CALI_SCHEME_LINE_FITTING_SUPPORTED: The Line Fitting scheme is generally not the primary or recommended calibration method for the ESP32-C6 in the standard ESP-IDF implementation [1][2]. While the enum ADC_CALI_SCHEME_VER_LINE_FITTING exists in the common API headers [1][3], the ESP32-C6 specifically documents and supports the Curve Fitting scheme [1][2]. To check the supported calibration schemes dynamically at runtime for your specific target, you can use the adc_cali_check_scheme() function provided by the ESP-IDF ADC calibration driver [1][2]. This function will populate a mask indicating which schemes are supported on the hardware [1][2]. You should rely on this API if you need to ensure compatibility or check for support programmatically [1][2].
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/latest/esp32c6/api-reference/peripherals/adc/adc_calibration.html
- 2: https://docs.espressif.com/projects/esp-idf/en/v6.0/esp32c6/api-reference/peripherals/adc/adc_calibration.html
- 3: https://docs.espressif.com/projects/esp-idf/en/v6.0.1/esp32c6/api-reference/peripherals/adc/adc_calibration.html
🌐 Web query:
ESP-IDF 5.2 ESP32-C6 ADC calibration curve fitting scheme ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED default scheme
💡 Result:
In ESP-IDF 5.2, the ESP32-C6 supports the curve fitting ADC calibration scheme, identified by the macro ADC_CALI_SCHEME_VER_CURVE_FITTING [1][2]. The support for this scheme is controlled by the definition ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED, which is set to 1 for the ESP32-C6 [2][3]. This scheme is the designated method for calibrating ADC readings on the ESP32-C6 [1][4]. To implement this in your application, you should: 1. Configure the adc_cali_curve_fitting_config_t structure with the appropriate unit_id, channel (if applicable), attenuation, and bitwidth [1][5]. 2. Initialize the calibration scheme by calling the function adc_cali_create_scheme_curve_fitting [1][5]. 3. If the initialization returns ESP_ERR_NOT_SUPPORTED, it indicates that the necessary calibration eFuse bits have not been burnt on the specific device [1][6]. When the calibration handle is no longer required, it should be released using adc_cali_delete_scheme_curve_fitting [1][5]. This curve fitting approach replaces older, deprecated calibration APIs in ESP-IDF 5.x [7].
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/v5.2/esp32c6/api-reference/peripherals/adc_calibration.html
- 2: https://github.com/espressif/esp-idf/blob/12f36a02/components/esp_adc/esp32c6/include/adc_cali_schemes.h
- 3: espressif/esp-idf@7a462d8
- 4: https://docs.espressif.com/projects/esp-idf/en/release-v5.2/esp32c6/api-reference/peripherals/adc_calibration.html
- 5: https://github.com/espressif/esp-idf/blob/v5.2.6/components/esp_adc/include/esp_adc/adc_cali_scheme.h
- 6: https://docs.espressif.com/projects/esp-idf/en/v5.2.1/esp32c6/api-reference/peripherals/adc_calibration.html
- 7: https://docs.espressif.com/projects/esp-idf/en/v5.2/esp32c6/migration-guides/release-5.x/5.0/peripherals.html
Use the curve-fitting calibration scheme when creating _cali.
_initCali() only tries the line-fitting scheme, which is the primary calibration scheme for ESP32; modern targets such as ESP32-S3, ESP32-C3, and ESP32-C6 use curve fitting. Add ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED and adc_cali_create_scheme_curve_fitting(), then delete _cali with adc_cali_delete_scheme_curve_fitting(_cali) in the destructor. The existing fallback to (raw * 3300) / 4095 makes analogReadMilliVolts() inaccurate for battery voltage in usermods/Battery/Battery.cpp.
🤖 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 `@lib/wled_ADCmanager/wled_ADCmanager.cpp` around lines 340 - 351, Update
WLEDAdcManager::_initCali() to support ADC_CALI_SCHEME_CURVE_FITTING_SUPPORTED
by creating _cali with adc_cali_create_scheme_curve_fitting(), while preserving
the existing line-fitting path as appropriate. In the WLEDAdcManager destructor,
release curve-fitting calibration handles with
adc_cali_delete_scheme_curve_fitting(_cali), ensuring the scheme-specific
cleanup matches the creation path.
1 participant
This adds an ADCmanager library like I mentioned in #5764 which makes it possible to use continuous ADC sampling along side single-shot pin sampling. What this means is that we get the best of both worlds for the AR usermod: one pin can be sampled "in the background" with high sampling rate. If a different pin needs to be sampled (analog button, battery usermod etc.) it will pause the sampling for ~1ms, saving whatever samples were already acquired, read the single-shot pin and continue sampling in continuous mode.
Tested this extensively on differend MCUs and found no issue (well, found many, but all are fixed)
The Arduino functions analogRead() and analogReadMilliVolts() are overriden with the ADCmanager's functions, so full backwards compatibility (and any usermod that uses those calls). On ESP8266 the manager is not doing anything, it keeps working as it was.
How it works:
A caller can simply use the function
WLEDAdcManager::begin(uint8_t pin, uint32_t sampleRateHz, uint16_t samplesPerFrame)To start sampling a pin. It will fill the lower level drivers buffer up to samplesPerFrame, any additional samples are dropped. The samples can be read back using
WLEDAdcManager::readSamples(int16_t* buffer, uint16_t numSamples, uint32_t timeoutMs)To read the full buffer or just a part of it. If timeout is set to 0 (or low enough) it will return the samples that are currently available.
So in a nutshell, this is like "I2S_GRAB_ADC1_COMPLETELY" in AR but without grabbing it completely.
Tested on C3, C6, S3 and classic ESP32
I only found one issue so far and that is if the pin-info page is open and analog pins are configured and the continuous sampling is active it can sometimes crash, could not find the exact reason (crash log:
`LoadProhibited (Exception 28: Access to invalid address: LOAD (wild pointer?))
0 0x4008baff xTaskRemoveFromEventList ??
1 0x401c13e3 xQueueGenericSend ??
2 0x400d4e76 WLEDAdcManager::analogRead(unsigned char) wled_ADCmanager.cpp:336
3 0x400f3958 handleAnalog(unsigned char) button.cpp:182`
Summary by CodeRabbit