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

Use small lock to protect usbdev and endpoint in mips. #15548

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

wangzhi-art
Copy link
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

Use small lock to protect usbdev and endpoint in mips.

Impact

nuttx/arch/mips/src/pic32mx/pic32mx_usbdev.c

Testing

CI

@github-actions github-actions bot added Arch: mips Issues related to the MIPS architecture Size: S The size of the change in this PR is small labels Jan 14, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 14, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the change, it lacks crucial details. Here's a breakdown:

  • Insufficient Summary: "Use small lock to protect usbdev and endpoint in mips" is too vague. Why was this lock necessary? What problem did it solve? What specific USB device and endpoint are affected? It should also mention related issues if any exist.
  • Missing Impact Assessment: The "Impact" section is severely lacking. Simply stating the file path is not enough. It needs to address all the impact points explicitly (user, build, hardware, documentation, security, compatibility). For each, it should clearly state YES or NO and provide a description if YES. Even if the answer is NO for all, it should explicitly state that.
  • Inadequate Testing: "CI" is not sufficient. While CI testing is important, the PR needs to show specific test results demonstrating the fix. It should include relevant logs from before the change showing the problem, and logs from after the change demonstrating the fix. The build host and target details are also missing. Just stating "mips" is not enough; the specific board and configuration should be mentioned.

Example of how the testing section should look:

Testing

Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
Target(s): MIPS sim:pic32mx795f512l

Testing logs before change:

<logs showing the problem, e.g., data corruption, crashes, etc.>


Testing logs after change:

```

In short, the PR needs to be significantly more detailed to meet the NuttX requirements. It should provide a comprehensive explanation of the change, its impact, and clear evidence that the change works as intended.

arch/mips/src/pic32mx/pic32mx_usbdev.c Outdated Show resolved Hide resolved
@xiaoxiang781216 xiaoxiang781216 merged commit ac2b495 into apache:master Jan 15, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: mips Issues related to the MIPS 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.

5 participants