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

xtensa/esp32: use common Espressif wireless source #15853

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fdcavalcanti
Copy link
Contributor

This PR is the second part of the WiFi common layer port for Espressif Xtensa devices.
ESP32S3 has been recently merged here #15816.

Summary

This PR modifies ESP32 to use WiFi source code from the common layer, which is already used by ESP32S2 and ESP32S3. KConfig prefixes such as ESP32_* are replaced with ESPRESSIF_* (CONFIG_ESP32_WIFI -> CONFIG_ESPRESSIF_WIFI). This is applied through arch/ and also board/.

BLE symbols are altered as well, since they share some common wireless implementations. However, source code for BLE has not been merged into the common layer.

Impact

  • Impact on User: KConfig changes will cause issues to users who have custom defconfigs, as they should now update the wireless symbols.
  • Impact on Build: Not in practice, but some build instructions are removed from arch/xtensa/src/esp32 and merged into arch/xtensa/src/common/espressif.
  • Impact on Hardware: Affects only ESP32S3 boards.
  • Impact on Documentation: No.
  • Impact on Security, Compatibility: Only compatibility issue is the KConfig symbols mentioned above.

Testing

Internal CI testing +
sta_softap, wifi, ble, blewifi defconfigs (among others) have been tested and are performing as expected.

Update the wireless symbols from ESP32_* to ESPRESSIF_* for using common layer.
Remove ESP32 specific WiFi files and edit build system to use common layer.
@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Board: xtensa Size: L The size of the change in this PR is large labels Feb 17, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 17, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although some minor improvements could be made. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the changes. The reference to the previous PR (xtensa/esp32s3: use common Espressif wireless source #15816) is helpful for context.
  • Impact Assessment: The impact section addresses all the required points. The description of the user impact (KConfig changes) is particularly important.
  • Testing Information: The testing section provides information on the host and targets used for testing. Mentioning specific defconfigs tested is good.

Areas for Improvement:

  • Missing Issue References: While the previous PR is mentioned, it's beneficial to link a related NuttX or NuttX Apps issue if one exists. Even if this PR is purely a refactoring following from a previous issue, referencing that issue is helpful for traceability.
  • Testing Logs: The PR claims testing logs are included, but the provided template sections are empty. Actual log snippets demonstrating functionality before and after the change should be included. This provides concrete evidence of successful testing. If logs are too extensive, consider providing a link to an external log file or CI run.
  • Hardware Impact Clarification: The impact on hardware states "Affects only ESP32S3 boards." However, the summary mentions modifying ESP32. This discrepancy should be clarified. Does it affect both ESP32 and ESP32S3 or just ESP32S3? If ESP32S3 was already transitioned in the previous PR, that needs to be stated more explicitly, as the current wording implies this PR is the first change for the ESP32S3.
  • Build Impact Nuance: While the practical build impact is minimal, the movement of build instructions is a change, even if minor. Acknowledging this explicitly and mentioning the specific files affected adds clarity.

Recommendation:

The PR is close to meeting the requirements. Adding the missing issue references, including actual testing logs (or links to them), and clarifying the hardware and build impact will strengthen the PR and make it easier for reviewers to assess.

default n
depends on ARCH_CHIP_ESP32
---help---
Enable optimization of WLAN memory
Copy link
Contributor

Choose a reason for hiding this comment

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

Please give some hints about that are these optimization. Why are they important? And why did they are enable by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually disabled by default.
Those are used for dynamic allocation of the buffers used on the WiFi. Seems to be a legacy thing from ESP32.
In fact, this will be probably removed soon as I'm testing an upgrade on esp_wlan.c to use IOBs.

Copy link
Contributor

@tmedicci tmedicci left a comment

Choose a reason for hiding this comment

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

This is the same PR of #15816 but for ESP32.

I've already reviewed it at #15749

@fdcavalcanti fdcavalcanti requested a review from acassis February 20, 2025 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: xtensa Issues related to the Xtensa architecture Board: xtensa Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants