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 #180

Closed marcelstoer closed 4 years ago

marcelstoer commented 4 years ago

Use plain TCP communication to fetch & process remote resources

Fixes #178 and superseds #179.

I tested with ESP8266 Arduino Core 2.4.2 and 2.6.3. Assumption: if it works for those two it'll work for anything in between as well.

Don't worry about the failing CI build for now. Once the community confirms the fix to be ok I'll update all other affected pieces of code as well.

reiyawea commented 4 years ago

I tried your new code and it worked! I still met the timeout problem (probability of around one fourth), but I believe that it is due to bad network condition. Tested with ESP8266 Arduino Core 2.6.3.

P.S. Suggest making code like this:

while (client.connected() || client.available()) {
  if (client.available()) {
    ...
    //yield();//TCP/IP is blocked when no data comes. so moved downwards.
  }
  yield();//moved here so that under no circumstances can TCP/IP be blocked.
}
client.stop();
marcelstoer commented 4 years ago

@reiyawea Thanks for testing. It doesn't matter where you place yield() as long as it's inside the TCP while loop. You want to give the firmware a chance to suspend your TCP loop temporarily to feed the watchdog if it has to.

reiyawea commented 4 years ago

Tests made today

under good network condition

To test whether it is due to bad network condition that causes timeout problem, I set up a web server at home, caching the JSON from openweathermap. The URL is also modified pointing to that local server. It turns out that the timeout problem is gone, and the loading process is much faster.

HTTP library

I also rolled back to the previous version which uses HTTP library, but with client->stop(); moved out of while (client->available()) loop. It also worked well. Meanwhile, with client->stop(); inside the loop, then problem of #178 still happens.

Conclusion

  1. Improper position of client->stop(); interrupts the connection when incoming TCP stream pauses, most probably caused by unstable network condition.
  2. For unclear reason, TCP stack gets stuck when network is slow or unstable.
tomte76 commented 4 years ago

Today I changed my CI to build based on the "fix/getting-remote-resources" branch and I can confirm that the "update weather data" problem is gone and the update is much faster then 1.6.6 I used before. I use arduino-cli latest and Arduino esp8266 v2.6.3 for building. Thank you very much.

marcelstoer commented 4 years ago

@tomte76 Thank you Daniel for testing. I will soon be rolling out this change across all affected classes and then push a new release. Stay tuned.