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

3.0.0 Network Refactoring #8760

Merged
merged 59 commits into from
Mar 26, 2024
Merged

3.0.0 Network Refactoring #8760

merged 59 commits into from
Mar 26, 2024

Conversation

me-no-dev
Copy link
Member

Better explanation will come later

@VojtechBartoska VojtechBartoska added the Status: In Progress Issue is in progress label Nov 28, 2023
@VojtechBartoska VojtechBartoska added this to the 3.0.0-RC1 milestone Nov 28, 2023
Copy link
Contributor

github-actions bot commented Dec 15, 2023

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Add extra interface info for Printable":
    • summary looks empty
    • type/action looks empty
  • the commit message "Add periman string for network and "fix" mdns on the first ETH":
    • summary looks empty
    • type/action looks empty
  • the commit message "Apply suggestions from code review":
    • summary looks empty
    • type/action looks empty
  • the commit message "Cleanup":
    • summary looks empty
    • type/action looks empty
  • the commit message "Create ESP_NetworkInterface class and have Ethernet extending it":
    • summary looks empty
    • type/action looks empty
  • the commit message "Decouple network related libraries from WiFi":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix CMake issues":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix CMakekLists and rework lib menu dependencies":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix build errors":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix examples and WiFiUpdate":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix periman":
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix web server missing fs.h":
    • summary looks empty
    • type/action looks empty
  • the commit message "Get the correct interface MAC address for mDND Workstation service":
    • summary looks empty
    • type/action looks empty
  • the commit message "Guard WiFi classes and fix RMII ETH examples":
    • summary looks empty
    • type/action looks empty
  • the commit message "Guard WiFiProv lib to compile only on WiFi chips":
    • summary looks empty
    • type/action looks empty
  • the commit message "More fixes":
    • summary looks empty
    • type/action looks empty
  • the commit message "More fixes":
    • summary looks empty
    • type/action looks empty
  • the commit message "Move Client, Server and Udp out of WiFi":
    • summary looks empty
    • type/action looks empty
  • the commit message "Move back WiFiClient":
    • summary looks empty
    • type/action looks empty
  • the commit message "Remove unnecessary guard":
    • summary looks empty
    • type/action looks empty
  • the commit message "Rename AP methods":
    • summary looks empty
    • type/action looks empty
  • the commit message "Rename IPv6 getters to clarify that they are returning LinkLocal address":
    • summary looks empty
    • type/action looks empty
  • the commit message "Rename Network library":
    • summary looks empty
    • type/action looks empty
  • the commit message "Rename WiFi Server and UDP":
    • summary looks empty
    • type/action looks empty
  • the commit message "Rename WiFiClient and WiFiClientSecure":
    • summary looks empty
    • type/action looks empty
  • the commit message "Rename network classes":
    • summary looks empty
    • type/action looks empty
  • the commit message "Some fixes from merging master":
    • summary looks empty
    • type/action looks empty
  • the commit message "Split networking from WiFi (H2 can now use Ethernet)":
    • summary looks empty
    • type/action looks empty
  • the commit message "Switch AP to the new interface":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update CMakeLists.txt":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update CMakeLists.txt":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update ETH_TLK110.ino":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update NetworkManager.h":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update WiFiGeneric.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update on-push.sh":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update progress":
    • summary looks empty
    • type/action looks empty
  • the commit message "more fixes":
    • summary looks empty
    • type/action looks empty
  • the commit message "move back WiFiClient to rebase with master":
    • summary looks empty
    • type/action looks empty

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 59 commits (simplifying branch history).
⚠️ The Pull Request description looks very brief, please check if more details can be added.
Messages
📖 This PR seems to be quite large (total lines of code: 7256), you might consider splitting it into smaller PRs

👋 Hello me-no-dev, we appreciate your contribution to 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.

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 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.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against fb0041c

@VojtechBartoska
Copy link
Collaborator

Closes #8735

@me-no-dev me-no-dev added Status: Test needed Issue needs testing Status: Review needed Issue or PR is awaiting review and removed Status: In Progress Issue is in progress labels Mar 20, 2024
@lucasssvaz lucasssvaz added the hil_test Run Hardware Tests label Mar 22, 2024
@@ -17,6 +17,10 @@
Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA

*/
#pragma once
Copy link
Collaborator

Choose a reason for hiding this comment

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

#pragma once is necessary in a cpp file?

Suggested change
#pragma once

Copy link
Collaborator

@SuGlider SuGlider Mar 25, 2024

Choose a reason for hiding this comment

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

it seems to be a copy&paste result.
Nice work anyway! Kudos!

Copy link
Member Author

Choose a reason for hiding this comment

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

funny... I do not remember touching this file at all 😆 maybe it felt left out and decided to make a statement

Copy link
Collaborator

@lucasssvaz lucasssvaz left a comment

Choose a reason for hiding this comment

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

Looks very good. Just some suggestions. Everything I've tested so far works flawlessly. Will do more testing and report if anything is out of the ordinary.

libraries/Ethernet/examples/ETH_LAN8720/ETH_LAN8720.ino Outdated Show resolved Hide resolved
libraries/Ethernet/examples/ETH_TLK110/ETH_TLK110.ino Outdated Show resolved Hide resolved
libraries/Ethernet/src/ETH.h Outdated Show resolved Hide resolved
libraries/Network/src/NetworkClient.h Outdated Show resolved Hide resolved
libraries/Network/src/NetworkClient.h Outdated Show resolved Hide resolved
libraries/WiFi/src/WiFiGeneric.cpp Outdated Show resolved Hide resolved
* else error code
*/
/*
* Deprecated Methods
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we mark all the newly deprecated functions with the deprecated attribute ?
https://en.cppreference.com/w/cpp/language/attributes/deprecated

Copy link
Member Author

Choose a reason for hiding this comment

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

No. There is too much code out there that uses those functions. Let's roll out as-is and re-think deprecation with 3.1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just confirming. Is this supposed updated in this PR or is it a leftover from tests ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to update the libs for some things to work

@@ -0,0 +1,62 @@
/*
Server.h - Server class for Raspberry Pi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should update this line to reflect the actual description of this file. I think as long as the copyright and the license stays untouched it should be fine. (might be wrong though)

@@ -0,0 +1,115 @@
/*
Client.h - Base class that provides Client
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should update this line to reflect the actual description of this file. I think as long as the copyright and the license stays untouched it should be fine. (might be wrong though)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I am also a bit scared of touching other's headers, so I would rather not

Co-authored-by: Lucas Saavedra Vaz <32426024+lucasssvaz@users.noreply.github.com>
@lucasssvaz lucasssvaz removed the hil_test Run Hardware Tests label Mar 25, 2024
@me-no-dev me-no-dev merged commit f2026f1 into master Mar 26, 2024
40 checks passed
@me-no-dev me-no-dev deleted the feature/network_refactoring branch March 26, 2024 21:31
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Mar 27, 2024
* Create ESP_NetworkInterface class and have Ethernet extending it

* Update CMakeLists.txt

* Split networking from WiFi (H2 can now use Ethernet)

Now all libs have been checked yet. More to do on WiFi side

* Fix build errors

* Guard WiFi classes and fix RMII ETH examples

* Decouple network related libraries from WiFi

* Fix examples and WiFiUpdate

* Guard WiFiProv lib to compile only on WiFi chips

* Add periman string for network and "fix" mdns on the first ETH

* Revert back location of Client/Server/Udp in order to accept some PRs

* Fix periman

* Some fixes from merging master

* Fix web server missing fs.h

* Move Client, Server and Udp out of WiFi

* More fixes

* more fixes

* Fix CMakekLists and rework lib menu dependencies

* Fix CMake issues

* move back WiFiClient to rebase with master

* Update ETH_TLK110.ino

* Move back WiFiClient

* Update progress

* Update WiFiGeneric.cpp

* More fixes

* Switch AP to the new interface

* Cleanup

* Rename AP methods

* Add extra interface info for Printable

* Rename IPv6 getters to clarify that they are returning LinkLocal address

cc @sgryphon

* Rename network classes

cc @sgryphon

* Update NetworkManager.h

* Rename WiFi Server and UDP

* Rename WiFiClient and WiFiClientSecure

* Update CMakeLists.txt

* Update on-push.sh

* Rename Network library

* Remove unnecessary guard

* Get the correct interface MAC address for mDND Workstation service

* Apply suggestions from code review



---------

Co-authored-by: Me No Dev <me-no-dev@users.noreply.github.com>
Co-authored-by: Lucas Saavedra Vaz <32426024+lucasssvaz@users.noreply.github.com>
Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Mar 27, 2024
* Create ESP_NetworkInterface class and have Ethernet extending it

* Update CMakeLists.txt

* Split networking from WiFi (H2 can now use Ethernet)

Now all libs have been checked yet. More to do on WiFi side

* Fix build errors

* Guard WiFi classes and fix RMII ETH examples

* Decouple network related libraries from WiFi

* Fix examples and WiFiUpdate

* Guard WiFiProv lib to compile only on WiFi chips

* Add periman string for network and "fix" mdns on the first ETH

* Revert back location of Client/Server/Udp in order to accept some PRs

* Fix periman

* Some fixes from merging master

* Fix web server missing fs.h

* Move Client, Server and Udp out of WiFi

* More fixes

* more fixes

* Fix CMakekLists and rework lib menu dependencies

* Fix CMake issues

* move back WiFiClient to rebase with master

* Update ETH_TLK110.ino

* Move back WiFiClient

* Update progress

* Update WiFiGeneric.cpp

* More fixes

* Switch AP to the new interface

* Cleanup

* Rename AP methods

* Add extra interface info for Printable

* Rename IPv6 getters to clarify that they are returning LinkLocal address

cc @sgryphon

* Rename network classes

cc @sgryphon

* Update NetworkManager.h

* Rename WiFi Server and UDP

* Rename WiFiClient and WiFiClientSecure

* Update CMakeLists.txt

* Update on-push.sh

* Rename Network library

* Remove unnecessary guard

* Get the correct interface MAC address for mDND Workstation service

* Apply suggestions from code review

Co-authored-by: Lucas Saavedra Vaz <32426024+lucasssvaz@users.noreply.github.com>

* Tasmota changes

* 3.0.0 Network Refactoring (espressif#8760)

* Create ESP_NetworkInterface class and have Ethernet extending it

* Update CMakeLists.txt

* Split networking from WiFi (H2 can now use Ethernet)

Now all libs have been checked yet. More to do on WiFi side

* Fix build errors

* Guard WiFi classes and fix RMII ETH examples

* Decouple network related libraries from WiFi

* Fix examples and WiFiUpdate

* Guard WiFiProv lib to compile only on WiFi chips

* Add periman string for network and "fix" mdns on the first ETH

* Revert back location of Client/Server/Udp in order to accept some PRs

* Fix periman

* Some fixes from merging master

* Fix web server missing fs.h

* Move Client, Server and Udp out of WiFi

* More fixes

* more fixes

* Fix CMakekLists and rework lib menu dependencies

* Fix CMake issues

* move back WiFiClient to rebase with master

* Update ETH_TLK110.ino

* Move back WiFiClient

* Update progress

* Update WiFiGeneric.cpp

* More fixes

* Switch AP to the new interface

* Cleanup

* Rename AP methods

* Add extra interface info for Printable

* Rename IPv6 getters to clarify that they are returning LinkLocal address

cc @sgryphon

* Rename network classes

cc @sgryphon

* Update NetworkManager.h

* Rename WiFi Server and UDP

* Rename WiFiClient and WiFiClientSecure

* Update CMakeLists.txt

* Update on-push.sh

* Rename Network library

* Remove unnecessary guard

* Get the correct interface MAC address for mDND Workstation service

* Apply suggestions from code review



---------

Co-authored-by: Me No Dev <me-no-dev@users.noreply.github.com>
Co-authored-by: Lucas Saavedra Vaz <32426024+lucasssvaz@users.noreply.github.com>

---------

Co-authored-by: me-no-dev <hristo@espressif.com>
Co-authored-by: Me No Dev <me-no-dev@users.noreply.github.com>
Co-authored-by: Lucas Saavedra Vaz <32426024+lucasssvaz@users.noreply.github.com>
@sgryphon
Copy link
Contributor

sgryphon commented Mar 30, 2024

I know this has already been merged. It is still not working that well for IPv6. I tested across multiple network types:

Network Dual-Stack IPv6 IPv4 TLS Dual-Stack
IPv4 (Shadow) IPv4 Not Possible Yes Yes
Dual-stack+NAT64 (Astral) IPv4 (1) DNS fail Yes (1) DNS fail
IPv6+NAT64 (Wildspace) DNS fail DNS fail DNS fail DNS fail

(1) Dual-stack initially worked for some, but then started failing. I suspect it is lack of support for IPv6, and because I have DNS64 everything has an IPv6 address. Could be a race condition that worked before DNS64 was resolved.

Some of the errors:

Dual-stack+NAT64 network, IPv6 destination:

[ 69767][I][Core2Logger.cpp:198] log(): [Core2Logger] CORE2: Button 2, scenario 1, v0.1.0-156-gaf345f5
[ 69779][I][Core2Logger.cpp:198] log(): [Core2Logger] CORE2: Global IPv6 2407:8800:bc61:1340:a3a:f2ff:fe65:db28
[ 69794][I][Core2Logger.cpp:198] log(): [Core2Logger] CORE2: IPv4 192.168.1.154
[ 69802][I][Core2Logger.cpp:198] log(): [Core2Logger] CORE2: Link-Local IPv6 fe80::a3a:f2ff:fe65:db28%st1
sta: <UP>
      ether 08:3A:F2:65:DB:28
      inet 192.168.1.154 netmask 255.255.255.0 broadcast 192.168.1.255
      gateway 192.168.1.1 dns 192.168.1.1
      inet6 fe80::a3a:f2ff:fe65:db28%st1 type LINK_LOCAL
      inet6 2407:8800:bc61:1340:a3a:f2ff:fe65:db28 type GLOBAL
      inet6 fd7c:e25e:67e8:40:a3a:f2ff:fe65:db28 type UNIQUE_LOCAL
[ 69840][I][Core2Logger.cpp:198] log(): [Core2Logger] CORE2: DNS0 192.168.1.1
[ 69854][I][Core2Logger.cpp:198] log(): [Core2Logger] CORE2: DNS1 0.0.0.0
[ 69862][I][Core2Logger.cpp:198] log(): [Core2Logger] CORE2: URL: http://v6.ipv6-test.com/api/myip.php
[ 69875][D][HTTPClient.cpp:303] beginInternal(): protocol: http, host: v6.ipv6-test.com port: 80 url: /api/myip.php
[ 69885][D][HTTPClient.cpp:598] sendRequest(): request type: 'GET' redirCount: 0

[ 76427][E][NetworkManager.cpp:125] hostByName(): DNS Failed for 'v6.ipv6-test.com' with error '-5' and result '-1'
[ 81441][I][NetworkClient.cpp:269] connect(): select returned due to timeout 5000 ms for fd 48
[ 81450][D][HTTPClient.cpp:1163] connect(): failed connect to v6.ipv6-test.com:80
[ 81458][W][HTTPClient.cpp:1486] returnError(): error(-1): connection refused
[ 81465][E][Core2Logger.cpp:192] log(): [Core2Logger] CORE2: HTTP GET error -1: connection refused

@me-no-dev
Copy link
Member Author

@sgryphon given that you have a full environment and test case, maybe you can figure out what is missing? Try maybe some menuconfig options and rebuild the libs?

vortigont added a commit to vortigont/arduino-esp32 that referenced this pull request Sep 22, 2024
When running on WiFi-AP mode server's start() method returned true while in fact UDP listening socket was never created
Regression introduced in espressif#8760
Closes espressif#10330
me-no-dev pushed a commit that referenced this pull request Sep 25, 2024
* DNSServer: fix improper startup code in WiFi mode

When running on WiFi-AP mode server's start() method returned true while in fact UDP listening socket was never created
Regression introduced in #8760
Closes #10330

* ci(pre-commit): Apply automatic fixes

---------

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High 🗻 Issues with high priority which needs to be solved first. Status: Review needed Issue or PR is awaiting review Status: Test needed Issue needs testing
Projects
Development

Successfully merging this pull request may close these issues.

Refactor the network stack to allow chips without WiFi and better Ethernet and ppp compatibility
8 participants