Skip to content

arch/arm/rp23xx: Fix IRQ prioritize #16214

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

shtirlic
Copy link
Contributor

@shtirlic shtirlic commented Apr 15, 2025

Fixes the reg addresses that were copied from armv6m rp2040 to the actual armv8m spec

Summary

Current IRQ prioritize routine failed to compile and broken.
CONFIG_ARCH_IRQPRIO=y

Impact

Working IRQ prioritize on rp23xx

Fixes the reg addresses that were copied from armv6 rp2040 to the actual
armv8 spec

Signed-off-by: Serg Podtynnyi <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small labels Apr 15, 2025
@nuttxpr
Copy link

nuttxpr commented Apr 15, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to mostly meet the NuttX requirements, but it's missing some crucial information.

Here's a breakdown of what's good and what needs improvement:

Strengths:

  • Clear problem statement: The summary clearly identifies the issue (broken IRQ prioritization) and the root cause (incorrect register addresses).
  • Impact on user is addressed: The impact is stated as "Working IRQ prioritize on rp23xx," which implies a positive change for users on that platform.
  • Configuration option mentioned: Referencing CONFIG_ARCH_IRQPRIO=y is helpful for understanding the context.

Weaknesses:

  • Incomplete Summary: While the problem is stated, the "how it works" part of the summary is missing. How were the register addresses fixed? What were the incorrect values and what are the correct ones?
  • Missing Issue References: Are there any related NuttX or NuttX Apps issues? Even if there aren't, explicitly stating "None" is helpful.
  • Impact Section is Severely Underdeveloped: Almost all "Impact" questions are unanswered. This is the most significant area for improvement. At a minimum, address the impact on:
    • Hardware: Specifically mention rp23xx (and any other affected architectures).
    • Documentation: Does this change require documentation updates?
    • Compatibility: Are there any backward compatibility concerns?
  • Missing Testing Details: The "Testing" section is entirely empty. This is critical. Provide:
    • Build Host Details: OS, CPU architecture, compiler version.
    • Target Details: Specific rp23xx board and configuration used for testing.
    • Testing Logs: "Before" and "after" logs demonstrating the fix. Even simple logs showing the IRQ prioritization working (or not working before the fix) would be valuable.

How to Improve the PR:

  1. Expand the Summary: Explain the how of the fix.
  2. Add Issue References: If none exist, state "None."
  3. Complete the Impact Section: Address all the questions, even if the answer is "NO." Be explicit.
  4. Provide Detailed Testing Information: Fill in the Build Host(s), Target(s), and crucially, the testing logs. Show evidence that the fix works.

By addressing these points, the PR will be much stronger and more likely to be accepted.

@xiaoxiang781216 xiaoxiang781216 merged commit 8ac09bc into apache:master Apr 15, 2025
25 checks passed
@shtirlic shtirlic deleted the rp23xx-fixprioirq branch April 15, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture 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