nuttxpr commented on PR #3014: URL: https://github.com/apache/nuttx-apps/pull/3014#issuecomment-2693998717
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details. Here's a breakdown of what's missing and how to improve it: **Missing Information:** * **Summary:** * **Why is the change necessary?** What problem does this real-time thread stats output solve? Is it for debugging, performance monitoring, or something else? Justify the change. * **What functional part of the code is being changed?** Which file(s) and function(s) are modified? (e.g., `pthread_stats.c`, `task_start()`, etc.) * **How does the change exactly work?** Explain the implementation details. Did you add a new system call? Modify an existing function? Describe the mechanism for periodic output and the effect of the `-q` flag. * **Related Issues:** Are there any related NuttX or NuttX Apps issues? If not, explicitly state "None." * **Impact:** The PR mentions impact on the user, but it needs to address *all* the impact points, even if the answer is "NO." For each "NO," briefly explain why. For example: * **Impact on build:** NO (No changes to Makefiles or build scripts) * **Impact on hardware:** NO (No hardware-specific changes) * **Impact on documentation:** YES (Documentation needs to be updated to explain the new `-q` option and how to interpret the real-time output) * **Impact on security:** NO (No changes to security-related code) * **Impact on compatibility:** NO (No changes to APIs or behavior that would affect existing applications, assuming that's the case) * **Testing:** * **Build Host(s):** Be specific. "Locally" isn't sufficient. Provide details about your build host OS, CPU architecture, and compiler version (e.g., "Linux x86_64, GCC 12.2.0"). * **Target(s):** Be specific about the SAMV71 configuration used. Which board? Which NuttX configuration? (e.g., "samv71-xplained, nshtest_full config"). * **Testing logs:** The PR claims to have tested, but provides no logs. Include *actual* logs demonstrating the behavior before and after the change. Show the output with and without the `-q` flag. The logs should clearly illustrate the change in functionality. **Example of an improved PR description:** ``` ## Summary This change adds real-time thread statistics output to help developers monitor and debug application performance. The `pthread_stats` command is modified to periodically output thread statistics by default. A new `-q` (quiet) option is introduced to suppress this output. This addresses the need for a dynamic view of thread activity without requiring manual polling. No related issues. The implementation modifies `apps/system/pthread_stats/pthread_stats.c`. A timer and a new output function are added to print the statistics every 100ms when the `-q` flag is not set. The `-q` flag is handled during command-line argument parsing. ## Impact * Is new feature added? YES (Real-time thread statistics output) * Impact on user: YES (Users can now view thread statistics dynamically. They can use the `-q` flag to disable the periodic output if needed.) * Impact on build: NO (No Makefile or build script changes.) * Impact on hardware: NO (No hardware-specific modifications.) * Impact on documentation: YES (The documentation for the `pthread_stats` command will be updated to describe the new real-time output and the `-q` option.) * Impact on security: NO (No security implications.) * Impact on compatibility: NO (Existing applications that don't use the `-q` flag will see the new output, but this should not break their functionality.) ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux x86_64, GCC 12.2.0 * Target(s): samv71-xplained, nshtest_full config Testing logs before change: ``` $ pthread_stats ... (old output) ... ``` Testing logs after change: ``` $ pthread_stats ... (initial output) ... ... (output after 100ms) ... ... (output after 200ms) ... $ pthread_stats -q ... (old output, no periodic updates) ... ``` ``` By providing specific details and addressing all the required points, you create a much stronger PR that is easier for reviewers to understand and approve. Remember to replace the example logs with your actual test results. -- 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]
