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

nsh:support wait command to wait task exit. #2612

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

anjiahao1
Copy link
Contributor

Summary

'wait' command will block nsh from running until pid1 and pid2 finish. This is useful for parallel requirements to perform certain tasks

Impact

nsh

Testing

usage like:
  nsh> sleep 10 &
  [5:100]
  nsh>set pid1=$!
  nsh>sleep 15 &
  [6:100]
  nsh>set pid2=$!
  nsh>wait $pid1 $pid2

@nuttxpr
Copy link

nuttxpr commented Sep 27, 2024

[Experimental Bot, please feedback here]

This PR description, while introducing a potentially useful feature, does not meet the NuttX requirements. Here's why and how to improve it:

Missing Information

  • Why is this change necessary? The summary states the "what" (adds a 'wait' command) but not the "why." Is there a specific use case or limitation in NSH this addresses? Explain the motivation behind the change.
  • What functional part of the code is changed? Is this a new NSH command? A modification to existing shell logic? Be specific about which files/modules are affected.
  • How does the change work? A brief explanation of the implementation is needed. Does 'wait' use polling, signals, or another mechanism to monitor processes?
  • Impact details are insufficient:
    • User Impact: While "nsh" is mentioned, clarify if users need to change existing scripts or commands due to this addition.
    • Build Impact: Will any build configurations or dependencies be affected by this change?
    • Other Impacts: The remaining impact sections (hardware, documentation, security, compatibility) lack any assessment. Even if the answer is "NO," state it explicitly.
  • Testing is inadequate:
    • Build Host details are missing: Provide the specific operating system, CPU architecture, and compiler used for testing.
    • Target details are missing: Specify the target architecture (e.g., simulated, RISC-V, ARM) and board configuration used for validation.
    • Testing logs are absent: Provide actual output from your tests both before and after the change. This demonstrates the problem the PR solves and that the solution works.

How to Improve

  1. Expand the Summary: Clearly articulate the problem this PR addresses and its benefits.
  2. Detail Code Changes: Briefly explain the technical implementation of the 'wait' command.
  3. Complete the Impact Assessment: Address each impact category, even if the answer is "NO."
  4. Provide Comprehensive Testing Information: Include specific details about your build host and target environments. Include actual testing logs to demonstrate the change's effectiveness.

By providing this missing information, you'll create a much stronger PR that's more likely to be reviewed and merged.

nshlib/nsh_wait.c Outdated Show resolved Hide resolved
nshlib/nsh_wait.c Outdated Show resolved Hide resolved
@anjiahao1 anjiahao1 force-pushed the 092703 branch 3 times, most recently from 128c689 to e76938c Compare September 30, 2024 06:20
@anjiahao1 anjiahao1 force-pushed the 092703 branch 3 times, most recently from a714dcc to 562c04b Compare October 1, 2024 06:29
usage like:
  nsh> sleep 10 &
  [5:100]
  nsh>set pid1=$!
  nsh>sleep 15 &
  [6:100]
  nsh>set pid2=$!
  nsh>wait $pid1 $pid2

'wait' command will block nsh from running until pid1 and pid2 finish.
This is useful for parallel requirements to perform certain tasks

Signed-off-by: anjiahao <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit c916f16 into apache:master Oct 2, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants