nuttxpr commented on PR #15044: URL: https://github.com/apache/nuttx/pull/15044#issuecomment-2516982023
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details. Here's a breakdown: **Missing/Insufficient Information:** * **Summary:** While the summary explains the "what," it doesn't fully explain the "why." Why is higher resolution needed? What problem does the original timer difference in the lower half cause? More context is needed. Mentioning the global clock generator is good, but explain *how* this change addresses the issue of timers not starting from 0. Be more explicit. * **Impact:** "N/A" is almost never acceptable. Think critically. Even seemingly small changes can have impacts. * **Impact on hardware:** This PR *definitely* impacts hardware as it deals with timers and clock generators. Specify which architectures/boards are affected. * **Impact on compatibility:** Does this change break any existing drivers or applications that rely on the old timer behavior? * **Impact on user:** Will users notice any changes in timer behavior? * **Testing:** "ci-check" is insufficient. Provide actual log output, even if it's just a snippet demonstrating the key difference before and after the change. The CI check itself doesn't tell reviewers anything about *what* was tested or *how* it was verified. List specific test cases. Mention the host OS, compiler, target architecture, and board configuration used for testing. **How to Improve the PR:** 1. **Expand the Summary:** Provide a more compelling reason for the change. Explain the problem with the existing implementation (e.g., inaccuracies, limitations) and how this PR addresses those issues. Be explicit about the benefits of the higher resolution and removing the wall clock difference. 2. **Detail the Impact:** Remove "N/A" and address each impact point specifically. For example: * **Impact on hardware:** "Affects timer drivers on [list architectures/boards]. Requires a global clock generator." * **Impact on compatibility:** "Potentially breaks drivers/applications that rely on the absolute value of the lower-half timer. Testing performed on [list drivers/applications] confirmed no regressions." (Or if it *does* break compatibility, explain how users can adapt.) * **Impact on user:** "Users should see improved timer accuracy." 3. **Provide Meaningful Test Logs:** Include snippets of log output demonstrating the timer behavior before and after the change. Show how the higher resolution is achieved and how the wall clock difference is eliminated. Don't just rely on CI. 4. **Specify Test Environment:** Be explicit about the host and target environments used for testing. Example: "Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11. Target: RISC-V, qemu-rv32ima_smode." By addressing these points, the PR will be much stronger and easier for reviewers to evaluate. Clear and comprehensive information is crucial for getting PRs merged efficiently. -- 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
