Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pico restouch lcd 2.8 #15864

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ZhuZhongjie
Copy link
Contributor

@ZhuZhongjie ZhuZhongjie commented Feb 19, 2025

Summary

add a new pico lcd module Pico restouch lcd 2.8
this commit depend on the pull request #15863

Impact

new lcd module support

Testing

I have tested run fb and nxhello nxdemo from USB acm console.

Signed-off-by: Zhu Zhongjie <zhongjiezhu1@gmail.com>
@github-actions github-actions bot added Board: arm Size: M The size of the change in this PR is medium labels Feb 19, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 19, 2025

[Experimental Bot, please feedback here]

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.

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much! :-)

@lupyuen
Copy link
Member

lupyuen commented Feb 19, 2025

Could you fill in the Commit Description? Just copy the PR Summary above into the Commit Description, so it's easier for others to understand the commit:

Also please remove this line from the PR Summary above. Thanks :-)

Note: Please adhere to Contributing Guidelines

…pport

This change adds support for the Pico ResTouch 2.8" LCD module to enhance display options for NuttX-based systems.

Signed-off-by: Zhu Zhongjie <zhongjiezhu1@gmail.com>
@ZhuZhongjie ZhuZhongjie force-pushed the pico-restouch-lcd-2.8 branch from ecb55e9 to f109afc Compare February 19, 2025 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: arm Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants