nuttxpr commented on PR #16257: URL: https://github.com/apache/nuttx/pull/16257#issuecomment-2828419635
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does a good job of outlining the changes and their impact. However, it could be improved by providing more specific information in a few areas to fully meet the NuttX requirements. Here's a breakdown of what's good and what needs improvement: **Strengths:** * **Clear Summary:** The summary provides a good overview of the changes and their purpose. * **Impact Addressed:** The impact section covers most of the required areas. * **Testing Information Provided:** Testing information is included, although it could be more detailed. * **Concerns Highlighted:** Listing concerns proactively is helpful for reviewers. **Areas for Improvement:** * **Summary - Related Issues:** If there's a related NuttX issue that prompted this change, include a link. Even if it's a feature request from a user (like @acassis), creating an issue to track it is recommended. This helps with traceability and organization. * **Impact - Be More Specific:** While the impact section addresses the required points, it's too general. Instead of "It should have no meaningful impact," explain *why* it shouldn't have an impact. For example, "This is an optional feature enabled via Kconfig, so users who don't select it won't be affected." For the default being enabled, justify the decision: "The default is enabled because splash screens are a common request and improve the user experience. This can be easily disabled in Kconfig." * **Impact - Build:** Address the build impact of adding the Python script. Does it require any new dependencies? How is it integrated into the build system? The mention of `make clean` suggests a potential build system issue that needs further investigation and clearer explanation. * **Testing - More Detail:** Provide more specific testing details: * **Build Host(s):** Specify the OS, CPU architecture, and compiler version used for building. For example: "Linux (Ubuntu 20.04), x86_64, GCC 9.4.0" * **Target(s):** Be very specific about the target architecture, board, and configuration. For example: "sama5d2-xplained:sama5d27c_d1g_lcd" * **Logs:** The "Testing logs before change" and "Testing logs after change" sections are empty. Include relevant log snippets that demonstrate the splash screen functionality before and after the change. Even simple logs showing the splash screen being displayed (or not) are helpful. * **Concerns - Actionable Items:** Frame concerns as questions or requests for feedback. For example: 1. "The logo files are very long due to `checkpatch.sh` requirements. Are there alternative ways to store these files that would reduce their length while remaining compliant?" 2. "The current implementation assumes the external splashscreen object file is built by the user. Is there a preferred mechanism within NuttX for handling out-of-tree object files?" 3. "The Python script is my first attempt at Python. I'm open to suggestions for improving its style and efficiency." 4. "Currently, the logo size must match the LCD size. I'd like to explore supporting different color depths and scaling. Any suggestions on how to approach this?" 5. "A `make clean` is required when changing the NXlogo bitmap size. I suspect this is a dependency issue in the Makefile. Could someone provide guidance on how to fix this?" By addressing these points, you can create a more comprehensive and informative PR that meets the NuttX requirements and makes it easier for reviewers to understand and approve your changes. This will also help ensure the quality and maintainability of the NuttX codebase. -- 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
