nuttxpr commented on PR #14502: URL: https://github.com/apache/nuttx/pull/14502#issuecomment-2436760143
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description is missing some key elements to meet NuttX requirements. Here's a breakdown and suggestions for improvement: **Summary:** * **Good:** You clearly state the "why" (bug fix) and "what" (timer initialization changes). * **Needs Improvement:** * **How:** The description of the solution is vague. Be more explicit about the code changes. For example: * _Instead of:_ "Enable timer and irq finally..." * _Try:_ "Modified timer initialization to register the callback function **before** enabling the timer interrupt and unmasking the IRQ. This ensures the callback is in place when the first interrupt fires." * **Issue References:** If this fixes a reported bug, link the relevant NuttX issue. **Impact:** * **Insufficient:** * **Specifics:** "armv8_r generic timer init" is too broad. List affected architectures, boards, or drivers. * **Consider all points:** Address *every* impact category (even if it's "NO"). This demonstrates thoroughness. * **User Impact:** Will applications using timers see any behavioral differences? * **Build Impact:** Are any new configuration options added? * **Documentation:** Does anything need updating in the docs? **Testing:** * **Incomplete:** * **Build Hosts:** Specify the OS, compiler, and versions used for testing. * **Targets:** Provide the exact board and configuration ("board:config"). * **Logs:** Include relevant snippets of logs demonstrating the issue *before* your change, and the correct behavior *after*. Highlight the important differences. **Example Enhancements:** ``` ## Summary This PR addresses a bug in the armv8_r generic timer initialization where the timer interrupt could fire before the callback function was registered, leading to a system hang. The fix ensures that the timer callback is registered *prior* to enabling the timer interrupt and unmasking the associated IRQ. This guarantees the callback is in place to handle the interrupt. This PR also removes unnecessary masking of the generic timer within the one-shot timer's ISR, as IRQs/FIQs are already disabled in that context. **Fixes Issue:** #[Insert Issue Number Here (if applicable)] ## Impact * **Is new feature added?** NO * **Is existing feature changed?** YES - armv8_r generic timer initialization * **Impact on user:** Potentially YES - Applications relying on very precise timer behavior immediately after boot might be affected. * **Impact on build:** NO * **Impact on hardware:** YES - armv8_r architectures using the generic timer. Specifically tested on [Board Name]:[Configuration]. * **Impact on documentation:** YES - The timer initialization section should be updated to reflect the new order of operations. A note about potential timing differences for early timer usage should be added. * **Impact on security:** NO * **Impact on compatibility:** NO ## Testing **Build Host:** * OS: Ubuntu 20.04 * Compiler: GCC 9.3.0 **Target:** * Architecture: armv8_r * Board: [Board Name] * Configuration: [Configuration Name] **Testing Logs (Before Change):** ``` [Relevant log lines showing the issue, e.g., interrupt firing prematurely, system hang] ``` **Testing Logs (After Change):** ``` [Relevant log lines demonstrating correct timer behavior, callback execution, no hangs] ``` ``` By providing specific details, you make it much easier for reviewers to understand your changes and verify their correctness, leading to a smoother PR process. -- 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]
