nuttxpr commented on PR #15849: URL: https://github.com/apache/nuttx/pull/15849#issuecomment-2662280891
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. This PR description does not fully meet the NuttX requirements. Here's a breakdown and suggestions for improvement: **Missing/Insufficient Information:** * **Summary:** While it states *what* the change is, it lacks a clear explanation of *why* it's necessary. What problem does slower/less reliable SPIRAM updating cause? What benefit does this new function provide to users? Is this addressing a specific bug or performance issue? Issue references are missing. * **Impact:** Listing ESP32, ESP32S2, and ESP32S3 is not sufficient. You must explicitly answer *all* the impact questions (user, build, hardware, documentation, security, compatibility) with YES/NO and provide descriptions where necessary. For example: * **Impact on user:** YES. Users can now utilize the `esp_spiram_writeback_range` function to potentially improve performance when updating SPIRAM. * **Impact on build:** NO. * ...and so on for all impact categories. * **Testing:** The provided testing shows *how* to build and flash, but not the *results* of the testing. The "before" and "after" logs are missing actual output. Simply stating "tested with `mm` command" is not enough; show the output of the `mm` command before and after the change. What was the observed improvement? How did you measure "faster and more reliable"? Quantify the results whenever possible (e.g., "Reduced update time by X%"). **Improvements and Clarifications:** * **Purpose/Motivation:** Clearly articulate the problem this change solves and the benefits it brings. Provide concrete examples or scenarios where this improvement is noticeable. * **Issue References:** Link to related NuttX and/or NuttX Apps issues if applicable. * **Impact Assessment:** Thoroughly address each impact category with a YES/NO answer and a detailed explanation when YES. * **Testing Logs:** Include actual "before" and "after" logs demonstrating the change's effect. Quantify improvements with metrics (e.g., timing, error rates). Don't just show build commands. Show the output of the `esp_spiram_test` and the `mm` command, highlighting the difference. * **Build Host Information:** Specify the details of your build host system as requested (OS, CPU, compiler version). * **Target Details:** Be more specific about the targets tested. Instead of "esp32-devkitc:psram", provide details like "ESP32-DevKitC V4 with 4MB PSRAM". **Example of Improved Testing Section:** ``` ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux (Ubuntu 22.04), x86_64 Intel i7-12700K, GCC 11.3.0 * Target(s): esp32-devkitc-v4:psram_4mb, esp32s2-saola-1:spiram_4mb, esp32s3-devkit-1:psram_8mb Testing logs before change (esp32-devkitc-v4): ``` // Output of esp_spiram_test and mm command before the change, showing errors or slow performance. ``` Testing logs after change (esp32-devkitc-v4): ``` // Output of esp_spiram_test and mm command after the change, showing no errors and improved performance (e.g., "SPI SRAM memory test OK!" and faster mm operations). Quantify the improvement if possible. ``` // Repeat the above testing logs for each target (esp32s2 and esp32s3). ``` By addressing these points, the PR description will be much clearer, more complete, and easier for reviewers to understand and approve. Remember, a well-written PR saves everyone time and helps ensure the quality of the NuttX project. -- 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
