nuttxpr commented on PR #16096: URL: https://github.com/apache/nuttx/pull/16096#issuecomment-2764598730
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR appears to mostly meet the NuttX requirements, but it's missing some key information. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary clearly explains the "why," "what," and "how" of the change. The mention of fixing `malloc` failures in `lvgldemo` provides good context. * **Impact Section Addresses Hardware Changes:** The impact section correctly identifies the affected board. * **Testing Shows Improvement:** The testing logs demonstrate the increased available memory after the change, supporting the claim of fixing `malloc` issues. **Weaknesses:** * **Missing Issue References:** If this PR addresses a specific issue in either the NuttX or NuttX Apps repositories, those issues should be linked. * **Incomplete Impact Assessment:** The impact section needs to be more thorough. Specifically, it needs to address: * **Is new feature added? Is existing feature changed?** (Yes, a new feature is added: PSRAM support). * **Impact on user (will user need to adapt to change)?** (Possibly, if users were relying on the previous memory layout). * **Impact on build (will build process change)?** (Likely yes, as the build system needs to be configured for PSRAM). * **Impact on documentation (is update required / provided)?** (Yes, documentation was updated, this should be explained more). * **Impact on security, compatibility:** Even if these are "NO," explicitly stating it is good practice. * **Insufficient Testing Detail:** While the `free` output is helpful, more testing is desirable. Does `lvgldemo` now run without error? What other tests were performed to validate the PSRAM functionality? The build host and target details are missing. * **Missing "Before Change" Documentation Diff:** While the summary mentions documentation updates, the PR should ideally show the *changes* to the documentation, not just the final state. This helps reviewers understand the extent of the documentation updates. **Recommendation:** To fully meet the NuttX requirements, the PR should be amended to include the missing information outlined above. A more complete impact assessment and more detailed testing information will significantly improve the review process and increase confidence in the change. -- 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
