nuttxpr commented on PR #15537: URL: https://github.com/apache/nuttx/pull/15537#issuecomment-2589499693
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although it could be stronger. Here's why and how it could be improved: **Strengths:** * **Clear Summary:** The summary adequately describes the *what* and *how* of the changes. * **Impact Section Mostly Addressed:** The impact section covers most of the required points, providing concise explanations. * **Testing Mentioned:** The mention of GitHub CI and local testing is a good start. **Weaknesses and Suggested Improvements:** * **Missing "Why" in Summary:** The summary lacks the crucial "why" this change is necessary. What problem does it solve? What improvement does it offer? This is the most important part of the summary. Example: "This change is necessary to simplify the RISC-V CPU initialization code, improve maintainability, and allow for more flexible CPU configuration in systems without advanced CPU ID mapping requirements." * **Impact - Be More Specific:** While the impact section addresses the points, it's a bit generic. Provide concrete examples where possible. For "Impact on user," even if the answer is NO, briefly explain *why* there's no impact. For "Impact on build," specifying the changes to the build system (e.g., added a new source file to a Makefile) would be helpful. For "Impact on hardware," specify which RISC-V architectures/boards are affected. * **Testing - Insufficient Detail:** "GitHub CI and local testing" is too vague. Specify the host operating system, compiler, and target architecture/board/configuration used for local testing. Provide *actual* test logs (even if abbreviated) demonstrating the functionality before and after the change. Show that the intended behavior is achieved and that nothing is broken. Quantitative results are always preferable if applicable (e.g., "Boot time reduced by 10%"). * **Missing Issue References:** If this PR addresses any specific issues in the NuttX or NuttX Apps repositories, include the issue numbers. **Example of Improved Testing Section:** ``` Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 * Target(s): RISC-V (QEMU virt), sim:nsh Testing logs before change: ``` up_cpu_index(): CPU0 Started ... (other boot logs showing CPU ID mapping issues, if applicable) ``` Testing logs after change: ``` up_cpu_index(): CPU0 Started ... (other boot logs demonstrating correct CPU indexing without CPU ID mapping side effects) ``` By adding this level of detail, the PR becomes much stronger and easier for reviewers to evaluate. It clearly demonstrates the value of the change and provides evidence that it works as expected. -- 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
