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

imxrt_serial: use small lock in arch/arm/src/imxrt/imxrt_serial.c #14995

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

Conversation

hujun260
Copy link
Contributor

Summary

imxrt_serial: use small lock in arch/arm/src/imxrt/imxrt_serial.c

Impact

imxrt_serial

Testing

ci ostest

@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 Nov 29, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 29, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. The provided information is far too brief. Here's a breakdown of what's missing:

  • Summary: Lacks crucial details. Why is this change necessary? What problem does it solve? How does using a "small lock" work and what are the benefits? Mentioning the specific file changed is helpful, but not sufficient. Issue references are missing.

  • Impact: Simply stating "imxrt_serial" is not an impact assessment. All the yes/no questions are unanswered. This section needs significant expansion to explain how the change affects users, the build process, hardware, documentation, security, and compatibility. Even if the answer to many of these is "NO," it's crucial to explicitly state that and briefly justify why.

  • Testing: "ci ostest" is insufficient. While CI testing is important, the PR should provide specific details on the local testing performed. What host operating system, compiler, and target hardware were used? The "Testing logs before change" and "Testing logs after change" sections are empty. These should contain actual log output demonstrating the issue before the change and the improvement after the change. Ideally, there would be some evidence of improved performance or reduced resource usage if a "small lock" is intended to optimize something.

In short, the PR needs much more detail to be acceptable. It needs to clearly explain the motivation, implementation details, potential impact, and provide concrete evidence of testing and verification.

@@ -1919,6 +1919,8 @@ static struct pm_callback_s g_serial_pmcb =
};
#endif

static spinlock_t g_imxrt_serial_lock = SP_UNLOCKED;
Copy link
Contributor

Choose a reason for hiding this comment

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

move to imxrt_uart_s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.

3 participants