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/signal: Simplified Implementation for SIGEV_THREAD_TID #13530

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Fix-Point
Copy link
Contributor

Summary

This commit simplified the implementation for SIGEV_THREAD_TID and added the missing siginfo initializer.

Impact

POSIX timer with SIGEV_THREAD_TID flag.

Testing

Tested on QEMU using rt-tests/cyclictest.

This patch is to make Coverity happy.

Signed-off-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
This commit simplified the implementation for SIGEV_THREAD_TID.

Signed-off-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
@lupyuen
Copy link
Member

lupyuen commented Sep 19, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Here's why:

  • Insufficient Summary:

    • The summary is too brief. While it mentions the changes made, it lacks crucial details about why these changes were necessary. Was there a bug? Did the previous implementation have limitations?
    • It doesn't specify which functional part of the code is affected. Is it related to the kernel, a specific driver, or a library?
  • Incomplete Impact Assessment:

    • Feature Addition/Change: The impact section vaguely states "POSIX timer with SIGEV_THREAD_TID flag." Be explicit. Is this a new feature? If so, describe its functionality. If it's a change, explain how the existing feature is impacted.
    • Other Impacts: The PR only addresses impact on users, leaving other aspects like build process, hardware, documentation, security, and compatibility completely unaddressed. Even for user impact, it lacks specifics. How will users interact with this new/modified feature?
  • Insufficient Testing Details:

    • Limited Testing Environment: The PR only mentions testing on QEMU. Specify the QEMU version and the specific architecture being emulated. Ideally, testing should be done on real hardware as well, if possible.
    • Missing Build Host Information: Details about the build host are entirely missing. This includes the operating system, CPU architecture, and the compiler used (including the version).
    • Absent Testing Logs: The PR provides placeholders for testing logs instead of the actual logs. This makes it impossible to verify the claimed functionality or identify potential issues.

To improve this PR:

  1. Expand the Summary: Explain the rationale behind the changes. Mention the specific code sections (files, functions) that were modified. If applicable, link to a relevant NuttX issue.
  2. Thoroughly Address Impact: Analyze and describe the impact on all the listed aspects (build, hardware, documentation, security, compatibility). Be specific about how each area is affected.
  3. Provide Comprehensive Testing Information:
    • List all build host environments used, including OS, CPU, and compiler details.
    • Mention the specific QEMU version and the emulated architecture.
    • Include actual testing logs from both before and after the changes.
    • If feasible, add testing results from real hardware platforms.

This commit simplified thread ID dispatching logic by integrating it into the `nxsig_dispatch` function.

Signed-off-by: ouyangxiangzhen <ouyangxiangzhen@xiaomi.com>
@xiaoxiang781216
Copy link
Contributor

@Fix-Point let's revert the commit 9a9d0a6 too.

@github-actions github-actions bot added the Size: S The size of the change in this PR is small label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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