nuttxpr commented on PR #15864: URL: https://github.com/apache/nuttx/pull/15864#issuecomment-2667368072
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details in several sections. Here's a breakdown of what's missing and how to improve it: **Summary:** * **Insufficient detail on *why* the change is necessary:** Simply stating "add a new pico lcd module" isn't enough. Explain the benefits of adding this specific module. Does it support a new feature? Does it replace a less capable or deprecated driver? Does it improve performance? * **Missing information on *how* the change works:** What files were modified? What new code was added? Don't just say it "depends on" another PR – briefly summarize the relevant interaction. * **Needs a proper NuttX Issue reference:** Replace "this commit depend on the pull request..." with a proper issue reference like `Fixes #[IssueNumber]`. Even if it depends on another PR, there should be an associated issue tracking the need for the LCD support. **Impact:** * **Incomplete "Is new feature added? Is existing feature changed?"**: Clearly state: "Yes, a new LCD driver is added." * **All other impact sections are marked as "NO" without justification.** Even if there's no impact, explicitly state "NO" followed by a brief reason, e.g., "Impact on build: NO (No build system changes are required)." For the new feature, you *must* detail the impact on the user. How do they configure and use the new driver? * **Missing detail on the new feature impact:** Explain what configurations are needed to enable this LCD. What are the available framebuffer settings? **Testing:** * **Insufficient Host and Target details:** Specify the full OS version (e.g., "Ubuntu 22.04"), compiler version (e.g., "GCC 11.2.0"), and the specific target architecture, board, and configuration (e.g., "sim:qemu-x86_64", "stm32f4discovery:nsh"). * **Missing "before" logs:** While not strictly required for a *new* feature, it's good practice to show the behavior before the change (e.g., the absence of the LCD driver or the use of a different driver). * **Vague "after" logs:** "run fb and nxhello nxdemo" isn't sufficient. Show specific commands used and relevant output demonstrating that the LCD is functioning correctly. Include things like framebuffer information, successful display of graphics, and touchscreen responsiveness (if applicable). **Example of an Improved Summary:** ``` Summary: This PR adds support for the Pico ResTouch 2.8" LCD module to enhance display options for NuttX-based systems. This addresses the need for a readily available, small form-factor display with touchscreen capabilities, as discussed in #[IssueNumber]. This change introduces a new display driver within the `drivers/lcd` directory, leveraging the recent SPI DMA enhancements from PR #15863 to improve performance. The driver initializes the LCD controller, configures the framebuffer, and provides basic touchscreen functionality. ``` By providing more specific information in each section, you'll create a much more comprehensive and helpful PR description that makes it easier for reviewers to understand and approve your contribution. Remember to follow the contributing guidelines linked in your initial post. -- 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
