Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esp32[s3]: Add ES8311 support for esp32s3-lcd-ev board #15785

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

eren-terzioglu
Copy link
Contributor

Summary

Add ES8311 audio codec support for esp32s3-lcd-ev board

  • esp32[s3]: Add ES8311 support for esp32s3-lcd-ev board

Impact

ESP32S3-LCD-EV

Testing

Command to build:

make -j distclean && ./tools/configure.sh esp32s3-lcd-ev:audio  && make flash EXTRAFLAGS="-Wno-cpp -Werror" ESPTOOL_BINDIR=./ ESPTOOL_PORT=/dev/ttyUSB0 -s -j$(nproc) && minicom

After build and flash operation audio config doc used to test it with these pins:

16:MCLK pin
4: BCLK pin
45: Word Select
18: Data out

@github-actions github-actions bot added Board: xtensa Size: M The size of the change in this PR is medium labels Feb 7, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 7, 2025

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

No, this PR description does not fully meet the NuttX requirements. Here's a breakdown of what's missing and how to improve it:

Missing/Insufficient Information:

  • Summary: While it mentions adding ES8311 support, it lacks crucial details:

    • Why is this change necessary? Is there no audio support currently? Is the existing audio support inadequate? Explain the motivation.
    • How does the change work? What code was added/modified? Did you add a new driver? Modify an existing one? Provide more technical detail. Mentioning the specific files changed would be helpful.
    • Related Issues: Are there any related NuttX or NuttX Apps issues this PR addresses? Provide links if so.
  • Impact: "ESP32S3-LCD-EV" is not sufficient. You need to answer all the questions with "YES" or "NO" and provide descriptions where necessary. For example:

    • Is new feature added? YES (ES8311 audio codec support)
    • Impact on user: YES (Users can now use audio on the esp32s3-lcd-ev board).
    • Impact on build: Potentially YES if any Kconfig changes were required. Describe them. If not, state NO.
    • Impact on hardware: YES (Specifies the pins used: 16:MCLK, 4:BCLK, 45:WS, 18:DOUT). This is good detail but still needs the YES/NO.
    • Impact on documentation: YES (The audio config doc was used, but does it need updating to reflect the new ES8311 support? If so, has the documentation been updated in this PR or a separate one?)
    • Impact on security, compatibility: Explicitly state NO if there are no impacts.
  • Testing:

    • Build Host(s): Provide details about your build host OS, CPU architecture, and compiler version.
    • Target(s): You mention the board, but specify the architecture (xtensa). Be more specific about the configuration used (e.g., esp32s3-lcd-ev:audio).
    • Testing logs: The provided command shows how to build and flash, but it doesn't provide any actual test logs. You need to show output demonstrating that audio works before the change (if there was previous audio support) and after the change, showcasing the ES8311 functionality. Play a sound file or generate a tone. Capture the output that proves it works. If there was no prior audio support, clearly state that and then show the output after the change demonstrating the ES8311 working.

Example of an Improved Summary:

This PR adds support for the ES8311 audio codec on the esp32s3-lcd-ev board. Currently, this board has no audio support. This change implements a new ES8311 driver (added files: drivers/audio/es8311.c, drivers/audio/es8311.h, etc.) and updates the board configuration to use this driver. This resolves [NuttX Issue #XYZ] (if applicable).

Example of Improved Testing Section:

Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.3.0
Target: xtensa, esp32s3-lcd-ev:audio

Testing logs before change:

(If there was previous behavior, show logs. Otherwise state "No prior audio support.")

Testing logs after change:

play -r 44100 -c 2 test.wav  # Or another appropriate command
... output showing audio playing ...

By providing more complete information as described above, your PR will be much easier to review and merge. Remember, clarity and detail are key to a successful PR.

@acassis
Copy link
Contributor

acassis commented Feb 7, 2025

@eren-terzioglu please update the Documentation/ of this board to include the audio board profile

@eren-terzioglu
Copy link
Contributor Author

eren-terzioglu commented Feb 7, 2025

@eren-terzioglu please update the Documentation/ of this board to include the audio board profile

We don't have a page to it right now

~/nuttxspace/nuttx$ ls Documentation/platforms/xtensa/esp32s3/boards/
esp32s3-devkit  esp32s3-eye  esp32s3-korvo-2  esp32s3-lhcbit

@acassis
Copy link
Contributor

acassis commented Feb 7, 2025

@eren-terzioglu please update the Documentation/ of this board to include the audio board profile

We don't have a page to it right now

~/nuttxspace/nuttx$ ls Documentation/platforms/xtensa/esp32s3/boards/
esp32s3-devkit  esp32s3-eye  esp32s3-korvo-2  esp32s3-lhcbit

Please create a page for this board. All boards should have proper documentation including a picture of it.

@acassis acassis merged commit b25c4ec into apache:master Feb 8, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: xtensa Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants