Skip to content

Conversation

@xiaoxiang781216
Copy link
Contributor

Summary

turn off interface by checking IFF_UP flag isn't set: #1838

Impact

improve the compatibility

Testing

internal

@xiaoxiang781216 xiaoxiang781216 linked an issue Oct 6, 2024 that may be closed by this pull request
@github-actions github-actions bot added Area: Networking Effects networking subsystem Area: Board support Board support issues Size: S The size of the change in this PR is small labels Oct 6, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 6, 2024

[Experimental Bot, please feedback here]

This PR appears incomplete based on the provided description.

Here's why and what's missing:

  • Insufficient Detail: The summary and impact sections lack specifics.

    • Summary: While it links a related issue, the PR needs to clearly explain what functional part of the code is changed to achieve turning off the interface when IFF_UP isn't set.
    • Impact: Simply stating "improve the compatibility" is too vague. The PR needs to detail:
      • Which compatibility issues are addressed?
      • Is this backward compatible?
      • Are there any potential side effects for existing users?
  • Missing Testing Information: "Internal" testing is not sufficient for a proper review. The PR must include:

    • Specific build host details: OS, CPU architecture, compiler version used.
    • Specific target details: Architecture (simulated or real hardware), board name, and the configuration used.
    • Relevant log snippets: Don't just paste entire logs. Include specific portions that demonstrate the issue before the change and the successful outcome after the change.

To improve this PR:

  1. Expand the Summary: Provide a technical explanation of how the change works.
  2. Detail the Impact: Be specific about the compatibility improvements, potential user impact, and any other notable effects.
  3. Provide Complete Testing Information: Follow the template's guidelines to include all necessary build/target details and focused log snippets.

A well-documented PR makes it easier for reviewers to understand, verify, and ultimately merge your contribution.

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 @xiaoxiang781216 :-)

@xiaoxiang781216 xiaoxiang781216 linked an issue Oct 7, 2024 that may be closed by this pull request
1 task
turn off interface by checking IFF_UP flag isn't set:
apache#1838

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Copy link
Contributor

@GUIDINGLI GUIDINGLI left a comment

Choose a reason for hiding this comment

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

LGTM

@GUIDINGLI GUIDINGLI merged commit 77821fb into apache:master Oct 8, 2024
@xiaoxiang781216 xiaoxiang781216 deleted the tmp branch October 8, 2024 02:57
@fdcavalcanti
Copy link
Contributor

@jerpelea would be possible to backport this commit on 12.7?
The related commit on nuttx-apps was backported, but it seems this has not. This causes issues on our internal CI wifi tests.

Just tested the release with this commit cherry-picked and it solves the problem.

@xiaoxiang781216 any problem on that?

@jerpelea
Copy link
Contributor

@fdcavalcanti please test #14319

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Board support Board support issues Area: Networking Effects networking subsystem 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.

[BUG] wapi_set_ifdown failed Do we still need IFF_DOWN?

6 participants