Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

segger: rm spin_lock_irqsave(NULL) in drivers/segger/config/SEGGER_RTT_Conf.h #15479

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Jan 9, 2025

Summary

segger: rm spin_lock_irqsave(NULL) in drivers/segger/config/SEGGER_RTT_Conf.h

Impact

segger
img_v3_02ic_b58e3a17-88ed-4b8d-9d5a-a2673a3d405l

Testing

ci

@github-actions github-actions bot added Area: Drivers Drivers issues Size: S The size of the change in this PR is small labels Jan 9, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 9, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides a summary of the what, it lacks crucial information regarding the why.

Here's a breakdown of what's missing:

  • Summary:

    • Missing: The PR summary needs to explain why spin_lock_irqsave(NULL) is being removed. What problem does this fix? What were the negative consequences of having it there? What was the original intent of that code, and why is it no longer necessary or correct?
    • Missing: What functional part of the code is being changed? It mentions drivers/segger/config/SEGGER_RTT_Conf.h, but what does that file control? What aspects of the Segger functionality are affected?
    • Missing: How does the removal of spin_lock_irqsave(NULL) work? What mechanisms replace it, if any?
    • Missing: Links to related NuttX or NuttX Apps issues or pull requests. Even if there isn't a directly related issue, it's helpful to link to discussions or documentation that provides context.
  • Impact:

    • Insufficient: The image provided doesn't give enough context. What does "segger" refer to in the image? Is this a build log excerpt? An error message? It's not clear what's being shown. Explicitly answer all the impact questions (user, build, hardware, documentation, security, compatibility) with "NO" or "YES" and provide detailed explanations where necessary.
    • Missing: Explicit "NO" or "YES" answers to all impact questions. Just saying "segger" and providing an image isn't sufficient.
  • Testing:

    • Insufficient: "ci" isn't enough information. Specify the CI environment. Which build hosts and targets were tested? What specific tests were run? The CI logs themselves should be linked, not just a mention of "ci."
    • Missing: "Testing logs before change" and "Testing logs after change" are completely empty. This is a critical part of demonstrating that the change has the intended effect and doesn't introduce regressions. Even if the change seems small, demonstrable proof is required.

In short, the PR needs significantly more detail and context to meet the NuttX requirements. It needs to clearly articulate the rationale behind the change, its effects, and provide verifiable evidence that it works as intended.

@xiaoxiang781216 xiaoxiang781216 merged commit 8fd4bea into apache:master Jan 9, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants