Inconsistency in setTimeout implementation

[cziter15]

Board / hardware

Doesn't really matter, affect all ESP32 boards

Version

v2.0.3

Description

This is not really a bug to be honest. It's rather small inconsistency that can cause big troubles and might be somehow tweaked in arduino-esp32 to match other implementations of Client.

In development of my hobby IoT framework I've encountered inconsistent behavior between Arduino for ESP32 and ESP8266.

The problem is that everywhere in Arduino framework timeout passed to TCP clients is passed in milliseconds (in Arduino-esp8266 too). However, ESP32's WiFiClient doesn't follow that rule and wants seconds. This is described in header files, but users porting existing code might not notice that fact.

https://www.arduino.cc/reference/en/libraries/ethernet/client.setconnectiontimeout/
https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/src/WiFiClient.h (setTimeout method in base client class)

https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiClient.h (setTimeout method)

[lbernstone]

Unfortunately, this is seriously breaking behavior for people that already have things set for seconds. Perhaps this could be implemented in stages, with a note indicating that the behavior will be deprecated at some point- temporarily put in a change so that numbers over 600 are interpreted as milliseconds, and then anything below as seconds, with a warning that the behavior will change. Then once the change has had time to soak, change over to milliseconds only.

[SuGlider]

@me-no-dev / @VojtechBartoska - I think we could do as @lbernstone suggested starting for the release of Core 2.1.0.
What do you think about it?

[petrkr]

Compared how many warnings are during compilation.. I am afraid nobody will notice just one more warning about deprecation.

[me-no-dev]

Compared how many warnings are during compilation.. I am afraid nobody will notice just one more warning about deprecation.

Warnings should be fixed (unless you mean from other libs or your code).

@SuGlider @lbernstone we can add deprecation notice now (2.0.4) question is what version to target in order to give it enough time. Also will deprecation warnings show in the IDE if no-warnings is selected in preferences?

[lbernstone]

I'd say 6-8 months for notification. It will be quite obvious once it changes. I think most people that actively set it are using timeouts less than 60 seconds, so when every connection fails, I'd expect they read the compile log or go look at the header and see that it has changed to milliseconds...

[petrkr]

Warnings should be fixed (unless you mean from other libs or your code

I agree, but lot of ArduinoIDE peoples are not developers but only glue-coders from stack overflow. And they do not understand to that black window output...

And still I do not take there are lot of warnings from other libs and code and this one will really be hidden in mass output... it should be somehow better visible. But in IDE do not know how...

What about use logger? As other debug messages take severity 'warn' and print it out for few releases by logging subsystem. So if someone will be debug why their code works 'strage' this increased log level will warn them

[lbernstone]

You can't make everybody happy. Noobs are unlikely to be setting a timeout in WiFiClient. It is a breaking change, so it is going to break things at some point, and then people will look to see what is failing. A warning in your compile regarding the piece that is breaking should be pretty clear.

[VojtechBartoska]

this relates to open Pull Request #6676

[VojtechBartoska]

I'm adding this to our Roadmap and also under 2.1.0 milestone.

[EdwardCooke]

When this is implemented will we be able to set a timeout less than 1 second?

[cziter15]

Getting unified into esp8266 direction will give you possibility to set millisecond-precise timeout. This is probably cleanest solution, but will break existing code. There must be some kind of warning emitted (best if done in steps), because function type won't change, so no one will notice the change.

Anyway, there are more differences between esp32 and 8266, like WiFiClient setFingerptint method.

Maybe getting overloaded function, where you have to pass timeval struct reference will make everyone happy? @me-no-dev what is the current state of this issue?

[EdwardCooke]

What about a new method, setTimeoutMs and a warning/error on setTimeout?

[cziter15]

Easiest thing to do with setTimeout is to introduce setTimeoutMs and setTimeoutSec for both platforms and mark setTimeout deprecated, but leave it in code with warning emitted.

Connect method should be also considered.

[supcik]

Units in computer science are not easy to handle (remember Mars Orbiter Crash).

What do you about overriding the method with an argument of type std::chrono::duration? It does not solve the inconsistency, but personally, I would prefer this instead of several methods such as setTimeoutMs and setTimeoutSec.

[cziter15]

Units in computer science are not easy to handle (remember Mars Orbiter Crash).

What do you about overriding the method with an argument of type std::chrono::duration? It does not solve the inconsistency, but personally, I would prefer this instead of several methods such as setTimeoutMs and setTimeoutSec.

Might be problematic, as the direction taken is to avoid std library extensions, but someone from the core team should confirm this.

[VojtechBartoska]

Closing this ticker as solved, mentioned PR have been merged.