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/esp32s3: use common Espressif wireless source #15816

Merged

Conversation

fdcavalcanti
Copy link
Contributor

@fdcavalcanti fdcavalcanti commented Feb 11, 2025

Summary

This PR modifies ESP32S3 to use WiFi source code from the common layer, which is already used by ESP32S2.

KConfig prefixes such as ESP32S3_* are replaced with ESPRESSIF_* (CONFIG_ESP32S3_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: No.
  • Impact on Hardware: Affects only ESP32S3 boards.
  • Impact on Documentation: No.
  • Impact on Security, Compatibility: No.

Testing

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

@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 11, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 11, 2025

[Experimental Bot, please feedback here]

This PR mostly meets the NuttX requirements but is missing some key information. Here's a breakdown:

Strengths:

  • Clear Summary of Changes: The summary clearly explains the what and why of the changes. It mentions the affected functional area (WiFi and BLE on ESP32S3), the method used (moving to common code), and the specific changes made (KConfig prefix changes).
  • Impact Assessment: The impact section acknowledges the KConfig changes and their potential impact on users with custom defconfigs.
  • Testing Confirmation: The author confirms testing and lists the configurations tested.

Weaknesses and Missing Information:

  • Missing Issue References: The PR lacks references to any related NuttX or NuttX Apps issues. Even if there isn't a directly related issue, stating "N/A" would be helpful for clarity.
  • Incomplete Impact Assessment:
    • Impact on User: While the KConfig impact is mentioned, it's not detailed enough. How will users need to adapt? Provide an example of a config change a user might need to make.
    • Impact on Build: Does the move to the common layer change any build dependencies or the build process itself (beyond the KConfig changes)? If not, state "NO".
    • Impact on Hardware: Specify "ESP32S3" explicitly. Even though it's implied, being explicit is better. Does it affect only the ESP32S3?
    • Impact on Documentation: Does this require documentation updates? If not, state "NO". If yes, has the documentation been updated as part of the PR?
    • Impact on Security, Compatibility: Explicitly state "NO" for these if there's no impact. Don't leave them blank.
    • Anything else to consider?: If nothing else, explicitly state "None."
  • Insufficient Testing Details:
    • Build Host(s): Missing information about the build host environment. Provide OS, CPU architecture, and compiler version.
    • Target(s): Be more specific. Instead of just "ESP32S3", mention the specific board and configuration used. Example: ESP32S3-DevKitC: sta_softap
    • Missing Test Logs: The PR template explicitly requests "Testing logs before change" and "Testing logs after change". These are crucial for demonstrating the effectiveness of the changes and aiding in review. Even if the logs are extensive, a summarized snippet showing key differences is better than nothing. Consider using a service like gist.github.com to host larger logs and linking them in the PR. If no specific logs are relevant, explain why and describe the testing methodology used instead. For example: "Testing involved verifying successful connection to an access point and confirming data transfer. No specific log snippets are included as the existing test suite adequately covers these scenarios."

Recommendation:

Revise the PR description to address the missing information and strengthen the weaker sections. Providing more detail will greatly improve the review process and increase the likelihood of acceptance.

@fdcavalcanti fdcavalcanti force-pushed the feature/xtensa_common_wifi_esp32s3 branch from 7646510 to 766e9fd Compare February 12, 2025 14:01
@fdcavalcanti
Copy link
Contributor Author

Fixed CI build error.

@fdcavalcanti fdcavalcanti force-pushed the feature/xtensa_common_wifi_esp32s3 branch 2 times, most recently from 5db9acc to 1408513 Compare February 12, 2025 17:18
Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

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

please update commit title to
arch/xtensa/esp32s3: use common Espressif wireless source

@fdcavalcanti
Copy link
Contributor Author

This commit also affects the boards directory.
Won't arch on the title be misleading?

please update commit title to arch/xtensa/esp32s3: use common Espressif wireless source

@jerpelea
Copy link
Contributor

jerpelea commented Feb 13, 2025

This commit also affects the boards directory. Won't arch on the title be misleading?

please update commit title to arch/xtensa/esp32s3: use common Espressif wireless source

if a commit changes 2 areas I think that it should be spitted in 2 commits arch and board in this case and submit them both in same PR

@fdcavalcanti
Copy link
Contributor Author

This commit also affects the boards directory. Won't arch on the title be misleading?

please update commit title to arch/xtensa/esp32s3: use common Espressif wireless source

if a commit changes 2 areas I think that it should be spitted in 2 commits arch and board in this case and submit them both in same PR

Sorry but it does not make sense.
See #15749 (comment).

If we do this separately, it will introduce a broken build in the git history.

Update the wireless symbols from ESP32S3_* to ESPRESSIF_* for using common layer.
Remove ESP32S3 specific WiFi files and edit build system to use common layer.
@fdcavalcanti fdcavalcanti force-pushed the feature/xtensa_common_wifi_esp32s3 branch from 1408513 to 0a8f349 Compare February 13, 2025 12:04
@fdcavalcanti
Copy link
Contributor Author

This commit also affects the boards directory. Won't arch on the title be misleading?

please update commit title to arch/xtensa/esp32s3: use common Espressif wireless source

if a commit changes 2 areas I think that it should be spitted in 2 commits arch and board in this case and submit them both in same PR

Sorry but it does not make sense. See #15749 (comment).

If we do this separately, it will introduce a broken build in the git history.

@jerpelea that first PR I opened (#15749) split changes in arch and board. That was deemed not good as it would break the build, since those were two heavily integrated commits.

Now this PR is following the expected "don't break the build between commits". Do you have any other demands here or can we proceed?

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @fdcavalcanti :-) Side question does ESP32C3 supports wifi/mesh on NuttX?

@jerpelea I think we can make an exception here, this makes sense to bundle changes not to break a build and eventual issues may be tracked to a single commit :-)

Ah I just followed the whole conversation chain @jerpelea sorry, my approval is conditional to your requests on backporting, so when all is ready can move forward :-)

@fdcavalcanti
Copy link
Contributor Author

Thank you @fdcavalcanti :-) Side question does ESP32C3 supports wifi/mesh on NuttX?

@jerpelea I think we can make an exception here, this makes sense to bundle changes not to break a build and eventual issues may be tracked to a single commit :-)

Ah I just followed the whole conversation chain @jerpelea sorry, my approval is conditional to your requests on backporting, so when all is ready can move forward :-)

I don't think there is any mesh support, sorry. But we do have WiFi working.

@jerpelea
Copy link
Contributor

Thank you @fdcavalcanti :-) Side question does ESP32C3 supports wifi/mesh on NuttX?
@jerpelea I think we can make an exception here, this makes sense to bundle changes not to break a build and eventual issues may be tracked to a single commit :-)
Ah I just followed the whole conversation chain @jerpelea sorry, my approval is conditional to your requests on backporting, so when all is ready can move forward :-)

I don't think there is any mesh support, sorry. But we do have WiFi working.

it is ok fro me

@jerpelea jerpelea merged commit 954081c into apache:master Feb 17, 2025
13 checks passed
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.

6 participants