nuttxpr commented on PR #16223: URL: https://github.com/apache/nuttx/pull/16223#issuecomment-2808600905
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description mostly meets the NuttX requirements, but needs a few improvements: **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the changes. * **Detailed Impact:** The impact section covers most of the required areas, although some could be more detailed. * **Testing Information:** Good inclusion of build instructions, sample app code, and expected output. The performance comparison is a nice addition. **Weaknesses:** * **Missing Issue References:** The summary should link to any related NuttX or NuttX Apps issues. Even if there isn't a directly related issue, consider creating one to track the feature request or bug that prompted this PR. * **Incomplete Documentation Impact:** You state there is a documentation impact but don't describe *what* the update is. Specify which files were added/modified and the nature of the changes. * **Insufficient "Testing Logs Before Change":** You provide *expected* output after the change, but don't show what the behavior was *before* the change. This makes it difficult to assess the impact of the PR. Ideally, provide logs showing the slower performance of the existing GPIO implementation. * **Target Details in Testing:** The testing section should be more specific about the target hardware used. E.g., "esp32c6-devkitc on ESP-C6-DevKitC-1 board". * **Build Host Information Missing:** You've documented the *targets*, but not the *host* used for the build. Include OS, CPU architecture, and compiler version. * **Overuse of Bolding:** The excessive bolding in the tables makes them harder to read. Reserve bolding for key information or headings. **Suggested Improvements:** 1. **Add Issue References:** Create and link to a NuttX issue describing the need for dedicated GPIO support. 2. **Detail Documentation Changes:** Explain what documentation was added (e.g., "Added a new section to the ESP32 driver documentation describing the dedicated GPIO interface and usage."). 3. **Provide "Before" Logs:** Show the output of the sample app (or a similar test) *before* the dedicated GPIO changes were implemented. This demonstrates the performance improvement. 4. **Specify Build Host Details:** Add the build host information (OS, CPU, compiler) to the Testing section. For example: "Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0" 5. **Refine Target Descriptions:** Be more precise about the target boards used. For example: "Target: esp32c6-devkitc on ESP-C6-DevKitC-1 board". 6. **Reduce Bolding in Tables:** Remove the excessive bolding in the result tables. Consider using just a header row for bolding. Simple Markdown tables are fine. **Example of Improved Testing Section:** ``` ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0 * Target(s): * esp32c6-devkitc on ESP-C6-DevKitC-1 board * esp32s3-devkit on ESP32-S3-DevKitC-1 board * esp32s2-saola-1 on ESP32-S2-Saola-1 board `esp32c6-devkitc:gpio`, `esp32s3-devkit:gpio` and `esp32s2-saola-1:gpio` config used with `CONFIG_ESPRESSIF_DEDICATED_GPIO` option enabled. Testing logs before change (using standard GPIO): ``` <Output of the test application before the changes, demonstrating slower performance> ``` Testing logs after change (using dedicated GPIO): <Existing "Results" section, including build instructions and sample app code> ``` By addressing these points, your PR will be much stronger and easier for reviewers to assess. Remember, a well-written PR significantly increases its chances of being merged quickly and smoothly. -- 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
