WiFiClient setTimeout is documented to accept milliseconds (sane) but value is used as seconds directly (loss of resolution is not sane). Protoype expects u32 instead of f32/f64.

[emilfihlman]

WiFiClient setTimeout is documented to accept milliseconds (sane) but value is used as seconds directly (loss of resolution is not sane). Protoype expects u32 instead of f32/f64.

Documentation (inheritance) on value being milliseconds, not seconds
https://www.arduino.cc/reference/en/language/functions/communication/stream/streamsettimeout/

/path/to/sketch.ino: In function 'void loop()':
sketch:614:31: error: invalid conversion from 'const char*' to 'uint32_t {aka unsigned int}' [-fpermissive]
connection.setTimeout("asd");
^
In file included from /path/to/arduino/.arduino15/packages/esp32/hardware/esp32/1.0.4/libraries/WiFi/src/WiFi.h:37:0,
from /path/to/sketch.ino:5:
/path/to/arduino/.arduino15/packages/esp32/hardware/esp32/1.0.4/libraries/WiFi/src/WiFiClient.h:91:9: note: initializing argument 1 of 'virtual int WiFiClient::setTimeout(uint32_t)'
int setTimeout(uint32_t seconds);

On Linux dell 4.19.0-6-amd64 #1 SMP Debian 4.19.67-2+deb10u2 (2019-11-11) x86_64 GNU/Linux
Arduino is 1.8.11 Hourly Build 2020/02/05 07:25

Reproduction:
u32 t;
u8 buf[1024];
WiFiClient connection;
connection.connect(server, port);
connection.setTimeout(2);
t=millis();
Serial.print(t);
connection.readBytes(buf, 1024);
//Send say a single character from your server
Serial.print(millis()-t);

[lbernstone]

While the choice to use seconds rather than msec is certainly debatable, WiFiClient is not a stream object, and setTimeout is not even available in the arduino version of the library. This is not a bug, and the header file clearly defines the parameter as seconds.

[MarkusAD]

It's a timeout for cryin' out loud. How accurate does it need to be? Don't waste any more of my memory for your bloody accurate timeout.

[emilfihlman]

@lbernstone alrighty, thanks for clearing it up. In the interest of adhering to common and expected behaviour in the Arduino flavour, I believe it should be changed to milliseconds (or possibly micros, nanos, whatever the best timebase is). Keeping it at a second is both unnecessary and silly, both decreasing resolution and making the lib harded to use, we are talking about an embedded device here afterall.

@markyad that's a pretty toxic, unhelpful and not relevant comment. It accepts an u32, are you expecting to wait for max 136 years? Wouldn't perhaps 50 days be a better max timeout?

[MarkusAD]

@emilfihlman Tempus fugit, time-tripper. If a client times out in 6 seconds instead of 5.6 why would anyone care? Besides, how things work around here doesn't involve sitting back and saying "this doesnt work the way I like, fix it". YOU fix it the way you think it should be and submit it as a pull request and the developers will either accept it or not. Now get busy, there is no core issue here.

[MarkusAD]

@emilfihlman quoting your original post:

sketch:614:31: error: invalid conversion from 'const char*' to 'uint32_t {aka unsigned int}' [-fpermissive]
connection.setTimeout("asd");

You tried passing it a character array? WTF is that idiocy?

Protoype expects u32 instead of f32/f64.

Look through the core header files and spot where those short variable types are used, they aren't. Its uint32_t btw. Why would you want to make it larger still by going to a 64 bit type? I could see making it uint16_t but thats not what you posted. No one said you couldn't pass it a uint8_t or better the way it is, did you even try that?

[MarkusAD]

@markyad that's a pretty toxic, unhelpful and not relevant comment. It accepts an u32, are you expecting to wait for max 136 years? Wouldn't perhaps 50 days be a better max timeout?

What you'll find is that the esp32 is fastest when it has to deal with 4-byte (uint32_t) aligned data types. Not that it matters for a function thats mostly used in setup() but thats why its coded the way it is.

Again, feel free to submit your pull request with why you think a sub-second timeout is needed and let the developers decide. @lbernstone already pointed out that there is no setTimeout() in the arduino library so its not a matter of "arduino flavor". esp32 is not an arduino in case you havent realized that yet, and a subsecond timeout for this routine amounts to unneeded fluff (IMHO).

Awaiting your pull request. Good luck.

[MarkusAD]

Get busy @emilfihlman

[emilfihlman]

@markyad do you not understand how automatic type coercion works or are you how angry now? Can you not figure out that to get the required information easily, you must give the function shit to make the compiler give you the actual datatypes? Not to mention that it sounded so insane to me to expect a second resolution that I offered to accept floats and a mistake in type selection.

And while you might not care, how can you ever make a decision what matters to others? Millisecond resolution is the expected and standard resolution and a departure from that to a smaller resolution is just stupid.

I'm honestly pretty disappointed that you represented this project at all. Please don't waste everyone's time by participating since clearly you are only about being an ass towards others.

[MarkusAD]

I dont represent this project I represent myself. Get to coding your pull request and STFU already.

[lbernstone]

A pull request would need to define a new function for this, as redefining it would have bad consequences for current users.

[emilfihlman]

@lbernstone we could redefine it to accept an f32 (a choice I'm not super thrilled about), which would prevent breaking existing applications. Making a new function is easy enough, though, but still gives a bad taste.

[emilfihlman]

Looking at the code, it's clearly a developer error in using seconds where milliseconds would have been correct. Everywhere else timeouts are in milliseconds, not to mention that WiFiClient write operates on Stream objects (

arduino-esp32/libraries/WiFi/src/WiFiClient.cpp

Line 413 in 109ba7a

size_t WiFiClient::write(Stream &stream)

), so it's even easier to see that it's developer error.

I recommend we simply change timeout to milliseconds as it has always been intended to be, and update any documentation to match. In this instance any "breaking" of existing applications is acceptable, since timeouts a) cannot be used for accurate timekeeping anyhow so client applications must address timing issues anyhow and b) documentation is easy to look up.

I'll try to write up a pull request this weekend but everyone else is of course welcome to do that in my place.

[stale]

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[presslab-us]

I came here trying to understand why setTimeout() was in seconds. It clearly looks like a bug that was 'covered up' so to speak. I'd like the additional resolution because I'm dealing with the default 5 second task watchdog timeout.

It's too bad some of the contributors here have such an attitude, but that's open source for you. I don't really want to be the one to deal with them, but if @emilfihlman were to do a pull request myself and I'm sure others would appreciate it.