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

bt_atomic: use small lock in wireless/bluetooth/bt_atomic.c #14985

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

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Nov 29, 2024

Summary

bt_atomic: use atomic macro to replace wireless/bluetooth/bt_atomic.c

Impact

bt

Testing

ci

@github-actions github-actions bot added Area: Bluetooth 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 description does not adequately meet the NuttX requirements. While it provides some information, it's far too brief and lacks crucial details. Here's a breakdown of what's missing:

  • Summary: "bt_atomic: use small lock in wireless/bluetooth/bt_atomic.c" is a title, not a summary. It states what the change is, but not why it's necessary, how it works, or its potential impact. There's no mention of related issues.

  • Impact: Simply stating "bt" is insufficient. It needs to specify the impact on users, build process, hardware, documentation, security, and compatibility. Even if the answer is "NO" for most of these, it should be explicitly stated.

  • Testing: "ci" suggests it relies on Continuous Integration testing. While CI is important, it's insufficient on its own. The PR needs to provide specific details about the local testing environment (host OS, compiler, target architecture, board) and show the testing logs before and after the change, demonstrating the improvement or fix. Simply stating "ci" doesn't provide evidence of proper testing.

To be acceptable, the PR description needs significant expansion, providing the missing context and details as outlined in the NuttX requirements.

Copy link
Contributor

@anchao anchao left a comment

Choose a reason for hiding this comment

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

Emm... I might remove bt_atomic.c and replace all of them with stdatomic's API, but this change is also fine for me.

@xiaoxiang781216
Copy link
Contributor

Emm... I might remove bt_atomic.c and replace all of them with stdatomic's API, but this change is also fine for me.

Yes, it's better to remove bt_atomic.c and change bt_atomic to macro to avoid the function invocation cost.

@@ -35,78 +35,54 @@

bt_atomic_t bt_atomic_incr(FAR bt_atomic_t *ptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

change to macro

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
Area: Bluetooth 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.

4 participants