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

Closed
wants to merge 1 commit into from

Conversation

reiyawea
Copy link

@reiyawea reiyawea commented Mar 14, 2020

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

Fix the bug that only 1 TCP packet can be received. Once client->available() becomes false, the connection is stoppped and further JSON is lost.
Logic is changed so that the connection is closed only on completion of HTTP body transmission or timeout.
@marcelstoer
Copy link
Member

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

reiyawea commented Mar 14, 2020

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

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

@marcelstoer
Copy link
Member

marcelstoer commented Mar 16, 2020

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 esp8266/Arduino#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
Copy link
Member

marcelstoer commented Mar 16, 2020

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 reiyawea closed this Mar 17, 2020
@reiyawea
Copy link
Author

Sorry for closing this PR accidentally.

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.

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

marcelstoer commented Mar 18, 2020

Superseded by #180. Thanks for triggering this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants