nuttxpr commented on PR #3064: URL: https://github.com/apache/nuttx-apps/pull/3064#issuecomment-2830105623
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although more detail in the "Testing" section would strengthen it. Here's a breakdown of why and some suggestions: **Strengths:** * **Clear Summary:** The summary clearly outlines the changes and their purpose. The numbered list helps readability. * **Good Impact Description:** The impact section clearly identifies the added API and provides a diff, showing exactly what changed. It addresses several impact categories implicitly (new feature, impact on the user, impact on documentation if the API is documented elsewhere). * **Testing Section Present:** The testing section demonstrates effort to validate the changes. **Weaknesses/Areas for Improvement:** * **Incomplete Impact Assessment:** While the impact of the new API is addressed, the other listed impacts (build, hardware, security, compatibility) are not explicitly mentioned. Even if there is *no* impact, stating "NO" for each is helpful for reviewers. * **Insufficient Testing Detail:** The testing section is too high-level. While the code snippets give some context, it lacks concrete details. * **Build Host:** Specify the OS, CPU architecture, and compiler used for testing. * **Target:** Specify the target architecture and board/configuration. "sim" is not specific enough; which simulator? QEMU? Something else? * **"CI" is insufficient:** Mention which CI system (e.g., GitHub Actions, Travis CI) and provide links to successful CI runs if available. Don't rely solely on reviewers to check CI status. * **Missing Logs:** The template requests "Testing logs before change" and "Testing logs after change". Include actual log output showing the behavior before and after your changes. This proves the fix works as intended. Demonstrate that the loop exits correctly after calling `orb_loop_exit_async()`. What happens without the fix? Does the loop hang? * **Selftest Explanation:** Briefly describe what the selftest does and how it exercises the new functionality. **Example Improvements for the Testing Section:** ``` ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 * Target(s): QEMU-based simulator for ARM Cortex-M4, STM32F4DISCOVERY board configuration The selftest creates a uORB loop in a separate thread and then uses `orb_loop_exit_async()` from the main thread to signal the loop to exit. The test then waits for the loop thread to join, verifying clean shutdown. Without the fix, the loop would hang indefinitely. Testing logs before change (loop hangs, manually terminated after 10 seconds): ``` [main thread]: Starting uORB loop thread... [uORB loop thread]: Entering orb_loop_run()... (10 seconds pass with no further output) ^C ``` Testing logs after change: ``` [main thread]: Starting uORB loop thread... [uORB loop thread]: Entering orb_loop_run()... [main thread]: Calling orb_loop_exit_async()... [uORB loop thread]: Received exit event. Exiting loop. [main thread]: uORB loop thread joined successfully. ``` CI: Passed on GitHub Actions (link to successful CI run here) ``` By adding these details, you significantly increase the reviewer's confidence in your changes and make the review process smoother. -- 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
