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
