nuttxpr commented on PR #13565: URL: https://github.com/apache/nuttx/pull/13565#issuecomment-2365211827
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR as presented does **not** meet the NuttX requirements fully. Here's why: **Missing Information:** * **Summary:** * **Why is this change necessary?** Is it a new feature request? Are there existing issues with LCD support? * **What functional part is changed?** Be specific - mention the driver(s) modified or added. * **How does the change work?** Explain the technical implementation (e.g., "Added an I2C driver for the LCD backpack and integrated it with the framebuffer driver"). * **Impact:** * The current description is too vague. * **Which architectures/boards are affected *specifically*?** * **Build impact?** Are there new Kconfig options? * **Documentation?** Was any added/updated to reflect the new LCD support? * **Testing:** * **Insufficient detail!** * **Build Hosts:** List the specific OS, CPU architecture, and compiler versions used for testing. * **Target(s):** Provide the exact architecture (e.g., `armv7-m`), board name (e.g., `stm32f4discovery`), and the NuttX configuration used (e.g., `stm32f4discovery_defconfig`). * **"Testing logs before change" are irrelevant.** There wouldn't have been LCD output before this feature. **Recommendations:** 1. **Expand the Summary:** Provide a clear, technical overview of the changes made. 2. **Be Specific about Impact:** Clearly list *all* affected architectures, boards, drivers, Kconfig options, and documentation. 3. **Detailed Testing Info:** Follow the template, providing complete information about your build and target environments. **Example Improvement:** ``` ## Summary This PR adds support for 16x2 LCD displays with I2C backpacks to the STM32F401RC-RS485 board. It achieves this by: 1. **Adding a new I2C driver** for the specific LCD backpack used (mention the chip/model). 2. **Integrating the LCD driver** with the NuttX framebuffer driver to enable text output. 3. **Adding a new board configuration option** (`CONFIG_VIDEO_LCD1602_I2C`) to enable this feature. This change addresses [link to NuttX issue or feature request, if applicable]. ## Impact * **New feature:** Support for 16x2 LCDs with I2C backpacks. * **Impact on user:** Users with the supported LCD and board can now use it for display output. * **Impact on build:** * New Kconfig option: `CONFIG_VIDEO_LCD1602_I2C` * **Impact on hardware:** * **Architectures:** ARM (specifically ARMv7-M) * **Boards:** STM32F401RC-RS485 * **Impact on documentation:** * Added a new section to the STM32F401RC-RS485 board documentation explaining how to configure and use the LCD. * **Impact on security:** None * **Impact on compatibility:** None ## Testing I confirm that changes are verified and work as intended: * **Build Host(s):** * OS: Ubuntu 20.04 * CPU: Intel Core i7-10700K * Compiler: GCC 9.4.0 * **Target(s):** * Architecture: armv7-m * Board: stm32f401rc-rs485 * Configuration: `stm32f401rc_rs485_defconfig` with `CONFIG_VIDEO_LCD1602_I2C=y` **Testing logs after change:** ``` nsh> slcd "Hello NuttX" Opening /dev/slcd0 for read/write access Attributes: rows: 2 columns: 16 nbars: 0 max contrast: 0 max brightness: 1 Clear screen WRITING: 0000: 1b5b46 .[F Set brightness to 1 Print [Hello NuttX] WRITING: 0000: 1b5b471b5b30304c1b5b4548656c6c6f 204e75747458 .[G.[00L.[EHello NuttX Test complete nsh> ``` ``` By providing this level of detail, you make it much easier for the NuttX maintainers to understand, review, and ultimately merge your contribution. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
