nuttxpr commented on PR #14740: URL: https://github.com/apache/nuttx/pull/14740#issuecomment-2470119300
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not adequately meet the NuttX requirements. Here's why and how to fix it: **Problems:** * **Summary is insufficient:** It only states *what* was done, not *why*. What problem does this solve? What part of the code was changed (files/modules)? How does it work (e.g., what mechanism is used to enable interrupt profiling)? * **Impact is overly simplistic:** "None" is unlikely to be entirely true. At a minimum, enabling profiling impacts performance (overhead). There might be memory usage implications. Think critically about *all* potential impacts. * **Testing is a repeat of the summary:** It doesn't provide any actual test results. "Testing logs before change" and "Testing logs after change" are empty. What was the expected outcome? What was the observed outcome? Show evidence that the changes work. **How to improve the PR description:** **Summary:** * Explain the motivation for adding gprof support. Was it requested by a user? Is it for debugging/performance analysis? * Specify the modified files/modules. E.g., "Modified the `arch/arm/src/xxx` files and the `sched/` directory to support gprof profiling..." * Describe the implementation details. E.g., "Interrupt profiling is enabled by setting CONFIG_GPROF_PROFILING, which adds a timer interrupt handler that samples the program counter. Instruction profiling is enabled by adding the -pg compiler option or setting CONFIG_GPROF_ALL..." * If applicable, link to a related NuttX issue. **Impact:** * **Impact on user:** "Users will need to enable CONFIG_GPROF_PROFILING or compile with -pg to use this feature. Enabling profiling will introduce performance overhead." * **Impact on build:** "The build system will now recognize the CONFIG_GPROF_PROFILING option." (Or if there are other build system changes, mention them). * **Impact on hardware:** Be specific. Does this work on all architectures? Only certain ones? "This feature is implemented for ARM and RISC-V architectures." (Or whichever are supported). * **Impact on documentation:** "Documentation updates are needed to explain how to enable and use gprof profiling." (And ideally, you would include those documentation updates in the PR). * **Impact on security, compatibility:** Consider these carefully. Even if the answer is NO, explicitly state it. If there are compatibility concerns (e.g., with older NuttX versions), explain them. **Testing:** * **Build Host(s):** Provide details. "Linux, x86_64, GCC 12.2" * **Target(s):** Provide specific board and configuration information. "QEMU-rv32ima, `nsh` configuration." "STM32F4 Discovery, `hello` configuration." * **Testing logs before change:** If applicable, show that profiling *wasn't* working before. Or state "N/A - Feature did not exist previously." * **Testing logs after change:** Show the output of `gprof` or other evidence that profiling is now working. Include relevant snippets of the output, not entire log files. Demonstrate both interrupt and instruction profiling if implemented. By providing this level of detail, you make it much easier for reviewers to understand and approve your PR. -- 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]
