Adding ADC manager in preparation for AR update to IDF V5 - #5773
Adding ADC manager in preparation for AR update to IDF V5#5773DedeHai wants to merge 18 commits into
Conversation
|
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:
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
sequenceDiagram
participant Caller
participant WLEDAdcManager
participant ESP32ADCDriver
Caller->>WLEDAdcManager: analogRead(pin)
WLEDAdcManager->>ESP32ADCDriver: Pause continuous sampling
WLEDAdcManager->>ESP32ADCDriver: Perform one-shot ADC read
ESP32ADCDriver-->>WLEDAdcManager: Return raw conversion
WLEDAdcManager->>ESP32ADCDriver: Resume continuous sampling
WLEDAdcManager-->>Caller: Return normalized ADC value
Possibly related PRs
🚥 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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 36: Update ADCManager’s mutex arbitration around readSamples() and
analogRead() so continuous reads cannot hold _mutex beyond
ADCMANAGER_LOCK_TIMEOUT_MS, or replace timeout-based contention handling with
bounded arbitration that never returns 0 solely because the lock is unavailable.
Preserve valid zero-valued ADC results while honoring the caller’s timeoutMs and
numSamples constraints.
🪄 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: b0a9001a-d3cc-4102-b313-aea697c5a475
📒 Files selected for processing (1)
lib/wled_ADCmanager/wled_ADCmanager.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/wled_ADCmanager/wled_ADCmanager.cpp (1)
220-234: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument
readSamples()capping the requested timeout.
WLEDAdcManager::readSamples()declarestimeoutMs = 100, but the implementation caps every read atADCMANAGER_LOCK_TIMEOUT_MS - 1and returns after 9 ms if fewer samples are ready. The current internal comment documents this only from the implementation side. Add the effective timeout limit to the publicreadSamples()API documentation and confirm callers handle partial returns.🤖 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 - 234, Update the public API documentation for WLEDAdcManager::readSamples() to state that timeoutMs is capped at ADCMANAGER_LOCK_TIMEOUT_MS - 1 and reads may return fewer samples when the limit is reached. Review callers of readSamples() and ensure they correctly handle partial returns rather than assuming numSamples are always produced.
🤖 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 25-34: Update the ESP32/ESP-IDF guard surrounding the ADC manager
definitions to use the same include-state protection pattern as wled.h, ensuring
ESP_IDF_VERSION_VAL is safely available before evaluating the version check.
Preserve the existing analogRead macro undefinitions and ADCMANAGER constants
while preventing preprocessor or include-path failures when the default
definition is absent.
- Around line 233-234: Update the adc_continuous_read() calls in the ADC read
flow to pass remainingMs directly as the timeout argument, rather than
converting it with pdMS_TO_TICKS(). Apply the same change to both affected call
sites while preserving the existing millisecond calculations and timeout bounds.
---
Nitpick comments:
In `@lib/wled_ADCmanager/wled_ADCmanager.cpp`:
- Around line 220-234: Update the public API documentation for
WLEDAdcManager::readSamples() to state that timeoutMs is capped at
ADCMANAGER_LOCK_TIMEOUT_MS - 1 and reads may return fewer samples when the limit
is reached. Review callers of readSamples() and ensure they correctly handle
partial returns rather than assuming numSamples are always produced.
🪄 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: 30f8fc41-1f43-4e59-8d91-1daf2b07a225
📒 Files selected for processing (1)
lib/wled_ADCmanager/wled_ADCmanager.cpp
|
@softhack007 is probably best to review this rather than me |
|
@softhack007 when reviewing this, check if the mutex is sound and will work with AR. Everything low level / hardware I tested in many combinations
what I did not test:
edit: |
…ightly less pronounced)
2 participants
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)
Requires the IDF bugfix which is available in latest tasmota (i.e. Update to IDF 5.5.4 #5769)
edit: latest version still seems to have that bug, it seems less severe though and I added a fix that works well.
Uses some RAM for the static manager - if no continuous sampling is used, its minimal (mostly for the semaphore, maybe 32bytes?)
Only single pin continuous sampling is supported
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
New Features