nuttxpr commented on PR #15048: URL: https://github.com/apache/nuttx/pull/15048#issuecomment-2517477453
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR appears to mostly meet the NuttX requirements, but is missing some crucial information. Here's a breakdown: **Strengths:** * **Clear Summary of the "Why":** The PR clearly explains the bug introduced by #13606 and the subsequent issues (#14363 and #14514) it caused regarding signal delivery. The proposed solution of handling signal context switching within the PendSV handler is also explained. * **Provides Context and Links:** Referencing related issues and the test-case PR is helpful for reviewers. * **Impact Description:** The impact on functionality (compatibility with sending signals in IRQ handlers) is stated. * **Mentions Testing Environments:** CI, local qemu mps2, and spersense (WIP) are mentioned. **Weaknesses:** * **Missing "What" and "How" in Summary:** While the "why" is well-explained, the summary lacks specifics on *what* parts of the code are changed and *how* the PendSV handler modification achieves the desired outcome. More detail is needed. * **Incomplete Impact Assessment:** Many impact fields are missing details. Simply stating "NO" or "YES" is insufficient. Even if there's no impact, briefly stating why is helpful for reviewers. The "YES" for compatibility needs further explanation – what kind of compatibility is improved? * **Missing "Before/After" Logs:** While testing environments are mentioned, the required "before" and "after" logs are absent. This makes it impossible to verify the claims. The link to the `nuttx-apps` PR helps, but logs demonstrating the fix in `nuttx` itself are essential. * **Build Host Details Missing:** The testing section needs to specify the build host details as requested (OS, CPU, compiler). * **Target Details Incomplete:** While `qemu mps2` and `spersense` are mentioned, specific board configurations and architectures are missing. For example: `qemu-mps2:sim:nsh` or `spresense:boardname:configname`. The "WIP" on spersense should be clarified – if testing is incomplete, the PR might not be ready for review. **Recommendation:** Before merging, the PR author should address the missing information: * **Expand the Summary:** Describe the code changes (files modified, functions added/changed) and the mechanism within the PendSV handler that addresses the signal delivery issues. * **Complete the Impact Assessment:** Provide details for *all* impact categories, even if the answer is "NO." Explain the improved compatibility. * **Include "Before/After" Logs:** Add the requested logs demonstrating the issue and the fix within the `nuttx` repository itself. * **Provide Build Host Details:** Specify the OS, CPU, and compiler used for building. * **Clarify Target Details:** Provide full target specifications (arch:board:config) for all tested platforms. Explain the status of spersense testing and whether it blocks merging. By providing this missing information, the PR will better meet the NuttX requirements and facilitate a smoother review 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
