Can anyone help with suggesting how to use the polledTimeout class properly?

in examples

Thanks, I will update the pull request when I get the time.

As requested, polledTimeout implemented.

@devyte It is true saying Stream::readBytes() already implements a timeout, opposed to non-arduino StreamInheritance::read(buf,len) which immediately returns when no more data is available (implemented in HardwareSerial and WiFiClient at least).

There are several locations in the core or our libraries where a retry is done before giving up. All of them do the same thing: move data from a Stream instance to a Print insntance (or local buffer). Fixing that would need to be done all at one by using a proper generic API or function template that allows transferring a Stream instance to a Print instance. I may have a something with no intermediate buffer but it is still an alpha state and I will propose something at some point.

@adrionics you may try with data.setTimeout(streamTimeout) or even better set data's timeout before starting the updater without changing writeStream() signature, and remove the retry.

set data's timeout before starting the updater

that is exactly what I was thinking.

"data" is a WiFiClient and it is using the default timeout. I found that by setting a timeout on data, it seems to make things less reliable. I tried setting a timeout before connecting to the host, but that makes the client wait too long to request a retry (I think?) and so if anything, you need a different timeout before connecting to the host and after connecting(?). Through experimenting, I found it works better to not mess with the WiFiClient's setTimeout() and just wait long enough for it to get data. For sure though, the way this timeout was implemented before was much worse. I found it never is able to complete the update on some devices with low signal strength.

I am of course willing to follow your advice and retry the experiment, so if you have specifics on the setTimeout as related to WiFiClient, I will try again.

@adrionics

Default timeout is 1000ms in Stream.
I don't understand why you would need a shorter timeout for other operations than in updater. Can you elaborate "I found that by setting a timeout on data, it seems to make things less reliable" if you set a timeout bigger ?

If really a bigger timeout is needed in Updater <<this update is really needed give it best luck>>, what about adding a Stream::getTimeout(), saving it in ::writeStream()'s beginning then setting it to twice its value (more? a big constant?), and restoring it in the end of ::writeStream() (still removing the retry) ?

There are two timeouts. A connect timeout and a stream timeout. setTimeout seems to change both though? I would not want a different timeout for other things. I was setting a timeout of 8 seconds - and tried 12 for the WiFiClient before connecting (it's default is 5). I then tried setting timeout on the stream of around 2 or 3 seconds.

Correct me if I am wrong though: The stream timeout is a timeout that after it expires, the stream will retry so if you have too long timeout set on that, it will wait for nothing and not help getting the data. That is why, if the stream timeout is lower, and the read is retried more times it works. Would that be right?

setTimeout seems to change both though?

Yes, ClientContext uses WiFiClient/Stream timeout in ::connect(). And yes WiFiClient overrides timeout to 5000.

The stream timeout is a timeout that after it expires, the stream will retry

Stream itself does not retry. If the delay between two separate receives is greater than timeout, it returns.
If timeout was longer, then there'd be no need to externally retry (like it does now).
That's why I propose to increase timeout in ::writeStream() (and restore it after), or increase your timeout once for all (onto data) to something big enough for the updater Stream/client.

@adrionics What is the status here? did you test based on @d-a-v 's comments?

@adrionics What is the status here? did you test based on @d-a-v 's comments?

Sorry, I did not have time to spend on this and used the modifications as posted already with excellent performance or reliability. I can revisit in the future, but it can be hard to test or verify on thousands of devices as has already been done now. I think it is better than it has ever been in terms of successful updates across the base with the changes described.

@d-a-v does this PR still make sense in view of your Stream:send implementation? Or would a different Stream-ing approach be possible/better?

The stream::send approach should indeed be considered but that could be in another PR.
This PR apparently solves some updater API issues.
I wonder if @PurpleAir is still using it.
(merge conflict is fixed)

Thank you, yes we are.