- Notifications
You must be signed in to change notification settings - Fork 7.8k
WiFiClients.setConnectionTimeout added#8863
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
Conversation
JAndrassy commented Nov 10, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
862dedf to fb1668eCompareVojtechBartoska commented Nov 28, 2023
Thanks for the PR @JAndrassy. This relates to #6676. @P-R-O-C-H-Y PTAL |
JAndrassy commented Nov 28, 2023
the difference is that #6676 doesn't define |
f5a93b1 to 49ea525Comparegithub-actionsbot commented Dec 13, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
👋 Hello JAndrassy, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
JAndrassy commented Dec 13, 2023
after #6676 was merged WiFiClient.setConnectionTimeut now looks here like a new method, but it just the old setTimeout modified (same way this PR does for WiFiClientSecure) |
me-no-dev commented Dec 13, 2023
Please revert any changes to WiFiClientSecure. It will inherit those methods from WiFiClient. That was the point of the previous PR that we merged. |
Uh oh!
There was an error while loading. Please reload this page.
me-no-dev commented Dec 14, 2023
@P-R-O-C-H-Y PTAL if this makes sense now :) |
P-R-O-C-H-Y left a comment
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.
@me-no-dev@JAndrassy please take a look on my comment, if it makes sense to you as well.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ce0b2a6 to 181e546Compare181e546 to 45abe3dCompareJAndrassy commented Dec 14, 2023
so at the end it is just a simple setter |
P-R-O-C-H-Y left a comment
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.
Just simple like that :) side question: Arduino API seems to be unit16_t, should we align or better will be to go with uint32_t as @JAndrassy did.
@me-no-dev?
TD-er commented Dec 14, 2023
It isn't unthinkable someone wants to set it to >65 seconds. |
JAndrassy commented Dec 14, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
the uint32_t is a leftover from rename from |
TD-er commented Dec 14, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
What would be the interpretation of a negative timeout? |
WiFiClient::setTimeoutnow shadowsStream::setTimeoutwhich has a different purpose and the unit of the parameter is different. Related issue.The solution is to renameWiFiClient::setTimeoutto Arduino standardWiFiClient::setConnectionTimeout.EDIT: at the end most of this PR was done in #6676 and #8998
overview of Client API in Arduino networking libraries:
https://github.com/JAndrassy/Arduino-Networking-API/blob/main/ArduinoNetAPILibs.md#client-getters-and-setters