search

ThingPulse / esp8266-weather-station

ESP8266 Weather Station library supporting OpenWeatherMap, Aeris and other sources
MIT License
1.06k stars 359 forks source link

Change logic of reading HTTP response stream #179

Closed reiyawea closed 4 years ago

reiyawea commented 4 years ago

\<Description of and rationale behind this PR> Fix the bug that only one TCP packet can be received. Once client->available() becomes false, the connection is closed and further data is lost. Logic is changed so that the connection is closed only on completion of HTTP body transmission or timeout. Timeout counting logic is also changed so that the timer is reset on receiving data.

marcelstoer commented 4 years ago

Does that fix #178? We strive for the general TCP loop to be in line with what the ESP8266 Arduino core reference suggests: https://arduino-esp8266.readthedocs.io/en/latest/esp8266wifi/client-examples.html#general-client-loop

reiyawea commented 4 years ago

Does that fix #178? We strive for the general TCP loop to be in line with what the ESP8266 Arduino core reference suggests: https://arduino-esp8266.readthedocs.io/en/latest/esp8266wifi/client-examples.html#general-client-loop

Yes, I also suffered from the phenomenon described in #178 . After looking into the forecast data, I realized that the TCP is interrupted by the "client->stop();" within the while loop, leaving "forecasts[]" empty.

I agree that we should thrive to be in line with Arduino reference suggests. Since it didn't mention where to put "client->stop();", I think it reasonable to be out of the while loop.

By the way, while this change fixed #178, it has a chance of ~30% to get timeout during updating forecast data and reboot. Maybe TCP stack ran out of memory and got stuck, or just because of bad network situation. Please help test whether timeout happens that frequenly. Thank you.

reiyawea commented 4 years ago

"client->stop();" is added in 8fc80586281423663ad95f519e95ee21a1214e6f, but inside the while loop. So versions before that work fine.

marcelstoer commented 4 years ago

I agree that we should thrive to be in line with Arduino reference suggests. Since it didn't mention where to put "client->stop();"

True, it's not in their blue print but in the sample sketch a few lines above the blue print: https://arduino-esp8266.readthedocs.io/en/latest/esp8266wifi/client-examples.html#now-to-the-sketch

I think it reasonable to be out of the while loop.

Riiiight... Oh f..., I got lost between the curly braces when I implemented #178 😢Totally my mistake. I am surprised this issue didn't surface during my tests back then and why it took so long until someone else noticed.

The reason why there even is a blueprint in their docs is thanks to my https://github.com/esp8266/Arduino/issues/5257 from 2018. We also discussed why one needs that double while loop and the notion of client.available() vs. client.connected(). To me that's all more complicated than necessary. Intuitively one would argue that "if no more data is available the client might as well close the connection" (-> stopping inside the outer loop).

marcelstoer commented 4 years ago

I sat down and did some testing. Results:

I also tested a few things with cURL against the forecast API. I wouldn't completely rule out odd behavior on the server end but I definitely need to dig deeper. At this point I don't trust that HTTPClient anymore; will try with plain socket.

reiyawea commented 4 years ago

Sorry for closing this PR accidentally.

I sat down and did some testing. Results:

  • I can confirm the results @manj9501 reported in #178 -> with your changes the app crashes.
  • With client->stop() in the outer while loop (i.e. current version on master released as 2.0.0) the connection is cut too early and we loose data.
  • With out client->stop() (anywhere) the client waits for quite some time until the remote host finally closes (I assume). That's why I initially introduced it in #174.

I also tested a few things with cURL against the forecast API. I wouldn't completely rule out odd behavior on the server end but I definitely need to dig deeper. At this point I don't trust that HTTPClient anymore; will try with plain socket.

Thank you for your great efforts. I'm sorry to hear that my change causes app to crash. I digged into HTTPClient and found that it does not use "keep-alive" feature, meaning that server will close the connection as soon as all data are sent. So client should wait only until client.connected() becomes false. Maybe it is a good idea to use socket instead of HTTP library, since it may save memory and run faster I think. I'll wait for your new version.

marcelstoer commented 4 years ago

Superseded by #180. Thanks for triggering this discussion.