nuttxpr commented on PR #16207: URL: https://github.com/apache/nuttx/pull/16207#issuecomment-2803176982
[**\[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 is missing some key information. Here's a breakdown of what's good and what needs improvement: **Good:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the change. Referencing the original code source is helpful. * **Impact Section Mostly Complete:** You've addressed most of the impact points, especially highlighting the runtime bug fix. * **Testing Information Provided:** You specify the board and chip used for testing. **Needs Improvement:** * **Missing Issue References:** Are there any related NuttX or NuttX Apps issues? If so, these *must* be linked in the summary. If not, consider creating one to track the bug this PR fixes. * **Incomplete Testing Information:** While you mention the target, the host build environment details are missing (OS, CPU, compiler). Critically, you haven't provided any *actual* testing logs. "your testing logs here" is insufficient. Include real output showing the issue before the change and the corrected behavior after. This is crucial for reviewers. * **Impact - User, Build, Hardware, Docs, Security, Compatibility:** While you say "Fixes runtime bugs with ADC," you need to be more specific. * **Impact on user:** Will the user need to change any configuration or code to benefit from this fix? If not, explicitly state "NO." * **Impact on build:** Does this change require any build system modifications? If not, explicitly state "NO." * **Impact on hardware:** You mention a custom board. Specify if this change is *only* applicable to your custom board or if it affects other STM32H5 boards. * **Impact on documentation:** Does the documentation need updating to reflect this change? If not, explicitly state "NO." * **Impact on security:** Does this fix have any security implications? If not, explicitly state "NO." * **Impact on compatibility:** Does this change break any existing functionality or compatibility? If not, explicitly state "NO." * **"Anything else to consider?"**: While not strictly required, this section is a good place to mention any potential caveats or special considerations. In short, the PR needs more detailed testing information and explicit answers (even if "NO") for all impact categories. Adding linked issues would significantly improve the context and traceability of this change. -- 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
