Skip to content

Conversation

@yangsong8-ai
Copy link

@yangsong8-ai yangsong8-ai commented Oct 12, 2024

Summary

only when devfd and stdfd both are tty, we call tcsetattr

Impact

cu

Testing

internal testing

only when devfd and stdfd both are tty, we call tcsetattr

Signed-off-by: zhanghu5 <zhanghu5@xiaomi.com>
@nuttxpr
Copy link

nuttxpr commented Oct 12, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

Meeting Requirements? Partially

Summary:

The provided summary is insufficient. It only states a specific condition (only when devfd and stdfd both are tty, we call tcsetattr) but lacks crucial information like:

  • Necessity: Is this a bug fix, optimization, or new feature? Why is this change needed?
  • Functionality: Which part of the codebase is affected (e.g., drivers, networking, specific module)?
  • Implementation: How does this change work? What was the previous behavior?

Impact:

The Impact section is entirely missing. Please address all the listed points:

  • New/changed features?
  • User impact and required adaptations
  • Build process changes
  • Hardware (architecture, boards, drivers) impact
  • Documentation needs
  • Security implications
  • Compatibility (backward, forward, interoperability)
  • Other considerations

Testing:

While the structure for presenting testing logs is provided, the actual logs are missing. Please include relevant logs from before and after the change to demonstrate the issue and the fix/improvement. Additionally, specify:

  • Build Host(s): OS, CPU architecture, compiler details
  • Target(s): Architecture (simulator, real hardware), board configuration

To Improve this PR:

  1. Expand the Summary: Clearly explain the motivation, affected functionality, and implementation details of your change.
  2. Complete the Impact Section: Address all the points to highlight potential consequences of this change.
  3. Provide Testing Logs: Include actual logs demonstrating the issue and the solution, along with details about your build and target environments.

By providing this missing information, you will make it much easier for reviewers to understand and evaluate your PR.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @yangsong8-ai :-)

Please provide some context next time (Summary description part), what is the problem, how it occurs, show what happened before and after the fix (Testing description part).

@xiaoxiang781216
Copy link
Contributor

let's ignore ci check error which is fixed by #2712 to avoid wasting github resources.

@xiaoxiang781216 xiaoxiang781216 merged commit 5f3e3fe into apache:master Oct 13, 2024
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.

5 participants