nuttxpr commented on PR #14805: URL: https://github.com/apache/nuttx/pull/14805#issuecomment-2478163514
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR appears to mostly meet the NuttX requirements, but is missing some key information. **Missing/Needs Improvement:** * **Summary:** While the summary explains the "why" (fixing an issue introduced by another PR), it lacks clarity on the "what" and "how." Be more specific about which functional part of the code is changed (e.g., memory allocation, heap management, atomic operations). Explain precisely *how* the use of arch atomics resolves the issue with S32C1I instructions and the iram heap. * **Impact:** The impact section is too brief. While it mentions the ESP32 iram heap, it doesn't explicitly answer the required questions (user impact, build impact, hardware impact, documentation, security, compatibility). Even if the answer is "NO" for most, it should be stated explicitly. For example: * Impact on user: NO * Impact on build: NO * Impact on hardware: YES (affects ESP32 using iram heap) * Impact on documentation: NO/YES (Specify if documentation needs updating or if you've updated it). * Impact on security: NO/YES (Consider potential implications of changing atomic operations). * Impact on compatibility: NO/YES * **Testing:** While you mention a successful boot, this isn't sufficient. Provide more details: * **Build Host:** Specify your build host OS, CPU architecture, and compiler version. * **Target:** You mentioned `esp32-devkit:sotest`. Be more explicit about the ESP32 chip variant and the NuttX configuration used (e.g., `esp32-devkitc:nsh`). * **Logs:** "boot success" is not a log. Show relevant log output *before* and *after* the change. Demonstrate that the issue existed before and is resolved after. Ideally, the logs should show the specific error that occurred before the fix and its absence afterward. If `sotest` was used, provide the sotest output. **Example of Improved Impact Section:** ``` ## Impact * Is new feature added? NO * Is existing feature changed? YES (Fixes iram heap functionality on ESP32) * Impact on user: NO (Users should see corrected behavior with iram heap). * Impact on build: NO * Impact on hardware: YES (Specifically affects ESP32 architectures using the iram heap). * Impact on documentation: NO (No documentation updates required/provided). * Impact on security: NO (No known security implications). * Impact on compatibility: NO (No compatibility issues expected). * Anything else to consider: NO ``` By providing more complete information in these sections, the PR will be much easier to review and merge. -- 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
