nuttxpr commented on PR #15943: URL: https://github.com/apache/nuttx/pull/15943#issuecomment-2698772519
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR looks mostly good and compliant with the requirements. Here's a breakdown with suggestions for improvement: **Strengths:** * **Clear Summary:** The summary adequately describes the change, its purpose, and its origin. The link to the explanatory blog post is helpful. Listing the modified/added files is also excellent practice. * **Impact Assessment:** Addresses most impact areas. The note about no impact on existing code is important. * **Testing Evidence:** Provides build logs and output showing successful boot and basic functionality tests (including `ostest`). This is crucial. **Areas for Improvement:** * **Summary:** While you mention "Most of the code was derived from NuttX for Milk-V Duo S," consider explicitly mentioning any *differences* in the implementation, especially if those differences are significant. This helps reviewers understand the scope of the changes beyond just porting. * **Impact:** * **Impact on user:** While you imply no impact, explicitly stating "NO" would be clearer. E.g., "Impact on user: NO" * **Impact on build:** While you provide build instructions, specifying "YES" and briefly describing the addition of a new build target (`starpro64:nsh`) would improve clarity. Something like: "Impact on build: YES (Adds a new configuration `starpro64:nsh` to the build system)" * **Impact on hardware:** Specify "YES" and list the specific hardware (StarPro64). E.g., "Impact on hardware: YES (Adds support for the PINE64 StarPro64 SBC based on ESWIN EIC7700X SoC)." * **Impact on documentation:** You rightly mention the updated documentation. Be more explicit. "Impact on documentation: YES (Added documentation for StarPro64 in `platforms/risc-v/eic7700x/` and `platforms/risc-v/eic7700x/boards/starpro64/`)" * **Impact on security, compatibility:** Explicitly state "NO" if there are no implications. * **Testing:** * **Build Host(s):** You haven't specified the details of your build host. Include this information (OS, CPU architecture, compiler version). * **Target(s):** Be more explicit: "Target: risc-v, StarPro64:nsh". * **Testing logs before change:** Since this is new hardware support, "N/A" or "Not applicable" is acceptable here. But explaining *why* there are no "before" logs is helpful for reviewers. **Example of Improved Impact Section:** ``` ## Impact This PR is needed to support NuttX on StarPro64 SBC. No impact on existing code, since the StarPro64 source files are not used by existing code. * Impact on user: NO * Impact on build: YES (Adds a new configuration `starpro64:nsh` to the build system) * Impact on hardware: YES (Adds support for the PINE64 StarPro64 SBC based on ESWIN EIC7700X SoC) * Impact on documentation: YES (Added documentation for StarPro64 in `platforms/risc-v/eic7700x/` and `platforms/risc-v/eic7700x/boards/starpro64/`) * Impact on security: NO * Impact on compatibility: NO * Anything else to consider: None ``` By making these small adjustments, you can further strengthen your PR and make it easier for reviewers to understand and approve. Remember, clarity and completeness are key to a successful PR. -- 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
