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

note: Remove enter_critical_section from the sched_note module #14926

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Nov 25, 2024

Summary

noterpmsg: Remove enter_critical_section from the sched_note module to avoid recursive calls.

Impact

note

Testing

ci ostest

@github-actions github-actions bot added Area: Drivers Drivers issues Size: S The size of the change in this PR is small labels Nov 25, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 25, 2024

[Experimental Bot, please feedback here]

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

  • Insufficient Summary: While the summary describes what was changed, it doesn't explain why. What problem did the recursive calls cause? What functionality was affected? Were there any bug reports (NuttX issue references) related to this?

  • Missing Impact Details: Simply writing "note" under Impact is inadequate. Each impact category (user, build, hardware, documentation, security, compatibility) needs a YES/NO answer followed by a description if the answer is YES. Even if the answer is NO for all, it should be explicitly stated (e.g., "Impact on user: NO"). Consider the implications of removing the critical section. Could this lead to race conditions? Does it affect interrupt handling?

  • Inadequate Testing: "ci ostest" isn't sufficient. What specific tests were run? Provide actual log output before and after the change. What build host was used? What target architecture and board configuration? The logs should demonstrate that the problem existed before the change and is resolved after the change. Just saying "ci ostest" doesn't prove anything without the supporting details and logs.

In short, the PR needs to be significantly more detailed to meet the requirements. It needs to provide the missing context and evidence to justify the change and demonstrate its effectiveness.

@xiaoxiang781216 xiaoxiang781216 merged commit eb65c5a into apache:master Nov 26, 2024
27 checks passed
@hujun260 hujun260 deleted the apache_24 branch December 1, 2024 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues 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