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

esp32s3-devkit: Add configuration for ADB #14906

Merged

Conversation

JianyuWang0623
Copy link
Contributor

@JianyuWang0623 JianyuWang0623 commented Nov 22, 2024

Summary

  1. Initializing configuration for ADB
  2. Add initialize scripts for ROMFS/etc
  3. Enable ROMFS/etc for ADB auto start
  4. Add doc for USB ADB

Impact

  1. esp32s3-devkit/adb (NEW)
  2. esp32s3-devkit/ROMFS/etc (NEW)

Testing

Selftest

  1. Build & Flash
./tools/configure.sh -l esp32s3-devkit:adb
make -j16
make flash ESPTOOL_PORT=/dev/ttyACMx
  1. Runtime
$ adb -s 1234 shell
nsh> uname -a
NuttX 0.0.0  Nov 22 2024 11:41:43 xtensa esp32s3-devkit

NuttX CI

Build & Flash

  ./tools/configure.sh -l esp32s3-devkit:adb
  make -j16
  make flash ESPTOOL_PORT=/dev/ttyACMx

Runtime

  $ adb -s 1234 shell
  nsh> uname -a
  NuttX 0.0.0  Nov 22 2024 11:41:43 xtensa esp32s3-devkit

Signed-off-by: wangjianyu3 <[email protected]>
@github-actions github-actions bot added Board: xtensa Size: S The size of the change in this PR is small labels Nov 22, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 22, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although some sections could be more detailed.

Here's a breakdown of what's good and what could be improved:

Good:

  • Clear Summary: You've listed the key changes made.
  • Impact Section Mostly Addressed: You've identified new additions.
  • Testing Evidence Provided: You included build instructions, flash instructions, and example output demonstrating ADB functionality.

Needs Improvement:

  • Summary - More Detail:

    • Why: Explain the motivation behind adding ADB support to the esp32s3-devkit. (e.g., debugging, application deployment, etc.)
    • How: Briefly describe the implementation (e.g., "Added ADB initialization scripts to the ROMFS/etc directory, configured the build system to include ADB, and enabled automatic start on boot.")
    • Issues/PRs: If this relates to any existing NuttX issues or PRs, link them here.
  • Impact - More Detail:

    • While you've listed the new configurations, you haven't explicitly answered the questions in the template. Use "NO" or "YES" followed by an explanation. For example:
      • Impact on user: YES. Users will now be able to connect to the esp32s3-devkit using ADB.
      • Impact on build: YES. The build system has been modified to include the ADB configuration.
      • Impact on hardware: NO.
      • Impact on documentation: YES/NO. If documentation is needed, mention that it will be added in a separate PR or that it has been included. If no documentation is needed, explicitly state "NO".
      • Impact on security: Potentially YES. ADB access can pose security risks if not properly managed. Consider mentioning any mitigations or considerations.
      • Impact on compatibility: NO/YES. Explain any compatibility concerns, particularly if this change could affect other boards or configurations.
  • Testing - More Detail:

    • Build Host(s): Specify your build host environment (OS, CPU architecture, compiler version).
    • Target(s): Already provided (esp32s3-devkit).
    • "Testing logs before change" is missing. Since this is a new feature, you might state "N/A - New Feature" or explain what behavior existed before (e.g., no ADB connectivity).
    • Expand on testing: Did you test other ADB commands besides uname -a? More comprehensive testing strengthens the PR.
    • NuttX CI: What were the results of the NuttX CI tests? If the CI hasn't run yet, mention that it's pending.

By addressing these points, you'll create a more complete and informative PR that's easier for reviewers to understand and approve.

@JianyuWang0623
Copy link
Contributor Author

Link: apache/nuttx-apps#2865

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions github-actions bot added the Area: Documentation Improvements or additions to documentation label Nov 22, 2024
@JianyuWang0623
Copy link
Contributor Author

@acassis acassis merged commit 2d20de7 into apache:master Nov 22, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Board: xtensa Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants