nuttxpr commented on PR #15086: URL: https://github.com/apache/nuttx/pull/15086#issuecomment-2527418774
[**\[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 some sections could be more explicit. **Strengths:** * **Clear problem description:** The summary explains the regression, its root cause (incorrect register storage), and the attempted solutions. * **Solution explained:** The approach for armv6-m, armv7-m, and armv8-m is outlined. The reasoning behind removing PRIMASK support is provided. * **Links to related PR:** The summary references the PR that introduced the regression. **Weaknesses/Areas for Improvement:** * **Impact Section is Insufficient:** The impact section simply lists architectures. It needs to explicitly address *all* the points in the template: user impact, build impact, hardware impact, documentation, security, compatibility. For example, will removing PRIMASK support break any existing user code? Are there any backwards compatibility issues? Even if the answer is "NO" for each item, it should be stated explicitly. * **Testing Section Lacks Detail:** "CI" is not enough information. List the specific CI configurations that were tested (e.g., QEMU-armv7-m, some specific hardware board). Provide snippets of the relevant log output demonstrating the fix. The "Testing logs before change" and "Testing logs after change" sections are empty. Show *specific* commands used for testing. * **Missing Issue Reference:** While the regressing PR is linked, an issue referencing the regression itself would be helpful for tracking. * **Summary could be more concise:** While the background is helpful, the summary itself could be shorter by focusing on the core problem and solution. For example: "Fixes a regression introduced in #14881 where svc calls could trigger a hard fault due to incorrect register storage. The fix involves using `(*running_task)==NULL` as a condition on armv6-m and removing PRIMASK support for armv7-m/armv8-m to ensure correct register handling." **Recommendation:** Expand the Impact and Testing sections with specific details. Consider adding a related NuttX issue. Condense the summary slightly while retaining essential information. -- 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]
