nuttxpr commented on PR #16132: URL: https://github.com/apache/nuttx/pull/16132#issuecomment-2776196785
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to mostly meet the NuttX requirements, but needs some improvements. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the changes. The link to the hardware wiki is helpful. * **Impact Section Started:** You've identified the affected areas. However, this section needs significant expansion (see below). * **Testing Information Provided:** You've included build instructions, relevant config options, and mentioned CI. Including actual log output (even if brief) would strengthen this section. **Weaknesses:** * **Incomplete Impact Assessment:** You've listed the affected areas, but haven't fully addressed the required points. You *must* answer all the YES/NO questions and provide descriptions where necessary. Consider these questions: * **New Feature?** Yes, FT5X06 support. * **User Impact?** Likely yes. How will a user enable this feature? New config options? Does this replace another touchscreen driver? * **Build Impact?** Potentially yes, if new Kconfig options are added. * **Hardware Impact?** Yes, specifically the szpi-esp32s3 board. * **Documentation Impact?** Yes, documentation should be added explaining how to use this new driver. You mentioned this is planned; stating that explicitly in the PR is important. * **Security Impact?** Unlikely, but explicitly stating "NO" is important. * **Compatibility Impact?** Unlikely, but explicitly stating "NO" is important. * **Missing "Before" and "After" Logs:** You've included the build command, but not the actual *output*. Even small snippets demonstrating functionality before and after the change are crucial. Show something like touchscreen input being detected or working in an application. * **Vague CI Mention:** Just saying "CI" is insufficient. Specify which CI system is being used and ideally link to a successful run (or anticipated run) demonstrating the changes. * **Missing Issue References:** While not strictly required if no issue exists, it's good practice to create an issue first to discuss the change before creating a PR. **Recommendations:** 1. **Expand the Impact section:** Answer all questions thoroughly. Even for "NO" answers, briefly explain why. 2. **Include actual log output:** Show the output of your tests *before* and *after* the changes. Demonstrate the driver working. 3. **Clarify CI:** Specify the CI system and provide a link if possible. 4. **Consider creating a related issue:** This helps with discussion and tracking. Even if you create the issue concurrently with the PR, it's beneficial. 5. **Be explicit about the new config option:** Instead of "maybe," confirm the new config option (`lckfb-szpi-esp32s3:ft5x06` or similar) and explain how a user would enable it. 6. **State clearly in the impact section that documentation will be added in a follow-up commit/PR.** This shows you're aware of the requirement and plan to address it. By addressing these points, your PR will be much stronger and more likely to be accepted. -- 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
