Re: [PR] ADC support on Espressif devices [nuttx]

2025-04-16 Thread via GitHub


nuttxpr commented on PR #16228:
URL: https://github.com/apache/nuttx/pull/16228#issuecomment-2809353486

   [**\[Experimental Bot, please feedback 
here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
   
   
   
   This PR appears to meet the NuttX requirements, although some minor 
improvements could be made.
   
   **Strengths:**
   
   * **Clear Summary:** The summary clearly outlines the changes and their 
purpose.
   * **Detailed Impact Assessment:**  The impact section addresses all the 
required points and provides specific details about the changes for ESP32S3.
   * **Thorough Testing:** The testing section describes the test setup, 
procedure, and results in detail, including example output.  The mention of a 
separate automated test application is also a positive.
   
   
   **Areas for Improvement:**
   
   * **Conciseness:** The testing section could be more concise.  While detail 
is good, the extensive log output could be trimmed or moved to a separate file 
referenced in the PR.  Summarizing the results across all boards is helpful, 
but the full tables aren't necessary in the PR description itself.  Offer to 
provide the detailed data upon request.
   * **Testing Logs Before Change:**  While you mention a testing procedure, 
you didn't include "before change" logs. These logs are essential to 
demonstrate the actual improvement or fix provided by the PR. Include a brief 
snippet of the output before the ADC changes were implemented, even if it 
simply shows a lack of functionality.
   * **Related Issues:** You mention updating the documentation, but it would 
be helpful to link any related NuttX or NuttX Apps issues that prompted this 
change, or that this change resolves.  Even if there wasn't a specific issue, 
mentioning it ("No related issues") is good practice.
   
   **Verdict:**
   
   Overall, the PR seems well-structured and provides enough information to 
understand the changes.  Addressing the minor points above, particularly 
including "before change" logs, would further strengthen the PR.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]



[PR] ADC support on Espressif devices [nuttx]

2025-04-16 Thread via GitHub


fdcavalcanti opened a new pull request, #16228:
URL: https://github.com/apache/nuttx/pull/16228

   
   ## Summary
   
   - documentation: update docs on ADC support for Espressif devices
   - boards/risc-v: ADC support on Espressif devices
   - boards/xtensa: ADC support on Espressif devices
   - arch/risc-v: ADC support for ESP32-C3|C6|H2
   - arch/xtensa: ADC support on ESP32|S2|S3
   
   This PR adds support for ADC oneshot on Espressif devices.
   
   It supports reading of all available ADC channels of the device. The user 
may use the adc defconfig which enables one ADC unit, usually with 3 or 4 
channels by default.
   
   The documentation has been updated for each SoC, describing the all GPIOs 
associated to a particular channel, some limitations and usage example.
   
   On the usability side, all channels, ADC units and input voltage attenuation 
can be modified via menuconfig. Also, ADC calibration is enabled and applied by 
default (if the device supports it).
   
   Special mention for ESP32S3: this device already had an ADC implementation. 
I have removed it but added some Kconfig entries to keep backwards 
compatibility, so the user should be able to compile but not use the same as 
before, since device registration has changed to keep up with ADC API.
   
   ## Impact
   
   Impact on user: Yes. Impacts users that use ADC on ESP32S3. Application 
should still compile, however the ADC character driver now represents an entire 
ADC unit instead of registering one character driver for each channel.
   
   Impact on build: No.
   
   Impact on hardware: Yes. This PR adds support for ADC on: 
ESP32|S2|S3|C3|C6|H2.
   
   Impact on documentation: Yes. Updates documentation for ESP32|S2|S3|C3|C6|H2 
regarding ADC peripheral support.
   
   Impact on security: No.
   
   Impact on compatibility: Only for ESP32S3. See "impact on user".
   
   ## Testing
   
   This procedure was done manually for all boards of this PR. All ADC channels 
were tested by setting 0 and 3.3 V and reading the ADC application output. A 
complementary test had a potentiometer to evaluate the voltage range.
   
   A test application was developed to apply the internal pull-up and pull-down 
resistors of the GPIOs to automatically test the ADC (which will be running on 
Espressif's internal CI).
   
   ### Building
   The ADC defconfig enables the ADC unit 1 and 3 or 4 channels by default. For 
testing, we enable the ADC unit 2 and all possible channels.
   
   - `./tools/configure.sh esp32-devkitc:adc`
   - `CONFIG_DEBUG_FEATURES`
   - `CONFIG_DEBUG_ASSERTIONS`
   - `CONFIG_DEBUG_ANALOG`
   - `CONFIG_ESPRESSIF_ADC_2`
   - `CONFIG_ESPRESSIF_ADC_1_CH0` through `CONFIG_ESPRESSIF_ADC_1_CH9`
   - `CONFIG_ESPRESSIF_ADC_2_CH0` through `CONFIG_ESPRESSIF_ADC_2_CH9`
   - `CONFIG_EXAMPLES_ADC_GROUPSIZE=10`
   - `CONFIG_ADC_FIFOSIZE=11`
   
   ### Running
   Connect the GPIO pins to a test voltage and run the ADC example for each 
unit:
   
   - `adc -n 1 -p /dev/adc0`
   - `adc -n 1 -p /dev/adc1`
   
   
   ### Results
   I'll summarize the results. OK means the voltage reading was good even 
though it might be off by some mV, which is expected on some SoCs.
   I have some tables with the test results (SOC, channel, voltage max, min, 
etc) but I won't put it here since it is too long. I can upload somewhere if 
necessary.
   
   - ESP32 (ESP32_DevKitc_V4)
 - ADC1 channels 0-7: OK
 - ADC2 channels 0-9: OK with warning: channels 1, 2 and 3 are used as 
strapping pins and can present undefined behavior
   
   - ESP32S2 (ESP32-S2-Saola-1_V1.2)
 - ADC1 channels 0-9: OK
 - ADC2 channels 0-9: OK
   
   - ESP32S3 (ESP32-S3-DevKitC-1 v1.0)
 - ADC1 channels 0-9: OK
 - ADC2 channels 0-9: OK
   
   - ESP32C3 (ESP32-C3-DevKitC-02 V1.1)
 - ADC1 channels 0-4: OK
 - ADC2 not implemented since it only has 1 channel
   
   - ESP32C6 (ESP32-C6-DevKitC-1 V1.1)
 - ADC1 channels 0-6: OK
   
   - ESP32H2 (ESP32-H2-DevKitM-1 V1.2)
 - ADC1 channels 0-4: OK
   
   Here's the log output for the ESP32S3, with verbosity high. All ADC channels 
are enabled on both ADC units (/dev/adc0 and /dev/adc1). We can see the ADC 
bringup, calibration and results. It is similar to all devices.
   
   ```
   *** Booting NuttX ***
   I (59) boot: chip revision: v0.1
   I (59) boot.esp32s3: Boot SPI Speed : 40MHz
   I (59) boot.esp32s3: SPI Mode   : DIO
   I (63) boot.esp32s3: SPI Flash Size : 8MB
   I (68) boot: Enabling RNG early entropy source...
   dram: lma 0x0020 vma 0x3fc8c800 len 0x2328   (9000)
   iram: lma 0x2350 vma 0x40374000 len 0x644c   (25676)
   padd: lma 0x87a8 vma 0x len 0x7850   (30800)
   imap: lma 0x0001 vma 0x4201 len 0x17664  (95844)
   padd: lma 0x0002766c vma 0x len 0x898c   (35212)
   dmap: lma 0x0003 vma 0x3c04 len 0x8d20   (36128)
   total segments stored 6
   ABesp_adc_initialize: initialize SAR ADC 1
   esp_adc_oneshot_new_unit: unit 0 freq 8000
   esp_ad