nuttxpr commented on PR #16307: URL: https://github.com/apache/nuttx/pull/16307#issuecomment-2848622469
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. This PR description mostly meets the NuttX requirements, but could be improved in a few areas: **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the change reasonably well. * **Impact Description:** The impact section addresses most of the required points and provides specific details about the changes. * **Testing Information:** Provides build host and target information. Includes example output demonstrating the functionality. **Weaknesses:** * **Missing Issue References:** If this PR addresses a specific issue in either the NuttX or NuttX-apps repositories, those issue numbers should be linked in the Summary. Even if it doesn't directly address an open issue, it might be related to a closed one, or it's possible a new issue should be created to track this enhancement. * **Incomplete Testing Logs:** While the "after" logs demonstrate the functionality, "before" logs are missing. Ideally, the "before" logs would show that the buttons *didn't* work or that only one button was detected. This demonstrates the positive impact of the change. Also, simply stating "not working" isn't sufficient. Show what happened before (e.g., no output, incorrect output, error messages). * **Missing Compiler Information:** The testing environment lists the OS and kernel versions, but not the compiler used (GCC, Clang, version). This information is important for reproducibility. * **Impact on Documentation:** If the documentation needs updating to reflect these new buttons (e.g., in the board's configuration documentation), the PR description should mention it. If documentation updates are included in the PR, that should be stated explicitly. * **Clarity on "following the same logic"**: The phrase "following the same logic as the stm32-f103minimum" could be more precise. What specific logic was replicated? Mentioning specific functions or code structures would be helpful. * **No mention of `stm32_bringup.c` changes in the summary:** The summary mentions changes to `stm32_buttons.c` but doesn't mention the crucial addition of `/dev/buttons` to `stm32_bringup.c`. This is a significant part of the change and should be included in the summary. **Suggested Improvements:** * Add any related NuttX/NuttX-apps issue links. If there are none, consider creating one. * Provide "before" logs showing the previous behavior. * Include the compiler version used for testing. * Clarify the impact on documentation (does it need updating? is it updated in this PR?). * Be more specific about the logic copied from the stm32-f103minimum example. Name functions or code structures. * Include the changes to `stm32_bringup.c` in the summary. By addressing these weaknesses, the PR description will be more complete and helpful for reviewers. A well-written PR description significantly increases the chances of it being reviewed and merged quickly. -- 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]
