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

esp32s3: don't clear pending interrupts on eg. up_putc #14975

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

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Nov 28, 2024

Summary

esp32s3: don't clear pending interrupts on eg. up_putc

Fixes #14872

Impact

esp32s3

Testing

esp32s3-devkitc

@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small labels Nov 28, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 28, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. While it addresses some points, it lacks crucial details.

Here's a breakdown of what's missing:

  • Summary: While it mentions the issue fixed, it doesn't explain why clearing pending interrupts was a problem, what functional part of the code was changed (e.g., serial driver, interrupt handler), and how the change works (e.g., "Modified the UART interrupt handler to only clear interrupts related to the current transmission").

  • Impact: The impact section is far too brief. It just mentions the affected architecture (esp32s3). It needs to explicitly address all the impact points with "YES" or "NO" and provide descriptions where necessary. For example:

    • Is new feature added? NO
    • Is existing feature changed? YES (Serial driver behavior modified)
    • Impact on user? Potentially YES (if relying on specific interrupt clearing behavior) - needs further explanation.
    • Impact on build? Likely NO, but should be stated explicitly.
    • Impact on hardware? YES (esp32s3) - needs to specify which hardware specifically within the esp32s3.
    • Impact on documentation? Possibly YES if the behavior change impacts users - needs clarification.
    • Impact on security? Potentially YES/NO depending on the nature of the bug - needs analysis and justification.
    • Impact on compatibility? Possibly YES if applications relied on the old behavior. Needs explanation.
  • Testing: It only mentions the target. It needs to include:

    • Build host details (OS, CPU, Compiler).
    • More specific target details (board:config).
    • Crucially, it's missing the actual testing logs before and after the change. This is essential to demonstrate the fix. "your testing logs here" is just a placeholder and needs to be replaced with real output. The logs should clearly show the problem before the change and the corrected behavior after the change.

In short, the PR needs significantly more detail to meet the NuttX requirements. It needs to clearly explain the problem, the solution, the impact of the change, and provide evidence of successful testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: xtensa Issues related to the Xtensa 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] esp32s3 up_putc clears all interrupts
2 participants