nuttx icon indicating copy to clipboard operation
nuttx copied to clipboard
verified publisher icon apache

esp32s3 openeth ethernet driver

Open casaroli opened this issue 1 year ago • 3 comments

Summary

Espressif recently added support for esp32s3 on their qemu, so we migrate the openeth ethernet driver to xtensa common and add it to the esp32s3-devkit config.

Impact for the user

  • Features added
    • Now we can run the esp32s3 port in qemu. By enabling the configuration ESP32S3_QEMU_IMAGE and nuttx.merged.bin will be built (as described in the documentation).
    • Now the user can enable the openeth driver in esp32s3 in qemu with the option ESP32S3_OPENETH and use network in qemu as described in https://github.com/espressif/esp-toolchain-docs/blob/main/qemu/esp32/README.md#ethernet-support
    • Adds a esp32s3-devkit:qemu-openeth config which automatically has networking enabled and is ready to boot on qemu with esp-idf legacy bootloader.
  • Impact on Build:
    • No changes to the build process itself.
    • Users will need to select the appropriate QEMU ESP32-S3 or ESP32 configurations.
  • Impact on Hardware: Support added for Opencore ethernet MAC on ESP32-S3 in QEMU.
  • Impact on Documentation: Documentation was updated to include instructions for running the ESP32-S3 port in QEMU with networking support.
  • Impact on Security: None.
  • Impact on Compatibility: No backward compatibility issues.

Currently, the simple boot is not working with QEMU. This issue was not introduced by this PR, as it is an effect or enabling the simple boot by default. The qemu-openeth defconfigs enable ESP32_APP_FORMAT_LEGACY or ESP32S3_APP_FORMAT_LEGACY to overcome this limitation._

Testing

  • Build Host: Ubuntu 22.04 LTS, esp-idf v5.3.1
  • Target: QEMU emulator version 9.0.0 (esp_develop_9.0.0_20240606)
  • Configs:
    • esp32-devkitc:qemu-openeth
    • esp32s3-devkit:qemu-openeth
  • Tests Performed:
    • Verified successful boot of NuttX in QEMU.
    • Network connectivity tested using ping command from the nsh console.

Commands

Download espressif qemu from https://github.com/espressif/qemu

esp32s3

$ ./tools/configure.sh -l esp32s3-devkit:qemu-openeth
$ make bootloader
$ make ESPTOOL_BINDIR=.
$ qemu-system-xtensa -nographic -machine esp32s3 -m 4 \
    -nic user,model=open_eth \
    -drive file=./nuttx.merged.bin,if=mtd,format=raw
...
NuttShell (NSH) NuttX-12.6.0-RC1
nsh> ping -c2 apache.org
PING 151.101.2.132 56 bytes of data
56 bytes from 151.101.2.132: icmp_seq=0 time=10.0 ms
56 bytes from 151.101.2.132: icmp_seq=1 time=10.0 ms
2 packets transmitted, 2 received, 0% packet loss, time 2020 ms
rtt min/avg/max/mdev = 10.000/10.000/10.000/0.000 ms
nsh>

esp32

QEMU is not emulating the chip revision v3.0, so the user has two options:

  • #define ESP32_IGNORE_CHIP_REVISION_CHECK in arch/xtensa/src/esp32/esp32_start.c
  • emulate the efuse as described here https://github.com/espressif/esp-toolchain-docs/blob/main/qemu/esp32/README.md#emulating-esp32-eco3
$ ./tools/configure.sh -l esp32-devkitc:qemu-openeth
$ make bootloader
$ make ESPTOOL_BINDIR=.
$ qemu-system-xtensa -nographic -machine esp32 -m 4 \
    -nic user,model=open_eth \
    -drive file=./nuttx.merged.bin,if=mtd,format=raw
...
WARNING: NuttX supports ESP32 chip revision >= v3.0 (chip is v0.0).
Ignoring this error and continuing because `ESP32_IGNORE_CHIP_REVISION_CHECK` is set...
THIS MAY NOT WORK! DON'T USE THIS CHIP IN PRODUCTION!

NuttShell (NSH) NuttX-12.6.0-RC1
nsh> ping -c2 apache.org
PING 151.101.2.132 56 bytes of data
56 bytes from 151.101.2.132: icmp_seq=0 time=20.0 ms
56 bytes from 151.101.2.132: icmp_seq=1 time=10.0 ms
2 packets transmitted, 2 received, 0% packet loss, time 2020 ms
rtt min/avg/max/mdev = 10.000/15.000/20.000/5.000 ms
nsh>

Out of scope

  • Currently the simple boot method is not working with QEMU. For this reason we enable the legacy esp-idf bootloader on the default config for qemu-openeth. As this PR did not introduce this defect, I plan on investigating this in the future, however I do not have any timeline for this.
  • esp32 qemu does not emulate the version v3.0, so we need to either add an efuse storage as described in the espressif documentation or define ESP32_IGNORE_CHIP_REVISION_CHECK in arch/xtensa/src/esp32/esp32_start.c. This is a defect of espressif qemu and is out of scope of NuttX. However, I have updated the esp32 documentation to reference this issue.

casaroli avatar Sep 18 '24 15:09 casaroli

[Experimental Bot, please feedback here]

The PR summary is OK, but the other sections need significant work to meet the requirements.

Here's a breakdown of what's missing and how to improve it:

Impact:

  • Not specific enough: Simply stating "Now we can run the esp32s3 port in qemu" isn't sufficient.
    • Be Explicit: List which features are added. Does this enable any new APIs for developers using NuttX on ESP32-S3?
    • Consider the User: Will users need to change configurations, build scripts, or application code to utilize this?
    • Address all points: Go through each impact category (build process, hardware, documentation, security, compatibility) and explicitly state "NO" or provide a detailed "YES" explanation.

Testing:

  • "Manual testing" is insufficient: Provide concrete details.
    • Commands: What commands did you run to build and test? (Show the full commands, not just "make").
    • Configurations: Which specific ESP32-S3 board and configurations were tested?
    • Functionality: What specific tests did you perform to validate the changes? (e.g., Networking tests, peripheral tests).
  • Logs: The "Testing logs" sections are empty. Include relevant logs before and after your changes, demonstrating the problem being solved or the new functionality working.

Out of Scope:

  • Actionable information is better: While it's good to note limitations, consider:
    • Issue Tracking: Is there an existing issue for the simple boot method problem? If so, link it here. If not, create one so it's tracked.
    • Timeline: When can users expect the separate PR? (e.g., "Will be addressed in an upcoming PR within the next sprint").

Example improvements:

Impact:

  • Feature Added: Support for running the ESP32-S3 port of NuttX in QEMU using the esp-idf bootloader.
  • Impact on User:
    • Users who want to use QEMU for ESP32-S3 development will need to use the esp-idf bootloader instead of the simple bootloader.
    • Documentation will be updated to reflect this requirement.
  • Impact on Build:
    • No changes to the build process itself.
    • Users will need to select the appropriate QEMU and ESP32-S3 configurations.
  • Impact on Hardware: Support added for ESP32-S3 in QEMU.
  • Impact on Documentation: Documentation will be updated to include instructions for running the ESP32-S3 port in QEMU.
  • Impact on Security: None.
  • Impact on Compatibility: No backward compatibility issues.

Testing:

  • Build Host: Ubuntu 20.04, GCC 9.4.0
  • Target: QEMU, ESP32-S3 DevKitC (config: esp32s3-devkitc_qemu)
  • Commands:
    • ./tools/configure.sh esp32s3-devkitc_qemu
    • make
    • qemu-system-xtensa -nographic -machine esp32s3 -drive file=nuttx.bin,if=mtd,format=raw
  • Tests Performed:
    • Verified successful boot of NuttX in QEMU.
    • Network connectivity tested using ping command from the NuttX console.
  • Testing Logs: (Include actual log output here)

Out of Scope:

  • The simple boot method is currently not working with QEMU for the ESP32-S3 port. This issue is being tracked in [link to GitHub issue]. A fix for this issue will be addressed in a separate PR.

By providing this level of detail, you make it much easier for reviewers to understand your changes and ensure they meet the project's requirements.

lupyuen avatar Sep 18 '24 15:09 lupyuen

Can anyone help me understand why the workflow is failing? What should I do to fix it?

casaroli avatar Sep 18 '24 20:09 casaroli

Can anyone help me understand why the workflow is failing? What should I do to fix it?

Looks like internal CI problem, builds fine for me on FreeBSD :-)

cederom avatar Sep 18 '24 22:09 cederom

need fix the ci error first: https://github.com/apache/nuttx/actions/runs/11194947243/job/31122067924?pr=13527

xiaoxiang781216 avatar Oct 06 '24 01:10 xiaoxiang781216

need fix the ci error first: https://github.com/apache/nuttx/actions/runs/11194947243/job/31122067924?pr=13527

done

casaroli avatar Oct 06 '24 10:10 casaroli

Hi @casaroli this is good, thanks for the contribution! I'm reviewing this and adding to the internal CI. Will take those last comments into consideration as well.

fdcavalcanti avatar Oct 07 '24 11:10 fdcavalcanti

Now that's what I call a well written PR. Congrats again. Was able to reproduce the issue and make some recommendations.

fdcavalcanti avatar Oct 07 '24 19:10 fdcavalcanti

I believe I have addressed all the concerns. Can you please provide another review?

@xiaoxiang781216 @fdcavalcanti

casaroli avatar Oct 12 '24 11:10 casaroli

Now that's what I call a well written PR. Congrats again. Was able to reproduce the issue and make some recommendations.

Well, you should thank @lupyuen's bot :sweat_smile: :sweat_smile:

casaroli avatar Oct 12 '24 11:10 casaroli