nuttxpr commented on PR #15576: URL: https://github.com/apache/nuttx/pull/15576#issuecomment-2594335533
[**\[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 of the *what*, it lacks crucial details regarding the *why*. Here's a breakdown of the missing information: * **Insufficient Summary:** * **Missing "Why":** The summary states *what* was removed (wd_cancel_irq()), and briefly *how* it relates to a lock change. It fails to explain *why* this removal is necessary. Why is the lock change needed? What problem did it solve? Why does the lock change allow for the removal of wd_cancel_irq()? What were the drawbacks of the previous implementation? * **Missing Context:** What functional part of the code is affected? "sched/wdog" is a starting point, but more detail is needed. Does this relate to the watchdog timer driver? How is it used? * **Missing Issue References:** Are there any related issues in either the NuttX or NuttX-apps repositories? * **Incomplete Impact Assessment:** "N/A" is insufficient. Even if there is no user-facing impact, the PR author needs to explicitly state that and justify it. For example: * **Impact on user:** NO - This is an internal change to the scheduler and watchdog timer implementation and has no direct impact on user applications. * **Impact on build:** Potentially YES - Removing a function *could* impact the build if other parts of the code rely on it (even if indirectly). This needs to be addressed. If there is no impact, explain why. * **Impact on hardware:** Likely NO, but needs explicit confirmation. * **Impact on documentation:** Potentially YES - If `wd_cancel_irq()` was documented, the documentation needs updating. * **Impact on security:** Needs explicit consideration. Does this change have any security implications, even indirectly? * **Impact on compatibility:** Needs explicit consideration. Does this break backward compatibility? * **Insufficient Testing:** "ci-check" is not enough. While CI is important, the PR needs to demonstrate *how* the change was tested locally. * **Missing Build Host Details:** What operating system, CPU architecture, and compiler version were used for testing? * **Missing Target Details:** Which architectures and boards were tested? Which configurations? * **Missing Logs:** "Testing logs before change" and "Testing logs after change" sections are empty. What tests were run? What were the results? Show evidence that the change works as intended and doesn't introduce regressions. Even a simple "before" and "after" demonstrating expected behavior is better than nothing. In short, the PR lacks crucial information to be properly reviewed and accepted. The author needs to provide more context, justification for the change, and evidence of thorough testing. -- 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