Skip to content

Conversation

@jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Dec 4, 2024

Summary

after #13606 , up_schedule_sigaction behavior got error when exception direct, as we did not save the last regs to speed up.

first issue is smp sig delivery,
#14363

after chip specific vectors, still issue on sig deliver inside irq handler.
#14514

As we always trigger a pendsv to do context switch, should also do signal requrements context switch inside pendsv handler.

test-case in mps2 qemu:
apache/nuttx-apps#2877

after this patch, the custom vector table for arm-v6/7/8m smp, no longer required

Impact

now compatible with send signal in irq handler。

Testing

CI-test,
local qemu mps2(by apps cmocka case),
spersense(pass ostest after revert ARCH_HAVE_CUSTOM_VECTORS in ARCH_CHIP_CXD56XX)

@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 Dec 4, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 4, 2024

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements, but is missing some crucial information. Here's a breakdown:

Strengths:

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.

@xiaoxiang781216 xiaoxiang781216 linked an issue Dec 4, 2024 that may be closed by this pull request
1 task
Nested irq possible cause readytorun not match with regs

Signed-off-by: buxiasen <buxiasen@xiaomi.com>
For exception directly, tcb->xcp.regs should not be used.

Signed-off-by: buxiasen <buxiasen@xiaomi.com>
@jasonbu jasonbu marked this pull request as ready for review December 5, 2024 10:15
@xiaoxiang781216 xiaoxiang781216 merged commit 617fee6 into apache:master Dec 5, 2024
25 checks passed
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.

[BUG] hardware timers broken for ARM devices

4 participants