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

sched/mqueue: choose either mqueue or sysv at one time #14830

Closed
wants to merge 1 commit into from

Conversation

zyfeier
Copy link
Contributor

@zyfeier zyfeier commented Nov 17, 2024

Summary

Only one initialization is needed for mqueue and sysv,to reduce the usage of .bss.

Impact

mqueue

Testing

./tools/configure.sh sim:citest pass

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

nuttxpr commented Nov 17, 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. "Only one initialization is needed" is vague. Why was multiple initialization happening before? What specific change was made to address it? How does the single initialization now work? Which files were modified?
    • Missing issue references if applicable.
  • Impact:

    • Simply stating "mqueue" is insufficient. Does this affect the user? The build process? Hardware? Documentation? Security? Compatibility? Each of these requires a "YES/NO" answer and a description if "YES".
  • Testing:

    • Insufficient build host information. What OS, CPU architecture, and compiler version were used?
    • "sim:citest pass" is better than nothing but still minimal. What specific tests were run? Were only the citest tests run? What about other relevant tests?
    • Critically, the "Testing logs before change" and "Testing logs after change" sections are empty. Without these, there's no evidence provided that the change actually fixes a problem or improves anything. These logs are crucial for reviewers to understand the impact of the change.

In short: The PR needs significantly more detail to be considered complete. It needs to clearly articulate the problem being solved, the specific solution implemented, the full impact of the change, and provide concrete evidence (logs) that the change works as intended.

@@ -49,12 +49,11 @@

struct msgpool_s
{
#ifndef CONFIG_DISABLE_MQUEUE
#ifdef CONFIG_DISABLE_MQUEUE_SYSV
Copy link
Contributor

Choose a reason for hiding this comment

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

but, the user could disable both message queue api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ifneq ($(CONFIG_DISABLE_MQUEUE)$(CONFIG_DISABLE_MQUEUE_SYSV),yy)
CSRCS += mq_initialize.c mq_waitirq.c mq_recover.c
endif

when both disabled, mq_initialize.c will not compile

Copy link
Contributor

Choose a reason for hiding this comment

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

if so, why we need change the source code

Only one initialization is needed for mqueue and sysv,
use to reduce .bss usage.

Signed-off-by: zhangyuan29 <[email protected]>
@@ -36,7 +36,7 @@ config DISABLE_MQUEUE

config DISABLE_MQUEUE_SYSV
bool "Disable System V message queue support"
default DISABLE_MQUEUE
default y
Copy link
Contributor

Choose a reason for hiding this comment

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

why need change the default setting

@@ -49,12 +49,11 @@

struct msgpool_s
{
#ifndef CONFIG_DISABLE_MQUEUE
#ifdef CONFIG_DISABLE_MQUEUE_SYSV
Copy link
Contributor

Choose a reason for hiding this comment

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

if so, why we need change the source code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Board: arm Board: xtensa 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