nuttxpr commented on PR #16094: URL: https://github.com/apache/nuttx/pull/16094#issuecomment-2763108398
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR summary is good, but could be improved by explicitly stating it's a new feature and briefly mentioning the benefits (e.g., enables display output on the szpi-esp32s3 board). The impact section is lacking detail and needs to address all the points raised in the requirements. The testing section, while showing some effort, needs significant improvement by providing more context and details. Here's a breakdown of how to improve the PR: **Summary:** * **Improved Wording:** "This PR adds support for GPIO and the ST7789 LCD on the szpi-esp32s3 development board. This enables display output and expands the board's capabilities. This includes initializing GPIO for LCD control, adding support for the ST7789 LCD controller, configuring the LCD, and providing documentation for both GPIO and LCD usage." * **Issue References:** If there are any related NuttX issues or NuttX Apps issues/PRs, link them here. Even if you think there aren't any, double-check! **Impact:** Address *all* points, even if the answer is NO. Provide concise explanations for YES answers. * **Is new feature added? Is existing feature changed?** YES, adds support for GPIO and LCD on the szpi-esp32s3 board. * **Impact on user (will user need to adapt to change)?** YES. Users will need to configure their application to use the new LCD driver. * **Impact on build (will build process change)?** Potentially YES. If a user selects the szpi-esp32s3 board, the build system will now incorporate the LCD driver and its dependencies. Mention any new configuration options introduced. * **Impact on hardware (will arch(s) / board(s) / driver(s) change)?** YES. Changes the szpi-esp32s3 board configuration to include GPIO and LCD support. Adds a new ST7789 LCD driver. Specify the specific changes to the board configuration files. * **Impact on documentation (is update required / provided)?** YES. This PR includes documentation for the GPIO and LCD configurations. Briefly describe what documentation was added and where it's located. * **Impact on security (any sort of implications)?** NO. * **Impact on compatibility (backward/forward/interoperability)?** Potentially YES if the new driver or board configuration conflicts with existing configurations. Explain further if applicable. If NO, state NO. * **Anything else to consider?** None. (Or mention anything relevant). **Testing:** * **Build Host(s):** Provide the *full* details: e.g., "Linux Ubuntu 22.04, x86_64, GCC 11.3.0" * **Target(s):** Be specific: e.g., "esp32s3, szpi-esp32s3:default" Mention if you used a simulator or real hardware. * **Testing logs before change:** Show the *relevant* logs demonstrating the lack of GPIO/LCD functionality. For example, attempt to access the framebuffer and show the error. If the driver didn't exist before, state that. * **Testing logs after change:** * **GPIO:** The provided test is a good start. However, clarify *how* you verified the pin levels with hardware (e.g., multimeter, logic analyzer). Include the expected output of the hardware verification alongside the software commands. * **LCD:** Showing the `fb` command output is good. But *also* describe *what* you saw on the physical LCD screen. Don't just rely on the picture. The reviewer may not be able to access the image, or the image may not clearly demonstrate functionality. Be explicit: "The LCD displayed the NuttX framebuffer test pattern correctly, showing colored rectangles and text as expected." * **CI:** If the CI is running, link to the CI run. If not, explain why. By providing more complete and detailed information in your PR, you make it easier for reviewers to understand your changes, verify their correctness, and ultimately merge your contribution into NuttX. Remember, the more information you provide upfront, the less back-and-forth you'll have with reviewers. -- 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
