nuttxpr commented on PR #15540: URL: https://github.com/apache/nuttx/pull/15540#issuecomment-2589819265
[**\[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 some sections could be more explicit. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary explains the "what" and "why" of the change effectively. * **Impact Description:** Addresses most impact areas. The description of changes to `up_cpu_index` and documentation updates is good. * **Testing Mention:** Mentions both CI and local testing. **Weaknesses & Suggestions for Improvement:** * **Missing Issue References:** If this relates to any existing NuttX issues, those should be linked. * **Impact - User:** While the impact is likely minimal, explicitly stating "NO" or providing a brief explanation (e.g., "NO, this is an internal change") would be clearer. * **Impact - Build:** While file removal is mentioned, clarifying "NO significant changes besides file removal" or similar would be helpful. * **Impact - Hardware:** Be specific about which RISC-V architectures/boards were tested. Just saying "RISC-V" is too broad. State "NO impact on other architectures." * **Impact - Security/Compatibility:** Explicitly stating "NO" for these sections, if applicable, is better than leaving them implied. * **Testing Detail:** "GitHub CI and local machine" is insufficient. Specify the OS, compiler, and importantly, the specific RISC-V target(s) (board:config) used for local testing. Provide snippets of relevant log output demonstrating the change in functionality. The current "before/after" log blocks are empty. Even a simple "print of the hart ID before and after" would be better than nothing. **Example of improved Testing section:** ``` Testing I confirm that changes are verified on local setup and works as intended: * Build Host: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 * Target: RISC-V, qemu-rv64:nsh, hifive1-revb:nsh Testing logs before change (qemu-rv64): ``` nsh> mhartid # Old command 0 ``` Testing logs after change (qemu-rv64): ``` nsh> echo $CPUID 0 ``` Testing logs before change (hifive1-revb): <add logs here> Testing logs after change (hifive1-revb): <add logs here> GitHub CI passed on all platforms. ``` By adding these details, the PR will be much stronger and easier for reviewers to assess. -- 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]
