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

riscv: remove up_set_current_regs/up_current_regs #14962

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hujun260
Copy link
Contributor

Summary

reason:
up_set_current_regs initially had two functions:

1: To mark the entry into an interrupt state.
2: To record the context before an interrupt/exception. If we switch to a new task, we need to store the upcoming context regs by calling up_set_current_regs(regs).

Currently, we record the context in other ways, so the second function is obsolete. Therefore, we need to rename up_set_current_regs to better reflect its actual meaning, which is solely to mark an interrupt.

Impact

riscv

Testing

ci ostest

@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: M The size of the change in this PR is medium labels Nov 27, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 27, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it could be improved with more detail.

Strengths:

  • Clear Reason: The summary explains the "why" behind the change, the obsolete functionality, and the desired outcome.
  • Specific Impact: Identifies the affected architecture (RISC-V).
  • Testing Performed: Indicates CI and ostest were used.

Weaknesses (and suggestions for improvement):

  • Insufficient Detail in Summary: While the reason is clear, the "what" and "how" are less so. Specifically describe which function is being renamed to what. For example: "This PR renames up_set_current_regs to up_enter_interrupt to more accurately reflect its sole purpose of marking interrupt entry."
  • Missing Detail in Impact: "riscv" is too brief. Specify which RISC-V boards/configurations were tested. Are there other architectures that aren't impacted and why? While you state there's no impact on the user, build process, or documentation, explicitly stating "NO" for each category as per the template is preferred for clarity. Consider the impact on compatibility – is this a breaking change for any out-of-tree code that might rely on the old function name?
  • Missing Test Logs: Providing actual test logs, even abbreviated ones, significantly strengthens the PR. Show specific outputs demonstrating the functionality before and after the change. For example, demonstrate that interrupt handling still functions correctly after the rename. Even a simple "passed ostest before/after" with relevant log snippets is better than nothing.
  • Missing Build Host Information: Specify the development environment used for building and testing (OS, Compiler, etc.).

By adding this missing detail, the PR will be significantly improved and easier to review.

@hujun260 hujun260 force-pushed the apache_33 branch 4 times, most recently from 0042f7d to adb3632 Compare November 27, 2024 10:11
arch/risc-v/include/irq.h Outdated Show resolved Hide resolved
arch/risc-v/include/irq.h Outdated Show resolved Hide resolved
Copy link
Contributor

@pussuw pussuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

reason:
up_set_current_regs initially had two functions:

1: To mark the entry into an interrupt state.
2: To record the context before an interrupt/exception. If we switch to a new task, we need to store the upcoming context regs by calling up_set_current_regs(regs).

Currently, we record the context in other ways, so the second function is obsolete. Therefore, we need to rename up_set_current_regs to better reflect its actual meaning, which is solely to mark an interrupt.

Signed-off-by: hujun5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants