nuttxpr commented on PR #15563: URL: https://github.com/apache/nuttx/pull/15563#issuecomment-2592227778
[**\[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 more detail could be provided. Here's a breakdown of why and where improvements could be made: **Strengths:** * **Clear Summary:** The summary clearly outlines the *what* and *how* of the changes. It mentions specific functions/macros added/modified. * **Impact Assessment:** Addresses most impact categories. Specifically calls out performance improvements, compatibility considerations, and the limited impact when `CONFIG_SCHED_THREAD_LOCAL` is enabled. * **Testing Performed:** Mentions both CI and local board testing, and importantly, tests with the relevant configuration option both enabled and disabled. **Weaknesses & Suggestions for Improvement:** * **Missing Issue References:** If this PR addresses a specific issue in NuttX or NuttX Apps, those should be linked in the Summary. Even if it's a performance improvement without a corresponding issue, stating "No related issue" is helpful. * **Impact - User:** While it mentions no impact if `CONFIG_SCHED_THREAD_LOCAL` is enabled, it doesn't explicitly state the user impact if it's *disabled*. Will applications need to be recompiled? Will there be any observable changes in behavior? * **Impact - Build:** Does adding these macros change the build process in any way? Even if the answer is "NO", explicitly stating it is helpful. * **Impact - Hardware:** Specifying the architecture (RISC-V as implied by `riscv_cpustart.c`) and the specific board used for local testing would be beneficial. "local board" is too vague. * **Impact - Documentation:** Does this change require documentation updates? If not, explicitly state "NO". If yes, provide the updates or a plan to update them. * **Impact - Security:** Even if the impact is "NO", stating it explicitly is good practice. * **Impact - Compatibility:** More detail is needed. What kind of backward/forward compatibility is maintained or broken? Are there interoperability concerns? * **Testing Logs:** The provided template requests testing logs. While mentioning CI and local testing is good, including snippets of relevant log output demonstrating the change (e.g., performance benchmarks, scheduler behavior) would significantly strengthen the PR. If logs are too extensive, consider linking to a separate file or gist. * **Build Host Details:** While GitHub CI likely covers various build hosts, specifying your *local* build host details (OS, CPU, compiler version) is still important. By addressing these points, the PR would be even stronger and provide reviewers with all the necessary information to efficiently assess the changes. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
