nuttxpr commented on PR #3010: URL: https://github.com/apache/nuttx-apps/pull/3010#issuecomment-2681217815
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR, as described, **does not fully meet** the NuttX requirements. While it provides a good starting point, it lacks crucial details in several sections. Here's a breakdown: **Summary:** * **Insufficient Detail:** While the changes are mentioned (GPIO input and LED matrix output), the *why* is missing. Why are these changes necessary? Do they address a specific user need or improve existing functionality? Explain the motivation behind the change. * **Missing How:** How does the GPIO input work? Which GPIOs are used? How is the LED matrix integrated? Which driver is used? The summary should provide a concise technical overview of the implementation. **Impact:** * **Too many "No" answers without justification:** While many impacts might genuinely be "no," it's crucial to provide a brief justification. For example, "Impact on build: No, as no new dependencies or build steps are introduced." This demonstrates that the impact was considered. * **Potential Hardware Impact:** Adding LED matrix support and GPIO input *almost certainly* has a hardware impact. The description mentions specific configurations and hardware. This needs to be clearly documented. Specify which architectures and boards are affected. The provided diagram is helpful, but the PR description must be self-contained. Embed the image directly in the PR description (if supported) or refer to it clearly in the repository. **Testing:** * **Insufficient Testing Logs:** Simply stating "Internal CI testing +" isn't sufficient. Provide snippets of relevant log output demonstrating the functionality before and after the change. Show that the GPIO input works and that the LED matrix displays correctly. * **Unclear Configuration:** While the configuration options are listed, it's not clear what the *default* behavior is or how a user would enable these new features. This information should be included in the documentation update (which should exist). * **Lack of Specific Test Cases:** What specific test cases were executed? Did you test different input patterns with the GPIO? Different display patterns with the LED matrix? Describe the tests performed. * **Missing Build Host Information:** Provide information about the build host used for testing. **Recommendations for Improvement:** * **Expand on the *Why* in the summary.** Justify the changes with a clear explanation of the benefits. * **Provide a concise technical explanation of *How* the changes are implemented.** Mention specific drivers, APIs, and configuration options used. * **Justify "No" impact statements.** Even if there's no impact, briefly explain why. * **Clearly document the hardware impact.** Specify affected architectures, boards, and any required hardware connections. Include the diagram in the PR description itself, if possible. * **Provide more detailed testing logs.** Demonstrate the functionality before and after the change with specific examples. * **Describe specific test cases executed.** Show the different scenarios tested and their expected outcomes. * **Document how to enable the new features.** This might require updates to the documentation, which should be included in the PR. * **Include Build Host information:** Specify OS, CPU, compiler details. By addressing these points, you can significantly improve the quality of your NuttX PR and make it easier for reviewers to understand and approve your 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
