nuttxpr commented on PR #14764: URL: https://github.com/apache/nuttx/pull/14764#issuecomment-2473593056
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR appears to **partially** meet the NuttX requirements. While it provides a reasonable summary and identifies the impacted area, it lacks crucial details in the Impact and Testing sections. Here's a breakdown of what's missing and how to improve it: **Missing Information:** * **Impact:** The PR only mentions "mm/iob" as the impacted area. It needs to specifically address *all* the impact points listed in the requirements: * Is this a new feature or a change to an existing one? * User impact (will users need to change anything in their applications?) * Build impact (any changes to the build process or configuration?) * Hardware impact (does it affect specific architectures, boards, or drivers?) * Documentation impact (does this require documentation updates? Were they provided?) * Security impact (are there any potential security implications?) * Compatibility impact (backward/forward compatibility concerns?) * Anything else to consider? * **Testing:** The testing information is insufficient. While it mentions a related NuttX Apps PR, it doesn't provide the *results* of the testing. The logs before and after the change are empty. This section needs to include: * Specific target details (architecture, board, configuration). "qemu-armv8a:nsh_smp" is a start, but more details might be necessary. * Actual log output demonstrating the behavior before and after the change, highlighting the improvement or fix. This is crucial for verifying the change works as intended. Simply stating "test by ..." isn't enough. **How to Improve:** * **Impact:** Fill in all the required impact categories with specific "YES/NO" answers and detailed explanations where applicable. Even if the answer is "NO," briefly explain why (e.g., "Impact on user: NO - This is an internal change and does not affect user APIs."). * **Testing:** Provide concrete test results. Include the actual log output before and after the change. Clearly demonstrate how the change addresses the issue or implements the new feature. Consider adding more test cases if necessary to cover different scenarios. **Example of Improved Testing Section:** ``` Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Ubuntu 20.04, x86_64, GCC 9.4.0 * Target(s): QEMU ARMv8-A (Cortex-A53), nsh_smp configuration Testing logs before change: ``` <Insert relevant log output showing the problem before the change. For example, demonstrating contention or performance issues related to the semcount.> ``` Testing logs after change: ``` <Insert relevant log output showing the improved behavior after the change. For example, demonstrating improved performance or the absence of contention.> ``` This PR fixes the contention issues observed previously. The logs after the change show a significant improvement in throughput/responsiveness/etc. (Be specific about the improvement). ``` By providing more complete information in the Impact and Testing sections, the PR will be much easier to review and more likely to be accepted. -- 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]
