nuttxpr commented on PR #15816: URL: https://github.com/apache/nuttx/pull/15816#issuecomment-2651832926
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
