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

espressif/mcpwm: fix compile error #14942

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

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Nov 26, 2024

Summary

espressif/mcpwm: fix compile error

continue work of PR #14938

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

ci-check

continue work of PR apache#14938

Signed-off-by: chao an <[email protected]>
@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small labels Nov 26, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 26, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details.

Here's a breakdown of what's missing:

  • Summary: While it mentions fixing a compile error and referencing a prior PR, it's insufficient. It needs to explain what compile error was fixed, what functional part of the MCPWM driver was changed, and how the change fixes the error.

  • Impact: Simply stating "N/A" is unacceptable. Each impact item needs to be explicitly addressed with either "NO" or "YES" followed by a description if "YES." Even if there's no user-facing change, for example, the submitter should state "Impact on user: NO" for clarity. The fix likely impacts the build (at least it should fix a compile error), so that needs to be detailed. It might also impact hardware if the driver behavior changes.

  • Testing: While "ci-check" suggests CI testing was done, it's not sufficient. The requirements clearly ask for testing logs before and after the change. These logs demonstrate the problem being fixed and the successful resolution. The details of the build host and target are also missing.

In short, the PR needs more detail and explicit answers to the requirements checklist to be considered complete.

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 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.

3 participants