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.

Sign up for GitHub

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

Jump to bottom

Fix IPv6 Eth MAC KSZ8851SNL IDF 4.4 (IDFGH-12400) #13426

Closed
wants to merge 2 commits into from
Closed

Fix IPv6 Eth MAC KSZ8851SNL IDF 4.4 (IDFGH-12400) #13426

wants to merge 2 commits into from

Conversation

ewpa
Copy link

@ewpa ewpa commented Mar 20, 2024

ESP32 (I tested S3) is not reachable via its IPv6 address using SPI Ethernet MAC KSZ8851SNL. MAC filters all non-local frames except FF:FF... but Neighbor Solicitation enters on MAC 33:33... Solution is to permit RX multi-cast frames during MAC init. Also extend basic Ethernet example to assign IPv6 address if IPv6 is enabled in LWIP menuconfig. This fix tested on IDF 4.4.

ewpa added 2 commits March 20, 2024 19:45
@ewpa
Change non-promiscuous address filtering of KSZ8851SNL Ethernet MAC
4584421 
from "hash perfect" to "perfect with multi-cast address passed" when
IPv6 is configured under menuconfig.  This permits ingress of Ethernet
frames with MAC 33:33.x which in turn allows us to receive IPv6
Neighbor Solicitation messages.  Without this change we never send
Neighbor Advertisements and are effectively unreachable over IPv6.
@ewpa
Assign IPv6 addresses in basic Ethernet example when enabled in
9a9f9a8 
menuconfig.
@CLAassistant
Copy link

CLAassistant commented Mar 20, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions GitHub Actions
Copy link

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Assign IPv6 addresses in basic Ethernet example when enabled in":
    • summary looks empty
    • type/action looks empty
    • body must have leading blank line
  • the commit message "Change non-promiscuous address filtering of KSZ8851SNL Ethernet MAC":
    • summary looks empty
    • type/action looks empty
    • body must have leading blank line
  • the commit message "Fix(lwip):bugfix for add config for tcp ooseq bufs":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix(lwip):bugfix for change default value for ip ttl":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix(tools): Use a construct dependency compatible with Python 3.12":
    • type/action should start with a lowercase letter
    • type/action should be one of [change, ci, docs, feat, fix, refactor, remove, revert, test]
  • the commit message "Fixed BLE scan request ifs timer error in coexistence scenarios":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fixed xQueueSemaphoreTask assert when deinit host during scan":
    • summary looks empty
    • type/action looks empty
  • the commit message "Merge branch 'bugfix/BLEQABR23-35_v4.4' into 'release/v4.4'":
    • probably contains Jira ticket reference (BLEQABR23-35). Please remove Jira tickets from commit messages.
  • the commit message "ble_mesh: update ble mesh ble adv type":
    • type/action should be one of [change, ci, docs, feat, fix, refactor, remove, revert, test]
  • the commit message "bugfix(spi_flash): Fix build error when octal flash is enabled,":
    • type/action should be one of [change, ci, docs, feat, fix, refactor, remove, revert, test]
    • body must have leading blank line
  • the commit message "doc(spi_flash): hide unsupported optional features":
    • type/action should be one of [change, ci, docs, feat, fix, refactor, remove, revert, test]
  • the commit message "fix(WiFi): update esp32s3 rom ld":
    • scope/component should be lowercase without whitespace, allowed special characters are _ / . , * - .
  • the commit message "fix(ble/controller): Fixed tx count in direct test mode on ESP32-C3 and ESP32-S3":
    • body must have leading blank line
  • the commit message "fix(coex): fix esp32 ble scan interrupted by Wi-Fi, reset Wi-Fi connectionless pm status":
    • summary appears to be too long
  • the commit message "fix(esp_phy): Fixed BLE TX 2M problem causing by phy_wifi_enable_set()":
    • body's lines must not be longer than 100 characters
    • body must have leading blank line
  • the commit message "fix(esp_wifi): Fix timer index out of bound issue causing execution of timer function to fail":
    • summary appears to be too long
  • the commit message "fix(esp_wifi):Fix WDT when esp_supp_dpp_start_listen called multiple times":
    • summary looks empty
    • type/action looks empty
  • the commit message "kconfig: support plain comment in the menu":
    • type/action should be one of [change, ci, docs, feat, fix, refactor, remove, revert, test]
  • the commit message "optimize(lwip):when psram is enable the number of ooseq is not limited":
    • summary looks empty
    • type/action looks empty
  • the commit message "spi_flash: fixed issue that enabling HPM-DC by default may cause app unable to restart":
    • summary appears to be too long
    • type/action should be one of [change, ci, docs, feat, fix, refactor, remove, revert, test]

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).
⚠️ Please consider squashing your 250 commits (simplifying branch history).
Messages
📖 This PR seems to be quite large (total lines of code: 336245), you might consider splitting it into smaller PRs

👋 Hello ewpa, we appreciate your contribution to this project!

📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.
🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.
Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

This GitHub project is public mirror of our internal git repository

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.

Generated by 🚫 dangerJS against 9a9f9a8

@ewpa ewpa marked this pull request as draft March 20, 2024 19:55
@ewpa ewpa changed the base branch from master to release/v4.4 March 20, 2024 19:57
@espressif-bot espressif-bot added the Status: Opened Issue is new label Mar 20, 2024
@github-actions github-actions bot changed the title Fix IPv6 Eth MAC KSZ8851SNL IDF 4.4 Fix IPv6 Eth MAC KSZ8851SNL IDF 4.4 (IDFGH-12400) Mar 20, 2024
@ewpa ewpa closed this Mar 20, 2024
@ewpa ewpa reopened this Mar 20, 2024
@ewpa ewpa marked this pull request as ready for review March 20, 2024 20:01
@kostaond
Copy link
Collaborator

@ewpa thank you for your contribution. However, we cannot merge directly to v4.4 branch. There is specific process for it. We need to fix master branch first and then the fixes are backported. Unfortunately, v4.4 is in maintenance period. This release only accepts backports for high severity issues or security issues. I'm afraid proposed fix does not fall within the specified categories... However, let's fix it for the current releases!

Please close this PR and open new one merging to master branch. Please also fix commit message as suggested by github-actions.

@ewpa
Copy link
Author

ewpa commented Apr 20, 2024

Closing as advised. Will review later IDF release.

@ewpa ewpa closed this Apr 20, 2024
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: Won't Do This will not be worked on and removed Status: Opened Issue is new labels Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@kostaond kostaond

Labels
Resolution: Won't Do This will not be worked on Status: Done Issue is done internally
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ewpa @CLAassistant @kostaond @espressif-bot