Update ADC to latest ESP32 api and enable ADC for esp32_H2 & C6 #3216
Conversation
WalkthroughEnables and wires ESP32 ADC support across build, presets, headers, device mappings, and native drivers; refactors native ADC controller/channel code to a dual-path (CONFIG_IDF_TARGET_ESP32 vs non-ESP32) model with per-unit adc_oneshot handles and lifecycle APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Managed App
participant Ctl as AdcController (native)
participant Map as DeviceMapping
participant IDF as ESP-IDF (adc_oneshot)
rect rgb(238,247,255)
note right of Ctl: New per-unit adc_oneshot flow (non-ESP32-target)
end
App->>Ctl: NativeInit / NativeOpenChannel(channel)
Ctl->>Map: Resolve unit, gpio, channel
Ctl->>Ctl: AdcController_Initialize(unit) [lazy init]
Ctl->>IDF: adc_oneshot_new_unit(unit)
Ctl->>IDF: adc_oneshot_config_channel(unit, channel, cfg)
App->>Ctl: NativeReadValue
Ctl->>IDF: adc_oneshot_read(unit, channel)
IDF-->>Ctl: value or error
Ctl-->>App: value or mapped error
sequenceDiagram
autonumber
actor App as Managed App
participant Ctl as AdcChannel (legacy ESP32)
participant IDF as ESP-IDF legacy ADC
rect rgb(255,249,240)
note right of Ctl: Classic ESP32 path (driver/adc.h)
end
App->>Ctl: Read channel N
alt ADC1
Ctl->>IDF: adc1_get_raw(N)
else ADC2
Ctl->>IDF: adc2_get_raw(N, &value)
else Internal Temp (channel 8)
Ctl->>IDF: temprature_sens_read()
else Removed Hall (channel 9)
Ctl-->>App: CLR_E_INVALID_PARAMETER
end
IDF-->>Ctl: value or status
Ctl-->>App: value or mapped error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CMake/Modules/FindSystem.Device.Adc.cmake(1 hunks)CMake/binutils.ESP32.cmake(2 hunks)CMake/riscv-esp32c6.json(1 hunks)CMake/riscv-esp32h2.json(1 hunks)targets/ESP32/_common/ESP32_C6_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/ESP32_H2_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/ESP32_P4_DeviceMapping.cpp(1 hunks)targets/ESP32/_include/esp32_idf.h(1 hunks)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp(2 hunks)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel_legacy.cpp(1 hunks)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp(3 hunks)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController_legacy.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController_legacy.cpp (1)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (4)
NativeOpenChannel___VOID__I4(83-145)NativeOpenChannel___VOID__I4(83-84)NativeGetChannelCount___I4(147-157)NativeGetChannelCount___I4(147-147)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp (2)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel_legacy.cpp (2)
NativeReadValue___I4(13-67)NativeReadValue___I4(13-13)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (2)
GetAdcHandle(49-57)GetAdcHandle(49-49)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (1)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController_legacy.cpp (2)
NativeOpenChannel___VOID__I4(35-97)NativeOpenChannel___VOID__I4(35-36)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel_legacy.cpp (1)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp (5)
NativeReadValue___I4(16-73)NativeReadValue___I4(16-16)temperature_sens_read(11-11)NativeDisposeChannel___VOID(75-84)NativeDisposeChannel___VOID(75-75)
🔇 Additional comments (8)
targets/ESP32/_common/ESP32_P4_DeviceMapping.cpp (1)
62-72: LGTM: ADC pin mappings updated for ESP32_P4.The explicit GPIO mappings for ADC1 (8 channels) and ADC2 (6 channels) are correctly specified and align with the PR objectives to update ESP32 ADC integration.
Note: The comment on lines 65-66 ("ESP32P4 ADC1 channels 0 - 9, ADC2 channels 10 - 19") is inconsistent with the actual mapping (8 + 6 = 14 channels). Consider removing or updating this comment to match the new description on lines 62-64.
CMake/riscv-esp32h2.json (1)
28-28: LGTM: ADC API enabled for ESP32-H2.The change enables
API_System.Device.Adcfor the RISCV ESP32-H2 preset, aligning with the PR objectives to enable ADC support for ESP32_H2 targets.targets/ESP32/_common/ESP32_H2_DeviceMapping.cpp (1)
61-66: LGTM: ADC mapping updated for ESP32_H2.The explicit channel mapping (1-5) for ESP32_H2 ADC1 is correctly specified and aligns with the PR objectives to enable ADC support for ESP32_H2 targets using the oneshot API.
targets/ESP32/_common/ESP32_C6_DeviceMapping.cpp (1)
59-65: LGTM: ADC mapping updated for ESP32_C6.The explicit channel mapping (0-6) for ESP32_C6 ADC1 is correctly specified and aligns with the PR objectives to enable ADC support for ESP32_C6 targets using the oneshot API.
CMake/riscv-esp32c6.json (1)
28-28: LGTM: ADC API enabled for ESP32-C6.The change enables
API_System.Device.Adcfor the RISCV ESP32-C6 preset, aligning with the PR objectives to enable ADC support for ESP32_C6 targets.targets/ESP32/_include/esp32_idf.h (1)
66-72: LGTM: Conditional ADC header inclusion for ESP32 targets.The conditional inclusion routes ESP32 through the legacy ADC driver (
driver/adc.h) due to the dependency with the legacy I2S driver and internal DAC, while other ESP32 variants use the oneshot ADC driver (esp_adc/adc_oneshot.h). This aligns with the PR objectives to update ADC to the latest ESP32 API while preserving legacy support for ESP32.The comment clearly explains the rationale for the conditional inclusion.
CMake/binutils.ESP32.cmake (1)
690-690: LGTM: esp_adc component integrated into ESP32 IDF build.The additions of
esp_adctoIDF_COMPONENTS_TO_ADD(line 690) andidf::esp_adctoIDF_LIBRARIES_TO_ADD(line 706) correctly integrate the ESP32 ADC component into the IDF build and link stages, aligning with the PR objectives to update ADC to the latest ESP32 API.Also applies to: 706-706
CMake/Modules/FindSystem.Device.Adc.cmake (1)
19-39: LGTM: Conditional ADC source selection for ESP32 targets.The conditional logic correctly routes ESP32 targets to legacy ADC source files (
sys_dev_adc_native_System_Device_Adc_AdcChannel_legacy.cpp,sys_dev_adc_native_System_Device_Adc_AdcController_legacy.cpp) due to the dependency with the legacy I2S driver and internal DAC, while preserving the standard non-legacy sources for other targets. This aligns with the PR objectives to update ADC to the latest ESP32 API while maintaining backward compatibility for ESP32.The comment on line 19 clearly explains the rationale for the conditional logic.
.../ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel_legacy.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel_legacy.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel_legacy.cpp (1)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp (4)
NativeReadValue___I4(16-73)NativeReadValue___I4(16-16)NativeDisposeChannel___VOID(75-84)NativeDisposeChannel___VOID(75-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (5)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel_legacy.cpp (5)
9-11: Legacy SDK symbol correctly declared.The misspelled
temprature_sens_read()matches the legacy ESP32 SDK's actual symbol name. The call site at line 38 correctly uses this name.
36-45: ESP32 internal sensor handling is correct.The temperature sensor correctly calls the legacy SDK's
temprature_sens_read(), and the Hall sensor appropriately returns an error since it's no longer available in IDF 5.x.
52-68: ADC2 handling is correct.The ADC2 channel adjustment, legacy API call with proper bitwidth, error checking, and invalid ADC number handling are all implemented correctly.
70-72: Result handling is correct.The reading value is properly returned to managed code using the standard stack result mechanism.
75-84: Dispose implementation is appropriate.The no-op implementation matches the non-legacy version and is correct for the legacy ADC API, which doesn't require explicit channel cleanup.
.../ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel_legacy.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CMake/Modules/FindSystem.Device.Adc.cmake(0 hunks)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp(4 hunks)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- CMake/Modules/FindSystem.Device.Adc.cmake
🧰 Additional context used
🧬 Code graph analysis (2)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp (2)
src/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel_stubs.cpp (2)
NativeReadValue___I4(9-16)NativeReadValue___I4(9-9)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (2)
GetAdcHandle(185-193)GetAdcHandle(185-185)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (3)
src/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController_stubs.cpp (14)
NativeOpenChannel___VOID__I4(9-17)NativeOpenChannel___VOID__I4(9-10)NativeGetChannelCount___I4(19-26)NativeGetChannelCount___I4(19-19)NativeGetMaxValue___I4(28-35)NativeGetMaxValue___I4(28-28)NativeGetMinValue___I4(37-44)NativeGetMinValue___I4(37-37)NativeIsChannelModeSupported___BOOLEAN__I4(46-54)NativeIsChannelModeSupported___BOOLEAN__I4(46-47)NativeGetResolutionInBits___I4(56-64)NativeGetResolutionInBits___I4(56-57)NativeInit___VOID(66-73)NativeInit___VOID(66-66)targets/ChibiOS/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (14)
NativeOpenChannel___VOID__I4(9-74)NativeOpenChannel___VOID__I4(9-10)NativeGetChannelCount___I4(76-84)NativeGetChannelCount___I4(76-76)NativeGetMaxValue___I4(86-94)NativeGetMaxValue___I4(86-86)NativeGetMinValue___I4(96-103)NativeGetMinValue___I4(96-96)NativeIsChannelModeSupported___BOOLEAN__I4(105-116)NativeIsChannelModeSupported___BOOLEAN__I4(105-106)NativeGetResolutionInBits___I4(118-127)NativeGetResolutionInBits___I4(118-119)NativeInit___VOID(129-138)NativeInit___VOID(129-129)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp (1)
GetAdcHandle(91-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (8)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp (4)
8-11: LGTM! Good addition of device mapping and target-specific guard.The inclusion of
Esp32_DeviceMapping.hand theCONFIG_IDF_TARGET_ESP32guard properly separate legacy from new ADC API paths.
38-46: LGTM! Correct handling of deprecated internal channels.The temperature sensor read and the hall sensor deprecation (IDF 5.x removal) are properly handled for the ESP32 legacy path.
59-60: LGTM! Improved readability.The multi-line formatting of the
adc2_get_rawcall improves readability without changing functionality.
107-110: TARGET_ADC_NUM_PINS defined in all ESP32 device mappings Verified the macro is present in every mapping file under targets/ESP32/_common.targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (4)
28-46: LGTM! Comprehensive documentation of ADC channel layouts.The updated comments clearly document the ADC channel configurations for all ESP32 variants (ESP32, S2, S3, P4, C3, C6, H2), including the note about IDF 5.x deprecating internal sensors.
283-293: LGTM! Dynamic channel count calculation improves maintainability.The dynamic calculation using
CONFIG_SOC_ADC_MAX_CHANNEL_NUM * CONFIG_SOC_ADC_PERIPH_NUMcorrectly determines the total channel count for each target without hardcoding, making it more maintainable than the legacy path's hardcoded value of 20.
295-348: LGTM! Native methods correctly implement ADC properties.All native methods (GetMaxValue, GetMinValue, IsChannelModeSupported, GetResolutionInBits, Init) are correctly implemented and consistent with the 12-bit ADC configuration and legacy behavior.
185-193: Verifyadc_unit_tsignedness: confirm in the ESP-IDF headers whetheradc_unit_tis signed; if it’s unsigned, theunit < 0check is always false and can be removed.
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp
Show resolved
Hide resolved
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp
Show resolved
Hide resolved
Automated fixes for code style.
Automated fixes for code style.
AdrianSoundy
force-pushed
the
Update-ADC-api-for-esp32
branch
from
f242cea to
1ef3624
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
targets/ESP32/_include/esp32_idf.h (1)
66-72: Conditional ADC driver inclusion looks correct.The target-specific conditional logic properly isolates ESP32 to use the legacy ADC driver while other variants (S2, S3, C3, C6, H2) use the new oneshot API. This aligns with the PR objectives of migrating to the new API while maintaining ESP32's I2S/DAC dependency.
Optionally, consider adding a tracking reference to the comment (e.g., linking to the Home issue #1580 mentioned in the PR) or a TODO to facilitate future migration when the I2S driver is upgraded:
#if defined(CONFIG_IDF_TARGET_ESP32) -// Use legacy ADC driver for ESP32 for now as the new one also requires the new I2S driver due to dependcy because of +// TODO: Migrate ESP32 to new ADC driver once I2S is upgraded (see https://github.com/nanoframework/Home/issues/1580) +// Use legacy ADC driver for ESP32 for now as the new one also requires the new I2S driver due to dependency because of // internal DAC other ESP32 variants don't have DAC so use the new ADC drivertargets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (1)
195-217: Consider graceful error handling instead of panics.Lines 207 and 214 use
ESP_ERROR_CHECK, which aborts on failure. While ESP-IDF initialization failures typically indicate misconfiguration, returning error codes to managed code (as done inNativeOpenChannelat line 275) would provide a more consistent and debuggable experience.Apply this pattern for graceful error handling:
void AdcController_Initialize(adc_unit_t unit) { if (adc_handles[unit] != NULL) { // already initialized return; } adc_oneshot_unit_init_cfg_t init_config1 = {}; init_config1.unit_id = unit; init_config1.ulp_mode = ADC_ULP_MODE_DISABLE; - ESP_ERROR_CHECK(adc_oneshot_new_unit(&init_config1, &adc_handles[unit])); + esp_err_t result = adc_oneshot_new_unit(&init_config1, &adc_handles[unit]); + if (result != ESP_OK) + { + adc_handles[unit] = NULL; + // Caller should check GetAdcHandle() result + } }Then update
NativeOpenChannelto check the handle:// Initialize ADC unit if required AdcController_Initialize(adcUnit); + + if (GetAdcHandle(adcUnit) == NULL) + { + NANOCLR_SET_AND_LEAVE(CLR_E_PIN_UNAVAILABLE); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMake/Modules/FindSystem.Device.Adc.cmake(0 hunks)CMake/binutils.ESP32.cmake(2 hunks)CMake/riscv-esp32c6.json(1 hunks)CMake/riscv-esp32h2.json(1 hunks)targets/ESP32/_common/ESP32_C6_DeviceMapping.cpp(2 hunks)targets/ESP32/_common/ESP32_H2_DeviceMapping.cpp(2 hunks)targets/ESP32/_common/ESP32_P4_DeviceMapping.cpp(2 hunks)targets/ESP32/_include/esp32_idf.h(1 hunks)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp(4 hunks)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- CMake/Modules/FindSystem.Device.Adc.cmake
🚧 Files skipped from review as they are similar to previous changes (4)
- targets/ESP32/_common/ESP32_H2_DeviceMapping.cpp
- CMake/riscv-esp32h2.json
- CMake/riscv-esp32c6.json
- targets/ESP32/_common/ESP32_C6_DeviceMapping.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp (1)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (2)
GetAdcHandle(185-193)GetAdcHandle(185-185)
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (2)
src/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController_stubs.cpp (14)
NativeOpenChannel___VOID__I4(9-17)NativeOpenChannel___VOID__I4(9-10)NativeGetChannelCount___I4(19-26)NativeGetChannelCount___I4(19-19)NativeGetMaxValue___I4(28-35)NativeGetMaxValue___I4(28-28)NativeGetMinValue___I4(37-44)NativeGetMinValue___I4(37-37)NativeIsChannelModeSupported___BOOLEAN__I4(46-54)NativeIsChannelModeSupported___BOOLEAN__I4(46-47)NativeGetResolutionInBits___I4(56-64)NativeGetResolutionInBits___I4(56-57)NativeInit___VOID(66-73)NativeInit___VOID(66-66)targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp (1)
GetAdcHandle(91-91)
🔇 Additional comments (11)
CMake/binutils.ESP32.cmake (1)
690-690: LGTM! Correct component registration for esp_adc.The additions properly register the
esp_adccomponent and its corresponding library (idf::esp_adc) following the established pattern in this file. The symmetry between the two lists is maintained, with both entries positioned consistently betweenesp_psramandlittlefs.Also applies to: 706-706
targets/ESP32/_common/ESP32_P4_DeviceMapping.cpp (1)
27-27: LGTM: Formatting change.The UART COM1 initialization has been reformatted to a single line with no functional changes.
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcChannel.cpp (4)
8-11: LGTM: Clear architecture documentation.The include and comment appropriately document the dual-path strategy (legacy ESP32 vs new API for other targets).
91-150: LGTM: Non-ESP32 path correctly implements new ADC API.The implementation properly uses the new
adc_oneshot_readAPI with appropriate error handling and channel validation. The ADC handle is obtained viaGetAdcHandle, which is initialized inNativeOpenChannelbefore any reads occur.
152-161: LGTM: Empty disposal is appropriate.Per-channel disposal is not required since ADC unit handles are managed at the controller level rather than per-channel.
13-13: No issues found—"temprature_sens_read" is the actual legacy ESP-IDF function name.The function "temprature_sens_read()" is a real ESP-IDF function, despite the misspelling. This function was available in librtc.a as part of the ESP-IDF, and the code correctly references it. The spelling is not a typo in this codebase; it matches the actual legacy ESP-IDF API. The current ESP-IDF versions use newer temperature sensor APIs, but this code maintains backward compatibility with older implementations.
targets/ESP32/_nanoCLR/System.Device.Adc/sys_dev_adc_native_System_Device_Adc_AdcController.cpp (5)
28-48: LGTM: Comprehensive hardware documentation.The detailed channel mapping comments for each ESP32 variant provide valuable context for understanding target-specific ADC configurations.
50-179: LGTM: ESP32 legacy path correctly implements existing API.The legacy ADC driver implementation maintains backward compatibility with appropriate error handling and hardware-specific constants (12-bit resolution, 20 channels).
219-281: LGTM: Non-ESP32 channel configuration correctly uses new API.The implementation properly validates channel numbers, initializes ADC units on-demand, adjusts channel indices for ADC2, and provides appropriate error handling via
CLR_E_PIN_UNAVAILABLE.
283-293: LGTM: Dynamic channel count calculation.Computing the channel count from SoC configuration constants properly adapts to different ESP32 variants without hardcoding.
295-348: LGTM: Consistent value/mode reporting across both paths.The non-ESP32 path returns the same hardware capabilities (12-bit resolution, 0-4095 range, single-ended mode) as the ESP32 legacy path, ensuring consistent behavior.
| // ESP32_P4 ADC1 with 8 channels (GPIO 16,17,18,19,20,21,22,23) | ||
| // plus 6 channels (GPIO 49,50,51,52,53,54) | ||
|
|
||
| // ESP32P4 ADC1 channels 0 - 9 | ||
| // ADC2 channels 10 - 19 |
There was a problem hiding this comment.
Correct the ADC channel documentation.
The comments contain inaccuracies:
- Lines 62-63 claim "ADC1 channels 0 - 9" and "ADC2 channels 10 - 19" (20 channels total), but the actual mapping has only 14 channels (8 ADC1 + 6 ADC2).
- Line 60's indentation makes it unclear whether it's describing ADC1 or ADC2.
- Inconsistent naming: "ESP32_P4" (line 59) vs "ESP32P4" (line 62).
Apply this diff to fix the documentation:
-// ESP32_P4 ADC1 with 8 channels (GPIO 16,17,18,19,20,21,22,23)
-// plus 6 channels (GPIO 49,50,51,52,53,54)
-
-// ESP32P4 ADC1 channels 0 - 9
-// ADC2 channels 10 - 19
+// ESP32_P4 ADC1 with 8 channels (GPIO 16,17,18,19,20,21,22,23)
+// ADC2 with 6 channels (GPIO 49,50,51,52,53,54)
+
+// ESP32_P4 ADC1 channels 0 - 7
+// ADC2 channels 8 - 13Note: Adjust the channel numbering if the per-unit indexing (ADC1: 0-7, ADC2: 0-5) is intended instead of sequential indexing.
📝 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.
| // ESP32_P4 ADC1 with 8 channels (GPIO 16,17,18,19,20,21,22,23) | |
| // plus 6 channels (GPIO 49,50,51,52,53,54) | |
| // ESP32P4 ADC1 channels 0 - 9 | |
| // ADC2 channels 10 - 19 | |
| // ESP32_P4 ADC1 with 8 channels (GPIO 16,17,18,19,20,21,22,23) | |
| // ADC2 with 6 channels (GPIO 49,50,51,52,53,54) | |
| // ESP32_P4 ADC1 channels 0 - 7 | |
| // ADC2 channels 8 - 13 |
🤖 Prompt for AI Agents
In targets/ESP32/_common/ESP32_P4_DeviceMapping.cpp around lines 59 to 63,
correct the ADC comment block: unify the chipset name to "ESP32_P4", clarify
which lines describe ADC1 vs ADC2 by fixing indentation and wording, and update
channel counts and numbering to reflect 14 channels total (ADC1: 8 channels on
GPIO16-23, ADC2: 6 channels on GPIO49-54). Also choose and document the indexing
scheme consistently (either per-unit indexing ADC1: 0-7 and ADC2: 0-5, or a
sequential 0-13 mapping) and update the text accordingly so it accurately
describes channel counts, GPIO pins, and numbering.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Description
This updates the IDF api for ADC from legacy to latest API before it gets removed from IDF.
Because there is a dependency between the legacy ADC and the legacy I2S was not able to upgrade the ESP32 target until the I2S driver has been updated. The reason for this is the ESP32 has an internal DAC & ADC which can be used with I2S. The other ESP32 series don't have these. It also looks like the latest API for I2S doesn't support the internal DAC and once upgraded ww will have to remove that support from managed I2S driver.
The ADC driver is now re-enabled for Esp32_C6 and ESP32_H2 as it no longer hangs on start with new API.
Motivation and Context
Start of change to update all legacy IDF api drivers and fix missing ADC from C6 & H2 targets
Part of change for nanoframework/Home#1580
How Has This Been Tested?
Tested with ESP32_S3 and ESP32_C6 targets
Types of changes
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores
Style