-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
WiFiServer - implementation aligned with Arduino.cc. With example. (breaking change) #9028
Conversation
👋 Hello JAndrassy, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
Thanks @JAndrassy for the PR, we will review it soon. Probably in January due to Christmas and New Years Eve vacations. We are trying to find the best approach for Wifi refactoring so hopefully we will end up in some reasonable solutions which will be also acceptable for you. :) It is a bit challenging. |
The server.available() method was what started me to evaluate differences between Arduino networking libraries. Almost two years ago we found a solution for the esp8266 WiFi library: deprecating In August this year I stared to write down all my experience with creating and maintaining Arduino networking libraries, because new libraries appear and have the same mistakes I try to eliminate in old libraries. But writing the Guide let to necessity of defining the networking API by analyzing the significant libraries. I found many often simple differences so I started to do PR in all repositories. Then I wrote test sketches for basic functions and those found more errors in implementations than I would expect (even in my libraries.). So more PR. |
I see, I have noticed you contributed to ESP8266 repository, I wasn`t aware about the contributions to other libraries as well. Good to know, we will check it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the errors shown by the CI
@lucasssvaz this PR fixes it #9029 the test failed on compiler warning
|
|
they stay connected. but it is possible that there is a timeout set in IDF LwIP. the AT firmware has it.
using server.accept(), not server.available(). The server.available() originates in Processing. It is a strange idea to simplify a TCP server for artists. I don't like it. But my PR to deprecate the method name And rejected PR #9028 is related too |
This is not a nice solution. Lingering resources with no way to clear them is not good.
But then the client will not be put in the list and you will not be able to further query it with I do understand very well your point, but I must be sure that this will cause more good than harm if merged :) |
libraries/WiFi/src/WiFiServer.h
Outdated
@@ -24,6 +24,10 @@ | |||
#include "WiFiClient.h" | |||
#include "IPAddress.h" | |||
|
|||
#ifndef SERVER_MAX_MONITORED_CLIENTS | |||
#define SERVER_MAX_MONITORED_CLIENTS 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONFIG_LWIP_MAX_SOCKETS
is 16
, meaning there can be more than 5 monitored clients. What happens with the rest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accept() is only invoked if there is a free position in the array.
I could change it to CONFIG_LWIP_MAX_SOCKETS
I would prefer deprecation. as I already wrote on multiple places, nobody expects what available() does in Arduino libraries. basically developers at Arduino don't know it and do it wrong in new libraries (arduino/uno-r4-wifi-usb-bridge#20, arduino/ArduinoCore-mbed#793) the link to deprecation PR was wrong in my previous comment. it is #8860 |
Ok :) before we can consider this for merging, here are some suggestions:
|
examples are already updated to accept(). that was not reverted from the deprecation PR. this PR adds an example for server.available(). |
now I realized there is a problem. Arduino doc instructs to stop() the connection returned by the server when it is handled. but client.stop() in this library does nothing and the connection is closed from local side only when the last copy of WiFiClient is destroyed. So the copy in WiFiServer will hold the connection. can we please deprecate server.available and be done. nobody will miss it |
so you are good to close this if we deprecate available? |
I already did the deprecation. it was reverted with the argument about "aligned with Arduino.cc" I am fan of accept(). I added it to all libraries by Arduino. some PR are merged, some are at least approved for merge |
I just need the full picture. Nobody has complained about this in the last 10 years, so if we are to break something, there ought to be a good reason. Since |
and #9028 ? |
Maybe i misunderstood the goal of the PR completely wrong. Drop |
the goal of this PR was to show what does it mean to implement proper available() and print-to-all-clients in esp8266 WiFi library the deprecation of server.available() was done two years ago. |
I agreed with that and listed the required changes necessary to make it mergeable. We are on FreeRTOS with multiple cores and threads, so safety is important. Current code will not do. Also some connections are saved and others not is also not a solution. Either all or none. |
@JAndrassy Thx, for clarification. @me-no-dev thats my fear, esp32 is different to all others Arduino supported MCUs. Code can not be always align. |
using CONFIG_LWIP_MAX_SOCKETS for SERVER_MAX_MONITORED_CLIENTS would solve that. semaphore could be done too. but what with I coded the available() and print-to-all-clients enhancement of WiFiServer added in this PR 3 years ago for my TelnetStream library. Now it is in my NetApiHelpers library. It doesn't have to be in the specific WiFi or Ethernet library. |
@JAndrassy if you are willing to implement what is outlined here, then we can discuss merging :) |
58ef04a
to
37b8688
Compare
to migration guide? how to fix the failing check? why does it compile a WiFi example for H2? |
You add a file named
And to the library documentation (if there is such) |
now I see it. I had .* files hidden in Eclipse, while I was looking at the other examples
There isn't. Only WiFi STA and WiFi SoftAP are documented. WiFiClient, WiFiUDP and WiFiServer are not documented in ESP32 Arduino doc |
1f13713
to
1975eb1
Compare
there is still the issue of clients acquired by |
directly accepted clients should not be added to the list, handled with print-to-all-clients and closed in end() (so then again. why not just deprecate I wrote a guide for Arduino networking API |
OK, you made your point :) Let's go with that instead |
Please open a new one with the deprecation |
implements
available()
as documented here https://www.arduino.cc/reference/en/libraries/wifi/server.available/implements print-to-all-clients as documented here https://www.arduino.cc/reference/en/libraries/wifi/server.print/
adds WiFiPagerServer example to demonstrate how server.available() and server.print() work.
Requires #9029