Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change logic of reading HTTP response stream #180

Merged
merged 2 commits into from
Mar 22, 2020

Conversation

marcelstoer
Copy link
Member

@marcelstoer marcelstoer commented Mar 18, 2020

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.

@marcelstoer marcelstoer changed the title [WIP] [DNM] Dump use of HTTPClient [WIP] [DNM] Change logic of reading HTTP response stream Mar 18, 2020
@reiyawea
Copy link

reiyawea commented Mar 19, 2020

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
Copy link
Member Author

marcelstoer commented Mar 19, 2020

@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
Copy link

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
Copy link

tomte76 commented Mar 21, 2020

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
Copy link
Member Author

@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.

@marcelstoer marcelstoer force-pushed the fix/getting-remote-resources branch 3 times, most recently from b94feaa to 91c20c1 Compare March 22, 2020 08:01
Use plain TCP communication to fetch & process remote resources

Fixes #178
@marcelstoer marcelstoer changed the title [WIP] [DNM] Change logic of reading HTTP response stream Change logic of reading HTTP response stream Mar 22, 2020
@marcelstoer marcelstoer merged commit 237442a into master Mar 22, 2020
@marcelstoer marcelstoer deleted the fix/getting-remote-resources branch March 22, 2020 11:40
srg74 added a commit to srg74/esp8266-weather-station that referenced this pull request Mar 23, 2020
Change logic of reading HTTP response stream (ThingPulse#180)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forecasts not updated properly on cold boot and reset.
3 participants